From d5fc780226f5304f3e97937b57bb97b5e18e5cef Mon Sep 17 00:00:00 2001 From: Vadim Peretokin Date: Mon, 26 Jun 2017 11:57:00 +0200 Subject: [PATCH 01/28] Added link to model API STU3 --- src/site/xdoc/docindex.xml | 1 + 1 file changed, 1 insertion(+) diff --git a/src/site/xdoc/docindex.xml b/src/site/xdoc/docindex.xml index cb7d6a7bf48..49cefb6481c 100644 --- a/src/site/xdoc/docindex.xml +++ b/src/site/xdoc/docindex.xml @@ -69,6 +69,7 @@
  • Core API
  • Model API (DSTU1)
  • Model API (DSTU2)
    • +
  • Model API (STU3)
  • JPA Server API
    • From d26fba161eedecb1d5224547ea60f8e2adb1d15c Mon Sep 17 00:00:00 2001 From: "michael.i.calderero" Date: Mon, 3 Jul 2017 16:06:52 -0500 Subject: [PATCH 02/28] remove '[x]' from paths in @ChildOrder annotation because they mess up 'forcedOrder' resolution in BaseRuntimeElementCompositeDefinition.scanCompositeElementForChildren() method --- .../fhir/dstu3/model/ActivityDefinition.java | 3 +- .../org/hl7/fhir/dstu3/model/ConceptMap.java | 3 +- .../uhn/fhir/parser/JsonParserDstu3Test.java | 77 ++++++++++++++++++ .../uhn/fhir/parser/XmlParserDstu3Test.java | 78 +++++++++++++++++++ 4 files changed, 159 insertions(+), 2 deletions(-) diff --git a/hapi-fhir-structures-dstu3/src/main/java/org/hl7/fhir/dstu3/model/ActivityDefinition.java b/hapi-fhir-structures-dstu3/src/main/java/org/hl7/fhir/dstu3/model/ActivityDefinition.java index 6859533a3bb..4e7ce7988cc 100644 --- a/hapi-fhir-structures-dstu3/src/main/java/org/hl7/fhir/dstu3/model/ActivityDefinition.java +++ b/hapi-fhir-structures-dstu3/src/main/java/org/hl7/fhir/dstu3/model/ActivityDefinition.java @@ -47,7 +47,8 @@ import org.hl7.fhir.exceptions.FHIRException; * This resource allows for the definition of some activity to be performed, independent of a particular patient, practitioner, or other performance context. */ @ResourceDef(name="ActivityDefinition", profile="http://hl7.org/fhir/Profile/ActivityDefinition") -@ChildOrder(names={"url", "identifier", "version", "name", "title", "status", "experimental", "date", "publisher", "description", "purpose", "usage", "approvalDate", "lastReviewDate", "effectivePeriod", "useContext", "jurisdiction", "topic", "contributor", "contact", "copyright", "relatedArtifact", "library", "kind", "code", "timing[x]", "location", "participant", "product[x]", "quantity", "dosage", "bodySite", "transform", "dynamicValue"}) +//@ChildOrder(names={"url", "identifier", "version", "name", "title", "status", "experimental", "date", "publisher", "description", "purpose", "usage", "approvalDate", "lastReviewDate", "effectivePeriod", "useContext", "jurisdiction", "topic", "contributor", "contact", "copyright", "relatedArtifact", "library", "kind", "code", "timing[x]", "location", "participant", "product[x]", "quantity", "dosage", "bodySite", "transform", "dynamicValue"}) +@ChildOrder(names={"url", "identifier", "version", "name", "title", "status", "experimental", "date", "publisher", "description", "purpose", "usage", "approvalDate", "lastReviewDate", "effectivePeriod", "useContext", "jurisdiction", "topic", "contributor", "contact", "copyright", "relatedArtifact", "library", "kind", "code", "timing", "location", "participant", "product", "quantity", "dosage", "bodySite", "transform", "dynamicValue"}) public class ActivityDefinition extends MetadataResource { public enum ActivityDefinitionKind { diff --git a/hapi-fhir-structures-dstu3/src/main/java/org/hl7/fhir/dstu3/model/ConceptMap.java b/hapi-fhir-structures-dstu3/src/main/java/org/hl7/fhir/dstu3/model/ConceptMap.java index ebb30d41722..7133530ea7a 100644 --- a/hapi-fhir-structures-dstu3/src/main/java/org/hl7/fhir/dstu3/model/ConceptMap.java +++ b/hapi-fhir-structures-dstu3/src/main/java/org/hl7/fhir/dstu3/model/ConceptMap.java @@ -47,7 +47,8 @@ import org.hl7.fhir.exceptions.FHIRException; * A statement of relationships from one set of concepts to one or more other concepts - either code systems or data elements, or classes in class models. */ @ResourceDef(name="ConceptMap", profile="http://hl7.org/fhir/Profile/ConceptMap") -@ChildOrder(names={"url", "identifier", "version", "name", "title", "status", "experimental", "date", "publisher", "contact", "description", "useContext", "jurisdiction", "purpose", "copyright", "source[x]", "target[x]", "group"}) +//@ChildOrder(names={"url", "identifier", "version", "name", "title", "status", "experimental", "date", "publisher", "contact", "description", "useContext", "jurisdiction", "purpose", "copyright", "source[x]", "target[x]", "group"}) +@ChildOrder(names={"url", "identifier", "version", "name", "title", "status", "experimental", "date", "publisher", "contact", "description", "useContext", "jurisdiction", "purpose", "copyright", "source", "target", "group"}) public class ConceptMap extends MetadataResource { public enum ConceptMapGroupUnmappedMode { diff --git a/hapi-fhir-structures-dstu3/src/test/java/ca/uhn/fhir/parser/JsonParserDstu3Test.java b/hapi-fhir-structures-dstu3/src/test/java/ca/uhn/fhir/parser/JsonParserDstu3Test.java index 8ffee00417e..ab3e464b5d7 100644 --- a/hapi-fhir-structures-dstu3/src/test/java/ca/uhn/fhir/parser/JsonParserDstu3Test.java +++ b/hapi-fhir-structures-dstu3/src/test/java/ca/uhn/fhir/parser/JsonParserDstu3Test.java @@ -36,6 +36,8 @@ import java.util.UUID; import org.apache.commons.io.IOUtils; import org.hamcrest.Matchers; import org.hamcrest.core.StringContains; +import org.hl7.fhir.dstu3.hapi.validation.DefaultProfileValidationSupport; +import org.hl7.fhir.dstu3.hapi.validation.FhirInstanceValidator; import org.hl7.fhir.dstu3.model.*; import org.hl7.fhir.dstu3.model.Address.AddressUse; import org.hl7.fhir.dstu3.model.Address.AddressUseEnumFactory; @@ -71,6 +73,9 @@ import ca.uhn.fhir.parser.json.JsonLikeValue.ValueType; import ca.uhn.fhir.rest.server.Constants; import ca.uhn.fhir.util.TestUtil; import ca.uhn.fhir.validation.FhirValidator; +import ca.uhn.fhir.validation.IValidationContext; +import ca.uhn.fhir.validation.SingleValidationMessage; +import ca.uhn.fhir.validation.ValidationContext; import ca.uhn.fhir.validation.ValidationResult; import net.sf.json.JSON; import net.sf.json.JSONSerializer; @@ -84,6 +89,78 @@ public class JsonParserDstu3Test { public void after() { ourCtx.setNarrativeGenerator(null); } + + @Test + public void testActivityDefinitionElementsOrder() throws Exception { + final String origContent = "{\"resourceType\":\"ActivityDefinition\",\"id\":\"x1\",\"url\":\"http://testing.org\",\"status\":\"draft\",\"timingDateTime\":\"2011-02-03\"}"; + final IParser parser = ourCtx.newJsonParser(); + DefaultProfileValidationSupport validationSupport = new DefaultProfileValidationSupport(); + + // verify that InstanceValidator likes the format + { + IValidationContext validationCtx = ValidationContext.forText(ourCtx, origContent); + new FhirInstanceValidator(validationSupport).validateResource(validationCtx); + ValidationResult result = validationCtx.toResult(); + for (SingleValidationMessage msg : result.getMessages()) { + ourLog.info("{}", msg); + } + Assert.assertEquals(0, result.getMessages().size()); + } + + ActivityDefinition fhirObj = parser.parseResource(ActivityDefinition.class, origContent); + String content = parser.encodeResourceToString(fhirObj); + ourLog.info("Serialized form: {}", content); + + // verify that InstanceValidator still likes the format + { + IValidationContext validationCtx = ValidationContext.forText(ourCtx, content); + new FhirInstanceValidator(validationSupport).validateResource(validationCtx); + ValidationResult result = validationCtx.toResult(); + for (SingleValidationMessage msg : result.getMessages()) { + ourLog.info("{}", msg); + } + Assert.assertEquals(0, result.getMessages().size()); + } + + // verify that the original and newly serialized match + Assert.assertEquals(origContent, content); + } + + @Test + public void testConceptMapElementsOrder() throws Exception { + final String origContent = "{\"resourceType\":\"ConceptMap\",\"id\":\"x1\",\"url\":\"http://testing.org\",\"status\":\"draft\",\"sourceUri\":\"http://y1\"}"; + final IParser parser = ourCtx.newJsonParser(); + DefaultProfileValidationSupport validationSupport = new DefaultProfileValidationSupport(); + + // verify that InstanceValidator likes the format + { + IValidationContext validationCtx = ValidationContext.forText(ourCtx, origContent); + new FhirInstanceValidator(validationSupport).validateResource(validationCtx); + ValidationResult result = validationCtx.toResult(); + for (SingleValidationMessage msg : result.getMessages()) { + ourLog.info("{}", msg); + } + Assert.assertEquals(0, result.getMessages().size()); + } + + ConceptMap fhirObj = parser.parseResource(ConceptMap.class, origContent); + String content = parser.encodeResourceToString(fhirObj); + ourLog.info("Serialized form: {}", content); + + // verify that InstanceValidator still likes the format + { + IValidationContext validationCtx = ValidationContext.forText(ourCtx, content); + new FhirInstanceValidator(validationSupport).validateResource(validationCtx); + ValidationResult result = validationCtx.toResult(); + for (SingleValidationMessage msg : result.getMessages()) { + ourLog.info("{}", msg); + } + Assert.assertEquals(0, result.getMessages().size()); + } + + // verify that the original and newly serialized match + Assert.assertEquals(origContent, content); + } /** * See #563 diff --git a/hapi-fhir-structures-dstu3/src/test/java/ca/uhn/fhir/parser/XmlParserDstu3Test.java b/hapi-fhir-structures-dstu3/src/test/java/ca/uhn/fhir/parser/XmlParserDstu3Test.java index e782d8913c1..5ededaf9f1d 100644 --- a/hapi-fhir-structures-dstu3/src/test/java/ca/uhn/fhir/parser/XmlParserDstu3Test.java +++ b/hapi-fhir-structures-dstu3/src/test/java/ca/uhn/fhir/parser/XmlParserDstu3Test.java @@ -34,6 +34,8 @@ import org.custommonkey.xmlunit.XMLUnit; import org.hamcrest.collection.IsEmptyCollection; import org.hamcrest.core.StringContains; import org.hamcrest.text.StringContainsInOrder; +import org.hl7.fhir.dstu3.hapi.validation.DefaultProfileValidationSupport; +import org.hl7.fhir.dstu3.hapi.validation.FhirInstanceValidator; import org.hl7.fhir.dstu3.model.*; import org.hl7.fhir.dstu3.model.Address.AddressUse; import org.hl7.fhir.dstu3.model.Address.AddressUseEnumFactory; @@ -67,6 +69,10 @@ import ca.uhn.fhir.parser.PatientWithCustomCompositeExtension.FooParentExtension import ca.uhn.fhir.rest.client.IGenericClient; import ca.uhn.fhir.rest.server.Constants; import ca.uhn.fhir.util.TestUtil; +import ca.uhn.fhir.validation.IValidationContext; +import ca.uhn.fhir.validation.SingleValidationMessage; +import ca.uhn.fhir.validation.ValidationContext; +import ca.uhn.fhir.validation.ValidationResult; public class XmlParserDstu3Test { private static FhirContext ourCtx = FhirContext.forDstu3(); @@ -79,6 +85,78 @@ public class XmlParserDstu3Test { } ourCtx.setNarrativeGenerator(null); } + + @Test + public void testActivityDefinitionElementsOrder() throws Exception { + final String origContent = ""; + final IParser parser = ourCtx.newXmlParser(); + DefaultProfileValidationSupport validationSupport = new DefaultProfileValidationSupport(); + + // verify that InstanceValidator likes the format + { + IValidationContext validationCtx = ValidationContext.forText(ourCtx, origContent); + new FhirInstanceValidator(validationSupport).validateResource(validationCtx); + ValidationResult result = validationCtx.toResult(); + for (SingleValidationMessage msg : result.getMessages()) { + ourLog.info("{}", msg); + } + Assert.assertEquals(0, result.getMessages().size()); + } + + ActivityDefinition fhirObj = parser.parseResource(ActivityDefinition.class, origContent); + String content = parser.encodeResourceToString(fhirObj); + ourLog.info("Serialized form: {}", content); + + // verify that InstanceValidator still likes the format + { + IValidationContext validationCtx = ValidationContext.forText(ourCtx, content); + new FhirInstanceValidator(validationSupport).validateResource(validationCtx); + ValidationResult result = validationCtx.toResult(); + for (SingleValidationMessage msg : result.getMessages()) { + ourLog.info("{}", msg); + } + Assert.assertEquals(0, result.getMessages().size()); + } + + // verify that the original and newly serialized match + Assert.assertEquals(origContent, content); + } + + @Test + public void testConceptMapElementsOrder() throws Exception { + final String origContent = ""; + final IParser parser = ourCtx.newXmlParser(); + DefaultProfileValidationSupport validationSupport = new DefaultProfileValidationSupport(); + + // verify that InstanceValidator likes the format + { + IValidationContext validationCtx = ValidationContext.forText(ourCtx, origContent); + new FhirInstanceValidator(validationSupport).validateResource(validationCtx); + ValidationResult result = validationCtx.toResult(); + for (SingleValidationMessage msg : result.getMessages()) { + ourLog.info("{}", msg); + } + Assert.assertEquals(0, result.getMessages().size()); + } + + ConceptMap fhirObj = parser.parseResource(ConceptMap.class, origContent); + String content = parser.encodeResourceToString(fhirObj); + ourLog.info("Serialized form: {}", content); + + // verify that InstanceValidator still likes the format + { + IValidationContext validationCtx = ValidationContext.forText(ourCtx, content); + new FhirInstanceValidator(validationSupport).validateResource(validationCtx); + ValidationResult result = validationCtx.toResult(); + for (SingleValidationMessage msg : result.getMessages()) { + ourLog.info("{}", msg); + } + Assert.assertEquals(0, result.getMessages().size()); + } + + // verify that the original and newly serialized match + Assert.assertEquals(origContent, content); + } /** * See #544 From a0f3e8c717f74669e500e1e3c73194555b76405d Mon Sep 17 00:00:00 2001 From: Eugene Lubarsky Date: Thu, 13 Jul 2017 01:03:05 +1000 Subject: [PATCH 03/28] CLI: command-line option to turn off or configure reuse of search results --- .../ca/uhn/fhir/cli/RunServerCommand.java | 27 ++++++++++++++++++- .../ca/uhn/fhir/jpa/demo/ContextHolder.java | 13 +++++++++ .../ca/uhn/fhir/jpa/demo/JpaServerDemo.java | 2 +- 3 files changed, 40 insertions(+), 2 deletions(-) diff --git a/hapi-fhir-cli/hapi-fhir-cli-app/src/main/java/ca/uhn/fhir/cli/RunServerCommand.java b/hapi-fhir-cli/hapi-fhir-cli-app/src/main/java/ca/uhn/fhir/cli/RunServerCommand.java index 2e70ad9ad9e..7e89d0d816f 100644 --- a/hapi-fhir-cli/hapi-fhir-cli-app/src/main/java/ca/uhn/fhir/cli/RunServerCommand.java +++ b/hapi-fhir-cli/hapi-fhir-cli-app/src/main/java/ca/uhn/fhir/cli/RunServerCommand.java @@ -21,6 +21,7 @@ import org.springframework.web.context.ContextLoader; import org.springframework.web.context.ContextLoaderListener; import org.springframework.web.context.support.AnnotationConfigWebApplicationContext; +import ca.uhn.fhir.jpa.dao.DaoConfig; import ca.uhn.fhir.jpa.demo.ContextHolder; import ca.uhn.fhir.jpa.demo.FhirServerConfig; import ca.uhn.fhir.jpa.demo.FhirServerConfigDstu3; @@ -30,6 +31,7 @@ public class RunServerCommand extends BaseCommand { private static final String OPTION_DISABLE_REFERENTIAL_INTEGRITY = "disable-referential-integrity"; private static final String OPTION_LOWMEM = "lowmem"; private static final String OPTION_ALLOW_EXTERNAL_REFS = "allow-external-refs"; + private static final String OPTION_REUSE_SEARCH_RESULTS_MILLIS = "reuse-search-results-milliseconds"; private static final int DEFAULT_PORT = 8080; private static final String OPTION_P = "p"; @@ -51,6 +53,10 @@ public class RunServerCommand extends BaseCommand { options.addOption(null, OPTION_LOWMEM, false, "If this flag is set, the server will operate in low memory mode (some features disabled)"); options.addOption(null, OPTION_ALLOW_EXTERNAL_REFS, false, "If this flag is set, the server will allow resources to be persisted contaning external resource references"); options.addOption(null, OPTION_DISABLE_REFERENTIAL_INTEGRITY, false, "If this flag is set, the server will not enforce referential integrity"); + + Long defaultReuseSearchResults = DaoConfig.DEFAULT_REUSE_CACHED_SEARCH_RESULTS_FOR_MILLIS; + String defaultReuseSearchResultsStr = defaultReuseSearchResults == null ? "off" : String.valueOf(defaultReuseSearchResults); + options.addOption(null, OPTION_REUSE_SEARCH_RESULTS_MILLIS, true, "The time in milliseconds within which the same results will be returned for multiple identical searches, or \"off\" (default is " + defaultReuseSearchResultsStr + ")"); return options; } @@ -58,7 +64,7 @@ public class RunServerCommand extends BaseCommand { try { return Integer.parseInt(theCommandLine.getOptionValue(opt, Integer.toString(defaultPort))); } catch (NumberFormatException e) { - throw new ParseException("Invalid value '" + theCommandLine.getOptionValue(opt) + " (must be numeric)"); + throw new ParseException("Invalid value '" + theCommandLine.getOptionValue(opt) + "' (must be numeric)"); } } @@ -81,6 +87,25 @@ public class RunServerCommand extends BaseCommand { ContextHolder.setDisableReferentialIntegrity(true); } + String reuseSearchResults = theCommandLine.getOptionValue(OPTION_REUSE_SEARCH_RESULTS_MILLIS); + if (reuseSearchResults != null) { + if (reuseSearchResults.equals("off")) { + ourLog.info("Server is configured to not reuse search results"); + ContextHolder.setReuseCachedSearchResultsForMillis(null); + } else { + try { + long reuseSearchResultsMillis = Long.parseLong(reuseSearchResults); + if (reuseSearchResultsMillis < 0) { + throw new NumberFormatException("expected a positive integer"); + } + ourLog.info("Server is configured to reuse search results for " + String.valueOf(reuseSearchResultsMillis) + " milliseconds"); + ContextHolder.setReuseCachedSearchResultsForMillis(reuseSearchResultsMillis); + } catch (NumberFormatException e) { + throw new ParseException("Invalid value '" + reuseSearchResults + "' (must be a positive integer)"); + } + } + } + ContextHolder.setCtx(getSpecVersionContext(theCommandLine)); ourLog.info("Preparing HAPI FHIR JPA server on port {}", myPort); diff --git a/hapi-fhir-cli/hapi-fhir-cli-jpaserver/src/main/java/ca/uhn/fhir/jpa/demo/ContextHolder.java b/hapi-fhir-cli/hapi-fhir-cli-jpaserver/src/main/java/ca/uhn/fhir/jpa/demo/ContextHolder.java index 5675424db19..345b3259c9d 100644 --- a/hapi-fhir-cli/hapi-fhir-cli-jpaserver/src/main/java/ca/uhn/fhir/jpa/demo/ContextHolder.java +++ b/hapi-fhir-cli/hapi-fhir-cli-jpaserver/src/main/java/ca/uhn/fhir/jpa/demo/ContextHolder.java @@ -3,6 +3,7 @@ package ca.uhn.fhir.jpa.demo; import org.apache.commons.cli.ParseException; import org.apache.commons.lang3.Validate; +import ca.uhn.fhir.jpa.dao.DaoConfig; import ca.uhn.fhir.context.FhirContext; public class ContextHolder { @@ -11,6 +12,11 @@ public class ContextHolder { private static FhirContext ourCtx; private static boolean ourDisableReferentialIntegrity; private static String ourPath; + private static Long ourReuseSearchResultsMillis; + + static { + ourReuseSearchResultsMillis = DaoConfig.DEFAULT_REUSE_CACHED_SEARCH_RESULTS_FOR_MILLIS; + } public static FhirContext getCtx() { Validate.notNull(ourPath, "Context not set"); @@ -53,4 +59,11 @@ public class ContextHolder { ourDisableReferentialIntegrity = theDisableReferentialIntegrity; } + public static void setReuseCachedSearchResultsForMillis(Long reuseSearchResultsMillis) { + ourReuseSearchResultsMillis = reuseSearchResultsMillis; + } + + public static Long getReuseCachedSearchResultsForMillis() { + return ourReuseSearchResultsMillis; + } } diff --git a/hapi-fhir-cli/hapi-fhir-cli-jpaserver/src/main/java/ca/uhn/fhir/jpa/demo/JpaServerDemo.java b/hapi-fhir-cli/hapi-fhir-cli-jpaserver/src/main/java/ca/uhn/fhir/jpa/demo/JpaServerDemo.java index 18ccf90f5e3..11407999a08 100644 --- a/hapi-fhir-cli/hapi-fhir-cli-jpaserver/src/main/java/ca/uhn/fhir/jpa/demo/JpaServerDemo.java +++ b/hapi-fhir-cli/hapi-fhir-cli-jpaserver/src/main/java/ca/uhn/fhir/jpa/demo/JpaServerDemo.java @@ -170,7 +170,7 @@ public class JpaServerDemo extends RestfulServer { daoConfig.setAllowExternalReferences(ContextHolder.isAllowExternalRefs()); daoConfig.setEnforceReferentialIntegrityOnDelete(!ContextHolder.isDisableReferentialIntegrity()); daoConfig.setEnforceReferentialIntegrityOnWrite(!ContextHolder.isDisableReferentialIntegrity()); - + daoConfig.setReuseCachedSearchResultsForMillis(ContextHolder.getReuseCachedSearchResultsForMillis()); } } From d7714c637257f425ae02d291896dbde6e38d8e2d Mon Sep 17 00:00:00 2001 From: Diederik Muylwyk Date: Thu, 13 Jul 2017 14:05:19 -0400 Subject: [PATCH 04/28] Added a hook so subclasses of BaseController can customize error messages. (#691) --- .../java/ca/uhn/fhir/to/BaseController.java | 81 ++++++++++--------- .../main/java/ca/uhn/fhir/to/Controller.java | 81 +++++++++---------- 2 files changed, 80 insertions(+), 82 deletions(-) diff --git a/hapi-fhir-testpage-overlay/src/main/java/ca/uhn/fhir/to/BaseController.java b/hapi-fhir-testpage-overlay/src/main/java/ca/uhn/fhir/to/BaseController.java index 35ab412e234..422086f283a 100644 --- a/hapi-fhir-testpage-overlay/src/main/java/ca/uhn/fhir/to/BaseController.java +++ b/hapi-fhir-testpage-overlay/src/main/java/ca/uhn/fhir/to/BaseController.java @@ -1,37 +1,5 @@ package ca.uhn.fhir.to; -import static org.apache.commons.lang3.StringUtils.defaultString; - -import java.io.IOException; -import java.io.UnsupportedEncodingException; -import java.net.URLDecoder; -import java.util.ArrayList; -import java.util.Collections; -import java.util.Comparator; -import java.util.HashMap; -import java.util.List; -import java.util.Map; - -import javax.servlet.ServletException; -import javax.servlet.http.HttpServletRequest; - -import org.apache.commons.io.IOUtils; -import org.apache.commons.lang3.StringEscapeUtils; -import org.apache.commons.lang3.StringUtils; -import org.apache.http.Header; -import org.apache.http.entity.ContentType; -import org.apache.http.message.BasicHeader; -import org.hl7.fhir.dstu3.model.CapabilityStatement; -import org.hl7.fhir.dstu3.model.CapabilityStatement.CapabilityStatementRestComponent; -import org.hl7.fhir.dstu3.model.CapabilityStatement.CapabilityStatementRestResourceComponent; -import org.hl7.fhir.dstu3.model.DecimalType; -import org.hl7.fhir.dstu3.model.Extension; -import org.hl7.fhir.instance.model.api.IBaseResource; -import org.hl7.fhir.instance.model.api.IDomainResource; -import org.springframework.beans.factory.annotation.Autowired; -import org.springframework.ui.ModelMap; -import org.thymeleaf.TemplateEngine; - import ca.uhn.fhir.context.FhirContext; import ca.uhn.fhir.context.FhirVersionEnum; import ca.uhn.fhir.context.RuntimeResourceDefinition; @@ -49,6 +17,31 @@ import ca.uhn.fhir.rest.server.Constants; import ca.uhn.fhir.rest.server.EncodingEnum; import ca.uhn.fhir.to.model.HomeRequest; import ca.uhn.fhir.util.ExtensionConstants; +import org.apache.commons.io.IOUtils; +import org.apache.commons.lang3.StringEscapeUtils; +import org.apache.commons.lang3.StringUtils; +import org.apache.http.Header; +import org.apache.http.entity.ContentType; +import org.apache.http.message.BasicHeader; +import org.hl7.fhir.dstu3.model.CapabilityStatement; +import org.hl7.fhir.dstu3.model.CapabilityStatement.CapabilityStatementRestComponent; +import org.hl7.fhir.dstu3.model.CapabilityStatement.CapabilityStatementRestResourceComponent; +import org.hl7.fhir.dstu3.model.DecimalType; +import org.hl7.fhir.dstu3.model.Extension; +import org.hl7.fhir.instance.model.api.IBaseResource; +import org.hl7.fhir.instance.model.api.IDomainResource; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.ui.ModelMap; +import org.thymeleaf.TemplateEngine; + +import javax.servlet.ServletException; +import javax.servlet.http.HttpServletRequest; +import java.io.IOException; +import java.io.UnsupportedEncodingException; +import java.net.URLDecoder; +import java.util.*; + +import static org.apache.commons.lang3.StringUtils.defaultString; public class BaseController { private static final org.slf4j.Logger ourLog = org.slf4j.LoggerFactory.getLogger(BaseController.class); @@ -298,7 +291,7 @@ public class BaseController { ourLog.warn("Failed to invoke server", e); if (e != null) { - theModel.put("errorMsg", "Error: " + e.getMessage()); + theModel.put("errorMsg", toDisplayError("Error: " + e.getMessage(), e)); } return returnsResource; @@ -328,7 +321,7 @@ public class BaseController { conformance = (Conformance) client.conformance(); } catch (Exception e) { ourLog.warn("Failed to load conformance statement", e); - theModel.put("errorMsg", "Failed to load conformance statement, error was: " + e.toString()); + theModel.put("errorMsg", toDisplayError("Failed to load conformance statement, error was: " + e.toString(), e)); conformance = new Conformance(); } @@ -388,7 +381,7 @@ public class BaseController { conformance = (ca.uhn.fhir.model.dstu2.resource.Conformance) client.conformance(); } catch (Exception e) { ourLog.warn("Failed to load conformance statement", e); - theModel.put("errorMsg", "Failed to load conformance statement, error was: " + e.toString()); + theModel.put("errorMsg", toDisplayError("Failed to load conformance statement, error was: " + e.toString(), e)); conformance = new ca.uhn.fhir.model.dstu2.resource.Conformance(); } @@ -448,7 +441,7 @@ public class BaseController { capabilityStatement = client.fetchConformance().ofType(org.hl7.fhir.dstu3.model.CapabilityStatement.class).execute(); } catch (Exception ex) { ourLog.warn("Failed to load conformance statement", ex); - theModel.put("errorMsg", "Failed to load conformance statement, error was: " + ex.toString()); + theModel.put("errorMsg", toDisplayError("Failed to load conformance statement, error was: " + ex.toString(), ex)); } theModel.put("jsonEncodedConf", getContext(theRequest).newJsonParser().encodeResourceToString(capabilityStatement)); @@ -676,7 +669,7 @@ public class BaseController { } catch (Exception e) { ourLog.error("Failure during processing", e); - theModelMap.put("errorMsg", "Error during processing: " + e.getMessage()); + theModelMap.put("errorMsg", toDisplayError("Error during processing: " + e.getMessage(), e)); } } @@ -753,4 +746,16 @@ public class BaseController { BUNDLE, NONE, RESOURCE, TAGLIST } -} \ No newline at end of file + /** + * A hook to be overridden by subclasses. The overriding method can modify the error message + * based on its content and/or the related exception. + * + * @param theErrorMsg The original error message to be displayed to the user. + * @param theException The exception that occurred. May be null. + * @return The modified error message to be displayed to the user. + */ + protected String toDisplayError(String theErrorMsg, Exception theException) { + return theErrorMsg; + } + +} 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 5dce6eaf8b2..dce94412dfc 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 @@ -1,36 +1,5 @@ package ca.uhn.fhir.to; -import static org.apache.commons.lang3.StringUtils.defaultIfEmpty; -import static org.apache.commons.lang3.StringUtils.defaultString; -import static org.apache.commons.lang3.StringUtils.isBlank; -import static org.apache.commons.lang3.StringUtils.isNotBlank; - -import java.io.IOException; -import java.io.StringWriter; -import java.util.ArrayList; -import java.util.Collections; -import java.util.List; -import java.util.TreeSet; - -import javax.servlet.ServletException; -import javax.servlet.http.HttpServletRequest; - -import org.apache.commons.lang3.StringUtils; -import org.hl7.fhir.dstu3.model.CapabilityStatement; -import org.hl7.fhir.dstu3.model.CapabilityStatement.CapabilityStatementRestComponent; -import org.hl7.fhir.dstu3.model.CapabilityStatement.CapabilityStatementRestResourceComponent; -import org.hl7.fhir.dstu3.model.CapabilityStatement.CapabilityStatementRestResourceSearchParamComponent; -import org.hl7.fhir.dstu3.model.CodeType; -import org.hl7.fhir.dstu3.model.StringType; -import org.hl7.fhir.instance.model.api.IBaseBundle; -import org.hl7.fhir.instance.model.api.IBaseConformance; -import org.hl7.fhir.instance.model.api.IBaseResource; -import org.springframework.ui.ModelMap; -import org.springframework.validation.BindingResult; -import org.springframework.web.bind.annotation.RequestMapping; - -import com.google.gson.stream.JsonWriter; - import ca.uhn.fhir.context.FhirContext; import ca.uhn.fhir.context.FhirVersionEnum; import ca.uhn.fhir.context.RuntimeResourceDefinition; @@ -58,6 +27,30 @@ import ca.uhn.fhir.to.model.HomeRequest; import ca.uhn.fhir.to.model.ResourceRequest; import ca.uhn.fhir.to.model.TransactionRequest; import ca.uhn.fhir.util.ExtensionConstants; +import com.google.gson.stream.JsonWriter; +import org.apache.commons.lang3.StringUtils; +import org.hl7.fhir.dstu3.model.CapabilityStatement; +import org.hl7.fhir.dstu3.model.CapabilityStatement.CapabilityStatementRestComponent; +import org.hl7.fhir.dstu3.model.CapabilityStatement.CapabilityStatementRestResourceComponent; +import org.hl7.fhir.dstu3.model.CapabilityStatement.CapabilityStatementRestResourceSearchParamComponent; +import org.hl7.fhir.dstu3.model.StringType; +import org.hl7.fhir.instance.model.api.IBaseBundle; +import org.hl7.fhir.instance.model.api.IBaseConformance; +import org.hl7.fhir.instance.model.api.IBaseResource; +import org.springframework.ui.ModelMap; +import org.springframework.validation.BindingResult; +import org.springframework.web.bind.annotation.RequestMapping; + +import javax.servlet.ServletException; +import javax.servlet.http.HttpServletRequest; +import java.io.IOException; +import java.io.StringWriter; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; +import java.util.TreeSet; + +import static org.apache.commons.lang3.StringUtils.*; @org.springframework.stereotype.Controller() public class Controller extends BaseController { @@ -128,13 +121,13 @@ public class Controller extends BaseController { try { def = getResourceType(theRequest, theReq); } catch (ServletException e) { - theModel.put("errorMsg", e.toString()); + theModel.put("errorMsg", toDisplayError(e.toString(), e)); return "resource"; } String id = StringUtils.defaultString(theReq.getParameter("resource-delete-id")); if (StringUtils.isBlank(id)) { - theModel.put("errorMsg", "No ID specified"); + theModel.put("errorMsg", toDisplayError("No ID specified", null)); return "resource"; } @@ -173,7 +166,7 @@ public class Controller extends BaseController { try { def = getResourceType(theRequest, theReq); } catch (ServletException e) { - theModel.put("errorMsg", e.toString()); + theModel.put("errorMsg", toDisplayError(e.toString(), e)); return "resource"; } @@ -237,7 +230,7 @@ public class Controller extends BaseController { if (myConfig.isRefuseToFetchThirdPartyUrls()) { if (!url.startsWith(theModel.get("base").toString())) { ourLog.warn(logPrefix(theModel) + "Refusing to load page URL: {}", url); - theModel.put("errorMsg", "Invalid page URL: " + url); + theModel.put("errorMsg", toDisplayError("Invalid page URL: " + url, null)); return "result"; } } @@ -279,12 +272,12 @@ public class Controller extends BaseController { try { def = getResourceType(theRequest, theReq); } catch (ServletException e) { - theModel.put("errorMsg", e.toString()); + theModel.put("errorMsg", toDisplayError(e.toString(), e)); return "resource"; } String id = StringUtils.defaultString(theReq.getParameter("id")); if (StringUtils.isBlank(id)) { - theModel.put("errorMsg", "No ID specified"); + theModel.put("errorMsg", toDisplayError("No ID specified", null)); return "resource"; } ResultType returnsResource = ResultType.RESOURCE; @@ -391,7 +384,7 @@ public class Controller extends BaseController { try { query = search.forResource((Class) getResourceType(theRequest, theReq).getImplementingClass()); } catch (ServletException e) { - theModel.put("errorMsg", e.toString()); + theModel.put("errorMsg", toDisplayError(e.toString(), e)); return "resource"; } clientCodeJsonWriter.name("resource"); @@ -463,7 +456,7 @@ public class Controller extends BaseController { String limit = theReq.getParameter("resource-search-limit"); if (isNotBlank(limit)) { if (!limit.matches("[0-9]+")) { - theModel.put("errorMsg", "Search limit must be a numeric value."); + theModel.put("errorMsg", toDisplayError("Search limit must be a numeric value.", null)); return "resource"; } int limitInt = Integer.parseInt(limit); @@ -534,12 +527,12 @@ public class Controller extends BaseController { } else if (body.startsWith("<")) { // XML content } else { - theModel.put("errorMsg", "Message body does not appear to be a valid FHIR resource instance document. Body should start with '<' (for XML encoding) or '{' (for JSON encoding)."); + theModel.put("errorMsg", toDisplayError("Message body does not appear to be a valid FHIR resource instance document. Body should start with '<' (for XML encoding) or '{' (for JSON encoding).", null)); return "home"; } } catch (DataFormatException e) { ourLog.warn("Failed to parse bundle", e); - theModel.put("errorMsg", "Failed to parse transaction bundle body. Error was: " + e.getMessage()); + theModel.put("errorMsg", toDisplayError("Failed to parse transaction bundle body. Error was: " + e.getMessage(), e)); return "home"; } @@ -587,7 +580,7 @@ public class Controller extends BaseController { String body = validate ? theReq.getParameter("resource-validate-body") : theReq.getParameter("resource-create-body"); if (isBlank(body)) { - theModel.put("errorMsg", "No message body specified"); + theModel.put("errorMsg", toDisplayError("No message body specified", null)); return; } @@ -602,12 +595,12 @@ public class Controller extends BaseController { resource = getContext(theRequest).newXmlParser().parseResource(type, body); client.setEncoding(EncodingEnum.XML); } else { - theModel.put("errorMsg", "Message body does not appear to be a valid FHIR resource instance document. Body should start with '<' (for XML encoding) or '{' (for JSON encoding)."); + theModel.put("errorMsg", toDisplayError("Message body does not appear to be a valid FHIR resource instance document. Body should start with '<' (for XML encoding) or '{' (for JSON encoding).", null)); return; } } catch (DataFormatException e) { ourLog.warn("Failed to parse resource", e); - theModel.put("errorMsg", "Failed to parse message body. Error was: " + e.getMessage()); + theModel.put("errorMsg", toDisplayError("Failed to parse message body. Error was: " + e.getMessage(), e)); return; } From d4fcf46dedd6a52429e537c6f188a0b15d734a47 Mon Sep 17 00:00:00 2001 From: Diederik Muylwyk Date: Thu, 13 Jul 2017 18:29:12 -0400 Subject: [PATCH 05/28] Removed annoying stacktrace from logs for failed conformance statements. (#692) * Removed annoying stacktrace from logs for failed conformance statements. * Minor tweak for consistency with slf4j. --- .../src/main/java/ca/uhn/fhir/to/BaseController.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/hapi-fhir-testpage-overlay/src/main/java/ca/uhn/fhir/to/BaseController.java b/hapi-fhir-testpage-overlay/src/main/java/ca/uhn/fhir/to/BaseController.java index 422086f283a..07b25b527d9 100644 --- a/hapi-fhir-testpage-overlay/src/main/java/ca/uhn/fhir/to/BaseController.java +++ b/hapi-fhir-testpage-overlay/src/main/java/ca/uhn/fhir/to/BaseController.java @@ -320,7 +320,7 @@ public class BaseController { try { conformance = (Conformance) client.conformance(); } catch (Exception e) { - ourLog.warn("Failed to load conformance statement", e); + ourLog.warn("Failed to load conformance statement, error was: {}", e.toString()); theModel.put("errorMsg", toDisplayError("Failed to load conformance statement, error was: " + e.toString(), e)); conformance = new Conformance(); } @@ -380,7 +380,7 @@ public class BaseController { try { conformance = (ca.uhn.fhir.model.dstu2.resource.Conformance) client.conformance(); } catch (Exception e) { - ourLog.warn("Failed to load conformance statement", e); + ourLog.warn("Failed to load conformance statement, error was: {}", e.toString()); theModel.put("errorMsg", toDisplayError("Failed to load conformance statement, error was: " + e.toString(), e)); conformance = new ca.uhn.fhir.model.dstu2.resource.Conformance(); } @@ -440,7 +440,7 @@ public class BaseController { try { capabilityStatement = client.fetchConformance().ofType(org.hl7.fhir.dstu3.model.CapabilityStatement.class).execute(); } catch (Exception ex) { - ourLog.warn("Failed to load conformance statement", ex); + ourLog.warn("Failed to load conformance statement, error was: {}", ex.toString()); theModel.put("errorMsg", toDisplayError("Failed to load conformance statement, error was: " + ex.toString(), ex)); } From 3a61f5a3d70c8aa904c3aeb287a27fb4912ae320 Mon Sep 17 00:00:00 2001 From: James Agnew Date: Thu, 13 Jul 2017 19:36:07 -0400 Subject: [PATCH 06/28] Credit for #679 --- pom.xml | 1 + src/changes/changes.xml | 4 ++++ 2 files changed, 5 insertions(+) diff --git a/pom.xml b/pom.xml index a7021cc309f..31cc901c29b 100644 --- a/pom.xml +++ b/pom.xml @@ -275,6 +275,7 @@ vadi2 Vadim Peretokin + Furore Informatica lawley diff --git a/src/changes/changes.xml b/src/changes/changes.xml index c26f5d1e37b..2955ee0df0d 100644 --- a/src/changes/changes.xml +++ b/src/changes/changes.xml @@ -156,6 +156,10 @@ StructureMapUtilities. Thanks to Travis Lukach for reporting and providing a test case! + + Add link to DSTU3 JavaDocs from documentation index. Thanks + to Vadim Peretokin for the pull request! + From e069d344b8841a0c87df1c614e8fa27e5ea88683 Mon Sep 17 00:00:00 2001 From: James Agnew Date: Thu, 13 Jul 2017 19:38:28 -0400 Subject: [PATCH 07/28] Credit for #680 --- pom.xml | 4 ++++ src/changes/changes.xml | 4 ++++ 2 files changed, 8 insertions(+) diff --git a/pom.xml b/pom.xml index 31cc901c29b..707f3e5b436 100644 --- a/pom.xml +++ b/pom.xml @@ -342,6 +342,10 @@ eug48 Eugene Lubarsky + + SarenCurrie + Saren Currie + diff --git a/src/changes/changes.xml b/src/changes/changes.xml index 2955ee0df0d..b1dc1d2d772 100644 --- a/src/changes/changes.xml +++ b/src/changes/changes.xml @@ -160,6 +160,10 @@ Add link to DSTU3 JavaDocs from documentation index. Thanks to Vadim Peretokin for the pull request! + + Fix a typo in the documentation. Thanks to Saren Currie + for the pull request! + From d2153fc4c399798db062119740114033951e4ba9 Mon Sep 17 00:00:00 2001 From: James Date: Fri, 14 Jul 2017 06:06:43 -0400 Subject: [PATCH 08/28] Credit for #689 --- src/changes/changes.xml | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/changes/changes.xml b/src/changes/changes.xml index b1dc1d2d772..e6473a818d5 100644 --- a/src/changes/changes.xml +++ b/src/changes/changes.xml @@ -164,6 +164,11 @@ Fix a typo in the documentation. Thanks to Saren Currie for the pull request! + + Add a command line flag to the CLI tool to allow configuration of the + server search result cache timeout period. Thanks to Eugene Lubarsky + for the pull request! + From 8d5102b4eadd996411bf0ea3e205f83be5228f5d Mon Sep 17 00:00:00 2001 From: James Date: Fri, 14 Jul 2017 06:13:36 -0400 Subject: [PATCH 09/28] Credit for #683 --- src/changes/changes.xml | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/changes/changes.xml b/src/changes/changes.xml index e6473a818d5..92aab8d3dac 100644 --- a/src/changes/changes.xml +++ b/src/changes/changes.xml @@ -169,6 +169,13 @@ server search result cache timeout period. Thanks to Eugene Lubarsky for the pull request! + + Correct an issue with the model classes for STU3 where any classes + containing the @ChildOrder annotation (basically the conformance + resources) will not correctly set the order if any of the + elements are a choice type (i.e. named "foo[x]"). Thanks to + GitHub user @CarthageKing for the pull request! + From f7d4296046fd5e6b20778825744cee66dbffff81 Mon Sep 17 00:00:00 2001 From: James Date: Fri, 14 Jul 2017 06:16:41 -0400 Subject: [PATCH 10/28] Tweak to #683 --- ...BaseRuntimeElementCompositeDefinition.java | 6 +- .../fhir/dstu3/model/ActivityDefinition.java | 3 +- .../uhn/fhir/parser/XmlParserDstu3Test.java | 123 +++++++++--------- 3 files changed, 69 insertions(+), 63 deletions(-) diff --git a/hapi-fhir-base/src/main/java/ca/uhn/fhir/context/BaseRuntimeElementCompositeDefinition.java b/hapi-fhir-base/src/main/java/ca/uhn/fhir/context/BaseRuntimeElementCompositeDefinition.java index 7015e0e6425..04960d0ef19 100644 --- a/hapi-fhir-base/src/main/java/ca/uhn/fhir/context/BaseRuntimeElementCompositeDefinition.java +++ b/hapi-fhir-base/src/main/java/ca/uhn/fhir/context/BaseRuntimeElementCompositeDefinition.java @@ -99,7 +99,11 @@ public abstract class BaseRuntimeElementCompositeDefinition ext if (childOrder != null) { forcedOrder = new HashMap(); for (int i = 0; i < childOrder.names().length; i++) { - forcedOrder.put(childOrder.names()[i], i); + String nextName = childOrder.names()[i]; + if (nextName.endsWith("[x]")) { + nextName = nextName.substring(0, nextName.length() - 3); + } + forcedOrder.put(nextName, i); } } } diff --git a/hapi-fhir-structures-dstu3/src/main/java/org/hl7/fhir/dstu3/model/ActivityDefinition.java b/hapi-fhir-structures-dstu3/src/main/java/org/hl7/fhir/dstu3/model/ActivityDefinition.java index 4e7ce7988cc..6859533a3bb 100644 --- a/hapi-fhir-structures-dstu3/src/main/java/org/hl7/fhir/dstu3/model/ActivityDefinition.java +++ b/hapi-fhir-structures-dstu3/src/main/java/org/hl7/fhir/dstu3/model/ActivityDefinition.java @@ -47,8 +47,7 @@ import org.hl7.fhir.exceptions.FHIRException; * This resource allows for the definition of some activity to be performed, independent of a particular patient, practitioner, or other performance context. */ @ResourceDef(name="ActivityDefinition", profile="http://hl7.org/fhir/Profile/ActivityDefinition") -//@ChildOrder(names={"url", "identifier", "version", "name", "title", "status", "experimental", "date", "publisher", "description", "purpose", "usage", "approvalDate", "lastReviewDate", "effectivePeriod", "useContext", "jurisdiction", "topic", "contributor", "contact", "copyright", "relatedArtifact", "library", "kind", "code", "timing[x]", "location", "participant", "product[x]", "quantity", "dosage", "bodySite", "transform", "dynamicValue"}) -@ChildOrder(names={"url", "identifier", "version", "name", "title", "status", "experimental", "date", "publisher", "description", "purpose", "usage", "approvalDate", "lastReviewDate", "effectivePeriod", "useContext", "jurisdiction", "topic", "contributor", "contact", "copyright", "relatedArtifact", "library", "kind", "code", "timing", "location", "participant", "product", "quantity", "dosage", "bodySite", "transform", "dynamicValue"}) +@ChildOrder(names={"url", "identifier", "version", "name", "title", "status", "experimental", "date", "publisher", "description", "purpose", "usage", "approvalDate", "lastReviewDate", "effectivePeriod", "useContext", "jurisdiction", "topic", "contributor", "contact", "copyright", "relatedArtifact", "library", "kind", "code", "timing[x]", "location", "participant", "product[x]", "quantity", "dosage", "bodySite", "transform", "dynamicValue"}) public class ActivityDefinition extends MetadataResource { public enum ActivityDefinitionKind { diff --git a/hapi-fhir-structures-dstu3/src/test/java/ca/uhn/fhir/parser/XmlParserDstu3Test.java b/hapi-fhir-structures-dstu3/src/test/java/ca/uhn/fhir/parser/XmlParserDstu3Test.java index 5ededaf9f1d..831bc8afc1c 100644 --- a/hapi-fhir-structures-dstu3/src/test/java/ca/uhn/fhir/parser/XmlParserDstu3Test.java +++ b/hapi-fhir-structures-dstu3/src/test/java/ca/uhn/fhir/parser/XmlParserDstu3Test.java @@ -86,8 +86,69 @@ public class XmlParserDstu3Test { ourCtx.setNarrativeGenerator(null); } + /** + * See #544 + */ @Test - public void testActivityDefinitionElementsOrder() throws Exception { + public void testBundleStitchReferencesByUuid() throws Exception { + Bundle bundle = new Bundle(); + + DocumentManifest dm = new DocumentManifest(); + dm.getSubject().setReference("urn:uuid:96e85cca-9797-45d6-834a-c4eb27f331d3"); + bundle.addEntry().setResource(dm); + + Patient patient = new Patient(); + patient.addName().setFamily("FAMILY"); + bundle.addEntry().setResource(patient).setFullUrl("urn:uuid:96e85cca-9797-45d6-834a-c4eb27f331d3"); + + String encoded = ourCtx.newXmlParser().setPrettyPrint(true).encodeResourceToString(bundle); + ourLog.info(encoded); + + bundle = ourCtx.newXmlParser().parseResource(Bundle.class, encoded); + dm = (DocumentManifest) bundle.getEntry().get(0).getResource(); + + assertEquals("urn:uuid:96e85cca-9797-45d6-834a-c4eb27f331d3", dm.getSubject().getReference()); + + Patient subject = (Patient) dm.getSubject().getResource(); + assertNotNull(subject); + assertEquals("FAMILY", subject.getNameFirstRep().getFamily()); + } + + @Test + public void testBundleWithBinary() { + + String bundle = "\n" + + " \n" + + " \n" + + " \n" + + " \n" + + " \n" + + " \n" + + " \n" + + " \n" + + " \n" + + " \n" + + " \n" + + " \n" + + " \n" + + " \n" + + " \n" + + " \n" + + ""; + + Bundle b = ourCtx.newXmlParser().parseResource(Bundle.class, bundle); + assertEquals(1, b.getEntry().size()); + + Binary bin = (Binary) b.getEntry().get(0).getResource(); + assertArrayEquals(new byte[] { 1, 2, 3, 4 }, bin.getContent()); + + } + + /** + * See #683 + */ + @Test + public void testChildOrderWithChoiceType() throws Exception { final String origContent = ""; final IParser parser = ourCtx.newXmlParser(); DefaultProfileValidationSupport validationSupport = new DefaultProfileValidationSupport(); @@ -121,7 +182,7 @@ public class XmlParserDstu3Test { // verify that the original and newly serialized match Assert.assertEquals(origContent, content); } - + @Test public void testConceptMapElementsOrder() throws Exception { final String origContent = ""; @@ -158,64 +219,6 @@ public class XmlParserDstu3Test { Assert.assertEquals(origContent, content); } - /** - * See #544 - */ - @Test - public void testBundleStitchReferencesByUuid() throws Exception { - Bundle bundle = new Bundle(); - - DocumentManifest dm = new DocumentManifest(); - dm.getSubject().setReference("urn:uuid:96e85cca-9797-45d6-834a-c4eb27f331d3"); - bundle.addEntry().setResource(dm); - - Patient patient = new Patient(); - patient.addName().setFamily("FAMILY"); - bundle.addEntry().setResource(patient).setFullUrl("urn:uuid:96e85cca-9797-45d6-834a-c4eb27f331d3"); - - String encoded = ourCtx.newXmlParser().setPrettyPrint(true).encodeResourceToString(bundle); - ourLog.info(encoded); - - bundle = ourCtx.newXmlParser().parseResource(Bundle.class, encoded); - dm = (DocumentManifest) bundle.getEntry().get(0).getResource(); - - assertEquals("urn:uuid:96e85cca-9797-45d6-834a-c4eb27f331d3", dm.getSubject().getReference()); - - Patient subject = (Patient) dm.getSubject().getResource(); - assertNotNull(subject); - assertEquals("FAMILY", subject.getNameFirstRep().getFamily()); - } - - @Test - public void testBundleWithBinary() { - - String bundle = "\n" + - " \n" + - " \n" + - " \n" + - " \n" + - " \n" + - " \n" + - " \n" + - " \n" + - " \n" + - " \n" + - " \n" + - " \n" + - " \n" + - " \n" + - " \n" + - " \n" + - ""; - - Bundle b = ourCtx.newXmlParser().parseResource(Bundle.class, bundle); - assertEquals(1, b.getEntry().size()); - - Binary bin = (Binary) b.getEntry().get(0).getResource(); - assertArrayEquals(new byte[] { 1, 2, 3, 4 }, bin.getContent()); - - } - @Test public void testContainedResourceInExtensionUndeclared() { Patient p = new Patient(); From a49e009cbaea172279a886c0addd1104b583c6ad Mon Sep 17 00:00:00 2001 From: Jeff Chung Date: Mon, 17 Jul 2017 15:10:38 -0700 Subject: [PATCH 11/28] Fix to prevent rare Concurrent Modification Exceptions --- .../RestHookSubscriptionDstu2Interceptor.java | 9 ++++++++- .../RestHookSubscriptionDstu3Interceptor.java | 8 +++++++- 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/interceptor/RestHookSubscriptionDstu2Interceptor.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/interceptor/RestHookSubscriptionDstu2Interceptor.java index c74fb12602f..bfb5d778c94 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/interceptor/RestHookSubscriptionDstu2Interceptor.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/interceptor/RestHookSubscriptionDstu2Interceptor.java @@ -83,7 +83,12 @@ public class RestHookSubscriptionDstu2Interceptor extends BaseRestHookSubscripti * @param theOperation */ private void checkSubscriptions(IIdType idType, String resourceType, RestOperationTypeEnum theOperation) { - for (Subscription subscription : myRestHookSubscriptions) { + //avoid a ConcurrentModificationException by copying to an array + for (Object object : myRestHookSubscriptions.toArray()) { + if (object == null) { + continue; + } + Subscription subscription = (Subscription) object; // see if the criteria matches the created object ourLog.info("Checking subscription {} for {} with criteria {}", subscription.getIdElement().getIdPart(), resourceType, subscription.getCriteria()); @@ -121,6 +126,8 @@ public class RestHookSubscriptionDstu2Interceptor extends BaseRestHookSubscripti } } + + /** * Creates an HTTP Post for a subscription */ diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/interceptor/RestHookSubscriptionDstu3Interceptor.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/interceptor/RestHookSubscriptionDstu3Interceptor.java index 0eda2bd28a2..ad53b7b8a67 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/interceptor/RestHookSubscriptionDstu3Interceptor.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/interceptor/RestHookSubscriptionDstu3Interceptor.java @@ -82,7 +82,13 @@ public class RestHookSubscriptionDstu3Interceptor extends BaseRestHookSubscripti * @param theOperation */ private void checkSubscriptions(IIdType idType, String resourceType, RestOperationTypeEnum theOperation) { - for (Subscription subscription : myRestHookSubscriptions) { + //avoid a ConcurrentModificationException by copying to an array + for (Object object : myRestHookSubscriptions.toArray()) { + //for (Subscription subscription : myRestHookSubscriptions) { + if (object == null) { + continue; + } + Subscription subscription = (Subscription) object; // see if the criteria matches the created object ourLog.info("Checking subscription {} for {} with criteria {}", subscription.getIdElement().getIdPart(), resourceType, subscription.getCriteria()); From 517fafbd0a3e3f7a316c363de66a0094fd8a4144 Mon Sep 17 00:00:00 2001 From: James Agnew Date: Thu, 20 Jul 2017 10:18:46 -0400 Subject: [PATCH 12/28] Add check --- .../uhn/fhir/jpa/config/TestDstu3Config.java | 4 +-- .../jpa/stresstest/StressTestDstu3Test.java | 35 +++++++++++++++++++ 2 files changed, 37 insertions(+), 2 deletions(-) diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/config/TestDstu3Config.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/config/TestDstu3Config.java index 0a964f37767..13b3bcaa318 100644 --- a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/config/TestDstu3Config.java +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/config/TestDstu3Config.java @@ -37,11 +37,11 @@ public class TestDstu3Config extends BaseJavaConfigDstu3 { retVal.setUrl("jdbc:derby:memory:myUnitTestDB;create=true"); retVal.setUsername(""); retVal.setPassword(""); - retVal.setMaxTotal(4); + retVal.setMaxTotal((int)(Math.random() * 6) + 1); DataSource dataSource = ProxyDataSourceBuilder .create(retVal) - .logQueryBySlf4j(SLF4JLogLevel.INFO, "SQL") +// .logQueryBySlf4j(SLF4JLogLevel.INFO, "SQL") .logSlowQueryBySlf4j(10, TimeUnit.SECONDS) .countQuery() .build(); diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/stresstest/StressTestDstu3Test.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/stresstest/StressTestDstu3Test.java index b3f72096863..059b27b9801 100644 --- a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/stresstest/StressTestDstu3Test.java +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/stresstest/StressTestDstu3Test.java @@ -42,6 +42,37 @@ public class StressTestDstu3Test extends BaseResourceProviderDstu3Test { ourRestServer.unregisterInterceptor(myRequestValidatingInterceptor); } + + + @Test + public void testMultithreadedSearch() throws Exception { + Bundle input = new Bundle(); + input.setType(BundleType.TRANSACTION); + for (int i = 0; i < 500; i++) { + Patient p = new Patient(); + p.addIdentifier().setSystem("http://test").setValue("BAR"); + input.addEntry().setResource(p).getRequest().setMethod(HTTPVerb.POST).setUrl("Patient"); + } + ourClient.transaction().withBundle(input).execute(); + + + List tasks = Lists.newArrayList(); + try { + for (int threadIndex = 0; threadIndex < 10; threadIndex++) { + SearchTask task = new SearchTask(); + tasks.add(task); + task.start(); + } + } finally { + for (BaseTask next : tasks) { + next.join(); + } + } + + validateNoErrors(tasks); + + } + /** * This test prevents a deadlock that was detected with a large number of @@ -89,6 +120,10 @@ public class StressTestDstu3Test extends BaseResourceProviderDstu3Test { } } + validateNoErrors(tasks); + } + + private void validateNoErrors(List tasks) { int total = 0; for (BaseTask next : tasks) { if (next.getError() != null) { From 9d08e1e2116d12b6d2142e0c66646b14fc612a93 Mon Sep 17 00:00:00 2001 From: James Agnew Date: Thu, 20 Jul 2017 11:00:17 -0400 Subject: [PATCH 13/28] Fix deadlock --- .../ca/uhn/fhir/jpa/dao/data/ISearchDao.java | 5 +- .../search/StaleSearchDeletingSvcImpl.java | 82 ++++++++----------- src/changes/changes.xml | 3 + 3 files changed, 40 insertions(+), 50 deletions(-) diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/data/ISearchDao.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/data/ISearchDao.java index 3d334deeda4..51c96c17f0e 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/data/ISearchDao.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/data/ISearchDao.java @@ -3,6 +3,9 @@ package ca.uhn.fhir.jpa.dao.data; import java.util.Collection; import java.util.Date; +import org.springframework.data.domain.Pageable; +import org.springframework.data.domain.Slice; + /* * #%L * HAPI FHIR JPA Server @@ -36,7 +39,7 @@ public interface ISearchDao extends JpaRepository { public Search findByUuid(@Param("uuid") String theUuid); @Query("SELECT s.myId FROM Search s WHERE s.mySearchLastReturned < :cutoff") - public Collection findWhereLastReturnedBefore(@Param("cutoff") Date theCutoff); + public Slice findWhereLastReturnedBefore(@Param("cutoff") Date theCutoff, Pageable thePage); // @Query("SELECT s FROM Search s WHERE s.myCreated < :cutoff") // public Collection findWhereCreatedBefore(@Param("cutoff") Date theCutoff); diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/StaleSearchDeletingSvcImpl.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/StaleSearchDeletingSvcImpl.java index dff4f46fa67..4654eab6bc8 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/StaleSearchDeletingSvcImpl.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/StaleSearchDeletingSvcImpl.java @@ -1,44 +1,23 @@ package ca.uhn.fhir.jpa.search; -/* - * #%L - * HAPI FHIR JPA Server - * %% - * Copyright (C) 2014 - 2017 University Health Network - * %% - * 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% - */ - -import java.util.Collection; import java.util.Date; import org.apache.commons.lang3.time.DateUtils; import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.data.domain.PageRequest; +import org.springframework.data.domain.Slice; import org.springframework.scheduling.annotation.Scheduled; import org.springframework.transaction.PlatformTransactionManager; import org.springframework.transaction.TransactionStatus; import org.springframework.transaction.annotation.Propagation; import org.springframework.transaction.annotation.Transactional; -import org.springframework.transaction.support.TransactionCallbackWithoutResult; +import org.springframework.transaction.support.TransactionCallback; import org.springframework.transaction.support.TransactionTemplate; import com.google.common.annotations.VisibleForTesting; import ca.uhn.fhir.jpa.dao.DaoConfig; -import ca.uhn.fhir.jpa.dao.data.ISearchDao; -import ca.uhn.fhir.jpa.dao.data.ISearchIncludeDao; -import ca.uhn.fhir.jpa.dao.data.ISearchResultDao; +import ca.uhn.fhir.jpa.dao.data.*; import ca.uhn.fhir.jpa.entity.Search; /** @@ -47,7 +26,6 @@ import ca.uhn.fhir.jpa.entity.Search; public class StaleSearchDeletingSvcImpl implements IStaleSearchDeletingSvc { public static final long DEFAULT_CUTOFF_SLACK = 10 * DateUtils.MILLIS_PER_SECOND; private static final org.slf4j.Logger ourLog = org.slf4j.LoggerFactory.getLogger(StaleSearchDeletingSvcImpl.class); - /* * We give a bit of extra leeway just to avoid race conditions where a query result @@ -71,41 +49,47 @@ public class StaleSearchDeletingSvcImpl implements IStaleSearchDeletingSvc { @Autowired private PlatformTransactionManager myTransactionManager; - protected void deleteSearch(final Long theSearchPid) { - TransactionTemplate tt = new TransactionTemplate(myTransactionManager); - tt.execute(new TransactionCallbackWithoutResult() { - @Override - protected void doInTransactionWithoutResult(TransactionStatus theArg0) { - Search searchToDelete = mySearchDao.findOne(theSearchPid); - ourLog.info("Deleting search {}/{} - Created[{}] -- Last returned[{}]", searchToDelete.getId(), searchToDelete.getUuid(), searchToDelete.getCreated(), searchToDelete.getSearchLastReturned()); - mySearchIncludeDao.deleteForSearch(searchToDelete.getId()); - mySearchResultDao.deleteForSearch(searchToDelete.getId()); - mySearchDao.delete(searchToDelete); - } - }); + private void deleteSearch(final Long theSearchPid) { + Search searchToDelete = mySearchDao.findOne(theSearchPid); + ourLog.info("Deleting search {}/{} - Created[{}] -- Last returned[{}]", searchToDelete.getId(), searchToDelete.getUuid(), searchToDelete.getCreated(), searchToDelete.getSearchLastReturned()); + mySearchIncludeDao.deleteForSearch(searchToDelete.getId()); + mySearchResultDao.deleteForSearch(searchToDelete.getId()); + mySearchDao.delete(searchToDelete); } - + @Override @Transactional(propagation = Propagation.NOT_SUPPORTED) public void pollForStaleSearchesAndDeleteThem() { - + long cutoffMillis = myDaoConfig.getExpireSearchResultsAfterMillis(); if (myDaoConfig.getReuseCachedSearchResultsForMillis() != null) { cutoffMillis = Math.max(cutoffMillis, myDaoConfig.getReuseCachedSearchResultsForMillis()); } - Date cutoff = new Date((System.currentTimeMillis() - cutoffMillis) - myCutoffSlack); - + final Date cutoff = new Date((System.currentTimeMillis() - cutoffMillis) - myCutoffSlack); + ourLog.debug("Searching for searches which are before {}", cutoff); - Collection toDelete = mySearchDao.findWhereLastReturnedBefore(cutoff); - if (!toDelete.isEmpty()) { - - for (final Long next : toDelete) { - deleteSearch(next); + TransactionTemplate tt = new TransactionTemplate(myTransactionManager); + int count = tt.execute(new TransactionCallback() { + @Override + public Integer doInTransaction(TransactionStatus theStatus) { + Slice toDelete = mySearchDao.findWhereLastReturnedBefore(cutoff, new PageRequest(0, 1000)); + for (final Long next : toDelete) { + deleteSearch(next); + } + return toDelete.getContent().size(); } + }); + + long total = tt.execute(new TransactionCallback() { + @Override + public Long doInTransaction(TransactionStatus theStatus) { + return mySearchDao.count(); + } + }); + + ourLog.info("Deleted {} searches, {} remaining", count, total); - ourLog.info("Deleted {} searches, {} remaining", toDelete.size(), mySearchDao.count()); - } } @Scheduled(fixedDelay = DEFAULT_CUTOFF_SLACK) diff --git a/src/changes/changes.xml b/src/changes/changes.xml index 92aab8d3dac..54fd5cc0991 100644 --- a/src/changes/changes.xml +++ b/src/changes/changes.xml @@ -176,6 +176,9 @@ elements are a choice type (i.e. named "foo[x]"). Thanks to GitHub user @CarthageKing for the pull request! + + Fix potential deadlock in stale search deleting task in JPA server + From 82171da0cc067f37c2b451cb2339a8ea6ba8ae86 Mon Sep 17 00:00:00 2001 From: James Agnew Date: Thu, 20 Jul 2017 11:19:53 -0400 Subject: [PATCH 14/28] One more deadlock --- .../fhir/jpa/term/BaseHapiTerminologySvc.java | 240 +++++++++--------- 1 file changed, 121 insertions(+), 119 deletions(-) diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/term/BaseHapiTerminologySvc.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/term/BaseHapiTerminologySvc.java index a2b3d0fa981..de92bb232ce 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/term/BaseHapiTerminologySvc.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/term/BaseHapiTerminologySvc.java @@ -10,7 +10,7 @@ package ca.uhn.fhir.jpa.term; * 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 + * 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, @@ -19,21 +19,10 @@ package ca.uhn.fhir.jpa.term; * limitations under the License. * #L% */ - -import java.util.ArrayList; -import java.util.Collection; -import java.util.Collections; -import java.util.HashMap; -import java.util.HashSet; -import java.util.IdentityHashMap; -import java.util.List; -import java.util.Map; -import java.util.Set; +import java.util.*; import java.util.concurrent.TimeUnit; -import javax.persistence.EntityManager; -import javax.persistence.PersistenceContext; -import javax.persistence.PersistenceContextType; +import javax.persistence.*; import org.apache.commons.lang3.Validate; import org.apache.commons.lang3.time.DateUtils; @@ -51,19 +40,12 @@ import org.springframework.transaction.support.TransactionTemplate; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Stopwatch; import com.google.common.collect.ArrayListMultimap; -import com.google.common.collect.Multimaps; import ca.uhn.fhir.context.FhirContext; import ca.uhn.fhir.jpa.dao.BaseHapiFhirDao; import ca.uhn.fhir.jpa.dao.DaoConfig; -import ca.uhn.fhir.jpa.dao.data.ITermCodeSystemDao; -import ca.uhn.fhir.jpa.dao.data.ITermCodeSystemVersionDao; -import ca.uhn.fhir.jpa.dao.data.ITermConceptDao; -import ca.uhn.fhir.jpa.dao.data.ITermConceptParentChildLinkDao; -import ca.uhn.fhir.jpa.entity.TermCodeSystem; -import ca.uhn.fhir.jpa.entity.TermCodeSystemVersion; -import ca.uhn.fhir.jpa.entity.TermConcept; -import ca.uhn.fhir.jpa.entity.TermConceptParentChildLink; +import ca.uhn.fhir.jpa.dao.data.*; +import ca.uhn.fhir.jpa.entity.*; import ca.uhn.fhir.jpa.entity.TermConceptParentChildLink.RelationshipTypeEnum; import ca.uhn.fhir.jpa.util.StopWatch; import ca.uhn.fhir.rest.server.exceptions.InvalidRequestException; @@ -76,6 +58,8 @@ public abstract class BaseHapiTerminologySvc implements IHapiTerminologySvc { private static final org.slf4j.Logger ourLog = org.slf4j.LoggerFactory.getLogger(BaseHapiTerminologySvc.class); private static final Object PLACEHOLDER_OBJECT = new Object(); + private ArrayListMultimap myChildToParentPidCache; + @Autowired protected ITermCodeSystemDao myCodeSystemDao; @@ -100,8 +84,8 @@ public abstract class BaseHapiTerminologySvc implements IHapiTerminologySvc { @PersistenceContext(type = PersistenceContextType.TRANSACTION) protected EntityManager myEntityManager; - private long myNextReindexPass; + private boolean myProcessDeferred = true; @Autowired @@ -121,7 +105,7 @@ public abstract class BaseHapiTerminologySvc implements IHapiTerminologySvc { private int ensureParentsSaved(Collection theParents) { ourLog.trace("Checking {} parents", theParents.size()); int retVal = 0; - + for (TermConceptParentChildLink nextLink : theParents) { if (nextLink.getRelationshipType() == RelationshipTypeEnum.ISA) { TermConcept nextParent = nextLink.getParent(); @@ -133,7 +117,7 @@ public abstract class BaseHapiTerminologySvc implements IHapiTerminologySvc { } } } - + return retVal; } @@ -206,8 +190,11 @@ public abstract class BaseHapiTerminologySvc implements IHapiTerminologySvc { /** * Subclasses may override - * @param theSystem The code system - * @param theCode The code + * + * @param theSystem + * The code system + * @param theCode + * The code */ protected List findCodesAboveUsingBuiltInSystems(String theSystem, String theCode) { return Collections.emptyList(); @@ -244,15 +231,19 @@ public abstract class BaseHapiTerminologySvc implements IHapiTerminologySvc { ArrayList retVal = toVersionIndependentConcepts(theSystem, codes); return retVal; } + /** * Subclasses may override - * @param theSystem The code system - * @param theCode The code + * + * @param theSystem + * The code system + * @param theCode + * The code */ protected List findCodesBelowUsingBuiltInSystems(String theSystem, String theCode) { return Collections.emptyList(); } - + private TermCodeSystemVersion findCurrentCodeSystemVersionForSystem(String theCodeSystem) { TermCodeSystem cs = getCodeSystem(theCodeSystem); if (cs == null || cs.getCurrentVersion() == null) { @@ -274,7 +265,7 @@ public abstract class BaseHapiTerminologySvc implements IHapiTerminologySvc { if (theConceptsStack.size() == 1 || theConceptsStack.size() % 10000 == 0) { float pct = (float) theConceptsStack.size() / (float) theTotalConcepts; - ourLog.info("Have processed {}/{} concepts ({}%)", theConceptsStack.size(), theTotalConcepts, (int)( pct*100.0f)); + ourLog.info("Have processed {}/{} concepts ({}%)", theConceptsStack.size(), theTotalConcepts, (int) (pct * 100.0f)); } theConcept.setCodeSystem(theCodeSystem); @@ -285,7 +276,7 @@ public abstract class BaseHapiTerminologySvc implements IHapiTerminologySvc { } else { myConceptsToSaveLater.add(theConcept); } - + for (TermConceptParentChildLink next : theConcept.getChildren()) { persistChildren(next.getChild(), theCodeSystem, theConceptsStack, theTotalConcepts); } @@ -297,7 +288,7 @@ public abstract class BaseHapiTerminologySvc implements IHapiTerminologySvc { myConceptLinksToSaveLater.add(next); } } - + } private void populateVersion(TermConcept theNext, TermCodeSystemVersion theCodeSystemVersion) { @@ -310,48 +301,56 @@ public abstract class BaseHapiTerminologySvc implements IHapiTerminologySvc { } } - private ArrayListMultimap myChildToParentPidCache; - + private void processDeferredConcepts() { + int codeCount = 0, relCount = 0; + StopWatch stopwatch = new StopWatch(); + + int count = Math.min(myDaoConfig.getDeferIndexingForCodesystemsOfSize(), myConceptsToSaveLater.size()); + ourLog.info("Saving {} deferred concepts...", count); + while (codeCount < count && myConceptsToSaveLater.size() > 0) { + TermConcept next = myConceptsToSaveLater.remove(0); + codeCount += saveConcept(next); + } + + if (codeCount > 0) { + ourLog.info("Saved {} deferred concepts ({} codes remain and {} relationships remain) in {}ms ({}ms / code)", + new Object[] { codeCount, myConceptsToSaveLater.size(), myConceptLinksToSaveLater.size(), stopwatch.getMillis(), stopwatch.getMillisPerOperation(codeCount) }); + } + + if (codeCount == 0) { + count = Math.min(myDaoConfig.getDeferIndexingForCodesystemsOfSize(), myConceptLinksToSaveLater.size()); + ourLog.info("Saving {} deferred concept relationships...", count); + while (relCount < count && myConceptLinksToSaveLater.size() > 0) { + TermConceptParentChildLink next = myConceptLinksToSaveLater.remove(0); + + if (myConceptDao.findOne(next.getChild().getId()) == null || myConceptDao.findOne(next.getParent().getId()) == null) { + ourLog.warn("Not inserting link from child {} to parent {} because it appears to have been deleted", next.getParent().getCode(), next.getChild().getCode()); + continue; + } + + saveConceptLink(next); + relCount++; + } + } + + if (relCount > 0) { + ourLog.info("Saved {} deferred relationships ({} remain) in {}ms ({}ms / code)", + new Object[] { relCount, myConceptLinksToSaveLater.size(), stopwatch.getMillis(), stopwatch.getMillisPerOperation(codeCount) }); + } + + if ((myConceptsToSaveLater.size() + myConceptLinksToSaveLater.size()) == 0) { + ourLog.info("All deferred concepts and relationships have now been synchronized to the database"); + } + } + private void processReindexing() { if (System.currentTimeMillis() < myNextReindexPass && !ourForceSaveDeferredAlwaysForUnitTest) { return; } - + TransactionTemplate tt = new TransactionTemplate(myTransactionMgr); tt.setPropagationBehavior(TransactionTemplate.PROPAGATION_REQUIRES_NEW); tt.execute(new TransactionCallbackWithoutResult() { - @Override - protected void doInTransactionWithoutResult(TransactionStatus theArg0) { - int maxResult = 1000; - Page concepts = myConceptDao.findResourcesRequiringReindexing(new PageRequest(0, maxResult)); - if (concepts.hasContent() == false) { - myNextReindexPass = System.currentTimeMillis() + DateUtils.MILLIS_PER_MINUTE; - myChildToParentPidCache = null; - return; - } - - if (myChildToParentPidCache == null) { - myChildToParentPidCache = ArrayListMultimap.create(); - } - - ourLog.info("Indexing {} / {} concepts", concepts.getContent().size(), concepts.getTotalElements()); - - int count = 0; - StopWatch stopwatch = new StopWatch(); - - for (TermConcept nextConcept : concepts) { - - StringBuilder parentsBuilder = new StringBuilder(); - createParentsString(parentsBuilder, nextConcept.getId()); - nextConcept.setParentPids(parentsBuilder.toString()); - - saveConcept(nextConcept); - count++; - } - - ourLog.info("Indexed {} / {} concepts in {}ms - Avg {}ms / resource", new Object[] { count, concepts.getContent().size(), stopwatch.getMillis(), stopwatch.getMillisPerOperation(count) }); - } - private void createParentsString(StringBuilder theParentsBuilder, Long theConceptPid) { Validate.notNull(theConceptPid, "theConceptPid must not be null"); List parents = myChildToParentPidCache.get(theConceptPid); @@ -368,8 +367,7 @@ public abstract class BaseHapiTerminologySvc implements IHapiTerminologySvc { } } } - - + for (Long nextParent : parents) { if (theParentsBuilder.length() > 0) { theParentsBuilder.append(' '); @@ -378,13 +376,45 @@ public abstract class BaseHapiTerminologySvc implements IHapiTerminologySvc { createParentsString(theParentsBuilder, nextParent); } } + + @Override + protected void doInTransactionWithoutResult(TransactionStatus theArg0) { + int maxResult = 1000; + Page concepts = myConceptDao.findResourcesRequiringReindexing(new PageRequest(0, maxResult)); + if (concepts.hasContent() == false) { + myNextReindexPass = System.currentTimeMillis() + DateUtils.MILLIS_PER_MINUTE; + myChildToParentPidCache = null; + return; + } + + if (myChildToParentPidCache == null) { + myChildToParentPidCache = ArrayListMultimap.create(); + } + + ourLog.info("Indexing {} / {} concepts", concepts.getContent().size(), concepts.getTotalElements()); + + int count = 0; + StopWatch stopwatch = new StopWatch(); + + for (TermConcept nextConcept : concepts) { + + StringBuilder parentsBuilder = new StringBuilder(); + createParentsString(parentsBuilder, nextConcept.getId()); + nextConcept.setParentPids(parentsBuilder.toString()); + + saveConcept(nextConcept); + count++; + } + + ourLog.info("Indexed {} / {} concepts in {}ms - Avg {}ms / resource", new Object[] { count, concepts.getContent().size(), stopwatch.getMillis(), stopwatch.getMillisPerOperation(count) }); + } }); } private int saveConcept(TermConcept theConcept) { int retVal = 0; - + /* * If the concept has an ID, we're reindexing, so there's no need to * save parent concepts first (it's way too slow to do that) @@ -392,25 +422,25 @@ public abstract class BaseHapiTerminologySvc implements IHapiTerminologySvc { if (theConcept.getId() == null) { retVal += ensureParentsSaved(theConcept.getParents()); } - + if (theConcept.getId() == null || theConcept.getIndexStatus() == null) { retVal++; theConcept.setIndexStatus(BaseHapiFhirDao.INDEX_STATUS_INDEXED); myConceptDao.save(theConcept); } - + ourLog.trace("Saved {} and got PID {}", theConcept.getCode(), theConcept.getId()); return retVal; } - + private void saveConceptLink(TermConceptParentChildLink next) { if (next.getId() == null) { myConceptParentChildLinkDao.save(next); } } - @Scheduled(fixedRate=5000) - @Transactional(propagation=Propagation.REQUIRED) + @Scheduled(fixedRate = 5000) + @Transactional(propagation = Propagation.NOT_SUPPORTED) @Override public synchronized void saveDeferred() { if (!myProcessDeferred) { @@ -419,46 +449,18 @@ public abstract class BaseHapiTerminologySvc implements IHapiTerminologySvc { processReindexing(); return; } - - int codeCount = 0, relCount = 0; - StopWatch stopwatch = new StopWatch(); - - int count = Math.min(myDaoConfig.getDeferIndexingForCodesystemsOfSize(), myConceptsToSaveLater.size()); - ourLog.info("Saving {} deferred concepts...", count); - while (codeCount < count && myConceptsToSaveLater.size() > 0) { - TermConcept next = myConceptsToSaveLater.remove(0); - codeCount += saveConcept(next); - } - if (codeCount > 0) { - ourLog.info("Saved {} deferred concepts ({} codes remain and {} relationships remain) in {}ms ({}ms / code)", new Object[] {codeCount, myConceptsToSaveLater.size(), myConceptLinksToSaveLater.size(), stopwatch.getMillis(), stopwatch.getMillisPerOperation(codeCount)}); - } - - if (codeCount == 0) { - count = Math.min(myDaoConfig.getDeferIndexingForCodesystemsOfSize(), myConceptLinksToSaveLater.size()); - ourLog.info("Saving {} deferred concept relationships...", count); - while (relCount < count && myConceptLinksToSaveLater.size() > 0) { - TermConceptParentChildLink next = myConceptLinksToSaveLater.remove(0); - - if (myConceptDao.findOne(next.getChild().getId()) == null || myConceptDao.findOne(next.getParent().getId()) == null) { - ourLog.warn("Not inserting link from child {} to parent {} because it appears to have been deleted", next.getParent().getCode(), next.getChild().getCode()); - continue; - } - - saveConceptLink(next); - relCount++; + TransactionTemplate tt = new TransactionTemplate(myTransactionMgr); + tt.setPropagationBehavior(TransactionTemplate.PROPAGATION_REQUIRES_NEW); + tt.execute(new TransactionCallbackWithoutResult() { + @Override + protected void doInTransactionWithoutResult(TransactionStatus theArg0) { + processDeferredConcepts(); } - } - - if (relCount > 0) { - ourLog.info("Saved {} deferred relationships ({} remain) in {}ms ({}ms / code)", new Object[] {relCount, myConceptLinksToSaveLater.size(), stopwatch.getMillis(), stopwatch.getMillisPerOperation(codeCount)}); - } - - if ((myConceptsToSaveLater.size() + myConceptLinksToSaveLater.size()) == 0) { - ourLog.info("All deferred concepts and relationships have now been synchronized to the database"); - } + + }); } - + @Override public void setProcessDeferred(boolean theProcessDeferred) { myProcessDeferred = theProcessDeferred; @@ -487,7 +489,7 @@ public abstract class BaseHapiTerminologySvc implements IHapiTerminologySvc { } ourLog.info("Flushing..."); - + myConceptParentChildLinkDao.flush(); myConceptDao.flush(); @@ -540,7 +542,7 @@ public abstract class BaseHapiTerminologySvc implements IHapiTerminologySvc { } ourLog.info("Saving {} concepts...", totalCodeCount); - + IdentityHashMap conceptsStack2 = new IdentityHashMap(); for (TermConcept next : theCodeSystemVersion.getConcepts()) { persistChildren(next, codeSystemVersion, conceptsStack2, totalCodeCount); @@ -552,7 +554,7 @@ public abstract class BaseHapiTerminologySvc implements IHapiTerminologySvc { myConceptParentChildLinkDao.flush(); ourLog.info("Done deleting old code system versions"); - + if (myConceptsToSaveLater.size() > 0 || myConceptLinksToSaveLater.size() > 0) { ourLog.info("Note that some concept saving was deferred - still have {} concepts and {} relationships", myConceptsToSaveLater.size(), myConceptLinksToSaveLater.size()); } @@ -563,7 +565,7 @@ public abstract class BaseHapiTerminologySvc implements IHapiTerminologySvc { TermCodeSystem cs = getCodeSystem(theSystem); return cs != null; } - + private ArrayList toVersionIndependentConcepts(String theSystem, Set codes) { ArrayList retVal = new ArrayList(codes.size()); for (TermConcept next : codes) { @@ -582,7 +584,7 @@ public abstract class BaseHapiTerminologySvc implements IHapiTerminologySvc { throw new InvalidRequestException("CodeSystem contains circular reference around code " + theConcept.getCode()); } theConceptsStack.add(theConcept.getCode()); - + int retVal = 0; if (theAllConcepts.put(theConcept, theAllConcepts) == null) { if (theAllConcepts.size() % 1000 == 0) { From 2b72bb6c2ff7321aabbcc5768b5155374ff5209c Mon Sep 17 00:00:00 2001 From: James Agnew Date: Thu, 20 Jul 2017 13:44:09 -0400 Subject: [PATCH 15/28] One more deadlock --- .../jpa/search/SearchCoordinatorSvcImpl.java | 8 ++--- .../uhn/fhir/jpa/config/TestDstu3Config.java | 9 +++++- .../jpa/stresstest/StressTestDstu3Test.java | 31 ++++++++++++++++--- 3 files changed, 39 insertions(+), 9 deletions(-) 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 8eb3e8885be..8aa917a79e3 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 @@ -23,8 +23,6 @@ import java.util.*; import java.util.concurrent.*; import javax.persistence.EntityManager; -import javax.transaction.Transactional; -import javax.transaction.Transactional.TxType; import org.apache.commons.lang3.ObjectUtils; import org.apache.commons.lang3.Validate; @@ -35,6 +33,8 @@ import org.springframework.beans.factory.annotation.Autowired; import org.springframework.data.domain.*; import org.springframework.scheduling.concurrent.CustomizableThreadFactory; import org.springframework.transaction.*; +import org.springframework.transaction.annotation.Propagation; +import org.springframework.transaction.annotation.Transactional; import org.springframework.transaction.support.*; import com.google.common.annotations.VisibleForTesting; @@ -106,7 +106,7 @@ public class SearchCoordinatorSvcImpl implements ISearchCoordinatorSvc { } @Override - @Transactional(value = TxType.NOT_SUPPORTED) + @Transactional(propagation=Propagation.SUPPORTS) public List getResources(final String theUuid, int theFrom, int theTo) { if (myNeverUseLocalSearchForUnitTests == false) { SearchTask task = myIdToSearchTask.get(theUuid); @@ -116,7 +116,7 @@ public class SearchCoordinatorSvcImpl implements ISearchCoordinatorSvc { } TransactionTemplate txTemplate = new TransactionTemplate(myManagedTxManager); - txTemplate.setPropagationBehavior(TransactionDefinition.PROPAGATION_REQUIRES_NEW); + txTemplate.setPropagationBehavior(TransactionDefinition.PROPAGATION_REQUIRED); Search search; StopWatch sw = new StopWatch(); diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/config/TestDstu3Config.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/config/TestDstu3Config.java index 13b3bcaa318..3a2e97ed30b 100644 --- a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/config/TestDstu3Config.java +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/config/TestDstu3Config.java @@ -37,7 +37,14 @@ public class TestDstu3Config extends BaseJavaConfigDstu3 { retVal.setUrl("jdbc:derby:memory:myUnitTestDB;create=true"); retVal.setUsername(""); retVal.setPassword(""); - retVal.setMaxTotal((int)(Math.random() * 6) + 1); + + /* + * We use a randomized number of maximum threads in order to try + * and catch any potential deadlocks caused by database connection + * starvation + */ + int maxThreads = (int)(Math.random() * 6) + 1; + retVal.setMaxTotal(maxThreads); DataSource dataSource = ProxyDataSourceBuilder .create(retVal) diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/stresstest/StressTestDstu3Test.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/stresstest/StressTestDstu3Test.java index 059b27b9801..6949380e8d5 100644 --- a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/stresstest/StressTestDstu3Test.java +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/stresstest/StressTestDstu3Test.java @@ -15,9 +15,11 @@ import org.hl7.fhir.dstu3.model.Bundle.BundleType; import org.hl7.fhir.dstu3.model.Bundle.HTTPVerb; import org.junit.*; +import com.google.common.base.Charsets; import com.google.common.collect.Lists; import ca.uhn.fhir.jpa.provider.dstu3.BaseResourceProviderDstu3Test; +import ca.uhn.fhir.rest.server.Constants; import ca.uhn.fhir.rest.server.interceptor.RequestValidatingInterceptor; import ca.uhn.fhir.util.TestUtil; @@ -162,16 +164,37 @@ public class StressTestDstu3Test extends BaseResourceProviderDstu3Test { @Override public void run() { - CloseableHttpResponse get = null; + CloseableHttpResponse getResp = null; for (int i = 0; i < 20; i++) { try { - get = ourHttpClient.execute(new HttpGet(ourServerBase + "/Patient?identifier=http%3A%2F%2Ftest%7CBAR," + UUID.randomUUID().toString())); + Bundle respBundle; + + // Load search + HttpGet get = new HttpGet(ourServerBase + "/Patient?identifier=http%3A%2F%2Ftest%7CBAR," + UUID.randomUUID().toString()); + get.addHeader(Constants.HEADER_CONTENT_TYPE, Constants.CT_FHIR_JSON_NEW); + getResp = ourHttpClient.execute(get); try { - assertEquals(200, get.getStatusLine().getStatusCode()); + assertEquals(200, getResp.getStatusLine().getStatusCode()); + String respBundleString = IOUtils.toString(getResp.getEntity().getContent(), Charsets.UTF_8); + respBundle = myFhirCtx.newJsonParser().parseResource(Bundle.class, respBundleString); myTaskCount++; } finally { - IOUtils.closeQuietly(get); + IOUtils.closeQuietly(getResp); } + + // Load page 2 + get = new HttpGet(respBundle.getLink("next").getUrl()); + get.addHeader(Constants.HEADER_CONTENT_TYPE, Constants.CT_FHIR_JSON_NEW); + getResp = ourHttpClient.execute(get); + try { + assertEquals(200, getResp.getStatusLine().getStatusCode()); + String respBundleString = IOUtils.toString(getResp.getEntity().getContent(), Charsets.UTF_8); + respBundle = myFhirCtx.newJsonParser().parseResource(Bundle.class, respBundleString); + myTaskCount++; + } finally { + IOUtils.closeQuietly(getResp); + } + } catch (Throwable e) { ourLog.error("Failure during search", e); myError = e; From 722eab62f2b392486f5fcd142293914f98effe3d Mon Sep 17 00:00:00 2001 From: James Agnew Date: Thu, 20 Jul 2017 14:03:11 -0400 Subject: [PATCH 16/28] Avoid opening two transactions for paging requests --- .../search/PersistedJpaBundleProvider.java | 33 ++++++++++--------- .../jpa/search/SearchCoordinatorSvcImpl.java | 2 +- 2 files changed, 19 insertions(+), 16 deletions(-) diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/PersistedJpaBundleProvider.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/PersistedJpaBundleProvider.java index d077dea033a..9145a745ab1 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/PersistedJpaBundleProvider.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/PersistedJpaBundleProvider.java @@ -34,8 +34,7 @@ import org.springframework.data.domain.PageRequest; import org.springframework.data.domain.Pageable; import org.springframework.transaction.PlatformTransactionManager; import org.springframework.transaction.TransactionStatus; -import org.springframework.transaction.support.TransactionCallback; -import org.springframework.transaction.support.TransactionTemplate; +import org.springframework.transaction.support.*; import ca.uhn.fhir.context.FhirContext; import ca.uhn.fhir.jpa.dao.IDao; @@ -113,7 +112,7 @@ public class PersistedJpaBundleProvider implements IBundleProvider { return retVal; } - protected List doSearchOrEverythingInTransaction(final int theFromIndex, final int theToIndex) { + protected List doSearchOrEverything(final int theFromIndex, final int theToIndex) { ISearchBuilder sb = myDao.newSearchBuilder(); String resourceName = mySearchEntity.getResourceType(); @@ -175,22 +174,26 @@ public class PersistedJpaBundleProvider implements IBundleProvider { TransactionTemplate template = new TransactionTemplate(myPlatformTransactionManager); - return template.execute(new TransactionCallback>() { + template.execute(new TransactionCallbackWithoutResult() { @Override - public List doInTransaction(TransactionStatus theStatus) { + protected void doInTransactionWithoutResult(TransactionStatus theStatus) { ensureSearchEntityLoaded(); - - switch (mySearchEntity.getSearchType()) { - case HISTORY: - return doHistoryInTransaction(theFromIndex, theToIndex); - case SEARCH: - case EVERYTHING: - default: - return doSearchOrEverythingInTransaction(theFromIndex, theToIndex); - } } - }); + + switch (mySearchEntity.getSearchType()) { + case HISTORY: + return template.execute(new TransactionCallback>() { + @Override + public List doInTransaction(TransactionStatus theStatus) { + return doHistoryInTransaction(theFromIndex, theToIndex); + } + }); + case SEARCH: + case EVERYTHING: + default: + return doSearchOrEverything(theFromIndex, theToIndex); + } } public String getUuid() { 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 8aa917a79e3..8e1e447de63 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 @@ -106,7 +106,7 @@ public class SearchCoordinatorSvcImpl implements ISearchCoordinatorSvc { } @Override - @Transactional(propagation=Propagation.SUPPORTS) + @Transactional(propagation=Propagation.NEVER) public List getResources(final String theUuid, int theFrom, int theTo) { if (myNeverUseLocalSearchForUnitTests == false) { SearchTask task = myIdToSearchTask.get(theUuid); From 49ddf76a5d45e78cc15b60c1eeb86f40edcc71c8 Mon Sep 17 00:00:00 2001 From: James Agnew Date: Thu, 20 Jul 2017 14:18:26 -0400 Subject: [PATCH 17/28] Reduce stress test impact --- .../ca/uhn/fhir/jpa/stresstest/StressTestDstu3Test.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/stresstest/StressTestDstu3Test.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/stresstest/StressTestDstu3Test.java index 6949380e8d5..2b8a976e045 100644 --- a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/stresstest/StressTestDstu3Test.java +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/stresstest/StressTestDstu3Test.java @@ -106,12 +106,12 @@ public class StressTestDstu3Test extends BaseResourceProviderDstu3Test { List tasks = Lists.newArrayList(); try { - for (int threadIndex = 0; threadIndex < 8; threadIndex++) { + for (int threadIndex = 0; threadIndex < 5; threadIndex++) { SearchTask task = new SearchTask(); tasks.add(task); task.start(); } - for (int threadIndex = 0; threadIndex < 8; threadIndex++) { + for (int threadIndex = 0; threadIndex < 5; threadIndex++) { CreateTask task = new CreateTask(); tasks.add(task); task.start(); @@ -165,7 +165,7 @@ public class StressTestDstu3Test extends BaseResourceProviderDstu3Test { @Override public void run() { CloseableHttpResponse getResp = null; - for (int i = 0; i < 20; i++) { + for (int i = 0; i < 10; i++) { try { Bundle respBundle; From 82e2aadf8655a8b849dabe3a74afd7ef02ea0d0a Mon Sep 17 00:00:00 2001 From: James Agnew Date: Thu, 20 Jul 2017 14:45:42 -0400 Subject: [PATCH 18/28] Adjust for postgres --- .../fhir/jpa/search/PersistedJpaBundleProvider.java | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/PersistedJpaBundleProvider.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/PersistedJpaBundleProvider.java index 9145a745ab1..bc1dacf764b 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/PersistedJpaBundleProvider.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/PersistedJpaBundleProvider.java @@ -113,15 +113,22 @@ public class PersistedJpaBundleProvider implements IBundleProvider { } protected List doSearchOrEverything(final int theFromIndex, final int theToIndex) { - ISearchBuilder sb = myDao.newSearchBuilder(); + final ISearchBuilder sb = myDao.newSearchBuilder(); String resourceName = mySearchEntity.getResourceType(); Class resourceType = myContext.getResourceDefinition(resourceName).getImplementingClass(); sb.setType(resourceType, resourceName); - List pidsSubList = mySearchCoordinatorSvc.getResources(myUuid, theFromIndex, theToIndex); + final List pidsSubList = mySearchCoordinatorSvc.getResources(myUuid, theFromIndex, theToIndex); - return toResourceList(sb, pidsSubList); + TransactionTemplate template = new TransactionTemplate(myPlatformTransactionManager); + template.setPropagationBehavior(TransactionTemplate.PROPAGATION_REQUIRED); + return template.execute(new TransactionCallback>() { + @Override + public List doInTransaction(TransactionStatus theStatus) { + return toResourceList(sb, pidsSubList); + } + }); } private void ensureDependenciesInjected() { From 0f50eae640e11fac483e150ab190ccca68a52089 Mon Sep 17 00:00:00 2001 From: James Agnew Date: Thu, 20 Jul 2017 15:23:33 -0400 Subject: [PATCH 19/28] Remove an unneeded log entry --- .../jpa/search/StaleSearchDeletingSvcImpl.java | 17 +++++++++-------- .../fhir/jpa/term/BaseHapiTerminologySvc.java | 2 +- 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/StaleSearchDeletingSvcImpl.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/StaleSearchDeletingSvcImpl.java index 4654eab6bc8..8e85287d683 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/StaleSearchDeletingSvcImpl.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/StaleSearchDeletingSvcImpl.java @@ -81,14 +81,15 @@ public class StaleSearchDeletingSvcImpl implements IStaleSearchDeletingSvc { } }); - long total = tt.execute(new TransactionCallback() { - @Override - public Long doInTransaction(TransactionStatus theStatus) { - return mySearchDao.count(); - } - }); - - ourLog.info("Deleted {} searches, {} remaining", count, total); + if (count > 0) { + long total = tt.execute(new TransactionCallback() { + @Override + public Long doInTransaction(TransactionStatus theStatus) { + return mySearchDao.count(); + } + }); + ourLog.info("Deleted {} searches, {} remaining", count, total); + } } diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/term/BaseHapiTerminologySvc.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/term/BaseHapiTerminologySvc.java index de92bb232ce..6db49a0e40b 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/term/BaseHapiTerminologySvc.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/term/BaseHapiTerminologySvc.java @@ -440,7 +440,7 @@ public abstract class BaseHapiTerminologySvc implements IHapiTerminologySvc { } @Scheduled(fixedRate = 5000) - @Transactional(propagation = Propagation.NOT_SUPPORTED) + @Transactional(propagation = Propagation.NEVER) @Override public synchronized void saveDeferred() { if (!myProcessDeferred) { From 6a178b08bd8f7bd2dad4676336f529285f0c73c7 Mon Sep 17 00:00:00 2001 From: James Date: Thu, 20 Jul 2017 15:24:06 -0400 Subject: [PATCH 20/28] Adjust tests --- .../src/test/java/ca/uhn/fhir/jpa/config/TestDstu3Config.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/config/TestDstu3Config.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/config/TestDstu3Config.java index 3a2e97ed30b..0a29df52c1c 100644 --- a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/config/TestDstu3Config.java +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/config/TestDstu3Config.java @@ -43,7 +43,7 @@ public class TestDstu3Config extends BaseJavaConfigDstu3 { * and catch any potential deadlocks caused by database connection * starvation */ - int maxThreads = (int)(Math.random() * 6) + 1; + int maxThreads = (int)(Math.random() * 6) + 2; retVal.setMaxTotal(maxThreads); DataSource dataSource = ProxyDataSourceBuilder From a13c78d6cc0ec5027b57d132ed907d9ea66a5555 Mon Sep 17 00:00:00 2001 From: James Date: Fri, 21 Jul 2017 07:39:11 -0400 Subject: [PATCH 21/28] FIx deadlock in transaction processing, and put transaction outcome in the right spot --- .../jpa/dao/dstu3/FhirSystemDaoDstu3.java | 486 +++++++++--------- .../ca/uhn/fhir/jpa/entity/SearchResult.java | 2 - .../search/StaleSearchDeletingSvcImpl.java | 20 + .../uhn/fhir/jpa/config/TestDstu3Config.java | 1 + .../jpa/dao/dstu3/FhirSystemDaoDstu3Test.java | 186 +++++-- src/changes/changes.xml | 11 +- 6 files changed, 426 insertions(+), 280 deletions(-) diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/dstu3/FhirSystemDaoDstu3.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/dstu3/FhirSystemDaoDstu3.java index 79a3d3a965f..9e602d9589d 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/dstu3/FhirSystemDaoDstu3.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/dstu3/FhirSystemDaoDstu3.java @@ -19,48 +19,21 @@ package ca.uhn.fhir.jpa.dao.dstu3; * limitations under the License. * #L% */ -import static org.apache.commons.lang3.StringUtils.defaultString; -import static org.apache.commons.lang3.StringUtils.isBlank; -import static org.apache.commons.lang3.StringUtils.isNotBlank; +import static org.apache.commons.lang3.StringUtils.*; -import java.util.ArrayList; -import java.util.Collection; -import java.util.Collections; -import java.util.Comparator; -import java.util.Date; -import java.util.HashMap; -import java.util.HashSet; -import java.util.IdentityHashMap; -import java.util.Iterator; -import java.util.LinkedHashSet; -import java.util.List; -import java.util.Map; +import java.util.*; import java.util.Map.Entry; -import java.util.Set; import javax.persistence.TypedQuery; -import javax.servlet.http.HttpServletRequest; +import org.apache.commons.lang3.Validate; import org.apache.http.NameValuePair; -import org.hl7.fhir.dstu3.model.Bundle; -import org.hl7.fhir.dstu3.model.Bundle.BundleEntryComponent; -import org.hl7.fhir.dstu3.model.Bundle.BundleEntryResponseComponent; -import org.hl7.fhir.dstu3.model.Bundle.BundleType; -import org.hl7.fhir.dstu3.model.Bundle.HTTPVerb; -import org.hl7.fhir.dstu3.model.IdType; -import org.hl7.fhir.dstu3.model.Meta; -import org.hl7.fhir.dstu3.model.OperationOutcome; +import org.hl7.fhir.dstu3.model.*; +import org.hl7.fhir.dstu3.model.Bundle.*; import org.hl7.fhir.dstu3.model.OperationOutcome.IssueSeverity; -import org.hl7.fhir.dstu3.model.Resource; -import org.hl7.fhir.instance.model.api.IAnyResource; -import org.hl7.fhir.instance.model.api.IBaseReference; -import org.hl7.fhir.instance.model.api.IBaseResource; -import org.hl7.fhir.instance.model.api.IIdType; -import org.hl7.fhir.instance.model.api.IPrimitiveType; +import org.hl7.fhir.instance.model.api.*; import org.springframework.beans.factory.annotation.Autowired; -import org.springframework.transaction.PlatformTransactionManager; -import org.springframework.transaction.TransactionDefinition; -import org.springframework.transaction.TransactionStatus; +import org.springframework.transaction.*; import org.springframework.transaction.annotation.Propagation; import org.springframework.transaction.annotation.Transactional; import org.springframework.transaction.support.TransactionCallback; @@ -69,10 +42,7 @@ import org.springframework.transaction.support.TransactionTemplate; import com.google.common.collect.ArrayListMultimap; import ca.uhn.fhir.context.RuntimeResourceDefinition; -import ca.uhn.fhir.jpa.dao.BaseHapiFhirSystemDao; -import ca.uhn.fhir.jpa.dao.DaoMethodOutcome; -import ca.uhn.fhir.jpa.dao.DeleteMethodOutcome; -import ca.uhn.fhir.jpa.dao.IFhirResourceDao; +import ca.uhn.fhir.jpa.dao.*; import ca.uhn.fhir.jpa.entity.ResourceTable; import ca.uhn.fhir.jpa.entity.TagDefinition; import ca.uhn.fhir.jpa.provider.ServletSubRequestDetails; @@ -80,19 +50,12 @@ import ca.uhn.fhir.jpa.util.DeleteConflict; import ca.uhn.fhir.model.api.ResourceMetadataKeyEnum; import ca.uhn.fhir.parser.DataFormatException; import ca.uhn.fhir.parser.IParser; -import ca.uhn.fhir.rest.api.PreferReturnEnum; -import ca.uhn.fhir.rest.api.RequestTypeEnum; -import ca.uhn.fhir.rest.api.RestOperationTypeEnum; -import ca.uhn.fhir.rest.method.BaseMethodBinding; -import ca.uhn.fhir.rest.method.BaseResourceReturningMethodBinding; +import ca.uhn.fhir.rest.api.*; +import ca.uhn.fhir.rest.method.*; import ca.uhn.fhir.rest.method.BaseResourceReturningMethodBinding.ResourceOrDstu1Bundle; -import ca.uhn.fhir.rest.method.RequestDetails; import ca.uhn.fhir.rest.server.Constants; import ca.uhn.fhir.rest.server.RestfulServerUtils; -import ca.uhn.fhir.rest.server.exceptions.BaseServerResponseException; -import ca.uhn.fhir.rest.server.exceptions.InternalErrorException; -import ca.uhn.fhir.rest.server.exceptions.InvalidRequestException; -import ca.uhn.fhir.rest.server.exceptions.NotModifiedException; +import ca.uhn.fhir.rest.server.exceptions.*; import ca.uhn.fhir.rest.server.interceptor.IServerInterceptor.ActionRequestDetails; import ca.uhn.fhir.rest.server.servlet.ServletRequestDetails; import ca.uhn.fhir.util.FhirTerser; @@ -121,6 +84,8 @@ public class FhirSystemDaoDstu3 extends BaseHapiFhirSystemDao { for (final BundleEntryComponent nextRequestEntry : theRequest.getEntry()) { + BaseServerResponseExceptionHolder caughtEx = new BaseServerResponseExceptionHolder(); + TransactionCallback callback = new TransactionCallback() { @Override public Bundle doInTransaction(TransactionStatus theStatus) { @@ -133,13 +98,12 @@ public class FhirSystemDaoDstu3 extends BaseHapiFhirSystemDao { } }; - BaseServerResponseException caughtEx; try { - Bundle nextResponseBundle = txTemplate.execute(callback); - caughtEx = null; + Bundle nextResponseBundle = callback.doInTransaction(null); BundleEntryComponent subResponseEntry = nextResponseBundle.getEntry().get(0); resp.addEntry(subResponseEntry); + /* * If the individual entry didn't have a resource in its response, bring the sub-transaction's OperationOutcome across so the client can see it */ @@ -148,21 +112,19 @@ public class FhirSystemDaoDstu3 extends BaseHapiFhirSystemDao { } } catch (BaseServerResponseException e) { - caughtEx = e; + caughtEx.setException(e); } catch (Throwable t) { ourLog.error("Failure during BATCH sub transaction processing", t); - caughtEx = new InternalErrorException(t); + caughtEx.setException(new InternalErrorException(t)); } - if (caughtEx != null) { + if (caughtEx.getException() != null) { BundleEntryComponent nextEntry = resp.addEntry(); - OperationOutcome oo = new OperationOutcome(); - oo.addIssue().setSeverity(IssueSeverity.ERROR).setDiagnostics(caughtEx.getMessage()); - nextEntry.setResource(oo); + populateEntryWithOperationOutcome(caughtEx.getException(), nextEntry); BundleEntryResponseComponent nextEntryResp = nextEntry.getResponse(); - nextEntryResp.setStatus(toStatusString(caughtEx.getStatusCode())); + nextEntryResp.setStatus(toStatusString(caughtEx.getException().getStatusCode())); } } @@ -173,117 +135,7 @@ public class FhirSystemDaoDstu3 extends BaseHapiFhirSystemDao { return resp; } - private String extractTransactionUrlOrThrowException(BundleEntryComponent nextEntry, HTTPVerb verb) { - String url = nextEntry.getRequest().getUrl(); - if (isBlank(url)) { - throw new InvalidRequestException(getContext().getLocalizer().getMessage(BaseHapiFhirSystemDao.class, "transactionMissingUrl", verb.name())); - } - return url; - } - - /** - * This method is called for nested bundles (e.g. if we received a transaction with an entry that - * was a GET search, this method is called on the bundle for the search result, that will be placed in the - * outer bundle). This method applies the _summary and _content parameters to the output of - * that bundle. - * - * TODO: This isn't the most efficient way of doing this.. hopefully we can come up with something better in the future. - */ - private IBaseResource filterNestedBundle(RequestDetails theRequestDetails, IBaseResource theResource) { - IParser p = getContext().newJsonParser(); - RestfulServerUtils.configureResponseParser(theRequestDetails, p); - return p.parseResource(theResource.getClass(), p.encodeResourceToString(theResource)); - } - - private IFhirResourceDao getDaoOrThrowException(Class theClass) { - IFhirResourceDao retVal = getDao(theClass); - if (retVal == null) { - throw new InvalidRequestException("Unable to process request, this server does not know how to handle resources of type " + getContext().getResourceDefinition(theClass).getName()); - } - return retVal; - } - - @Override - public Meta metaGetOperation(RequestDetails theRequestDetails) { - // Notify interceptors - ActionRequestDetails requestDetails = new ActionRequestDetails(theRequestDetails); - notifyInterceptors(RestOperationTypeEnum.META, requestDetails); - - String sql = "SELECT d FROM TagDefinition d WHERE d.myId IN (SELECT DISTINCT t.myTagId FROM ResourceTag t)"; - TypedQuery q = myEntityManager.createQuery(sql, TagDefinition.class); - List tagDefinitions = q.getResultList(); - - Meta retVal = toMeta(tagDefinitions); - - return retVal; - } - - protected Meta toMeta(Collection tagDefinitions) { - Meta retVal = new Meta(); - for (TagDefinition next : tagDefinitions) { - switch (next.getTagType()) { - case PROFILE: - retVal.addProfile(next.getCode()); - break; - case SECURITY_LABEL: - retVal.addSecurity().setSystem(next.getSystem()).setCode(next.getCode()).setDisplay(next.getDisplay()); - break; - case TAG: - retVal.addTag().setSystem(next.getSystem()).setCode(next.getCode()).setDisplay(next.getDisplay()); - break; - } - } - return retVal; - } - - private ca.uhn.fhir.jpa.dao.IFhirResourceDao toDao(UrlParts theParts, String theVerb, String theUrl) { - RuntimeResourceDefinition resType; - try { - resType = getContext().getResourceDefinition(theParts.getResourceType()); - } catch (DataFormatException e) { - String msg = getContext().getLocalizer().getMessage(BaseHapiFhirSystemDao.class, "transactionInvalidUrl", theVerb, theUrl); - throw new InvalidRequestException(msg); - } - IFhirResourceDao dao = null; - if (resType != null) { - dao = getDao(resType.getImplementingClass()); - } - if (dao == null) { - String msg = getContext().getLocalizer().getMessage(BaseHapiFhirSystemDao.class, "transactionInvalidUrl", theVerb, theUrl); - throw new InvalidRequestException(msg); - } - - // if (theParts.getResourceId() == null && theParts.getParams() == null) { - // String msg = getContext().getLocalizer().getMessage(BaseHapiFhirSystemDao.class, "transactionInvalidUrl", theVerb, theUrl); - // throw new InvalidRequestException(msg); - // } - - return dao; - } - - @Transactional(propagation = Propagation.REQUIRED) - @Override - public Bundle transaction(RequestDetails theRequestDetails, Bundle theRequest) { - if (theRequestDetails != null) { - ActionRequestDetails requestDetails = new ActionRequestDetails(theRequestDetails, theRequest, "Bundle", null); - notifyInterceptors(RestOperationTypeEnum.TRANSACTION, requestDetails); - } - - String actionName = "Transaction"; - return transaction((ServletRequestDetails) theRequestDetails, theRequest, actionName); - } - - private Bundle transaction(ServletRequestDetails theRequestDetails, Bundle theRequest, String theActionName) { - super.markRequestAsProcessingSubRequest(theRequestDetails); - try { - return doTransaction(theRequestDetails, theRequest, theActionName); - } finally { - super.clearRequestAsProcessingSubRequest(theRequestDetails); - } - } - - @SuppressWarnings("unchecked") - private Bundle doTransaction(ServletRequestDetails theRequestDetails, Bundle theRequest, String theActionName) { + private Bundle doTransaction(final ServletRequestDetails theRequestDetails, final Bundle theRequest, final String theActionName) { BundleType transactionType = theRequest.getTypeElement().getValue(); if (transactionType == BundleType.BATCH) { return batch(theRequestDetails, theRequest); @@ -301,11 +153,11 @@ public class FhirSystemDaoDstu3 extends BaseHapiFhirSystemDao { ourLog.info("Beginning {} with {} resources", theActionName, theRequest.getEntry().size()); long start = System.currentTimeMillis(); - Date updateTime = new Date(); + final Date updateTime = new Date(); - Set allIds = new LinkedHashSet(); - Map idSubstitutions = new HashMap(); - Map idToPersistedOutcome = new HashMap(); + final Set allIds = new LinkedHashSet(); + final Map idSubstitutions = new HashMap(); + final Map idToPersistedOutcome = new HashMap(); // Do all entries have a verb? for (int i = 0; i < theRequest.getEntry().size(); i++) { @@ -326,9 +178,9 @@ public class FhirSystemDaoDstu3 extends BaseHapiFhirSystemDao { * are saved in a two-phase way in order to deal with interdependencies, and * we want the GET processing to use the final indexing state */ - Bundle response = new Bundle(); + final Bundle response = new Bundle(); List getEntries = new ArrayList(); - IdentityHashMap originalRequestOrder = new IdentityHashMap(); + final IdentityHashMap originalRequestOrder = new IdentityHashMap(); for (int i = 0; i < theRequest.getEntry().size(); i++) { originalRequestOrder.put(theRequest.getEntry().get(i), i); response.addEntry(); @@ -338,6 +190,108 @@ public class FhirSystemDaoDstu3 extends BaseHapiFhirSystemDao { } Collections.sort(theRequest.getEntry(), new TransactionSorter()); + /* + * All of the write operations in the transaction (PUT, POST, etc.. basically anything + * except GET) are performed in their own database transaction before we do the reads. + * We do this because the reads (specifically the searches) often spawn their own + * secondary database transaction and if we allow that within the primary + * database transaction we can end up with deadlocks if the server is under + * heavy load with lots of concurrent transactions using all available + * database connections. + */ + TransactionTemplate txManager = new TransactionTemplate(myTxManager); + Map entriesToProcess = txManager.execute(new TransactionCallback>() { + @Override + public Map doInTransaction(TransactionStatus status) { + return doTransactionWriteOperations(theRequestDetails, theRequest, theActionName, updateTime, allIds, idSubstitutions, idToPersistedOutcome, response, originalRequestOrder); + } + }); + for (Entry nextEntry : entriesToProcess.entrySet()) { + String responseLocation = nextEntry.getValue().getIdDt().toUnqualified().getValue(); + String responseEtag = nextEntry.getValue().getIdDt().getVersionIdPart(); + nextEntry.getKey().getResponse().setLocation(responseLocation); + nextEntry.getKey().getResponse().setEtag(responseEtag); + } + + /* + * Loop through the request and process any entries of type GET + */ + for (int i = 0; i < getEntries.size(); i++) { + BundleEntryComponent nextReqEntry = getEntries.get(i); + Integer originalOrder = originalRequestOrder.get(nextReqEntry); + BundleEntryComponent nextRespEntry = response.getEntry().get(originalOrder); + + ServletSubRequestDetails requestDetails = new ServletSubRequestDetails(); + requestDetails.setServletRequest(theRequestDetails.getServletRequest()); + requestDetails.setRequestType(RequestTypeEnum.GET); + requestDetails.setServer(theRequestDetails.getServer()); + + String url = extractTransactionUrlOrThrowException(nextReqEntry, HTTPVerb.GET); + + int qIndex = url.indexOf('?'); + ArrayListMultimap paramValues = ArrayListMultimap.create(); + requestDetails.setParameters(new HashMap()); + if (qIndex != -1) { + String params = url.substring(qIndex); + List parameters = translateMatchUrl(params); + for (NameValuePair next : parameters) { + paramValues.put(next.getName(), next.getValue()); + } + for (java.util.Map.Entry> nextParamEntry : paramValues.asMap().entrySet()) { + String[] nextValue = nextParamEntry.getValue().toArray(new String[nextParamEntry.getValue().size()]); + requestDetails.getParameters().put(nextParamEntry.getKey(), nextValue); + } + url = url.substring(0, qIndex); + } + + requestDetails.setRequestPath(url); + requestDetails.setFhirServerBase(theRequestDetails.getFhirServerBase()); + + theRequestDetails.getServer().populateRequestDetailsFromRequestPath(requestDetails, url); + BaseMethodBinding method = theRequestDetails.getServer().determineResourceMethod(requestDetails, url); + if (method == null) { + throw new IllegalArgumentException("Unable to handle GET " + url); + } + + if (isNotBlank(nextReqEntry.getRequest().getIfMatch())) { + requestDetails.addHeader(Constants.HEADER_IF_MATCH, nextReqEntry.getRequest().getIfMatch()); + } + if (isNotBlank(nextReqEntry.getRequest().getIfNoneExist())) { + requestDetails.addHeader(Constants.HEADER_IF_NONE_EXIST, nextReqEntry.getRequest().getIfNoneExist()); + } + if (isNotBlank(nextReqEntry.getRequest().getIfNoneMatch())) { + requestDetails.addHeader(Constants.HEADER_IF_NONE_MATCH, nextReqEntry.getRequest().getIfNoneMatch()); + } + + Validate.isTrue(method instanceof BaseResourceReturningMethodBinding, "Unable to handle GET {}", url); + try { + ResourceOrDstu1Bundle responseData = ((BaseResourceReturningMethodBinding) method).doInvokeServer(theRequestDetails.getServer(), requestDetails); + IBaseResource resource = responseData.getResource(); + if (paramValues.containsKey(Constants.PARAM_SUMMARY) || paramValues.containsKey(Constants.PARAM_CONTENT)) { + resource = filterNestedBundle(requestDetails, resource); + } + nextRespEntry.setResource((Resource) resource); + nextRespEntry.getResponse().setStatus(toStatusString(Constants.STATUS_HTTP_200_OK)); + } catch (NotModifiedException e) { + nextRespEntry.getResponse().setStatus(toStatusString(Constants.STATUS_HTTP_304_NOT_MODIFIED)); + } catch (BaseServerResponseException e) { + ourLog.info("Failure processing transaction GET {}: {}", url, e.toString()); + nextRespEntry.getResponse().setStatus(toStatusString(e.getStatusCode())); + populateEntryWithOperationOutcome(e, nextRespEntry); + } + + } + + long delay = System.currentTimeMillis() - start; + ourLog.info(theActionName + " completed in {}ms", new Object[] { delay }); + + response.setType(BundleType.TRANSACTIONRESPONSE); + return response; + } + + @SuppressWarnings("unchecked") + private Map doTransactionWriteOperations(ServletRequestDetails theRequestDetails, Bundle theRequest, String theActionName, Date updateTime, Set allIds, + Map idSubstitutions, Map idToPersistedOutcome, Bundle response, IdentityHashMap originalRequestOrder) { Set deletedResources = new HashSet(); List deleteConflicts = new ArrayList(); Map entriesToProcess = new IdentityHashMap(); @@ -562,87 +516,122 @@ public class FhirSystemDaoDstu3 extends BaseHapiFhirSystemDao { } ourLog.info("Placeholder resource ID \"{}\" was replaced with permanent ID \"{}\"", next, replacement); } + return entriesToProcess; + } - /* - * Loop through the request and process any entries of type GET - */ - for (int i = 0; i < getEntries.size(); i++) { - BundleEntryComponent nextReqEntry = getEntries.get(i); - Integer originalOrder = originalRequestOrder.get(nextReqEntry); - BundleEntryComponent nextRespEntry = response.getEntry().get(originalOrder); + private String extractTransactionUrlOrThrowException(BundleEntryComponent nextEntry, HTTPVerb verb) { + String url = nextEntry.getRequest().getUrl(); + if (isBlank(url)) { + throw new InvalidRequestException(getContext().getLocalizer().getMessage(BaseHapiFhirSystemDao.class, "transactionMissingUrl", verb.name())); + } + return url; + } - ServletSubRequestDetails requestDetails = new ServletSubRequestDetails(); - requestDetails.setServletRequest(theRequestDetails.getServletRequest()); - requestDetails.setRequestType(RequestTypeEnum.GET); - requestDetails.setServer(theRequestDetails.getServer()); + /** + * This method is called for nested bundles (e.g. if we received a transaction with an entry that + * was a GET search, this method is called on the bundle for the search result, that will be placed in the + * outer bundle). This method applies the _summary and _content parameters to the output of + * that bundle. + * + * TODO: This isn't the most efficient way of doing this.. hopefully we can come up with something better in the future. + */ + private IBaseResource filterNestedBundle(RequestDetails theRequestDetails, IBaseResource theResource) { + IParser p = getContext().newJsonParser(); + RestfulServerUtils.configureResponseParser(theRequestDetails, p); + return p.parseResource(theResource.getClass(), p.encodeResourceToString(theResource)); + } - String url = extractTransactionUrlOrThrowException(nextReqEntry, HTTPVerb.GET); + private IFhirResourceDao getDaoOrThrowException(Class theClass) { + IFhirResourceDao retVal = getDao(theClass); + if (retVal == null) { + throw new InvalidRequestException("Unable to process request, this server does not know how to handle resources of type " + getContext().getResourceDefinition(theClass).getName()); + } + return retVal; + } - int qIndex = url.indexOf('?'); - ArrayListMultimap paramValues = ArrayListMultimap.create(); - requestDetails.setParameters(new HashMap()); - if (qIndex != -1) { - String params = url.substring(qIndex); - List parameters = translateMatchUrl(params); - for (NameValuePair next : parameters) { - paramValues.put(next.getName(), next.getValue()); - } - for (java.util.Map.Entry> nextParamEntry : paramValues.asMap().entrySet()) { - String[] nextValue = nextParamEntry.getValue().toArray(new String[nextParamEntry.getValue().size()]); - requestDetails.getParameters().put(nextParamEntry.getKey(), nextValue); - } - url = url.substring(0, qIndex); - } + @Override + public Meta metaGetOperation(RequestDetails theRequestDetails) { + // Notify interceptors + ActionRequestDetails requestDetails = new ActionRequestDetails(theRequestDetails); + notifyInterceptors(RestOperationTypeEnum.META, requestDetails); - requestDetails.setRequestPath(url); - requestDetails.setFhirServerBase(theRequestDetails.getFhirServerBase()); + String sql = "SELECT d FROM TagDefinition d WHERE d.myId IN (SELECT DISTINCT t.myTagId FROM ResourceTag t)"; + TypedQuery q = myEntityManager.createQuery(sql, TagDefinition.class); + List tagDefinitions = q.getResultList(); - theRequestDetails.getServer().populateRequestDetailsFromRequestPath(requestDetails, url); - BaseMethodBinding method = theRequestDetails.getServer().determineResourceMethod(requestDetails, url); - if (method == null) { - throw new IllegalArgumentException("Unable to handle GET " + url); - } + Meta retVal = toMeta(tagDefinitions); - if (isNotBlank(nextReqEntry.getRequest().getIfMatch())) { - requestDetails.addHeader(Constants.HEADER_IF_MATCH, nextReqEntry.getRequest().getIfMatch()); - } - if (isNotBlank(nextReqEntry.getRequest().getIfNoneExist())) { - requestDetails.addHeader(Constants.HEADER_IF_NONE_EXIST, nextReqEntry.getRequest().getIfNoneExist()); - } - if (isNotBlank(nextReqEntry.getRequest().getIfNoneMatch())) { - requestDetails.addHeader(Constants.HEADER_IF_NONE_MATCH, nextReqEntry.getRequest().getIfNoneMatch()); - } + return retVal; + } - if (method instanceof BaseResourceReturningMethodBinding) { - try { - ResourceOrDstu1Bundle responseData = ((BaseResourceReturningMethodBinding) method).doInvokeServer(theRequestDetails.getServer(), requestDetails); - IBaseResource resource = responseData.getResource(); - if (paramValues.containsKey(Constants.PARAM_SUMMARY) || paramValues.containsKey(Constants.PARAM_CONTENT)) { - resource = filterNestedBundle(requestDetails, resource); - } - nextRespEntry.setResource((Resource) resource); - nextRespEntry.getResponse().setStatus(toStatusString(Constants.STATUS_HTTP_200_OK)); - } catch (NotModifiedException e) { - nextRespEntry.getResponse().setStatus(toStatusString(Constants.STATUS_HTTP_304_NOT_MODIFIED)); - } - } else { - throw new IllegalArgumentException("Unable to handle GET " + url); - } + private void populateEntryWithOperationOutcome(BaseServerResponseException caughtEx, BundleEntryComponent nextEntry) { + OperationOutcome oo = new OperationOutcome(); + oo.addIssue().setSeverity(IssueSeverity.ERROR).setDiagnostics(caughtEx.getMessage()); + nextEntry.getResponse().setOutcome(oo); + } + private ca.uhn.fhir.jpa.dao.IFhirResourceDao toDao(UrlParts theParts, String theVerb, String theUrl) { + RuntimeResourceDefinition resType; + try { + resType = getContext().getResourceDefinition(theParts.getResourceType()); + } catch (DataFormatException e) { + String msg = getContext().getLocalizer().getMessage(BaseHapiFhirSystemDao.class, "transactionInvalidUrl", theVerb, theUrl); + throw new InvalidRequestException(msg); + } + IFhirResourceDao dao = null; + if (resType != null) { + dao = getDao(resType.getImplementingClass()); + } + if (dao == null) { + String msg = getContext().getLocalizer().getMessage(BaseHapiFhirSystemDao.class, "transactionInvalidUrl", theVerb, theUrl); + throw new InvalidRequestException(msg); } - for (Entry nextEntry : entriesToProcess.entrySet()) { - String responseLocation = nextEntry.getValue().getIdDt().toUnqualified().getValue(); - String responseEtag = nextEntry.getValue().getIdDt().getVersionIdPart(); - nextEntry.getKey().getResponse().setLocation(responseLocation); - nextEntry.getKey().getResponse().setEtag(responseEtag); + // if (theParts.getResourceId() == null && theParts.getParams() == null) { + // String msg = getContext().getLocalizer().getMessage(BaseHapiFhirSystemDao.class, "transactionInvalidUrl", theVerb, theUrl); + // throw new InvalidRequestException(msg); + // } + + return dao; + } + + protected Meta toMeta(Collection tagDefinitions) { + Meta retVal = new Meta(); + for (TagDefinition next : tagDefinitions) { + switch (next.getTagType()) { + case PROFILE: + retVal.addProfile(next.getCode()); + break; + case SECURITY_LABEL: + retVal.addSecurity().setSystem(next.getSystem()).setCode(next.getCode()).setDisplay(next.getDisplay()); + break; + case TAG: + retVal.addTag().setSystem(next.getSystem()).setCode(next.getCode()).setDisplay(next.getDisplay()); + break; + } + } + return retVal; + } + + @Transactional(propagation = Propagation.NEVER) + @Override + public Bundle transaction(RequestDetails theRequestDetails, Bundle theRequest) { + if (theRequestDetails != null) { + ActionRequestDetails requestDetails = new ActionRequestDetails(theRequestDetails, theRequest, "Bundle", null); + notifyInterceptors(RestOperationTypeEnum.TRANSACTION, requestDetails); } - long delay = System.currentTimeMillis() - start; - ourLog.info(theActionName + " completed in {}ms", new Object[] { delay }); + String actionName = "Transaction"; + return transaction((ServletRequestDetails) theRequestDetails, theRequest, actionName); + } - response.setType(BundleType.TRANSACTIONRESPONSE); - return response; + private Bundle transaction(ServletRequestDetails theRequestDetails, Bundle theRequest, String theActionName) { + super.markRequestAsProcessingSubRequest(theRequestDetails); + try { + return doTransaction(theRequestDetails, theRequest, theActionName); + } finally { + super.clearRequestAsProcessingSubRequest(theRequestDetails); + } } private static void handleTransactionCreateOrUpdateOutcome(Map idSubstitutions, Map idToPersistedOutcome, IdType nextResourceId, DaoMethodOutcome outcome, @@ -693,6 +682,19 @@ public class FhirSystemDaoDstu3 extends BaseHapiFhirSystemDao { return Integer.toString(theStatusCode) + " " + defaultString(Constants.HTTP_STATUS_NAMES.get(theStatusCode)); } + private static class BaseServerResponseExceptionHolder + { + private BaseServerResponseException myException; + + public BaseServerResponseException getException() { + return myException; + } + + public void setException(BaseServerResponseException myException) { + this.myException = myException; + } + } + //@formatter:off /** * Transaction Order, per the spec: diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/entity/SearchResult.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/entity/SearchResult.java index 02302e89db4..e0b3a163fac 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/entity/SearchResult.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/entity/SearchResult.java @@ -34,12 +34,10 @@ import javax.persistence.SequenceGenerator; import javax.persistence.Table; import javax.persistence.UniqueConstraint; -//@formatter:off @Entity @Table(name = "HFJ_SEARCH_RESULT", uniqueConstraints= { @UniqueConstraint(name="IDX_SEARCHRES_ORDER", columnNames= {"SEARCH_PID", "SEARCH_ORDER"}) }) -//@formatter:on public class SearchResult implements Serializable { private static final long serialVersionUID = 1L; diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/StaleSearchDeletingSvcImpl.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/StaleSearchDeletingSvcImpl.java index 8e85287d683..9c5a2c56be7 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/StaleSearchDeletingSvcImpl.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/StaleSearchDeletingSvcImpl.java @@ -1,5 +1,25 @@ package ca.uhn.fhir.jpa.search; +/*- + * #%L + * HAPI FHIR JPA Server + * %% + * Copyright (C) 2014 - 2017 University Health Network + * %% + * 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% + */ + import java.util.Date; import org.apache.commons.lang3.time.DateUtils; diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/config/TestDstu3Config.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/config/TestDstu3Config.java index 0a29df52c1c..b0e9612cbda 100644 --- a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/config/TestDstu3Config.java +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/config/TestDstu3Config.java @@ -44,6 +44,7 @@ public class TestDstu3Config extends BaseJavaConfigDstu3 { * starvation */ int maxThreads = (int)(Math.random() * 6) + 2; + maxThreads = 2; retVal.setMaxTotal(maxThreads); DataSource dataSource = ProxyDataSourceBuilder diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/dstu3/FhirSystemDaoDstu3Test.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/dstu3/FhirSystemDaoDstu3Test.java index 1090bfdd9d0..5ecd70784f8 100644 --- a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/dstu3/FhirSystemDaoDstu3Test.java +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/dstu3/FhirSystemDaoDstu3Test.java @@ -26,31 +26,14 @@ import java.nio.charset.StandardCharsets; import java.util.*; import org.apache.commons.io.IOUtils; -import org.hl7.fhir.dstu3.model.Appointment; -import org.hl7.fhir.dstu3.model.Bundle; +import org.hl7.fhir.dstu3.model.*; import org.hl7.fhir.dstu3.model.Bundle.BundleEntryComponent; import org.hl7.fhir.dstu3.model.Bundle.BundleEntryRequestComponent; import org.hl7.fhir.dstu3.model.Bundle.BundleEntryResponseComponent; import org.hl7.fhir.dstu3.model.Bundle.BundleType; import org.hl7.fhir.dstu3.model.Bundle.HTTPVerb; -import org.hl7.fhir.dstu3.model.Coding; -import org.hl7.fhir.dstu3.model.Condition; -import org.hl7.fhir.dstu3.model.DiagnosticReport; -import org.hl7.fhir.dstu3.model.Encounter; -import org.hl7.fhir.dstu3.model.EpisodeOfCare; -import org.hl7.fhir.dstu3.model.IdType; -import org.hl7.fhir.dstu3.model.Medication; -import org.hl7.fhir.dstu3.model.MedicationRequest; -import org.hl7.fhir.dstu3.model.Meta; -import org.hl7.fhir.dstu3.model.Observation; import org.hl7.fhir.dstu3.model.Observation.ObservationStatus; -import org.hl7.fhir.dstu3.model.OperationOutcome; import org.hl7.fhir.dstu3.model.OperationOutcome.IssueSeverity; -import org.hl7.fhir.dstu3.model.Organization; -import org.hl7.fhir.dstu3.model.Patient; -import org.hl7.fhir.dstu3.model.Reference; -import org.hl7.fhir.dstu3.model.UriType; -import org.hl7.fhir.dstu3.model.ValueSet; import org.hl7.fhir.instance.model.api.IAnyResource; import org.hl7.fhir.instance.model.api.IIdType; import org.junit.After; @@ -497,9 +480,10 @@ public class FhirSystemDaoDstu3Test extends BaseJpaDstu3SystemTest { assertThat(resp.getEntry().get(0).getResponse().getLocation(), startsWith("Patient/")); // Bundle.entry[1] is failed read response - assertEquals(OperationOutcome.class, resp.getEntry().get(1).getResource().getClass()); - assertEquals(IssueSeverity.ERROR, ((OperationOutcome) resp.getEntry().get(1).getResource()).getIssue().get(0).getSeverityElement().getValue()); - assertEquals("Resource Patient/THIS_ID_DOESNT_EXIST is not known", ((OperationOutcome) resp.getEntry().get(1).getResource()).getIssue().get(0).getDiagnostics()); + Resource oo = resp.getEntry().get(1).getResponse().getOutcome(); + assertEquals(OperationOutcome.class, oo.getClass()); + assertEquals(IssueSeverity.ERROR, ((OperationOutcome) oo).getIssue().get(0).getSeverityElement().getValue()); + assertEquals("Resource Patient/THIS_ID_DOESNT_EXIST is not known", ((OperationOutcome) oo).getIssue().get(0).getDiagnostics()); assertEquals("404 Not Found", resp.getEntry().get(1).getResponse().getStatus()); // Check POST @@ -891,6 +875,143 @@ public class FhirSystemDaoDstu3Test extends BaseJpaDstu3SystemTest { } } + @Test + public void testTransactionCreateWithBadRead() { + Bundle request = new Bundle(); + request.setType(BundleType.TRANSACTION); + + Patient p; + p = new Patient(); + p.addIdentifier().setSystem("urn:system").setValue("FOO"); + request + .addEntry() + .setResource(p) + .getRequest() + .setMethod(HTTPVerb.POST) + .setUrl("Patient"); + + request + .addEntry() + .getRequest() + .setMethod(HTTPVerb.GET) + .setUrl("Patient/BABABABA"); + + Bundle response = mySystemDao.transaction(mySrd, request); + assertEquals(2, response.getEntry().size()); + + assertEquals("201 Created", response.getEntry().get(0).getResponse().getStatus()); + assertThat(response.getEntry().get(0).getResponse().getLocation(), matchesPattern(".*Patient/[0-9]+.*")); + assertEquals("404 Not Found", response.getEntry().get(1).getResponse().getStatus()); + + OperationOutcome oo = (OperationOutcome) response.getEntry().get(1).getResponse().getOutcome(); + ourLog.info(myFhirCtx.newXmlParser().setPrettyPrint(true).encodeResourceToString(oo)); + assertEquals(IssueSeverity.ERROR, oo.getIssue().get(0).getSeverity()); + assertEquals("Resource Patient/BABABABA is not known", oo.getIssue().get(0).getDiagnostics()); + } + + @Test + public void testTransactionCreateWithBadSearch() { + Bundle request = new Bundle(); + request.setType(BundleType.TRANSACTION); + + Patient p; + p = new Patient(); + p.addIdentifier().setSystem("urn:system").setValue("FOO"); + request + .addEntry() + .setResource(p) + .getRequest() + .setMethod(HTTPVerb.POST) + .setUrl("Patient"); + + request + .addEntry() + .getRequest() + .setMethod(HTTPVerb.GET) + .setUrl("Patient?foobadparam=1"); + + Bundle response = mySystemDao.transaction(mySrd, request); + assertEquals(2, response.getEntry().size()); + + assertEquals("201 Created", response.getEntry().get(0).getResponse().getStatus()); + assertThat(response.getEntry().get(0).getResponse().getLocation(), matchesPattern(".*Patient/[0-9]+.*")); + assertEquals("400 Bad Request", response.getEntry().get(1).getResponse().getStatus()); + + OperationOutcome oo = (OperationOutcome) response.getEntry().get(1).getResponse().getOutcome(); + ourLog.info(myFhirCtx.newXmlParser().setPrettyPrint(true).encodeResourceToString(oo)); + assertEquals(IssueSeverity.ERROR, oo.getIssue().get(0).getSeverity()); + assertThat(oo.getIssue().get(0).getDiagnostics(), containsString("Unknown search parameter")); + } + + @Test + public void testBatchCreateWithBadRead() { + Bundle request = new Bundle(); + request.setType(BundleType.BATCH); + + Patient p; + p = new Patient(); + p.addIdentifier().setSystem("urn:system").setValue("FOO"); + request + .addEntry() + .setResource(p) + .getRequest() + .setMethod(HTTPVerb.POST) + .setUrl("Patient"); + + request + .addEntry() + .getRequest() + .setMethod(HTTPVerb.GET) + .setUrl("Patient/BABABABA"); + + Bundle response = mySystemDao.transaction(mySrd, request); + assertEquals(2, response.getEntry().size()); + + assertEquals("201 Created", response.getEntry().get(0).getResponse().getStatus()); + assertThat(response.getEntry().get(0).getResponse().getLocation(), matchesPattern(".*Patient/[0-9]+.*")); + assertEquals("404 Not Found", response.getEntry().get(1).getResponse().getStatus()); + + OperationOutcome oo = (OperationOutcome) response.getEntry().get(1).getResponse().getOutcome(); + ourLog.info(myFhirCtx.newXmlParser().setPrettyPrint(true).encodeResourceToString(oo)); + assertEquals(IssueSeverity.ERROR, oo.getIssue().get(0).getSeverity()); + assertEquals("Resource Patient/BABABABA is not known", oo.getIssue().get(0).getDiagnostics()); + } + + @Test + public void testBatchCreateWithBadSearch() { + Bundle request = new Bundle(); + request.setType(BundleType.BATCH); + + Patient p; + p = new Patient(); + p.addIdentifier().setSystem("urn:system").setValue("FOO"); + request + .addEntry() + .setResource(p) + .getRequest() + .setMethod(HTTPVerb.POST) + .setUrl("Patient"); + + request + .addEntry() + .getRequest() + .setMethod(HTTPVerb.GET) + .setUrl("Patient?foobadparam=1"); + + Bundle response = mySystemDao.transaction(mySrd, request); + assertEquals(2, response.getEntry().size()); + + assertEquals("201 Created", response.getEntry().get(0).getResponse().getStatus()); + assertThat(response.getEntry().get(0).getResponse().getLocation(), matchesPattern(".*Patient/[0-9]+.*")); + assertEquals("400 Bad Request", response.getEntry().get(1).getResponse().getStatus()); + + OperationOutcome oo = (OperationOutcome) response.getEntry().get(1).getResponse().getOutcome(); + ourLog.info(myFhirCtx.newXmlParser().setPrettyPrint(true).encodeResourceToString(oo)); + assertEquals(IssueSeverity.ERROR, oo.getIssue().get(0).getSeverity()); + assertThat(oo.getIssue().get(0).getDiagnostics(), containsString("Unknown search parameter")); + } + + @Test public void testTransactionCreateWithDuplicateMatchUrl02() { String methodName = "testTransactionCreateWithDuplicateMatchUrl02"; @@ -1331,20 +1452,15 @@ public class FhirSystemDaoDstu3Test extends BaseJpaDstu3SystemTest { @Test public void testTransactionDoesNotLeavePlaceholderIds() throws Exception { - newTxTemplate().execute(new TransactionCallbackWithoutResult() { - @Override - protected void doInTransactionWithoutResult(TransactionStatus theStatus) { - String input; - try { - input = IOUtils.toString(getClass().getResourceAsStream("/cdr-bundle.json"), StandardCharsets.UTF_8); - } catch (IOException e) { - fail(e.toString()); - return; - } - Bundle bundle = myFhirCtx.newJsonParser().parseResource(Bundle.class, input); - mySystemDao.transaction(mySrd, bundle); - } - }); + String input; + try { + input = IOUtils.toString(getClass().getResourceAsStream("/cdr-bundle.json"), StandardCharsets.UTF_8); + } catch (IOException e) { + fail(e.toString()); + return; + } + Bundle bundle = myFhirCtx.newJsonParser().parseResource(Bundle.class, input); + mySystemDao.transaction(mySrd, bundle); IBundleProvider history = mySystemDao.history(null, null, null); Bundle list = toBundle(history); diff --git a/src/changes/changes.xml b/src/changes/changes.xml index 54fd5cc0991..87f2b827367 100644 --- a/src/changes/changes.xml +++ b/src/changes/changes.xml @@ -177,8 +177,17 @@ GitHub user @CarthageKing for the pull request! - Fix potential deadlock in stale search deleting task in JPA server + Fix potential deadlock in stale search deleting task in JPA server, as well + as potential deadlock when executing transactions containing nested + searches when operating under extremely heavy load. + + JPA server transaction operations now put OperationOutcome resources resulting + from actions in + Bundle.entry.response.outcome]]> + instead of the previous + Bundle.entry.resource]]> + From fa050c466559f2376c5efd18929893552251211e Mon Sep 17 00:00:00 2001 From: James Date: Fri, 21 Jul 2017 08:12:22 -0400 Subject: [PATCH 22/28] Restore random connection count to tests --- .../src/test/java/ca/uhn/fhir/jpa/config/TestDstu3Config.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/config/TestDstu3Config.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/config/TestDstu3Config.java index b0e9612cbda..3a2e97ed30b 100644 --- a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/config/TestDstu3Config.java +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/config/TestDstu3Config.java @@ -43,8 +43,7 @@ public class TestDstu3Config extends BaseJavaConfigDstu3 { * and catch any potential deadlocks caused by database connection * starvation */ - int maxThreads = (int)(Math.random() * 6) + 2; - maxThreads = 2; + int maxThreads = (int)(Math.random() * 6) + 1; retVal.setMaxTotal(maxThreads); DataSource dataSource = ProxyDataSourceBuilder From f5f1f5bd675f5a6192a5b4fc5655a68ff58be5fc Mon Sep 17 00:00:00 2001 From: James Agnew Date: Fri, 21 Jul 2017 18:40:40 -0400 Subject: [PATCH 23/28] More work on detecting deadlocks --- .../jpa/search/SearchCoordinatorSvcImpl.java | 77 ++--- .../fhir/jpa/config/ConnectionWrapper.java | 289 ++++++++++++++++++ .../uhn/fhir/jpa/config/TestDstu3Config.java | 58 +++- 3 files changed, 372 insertions(+), 52 deletions(-) create mode 100644 hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/config/ConnectionWrapper.java 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 8e1e447de63..3a716126bcf 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 @@ -78,13 +78,13 @@ public class SearchCoordinatorSvcImpl implements ISearchCoordinatorSvc { private int mySyncSize = DEFAULT_SYNC_SIZE; -// @Autowired -// private DataSource myDataSource; -// @PostConstruct -// public void start() { -// JpaTransactionManager txManager = (JpaTransactionManager) myManagedTxManager; -// } - + // @Autowired + // private DataSource myDataSource; + // @PostConstruct + // public void start() { + // JpaTransactionManager txManager = (JpaTransactionManager) myManagedTxManager; + // } + /** * Constructor */ @@ -106,7 +106,7 @@ public class SearchCoordinatorSvcImpl implements ISearchCoordinatorSvc { } @Override - @Transactional(propagation=Propagation.NEVER) + @Transactional(propagation = Propagation.NEVER) public List getResources(final String theUuid, int theFrom, int theTo) { if (myNeverUseLocalSearchForUnitTests == false) { SearchTask task = myIdToSearchTask.get(theUuid); @@ -186,9 +186,9 @@ public class SearchCoordinatorSvcImpl implements ISearchCoordinatorSvc { } @Override - public IBundleProvider registerSearch(final IDao theCallingDao, SearchParameterMap theParams, String theResourceType) { + public IBundleProvider registerSearch(final IDao theCallingDao, final SearchParameterMap theParams, String theResourceType) { StopWatch w = new StopWatch(); - String searchUuid = UUID.randomUUID().toString(); + final String searchUuid = UUID.randomUUID().toString(); Class resourceTypeClass = myContext.getResourceDefinition(theResourceType).getImplementingClass(); final ISearchBuilder sb = theCallingDao.newSearchBuilder(); @@ -196,36 +196,37 @@ public class SearchCoordinatorSvcImpl implements ISearchCoordinatorSvc { if (theParams.isLoadSynchronous()) { - // Load the results synchronously - final List pids = new ArrayList(); - - Iterator resultIter = sb.createQuery(theParams, searchUuid); - while (resultIter.hasNext()) { - pids.add(resultIter.next()); - if (theParams.getLoadSynchronousUpTo() != null && pids.size() >= theParams.getLoadSynchronousUpTo()) { - break; - } - } - - /* - * For synchronous queries, we load all the includes right away - * since we're returning a static bundle with all the results - * pre-loaded. This is ok because syncronous requests are not - * expected to be paged - * - * On the other hand for async queries we load includes/revincludes - * individually for pages as we return them to clients - */ - final Set includedPids = new HashSet(); - includedPids.addAll(sb.loadReverseIncludes(theCallingDao, myContext, myEntityManager, pids, theParams.getRevIncludes(), true, theParams.getLastUpdated())); - includedPids.addAll(sb.loadReverseIncludes(theCallingDao, myContext, myEntityManager, pids, theParams.getIncludes(), false, theParams.getLastUpdated())); - // Execute the query and make sure we return distinct results TransactionTemplate txTemplate = new TransactionTemplate(myManagedTxManager); txTemplate.setPropagationBehavior(TransactionTemplate.PROPAGATION_REQUIRED); return txTemplate.execute(new TransactionCallback() { @Override public SimpleBundleProvider doInTransaction(TransactionStatus theStatus) { + + // Load the results synchronously + final List pids = new ArrayList(); + + Iterator resultIter = sb.createQuery(theParams, searchUuid); + while (resultIter.hasNext()) { + pids.add(resultIter.next()); + if (theParams.getLoadSynchronousUpTo() != null && pids.size() >= theParams.getLoadSynchronousUpTo()) { + break; + } + } + + /* + * For synchronous queries, we load all the includes right away + * since we're returning a static bundle with all the results + * pre-loaded. This is ok because syncronous requests are not + * expected to be paged + * + * On the other hand for async queries we load includes/revincludes + * individually for pages as we return them to clients + */ + final Set includedPids = new HashSet(); + includedPids.addAll(sb.loadReverseIncludes(theCallingDao, myContext, myEntityManager, pids, theParams.getRevIncludes(), true, theParams.getLastUpdated())); + includedPids.addAll(sb.loadReverseIncludes(theCallingDao, myContext, myEntityManager, pids, theParams.getIncludes(), false, theParams.getLastUpdated())); + List resources = new ArrayList(); sb.loadResourcesByPid(pids, resources, includedPids, false, myEntityManager, myContext, theCallingDao); return new SimpleBundleProvider(resources); @@ -441,7 +442,7 @@ public class SearchCoordinatorSvcImpl implements ISearchCoordinatorSvc { try { saveSearch(); - + TransactionTemplate txTemplate = new TransactionTemplate(myManagedTxManager); txTemplate.setPropagationBehavior(TransactionTemplate.PROPAGATION_REQUIRES_NEW); txTemplate.execute(new TransactionCallbackWithoutResult() { @@ -454,7 +455,7 @@ public class SearchCoordinatorSvcImpl implements ISearchCoordinatorSvc { ourLog.info("Completed search for {} resources in {}ms", mySyncedPids.size(), sw.getMillis()); } catch (Throwable t) { - + /* * Don't print a stack trace for client errors.. that's just noisy */ @@ -465,8 +466,8 @@ public class SearchCoordinatorSvcImpl implements ISearchCoordinatorSvc { 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); } diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/config/ConnectionWrapper.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/config/ConnectionWrapper.java new file mode 100644 index 00000000000..51872f77f2b --- /dev/null +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/config/ConnectionWrapper.java @@ -0,0 +1,289 @@ +package ca.uhn.fhir.jpa.config; + +import java.sql.*; +import java.util.Map; +import java.util.Properties; +import java.util.concurrent.Executor; + +public class ConnectionWrapper implements Connection { + + private static final org.slf4j.Logger ourLog = org.slf4j.LoggerFactory.getLogger(ConnectionWrapper.class); + + private Connection myWrap; + + public ConnectionWrapper(Connection theConnection) { + myWrap = theConnection; + } + + @Override + public void abort(Executor theExecutor) throws SQLException { + myWrap.abort(theExecutor); + } + + @Override + public void clearWarnings() throws SQLException { + myWrap.clearWarnings(); + } + + @Override + public void close() throws SQLException { + ourLog.info("** Closing connection"); + myWrap.close(); + } + + @Override + public void commit() throws SQLException { + myWrap.commit(); + } + + @Override + public Array createArrayOf(String theTypeName, Object[] theElements) throws SQLException { + return myWrap.createArrayOf(theTypeName, theElements); + } + + @Override + public Blob createBlob() throws SQLException { + return myWrap.createBlob(); + } + + @Override + public Clob createClob() throws SQLException { + return myWrap.createClob(); + } + + @Override + public NClob createNClob() throws SQLException { + return myWrap.createNClob(); + } + + @Override + public SQLXML createSQLXML() throws SQLException { + return myWrap.createSQLXML(); + } + + @Override + public Statement createStatement() throws SQLException { + return myWrap.createStatement(); + } + + @Override + public Statement createStatement(int theResultSetType, int theResultSetConcurrency) throws SQLException { + return myWrap.createStatement(theResultSetType, theResultSetConcurrency); + } + + @Override + public Statement createStatement(int theResultSetType, int theResultSetConcurrency, int theResultSetHoldability) throws SQLException { + return myWrap.createStatement(theResultSetType, theResultSetConcurrency, theResultSetHoldability); + } + + @Override + public Struct createStruct(String theTypeName, Object[] theAttributes) throws SQLException { + return myWrap.createStruct(theTypeName, theAttributes); + } + + @Override + public boolean getAutoCommit() throws SQLException { + return myWrap.getAutoCommit(); + } + + @Override + public String getCatalog() throws SQLException { + return myWrap.getCatalog(); + } + + @Override + public Properties getClientInfo() throws SQLException { + return myWrap.getClientInfo(); + } + + @Override + public String getClientInfo(String theName) throws SQLException { + return getClientInfo(theName); + } + + @Override + public int getHoldability() throws SQLException { + return myWrap.getHoldability(); + } + + @Override + public DatabaseMetaData getMetaData() throws SQLException { + return myWrap.getMetaData(); + } + + @Override + public int getNetworkTimeout() throws SQLException { + return myWrap.getNetworkTimeout(); + } + + @Override + public String getSchema() throws SQLException { + return myWrap.getSchema(); + } + + @Override + public int getTransactionIsolation() throws SQLException { + return myWrap.getTransactionIsolation(); + } + + @Override + public Map> getTypeMap() throws SQLException { + return myWrap.getTypeMap(); + } + + @Override + public SQLWarning getWarnings() throws SQLException { + return myWrap.getWarnings(); + } + + @Override + public boolean isClosed() throws SQLException { + return myWrap.isClosed(); + } + + @Override + public boolean isReadOnly() throws SQLException { + return myWrap.isReadOnly(); + } + + @Override + public boolean isValid(int theTimeout) throws SQLException { + return myWrap.isValid(theTimeout); + } + + @Override + public boolean isWrapperFor(Class theIface) throws SQLException { + return myWrap.isWrapperFor(theIface); + } + + @Override + public String nativeSQL(String theSql) throws SQLException { + return myWrap.nativeSQL(theSql); + } + + @Override + public CallableStatement prepareCall(String theSql) throws SQLException { + return myWrap.prepareCall(theSql); + } + + @Override + public CallableStatement prepareCall(String theSql, int theResultSetType, int theResultSetConcurrency) throws SQLException { + return myWrap.prepareCall(theSql, theResultSetType, theResultSetConcurrency); + } + + @Override + public CallableStatement prepareCall(String theSql, int theResultSetType, int theResultSetConcurrency, int theResultSetHoldability) throws SQLException { + return myWrap.prepareCall(theSql, theResultSetType, theResultSetConcurrency, theResultSetHoldability); + } + + @Override + public PreparedStatement prepareStatement(String theSql) throws SQLException { + return myWrap.prepareStatement(theSql); + } + + @Override + public PreparedStatement prepareStatement(String theSql, int theAutoGeneratedKeys) throws SQLException { + return myWrap.prepareStatement(theSql, theAutoGeneratedKeys); + } + + @Override + public PreparedStatement prepareStatement(String theSql, int theResultSetType, int theResultSetConcurrency) throws SQLException { + return myWrap.prepareStatement(theSql, theResultSetType, theResultSetConcurrency); + } + + @Override + public PreparedStatement prepareStatement(String theSql, int theResultSetType, int theResultSetConcurrency, int theResultSetHoldability) throws SQLException { + return myWrap.prepareStatement(theSql, theResultSetType, theResultSetConcurrency, theResultSetHoldability); + } + + @Override + public PreparedStatement prepareStatement(String theSql, int[] theColumnIndexes) throws SQLException { + return myWrap.prepareStatement(theSql, theColumnIndexes); + } + + @Override + public PreparedStatement prepareStatement(String theSql, String[] theColumnNames) throws SQLException { + return myWrap.prepareStatement(theSql, theColumnNames); + } + + @Override + public void releaseSavepoint(Savepoint theSavepoint) throws SQLException { + myWrap.releaseSavepoint(theSavepoint); + } + + @Override + public void rollback() throws SQLException { + myWrap.rollback(); + } + + @Override + public void rollback(Savepoint theSavepoint) throws SQLException { + myWrap.rollback(theSavepoint); + } + + @Override + public void setAutoCommit(boolean theAutoCommit) throws SQLException { + myWrap.setAutoCommit(theAutoCommit); + } + + @Override + public void setCatalog(String theCatalog) throws SQLException { + myWrap.setCatalog(theCatalog); + } + + @Override + public void setClientInfo(Properties theProperties) throws SQLClientInfoException { + myWrap.setClientInfo(theProperties); + } + + @Override + public void setClientInfo(String theName, String theValue) throws SQLClientInfoException { + myWrap.setClientInfo(theName, theValue); + } + + @Override + public void setHoldability(int theHoldability) throws SQLException { + myWrap.setHoldability(theHoldability); + } + + @Override + public void setNetworkTimeout(Executor theExecutor, int theMilliseconds) throws SQLException { + myWrap.setNetworkTimeout(theExecutor, theMilliseconds); + } + + @Override + public void setReadOnly(boolean theReadOnly) throws SQLException { + myWrap.setReadOnly(theReadOnly); + } + + @Override + public Savepoint setSavepoint() throws SQLException { + return myWrap.setSavepoint(); + } + + @Override + public Savepoint setSavepoint(String theName) throws SQLException { + return myWrap.setSavepoint(theName); + } + + @Override + public void setSchema(String theSchema) throws SQLException { + myWrap.setSchema(theSchema); + } + + @Override + public void setTransactionIsolation(int theLevel) throws SQLException { + myWrap.setTransactionIsolation(theLevel); + } + + @Override + public void setTypeMap(Map> theMap) throws SQLException { + myWrap.setTypeMap(theMap); + } + + @Override + public T unwrap(Class theIface) throws SQLException { + return myWrap.unwrap(theIface); + } + +} \ No newline at end of file diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/config/TestDstu3Config.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/config/TestDstu3Config.java index 3a2e97ed30b..098a1b6d110 100644 --- a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/config/TestDstu3Config.java +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/config/TestDstu3Config.java @@ -1,5 +1,7 @@ package ca.uhn.fhir.jpa.config; +import java.sql.Connection; +import java.sql.SQLException; import java.util.Properties; import java.util.concurrent.TimeUnit; @@ -8,9 +10,7 @@ import javax.sql.DataSource; import org.apache.commons.dbcp2.BasicDataSource; import org.hibernate.jpa.HibernatePersistenceProvider; -import org.springframework.context.annotation.Bean; -import org.springframework.context.annotation.Configuration; -import org.springframework.context.annotation.Lazy; +import org.springframework.context.annotation.*; import org.springframework.orm.jpa.JpaTransactionManager; import org.springframework.orm.jpa.LocalContainerEntityManagerFactoryBean; import org.springframework.transaction.annotation.EnableTransactionManagement; @@ -18,21 +18,51 @@ import org.springframework.transaction.annotation.EnableTransactionManagement; import ca.uhn.fhir.jpa.dao.DaoConfig; import ca.uhn.fhir.rest.server.interceptor.RequestValidatingInterceptor; import ca.uhn.fhir.validation.ResultSeverityEnum; -import net.ttddyy.dsproxy.listener.logging.SLF4JLogLevel; import net.ttddyy.dsproxy.support.ProxyDataSourceBuilder; @Configuration @EnableTransactionManagement() public class TestDstu3Config extends BaseJavaConfigDstu3 { + static final org.slf4j.Logger ourLog = org.slf4j.LoggerFactory.getLogger(TestDstu3Config.class); + @Bean() public DaoConfig daoConfig() { return new DaoConfig(); } + private boolean myLogConnection = false; + @Bean() public DataSource dataSource() { - BasicDataSource retVal = new BasicDataSource(); + BasicDataSource retVal = new BasicDataSource() { + + @Override + public Connection getConnection() throws SQLException { + if (myLogConnection) { + logGetConnectionStackTrace(); + return new ConnectionWrapper(super.getConnection()); + } else { + return super.getConnection(); + } + } + + private void logGetConnectionStackTrace() { + try { + throw new Exception(); + } catch (Exception e) { + StringBuilder b = new StringBuilder(); + b.append("New connection request:"); + for (StackTraceElement next : e.getStackTrace()) { + if (next.getClassName().contains("fhir")) { + b.append("\n ").append(next.getClassName()).append(" ").append(next.getFileName()).append(":").append(next.getLineNumber()); + } + } + ourLog.info(b.toString()); + } + } + + }; retVal.setDriver(new org.apache.derby.jdbc.EmbeddedDriver()); retVal.setUrl("jdbc:derby:memory:myUnitTestDB;create=true"); retVal.setUsername(""); @@ -43,12 +73,12 @@ public class TestDstu3Config extends BaseJavaConfigDstu3 { * and catch any potential deadlocks caused by database connection * starvation */ - int maxThreads = (int)(Math.random() * 6) + 1; + int maxThreads = (int) (Math.random() * 6) + 1; retVal.setMaxTotal(maxThreads); DataSource dataSource = ProxyDataSourceBuilder .create(retVal) -// .logQueryBySlf4j(SLF4JLogLevel.INFO, "SQL") + // .logQueryBySlf4j(SLF4JLogLevel.INFO, "SQL") .logSlowQueryBySlf4j(10, TimeUnit.SECONDS) .countQuery() .build(); @@ -56,13 +86,6 @@ public class TestDstu3Config extends BaseJavaConfigDstu3 { return dataSource; } - @Bean() - public JpaTransactionManager transactionManager(EntityManagerFactory entityManagerFactory) { - JpaTransactionManager retVal = new JpaTransactionManager(); - retVal.setEntityManagerFactory(entityManagerFactory); - return retVal; - } - @Bean() public LocalContainerEntityManagerFactoryBean entityManagerFactory() { LocalContainerEntityManagerFactoryBean retVal = new LocalContainerEntityManagerFactoryBean(); @@ -102,4 +125,11 @@ public class TestDstu3Config extends BaseJavaConfigDstu3 { return requestValidator; } + @Bean() + public JpaTransactionManager transactionManager(EntityManagerFactory entityManagerFactory) { + JpaTransactionManager retVal = new JpaTransactionManager(); + retVal.setEntityManagerFactory(entityManagerFactory); + return retVal; + } + } From 42a81f4a8523b9c35c139caa717c17caa7508bca Mon Sep 17 00:00:00 2001 From: James Date: Mon, 24 Jul 2017 06:06:27 -0400 Subject: [PATCH 24/28] Fix some unit tests --- .../uhn/fhir/util/OperationOutcomeUtil.java | 28 +++--------- .../dao/FhirResourceDaoSubscriptionDstu2.java | 1 + .../FhirResourceDaoSearchParameterDstu3.java | 2 +- .../FhirResourceDaoSubscriptionDstu3.java | 2 + .../fhir/jpa/config/ConnectionWrapper.java | 2 +- .../uhn/fhir/jpa/config/TestDstu3Config.java | 45 ++++++++++++------- .../dstu3/ResourceProviderDstu3Test.java | 25 ++++------- 7 files changed, 48 insertions(+), 57 deletions(-) diff --git a/hapi-fhir-base/src/main/java/ca/uhn/fhir/util/OperationOutcomeUtil.java b/hapi-fhir-base/src/main/java/ca/uhn/fhir/util/OperationOutcomeUtil.java index 07238019a2f..c4a4b30f442 100644 --- a/hapi-fhir-base/src/main/java/ca/uhn/fhir/util/OperationOutcomeUtil.java +++ b/hapi-fhir-base/src/main/java/ca/uhn/fhir/util/OperationOutcomeUtil.java @@ -10,7 +10,7 @@ package ca.uhn.fhir.util; * 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 + * 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, @@ -41,24 +41,6 @@ import ca.uhn.fhir.rest.server.exceptions.InternalErrorException; */ public class OperationOutcomeUtil { -// /** -// * Add an issue to an OperationOutcome -// * -// * @param theCtx -// * The fhir context -// * @param theOperationOutcome -// * The OO resource to add to -// * @param theSeverity -// * The severity (e.g. "error") -// * @param theDetails -// * The details string -// * @param theCode -// */ -// public static void addIssue(FhirContext theCtx, IBaseOperationOutcome theOperationOutcome, String theSeverity, String theDetails, String theCode) { -// IBase issue = createIssue(theCtx, theOperationOutcome); -// populateDetails(theCtx, issue, theSeverity, theDetails, null, theCode); -// } - /** * Add an issue to an OperationOutcome * @@ -67,10 +49,10 @@ public class OperationOutcomeUtil { * @param theOperationOutcome * The OO resource to add to * @param theSeverity - * The severity (e.g. "error") + * The severity (fatal | error | warning | information) * @param theDetails * The details string - * @param theCode + * @param theCode */ public static void addIssue(FhirContext theCtx, IBaseOperationOutcome theOperationOutcome, String theSeverity, String theDetails, String theLocation, String theCode) { IBase issue = createIssue(theCtx, theOperationOutcome); @@ -150,7 +132,7 @@ public class OperationOutcomeUtil { BaseRuntimeChildDefinition detailsChild; if (theCtx.getVersion().getVersion().isNewerThan(FhirVersionEnum.DSTU1)) { detailsChild = issueElement.getChildByName("diagnostics"); - + BaseRuntimeChildDefinition codeChild = issueElement.getChildByName("code"); IPrimitiveType codeElem = (IPrimitiveType) codeChild.getChildByName("code").newInstance(codeChild.getInstanceConstructorArguments()); codeElem.setValueAsString(theCode); @@ -158,7 +140,7 @@ public class OperationOutcomeUtil { } else { detailsChild = issueElement.getChildByName("details"); } - + BaseRuntimeElementDefinition stringDef = detailsChild.getChildByName(detailsChild.getElementName()); BaseRuntimeChildDefinition severityChild = issueElement.getChildByName("severity"); BaseRuntimeChildDefinition locationChild = issueElement.getChildByName("location"); diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/FhirResourceDaoSubscriptionDstu2.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/FhirResourceDaoSubscriptionDstu2.java index 6d2be5c3aa5..af25de698ad 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/FhirResourceDaoSubscriptionDstu2.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/FhirResourceDaoSubscriptionDstu2.java @@ -116,6 +116,7 @@ public class FhirResourceDaoSubscriptionDstu2 extends FhirResourceDaoDstu2() { @Override public Integer doInTransaction(TransactionStatus theStatus) { diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/dstu3/FhirResourceDaoSubscriptionDstu3.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/dstu3/FhirResourceDaoSubscriptionDstu3.java index 18e859ecbd2..202fd47eb8f 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/dstu3/FhirResourceDaoSubscriptionDstu3.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/dstu3/FhirResourceDaoSubscriptionDstu3.java @@ -121,6 +121,8 @@ public class FhirResourceDaoSubscriptionDstu3 extends FhirResourceDaoDstu3 Date: Wed, 26 Jul 2017 15:25:19 -0400 Subject: [PATCH 25/28] Move tests to DSTU3 module --- .../src/test/java/ca/uhn/fhir/rest/param/DateRangeParamTest.java | 0 .../src/test/java/ca/uhn/fhir/rest/param/QuantityParamTest.java | 0 .../src/test/java/ca/uhn/fhir/rest/param/ReferenceParamTest.java | 0 .../test/java/ca/uhn/fhir/rest/param/TokenOrListParamTest.java | 0 4 files changed, 0 insertions(+), 0 deletions(-) rename {hapi-fhir-structures-dstu => hapi-fhir-structures-dstu3}/src/test/java/ca/uhn/fhir/rest/param/DateRangeParamTest.java (100%) rename {hapi-fhir-structures-dstu => hapi-fhir-structures-dstu3}/src/test/java/ca/uhn/fhir/rest/param/QuantityParamTest.java (100%) rename {hapi-fhir-structures-dstu => hapi-fhir-structures-dstu3}/src/test/java/ca/uhn/fhir/rest/param/ReferenceParamTest.java (100%) rename {hapi-fhir-structures-dstu => hapi-fhir-structures-dstu3}/src/test/java/ca/uhn/fhir/rest/param/TokenOrListParamTest.java (100%) diff --git a/hapi-fhir-structures-dstu/src/test/java/ca/uhn/fhir/rest/param/DateRangeParamTest.java b/hapi-fhir-structures-dstu3/src/test/java/ca/uhn/fhir/rest/param/DateRangeParamTest.java similarity index 100% rename from hapi-fhir-structures-dstu/src/test/java/ca/uhn/fhir/rest/param/DateRangeParamTest.java rename to hapi-fhir-structures-dstu3/src/test/java/ca/uhn/fhir/rest/param/DateRangeParamTest.java diff --git a/hapi-fhir-structures-dstu/src/test/java/ca/uhn/fhir/rest/param/QuantityParamTest.java b/hapi-fhir-structures-dstu3/src/test/java/ca/uhn/fhir/rest/param/QuantityParamTest.java similarity index 100% rename from hapi-fhir-structures-dstu/src/test/java/ca/uhn/fhir/rest/param/QuantityParamTest.java rename to hapi-fhir-structures-dstu3/src/test/java/ca/uhn/fhir/rest/param/QuantityParamTest.java diff --git a/hapi-fhir-structures-dstu/src/test/java/ca/uhn/fhir/rest/param/ReferenceParamTest.java b/hapi-fhir-structures-dstu3/src/test/java/ca/uhn/fhir/rest/param/ReferenceParamTest.java similarity index 100% rename from hapi-fhir-structures-dstu/src/test/java/ca/uhn/fhir/rest/param/ReferenceParamTest.java rename to hapi-fhir-structures-dstu3/src/test/java/ca/uhn/fhir/rest/param/ReferenceParamTest.java diff --git a/hapi-fhir-structures-dstu/src/test/java/ca/uhn/fhir/rest/param/TokenOrListParamTest.java b/hapi-fhir-structures-dstu3/src/test/java/ca/uhn/fhir/rest/param/TokenOrListParamTest.java similarity index 100% rename from hapi-fhir-structures-dstu/src/test/java/ca/uhn/fhir/rest/param/TokenOrListParamTest.java rename to hapi-fhir-structures-dstu3/src/test/java/ca/uhn/fhir/rest/param/TokenOrListParamTest.java From 6aa58cf887e9cfea22e64fd1353be85a8928ff22 Mon Sep 17 00:00:00 2001 From: James Agnew Date: Wed, 26 Jul 2017 15:30:24 -0400 Subject: [PATCH 26/28] Move tests in anticipation of HAPI 3 --- .../rest/param/TokenOrListParamDstu2Test.java | 35 +++++-------------- .../rest/param/TokenOrListParamDstu3Test.java | 34 ++++++++++++++++++ 2 files changed, 43 insertions(+), 26 deletions(-) rename hapi-fhir-structures-dstu3/src/test/java/ca/uhn/fhir/rest/param/TokenOrListParamTest.java => hapi-fhir-structures-dstu2/src/test/java/ca/uhn/fhir/rest/param/TokenOrListParamDstu2Test.java (67%) create mode 100644 hapi-fhir-structures-dstu3/src/test/java/ca/uhn/fhir/rest/param/TokenOrListParamDstu3Test.java diff --git a/hapi-fhir-structures-dstu3/src/test/java/ca/uhn/fhir/rest/param/TokenOrListParamTest.java b/hapi-fhir-structures-dstu2/src/test/java/ca/uhn/fhir/rest/param/TokenOrListParamDstu2Test.java similarity index 67% rename from hapi-fhir-structures-dstu3/src/test/java/ca/uhn/fhir/rest/param/TokenOrListParamTest.java rename to hapi-fhir-structures-dstu2/src/test/java/ca/uhn/fhir/rest/param/TokenOrListParamDstu2Test.java index b8a29bda234..988403e51ac 100644 --- a/hapi-fhir-structures-dstu3/src/test/java/ca/uhn/fhir/rest/param/TokenOrListParamTest.java +++ b/hapi-fhir-structures-dstu2/src/test/java/ca/uhn/fhir/rest/param/TokenOrListParamDstu2Test.java @@ -1,19 +1,18 @@ package ca.uhn.fhir.rest.param; -import static org.junit.Assert.*; - -import ca.uhn.fhir.context.FhirContext; -import ca.uhn.fhir.model.dstu.composite.CodingDt; -import ca.uhn.fhir.rest.method.QualifiedParamList; -import ca.uhn.fhir.util.TestUtil; - -import org.junit.AfterClass; -import org.junit.Test; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; import java.util.ArrayList; import java.util.List; -public class TokenOrListParamTest { +import org.junit.AfterClass; +import org.junit.Test; + +import ca.uhn.fhir.model.dstu2.composite.CodingDt; +import ca.uhn.fhir.util.TestUtil; + +public class TokenOrListParamDstu2Test { @Test public void testWhenParamListHasAnyMatchingCodingsForCodingList_doesCodingListMatch_shouldBeTrue() { TokenOrListParam params = new TokenOrListParam(); @@ -53,22 +52,6 @@ public class TokenOrListParamTest { assertFalse(params.doesCodingListMatch(codings)); } - /** - * See #192 - */ - @Test - public void testParseExcaped() { - TokenOrListParam params = new TokenOrListParam(); - params.setValuesAsQueryTokens(ourCtx, null, QualifiedParamList.singleton("system|code-include-but-not-end-with-comma\\,suffix")); - - assertEquals(1, params.getListAsCodings().size()); - assertEquals("system", params.getListAsCodings().get(0).getSystemElement().getValue()); - assertEquals("code-include-but-not-end-with-comma,suffix", params.getListAsCodings().get(0).getCodeElement().getValue()); - } - - - private static FhirContext ourCtx = FhirContext.forDstu1(); - @AfterClass public static void afterClassClearContext() { TestUtil.clearAllStaticFieldsForUnitTest(); diff --git a/hapi-fhir-structures-dstu3/src/test/java/ca/uhn/fhir/rest/param/TokenOrListParamDstu3Test.java b/hapi-fhir-structures-dstu3/src/test/java/ca/uhn/fhir/rest/param/TokenOrListParamDstu3Test.java new file mode 100644 index 00000000000..152a4270de3 --- /dev/null +++ b/hapi-fhir-structures-dstu3/src/test/java/ca/uhn/fhir/rest/param/TokenOrListParamDstu3Test.java @@ -0,0 +1,34 @@ +package ca.uhn.fhir.rest.param; + +import static org.junit.Assert.assertEquals; + +import org.junit.AfterClass; +import org.junit.Test; + +import ca.uhn.fhir.context.FhirContext; +import ca.uhn.fhir.rest.method.QualifiedParamList; +import ca.uhn.fhir.util.TestUtil; + +public class TokenOrListParamDstu3Test { + + private static FhirContext ourCtx = FhirContext.forDstu3(); + + /** + * See #192 + */ + @Test + public void testParseExcaped() { + TokenOrListParam params = new TokenOrListParam(); + params.setValuesAsQueryTokens(ourCtx, null, QualifiedParamList.singleton("system|code-include-but-not-end-with-comma\\,suffix")); + + assertEquals(1, params.getListAsCodings().size()); + assertEquals("system", params.getListAsCodings().get(0).getSystemElement().getValue()); + assertEquals("code-include-but-not-end-with-comma,suffix", params.getListAsCodings().get(0).getCodeElement().getValue()); + } + + @AfterClass + public static void afterClassClearContext() { + TestUtil.clearAllStaticFieldsForUnitTest(); + } + +} From 274e218494b2b06f7ebd04b4c9aa6bb8b8105157 Mon Sep 17 00:00:00 2001 From: James Agnew Date: Wed, 26 Jul 2017 16:59:25 -0400 Subject: [PATCH 27/28] Fix #696: Corresctly handle negative numbers --- .../fhir/rest/param/BaseParamWithPrefix.java | 13 +++- .../dstu3/ResourceProviderDstu3Test.java | 26 +++++++ .../uhn/fhir/rest/param/NumberParamTest.java | 76 +++++++++++++++++++ .../fhir/rest/param/QuantityParamTest.java | 76 +++++++++++++++++-- src/changes/changes.xml | 5 ++ 5 files changed, 187 insertions(+), 9 deletions(-) create mode 100644 hapi-fhir-structures-dstu3/src/test/java/ca/uhn/fhir/rest/param/NumberParamTest.java diff --git a/hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/param/BaseParamWithPrefix.java b/hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/param/BaseParamWithPrefix.java index 37a5c4b0ef1..e3745f39d17 100644 --- a/hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/param/BaseParamWithPrefix.java +++ b/hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/param/BaseParamWithPrefix.java @@ -10,7 +10,7 @@ package ca.uhn.fhir.rest.param; * 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 + * 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, @@ -44,8 +44,13 @@ public abstract class BaseParamWithPrefix extends BaseParam String extractPrefixAndReturnRest(String theString) { int offset = 0; while (true) { - if (theString.length() == offset || Character.isDigit(theString.charAt(offset))) { + if (theString.length() == offset) { break; + } else { + char nextChar = theString.charAt(offset); + if (nextChar == '-' || Character.isDigit(nextChar)) { + break; + } } offset++; } @@ -60,7 +65,7 @@ public abstract class BaseParamWithPrefix extends BaseParam } /** - * @deprecated Use {@link #getPrefix() instead} + * @deprecated Use {@link #getPrefix()} instead */ @Deprecated public QuantityCompararatorEnum getComparator() { @@ -68,7 +73,7 @@ public abstract class BaseParamWithPrefix extends BaseParam if (prefix == null) { return null; } - + return QuantityCompararatorEnum.forCode(prefix.getDstu1Value()); } 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 4ae19d1d315..022830a463a 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 @@ -11,6 +11,7 @@ import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; import java.io.*; +import java.math.BigDecimal; import java.net.*; import java.nio.charset.StandardCharsets; import java.util.*; @@ -3197,6 +3198,31 @@ public class ResourceProviderDstu3Test extends BaseResourceProviderDstu3Test { } } + @Test() + public void testSearchNegativeNumbers() throws Exception { + Observation o = new Observation(); + o.setValue(new Quantity().setValue(new BigDecimal("-10"))); + String oid1 = myObservationDao.create(o, mySrd).getId().toUnqualifiedVersionless().getValue(); + + Observation o2 = new Observation(); + o2.setValue(new Quantity().setValue(new BigDecimal("-20"))); + String oid2 = myObservationDao.create(o2, mySrd).getId().toUnqualifiedVersionless().getValue(); + + HttpGet get = new HttpGet(ourServerBase + "/Observation?value-quantity=gt-15"); + CloseableHttpResponse resp = ourHttpClient.execute(get); + try { + assertEquals(200, resp.getStatusLine().getStatusCode()); + Bundle bundle = myFhirCtx.newXmlParser().parseResource(Bundle.class, IOUtils.toString(resp.getEntity().getContent(), Constants.CHARSET_UTF8)); + + List ids = toUnqualifiedVersionlessIdValues(bundle); + assertThat(ids, contains(oid1)); + assertThat(ids, not(contains(oid2))); + } finally { + IOUtils.closeQuietly(resp); + } + + } + @Test(expected = InvalidRequestException.class) public void testSearchWithInvalidSort() throws Exception { Observation o = new Observation(); diff --git a/hapi-fhir-structures-dstu3/src/test/java/ca/uhn/fhir/rest/param/NumberParamTest.java b/hapi-fhir-structures-dstu3/src/test/java/ca/uhn/fhir/rest/param/NumberParamTest.java new file mode 100644 index 00000000000..d15fac757e0 --- /dev/null +++ b/hapi-fhir-structures-dstu3/src/test/java/ca/uhn/fhir/rest/param/NumberParamTest.java @@ -0,0 +1,76 @@ +package ca.uhn.fhir.rest.param; + +import static org.junit.Assert.assertEquals; + +import java.math.BigDecimal; + +import org.junit.AfterClass; +import org.junit.Test; + +import ca.uhn.fhir.context.FhirContext; +import ca.uhn.fhir.util.TestUtil; + +public class NumberParamTest { + private static FhirContext ourCtx = FhirContext.forDstu3(); + + @Test + public void testFull() { + NumberParam p = new NumberParam(); + p.setValueAsQueryToken(ourCtx, null, null, "<5.4"); + assertEquals(ParamPrefixEnum.LESSTHAN, p.getPrefix()); + assertEquals("5.4", p.getValue().toPlainString()); + assertEquals("lt5.4", p.getValueAsQueryToken(ourCtx)); + } + + @Test + public void testApproximateLegacy() { + NumberParam p = new NumberParam(); + p.setValueAsQueryToken(ourCtx, null, null, "~5.4"); + assertEquals(null,p.getComparator()); + assertEquals(ParamPrefixEnum.APPROXIMATE, p.getPrefix()); + assertEquals("5.4", p.getValue().toPlainString()); + assertEquals("ap5.4", p.getValueAsQueryToken(ourCtx)); + } + + @Test + public void testApproximate() { + NumberParam p = new NumberParam(); + p.setValueAsQueryToken(ourCtx, null, null, "ap5.4"); + assertEquals(null,p.getComparator()); + assertEquals(ParamPrefixEnum.APPROXIMATE, p.getPrefix()); + assertEquals("5.4", p.getValue().toPlainString()); + assertEquals("ap5.4", p.getValueAsQueryToken(ourCtx)); + } + + @Test + public void testNoQualifier() { + NumberParam p = new NumberParam(); + p.setValueAsQueryToken(ourCtx, null, null, "5.4"); + assertEquals(null, p.getComparator()); + assertEquals(null, p.getPrefix()); + assertEquals("5.4", p.getValue().toPlainString()); + assertEquals("5.4", p.getValueAsQueryToken(ourCtx)); + } + + + /** + * See #696 + */ + @Test + public void testNegativeNumber() { + NumberParam p = new NumberParam(); + p.setValueAsQueryToken(ourCtx, null, null, "-5.4"); + assertEquals(null, p.getComparator()); + assertEquals(null, p.getPrefix()); + assertEquals("-5.4", p.getValue().toPlainString()); + assertEquals(new BigDecimal("-5.4"), p.getValue()); + assertEquals("-5.4", p.getValueAsQueryToken(ourCtx)); + } + + + @AfterClass + public static void afterClassClearContext() { + TestUtil.clearAllStaticFieldsForUnitTest(); + } + +} diff --git a/hapi-fhir-structures-dstu3/src/test/java/ca/uhn/fhir/rest/param/QuantityParamTest.java b/hapi-fhir-structures-dstu3/src/test/java/ca/uhn/fhir/rest/param/QuantityParamTest.java index 0784dc19a0b..853560b722e 100644 --- a/hapi-fhir-structures-dstu3/src/test/java/ca/uhn/fhir/rest/param/QuantityParamTest.java +++ b/hapi-fhir-structures-dstu3/src/test/java/ca/uhn/fhir/rest/param/QuantityParamTest.java @@ -2,6 +2,8 @@ package ca.uhn.fhir.rest.param; import static org.junit.Assert.*; +import java.math.BigDecimal; + import org.junit.AfterClass; import org.junit.Test; @@ -10,37 +12,52 @@ import ca.uhn.fhir.model.dstu.valueset.QuantityCompararatorEnum; import ca.uhn.fhir.util.TestUtil; public class QuantityParamTest { - private static FhirContext ourCtx = FhirContext.forDstu1(); + private static FhirContext ourCtx = FhirContext.forDstu3(); @Test public void testFull() { QuantityParam p = new QuantityParam(); p.setValueAsQueryToken(ourCtx, null, null, "<5.4|http://unitsofmeasure.org|mg"); assertEquals(QuantityCompararatorEnum.LESSTHAN,p.getComparator()); + assertEquals(ParamPrefixEnum.LESSTHAN, p.getPrefix()); assertEquals("5.4", p.getValue().toPlainString()); assertEquals("http://unitsofmeasure.org", p.getSystem()); assertEquals("mg", p.getUnits()); - assertEquals("<5.4|http://unitsofmeasure.org|mg", p.getValueAsQueryToken(ourCtx)); + assertEquals("lt5.4|http://unitsofmeasure.org|mg", p.getValueAsQueryToken(ourCtx)); + } + + @Test + public void testApproximateLegacy() { + QuantityParam p = new QuantityParam(); + p.setValueAsQueryToken(ourCtx, null, null, "~5.4|http://unitsofmeasure.org|mg"); + assertEquals(null,p.getComparator()); + assertEquals(ParamPrefixEnum.APPROXIMATE, p.getPrefix()); + assertEquals(true, p.isApproximate()); + assertEquals("5.4", p.getValue().toPlainString()); + assertEquals("http://unitsofmeasure.org", p.getSystem()); + assertEquals("mg", p.getUnits()); + assertEquals("ap5.4|http://unitsofmeasure.org|mg", p.getValueAsQueryToken(ourCtx)); } @Test public void testApproximate() { QuantityParam p = new QuantityParam(); - p.setValueAsQueryToken(ourCtx, null, null, "~5.4|http://unitsofmeasure.org|mg"); + p.setValueAsQueryToken(ourCtx, null, null, "ap5.4|http://unitsofmeasure.org|mg"); assertEquals(null,p.getComparator()); + assertEquals(ParamPrefixEnum.APPROXIMATE, p.getPrefix()); assertEquals(true, p.isApproximate()); assertEquals("5.4", p.getValue().toPlainString()); assertEquals("http://unitsofmeasure.org", p.getSystem()); assertEquals("mg", p.getUnits()); - assertEquals("~5.4|http://unitsofmeasure.org|mg", p.getValueAsQueryToken(ourCtx)); + assertEquals("ap5.4|http://unitsofmeasure.org|mg", p.getValueAsQueryToken(ourCtx)); } - @Test public void testNoQualifier() { QuantityParam p = new QuantityParam(); p.setValueAsQueryToken(ourCtx, null, null, "5.4|http://unitsofmeasure.org|mg"); assertEquals(null, p.getComparator()); + assertEquals(null, p.getPrefix()); assertEquals("5.4", p.getValue().toPlainString()); assertEquals("http://unitsofmeasure.org", p.getSystem()); assertEquals("mg", p.getUnits()); @@ -53,6 +70,7 @@ public class QuantityParamTest { QuantityParam p = new QuantityParam(); p.setValueAsQueryToken(ourCtx, null, null, "5.4"); assertEquals(null, p.getComparator()); + assertEquals(null, p.getPrefix()); assertEquals("5.4", p.getValue().toPlainString()); assertEquals(null, p.getSystem()); assertEquals(null, p.getUnits()); @@ -84,6 +102,54 @@ public class QuantityParamTest { assertEquals("mg", param.getUnits()); } + /** + * See #696 + */ + @Test + public void testNegativeQuantityWithUnits() { + QuantityParam p = new QuantityParam(); + p.setValueAsQueryToken(ourCtx, null, null, "-5.4|http://unitsofmeasure.org|mg"); + assertEquals(null, p.getComparator()); + assertEquals(null, p.getPrefix()); + assertEquals("-5.4", p.getValue().toPlainString()); + assertEquals(new BigDecimal("-5.4"), p.getValue()); + assertEquals("http://unitsofmeasure.org", p.getSystem()); + assertEquals("mg", p.getUnits()); + assertEquals("-5.4|http://unitsofmeasure.org|mg", p.getValueAsQueryToken(ourCtx)); + } + + /** + * See #696 + */ + @Test + public void testNegativeQuantityWithoutUnits() { + QuantityParam p = new QuantityParam(); + p.setValueAsQueryToken(ourCtx, null, null, "-5.4"); + assertEquals(null, p.getComparator()); + assertEquals(null, p.getPrefix()); + assertEquals("-5.4", p.getValue().toPlainString()); + assertEquals(new BigDecimal("-5.4"), p.getValue()); + assertEquals(null, p.getSystem()); + assertEquals(null, p.getUnits()); + assertEquals("-5.4||", p.getValueAsQueryToken(ourCtx)); + } + + /** + * See #696 + */ + @Test + public void testNegativeQuantityWithoutUnitsWithComparator() { + QuantityParam p = new QuantityParam(); + p.setValueAsQueryToken(ourCtx, null, null, "gt-5.4"); + assertEquals(QuantityCompararatorEnum.GREATERTHAN, p.getComparator()); + assertEquals(ParamPrefixEnum.GREATERTHAN, p.getPrefix()); + assertEquals("-5.4", p.getValue().toPlainString()); + assertEquals(new BigDecimal("-5.4"), p.getValue()); + assertEquals(null, p.getSystem()); + assertEquals(null, p.getUnits()); + assertEquals("gt-5.4||", p.getValueAsQueryToken(ourCtx)); + } + @AfterClass public static void afterClassClearContext() { TestUtil.clearAllStaticFieldsForUnitTest(); diff --git a/src/changes/changes.xml b/src/changes/changes.xml index 87f2b827367..9a9520866c1 100644 --- a/src/changes/changes.xml +++ b/src/changes/changes.xml @@ -188,6 +188,11 @@ instead of the previous Bundle.entry.resource]]> + + An issue was corrected where search parameters containing negative numbers + were sometimes treated as positive numbers when processing the search. Thanks + to Keith Boone for reporting and suggesting a fix! + From e7aaca32d8697328129289b427a31a054b466a66 Mon Sep 17 00:00:00 2001 From: James Agnew Date: Wed, 26 Jul 2017 17:09:22 -0400 Subject: [PATCH 28/28] Credit for #693 --- src/changes/changes.xml | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/changes/changes.xml b/src/changes/changes.xml index 9a9520866c1..1422c16141e 100644 --- a/src/changes/changes.xml +++ b/src/changes/changes.xml @@ -58,10 +58,14 @@ generated. This should not affect most users. - FIx issue in SubscriptionInterceptor that caused interceptor to only + Fix issue in SubscriptionInterceptor that caused interceptor to only actually notify listeners of the first 10 subscriptions. Thanks to Jeff Chung for the pull request! + + Fix potential ConcurrentModificationException when adding subscriptions while + running under heavy load. Thanks to Jeff Chung for the pull request! + JPA search now uses hibernate ScrollableResults instead of plain JPA List. This should improve performance over large search results.