From fdd82e0b2a1c5324838900fe410a847f234f4f18 Mon Sep 17 00:00:00 2001 From: James Agnew Date: Thu, 4 May 2017 05:51:02 -0400 Subject: [PATCH] Shorten column names in Search table for JPA --- .../ca/uhn/fhir/jpa/dao/BaseHapiFhirDao.java | 6 + .../BaseResourceIndexedSearchParam.java | 10 +- .../java/ca/uhn/fhir/jpa/entity/Search.java | 4 +- .../ca/uhn/fhir/jpa/entity/SearchParam.java | 3 + .../fhir/jpa/entity/SearchParamPresent.java | 7 + .../jpa/search/SearchCoordinatorSvcImpl.java | 21 +- .../jpa/sp/SearchParamPresenceSvcImpl.java | 3 +- .../java/ca/uhn/fhir/jpa/util/TestUtil.java | 98 +++ .../fhir/jpa/config/IdentifierLengthTest.java | 16 + .../jpa/dao/dstu2/FhirSystemDaoDstu2Test.java | 16 + .../dstu3/ResourceProviderDstu3Test.java | 22 + .../src/test/resources/bug638.xml | 651 ++++++++++++++++++ 12 files changed, 841 insertions(+), 16 deletions(-) create mode 100644 hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/util/TestUtil.java create mode 100644 hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/config/IdentifierLengthTest.java create mode 100644 hapi-fhir-jpaserver-base/src/test/resources/bug638.xml diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseHapiFhirDao.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseHapiFhirDao.java index 3aa1773f9f6..6d8694a60a7 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseHapiFhirDao.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseHapiFhirDao.java @@ -1539,6 +1539,12 @@ public abstract class BaseHapiFhirDao implements IDao { /* * Update the "search param present" table which is used for the * ?foo:missing=true queries + * + * Note that we're only populating this for reference params + * because the index tables for all other types have a MISSING column + * right on them for handling the :missing queries. We can't use the + * index table for resource links (reference indexes) because we index + * those by path and not by parameter name. */ if (thePerformIndexing) { Map presentSearchParams = new HashMap(); diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/entity/BaseResourceIndexedSearchParam.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/entity/BaseResourceIndexedSearchParam.java index 4113fc3e5ec..c01fa62e1b4 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/entity/BaseResourceIndexedSearchParam.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/entity/BaseResourceIndexedSearchParam.java @@ -23,15 +23,8 @@ package ca.uhn.fhir.jpa.entity; import java.io.Serializable; import java.util.Date; -import javax.persistence.Column; -import javax.persistence.JoinColumn; -import javax.persistence.ManyToOne; -import javax.persistence.MappedSuperclass; -import javax.persistence.Temporal; -import javax.persistence.TemporalType; +import javax.persistence.*; -import org.hibernate.annotations.ColumnDefault; -import org.hibernate.annotations.Columns; import org.hibernate.search.annotations.ContainedIn; import org.hibernate.search.annotations.Field; @@ -44,7 +37,6 @@ public abstract class BaseResourceIndexedSearchParam implements Serializable { @Field() @Column(name = "SP_MISSING", nullable = true) - @ColumnDefault("false") private boolean myMissing; @Field diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/entity/Search.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/entity/Search.java index 330acb1529c..dfbc745c0b9 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/entity/Search.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/entity/Search.java @@ -42,8 +42,8 @@ import ca.uhn.fhir.rest.param.DateRangeParam; @Table(name = "HFJ_SEARCH", uniqueConstraints= { @UniqueConstraint(name="IDX_SEARCH_UUID", columnNames="SEARCH_UUID") }, indexes= { - @Index(name="JDX_SEARCH_LASTRETURNED", columnList="SEARCH_LAST_RETURNED"), - @Index(name="JDX_SEARCH_RESTYPE_STRINGHASHCREATED", columnList="RESOURCE_TYPE,SEARCH_QUERY_STRING_HASH,CREATED") + @Index(name="IDX_SEARCH_LASTRETURNED", columnList="SEARCH_LAST_RETURNED"), + @Index(name="IDX_SEARCH_RESTYPE_HASHS", columnList="RESOURCE_TYPE,SEARCH_QUERY_STRING_HASH,CREATED") }) public class Search implements Serializable { diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/entity/SearchParam.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/entity/SearchParam.java index d119f0a3d12..d03175ada02 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/entity/SearchParam.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/entity/SearchParam.java @@ -52,5 +52,8 @@ public class SearchParam { myResourceName = theResourceName; } + public Long getId() { + return myId; + } } diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/entity/SearchParamPresent.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/entity/SearchParamPresent.java index df005e572ad..ef6cbcacff7 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/entity/SearchParamPresent.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/entity/SearchParamPresent.java @@ -54,6 +54,13 @@ public class SearchParamPresent implements Serializable { @JoinColumn(name = "SP_ID", referencedColumnName = "PID", nullable = false, foreignKey = @ForeignKey(name = "FK_RESPARMPRES_SPID")) private SearchParam mySearchParam; + /** + * Constructor + */ + public SearchParamPresent() { + super(); + } + public ResourceTable getResource() { return myResource; } diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/SearchCoordinatorSvcImpl.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/SearchCoordinatorSvcImpl.java index 0aec79fb1e3..e3562f430eb 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/SearchCoordinatorSvcImpl.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/SearchCoordinatorSvcImpl.java @@ -80,9 +80,7 @@ import ca.uhn.fhir.model.api.Include; import ca.uhn.fhir.rest.method.PageMethodBinding; import ca.uhn.fhir.rest.server.IBundleProvider; import ca.uhn.fhir.rest.server.SimpleBundleProvider; -import ca.uhn.fhir.rest.server.exceptions.BaseServerResponseException; -import ca.uhn.fhir.rest.server.exceptions.InternalErrorException; -import ca.uhn.fhir.rest.server.exceptions.ResourceGoneException; +import ca.uhn.fhir.rest.server.exceptions.*; public class SearchCoordinatorSvcImpl implements ISearchCoordinatorSvc { static final int DEFAULT_SYNC_SIZE = 250; @@ -475,7 +473,22 @@ public class SearchCoordinatorSvcImpl implements ISearchCoordinatorSvc { ourLog.info("Completed search for {} resources in {}ms", mySyncedPids.size(), sw.getMillis()); } catch (Throwable t) { - ourLog.error("Failed during search loading after {}ms", sw.getMillis(), t); + + /* + * Don't print a stack trace for client errors.. that's just noisy + */ + boolean logged = false; + if (t instanceof BaseServerResponseException) { + BaseServerResponseException exception = (BaseServerResponseException) t; + if (exception.getStatusCode() >= 400 && exception.getStatusCode() < 500) { + logged = true; + ourLog.warn("Failed during search due to invalid request: {}", t.toString()); + } + } + + if (!logged) { + ourLog.error("Failed during search loading after {}ms", sw.getMillis(), t); + } myUnsyncedPids.clear(); Throwable rootCause = ExceptionUtils.getRootCause(t); diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/sp/SearchParamPresenceSvcImpl.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/sp/SearchParamPresenceSvcImpl.java index 3812454fec6..04dc881203a 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/sp/SearchParamPresenceSvcImpl.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/sp/SearchParamPresenceSvcImpl.java @@ -82,7 +82,8 @@ public class SearchParamPresenceSvcImpl implements ISearchParamPresenceSvc { searchParam = new SearchParam(); searchParam.setResourceName(resourceType); searchParam.setParamName(paramName); - mySearchParamDao.save(searchParam); + searchParam = mySearchParamDao.saveAndFlush(searchParam); + ourLog.info("Added search param {} with pid {}", paramName, searchParam.getId()); // Don't add the newly saved entity to the map in case the save fails } } diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/util/TestUtil.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/util/TestUtil.java new file mode 100644 index 00000000000..3cd2bcaebb4 --- /dev/null +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/util/TestUtil.java @@ -0,0 +1,98 @@ +package ca.uhn.fhir.jpa.util; + +import java.io.IOException; +import java.lang.reflect.AnnotatedElement; +import java.lang.reflect.Field; + +import javax.persistence.*; + +import org.apache.commons.lang3.Validate; + +import com.google.common.collect.ImmutableSet; +import com.google.common.reflect.ClassPath; +import com.google.common.reflect.ClassPath.ClassInfo; + +import ca.uhn.fhir.rest.server.exceptions.InternalErrorException; + +public class TestUtil { + private static final int MAX_LENGTH = 30; + private static final org.slf4j.Logger ourLog = org.slf4j.LoggerFactory.getLogger(TestUtil.class); + + /** non instantiable */ + private TestUtil() { + super(); + } + + /** + * This is really only useful for unit tests, do not call otherwise + */ + public static void scanEntities(String packageName) throws IOException, ClassNotFoundException { + ImmutableSet classes = ClassPath.from(TestUtil.class.getClassLoader()).getTopLevelClasses(packageName); + + if (classes.size() <= 1) { + throw new InternalErrorException("Found no classes"); + } + + for (ClassInfo classInfo : classes) { + Class clazz = Class.forName(classInfo.getName()); + Entity entity = clazz.getAnnotation(Entity.class); + if (entity == null) { + continue; + } + + ourLog.info("Scanning: {}", clazz.getSimpleName()); + + scan(clazz); + + for (Field nextField : clazz.getFields()) { + scan(nextField); + } + + } + } + + private static void scan(AnnotatedElement ae) { + Table table = ae.getAnnotation(Table.class); + if (table != null) { + assertThat(table.name()); + for (UniqueConstraint nextConstraint : table.uniqueConstraints()) { + assertThat(nextConstraint.name()); + } + for (Index nextConstraint : table.indexes()) { + assertThat(nextConstraint.name()); + } + } + + JoinColumn joinColumn = ae.getAnnotation(JoinColumn.class); + if (joinColumn != null) { + assertThat(joinColumn.name()); + ForeignKey fk = joinColumn.foreignKey(); + assertThat(fk.name()); + } + + Column column = ae.getAnnotation(Column.class); + if (column != null) { + assertThat(column.name()); + } + + GeneratedValue gen = ae.getAnnotation(GeneratedValue.class); + if (gen != null) { + assertThat(gen.generator()); + SequenceGenerator sg = ae.getAnnotation(SequenceGenerator.class); + assertThat(sg.name()); + assertThat(sg.sequenceName()); + assertEquals(gen.generator(), sg.name()); + assertEquals(gen.generator(), sg.sequenceName()); + } + + } + + private static void assertEquals(String theGenerator, String theName) { + Validate.isTrue(theGenerator.equals(theName)); + } + + private static void assertThat(String theName) { + Validate.isTrue(theName.length() <= MAX_LENGTH, "Identifier \"" + theName + "\" is " + theName.length() + " chars long"); + } + +} diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/config/IdentifierLengthTest.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/config/IdentifierLengthTest.java new file mode 100644 index 00000000000..2c97da80840 --- /dev/null +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/config/IdentifierLengthTest.java @@ -0,0 +1,16 @@ +package ca.uhn.fhir.jpa.config; + +import org.junit.Test; + +import ca.uhn.fhir.jpa.entity.ResourceTable; +import ca.uhn.fhir.jpa.util.TestUtil; + +public class IdentifierLengthTest { + + @Test + public void testIdentifierLength() throws Exception { + TestUtil.scanEntities(ResourceTable.class.getPackage().getName()); + } + + +} diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/dstu2/FhirSystemDaoDstu2Test.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/dstu2/FhirSystemDaoDstu2Test.java index 4c867b74941..33a3b6d610d 100644 --- a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/dstu2/FhirSystemDaoDstu2Test.java +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/dstu2/FhirSystemDaoDstu2Test.java @@ -25,6 +25,7 @@ import java.util.List; import org.apache.commons.io.IOUtils; import org.hl7.fhir.dstu3.model.IdType; +import org.hl7.fhir.dstu3.model.Bundle.BundleEntryComponent; import org.hl7.fhir.instance.model.api.IIdType; import org.junit.AfterClass; import org.junit.Test; @@ -75,6 +76,21 @@ public class FhirSystemDaoDstu2Test extends BaseJpaDstu2SystemTest { private static final org.slf4j.Logger ourLog = org.slf4j.LoggerFactory.getLogger(FhirSystemDaoDstu2Test.class); + /** + * See #638 + */ + @Test + public void testTransactionBug638() throws Exception { + String input = loadClasspath("/bug638.xml"); + Bundle request = myFhirCtx.newXmlParser().parseResource(Bundle.class, input); + + Bundle resp = mySystemDao.transaction(mySrd, request); + + ourLog.info(myFhirCtx.newXmlParser().setPrettyPrint(true).encodeResourceToString(resp)); + + assertEquals(18, resp.getEntry().size()); + } + /** * Per a message on the mailing list */ diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/dstu3/ResourceProviderDstu3Test.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/dstu3/ResourceProviderDstu3Test.java index cf5573d0b5b..1968268f136 100644 --- a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/dstu3/ResourceProviderDstu3Test.java +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/dstu3/ResourceProviderDstu3Test.java @@ -3094,6 +3094,28 @@ public class ResourceProviderDstu3Test extends BaseResourceProviderDstu3Test { checkParamMissing(Observation.SP_DATE); } + @Test + public void testSearchInvalidParam() throws Exception { + Patient patient = new Patient(); + patient.addIdentifier().setSystem("urn:system").setValue("0"); + patient.addName().setFamily("testSearchWithMixedParams").addGiven("Joe"); + myPatientDao.create(patient, mySrd).getId().toUnqualifiedVersionless(); + + // should be subject._id + HttpGet httpPost = new HttpGet(ourServerBase + "/Observation?subject.id=FOO"); + + CloseableHttpResponse resp = ourHttpClient.execute(httpPost); + try { + String respString = IOUtils.toString(resp.getEntity().getContent(), StandardCharsets.UTF_8); + ourLog.info(respString); + assertThat(respString, containsString("Invalid parameter chain: subject.id")); + assertEquals(400, resp.getStatusLine().getStatusCode()); + } finally { + IOUtils.closeQuietly(resp.getEntity().getContent()); + } + ourLog.info("Outgoing post: {}", httpPost); + } + /** * See #411 * diff --git a/hapi-fhir-jpaserver-base/src/test/resources/bug638.xml b/hapi-fhir-jpaserver-base/src/test/resources/bug638.xml new file mode 100644 index 00000000000..2766984f932 --- /dev/null +++ b/hapi-fhir-jpaserver-base/src/test/resources/bug638.xml @@ -0,0 +1,651 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + +