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.

Sign up for GitHub

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

Jump to bottom

issues 3242 and 3265 - scope search-system and search-history for fhirVersion #3268

Merged
merged 8 commits into from
Feb 4, 2022
Merged

issues 3242 and 3265 - scope search-system and search-history for fhirVersion #3268

merged 8 commits into from
Feb 4, 2022

Conversation

lmsurpre
Copy link
Member

@lmsurpre lmsurpre commented Feb 2, 2022 •
edited
Loading

  1. introduce enums and utilities for working with resource type names
    and fhirVersion values ('4.0' and '4.3') in fhir-core (we had similar in fhir-model,
    but those weren't available to fhir-config and the added complexity of the FHIR
    model makes them a tad harder to work with)
  2. use those to provide a better abstraction for working with the
    fhir-server-config fhirServer/resources property group
    (ResourcesConfigAdapter)
  3. move fhirVersion MIME-type parameter processing into a new JAX-RS
    RequestFilter FHIRVersionRequestFilter and update the JAX-RS resources and
    tests accordingly
  4. update FHIRPersistenceUtil.parseSystemHistoryParameters to take into
    account the requested fhirVersion and scope the HistoryContext
    appropriately
  5. update SearchUtil.parseQueryParameters to take into account the
    requested fhirVersion and scope the SearchContext appropriately
  6. update $everything to use ResourcesConfigAdapter to get the list of applicable
    resource types in EverythingOperation.getDefaultIncludedResourceTypes
    • to implement this, I added the FHIRVersionParam to the OperationContext

also included the small removal for #3265

@lmsurpre
issue #3242 - introduce defaultFhirVersion config property
0b11970 
This changeset adds a JAX-RS RequestFilter that sets a requestContext
property, "com.ibm.fhir.server.fhirVersion" with the following order of
precedence:
   1. "4.3" from the acceptableMediaTypes
   2. "4.0" from the acceptableMediaTypes
   3. whatever is configured in the fhirServer/core/defaultFhirVersion
config property
   4. "4.0"

Additionally, I updated the Capabilities resource to use the fhirVersion
from the request context instead of processing the Accept header itself.

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
tbieste
tbieste approved these changes Feb 2, 2022
View reviewed changes
docs/src/pages/guides/FHIRServerUsersGuide.md Outdated Show resolved Hide resolved
@lmsurpre
issue #3265 - checkType throws instead of logging for abstract types
9710705 
Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
@lmsurpre
issue #3242 - scope search-system and search-history for fhirVersion
44841c9 
1. introduce enums and utilities for working with resource type names
and fhirVersion values ('4.0' and '4.3')
2. use those to provide a better abstraction for working with the
fhir-server-config `fhirServer/resources` property group
(ResourcesConfigAdapter)
3. move fhirVersion MIME-type parameter processing into a new JAX-RS
RequestFilter FHIRVersionRequestFilter and update tests accordingly
4. update FHIRPersistenceUtil.parseSystemHistoryParameters to take into
account the requested fhirVersion and scope the HistoryContext
appropriately
5. update SearchUtil.parseQueryParameters to take into account the
requested fhirVersion and scope the SearchContext appropriately

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
@lmsurpre
issue #3242 - update $everything to use ResourcesConfigAdapter
2430a86 
1. add the FHIRVersionParam to the OperationContext
2. use that with a ResourcesConfigAdapter to get the list of applicable
resource types in EverythingOperation.getDefaultIncludedResourceTypes

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
@lmsurpre
issue #3242 - address review feedback
053f2ea 
mostly javadoc updates and a couple minor refactorings

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
@lmsurpre
issue #3242 - get fhirVersion from Content-Type for create/update
c4d5ef9 
Previously I was only looking in the Accept header. Now, for PUT and
POST requests, I'll check the Content-Type header first.

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
punktilious
punktilious reviewed Feb 4, 2022
View reviewed changes
fhir-persistence/src/test/java/com/ibm/fhir/persistence/util/FHIRPersistenceUtilTest.java

assertTrue(historyContext.getResourceTypes().isEmpty(), historyContext.getResourceTypes().toString());
} finally {
FHIRRequestContext.get().setTenantId(originalTenantId);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you think we should use remove (or at least clear the tenantId here...all tests should be forced to establish their own tenant in the context, so I don't think setting the originalTenantId should be necessary (unless there's a specific use case I'm not thinking of here.)

Copy link
Member Author

Choose a reason for hiding this comment

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

yes. i'm not sure how important it is, but i think calling FHIRRequestContext.remove() after each test is probably a good pattern to help prevent us from more errors related to the thread-local

Copy link
Member Author

Choose a reason for hiding this comment

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

maybe as part of #3283 ?

@lmsurpre
resolve TODO about PlanDefinition and ActivityDefintion
7790091 
In the latest draft of R4B, these resource types have a new choice type
for the subject field.
This means that all R4 instances will still be valid in R4B, but not
vice-versa.
Instead of coding up special support where R4 clients can PUT/POST these
resource instances, but not read them, we chose to treat them like
backwards-compatible resources and document the incompatibility concern
for R4 clients.

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
punktilious
punktilious approved these changes Feb 4, 2022
View reviewed changes
Copy link
Collaborator

@punktilious punktilious left a comment

Choose a reason for hiding this comment

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

LGTM, just some minor comments

@lmsurpre
Address more review comments
1496f69 
1. resolve TODO in Capabilities (replace getSupportedResourceTypes with
ResourcesConfigAdapter)
2. fix stale javadoc for SearchUtil.useStoredCompartmentParam

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
@lmsurpre
Copy link
Member Author

lmsurpre commented Feb 4, 2022

the latest runs failed due to an unrelated issue in our CI that has already been fixed in main, so I'm gonna go ahead and merge this into r4b-rebased so that i can rebase the whole lot onto main all together

@lmsurpre lmsurpre merged commit 973a820 into r4b-rebased Feb 4, 2022
@lmsurpre lmsurpre deleted the issue-3242 branch February 4, 2022 23:02
@lmsurpre lmsurpre mentioned this pull request Feb 7, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tbieste tbieste tbieste approved these changes

@punktilious punktilious punktilious approved these changes

@sfc42 sfc42 Awaiting requested review from sfc42

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@lmsurpre @punktilious @tbieste