Skip to content

Added fn to find all invalid resources in a bundle #104

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.

Sign up for GitHub

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

Merged
Dtphelan1 merged 3 commits into from
Apr 14, 2021
Merged

Added fn to find all invalid resources in a bundle #104

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8bccace
Added fn to find all invalid resources in a bundle
Dtphelan1 Apr 12, 2021
65a7fd0
error message is now a warning
Dtphelan1 Apr 12, 2021
b3aaff8
fixing lint and cross-test errs
Dtphelan1 Apr 12, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/client/BaseClient.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
const { isValidFHIR } = require('../helpers/fhirUtils');
const { isValidFHIR, invalidResourcesFromBundle } = require('../helpers/fhirUtils');
const logger = require('../helpers/logger');

class BaseClient {
Expand Down Expand Up @@ -97,7 +97,7 @@ class BaseClient {
}, Promise.resolve(contextBundle));

if (!isValidFHIR(contextBundle)) {
logger.error('Extracted bundle is not valid FHIR.');
logger.warn(`Extracted bundle is not valid FHIR, the following resources failed validation: ${invalidResourcesFromBundle(contextBundle).join(',')}`);
}

return { bundle: contextBundle, extractionErrors };
Expand Down
19 changes: 17 additions & 2 deletions src/helpers/fhirUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,8 +163,22 @@ const logOperationOutcomeInfo = (operationOutcome) => {
});
};

const isValidFHIR = (resource) => validator.validate('FHIR', resource);

function isValidFHIR(resource) {
return validator.validate('FHIR', resource);
}
function invalidResourcesFromBundle(bundle) {
// Bundle is assumed to have all resources in bundle.entry[x].resource
const failingResources = [];
bundle.entry.forEach((e) => {
const { resource } = e;
const { id, resourceType } = resource;
if (!validator.validate('FHIR', resource)) {
const failureId = `${resourceType}-${id}`;
Copy link
Contributor

@dmendelowitz dmendelowitz Apr 13, 2021

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

@Dtphelan1 Dtphelan1 Apr 13, 2021

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

@dmendelowitz dmendelowitz Apr 13, 2021

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

@dmendelowitz dmendelowitz Apr 13, 2021

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

@dmendelowitz dmendelowitz Apr 13, 2021

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

failingResources.push(failureId);
}
});
return failingResources;
}

module.exports = {
allResourcesInBundle,
Expand All @@ -182,4 +196,5 @@ module.exports = {
mapFHIRVersions,
quantityCodeToUnitLookup,
isValidFHIR,
invalidResourcesFromBundle,
};
4 changes: 2 additions & 2 deletions test/client/BaseClient.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,8 +76,8 @@ describe('BaseClient', () => {
});
it('should iterate over all extractors gets, aggregating resultant entries in a bundle', async () => {
const extractorClassesWithEntryGets = [
class Ext1 { get() { return { entry: [{ data: 'here' }] }; }},
class Ext2 { get() { return { entry: [{ data: 'alsoHere' }] }; }},
class Ext1 { get() { return { entry: [{ resource: 'here' }] }; }},
class Ext2 { get() { return { entry: [{ resource: 'alsoHere' }] }; }},
class Ext3 { get() { throw new Error('Error'); }},
];
engine.registerExtractors(...extractorClassesWithEntryGets);
Expand Down
26 changes: 26 additions & 0 deletions test/helpers/fhirUtils.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,17 @@ const {
allResourcesInBundle,
quantityCodeToUnitLookup,
getResourceCountInBundle,
isValidFHIR,
invalidResourcesFromBundle,
} = require('../../src/helpers/fhirUtils.js');
const emptyBundle = require('./fixtures/emptyBundle.json');
const bundleWithOneEntry = require('./fixtures/searchsetBundleWithOneEntry.json');
const bundleWithMultipleEntries = require('./fixtures/searchsetBundleWithMultipleEntries.json');
const countBundle5Unique = require('./fixtures/count-bundle-5-unique.json');
const countBundle5Same = require('./fixtures/count-bundle-5-same.json');
const countBundle5Nested = require('./fixtures/count-bundle-5-nested.json');
const validResource = require('./fixtures/valid-resource');
const invalidResource = require('./fixtures/invalid-resource');

describe('getQuantityUnit', () => {
test('Should return unit text if provided in lookup table', () => {
Expand Down Expand Up @@ -137,3 +141,25 @@ describe('getResourceCountInBundle', () => {
expect(getResourceCountInBundle(countBundle5Nested)).toEqual(counts);
});
});

describe('isValidFHIR', () => {
test('Should return true when provided valid FHIR resources', () => {
expect(isValidFHIR(validResource)).toEqual(true);
});
test('Should return false when provided invalid FHIR resources', () => {
expect(isValidFHIR(invalidResource)).toEqual(false);
});
});

describe('invalidResourcesFromBundle', () => {
test('Should return an empty array when all resources are valid', () => {
expect(invalidResourcesFromBundle(emptyBundle)).toEqual([]);
});
test('Should return an array of all invalid resources when they exist', () => {
const invalidBundle = { ...bundleWithOneEntry };
invalidBundle.entry[0].resource = invalidResource;
// This is dependent on implementation details intrinsic to invalidResourcesFromBundle
const invalidResourceId = `${invalidResource.resourceType}-${invalidResource.id}`;
expect(invalidResourcesFromBundle(invalidBundle)).toEqual([invalidResourceId]);
});
});
14 changes: 14 additions & 0 deletions test/helpers/fixtures/invalid-resource.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
{
"resourceType": "Patient",
"id": "invalid-patient",
"name": [
{
"family": "Godel",
"given": [
"Kurt"
]
}
],
"gender": "invalid-enum-value",
"birthDate": "1906-04-28"
}
14 changes: 14 additions & 0 deletions test/helpers/fixtures/valid-resource.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
{
"resourceType": "Patient",
"id": "valid-patient",
"name": [
{
"family": "Frege",
"given": [
"Gottlob"
]
}
],
"gender": "male",
"birthDate": "1848-11-08"
}