-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Fix NPE in Storage with Mass Ingestion and Compartment Interceptor #6598
base: master
Are you sure you want to change the base?
Fix NPE in Storage with Mass Ingestion and Compartment Interceptor #6598
Conversation
…compatibility in mass ingestion mode
Formatting check succeeded! |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #6598 +/- ##
============================================
- Coverage 83.54% 83.49% -0.06%
- Complexity 27432 28552 +1120
============================================
Files 1707 1797 +90
Lines 106185 111119 +4934
Branches 13397 13963 +566
============================================
+ Hits 88710 92776 +4066
- Misses 11750 12358 +608
- Partials 5725 5985 +260 ☔ View full report in Codecov by Sentry. |
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.
minor changes
@@ -2581,7 +2586,8 @@ protected DaoMethodOutcome doUpdateForUpdateOrPatch( | |||
theResource, | |||
theEntity, | |||
theOperationType, | |||
theTransactionDetails); | |||
theTransactionDetails, | |||
theShouldForcePopulateOldResourceForProcessing); |
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.
We're at 11 parameters here.
We probably should've wrapped these in a parameters object 9 parameters ago.
What would be the impact of doing so now?
false); | ||
} | ||
|
||
protected DaoMethodOutcome doUpdateForUpdateOrPatch( |
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.
This multiple method thing is exactly why i think we should make a new parameters object and have that API be the 'main' one.
Multiple overrides and multiple methods will lead to funny bugs if people make methods with different 'combinations' of parameters.
What was done:
Fixed a couple bugs relating how different settings interact with the Patient Compartment Enforcing Interceptor:
ResourceCompartmentUtil#getPatientCompartmentIdentity()
(used by the interceptor) would throw a NPE when the provided resource isnull
. Fixed by adding a null check.null
. Fixed by ensuring resource is not set to null if the interceptor is present.Closes #6599