Resolve 5029 Token params with system and value longer than maximum length should be created and searched differently (#4998)

* Partial implementation after this has been paused:  Move trimming of value and system from BaseSearchParamExtractor to ResourceIndexedSearchParamToken.  Do not throw Exceptions in TokenPredicateBuilder.  Ensure that the hash computing in the search param token takes the 200+ token value into account.  Other search param tokens are not yet implemented.

* in ResourceIndexedSearchParamToken,changed hardcoded length to constant MAX_LENGTH;
in BaseSearchParamExtractor, added check in createTokenIndexIfNotBlank so that the return value is null if system or value is blank.

* added test for TokenPredicateBuilder to verify that long values exceeds the max length does not cause exception.

added test in FhirSystemDaoR4SearchTest to verify searchParamToken with longer system and value than max length can be created successfully. And hash value will be calculated before truncation

* fixed a logical error in previous commits

* changed existing tests to accommodate for the changes

* modified existing tests

* added changelog

* changed truncation method name and added comments

Co-authored-by: michaelabuckley <michaelabuckley@gmail.com>

* modified changelog

Co-authored-by: michaelabuckley <michaelabuckley@gmail.com>

* combined redundant tests

---------

Co-authored-by: tyner <tyner.guo@smilecdr.com>
Co-authored-by: TynerGjs <132295567+TynerGjs@users.noreply.github.com>
Co-authored-by: michaelabuckley <michaelabuckley@gmail.com>

hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_8_0/5029-token-params-with-system-and-value-longer-than-maximum-length-should-be-created-and-searched-differently.yaml

@@ -0,0 +1,5 @@
+---
+type: change
+issue: 5029
+title: "Searching for token parameters has been extended to systems and values of any length.  Sorting by tokens is still limited to the first 200 characters each of the system and value.
+valid request."

hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/predicate/TokenPredicateBuilder.java

@@ -32,6 +32,7 @@ import ca.uhn.fhir.context.support.ValueSetExpansionOptions;
 import ca.uhn.fhir.i18n.Msg;
 import ca.uhn.fhir.interceptor.model.RequestPartitionId;
 import ca.uhn.fhir.jpa.api.config.JpaStorageSettings;
+import ca.uhn.fhir.jpa.dao.BaseHapiFhirResourceDao;
 import ca.uhn.fhir.jpa.dao.predicate.SearchFilterParser;
 import ca.uhn.fhir.jpa.model.entity.BaseResourceIndexedSearchParam;
 import ca.uhn.fhir.jpa.model.entity.ResourceIndexedSearchParamToken;
@@ -70,6 +71,7 @@ import static org.apache.commons.lang3.StringUtils.isBlank;
 import static org.apache.commons.lang3.StringUtils.isNotBlank;
 
 public class TokenPredicateBuilder extends BaseSearchParamPredicateBuilder {
+	private static final org.slf4j.Logger ourLog = org.slf4j.LoggerFactory.getLogger(TokenPredicateBuilder.class);
 
 	private final DbColumn myColumnResId;
 	private final DbColumn myColumnHashSystemAndValue;
@@ -160,11 +162,11 @@ public class TokenPredicateBuilder extends BaseSearchParamPredicateBuilder {
 			}
 
 			if (system != null && system.length() > ResourceIndexedSearchParamToken.MAX_LENGTH) {
-				throw new InvalidRequestException(Msg.code(1237) + "Parameter[" + paramName + "] has system (" + system.length() + ") that is longer than maximum allowed (" + ResourceIndexedSearchParamToken.MAX_LENGTH + "): " + system);
+				ourLog.info("Parameter[{}] has system ({}) that is longer than maximum ({}) so will truncate: {} ", paramName, system.length(), ResourceIndexedSearchParamToken.MAX_LENGTH, system);
 			}
 
 			if (code != null && code.length() > ResourceIndexedSearchParamToken.MAX_LENGTH) {
-				throw new InvalidRequestException(Msg.code(1238) + "Parameter[" + paramName + "] has code (" + code.length() + ") that is longer than maximum allowed (" + ResourceIndexedSearchParamToken.MAX_LENGTH + "): " + code);
+				ourLog.info("Parameter[{}] has code ({}) that is longer than maximum ({}) so will truncate: {} ", paramName, code.length(), ResourceIndexedSearchParamToken.MAX_LENGTH, code);
 			}
 
 			/*

hapi-fhir-jpaserver-model/src/main/java/ca/uhn/fhir/jpa/model/entity/ResourceIndexedSearchParamToken.java

@@ -42,6 +42,7 @@ import javax.persistence.Id;
 import javax.persistence.Index;
 import javax.persistence.JoinColumn;
 import javax.persistence.ManyToOne;
+import javax.persistence.PrePersist;
 import javax.persistence.SequenceGenerator;
 import javax.persistence.Table;
 
@@ -370,4 +371,14 @@ public class ResourceIndexedSearchParamToken extends BaseResourceIndexedSearchPa
 		setResourceType(theResource.getResourceType());
 		return this;
 	}
+
+	@PrePersist
+/**
+ * We truncate the fields at the last moment because the tables have limited size.
+ * We don't truncate earlier in the flow because the index hashes MUST be calculated on the full string.
+ */
+	public void truncateFieldsForDB() {
+		mySystem = StringUtils.truncate(mySystem, MAX_LENGTH);
+		myValue = StringUtils.truncate(myValue, MAX_LENGTH);
+	}
 }

hapi-fhir-jpaserver-searchparam/src/main/java/ca/uhn/fhir/jpa/searchparam/extractor/BaseSearchParamExtractor.java

@@ -1471,20 +1471,10 @@ public abstract class BaseSearchParamExtractor implements ISearchParamExtractor
 	}
 
 	private ResourceIndexedSearchParamToken createTokenIndexIfNotBlank(String theResourceType, String theSystem, String theValue, String searchParamName) {
-		String system = theSystem;
-		String value = theValue;
 		ResourceIndexedSearchParamToken nextEntity = null;
-		if (isNotBlank(system) || isNotBlank(value)) {
+		if (isNotBlank(theSystem) || isNotBlank(theValue)) {
-			if (system != null && system.length() > ResourceIndexedSearchParamToken.MAX_LENGTH) {
+			nextEntity = new ResourceIndexedSearchParamToken(myPartitionSettings, theResourceType, searchParamName, theSystem, theValue);
-				system = system.substring(0, ResourceIndexedSearchParamToken.MAX_LENGTH);
-			}
-			if (value != null && value.length() > ResourceIndexedSearchParamToken.MAX_LENGTH) {
-				value = value.substring(0, ResourceIndexedSearchParamToken.MAX_LENGTH);
-			}
-
-			nextEntity = new ResourceIndexedSearchParamToken(myPartitionSettings, theResourceType, searchParamName, system, value);
 		}
-
 		return nextEntity;
 	}
 

hapi-fhir-jpaserver-test-dstu2/src/test/java/ca/uhn/fhir/jpa/dao/dstu2/FhirResourceDaoDstu2Test.java

@@ -9,6 +9,7 @@ import ca.uhn.fhir.jpa.dao.DaoTestUtils;
 import ca.uhn.fhir.jpa.dao.data.IForcedIdDao;
 import ca.uhn.fhir.jpa.model.dao.JpaPid;
 import ca.uhn.fhir.jpa.model.entity.ResourceIndexedSearchParamString;
+import ca.uhn.fhir.jpa.model.entity.ResourceIndexedSearchParamToken;
 import ca.uhn.fhir.jpa.model.entity.TagTypeEnum;
 import ca.uhn.fhir.jpa.searchparam.SearchParamConstants;
 import ca.uhn.fhir.jpa.searchparam.SearchParameterMap;
@@ -2769,8 +2770,8 @@ public class FhirResourceDaoDstu2Test extends BaseJpaDstu2Test {
 	@Test
 	public void testTokenParamWhichIsTooLong() {
 
-		String longStr1 = RandomStringUtils.randomAlphanumeric(ResourceIndexedSearchParamString.MAX_LENGTH + 100);
+		String longStr1 = RandomStringUtils.randomAlphanumeric(ResourceIndexedSearchParamToken.MAX_LENGTH + 100);
-		String longStr2 = RandomStringUtils.randomAlphanumeric(ResourceIndexedSearchParamString.MAX_LENGTH + 100);
+		String longStr2 = RandomStringUtils.randomAlphanumeric(ResourceIndexedSearchParamToken.MAX_LENGTH + 100);
 
 		Organization org = new Organization();
 		org.getNameElement().setValue("testTokenParamWhichIsTooLong");
@@ -2784,21 +2785,10 @@ public class FhirResourceDaoDstu2Test extends BaseJpaDstu2Test {
 		myOrganizationDao.create(org, mySrd);
 
 		val = myOrganizationDao.searchForIds(new SearchParameterMap("type", new IdentifierDt(subStr1, subStr2)), null);
+		assertEquals(initial, val.size());
+
+		val = myOrganizationDao.searchForIds(new SearchParameterMap("type", new IdentifierDt(longStr1, longStr2)), null);
 		assertEquals(initial + 1, val.size());
-
-		try {
-			myOrganizationDao.searchForIds(new SearchParameterMap("type", new IdentifierDt(longStr1, subStr2)), null);
-			fail();
-		} catch (InvalidRequestException e) {
-			// ok
-		}
-
-		try {
-			myOrganizationDao.searchForIds(new SearchParameterMap("type", new IdentifierDt(subStr1, longStr2)), null);
-			fail();
-		} catch (InvalidRequestException e) {
-			// ok
-		}
 	}
 
 	@Test

hapi-fhir-jpaserver-test-dstu3/src/test/java/ca/uhn/fhir/jpa/dao/dstu3/FhirResourceDaoDstu3Test.java

@@ -13,6 +13,7 @@ import ca.uhn.fhir.jpa.entity.ResourceSearchView;
 import ca.uhn.fhir.jpa.model.dao.JpaPid;
 import ca.uhn.fhir.jpa.model.entity.ResourceHistoryTable;
 import ca.uhn.fhir.jpa.model.entity.ResourceIndexedSearchParamString;
+import ca.uhn.fhir.jpa.model.entity.ResourceIndexedSearchParamToken;
 import ca.uhn.fhir.jpa.model.entity.ResourceTable;
 import ca.uhn.fhir.jpa.model.entity.TagTypeEnum;
 import ca.uhn.fhir.jpa.searchparam.SearchParamConstants;
@@ -3431,8 +3432,8 @@ public class FhirResourceDaoDstu3Test extends BaseJpaDstu3Test {
 	@Test
 	public void testTokenParamWhichIsTooLong() {
 
-		String longStr1 = RandomStringUtils.randomAlphanumeric(ResourceIndexedSearchParamString.MAX_LENGTH + 100);
+		String longStr1 = RandomStringUtils.randomAlphanumeric(ResourceIndexedSearchParamToken.MAX_LENGTH + 100);
-		String longStr2 = RandomStringUtils.randomAlphanumeric(ResourceIndexedSearchParamString.MAX_LENGTH + 100);
+		String longStr2 = RandomStringUtils.randomAlphanumeric(ResourceIndexedSearchParamToken.MAX_LENGTH + 100);
 
 		Organization org = new Organization();
 		org.getNameElement().setValue("testTokenParamWhichIsTooLong");
@@ -3446,21 +3447,10 @@ public class FhirResourceDaoDstu3Test extends BaseJpaDstu3Test {
 		myOrganizationDao.create(org, mySrd);
 
 		val = myOrganizationDao.searchForIds(new SearchParameterMap("type", new TokenParam(subStr1, subStr2)), null);
+		assertEquals(initial, val.size());
+
+		val = myOrganizationDao.searchForIds(new SearchParameterMap("type", new TokenParam(longStr1, longStr2)), null);
 		assertEquals(initial + 1, val.size());
-
-		try {
-			myOrganizationDao.searchForIds(new SearchParameterMap("type", new TokenParam(longStr1, subStr2)), null);
-			fail();
-		} catch (InvalidRequestException e) {
-			// ok
-		}
-
-		try {
-			myOrganizationDao.searchForIds(new SearchParameterMap("type", new TokenParam(subStr1, longStr2)), null);
-			fail();
-		} catch (InvalidRequestException e) {
-			// ok
-		}
 	}
 
 	@Test

hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4Test.java

@@ -14,6 +14,7 @@ import ca.uhn.fhir.jpa.model.entity.NormalizedQuantitySearchLevel;
 import ca.uhn.fhir.jpa.model.entity.ResourceEncodingEnum;
 import ca.uhn.fhir.jpa.model.entity.ResourceHistoryTable;
 import ca.uhn.fhir.jpa.model.entity.ResourceIndexedSearchParamString;
+import ca.uhn.fhir.jpa.model.entity.ResourceIndexedSearchParamToken;
 import ca.uhn.fhir.jpa.model.entity.ResourceTable;
 import ca.uhn.fhir.jpa.model.entity.TagTypeEnum;
 import ca.uhn.fhir.jpa.model.search.SearchStatusEnum;
@@ -4154,8 +4155,8 @@ public class FhirResourceDaoR4Test extends BaseJpaR4Test {
 	@Test
 	public void testTokenParamWhichIsTooLong() {
 
-		String longStr1 = RandomStringUtils.randomAlphanumeric(ResourceIndexedSearchParamString.MAX_LENGTH + 100);
+		String longStr1 = RandomStringUtils.randomAlphanumeric(ResourceIndexedSearchParamToken.MAX_LENGTH + 100);
-		String longStr2 = RandomStringUtils.randomAlphanumeric(ResourceIndexedSearchParamString.MAX_LENGTH + 100);
+		String longStr2 = RandomStringUtils.randomAlphanumeric(ResourceIndexedSearchParamToken.MAX_LENGTH + 100);
 
 		Organization org = new Organization();
 		org.getNameElement().setValue("testTokenParamWhichIsTooLong");
@@ -4168,22 +4169,13 @@ public class FhirResourceDaoR4Test extends BaseJpaR4Test {
 
 		myOrganizationDao.create(org, mySrd);
 
+		// search should have 0 result since it is hashed before truncation
 		val = myOrganizationDao.searchForIds(new SearchParameterMap("type", new TokenParam(subStr1, subStr2)), null);
+		assertEquals(initial, val.size());
+
+		// search using the original string should success
+		val = myOrganizationDao.searchForIds(new SearchParameterMap("type", new TokenParam(longStr1, longStr2)), null);
 		assertEquals(initial + 1, val.size());
-
-		try {
-			myOrganizationDao.searchForIds(new SearchParameterMap("type", new TokenParam(longStr1, subStr2)), null);
-			fail();
-		} catch (InvalidRequestException e) {
-			// ok
-		}
-
-		try {
-			myOrganizationDao.searchForIds(new SearchParameterMap("type", new TokenParam(subStr1, longStr2)), null);
-			fail();
-		} catch (InvalidRequestException e) {
-			// ok
-		}
 	}
 
 	@Test

hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirSystemDaoR4SearchTest.java

@@ -1,13 +1,76 @@
 package ca.uhn.fhir.jpa.dao.r4;
 
+import ca.uhn.fhir.interceptor.model.RequestPartitionId;
+import ca.uhn.fhir.jpa.model.config.PartitionSettings;
+import ca.uhn.fhir.jpa.model.entity.ResourceIndexedSearchParamToken;
+import org.apache.commons.lang3.RandomStringUtils;
+import org.eclipse.jetty.util.StringUtil;
+import org.hl7.fhir.r4.model.Identifier;
+import org.hl7.fhir.r4.model.Organization;
 import org.junit.jupiter.api.Test;
 
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
 public class FhirSystemDaoR4SearchTest extends BaseJpaR4SystemTest {
 
+	@Test
+	public void testSearchParameter_WithIdentifierValueUnderMaxLength_ShouldSuccess() {
+		// setup
+		final String identifierSystem = "http://www.acme.org.au/units";
+		final String identifierValue = "identifierValue";
+		final Identifier identifier = new Identifier().setSystem(identifierSystem).setValue(identifierValue);
+		final Organization organization = new Organization().addIdentifier(identifier);
+
+		// execute
+		myOrganizationDao.create(organization, mySrd);
+
+		// verify token value
+		final ResourceIndexedSearchParamToken resultSearchTokens = myResourceIndexedSearchParamTokenDao.findAll().get(0);
+		final String tokenSystem = resultSearchTokens.getSystem();
+		final String tokenValue = resultSearchTokens.getValue();
+
+		assertEquals(identifierSystem, tokenSystem);
+		assertEquals(identifierValue, tokenValue);
+
+		// verify hash
+		final long tokensHashSystem = resultSearchTokens.getHashSystem();
+		final long expectedHashSystem = ResourceIndexedSearchParamToken.calculateHashValue(new PartitionSettings(), RequestPartitionId.defaultPartition(), "Organization", "identifier", identifierSystem);
+		final long tokensHashValue = resultSearchTokens.getHashValue();
+		final long expectedHashValue = ResourceIndexedSearchParamToken.calculateHashValue(new PartitionSettings(), RequestPartitionId.defaultPartition(), "Organization", "identifier", identifierValue);
+
+		assertEquals(expectedHashSystem, tokensHashSystem);
+		assertEquals(expectedHashValue, tokensHashValue);
+	}
 
 	@Test
-	public void testSearchByParans() {
+	public void testSearchParameter_WithIdentifierSystemAndValueOverMaxLength_ShouldHashBeforeTruncate() {
-		// code to come.. just here to prevent a failure 
+		// setup
+		final String identifierSystem = RandomStringUtils.randomAlphanumeric(ResourceIndexedSearchParamToken.MAX_LENGTH + 100);
+		final String identifierValue = RandomStringUtils.randomAlphanumeric(ResourceIndexedSearchParamToken.MAX_LENGTH + 100);
+		final Identifier identifier = new Identifier().setSystem(identifierSystem).setValue(identifierValue);
+		final Organization organization = new Organization().addIdentifier(identifier);
+
+		// execute
+		myOrganizationDao.create(organization, mySrd);
+
+		// verify token value
+		final ResourceIndexedSearchParamToken resultSearchTokens = myResourceIndexedSearchParamTokenDao.findAll().get(0);
+		final String tokenSystem = resultSearchTokens.getSystem();
+		final String tokenValue = resultSearchTokens.getValue();
+		final String expectedSystem = StringUtil.truncate(identifierSystem, ResourceIndexedSearchParamToken.MAX_LENGTH);
+		final String expectedValue = StringUtil.truncate(identifierValue, ResourceIndexedSearchParamToken.MAX_LENGTH);
+
+		assertEquals(expectedSystem, tokenSystem);
+		assertEquals(expectedValue, tokenValue);
+
+		// verify hash
+		final long tokensHashSystem = resultSearchTokens.getHashSystem();
+		final long expectedHashSystem = ResourceIndexedSearchParamToken.calculateHashValue(new PartitionSettings(), RequestPartitionId.defaultPartition(), "Organization", "identifier", identifierSystem);
+		final long tokensHashValue = resultSearchTokens.getHashValue();
+		final long expectedHashValue = ResourceIndexedSearchParamToken.calculateHashValue(new PartitionSettings(), RequestPartitionId.defaultPartition(), "Organization", "identifier", identifierValue);
+
+		assertEquals(expectedHashSystem, tokensHashSystem);
+		assertEquals(expectedHashValue, tokensHashValue);
 	}
 	
 	/*//@formatter:off

hapi-fhir-jpaserver-test-utilities/src/test/java/ca/uhn/fhir/jpa/search/builder/predicate/TokenPredicateBuilderTest.java

@@ -0,0 +1,89 @@
+package ca.uhn.fhir.jpa.search.builder.predicate;
+
+import ca.uhn.fhir.context.RuntimeSearchParam;
+import ca.uhn.fhir.interceptor.model.RequestPartitionId;
+import ca.uhn.fhir.jpa.model.config.PartitionSettings;
+import ca.uhn.fhir.jpa.model.entity.ResourceIndexedSearchParamToken;
+import ca.uhn.fhir.jpa.search.builder.sql.SearchQueryBuilder;
+import ca.uhn.fhir.model.api.IQueryParameterType;
+import ca.uhn.fhir.rest.param.TokenParam;
+import ch.qos.logback.classic.Level;
+import ch.qos.logback.classic.Logger;
+import ch.qos.logback.classic.LoggerContext;
+import ch.qos.logback.classic.spi.ILoggingEvent;
+import ch.qos.logback.core.read.ListAppender;
+import com.healthmarketscience.sqlbuilder.dbspec.basic.DbSchema;
+import com.healthmarketscience.sqlbuilder.dbspec.basic.DbSpec;
+import com.healthmarketscience.sqlbuilder.dbspec.basic.DbTable;
+import org.apache.commons.lang3.RandomStringUtils;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.mockito.Mock;
+import org.mockito.Mockito;
+import org.mockito.junit.jupiter.MockitoExtension;
+import org.slf4j.LoggerFactory;
+
+import java.util.ArrayList;
+import java.util.List;
+
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.containsString;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.mockito.Mockito.when;
+
+@ExtendWith(MockitoExtension.class)
+public class TokenPredicateBuilderTest {
+
+	private Logger myLogger;
+
+	private ListAppender<ILoggingEvent> myListAppender;
+
+	private TokenPredicateBuilder myTokenPredicateBuilder;
+
+	@Mock
+	private SearchQueryBuilder mySearchQueryBuilder;
+
+	@BeforeEach
+	public void beforeEach() {
+		myLogger = (Logger) LoggerFactory.getLogger(TokenPredicateBuilder.class);
+		myListAppender = new ListAppender<>();
+		myListAppender.setContext((LoggerContext) LoggerFactory.getILoggerFactory());
+		myLogger.setLevel(Level.ALL);
+		myLogger.addAppender(myListAppender);
+		myListAppender.start();
+
+		DbSpec spec = new DbSpec();
+		DbSchema schema = new DbSchema(spec, "schema");
+		DbTable table = new DbTable(schema, "table");
+		when(mySearchQueryBuilder.addTable(Mockito.anyString())).thenReturn(table);
+		when(mySearchQueryBuilder.getPartitionSettings()).thenReturn(new PartitionSettings());
+		myTokenPredicateBuilder = new TokenPredicateBuilder(mySearchQueryBuilder);
+	}
+
+	@AfterEach
+	public void afterEach(){
+		myListAppender.stop();
+		myLogger.detachAppender(myListAppender);
+	}
+
+	@Test
+	public void testCreatePredicateToken_withParameterHasSystemLengthGreaterThanMax_ShouldLogAndNotThrowException() {
+		// setup
+		ArrayList<IQueryParameterType> tokenParams = new ArrayList<>();
+		String theLongSystem = RandomStringUtils.randomAlphanumeric(ResourceIndexedSearchParamToken.MAX_LENGTH + 100);
+		String theLongValue = RandomStringUtils.randomAlphanumeric(ResourceIndexedSearchParamToken.MAX_LENGTH + 100);
+		tokenParams.add(new TokenParam().setSystem(theLongSystem).setValue(theLongValue));
+		RuntimeSearchParam runtimeSearchParam = new RuntimeSearchParam(null, null, "SearchParameter", null, null, null, null, null, null, null);
+
+		// execute
+		myTokenPredicateBuilder.createPredicateToken(tokenParams, "SearchParameter", "SPPrefix", runtimeSearchParam, RequestPartitionId.defaultPartition());
+
+		// verify
+		List<ILoggingEvent> logList = myListAppender.list;
+		assertEquals(2, logList.size());
+		assertThat(logList.get(0).getFormattedMessage(), containsString(theLongSystem));
+		assertThat(logList.get(1).getFormattedMessage(), containsString(theLongValue));
+	}
+}