Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Warn when reference values do not resolve and have incorrect format #1468

Merged
merged 7 commits into from
Jun 27, 2024

Conversation

KaelynJefferson
Copy link
Collaborator

This PR includes:

  • Issues a warning when the user provides reference values to entities that do not resolve and have incorrect FHIR Reference format
  • Tests affected were modified

Copy link
Collaborator

@mint-thompson mint-thompson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there may need to be some extra logic to test if the resulting value conforms to the allowed format for a FHIR Reference. Values that look like Type/Id, such as Patient/MyPatient, are allowed. URLs are also allowed. So, these values should not result in a message that says that the reference does not conform to the FHIR Reference format. There are more details in the documentation for Reference.

Copy link
Collaborator

@mint-thompson mint-thompson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the updates. I have a few more changes to request: turns out the Reference type can get a little complicated!

Comment on lines 999 to 1004
// ensure it is not an absolute url
if (
value.reference.indexOf('http://') == -1 &&
value.reference.indexOf('https://') == -1 &&
value.reference.indexOf('www.') == -1
) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For checking if the value of the reference is an absolute url, you should be able to use the valid-url package. It is used in a few places in SUSHI for this purpose.

Comment on lines 995 to 998
if (
value.reference.includes('/') &&
value.reference.indexOf('/') !== value.reference.lastIndexOf('/')
) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on an example in the FHIR documentation, I think that a value with more than one / might actually be acceptable? The last example in the Literal References section shows Observation/1x2/_history/2 as a version-specific reference value. So maybe, only a value without any / would be one that we warn about? I'd like to get opinions from the rest of the team on this.

mint-thompson
mint-thompson previously approved these changes Jun 18, 2024
Copy link
Collaborator

@mint-thompson mint-thompson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks fine to me! Since so many formats are allowed, we don't even need to check if something is a URL (since those will contain a /, and therefore not emit a warning). Thank you for making all the requested updates!

Copy link
Collaborator

@jafeltra jafeltra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking good. I found one case where we want to avoid logging this warning message and left a few other nitpicky comments. Let me know if you disagree with any of the nitpicky ones.

src/fhirtypes/common.ts Outdated Show resolved Hide resolved
src/fhirtypes/common.ts Outdated Show resolved Hide resolved
else if (type == null && id == null && value.reference) {
// if reference does not have type / id reference format
// example: Patient-oops
if (!value.reference.includes('/')) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ran a regression on this branch and noticed there is actually one format that does not include a / but is allowed. You can have a reference to a UUID or OID, which would look like urn:uuid:some-uuid or urn:oid:some-oid. For simplicity and to make sure we catch all the possible cases, we should probably just check if the value starts with urn:. If it does, we should not log the warning.

src/fhirtypes/common.ts Outdated Show resolved Hide resolved
src/fhirtypes/common.ts Outdated Show resolved Hide resolved
src/fhirtypes/common.ts Show resolved Hide resolved
Copy link
Collaborator

@jafeltra jafeltra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good! Thanks for adding that new case and adding a test! I just had one small comment inline.

There are still 24 repos that get new warnings, but from looking through the warning messages, it looks like they're all relevant warnings, so I think that's okay. In case anyone else is curious, this is the list of repos that I see with warnings:

davidhay25/models#main
hl7-be/referral#master
ansforge/IG-fhir-telesurveillance#main
BIH-CEI/interagent#main
cander2/epi-test#master
CEOsys/cpg-on-ebm-on-fhir#master
hl7-be/lab#master
hl7-eu/gravitate-health#master
HL7/codex-radiation-therapy#master
HL7/cqf-recommendations#master
HL7/ReportIIITraveler-ig#master
HL7NZ/nes-ig#master
IHE/PCC.mAPS#master
IHE/PCC.mAPS.us#master
InstitutNationalduCancer/ImplementationGuide_OsirisFHIR#master
Interop-Sante/hl7.fhir.fr.core#main
joofio/ufis-ig#main
joofio/ufis-swe-ig#main
medizininformatik-initiative/kerndatensatz-bildgebung#main
medizininformatik-initiative/kerndatensatzmodul-medikation#master
medizininformatik-initiative/kerndatensatzmodul-onkologie#dev
servicewell/wof-portal-fhir-ig#master
swantrace/ASU-research-consent-on-FHIR#main
tewhatuora/fhir-rheumatic-fever#master

Comment on lines 984 to 985
if (!value.reference.startsWith('urn:')) {
if (!value.reference.includes('/')) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The way this logic is written with the nested if statements makes me think there's something else we want to do if the value starts with urn: but does contain a /. I think it would be simpler to just have one if statement with both conditions combined with "and" (i.e. !value.reference.startsWith('urn:') && !value.reference.includes('/')). While that doesn't change the logic as it currently is, I think it makes the intent a little clearer.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or there is, of course, the equivalent inverse expression: !(value.reference.startsWith('urn:') || value.reference.includes('/')). Take your pick!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

De Morgan would be proud of this comment.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh right. That's his name!

@jafeltra
Copy link
Collaborator

I also noticed we still have #968 open. I think this PR addresses that issue, and we should be able to close it when this gets merged.

Copy link
Collaborator

@jafeltra jafeltra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Thanks for making those updates!

@mint-thompson mint-thompson merged commit 34105af into master Jun 27, 2024
14 checks passed
@mint-thompson mint-thompson deleted the correct-format-reference branch June 27, 2024 18:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants