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

issue #1777 - extract compartment inclusion params even when filtered #3376

Merged
merged 9 commits into from
Mar 2, 2022
Merged

issue #1777 - extract compartment inclusion params even when filtered #3376

merged 9 commits into from
Mar 2, 2022

Conversation

lmsurpre
Copy link
Member

@lmsurpre lmsurpre commented Feb 21, 2022 •
edited
Loading

  1. When building our ParametersMaps (e.g. on server start), use CompartmentUtil to get the list of compartment inclusion criteria params (e.g. patient for the Observation resourceType on the Patient compartment) and add them to a new Map within ParametersMap (separate from the other search parameters).

    • If the corresponding inclusion criteria param is explicitly configured in the config, use that
    • If not, then look it up in the registry and log a warning if there is more than one SearchParameter for that Resource+code (just like normal ParametersMap loading).

  2. When extracting search parameter values in SearchUtil, use CompartmentUtil to get the list of compartment inclusion criteria params. For those that aren't configured as external search parameters, add the inclusion parameter from this special map in ParametersMap and add a special DO_NOT_STORE extension to differentiate them from the external search parameters.

  3. When processing the search parameters in FHIRPersistenceJDBCImpl, propagate the DO_NOT_STORE extension into the ExtractedParameterValues via a new "isForStoring" member variable. Then, use CompartmentUtil to get the list of compartment inclusion criteria params one last time and process the corresponding extracted parameter values.

    • If "isForStoring", then this is an external search parameter and so we make a copy of the ExtractedParameterValue for our ibm-internal-[x]-compartment param and add it to the list
    • If not "isForStoring", then rename the search parameter to ibm-internal-[x]-compartment in-place and set isForStoring to true

  4. Added a quickfix for Clean up use of FHIRRequestContext in unit tests #3283 and also updated corresponding fhir-search tests to debug some failing tests and, eventually, work around a design limitation I found in ParametersUtil (and opened ParametersUtil should be more test-friendly #3385 for that)

@lmsurpre
issue #1777 - extract compartment inclusion params even when filtered
2c9ea8c 
Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
@lmsurpre lmsurpre marked this pull request as ready for review February 22, 2022 02:20
@lmsurpre
issue #1777 - do not store temporary/internal param values
21eeb11 
In the previous commit, we started extracting all compartment inclusion
criteria search parameters, even in cases where they are filtered out
via config.
Although the filtered out parameters would not be searchable, we were
still storing these non-searchable parameter values in the db (once with
the inclusion criteria name and once with the corresponding
ibm-internal-x-compartment search parameter name).

This commit updates the search extraction implementation to avoid
storing these intermediate inclusion criteria parameters to the db.
To accomplish this, we now:
1. Add a DO_NOT_STORE extension on the intermediate search parameters
that are used to extract compartment membership information prior to
collecting those values for the ibm-internal-x-compartment search params
2. Propagate that information to the ExtractedParameterValues
3. Filter out parameter values that are not for storage before computing
the extract search parameters hash and returning
ExtractedSearchParameters

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
tbieste
tbieste reviewed Feb 22, 2022
View reviewed changes
Copy link
Contributor

@tbieste tbieste left a comment

Choose a reason for hiding this comment

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

It would be good to add some testcases in some fashion for this.

...rsistence-jdbc/src/main/java/com/ibm/fhir/persistence/jdbc/impl/FHIRPersistenceJDBCImpl.java Show resolved Hide resolved
@lmsurpre
issue #1777 - added tests and minor refactor
20fc22d 
1. added simple unit tests for the new ParametersMap behavior
2. added single e2e test to SearchTest to cover the case where the
compartment inclusion criteria parameter is filtered out in the config

Also, we now add the DO_NOT_STORE extension from SearchUtil instead of
as the parameters are added to the ParametersMap

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

It would be good to add some testcases in some fashion for this.

I just added a couple unit tests and an e2e test under SearchTest. See if you think thats adequate or if I should be adding some more.

@lmsurpre
issue #3283 - set ThreadLocal as part of FHIRRequestContext.get wrapper
7925034 
Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
@lmsurpre lmsurpre force-pushed the issue-1777 branch 5 times, most recently from d57c123 to f585684 Compare February 23, 2022 15:07
@lmsurpre
Attempt to compute search parameter config if missing in ParametersUtil
eec15d5 
Currently, if a tenant gets added / fixed it requires a server restart
to take effect.
With these changes, we'll log a warning and then attempt to compute the
missing tenant search parameter config. Its still computed only once,
but this should enable better support for dynamically adding new
tenants.

Additionally, ParametersSearchUtilTest and
MultiTenantSearchParameterTest now use an 'extended' tenant to test
search-parameter-extensions (instead of 'default') so-as to avoid
conflicts with other tests.

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
@lmsurpre
issue #3283 - revert set ThreadLocal as part of FHIRRequestContext.ge…
9e8ff41 
…t wrapper

Thankfully I was mistaken in what I was seeing... I was thinking that the initialValue wasn't associated with the current thread (which made no sense)
but in reality I was observing sloppy handling of the FHIRRequestContext

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
@lmsurpre
address review comments
1a69a03 
1. added javadoc for the new ParametersMap methods and removed some of
the other unused methods
2. use LinkedList instead of ArrayList in
FHIRPersistenceJDBCImpl.extractSearchParameters
3. update warning message in
FHIRPersistenceJDBCImpl.extractSearchParameters

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
@lmsurpre lmsurpre merged commit 1e4e99f into main Mar 2, 2022
@lmsurpre lmsurpre deleted the issue-1777 branch March 2, 2022 12:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@punktilious punktilious punktilious left review comments

@tbieste tbieste tbieste approved these changes

@prb112 prb112 Awaiting requested review from prb112

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