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 the internal patient compartment search parameter during Patient/Group export #2094

Closed
lmsurpre opened this issue Mar 15, 2021 · 4 comments
Assignees

Comments

@lmsurpre
Copy link
Member

We now have a special internal "compartment" search parameter that we should be able to leverage so that we don't need to aggregate all the different compartment inclusion criteria parameters separately.

For example, we might be able to get rid of this code in PatientResourceHandler.fillChunkDataBuffer(List<String> patientIds, ReadResultDTO dto):

            List<String> compartmentSearchCriterias = CompartmentUtil.getCompartmentResourceTypeInclusionCriteria("Patient", resourceType.getSimpleName());
            if (compartmentSearchCriterias.size() > 1) {
                isDoDuplicationCheck = true;
            }
            for (String compartmentSearchCriteria : compartmentSearchCriterias) {
                HashMap<String, List<String>> queryTmpParameters = new HashMap<>();
                queryTmpParameters.putAll(queryParameters);
                queryTmpParameters.put(compartmentSearchCriteria, Arrays.asList(String.join(",", patientIds)));
                searchContext = SearchUtil.parseQueryParameters(resourceType, queryTmpParameters);

One thing that might make this difficult is that, under normal operation, a patient compartment search is only ever scoped to a single patient (whereas here we have a list of patients). A couple ideas:
A. Perform a different search for each specific patient included in the export. It would definitely be a lot more searches (i.e. more round trips to the DB), but maybe these simple searches will perform better than one giant OR query like we have now?
B. Add support (if we don't already support it) for having a logical OR on the internal compartment search parameter.

If we're able to do B, this would also help us to support user scoped SMART App Launch scopes in our AuthzPolicyEnforcementPersistenceInterceptor, which are for cases where the user has access to a set of patient compartment resources and not just their own.

Originally posted by @lmsurpre in #2058 (comment)

lmsurpre added a commit that referenced this issue Mar 17, 2021
1. Introduces SearchUtil.buildInclusionCriteria() for building the
inclusion criteria parameter with multiple compartmentId values (maybe
this should go in CompartmentUtil instead?)
2. Updates JDBCQueryBuilder.processInclusionCriteria to use all of the
inclusionCriteria values; previously it used `=` against the first value
but now it uses `IN (val1,val2,val3,...)` instead.

And update 3 places to use it:
1. A new test in AbstractSearchCompartmentTest
2. Patient export (PatientResourceHandler.fillChunkDataBuffer()
    This should be much simpler now and also much more efficient.
Instead of searching with a value of `id1,id2,id3,...` on each and every
inclusion criteria, we can do a single query with a whole page of
patient ids.
3. AuthzPolicyEnforcementPersistenceInterceptor.beforeSearch()
    This lays the groundwork for supporting `user` scopes. Instead of
converting the request to a compartment search for a single patient, we
can 'or' between the full set of patient_id values included in the
access token.
Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
lmsurpre added a commit that referenced this issue Mar 17, 2021
Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
lmsurpre added a commit that referenced this issue Mar 17, 2021
Handle the Patient resource specially since we already know the ids (and
because it technically doesn't belong to the Patient compartment).

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
lmsurpre added a commit that referenced this issue Mar 17, 2021
Handle the Patient resource specially since we already know the ids (and
because it technically doesn't belong to the Patient compartment).

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
lmsurpre added a commit that referenced this issue Mar 17, 2021
1. Handle the Patient resource specially since we already know the ids (and
because it technically doesn't belong to the Patient compartment).

2. Update the GroupHandler to use ReferenceUtil from fhir-search and log
warnings for member references that won't be resolved

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
lmsurpre added a commit that referenced this issue Mar 17, 2021
1. Introduces SearchUtil.buildInclusionCriteria() for building the
inclusion criteria parameter with multiple compartmentId values (maybe
this should go in CompartmentUtil instead?)
2. Updates JDBCQueryBuilder.processInclusionCriteria to use all of the
inclusionCriteria values; previously it used `=` against the first value
but now it uses `IN (val1,val2,val3,...)` instead.

And update 3 places to use it:
1. A new test in AbstractSearchCompartmentTest
2. Patient export (PatientResourceHandler.fillChunkDataBuffer()
    This should be much simpler now and also much more efficient.
Instead of searching with a value of `id1,id2,id3,...` on each and every
inclusion criteria, we can do a single query with a whole page of
patient ids.
3. AuthzPolicyEnforcementPersistenceInterceptor.beforeSearch()
    This lays the groundwork for supporting `user` scopes. Instead of
converting the request to a compartment search for a single patient, we
can 'or' between the full set of patient_id values included in the
access token.
Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
lmsurpre added a commit that referenced this issue Mar 17, 2021
1. Handle the Patient resource specially since we already know the ids (and
because it technically doesn't belong to the Patient compartment).

2. Update the GroupHandler to use ReferenceUtil from fhir-search and log
warnings for member references that won't be resolved

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
@lmsurpre lmsurpre self-assigned this Mar 18, 2021
@lmsurpre
Copy link
Member Author

lmsurpre commented Mar 18, 2021

Before this change, a compartment search like Patient/123/Basic?string=teststring (with "useStoredCompartmentParam": false) would result in SQL like:

SELECT COUNT(DISTINCT R.LOGICAL_RESOURCE_ID)
  FROM Basic_LOGICAL_RESOURCES LR
  JOIN Basic_RESOURCES R
    ON R.RESOURCE_ID = LR.CURRENT_RESOURCE_ID
   AND R.IS_DELETED = 'N'
  JOIN Basic_TOKEN_VALUES_V AS param0
    ON ((param0.PARAMETER_NAME_ID = 30000 AND param0.TOKEN_VALUE = ? AND param0.CODE_SYSTEM_ID = 30001))
   AND LR.LOGICAL_RESOURCE_ID = param0.LOGICAL_RESOURCE_ID 
  JOIN Basic_STR_VALUES  AS param1
    ON (param1.PARAMETER_NAME_ID = 20006 AND (param1.STR_VALUE_LCASE LIKE ? ESCAPE '+'))
   AND LR.LOGICAL_RESOURCE_ID = param1.LOGICAL_RESOURCE_ID

searchArgs=[123, teststring%]

After this change, the same query now looks like:

SELECT COUNT(DISTINCT R.LOGICAL_RESOURCE_ID)  
  FROM Basic_LOGICAL_RESOURCES LR  
  JOIN Basic_RESOURCES R ON R.RESOURCE_ID = LR.CURRENT_RESOURCE_ID  
   AND R.IS_DELETED = 'N'  
  JOIN Basic_TOKEN_VALUES_V  AS param0 
    ON ((param0.PARAMETER_NAME_ID = 30000 AND param0.TOKEN_VALUE IN (?) AND param0.CODE_SYSTEM_ID = 30001)) 
   AND LR.LOGICAL_RESOURCE_ID = param0.LOGICAL_RESOURCE_ID 
  JOIN Basic_STR_VALUES  AS param1 
    ON (param1.PARAMETER_NAME_ID = 20006 AND (param1.STR_VALUE_LCASE LIKE ? ESCAPE '+')) 
   AND LR.LOGICAL_RESOURCE_ID = param1.LOGICAL_RESOURCE_ID

searchArgs=[123, teststring%]

Or, if "useStoredCompartmentParam": true and the common_token_value exists:

SELECT COUNT(DISTINCT R.LOGICAL_RESOURCE_ID)  
  FROM Basic_LOGICAL_RESOURCES LR  
  JOIN Basic_RESOURCES R ON R.RESOURCE_ID = LR.CURRENT_RESOURCE_ID  
   AND R.IS_DELETED = 'N'  
  JOIN Basic_TOKEN_VALUES_V  AS param0 
    ON ((param0.PARAMETER_NAME_ID = 40000 AND param0.COMMON_TOKEN_VALUE_ID IN (100001))) 
   AND LR.LOGICAL_RESOURCE_ID = param0.LOGICAL_RESOURCE_ID JOIN Basic_STR_VALUES  AS param1 
    ON (param1.PARAMETER_NAME_ID = 20006 AND (param1.STR_VALUE_LCASE LIKE ? ESCAPE '+')) 
   AND LR.LOGICAL_RESOURCE_ID = param1.LOGICAL_RESOURCE_ID

searchArgs=[teststring%]

Basically, we replace the first = from param0.TOKEN_VALUE = ? AND param0.CODE_SYSTEM_ID = 30001 with the IN construct.
Additionally I noticed that, even when using COMMON_TOKEN_VALUE, we were still including the TOKEN_VALUE in the where clause...I think that should already be wrapped up in the COMMON_TOKEN_VALUE and so I removed that part.
See the diff on JDBCQueryBuilder.processInclusionCriteria() for more detail.

@lmsurpre
Copy link
Member Author

The payoff here is that we can now perform queries against multiple patient compartments at once (for example for Patient export or for scoping a query for security/privacy reasons). Here is the above query, but scoped to patient compartments 123 and xyz.
Note that Patient/123 exists, but Patient/xyz does not, and so it falls back to the "old way":

SELECT COUNT(DISTINCT R.LOGICAL_RESOURCE_ID)  
  FROM Basic_LOGICAL_RESOURCES LR  
  JOIN Basic_RESOURCES R ON R.RESOURCE_ID = LR.CURRENT_RESOURCE_ID  
   AND R.IS_DELETED = 'N'  
  JOIN Basic_TOKEN_VALUES_V  AS param0 
    ON ((param0.PARAMETER_NAME_ID = 20017 AND param0.TOKEN_VALUE IN (?, ?) AND param0.CODE_SYSTEM_ID = 20020)) 
   AND LR.LOGICAL_RESOURCE_ID = param0.LOGICAL_RESOURCE_ID 
  JOIN Basic_STR_VALUES  AS param1 
    ON (param1.PARAMETER_NAME_ID = 20006 AND (param1.STR_VALUE_LCASE LIKE ? ESCAPE '+')) 
   AND LR.LOGICAL_RESOURCE_ID = param1.LOGICAL_RESOURCE_ID

searchArgs=[123, xyz, teststring%]

@kmbarton423 kmbarton423 added this to the Sprint 2021-04 milestone Mar 18, 2021
lmsurpre added a commit that referenced this issue Mar 19, 2021
1. Introduces SearchUtil.buildInclusionCriteria() for building the
inclusion criteria parameter with multiple compartmentId values (maybe
this should go in CompartmentUtil instead?)
2. Updates JDBCQueryBuilder.processInclusionCriteria to use all of the
inclusionCriteria values; previously it used `=` against the first value
but now it uses `IN (val1,val2,val3,...)` instead.

And update 3 places to use it:
1. A new test in AbstractSearchCompartmentTest
2. Patient export (PatientResourceHandler.fillChunkDataBuffer()
    This should be much simpler now and also much more efficient.
Instead of searching with a value of `id1,id2,id3,...` on each and every
inclusion criteria, we can do a single query with a whole page of
patient ids.
3. AuthzPolicyEnforcementPersistenceInterceptor.beforeSearch()
    This lays the groundwork for supporting `user` scopes. Instead of
converting the request to a compartment search for a single patient, we
can 'or' between the full set of patient_id values included in the
access token.
Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
lmsurpre added a commit that referenced this issue Mar 19, 2021
1. Handle the Patient resource specially since we already know the ids (and
because it technically doesn't belong to the Patient compartment).

2. Update the GroupHandler to use ReferenceUtil from fhir-search and log
warnings for member references that won't be resolved

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
lmsurpre added a commit that referenced this issue Mar 19, 2021
1. Introduces SearchUtil.buildInclusionCriteria() for building the
inclusion criteria parameter with multiple compartmentId values (maybe
this should go in CompartmentUtil instead?)
2. Updates JDBCQueryBuilder.processInclusionCriteria to use all of the
inclusionCriteria values; previously it used `=` against the first value
but now it uses `IN (val1,val2,val3,...)` instead.

And update 3 places to use it:
1. A new test in AbstractSearchCompartmentTest
2. Patient export (PatientResourceHandler.fillChunkDataBuffer()
    This should be much simpler now and also much more efficient.
Instead of searching with a value of `id1,id2,id3,...` on each and every
inclusion criteria, we can do a single query with a whole page of
patient ids.
3. AuthzPolicyEnforcementPersistenceInterceptor.beforeSearch()
    This lays the groundwork for supporting `user` scopes. Instead of
converting the request to a compartment search for a single patient, we
can 'or' between the full set of patient_id values included in the
access token.
Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
lmsurpre added a commit that referenced this issue Mar 19, 2021
1. Handle the Patient resource specially since we already know the ids (and
because it technically doesn't belong to the Patient compartment).

2. Update the GroupHandler to use ReferenceUtil from fhir-search and log
warnings for member references that won't be resolved

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
lmsurpre added a commit that referenced this issue Mar 21, 2021
1. Introduces SearchUtil.buildInclusionCriteria() for building the
inclusion criteria parameter with multiple compartmentId values (maybe
this should go in CompartmentUtil instead?)
2. Updates JDBCQueryBuilder.processInclusionCriteria to use all of the
inclusionCriteria values; previously it used `=` against the first value
but now it uses `IN (val1,val2,val3,...)` instead.

And update 3 places to use it:
1. A new test in AbstractSearchCompartmentTest
2. Patient export (PatientResourceHandler.fillChunkDataBuffer()
    This should be much simpler now and also much more efficient.
Instead of searching with a value of `id1,id2,id3,...` on each and every
inclusion criteria, we can do a single query with a whole page of
patient ids.
3. AuthzPolicyEnforcementPersistenceInterceptor.beforeSearch()
    This lays the groundwork for supporting `user` scopes. Instead of
converting the request to a compartment search for a single patient, we
can 'or' between the full set of patient_id values included in the
access token.
Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
lmsurpre added a commit that referenced this issue Mar 21, 2021
1. Handle the Patient resource specially since we already know the ids (and
because it technically doesn't belong to the Patient compartment).

2. Update the GroupHandler to use ReferenceUtil from fhir-search and log
warnings for member references that won't be resolved

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

d0roppe commented Mar 23, 2021

Tested bulk data export for both patient compartment and group. Things are working as expected.

@prb112
Copy link
Contributor

prb112 commented Mar 23, 2021

Closing Per Dag's Comment. I've done the same, and things are working as expected for me too.

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

No branches or pull requests

4 participants