Skip to content

Conversation

@mgramigna
Copy link
Contributor

Summary

This pull requests adds more detailed log information when a resource within the extracted bundle fails validation.

It introduces use of the compile function in AJV to generate a validator object that comes with errors we can filter and display in a more detailed manner.

We also now use the subSchema motif to pul off refs to specific resources in the schema that we wish to validate from within the loop, allowing us to collect all the error messages present for that resource rather than short circuit after one.

New behavior

New log messages to the console when a resource fails validation. E.g.:

# Warning level
15:20:21.41 [warn]:     Extracted bundle is not valid FHIR, the following resources failed validation: Patient-789 at .gender
# Debug level
15:20:21.41 [warn]:     Extracted bundle is not valid FHIR, the following resources failed validation: Patient-789 at .gender
15:20:21.41 [debug]:    Patient-789 at .gender - should be equal to one of the allowed values

The default behavior now logs the resource ID and any attributes on that resource that failed validation. When debug is enabled, the specific error message will be displayed too for every error.

Code changes

  • Modify fhirUtils to use ajv compile and the sub schema
  • Update BaseClient to join all errors and resources failing during log warn and debug statements
  • Update some test fixtures to adhere to new validation behavior

Testing guidance

To cause some validation errors, I would recommend making some changes in our templates. Some test cases that I tried included changing properties that were supposed to be dates and making them just strings or booleans or something, and also changing attributes that are enums (e.g. gender) to have a value that doesn't exist in that enum.

Feel free to experiment with other cases that you might expect to fail validation smartly.

Run without debug to see just the basic warning message that lists the resources that are failing and where they are failing. Run with debug to see the detailed error messages.

@jafeltra
Copy link
Contributor

How do you feel about updating the README section that talks about date filtering here? I know it doesn't related to this PR specifically, but I want to reflect the conversation we had at stand up earlier this week. In the "Extraction Date Range" section, I updated the second line and added a third to result in the section reading like this:

### Extraction Date Range

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

Let me know if you think this is overstepping this PR or if you don't like how this reads.

@dmendelowitz dmendelowitz self-assigned this Jun 28, 2021
@jafeltra jafeltra self-assigned this Jun 28, 2021
Copy link
Contributor

@jafeltra jafeltra 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! I think the warning/debug messages make sense. I had two really small comments inline. Let me know what you think.

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.

I agree with Julia's comments. but this looks good to me otherwise!

@mgramigna mgramigna force-pushed the new-validation-logging branch from 25b507d to c26a86c Compare June 28, 2021 15:58
@mgramigna
Copy link
Contributor Author

Added some test cases!

Copy link
Contributor

@jafeltra jafeltra 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!

@dmendelowitz dmendelowitz merged commit 40d8a6c into develop Jun 28, 2021
@dmendelowitz dmendelowitz deleted the new-validation-logging branch June 28, 2021 17:37
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