-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Feature - Permit Bulk Export on Compartment by FHIR Query #7310
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: rel_8_6
Are you sure you want to change the base?
Conversation
|
Formatting check succeeded! |
- add message.properties
add unit tests
… updated some tests
… updated some tests
…xport-permissions
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## rel_8_6 #7310 +/- ##
==========================================
Coverage ? 83.78%
Complexity ? 30207
==========================================
Files ? 1836
Lines ? 116708
Branches ? 14811
==========================================
Hits ? 97786
Misses ? 12617
Partials ? 6305 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/interceptor/auth/RuleBuilder.java
Show resolved
Hide resolved
.../ca/uhn/fhir/rest/server/interceptor/auth/RulePatientBulkExportByCompartmentMatcherImpl.java
Outdated
Show resolved
Hide resolved
.../ca/uhn/fhir/rest/server/interceptor/auth/RulePatientBulkExportByCompartmentMatcherImpl.java
Outdated
Show resolved
Hide resolved
.../ca/uhn/fhir/rest/server/interceptor/auth/RulePatientBulkExportByCompartmentMatcherImpl.java
Show resolved
Hide resolved
hapi-fhir-server/src/test/java/ca/uhn/fhir/rest/server/interceptor/auth/RuleBuilderTest.java
Outdated
Show resolved
Hide resolved
...a/uhn/fhir/rest/server/interceptor/auth/RuleGroupBulkExportByCompartmentMatcherImplTest.java
Outdated
Show resolved
Hide resolved
...a/uhn/fhir/rest/server/interceptor/auth/RuleGroupBulkExportByCompartmentMatcherImplTest.java
Outdated
Show resolved
Hide resolved
...a/uhn/fhir/rest/server/interceptor/auth/RuleGroupBulkExportByCompartmentMatcherImplTest.java
Show resolved
Hide resolved
hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/interceptor/AuthResourceResolver.java
Outdated
Show resolved
Hide resolved
hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/interceptor/AuthResourceResolver.java
Show resolved
Hide resolved
hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/interceptor/AuthResourceResolver.java
Show resolved
Hide resolved
hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/interceptor/auth/BaseRule.java
Outdated
Show resolved
Hide resolved
...hir-server/src/main/java/ca/uhn/fhir/rest/server/interceptor/auth/IAuthResourceResolver.java
Outdated
Show resolved
Hide resolved
...hir-server/src/main/java/ca/uhn/fhir/rest/server/interceptor/auth/IAuthResourceResolver.java
Show resolved
Hide resolved
...a/ca/uhn/fhir/rest/server/interceptor/auth/IAuthRuleBuilderRulePatientMatcherBulkExport.java
Outdated
Show resolved
Hide resolved
hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/interceptor/auth/RuleBulkExportImpl.java
Show resolved
Hide resolved
This reverts commit 31d143c.
| * | ||
| * @since 8.6.0 | ||
| */ | ||
| IAuthRuleBuilderRuleGroupMatcherBulkExport bulkExportGroupCompartmentMatcher(); |
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.
I don't love the names of these new methods - All of the existing methods in this interface are verbs (update(), patch(), bulkExport(), etc) that read fluently, but these new ones break that pattern with a really cryptic noun instead.
Do we need these new interfaces and methods at all? E.g. could we move the new functionality into IAuthRuleBuilderRuleBulkExport so that instead of needing to add rule:
builder.allow().bulkExportGroupCompartmentMatcher().groupExportOnGroup("?" + theCompartmentMatcherFilter).withResourceTypes(resourceTypes);could we just allow a syntax like:
```java
builder.allow().bulkExport().groupExportOnFilter("?" + theCompartmentMatcherFilter).withResourceTypes(resourceTypes);This would make the API a lot cleaner, and make the new features more discoverable since we wouldn't have 3 top level methods about the same operation.
| * Small service class to inject DB access into an interceptor | ||
| * For example, used in bulk export security to allow querying for resource to match against permission argument filters | ||
| */ | ||
| public class AuthResourceResolver implements IAuthResourceResolver { |
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 makes me nervous. I get the design, but it's breaking the well-established pattern we have that auth interceptor doesn't resolve anything, it just looks at the data it's passed and makes a decision.
Instead of creating a loading infrastructure, could we add a new parameter object to STORAGE_PRE_INITIATE_BULK_EXPORT so that BulkExportJobService could handle loading the Group and pass it to AuthInterceptor as a part of the interceptor request? That way AuthInterceptor could look at it and make its decisions without needing to know how to fetch anything
What was done:
AuthResourceResolverAuthResourceResolverservice to query the DB to apply SearchParameter matching using anInMemoryResourceMatcherCloses #7342