From 4d409c7dc24b16bb0d7c2fb8f387de5c94dcb88f Mon Sep 17 00:00:00 2001 From: Matthew Gramigna Date: Fri, 25 Jun 2021 15:15:07 -0400 Subject: [PATCH 1/5] Add more granular resource-level validation and logging --- src/client/BaseClient.js | 19 ++++++++++++++++++- src/helpers/fhirUtils.js | 20 ++++++++++++++++---- test/client/BaseClient.test.js | 4 ++-- test/helpers/fhirUtils.test.js | 22 +++++++++++++++++++++- 4 files changed, 57 insertions(+), 8 deletions(-) diff --git a/src/client/BaseClient.js b/src/client/BaseClient.js index feaa6940..6d095319 100644 --- a/src/client/BaseClient.js +++ b/src/client/BaseClient.js @@ -96,8 +96,25 @@ class BaseClient { } }, Promise.resolve(contextBundle)); + // Report detailed validation errors if (!isValidFHIR(contextBundle)) { - logger.warn(`Extracted bundle is not valid FHIR, the following resources failed validation: ${invalidResourcesFromBundle(contextBundle).join(',')}`); + const invalidResources = invalidResourcesFromBundle(contextBundle); + const baseWarningMessage = 'Extracted bundle is not valid FHIR, the following resources failed validation: '; + + const warnMessages = []; + const debugMessages = []; + invalidResources.forEach(({ failureId, errors }) => { + warnMessages.push(`${failureId} at ${errors.map((e) => e.dataPath).join(',')}`); + + errors.forEach((e) => { + debugMessages.push(`${failureId} at ${e.dataPath} - ${e.message}`); + }); + }); + + logger.warn(`${baseWarningMessage}${warnMessages.join(', ')}`); + debugMessages.forEach((m) => { + logger.debug(m); + }); } return { bundle: contextBundle, extractionErrors }; diff --git a/src/helpers/fhirUtils.js b/src/helpers/fhirUtils.js index e98969cc..51056b6b 100644 --- a/src/helpers/fhirUtils.js +++ b/src/helpers/fhirUtils.js @@ -7,7 +7,7 @@ const logger = require('./logger'); const ajv = new Ajv({ logger: false }); ajv.addMetaSchema(metaSchema); -const validator = ajv.addSchema(schema, 'FHIR'); +const validate = ajv.compile(schema); // Unit codes and display values fo Vital Signs values // Code mapping is based on http://hl7.org/fhir/R4/observation-vitalsigns.html @@ -164,17 +164,29 @@ const logOperationOutcomeInfo = (operationOutcome) => { }; function isValidFHIR(resource) { - return validator.validate('FHIR', resource); + return validate(resource); } + +function errorFilter(error) { + return error.message !== 'should NOT have additional properties' && error.keyword !== 'oneOf' && error.keyword !== 'const'; +} + 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)) { + + // Validate only this resource to get more scoped errors + const subSchema = schema; + subSchema.oneOf = [{ $ref: `#/definitions/${resourceType}` }]; + + const validateResource = ajv.compile(subSchema); + + if (!validateResource(resource)) { const failureId = `${resourceType}-${id}`; - failingResources.push(failureId); + failingResources.push({ failureId, errors: validateResource.errors.filter(errorFilter) }); } }); return failingResources; diff --git a/test/client/BaseClient.test.js b/test/client/BaseClient.test.js index a77de7f9..6d316ac2 100644 --- a/test/client/BaseClient.test.js +++ b/test/client/BaseClient.test.js @@ -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: [{ resource: 'here' }] }; }}, - class Ext2 { get() { return { entry: [{ resource: 'alsoHere' }] }; }}, + class Ext1 { get() { return { entry: [{ resource: { resourceType: 'Patient' } }] }; }}, + class Ext2 { get() { return { entry: [{ resource: { resourceType: 'Patient' } }] }; }}, class Ext3 { get() { throw new Error('Error'); }}, ]; engine.registerExtractors(...extractorClassesWithEntryGets); diff --git a/test/helpers/fhirUtils.test.js b/test/helpers/fhirUtils.test.js index 7b8b9425..5b813eef 100644 --- a/test/helpers/fhirUtils.test.js +++ b/test/helpers/fhirUtils.test.js @@ -160,6 +160,26 @@ describe('invalidResourcesFromBundle', () => { 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]); + expect(invalidResourcesFromBundle(invalidBundle)).toEqual([ + { + failureId: invalidResourceId, + errors: [ + { + keyword: 'enum', + dataPath: '.gender', + schemaPath: '#/properties/gender/enum', + params: { + allowedValues: [ + 'male', + 'female', + 'other', + 'unknown', + ], + }, + message: 'should be equal to one of the allowed values', + }, + ], + }, + ]); }); }); From c26a86c8a96c950e4fecb40a1c9a234a20352824 Mon Sep 17 00:00:00 2001 From: Matthew Gramigna Date: Mon, 28 Jun 2021 09:29:13 -0400 Subject: [PATCH 2/5] Add back allErrors flag :( --- src/helpers/fhirUtils.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/helpers/fhirUtils.js b/src/helpers/fhirUtils.js index 51056b6b..c34e7999 100644 --- a/src/helpers/fhirUtils.js +++ b/src/helpers/fhirUtils.js @@ -5,7 +5,7 @@ const metaSchema = require('ajv/lib/refs/json-schema-draft-06.json'); const schema = require('./schemas/fhir.schema.json'); const logger = require('./logger'); -const ajv = new Ajv({ logger: false }); +const ajv = new Ajv({ logger: false, allErrors: true }); ajv.addMetaSchema(metaSchema); const validate = ajv.compile(schema); From 510c8b53ccc6baddef229981191c632cb73f3160 Mon Sep 17 00:00:00 2001 From: Matthew Gramigna Date: Mon, 28 Jun 2021 11:59:00 -0400 Subject: [PATCH 3/5] Add space to comma join Co-authored-by: Julia Afeltra <30803904+jafeltra@users.noreply.github.com> --- src/client/BaseClient.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/client/BaseClient.js b/src/client/BaseClient.js index 6d095319..9f20faf6 100644 --- a/src/client/BaseClient.js +++ b/src/client/BaseClient.js @@ -104,7 +104,7 @@ class BaseClient { const warnMessages = []; const debugMessages = []; invalidResources.forEach(({ failureId, errors }) => { - warnMessages.push(`${failureId} at ${errors.map((e) => e.dataPath).join(',')}`); + warnMessages.push(`${failureId} at ${errors.map((e) => e.dataPath).join(', ')}`); errors.forEach((e) => { debugMessages.push(`${failureId} at ${e.dataPath} - ${e.message}`); From 7ece0858b365c8b04c4ae36d0c2a4a46589e1b68 Mon Sep 17 00:00:00 2001 From: Matthew Gramigna Date: Mon, 28 Jun 2021 13:15:17 -0400 Subject: [PATCH 4/5] Add more tests for error cases --- test/helpers/fhirUtils.test.js | 64 +++++++++++++++++++++++++++++++++- 1 file changed, 63 insertions(+), 1 deletion(-) diff --git a/test/helpers/fhirUtils.test.js b/test/helpers/fhirUtils.test.js index 5b813eef..8a6d9786 100644 --- a/test/helpers/fhirUtils.test.js +++ b/test/helpers/fhirUtils.test.js @@ -146,6 +146,7 @@ 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); }); @@ -155,7 +156,42 @@ 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', () => { + + test('Should return an error for each invalid resource', () => { + const secondInvalidResource = { + ...invalidResource, + id: 'secondInvalidResource', + }; + + const invalidBundleWithTwoResources = { + resourceType: 'Bundle', + entry: [ + { + resource: invalidResource, + }, + { + resource: secondInvalidResource, + }, + ], + }; + + const response = invalidResourcesFromBundle(invalidBundleWithTwoResources); + + const invalidResourceId = `${invalidResource.resourceType}-${invalidResource.id}`; + const invalidResourceId2 = `${secondInvalidResource.resourceType}-${secondInvalidResource.id}`; + + expect(response).toHaveLength(2); + expect(response).toEqual(expect.arrayContaining([ + expect.objectContaining({ + failureId: invalidResourceId, + }), + expect.objectContaining({ + failureId: invalidResourceId2, + }), + ])); + }); + + test('Should return detailed error for invalid resource', () => { const invalidBundle = { ...bundleWithOneEntry }; invalidBundle.entry[0].resource = invalidResource; // This is dependent on implementation details intrinsic to invalidResourcesFromBundle @@ -182,4 +218,30 @@ describe('invalidResourcesFromBundle', () => { }, ]); }); + + test('Should return multiple errors if present within the same resource', () => { + // invalidResource already has an invalid gender enum value + const invalidResourceWithTwoProps = { + ...invalidResource, + birthDate: 'not-a-real-date', + }; + + const invalidBundle = { + resourceType: 'Bundle', + entry: [ + { + resource: invalidResourceWithTwoProps, + }, + ], + }; + + const response = invalidResourcesFromBundle(invalidBundle); + + expect(response).toHaveLength(1); + + const [invalidResponseObj] = response; + + expect(invalidResponseObj.errors).toBeDefined(); + expect(invalidResponseObj.errors).toHaveLength(2); + }); }); From 3a3fba2e62be2a3bf1d3e25f5d25f17635e900c8 Mon Sep 17 00:00:00 2001 From: Matthew Gramigna Date: Mon, 28 Jun 2021 13:28:24 -0400 Subject: [PATCH 5/5] Update README with date info --- README.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index 95576393..d37b0063 100644 --- a/README.md +++ b/README.md @@ -152,7 +152,9 @@ To mask a property, provide an array of the properties to mask in the `construct The mCODE Extraction Client will extract all data that is provided in the CSV files by default, regardless of any dates associated with each row of data. It is recommended that any required date filtering is performed outside of the scope of this client. -If for any reason a user is required to specify a date range to be extracted through this client, users _must_ add a `dateRecorded` column in every data CSV file. This column will indicate when each row of data was added to the CSV file. Note that this date _does not_ correspond to any date associated with the data element. +If for any reason a user is required to specify a date range to be extracted through this client, users _must_ add a `dateRecorded` column in every relevant data CSV file. This column will indicate when each row of data was added to the CSV file. Note that this date _does not_ correspond to any date associated with the data element. + +Note that some resources should always be included and should not be filtered out with a `dateRecorded` column and date. For example, every extraction should extract patient information to a Patient resource, so no `dateRecorded` column should be provided in a CSV that contains the Patient information. #### CLI From-Date and To-Date (NOT recommended use)