Skip to content

Conversation

@Dtphelan1
Copy link
Contributor

Summary

Adds the invalidResourcesFromBundle function to be used by the baseClient and other utilities that desire resource-level information on invalid bundles. Updates the baseClient to use this new function. Adds some basic tests for this fn and for the general isValidFhir function. Updates all FHIR-validation related logger.error messages to logger.warn, since the clients shouldn't fail on account of invalid FHIR.

To test this, an easy change to templates that renders the resulting FHIR invalid is to fix the gender value in PatientTemplate to an unknown enum value – gender: invalid-gender would work, for example. This should render all patient resources invalid and you should see as much when you run the baseclient in the form of a warning.

const { resource } = e;
const { id, resourceType } = resource;
if (!validator.validate('FHIR', resource)) {
const failureId = `${resourceType}-${id}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you considered the case where resourceType or id were somehow the areas causing the invalid FHIR? Is that a case we even need to worry about?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a very good question. I thought about it a bit – in fact, an easy way to cause a resource to be invalid is to remove the resourceType from the Template generator.

For ResourceType, I believe we are justified in assuming it will be there; those are almost always statically defined at the level of the template generator.

For id, I believe we will always have some id information. However, I do think we may have more trouble in connecting that information back to a CSV row. That ID won't always correspond to something obvious in the source CSV data – thinking here of the MRN masking on Patient.id. That said, I couldn't come up with another alternative that is a) consistently available across all resource types, and 2) plays some sort of uniquely identifying role.

Is there another property you'd suggest here? Would you imagine removing it all-together as an alternative? Curious if you had thoughts @dmendelowitz 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

For example, if I go in and mess things up by making both resourceType and id have the value null, I just get Extracted bundle is not valid FHIR, the following resources failed validation: null-null as the warning, which wouldn't help me find where the invalid FHIR is, but maybe that's just the best you can do in that case.

Copy link
Contributor

Choose a reason for hiding this comment

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

Now that you mention that resourceType is statically defined, I don't feel as worried about it, since at least the user can narrow it down to once specific resource

Copy link
Contributor

Choose a reason for hiding this comment

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

If anything, if the ID looks messed up in the invalid FHIR warning, they'll know exactly what went wrong

Copy link
Contributor

@dmendelowitz dmendelowitz 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 to me. After thinking it through, I think the use of the ID in the invalid FHIR warning is fine. Even if it doesn't always connect back to the CSV (like in the case where MRN is masked), it's still very useful in the cases where you can connect it back to the CSV, so it's worth keeping.

@julianxcarter julianxcarter self-assigned this Apr 13, 2021
Copy link
Contributor

@julianxcarter julianxcarter left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@Dtphelan1 Dtphelan1 force-pushed the fhir-validation-errors branch from 0f8469f to b3aaff8 Compare April 13, 2021 23:21
@Dtphelan1 Dtphelan1 changed the base branch from main to develop April 14, 2021 13:40
@Dtphelan1 Dtphelan1 merged commit 4abb003 into develop Apr 14, 2021
@Dtphelan1 Dtphelan1 deleted the fhir-validation-errors branch April 14, 2021 13:47
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