Skip to content

Commit

Permalink
Merge pull request #2710 from IBM/issue-2553
Browse files Browse the repository at this point in the history 
Issue #2553 - fix chained search and system-only global parm search
  • Loading branch information
@michaelwschroeder
michaelwschroeder committed Aug 24, 2021
2 parents 94e7f63 + aeb0057 commit 9900736
Show file tree
Hide file tree
Showing 8 changed files with 397 additions and 22 deletions.
12 changes: 11 additions & 1 deletion ...rsistence-jdbc/src/main/java/com/ibm/fhir/persistence/jdbc/domain/ChainedSearchParam.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,11 @@

package com.ibm.fhir.persistence.jdbc.domain;

import static com.ibm.fhir.search.SearchConstants.PROFILE;
import static com.ibm.fhir.search.SearchConstants.SECURITY;
import static com.ibm.fhir.search.SearchConstants.TAG;
import static com.ibm.fhir.search.SearchConstants.URL;

import java.util.logging.Logger;

import com.ibm.fhir.persistence.exception.FHIRPersistenceException;
Expand Down Expand Up @@ -85,8 +90,13 @@ private <T> void addFinalFilter(T currentSubQuery, SearchQueryVisitor<T> visitor
msp.visit(currentSubQuery, visitor);
} else if (currentParm.getType() == Type.COMPOSITE) {
visitor.addCompositeParam(currentSubQuery, currentParm);
} else if (currentParm.isCanonical()) {
} else if (currentParm.isCanonical() || PROFILE.equals(currentParm.getCode()) ||
(currentParm.getType() == Type.URI && URL.equals(currentParm.getCode()))) {
visitor.addCanonicalParam(currentSubQuery, ((QueryData)currentSubQuery).getResourceType(), currentParm);
} else if (TAG.equals(currentParm.getCode())) {
visitor.addTagParam(currentSubQuery, ((QueryData)currentSubQuery).getResourceType(), currentParm);
} else if (SECURITY.equals(currentParm.getCode())) {
visitor.addSecurityParam(currentSubQuery, ((QueryData)currentSubQuery).getResourceType(), currentParm);
} else {
visitor.addFilter(currentSubQuery, getRootResourceType(), currentParm);
}
Expand Down
22 changes: 16 additions & 6 deletions ...sistence-jdbc/src/main/java/com/ibm/fhir/persistence/jdbc/domain/SearchQueryRenderer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1757,6 +1757,14 @@ private QueryData addGlobalTokenParam(QueryData queryData, QueryParameter queryP
// need to use paramAlias as the common token values alias and generate a new alias for the parameter table.
tokenValuesAlias = getParamAlias(getNextAliasIndex());
tokenValueSearch = true;
} else {
ColumnExpNodeVisitor visitor = new ColumnExpNodeVisitor(); // gathers all columns used in the filter expression
Set<String> columns = filter.visit(visitor);
if (columns.contains(DataDefinitionUtil.getQualifiedName(paramAlias, CODE_SYSTEM_ID)) ||
columns.contains(DataDefinitionUtil.getQualifiedName(paramAlias, TOKEN_VALUE))) {
tokenValuesAlias = getParamAlias(getNextAliasIndex());
tokenValueSearch = true;
}
}

if (Modifier.NOT.equals(queryParm.getModifier()) || Modifier.NOT_IN.equals(queryParm.getModifier())) {
Expand All @@ -1765,10 +1773,6 @@ private QueryData addGlobalTokenParam(QueryData queryData, QueryParameter queryP
exists.from(parameterTable, alias(tokenValuesAlias))
.where(tokenValuesAlias, "LOGICAL_RESOURCE_ID").eq(lrAlias, "LOGICAL_RESOURCE_ID"); // correlate with the main query
if (tokenValueSearch) {
if (Modifier.TEXT.equals(queryParm.getModifier())) {
exists.from().where().and(tokenValuesAlias, "PARAMETER_NAME_ID")
.eq(getParameterNameId(queryParm.getCode() + SearchConstants.TEXT_MODIFIER_SUFFIX));
}
// Join to common token values table and add filter predicate to the common token values join on clause
exists.from().innerJoin("COMMON_TOKEN_VALUES", alias(paramAlias), on(paramAlias, "COMMON_TOKEN_VALUE_ID")
.eq(tokenValuesAlias, "COMMON_TOKEN_VALUE_ID")
Expand Down Expand Up @@ -2083,10 +2087,16 @@ public void addFilter(QueryData queryData, String resourceType, QueryParameter c
// AND P3.PARAMETER_NAME_ID = 123 -- 'name parameter'
// AND P3.STR_VALUE = 'Jones') -- 'name filter'
final int aliasIndex = getNextAliasIndex();
final String paramTable = paramValuesTableName(queryData.getResourceType(), currentParm);
final String paramAlias = getParamAlias(aliasIndex);

WhereFragment pf = paramFilter(currentParm, paramAlias);
final String paramTable;
if (Type.TOKEN.equals(currentParm.getType()) &&
!(TAG.equals(currentParm.getCode()) || SECURITY.equals(currentParm.getCode()))) {
paramTable = getTokenParamTable(pf.getExpression(), queryData.getResourceType(), paramAlias);
} else {
paramTable = paramValuesTableName(queryData.getResourceType(), currentParm);
}

if (currentParm.getModifier() == Modifier.NOT) {
// Needs to be handled as a NOT EXISTS correlated subquery
SelectAdapter exists = Select.select("1");
Expand Down
16 changes: 5 additions & 11 deletions ...rsistence/src/test/java/com/ibm/fhir/persistence/search/test/AbstractSearchTokenTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -248,7 +248,7 @@ public void testSearchToken_CodeableConcept() throws Exception {
assertSearchReturnsSavedResource("CodeableConcept-validCodeAndSystem", "http://terminology.hl7.org/CodeSystem/v3-ObservationInterpretation|");

// This shouldn't return any results because the CodeableConcept has a system
// assertSearchDoesntReturnSavedResource("CodeableConcept", "|code");
assertSearchDoesntReturnSavedResource("CodeableConcept", "|code");
}

@Test
Expand All @@ -268,9 +268,7 @@ public void testSearchToken_CodeableConcept_escaped() throws Exception {
public void testSearchToken_CodeableConcept_chained() throws Exception {
assertSearchReturnsComposition("subject:Basic.CodeableConcept", "code");
assertSearchReturnsComposition("subject:Basic.CodeableConcept", "http://example.org/codesystem|code");

// system-only token search is not working for chained searches yet (https://github.com/IBM/FHIR/issues/2553)
// assertSearchReturnsComposition("subject:Basic.CodeableConcept", "http://example.org/codesystem|");
assertSearchReturnsComposition("subject:Basic.CodeableConcept", "http://example.org/codesystem|");

// This shouldn't return any results because the CodeableConcept has a system
assertSearchDoesntReturnComposition("subject:Basic.CodeableConcept", "|code");
Expand Down Expand Up @@ -400,9 +398,7 @@ public void testSearchToken_Coding_escaped() throws Exception {
public void testSearchToken_Coding_chained() throws Exception {
assertSearchReturnsComposition("subject:Basic.Coding", "code");
assertSearchReturnsComposition("subject:Basic.Coding", "http://example.org/codesystem|code");

// system-only token search is not working for chained searches yet (https://github.com/IBM/FHIR/issues/2553)
// assertSearchReturnsComposition("subject:Basic.Coding", "http://example.org/codesystem|");
assertSearchReturnsComposition("subject:Basic.Coding", "http://example.org/codesystem|");

// This shouldn't return any results because the Coding has a system
assertSearchDoesntReturnComposition("subject:Basic.Coding", "|code");
Expand Down Expand Up @@ -523,16 +519,14 @@ public void testSearchToken_Identifier() throws Exception {
assertSearchReturnsSavedResource("Identifier-validValueAndSystem", "http://hl7.org/fhir/identifier-use|");

// This shouldn't return any results because the Identifier has a system
// assertSearchDoesntReturnSavedResource("Identifier", "|code");
assertSearchDoesntReturnSavedResource("Identifier", "|code");
}

@Test
public void testSearchToken_Identifier_chained() throws Exception {
assertSearchReturnsComposition("subject:Basic.Identifier", "code");
assertSearchReturnsComposition("subject:Basic.Identifier", "http://example.org/identifiersystem|code");

// system-only token search is not working for chained searches yet (https://github.com/IBM/FHIR/issues/2553)
// assertSearchReturnsComposition("subject:Basic.Identifier", "http://example.org/identifiersystem|");
assertSearchReturnsComposition("subject:Basic.Identifier", "http://example.org/identifiersystem|");

// This shouldn't return any results because the Identifier has a system
assertSearchDoesntReturnComposition("subject:Basic.Identifier", "|code");
Expand Down
16 changes: 16 additions & 0 deletions ...nce/src/test/java/com/ibm/fhir/persistence/search/test/AbstractWholeSystemSearchTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -226,6 +226,14 @@ public void testSearchAllUsingSecurity() throws Exception {
assertTrue(isResourceInResponse(savedResource, resources), "Expected resource not found in the response");
}

@Test
public void testSearchAllUsingSecuritySystemOnly() throws Exception {
List<Resource> resources = runQueryTest(Resource.class, "_security", SECURITY_SYSTEM + "|");
assertNotNull(resources);
assertEquals(resources.size(), 1, "Number of resources returned");
assertTrue(isResourceInResponse(savedResource, resources), "Expected resource not found in the response");
}

@Test
public void testSearchAllUsingSource() throws Exception {
List<Resource> resources = runQueryTest(Resource.class, "_source", SOURCE);
Expand Down Expand Up @@ -282,6 +290,14 @@ public void testSearchAllUsingTagModifierBelow() throws Exception {
assertTrue(isResourceInResponse(savedResource, resources), "Expected resource not found in the response");
}

@Test
public void testSearchAllUsingTagSystemOnly() throws Exception {
List<Resource> resources = runQueryTest(Resource.class, "_tag", TAG_SYSTEM2 + "|");
assertNotNull(resources);
assertEquals(resources.size(), 1, "Number of resources returned");
assertTrue(isResourceInResponse(savedResource, resources), "Expected resource not found in the response");
}

@Test
public void testSearchAllUsingProfile() throws Exception {
List<Resource> resources = runQueryTest(Resource.class, "_profile", PROFILE);
Expand Down
Loading

0 comments on commit 9900736

Please sign in to comment.