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.

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

Use search parameter filtering for search parameter disambiguation #1839

Closed
lmsurpre opened this issue Dec 23, 2020 · 1 comment
Closed

Use search parameter filtering for search parameter disambiguation #1839

lmsurpre opened this issue Dec 23, 2020 · 1 comment
Assignees

Comments

@lmsurpre
Copy link
Member

Is your feature request related to a problem? Please describe.
In #1272 we introduced url-based search parameter configuration in fhir-server-config.json.

However, when we build the built-in ParametersMap object, this is done outside of the context of any tenant and so we cannot really consult the tenant-specific search filter.

Further, the current ParametersMap only supports having a single search parameter per code (per resourceType).
There is some logic to warn about cases where there are multiple search parameters with the same code but differing expressions, but I thought that having multiple search parameters with the same code and equivalent expressions would be fine.

However, for our 4.5.3 release, we added some logic to SearchUtil.filterSearchParameters where, if the search parameter chosen during the initialization of the ParametersMap has a URL that differs from the one used in the tenant configuration, then the system skips over this parameter altogether.

Describe the solution you'd like

  1. Update the ParametersMap to support having multiple search parameters with the same code
  2. Emit warnings whenever we have conflicting search parameters and ask operators to disambiguate via fhir-server-config.json

For number 2, we used to do just this. However, since we had no mechanism for disambiguating, the logs were very frustrating because we were packaging multiple IGs with conflicting parameters. Now that we don't prepackage any IGs, I think it is reasonable to expect operators to do the disambiguation in the config when they package IGs that conflict with base spec search params.

Describe alternatives you've considered
#1625 outlines an alternative idea I had which was to build a per-tenant ParametersMap object. If we did that, then we probably wouldn't need to have ParametersMap support having multiple search parameters with the same code. However, that change seems slightly larger.

Additional context
The workaround for this issue is to avoid packaging conflicting search parameters.

lmsurpre added a commit that referenced this issue Dec 23, 2020
1. Update ParametersMap to support storing multiple search parameters
with the same code
2. Address #1743 by collecting to a map instead of a list
3. Update SearchUtil.getSearchParameter to lookup the search parameter
by URI from the config if possible (instead of applying the filter to
the full set of built-in parameters).
4. Update the docs to reflect that search parameter filtering now
applies to tenant-specific search parameters as well. This should help
us move toward #1596

Also fixed a bad trace message and did some minor formatting / javadoc.

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
@lmsurpre lmsurpre self-assigned this Dec 28, 2020
lmsurpre added a commit that referenced this issue Jan 4, 2021
1. Update ParametersMap to support storing multiple search parameters
with the same code
2. Address #1743 by collecting to a map instead of a list
3. Update SearchUtil.getSearchParameter to lookup the search parameter
by URI from the config if possible (instead of applying the filter to
the full set of built-in parameters).
4. Update the docs to reflect that search parameter filtering now
applies to tenant-specific search parameters as well. This should help
us move toward #1596

Also fixed a bad trace message and did some minor formatting / javadoc.

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
@lmsurpre lmsurpre added this to the Sprint 2021-01 milestone Jan 6, 2021
JohnTimm added a commit that referenced this issue Jan 7, 2021
* ci: introduce integration tests for fhir-audit

Signed-off-by: Paul Bastide <pbastide@us.ibm.com>

* ci: introduce integration tests for fhir-audit

Signed-off-by: Paul Bastide <pbastide@us.ibm.com>

* ci: add integration tests for fhir-audit feature and fix one bug with use of/from

Signed-off-by: Paul Bastide <pbastide@us.ibm.com>

* fix: pseudo tty

Signed-off-by: Paul Bastide <pbastide@us.ibm.com>

* fix: small change to the timeout length to 120s

Signed-off-by: Paul Bastide <pbastide@us.ibm.com>

* fix: alternative method for getting the results from the kafka-1 container

Signed-off-by: Paul Bastide <pbastide@us.ibm.com>

* fix: alternative method for getting the results from the kafka-1 container

Signed-off-by: Paul Bastide <pbastide@us.ibm.com>

* fix: add get_results.sh creation of the directory

Signed-off-by: Paul Bastide <pbastide@us.ibm.com>

* fix: update

Signed-off-by: Paul Bastide <pbastide@us.ibm.com>

* fix: update

Signed-off-by: Paul Bastide <pbastide@us.ibm.com>

* fix: for privileged execution

Signed-off-by: Paul Bastide <pbastide@us.ibm.com>

* fix: difference running ci local and remote

Signed-off-by: Paul Bastide <pbastide@us.ibm.com>

* fix: audit

Signed-off-by: Paul Bastide <pbastide@us.ibm.com>

* fix: audit with docker copy

Signed-off-by: Paul Bastide <pbastide@us.ibm.com>

* fix: adding tty support and stdin support

Signed-off-by: Paul Bastide <pbastide@us.ibm.com>

* fix: work around tty issue

Signed-off-by: Paul Bastide <pbastide@us.ibm.com>

* removing the tty references -it

Signed-off-by: Paul Bastide <pbastide@us.ibm.com>

* changed the execution pattern

Signed-off-by: Paul Bastide <pbastide@us.ibm.com>

* changed the execution pattern

Signed-off-by: Paul Bastide <pbastide@us.ibm.com>

* fix: update to predefine output file

Signed-off-by: Paul Bastide <pbastide@us.ibm.com>

* fix: update to predefine output file

Signed-off-by: Paul Bastide <pbastide@us.ibm.com>

* fix permissions

Signed-off-by: Paul Bastide <pbastide@us.ibm.com>

* fix permissions

Signed-off-by: Paul Bastide <pbastide@us.ibm.com>

* fix permissions

Signed-off-by: Paul Bastide <pbastide@us.ibm.com>

* fix permissions

Signed-off-by: Paul Bastide <pbastide@us.ibm.com>

* fix permissions

Signed-off-by: Paul Bastide <pbastide@us.ibm.com>

* fix permissions

Signed-off-by: Paul Bastide <pbastide@us.ibm.com>

* issues #1839 and #1743 - support search parameter disambiguation

1. Update ParametersMap to support storing multiple search parameters
with the same code
2. Address #1743 by collecting to a map instead of a list
3. Update SearchUtil.getSearchParameter to lookup the search parameter
by URI from the config if possible (instead of applying the filter to
the full set of built-in parameters).
4. Update the docs to reflect that search parameter filtering now
applies to tenant-specific search parameters as well. This should help
us move toward #1596

Also fixed a bad trace message and did some minor formatting / javadoc.

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>

* ci: work around issue with tty

Signed-off-by: Paul Bastide <pbastide@us.ibm.com>

* fix: test that doesn't account for year shifts

Signed-off-by: Paul Bastide <pbastide@us.ibm.com>

* fix: test that doesn't account for year shifts

Signed-off-by: Paul Bastide <pbastide@us.ibm.com>

* ci: work around issue with tty

Signed-off-by: Paul Bastide <pbastide@us.ibm.com>

* remove hardcoded year from SearchLastUpdatedIdTest

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>

* fix: change the integration pattern slightly for tty

Signed-off-by: Paul Bastide <pbastide@us.ibm.com>

* fix: change the integration pattern slightly for tty

Signed-off-by: Paul Bastide <pbastide@us.ibm.com>

* fix: change the integration pattern slightly for tty

Signed-off-by: Paul Bastide <pbastide@us.ibm.com>

* fix: last two tests to update with dynamic year

Signed-off-by: Paul Bastide <pbastide@us.ibm.com>

* issues #1839 and #1743 - support search parameter disambiguation

1. Update ParametersMap to support storing multiple search parameters
with the same code
2. Address #1743 by collecting to a map instead of a list
3. Update SearchUtil.getSearchParameter to lookup the search parameter
by URI from the config if possible (instead of applying the filter to
the full set of built-in parameters).
4. Update the docs to reflect that search parameter filtering now
applies to tenant-specific search parameters as well. This should help
us move toward #1596

Also fixed a bad trace message and did some minor formatting / javadoc.

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>

* Apply suggestions from code review

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>

* Update build/audit/README.md

Signed-off-by: Paul Bastide <pbastide@us.ibm.com>

Co-authored-by: Lee Surprenant <lmsurpre@us.ibm.com>

* add info on accessing the bulk operation job logs

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>

* Add unit tests for the ParametersMap

Also made a minor change to insertAll so it gets the code from the
existing ParametersMap instead of from the SearchParameters in the map.
Usually these are the same, but they can differ.

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>

* Issue #1849 - AuthZ interceptor validate/convert search requests

Signed-off-by: Mike Schroeder <mschroed@us.ibm.com>

* Issue #1849 - address review comments

Signed-off-by: Mike Schroeder <mschroed@us.ibm.com>

Co-authored-by: Paul Bastide <pbastide@us.ibm.com>
Co-authored-by: Lee Surprenant <lmsurpre@us.ibm.com>
Co-authored-by: Mike Schroeder <mschroed@us.ibm.com>
Co-authored-by: Michael W Schroeder <66479070+michaelwschroeder@users.noreply.github.com>
JohnTimm added a commit that referenced this issue Jan 21, 2021
* ci: introduce integration tests for fhir-audit

Signed-off-by: Paul Bastide <pbastide@us.ibm.com>

* ci: introduce integration tests for fhir-audit

Signed-off-by: Paul Bastide <pbastide@us.ibm.com>

* ci: add integration tests for fhir-audit feature and fix one bug with use of/from

Signed-off-by: Paul Bastide <pbastide@us.ibm.com>

* fix: pseudo tty

Signed-off-by: Paul Bastide <pbastide@us.ibm.com>

* fix: small change to the timeout length to 120s

Signed-off-by: Paul Bastide <pbastide@us.ibm.com>

* fix: alternative method for getting the results from the kafka-1 container

Signed-off-by: Paul Bastide <pbastide@us.ibm.com>

* fix: alternative method for getting the results from the kafka-1 container

Signed-off-by: Paul Bastide <pbastide@us.ibm.com>

* fix: add get_results.sh creation of the directory

Signed-off-by: Paul Bastide <pbastide@us.ibm.com>

* fix: update

Signed-off-by: Paul Bastide <pbastide@us.ibm.com>

* fix: update

Signed-off-by: Paul Bastide <pbastide@us.ibm.com>

* fix: for privileged execution

Signed-off-by: Paul Bastide <pbastide@us.ibm.com>

* fix: difference running ci local and remote

Signed-off-by: Paul Bastide <pbastide@us.ibm.com>

* fix: audit

Signed-off-by: Paul Bastide <pbastide@us.ibm.com>

* fix: audit with docker copy

Signed-off-by: Paul Bastide <pbastide@us.ibm.com>

* fix: adding tty support and stdin support

Signed-off-by: Paul Bastide <pbastide@us.ibm.com>

* fix: work around tty issue

Signed-off-by: Paul Bastide <pbastide@us.ibm.com>

* removing the tty references -it

Signed-off-by: Paul Bastide <pbastide@us.ibm.com>

* changed the execution pattern

Signed-off-by: Paul Bastide <pbastide@us.ibm.com>

* changed the execution pattern

Signed-off-by: Paul Bastide <pbastide@us.ibm.com>

* fix: update to predefine output file

Signed-off-by: Paul Bastide <pbastide@us.ibm.com>

* fix: update to predefine output file

Signed-off-by: Paul Bastide <pbastide@us.ibm.com>

* fix permissions

Signed-off-by: Paul Bastide <pbastide@us.ibm.com>

* fix permissions

Signed-off-by: Paul Bastide <pbastide@us.ibm.com>

* fix permissions

Signed-off-by: Paul Bastide <pbastide@us.ibm.com>

* fix permissions

Signed-off-by: Paul Bastide <pbastide@us.ibm.com>

* fix permissions

Signed-off-by: Paul Bastide <pbastide@us.ibm.com>

* fix permissions

Signed-off-by: Paul Bastide <pbastide@us.ibm.com>

* issues #1839 and #1743 - support search parameter disambiguation

1. Update ParametersMap to support storing multiple search parameters
with the same code
2. Address #1743 by collecting to a map instead of a list
3. Update SearchUtil.getSearchParameter to lookup the search parameter
by URI from the config if possible (instead of applying the filter to
the full set of built-in parameters).
4. Update the docs to reflect that search parameter filtering now
applies to tenant-specific search parameters as well. This should help
us move toward #1596

Also fixed a bad trace message and did some minor formatting / javadoc.

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>

* ci: work around issue with tty

Signed-off-by: Paul Bastide <pbastide@us.ibm.com>

* fix: test that doesn't account for year shifts

Signed-off-by: Paul Bastide <pbastide@us.ibm.com>

* fix: test that doesn't account for year shifts

Signed-off-by: Paul Bastide <pbastide@us.ibm.com>

* ci: work around issue with tty

Signed-off-by: Paul Bastide <pbastide@us.ibm.com>

* remove hardcoded year from SearchLastUpdatedIdTest

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>

* fix: change the integration pattern slightly for tty

Signed-off-by: Paul Bastide <pbastide@us.ibm.com>

* fix: change the integration pattern slightly for tty

Signed-off-by: Paul Bastide <pbastide@us.ibm.com>

* fix: change the integration pattern slightly for tty

Signed-off-by: Paul Bastide <pbastide@us.ibm.com>

* fix: last two tests to update with dynamic year

Signed-off-by: Paul Bastide <pbastide@us.ibm.com>

* issues #1839 and #1743 - support search parameter disambiguation

1. Update ParametersMap to support storing multiple search parameters
with the same code
2. Address #1743 by collecting to a map instead of a list
3. Update SearchUtil.getSearchParameter to lookup the search parameter
by URI from the config if possible (instead of applying the filter to
the full set of built-in parameters).
4. Update the docs to reflect that search parameter filtering now
applies to tenant-specific search parameters as well. This should help
us move toward #1596

Also fixed a bad trace message and did some minor formatting / javadoc.

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>

* Apply suggestions from code review

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>

* Update build/audit/README.md

Signed-off-by: Paul Bastide <pbastide@us.ibm.com>

Co-authored-by: Lee Surprenant <lmsurpre@us.ibm.com>

* add info on accessing the bulk operation job logs

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>

* Add unit tests for the ParametersMap

Also made a minor change to insertAll so it gets the code from the
existing ParametersMap instead of from the SearchParameters in the map.
Usually these are the same, but they can differ.

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>

* Issue #1849 - AuthZ interceptor validate/convert search requests

Signed-off-by: Mike Schroeder <mschroed@us.ibm.com>

* Issue #1849 - address review comments

Signed-off-by: Mike Schroeder <mschroed@us.ibm.com>

* Duplicate Job Parameters fhir.dataSourcesInfo are created #1855

- Removed the duplicate serialization of the fhir.dataSourcesInfo

Signed-off-by: Paul Bastide <pbastide@us.ibm.com>

* Confusing error when request is targetted for an invalid tenant id #1792

- Disambiguates the Error Messages that are bubbled up through the
Persistence Layer
- Add Integration Test

Signed-off-by: Paul Bastide <pbastide@us.ibm.com>

* Confusing error when request is targetted for an invalid tenant id #1792

1 - added layer at the rest level

Signed-off-by: Paul Bastide <pbastide@us.ibm.com>

* Issue #1615 - Enforce configured interactions in REST layer

Signed-off-by: Mike Schroeder <mschroed@us.ibm.com>

* Update FHIRValidationGuide.md

1. update the version references for the packaged implementation guides
2. add a section at the top to describe where to get the validation module

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>

* Issue #1615 - address review comments

Signed-off-by: Mike Schroeder <mschroed@us.ibm.com>

* Issue #1615 - fix whitespace

Signed-off-by: Mike Schroeder <mschroed@us.ibm.com>

* Issue #1615 - add enum for interaction types

Signed-off-by: Mike Schroeder <mschroed@us.ibm.com>

* Modify davinci-pdex CapabilityStatement-pdex-server.json

updated the searchRevInclude value for the Coverage resource in
pdex-server to work around https://jira.hl7.org/browse/FHIR-30338

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>

* Issue #1494 - add Bundle.entry.search to search results

Signed-off-by: Mike Schroeder <mschroed@us.ibm.com>

* Issue #1494 - add documentation

Signed-off-by: Mike Schroeder <mschroed@us.ibm.com>

* Issue #1494 - address review comments

Signed-off-by: Mike Schroeder <mschroed@us.ibm.com>

Co-authored-by: Paul Bastide <pbastide@us.ibm.com>
Co-authored-by: Lee Surprenant <lmsurpre@us.ibm.com>
Co-authored-by: Mike Schroeder <mschroed@us.ibm.com>
Co-authored-by: Michael W Schroeder <66479070+michaelwschroeder@users.noreply.github.com>
@d0roppe
Copy link
Collaborator

d0roppe commented Jan 25, 2021

Started the server and noted these messages in the log file:
[1/25/21, 17:56:11:995 UTC] 00000035 com.ibm.fhir.search.util.SearchUtil W Skipping search parameter with id='location-address'. Found multiple search parameters for code 'address' on resource type 'Location'; use search parameter filtering to disambiguate.
[1/25/21, 17:56:11:995 UTC] 00000035 com.ibm.fhir.search.util.SearchUtil W Skipping search parameter with id='location-address-state'. Found multiple search parameters for code 'address-state' on resource type 'Location'; use search parameter filtering to disambiguate.

Added the following to the fhir-server-config.json
"resources": {
"open": true,
"Location": {
"searchParameters": {
"address": "http://hl7.org/fhir/us/core/SearchParameter/us-core-location-address",
"address-state": "http://hl7.org/fhir/us/core/SearchParameter/us-core-location-address-state"
}
}
},

restarted the server, and those messages were no longer issued.

@d0roppe d0roppe closed this as completed Jan 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants