Skip to content

Commit 40d8a6c

Browse files
authored
Merge pull request #136 from mcode/new-validation-logging
New validation logging
2 parents c9ec376 + 3a3fba2 commit 40d8a6c

File tree

5 files changed

+124
-11
lines changed

5 files changed

+124
-11
lines changed

README.md

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -152,7 +152,9 @@ To mask a property, provide an array of the properties to mask in the `construct
152152

153153
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.
154154

155-
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.
155+
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.
156+
157+
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.
156158

157159
#### CLI From-Date and To-Date (NOT recommended use)
158160

src/client/BaseClient.js

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -96,8 +96,25 @@ class BaseClient {
9696
}
9797
}, Promise.resolve(contextBundle));
9898

99+
// Report detailed validation errors
99100
if (!isValidFHIR(contextBundle)) {
100-
logger.warn(`Extracted bundle is not valid FHIR, the following resources failed validation: ${invalidResourcesFromBundle(contextBundle).join(',')}`);
101+
const invalidResources = invalidResourcesFromBundle(contextBundle);
102+
const baseWarningMessage = 'Extracted bundle is not valid FHIR, the following resources failed validation: ';
103+
104+
const warnMessages = [];
105+
const debugMessages = [];
106+
invalidResources.forEach(({ failureId, errors }) => {
107+
warnMessages.push(`${failureId} at ${errors.map((e) => e.dataPath).join(', ')}`);
108+
109+
errors.forEach((e) => {
110+
debugMessages.push(`${failureId} at ${e.dataPath} - ${e.message}`);
111+
});
112+
});
113+
114+
logger.warn(`${baseWarningMessage}${warnMessages.join(', ')}`);
115+
debugMessages.forEach((m) => {
116+
logger.debug(m);
117+
});
101118
}
102119

103120
return { bundle: contextBundle, extractionErrors };

src/helpers/fhirUtils.js

Lines changed: 17 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -5,9 +5,9 @@ const metaSchema = require('ajv/lib/refs/json-schema-draft-06.json');
55
const schema = require('./schemas/fhir.schema.json');
66
const logger = require('./logger');
77

8-
const ajv = new Ajv({ logger: false });
8+
const ajv = new Ajv({ logger: false, allErrors: true });
99
ajv.addMetaSchema(metaSchema);
10-
const validator = ajv.addSchema(schema, 'FHIR');
10+
const validate = ajv.compile(schema);
1111

1212
// Unit codes and display values fo Vital Signs values
1313
// Code mapping is based on http://hl7.org/fhir/R4/observation-vitalsigns.html
@@ -164,17 +164,29 @@ const logOperationOutcomeInfo = (operationOutcome) => {
164164
};
165165

166166
function isValidFHIR(resource) {
167-
return validator.validate('FHIR', resource);
167+
return validate(resource);
168168
}
169+
170+
function errorFilter(error) {
171+
return error.message !== 'should NOT have additional properties' && error.keyword !== 'oneOf' && error.keyword !== 'const';
172+
}
173+
169174
function invalidResourcesFromBundle(bundle) {
170175
// Bundle is assumed to have all resources in bundle.entry[x].resource
171176
const failingResources = [];
172177
bundle.entry.forEach((e) => {
173178
const { resource } = e;
174179
const { id, resourceType } = resource;
175-
if (!validator.validate('FHIR', resource)) {
180+
181+
// Validate only this resource to get more scoped errors
182+
const subSchema = schema;
183+
subSchema.oneOf = [{ $ref: `#/definitions/${resourceType}` }];
184+
185+
const validateResource = ajv.compile(subSchema);
186+
187+
if (!validateResource(resource)) {
176188
const failureId = `${resourceType}-${id}`;
177-
failingResources.push(failureId);
189+
failingResources.push({ failureId, errors: validateResource.errors.filter(errorFilter) });
178190
}
179191
});
180192
return failingResources;

test/client/BaseClient.test.js

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -76,8 +76,8 @@ describe('BaseClient', () => {
7676
});
7777
it('should iterate over all extractors gets, aggregating resultant entries in a bundle', async () => {
7878
const extractorClassesWithEntryGets = [
79-
class Ext1 { get() { return { entry: [{ resource: 'here' }] }; }},
80-
class Ext2 { get() { return { entry: [{ resource: 'alsoHere' }] }; }},
79+
class Ext1 { get() { return { entry: [{ resource: { resourceType: 'Patient' } }] }; }},
80+
class Ext2 { get() { return { entry: [{ resource: { resourceType: 'Patient' } }] }; }},
8181
class Ext3 { get() { throw new Error('Error'); }},
8282
];
8383
engine.registerExtractors(...extractorClassesWithEntryGets);

test/helpers/fhirUtils.test.js

Lines changed: 84 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -146,6 +146,7 @@ describe('isValidFHIR', () => {
146146
test('Should return true when provided valid FHIR resources', () => {
147147
expect(isValidFHIR(validResource)).toEqual(true);
148148
});
149+
149150
test('Should return false when provided invalid FHIR resources', () => {
150151
expect(isValidFHIR(invalidResource)).toEqual(false);
151152
});
@@ -155,11 +156,92 @@ describe('invalidResourcesFromBundle', () => {
155156
test('Should return an empty array when all resources are valid', () => {
156157
expect(invalidResourcesFromBundle(emptyBundle)).toEqual([]);
157158
});
158-
test('Should return an array of all invalid resources when they exist', () => {
159+
160+
test('Should return an error for each invalid resource', () => {
161+
const secondInvalidResource = {
162+
...invalidResource,
163+
id: 'secondInvalidResource',
164+
};
165+
166+
const invalidBundleWithTwoResources = {
167+
resourceType: 'Bundle',
168+
entry: [
169+
{
170+
resource: invalidResource,
171+
},
172+
{
173+
resource: secondInvalidResource,
174+
},
175+
],
176+
};
177+
178+
const response = invalidResourcesFromBundle(invalidBundleWithTwoResources);
179+
180+
const invalidResourceId = `${invalidResource.resourceType}-${invalidResource.id}`;
181+
const invalidResourceId2 = `${secondInvalidResource.resourceType}-${secondInvalidResource.id}`;
182+
183+
expect(response).toHaveLength(2);
184+
expect(response).toEqual(expect.arrayContaining([
185+
expect.objectContaining({
186+
failureId: invalidResourceId,
187+
}),
188+
expect.objectContaining({
189+
failureId: invalidResourceId2,
190+
}),
191+
]));
192+
});
193+
194+
test('Should return detailed error for invalid resource', () => {
159195
const invalidBundle = { ...bundleWithOneEntry };
160196
invalidBundle.entry[0].resource = invalidResource;
161197
// This is dependent on implementation details intrinsic to invalidResourcesFromBundle
162198
const invalidResourceId = `${invalidResource.resourceType}-${invalidResource.id}`;
163-
expect(invalidResourcesFromBundle(invalidBundle)).toEqual([invalidResourceId]);
199+
expect(invalidResourcesFromBundle(invalidBundle)).toEqual([
200+
{
201+
failureId: invalidResourceId,
202+
errors: [
203+
{
204+
keyword: 'enum',
205+
dataPath: '.gender',
206+
schemaPath: '#/properties/gender/enum',
207+
params: {
208+
allowedValues: [
209+
'male',
210+
'female',
211+
'other',
212+
'unknown',
213+
],
214+
},
215+
message: 'should be equal to one of the allowed values',
216+
},
217+
],
218+
},
219+
]);
220+
});
221+
222+
test('Should return multiple errors if present within the same resource', () => {
223+
// invalidResource already has an invalid gender enum value
224+
const invalidResourceWithTwoProps = {
225+
...invalidResource,
226+
birthDate: 'not-a-real-date',
227+
};
228+
229+
const invalidBundle = {
230+
resourceType: 'Bundle',
231+
entry: [
232+
{
233+
resource: invalidResourceWithTwoProps,
234+
},
235+
],
236+
};
237+
238+
const response = invalidResourcesFromBundle(invalidBundle);
239+
240+
expect(response).toHaveLength(1);
241+
242+
const [invalidResponseObj] = response;
243+
244+
expect(invalidResponseObj.errors).toBeDefined();
245+
expect(invalidResponseObj.errors).toHaveLength(2);
164246
});
165247
});

0 commit comments

Comments
 (0)