Merge pull request #136 from mcode/new-validation-logging

New validation logging

Author: dmendelowitz

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)
 

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 };

src/helpers/fhirUtils.js

@@ -5,9 +5,9 @@ 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 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;

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);

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,11 +156,92 @@ 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
     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',
+          },
+        ],
+      },
+    ]);
+  });
+
+  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);
   });
 });