Skip to content
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.

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

Support Referenced Resources in Message Parser Schema #1037

Merged
merged 9 commits into from
Jan 12, 2024
Merged

Conversation

bamader
Copy link
Collaborator

@bamader bamader commented Jan 9, 2024

PULL REQUEST

Summary

This PR adds some basic support for the message parser service to accept referenced resources in its parsing schemas. These referenced resources will now use a special DIBBs placeholder token #REF# to denote when a resource must be located elsewhere in the bundle. Validation, unit testing, a new schema, and quality test data has also been developed according to the FHIR spec.

Related Issue

Fixes #1021

Additional Information

Some of the code here is in a temporary location due to us wanting to add a separate extract-phdc-from-fhir endpoint, so don't worry that it and the validation code currently lives within the message parser. This change set is meant to cover the logic rather than the location, so that code will get moved once this lands and we begin work on the final endpoints.

Copy link

codecov bot commented Jan 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (b275b70) 96.93% compared to head (6c67df0) 96.93%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1037   +/-   ##
=======================================
  Coverage   96.93%   96.93%           
=======================================
  Files          48       48           
  Lines        3037     3037           
=======================================
  Hits         2944     2944           
  Misses         93       93           
Flag Coverage Δ
unit-tests 96.93% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Copy link
Collaborator

@DanPaseltiner DanPaseltiner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bamader thanks for getting started on this work so quickly! I like the changes you have introduced to the parsing schema config. I have also identified a few other tickets we'll need eventually, these have been added to our backlog and are not blocking for this PR. There are 2 areas that I still think we need a little work:

  1. See my comment about computing parsers for the reference lookup fhirpaths as part of get_parsers.
  2. The additional unit tests are great, but I don't see any tests or modifications to existing tests that prove we are ensuring backwards compatibility. What happens in one of these new schemas is passed to /parse_message do things work correctly?

containers/message-parser/app/main.py Outdated Show resolved Hide resolved
containers/message-parser/app/main.py Outdated Show resolved Hide resolved
containers/message-parser/app/utils.py Outdated Show resolved Hide resolved
bamader and others added 3 commits January 10, 2024 13:55
@bamader
Copy link
Collaborator Author

bamader commented Jan 10, 2024

@DanPaseltiner Ultimately it seemed like the best way to address your comments was to directly point to the places in my other PR where I'd implemented the logic, but that seemed like it'd be really clunky and ineffective. I ended up merging my other feature branch directly into this one because it was a direct work-off anyway, and I think this now clears things up a bit more effectively. Logic is correctly located, references are all compiled in get_parsers, and we have appropriate sample data, examples, and unit tests to show validation and successful reference schema parsing.

The one big thing to note is that with the fully implementation of the separate fhir-to-phdc endpoint, the parse-message endpoint is now left completely unaltered. I know we talked this morning about how that endpoint might also make use of some of the reference extraction stuff, but for the time being that's fully contained in the fhir to phdc endpoint. The parsing compilation is still present in get_parsers so theoretically the message parser could do the same thing (if we moved the logic in the endpoint up into parse-message), but we had said we wanted to do the development insulated from parse-message in the beginning. Let me know how much, if any, logic you think should be ported backward into parse-message, or we can make that a separate ticket to expand the functionality.

@bamader bamader requested a review from DanPaseltiner January 10, 2024 20:04
Copy link
Collaborator

@DanPaseltiner DanPaseltiner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll finish reviewing later on, but here is one initial comment. Thanks for your continued work on this!

containers/message-parser/app/models.py Outdated Show resolved Hide resolved
Comment on lines 105 to 107
secondary_parsers[secondary_field] = fhirpathpy.compile(
secondary_field_definition["reference_lookup"]
)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking about how this would work in practice and not quite sure I follow. We'll need the parser and the fhir path string to insert the reference into. What if we make the value of secondary_parsers[secondary_field] a dict and set it equal to something like

{
"reference_parser": fhirpathpy.compile(
                        secondary_field_definition["reference_lookup"],
"secondary_fhir_path": FHIRPATH-TO-INSERT-REFERENCE-INTO  
}

This way when we run through our parsers everything we need is available in one data structure.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realize this will break /parse-message in its current form if it receives a schema containing references. We should be able to address this though, potentially in another ticket.

@bamader
Copy link
Collaborator Author

bamader commented Jan 11, 2024

@DanPaseltiner Everything we talked about yesterday during the review should now be in place! Major changes include:

  • Modified get_parsers to create a data structure within the secondary_fields dictionary, where we can hold the pre-compiled reference lookups as well as the FHIR paths we need to insert into
  • Refactoring the repeated code of both endpoints that extracted and applied the get_parsers into its own helper function that both endpoints now call--both are therefore set up to handle reference resources
  • Refactoring the validation that enforces a parsing schema that uses reference fields having all required attributes so that it can be called by both endpoints without duplication
  • Added unit tests to both endpoints to confirm that they can handle reference-based schemas while still maintaining backwards compatibility with parse-message
  • Documentation and comments throughout that indicate where changes might need to be made in the future based on other tickets we've created and on work we intend to do (e.g. make the phdc endpoint return actual phdc)

I think this addresses everything that was outstanding, so feel free to review again when you have the chance!

@bamader bamader requested a review from DanPaseltiner January 11, 2024 19:22
DanPaseltiner
DanPaseltiner previously approved these changes Jan 11, 2024
Copy link
Collaborator

@DanPaseltiner DanPaseltiner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for bearing with me on this as we figured out the right path forward here. 🚀

@bamader bamader added this pull request to the merge queue Jan 12, 2024
Merged via the queue into main with commit 34521fc Jan 12, 2024
34 checks passed
@bamader bamader deleted the support-refs branch January 12, 2024 19:17
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.

Update message parser to handle referenced resources in schema
2 participants