-
Couldn't load subscription status.
- Fork 117
Bug fix to prevent an issue mapping a single FHIR resource from crash… #1398
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
base: master
Are you sure you want to change the base?
Conversation
…ing the whole pipeline.
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @rgeimer for this change and sorry for the late review. Can you please add a version of the resource that fails the conversion as a unit-test case? Also please sign the Google CLA.
| @Nullable GenericRecord convertToAvro(Resource resource) throws ProfileException { | ||
| AvroConverter converter = getConverter(resource.getResourceType().name()); | ||
| // TODO: Check why Bunsen returns IndexedRecord instead of GenericRecord. | ||
| GenericRecord gr = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make sure that you run mvn clean package and include the formatting changes after that in the PR. There are some style issues in these lines which should be fixed by that.
| gr = (GenericRecord) converter.resourceToAvro(resource); | ||
| } catch (RuntimeException e) { | ||
| String errorMsg = "Unable to process resource " + resource.getResourceType().name() + "/" + resource.getId() + ": " + e.toString(); | ||
| log.error(errorMsg); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| log.error(errorMsg); | |
| log.error(errorMsg, e); |
| } | ||
| */ | ||
|
|
||
| // Catch any RuntimeException and log the error instead of crashing the whole pipeline for an issue with one FHIR resource. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please move this comment inside the function; this does not really document what the function does.
| return converterMap.get(resourceType); | ||
| } | ||
|
|
||
| /* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove this older version.
Description of what I changed
Changed the convertToAvro() method in AvroConversionUtil.java to catch any RuntimeException errors while processing a given FHIR resource and logging the error instead of allowing a single mapping issue to crash the entire pipeline.
E2E test
TESTED:
I posted an eICR FHIR document (converted to a transaction Bundle) to my FHIR server and re-ran the pipeline. Before my change this crashed the pipeline. After my change it ran successfully and logged an error in a single FHIR resource that included dataAbsentReason extensions, which seem to not be mapped currently.
Checklist: I completed these to help reviewers :)
[X ] I have read and will follow the review process.
I am familiar with Google Style Guides for the language I have coded in.
No? Please take some time and review Java and Python style guides.
My IDE is configured to follow the Google code styles.
No? Unsure? -> configure your IDE.
I have added tests to cover my changes. (If you refactored existing code that was well tested you do not have to add tests)
I ran
mvn clean packageright before creating this pull request and added all formatting changes to my commit.All new and existing tests passed.
My pull request is based on the latest changes of the master branch.
No? Unsure? -> execute command
git pull --rebase upstream master