From 1e45506526e15bee8ebc7e892807404b5b4b532c Mon Sep 17 00:00:00 2001 From: James Agnew Date: Fri, 18 Aug 2023 12:30:00 -0400 Subject: [PATCH] Improve ID filtering (#5215) * Improve ID filtering * Formatting * Add license header * Address review comments --- .../7_0_0/5215-improve-hfql-id-filtering.yaml | 5 ++ .../models/ResolvedSearchQueryExecutor.java | 19 ++++++++ .../fhir/jpa/fql/executor/HfqlExecutor.java | 48 +++++++++++++++++++ .../fhir/jpa/fql/parser/HfqlStatement.java | 6 +++ .../jpa/fql/executor/HfqlExecutorTest.java | 45 ++++++++++++++++- .../main/java/ca/uhn/fhir/to/Controller.java | 2 +- .../main/webapp/WEB-INF/templates/hfql.html | 13 +---- .../templates/tmpl-hfql-banner-card.html | 12 +++++ 8 files changed, 136 insertions(+), 14 deletions(-) create mode 100644 hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_0_0/5215-improve-hfql-id-filtering.yaml create mode 100644 hapi-fhir-testpage-overlay/src/main/webapp/WEB-INF/templates/tmpl-hfql-banner-card.html diff --git a/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_0_0/5215-improve-hfql-id-filtering.yaml b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_0_0/5215-improve-hfql-id-filtering.yaml new file mode 100644 index 00000000000..00153b26474 --- /dev/null +++ b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_0_0/5215-improve-hfql-id-filtering.yaml @@ -0,0 +1,5 @@ +--- +type: perf +issue: 5215 +title: "When executing an HFQL search with a FHIRPath filter on the `id` element, this will now + be automatically converted into an `_id` search parameter match for better performance." diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/models/ResolvedSearchQueryExecutor.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/models/ResolvedSearchQueryExecutor.java index 44fa3993f63..62a19e3b89b 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/models/ResolvedSearchQueryExecutor.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/models/ResolvedSearchQueryExecutor.java @@ -1,3 +1,22 @@ +/*- + * #%L + * HAPI FHIR JPA Server + * %% + * Copyright (C) 2014 - 2023 Smile CDR, Inc. + * %% + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * #L% + */ package ca.uhn.fhir.jpa.search.builder.models; import ca.uhn.fhir.jpa.search.builder.ISearchQueryExecutor; diff --git a/hapi-fhir-jpaserver-hfql/src/main/java/ca/uhn/fhir/jpa/fql/executor/HfqlExecutor.java b/hapi-fhir-jpaserver-hfql/src/main/java/ca/uhn/fhir/jpa/fql/executor/HfqlExecutor.java index 4edb5ec95f3..424c713f90a 100644 --- a/hapi-fhir-jpaserver-hfql/src/main/java/ca/uhn/fhir/jpa/fql/executor/HfqlExecutor.java +++ b/hapi-fhir-jpaserver-hfql/src/main/java/ca/uhn/fhir/jpa/fql/executor/HfqlExecutor.java @@ -42,11 +42,13 @@ import ca.uhn.fhir.rest.api.server.IBundleProvider; import ca.uhn.fhir.rest.api.server.RequestDetails; import ca.uhn.fhir.rest.param.DateOrListParam; import ca.uhn.fhir.rest.param.DateParam; +import ca.uhn.fhir.rest.param.ParameterUtil; import ca.uhn.fhir.rest.param.QualifierDetails; import ca.uhn.fhir.rest.param.TokenOrListParam; import ca.uhn.fhir.rest.server.IPagingProvider; import ca.uhn.fhir.rest.server.exceptions.InvalidRequestException; import ca.uhn.fhir.rest.server.util.ISearchParamRegistry; +import ca.uhn.fhir.rest.server.util.ResourceSearchParams; import ca.uhn.fhir.util.UrlUtil; import com.google.common.collect.Lists; import org.apache.commons.collections4.ListUtils; @@ -142,6 +144,8 @@ public class HfqlExecutor implements IHfqlExecutor { massageSelectColumnNames(statement); populateSelectColumnDataTypes(statement); + validateWhereClauses(statement); + massageWhereClauses(statement); SearchParameterMap map = new SearchParameterMap(); addHfqlWhereClausesToSearchParameterMap(statement, map); @@ -179,6 +183,39 @@ public class HfqlExecutor implements IHfqlExecutor { return executionResult; } + private void validateWhereClauses(HfqlStatement theStatement) { + for (HfqlStatement.WhereClause next : theStatement.getWhereClauses()) { + if (isDataValueWhereClause(next)) { + if (next.getLeft().matches("^[a-zA-Z]+$")) { + RuntimeResourceDefinition resDef = + myFhirContext.getResourceDefinition(theStatement.getFromResourceName()); + if (resDef.getChildByName(next.getLeft()) == null) { + throw new InvalidRequestException( + Msg.code(2429) + "Resource type " + theStatement.getFromResourceName() + + " does not have a root element named '" + next.getLeft() + "'"); + } + } + } + } + } + + private void massageWhereClauses(HfqlStatement theStatement) { + ResourceSearchParams activeSearchParams = + mySearchParamRegistry.getActiveSearchParams(theStatement.getFromResourceName()); + + for (HfqlStatement.WhereClause nextWhereClause : theStatement.getWhereClauses()) { + if (isDataValueWhereClause(nextWhereClause)) { + if ("id".equals(nextWhereClause.getLeft())) { + nextWhereClause.setOperator(HfqlStatement.WhereClauseOperatorEnum.SEARCH_MATCH); + String joinedParamValues = nextWhereClause.getRightAsStrings().stream() + .map(ParameterUtil::escape) + .collect(Collectors.joining(",")); + nextWhereClause.setRight("_id", joinedParamValues); + } + } + } + } + private void addHfqlWhereClausesToSearchParameterMap(HfqlStatement statement, SearchParameterMap map) { List searchClauses = statement.getWhereClauses(); for (HfqlStatement.WhereClause nextSearchClause : searchClauses) { @@ -740,6 +777,17 @@ public class HfqlExecutor implements IHfqlExecutor { return new StaticHfqlExecutionResult(null, columns, dataTypes, rows); } + /** + * Returns {@literal true} if a where clause has an operator of + * {@link ca.uhn.fhir.jpa.fql.parser.HfqlStatement.WhereClauseOperatorEnum#EQUALS} + * or + * {@link ca.uhn.fhir.jpa.fql.parser.HfqlStatement.WhereClauseOperatorEnum#IN} + */ + private static boolean isDataValueWhereClause(HfqlStatement.WhereClause next) { + return next.getOperator() == HfqlStatement.WhereClauseOperatorEnum.EQUALS + || next.getOperator() == HfqlStatement.WhereClauseOperatorEnum.IN; + } + @SuppressWarnings("unchecked") static Comparator newRowComparator(int columnIndex, HfqlDataTypeEnum dataType) { return Comparator.comparing(new RowValueExtractor(columnIndex, dataType)); diff --git a/hapi-fhir-jpaserver-hfql/src/main/java/ca/uhn/fhir/jpa/fql/parser/HfqlStatement.java b/hapi-fhir-jpaserver-hfql/src/main/java/ca/uhn/fhir/jpa/fql/parser/HfqlStatement.java index a6419017c3a..333c8d2fa94 100644 --- a/hapi-fhir-jpaserver-hfql/src/main/java/ca/uhn/fhir/jpa/fql/parser/HfqlStatement.java +++ b/hapi-fhir-jpaserver-hfql/src/main/java/ca/uhn/fhir/jpa/fql/parser/HfqlStatement.java @@ -26,6 +26,7 @@ import com.fasterxml.jackson.annotation.JsonInclude; import com.fasterxml.jackson.annotation.JsonProperty; import java.util.ArrayList; +import java.util.Arrays; import java.util.List; import java.util.stream.Collectors; import javax.annotation.Nonnull; @@ -303,6 +304,11 @@ public class HfqlStatement implements IModelJson { return myRight; } + public void setRight(String... theValues) { + myRight.clear(); + myRight.addAll(Arrays.asList(theValues)); + } + public void addRight(String theRight) { myRight.add(theRight); } diff --git a/hapi-fhir-jpaserver-hfql/src/test/java/ca/uhn/fhir/jpa/fql/executor/HfqlExecutorTest.java b/hapi-fhir-jpaserver-hfql/src/test/java/ca/uhn/fhir/jpa/fql/executor/HfqlExecutorTest.java index 36645b4c3ef..5136c116ad0 100644 --- a/hapi-fhir-jpaserver-hfql/src/test/java/ca/uhn/fhir/jpa/fql/executor/HfqlExecutorTest.java +++ b/hapi-fhir-jpaserver-hfql/src/test/java/ca/uhn/fhir/jpa/fql/executor/HfqlExecutorTest.java @@ -924,7 +924,7 @@ public class HfqlExecutorTest { } @Test - public void testWhere_Id_In_CommaList() { + public void testWhere_Id_In_CommaList_SearchMatch() { IFhirResourceDao patientDao = initDao(Observation.class); Observation resource = new Observation(); resource.getMeta().setVersionId("5"); @@ -961,6 +961,34 @@ public class HfqlExecutorTest { assertEquals("Patient/456", ((TokenParam) map.get("_id").get(0).get(1)).getValue()); } + @Test + public void testWhere_FhirPathElevatedToSearchParam_Id_Equals() { + IFhirResourceDao patientDao = initDao(Patient.class); + when(patientDao.search(any(), any())).thenReturn(createProviderWithSomeSimpsonsAndFlanders()); + + String statement = """ + select id + from Patient + where id IN ('HOMER0', 'HOMER1') + """; + + IHfqlExecutionResult result = myHfqlExecutor.executeInitialSearch(statement, null, mySrd); + + assertTrue(result.hasNext()); + List nextRow = result.getNextRow().getRowValues(); + assertEquals("HOMER0", nextRow.get(0)); + + verify(patientDao, times(1)).search(mySearchParameterMapCaptor.capture(), any()); + SearchParameterMap map = mySearchParameterMapCaptor.getValue(); + assertEquals(1, map.get("_id").size()); + assertEquals(2, map.get("_id").get(0).size()); + assertNull(((TokenParam) map.get("_id").get(0).get(0)).getSystem()); + assertEquals("HOMER0", ((TokenParam) map.get("_id").get(0).get(0)).getValue()); + assertNull(((TokenParam) map.get("_id").get(0).get(1)).getSystem()); + assertEquals("HOMER1", ((TokenParam) map.get("_id").get(0).get(1)).getValue()); + } + + @Test public void testSearch_QualifiedSelect() { IFhirResourceDao patientDao = initDao(Patient.class); @@ -1211,6 +1239,20 @@ public class HfqlExecutorTest { assertErrorMessage(result, "HAPI-2413: search_match function requires 2 arguments"); } + @Test + public void testError_InvalidWhereParameter() { + initDao(Patient.class); + + String input = """ + select name.family + from Patient + where Blah = '123' + """; + + IHfqlExecutionResult result = myHfqlExecutor.executeInitialSearch(input, null, mySrd); + assertErrorMessage(result, "HAPI-2429: Resource type Patient does not have a root element named 'Blah'"); + } + @SuppressWarnings("unchecked") private IFhirResourceDao initDao(Class theType) { IFhirResourceDao retVal = mock(IFhirResourceDao.class); @@ -1325,6 +1367,7 @@ public class HfqlExecutorTest { @Nonnull private static Patient createPatientHomerSimpson() { Patient homer = new Patient(); + homer.setId("HOMER0"); homer.getMeta().setVersionId("2"); homer.addName().setFamily("Simpson").addGiven("Homer").addGiven("Jay"); homer.addIdentifier().setSystem("http://system").setValue("value0"); diff --git a/hapi-fhir-testpage-overlay/src/main/java/ca/uhn/fhir/to/Controller.java b/hapi-fhir-testpage-overlay/src/main/java/ca/uhn/fhir/to/Controller.java index 9a980b0586b..eed593d3075 100644 --- a/hapi-fhir-testpage-overlay/src/main/java/ca/uhn/fhir/to/Controller.java +++ b/hapi-fhir-testpage-overlay/src/main/java/ca/uhn/fhir/to/Controller.java @@ -384,7 +384,7 @@ public class Controller extends BaseController { final ModelMap theModel) { addCommonParamsForHfql(theServletRequest, theRequest, theModel); - ourLog.info("Executing HFQL query: {}", theHfqlQuery); + ourLog.info("Executing HFQL query: {}", theHfqlQuery.replaceAll("\\s+", " ")); StopWatch sw = new StopWatch(); List> rows = new ArrayList<>(); diff --git a/hapi-fhir-testpage-overlay/src/main/webapp/WEB-INF/templates/hfql.html b/hapi-fhir-testpage-overlay/src/main/webapp/WEB-INF/templates/hfql.html index 0f6ec7d7149..8d9692192a8 100644 --- a/hapi-fhir-testpage-overlay/src/main/webapp/WEB-INF/templates/hfql.html +++ b/hapi-fhir-testpage-overlay/src/main/webapp/WEB-INF/templates/hfql.html @@ -41,16 +41,7 @@
-
-

HFQL / SQL Execution

-
- This page can be used to execute queries using the HAPI FHIR Query Language (HFQL), - which is a SQL-like syntax for querying FHIR repositories. Learn more about - the HFQL syntax at: - https://smilecdr.com/docs/hfql/. - This UI will display a maximum of [[${rowLimit}]] rows. -
-
+
@@ -71,8 +62,6 @@