From 78b7188fbc4ed909bf9d0bfe81bc64840a484829 Mon Sep 17 00:00:00 2001 From: James Agnew Date: Wed, 11 Oct 2017 10:20:12 -0400 Subject: [PATCH] Fix validation error when Bundle has no type --- .../search/StaleSearchDeletingSvcImpl.java | 10 ++- ...quireManualActivationInterceptorDstu2.java | 2 +- .../dstu3/validation/InstanceValidator.java | 45 ++++-------- .../fhir/r4/validation/InstanceValidator.java | 10 ++- .../FhirInstanceValidatorR4Test.java | 17 ++++- .../FhirInstanceValidatorDstu3Test.java | 9 +++ .../resources/dstu3/bundle-with-no-type.json | 73 +++++++++++++++++++ .../resources/r4/bundle-with-no-type.json | 73 +++++++++++++++++++ src/changes/changes.xml | 4 + 9 files changed, 204 insertions(+), 39 deletions(-) rename hapi-fhir-validation/src/test/java/ca/uhn/fhir/{ => r4}/validation/FhirInstanceValidatorR4Test.java (97%) create mode 100644 hapi-fhir-validation/src/test/resources/dstu3/bundle-with-no-type.json create mode 100644 hapi-fhir-validation/src/test/resources/r4/bundle-with-no-type.json 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 9c5a2c56be7..5ea239ac22e 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 @@ -71,10 +71,12 @@ public class StaleSearchDeletingSvcImpl implements IStaleSearchDeletingSvc { 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); + if (searchToDelete != null) { + 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 diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/util/SubscriptionsRequireManualActivationInterceptorDstu2.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/util/SubscriptionsRequireManualActivationInterceptorDstu2.java index 61d128fd36e..f4ebde30368 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/util/SubscriptionsRequireManualActivationInterceptorDstu2.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/util/SubscriptionsRequireManualActivationInterceptorDstu2.java @@ -42,7 +42,7 @@ import static org.apache.commons.lang3.StringUtils.isNotBlank; public class SubscriptionsRequireManualActivationInterceptorDstu2 extends ServerOperationInterceptorAdapter { @Autowired - @Qualifier("mySubscriptionDaoR4") + @Qualifier("mySubscriptionDaoDstu2") private IFhirResourceDao myDao; @Override diff --git a/hapi-fhir-validation/src/main/java/org/hl7/fhir/dstu3/validation/InstanceValidator.java b/hapi-fhir-validation/src/main/java/org/hl7/fhir/dstu3/validation/InstanceValidator.java index 823c3e90827..b0b1826613e 100644 --- a/hapi-fhir-validation/src/main/java/org/hl7/fhir/dstu3/validation/InstanceValidator.java +++ b/hapi-fhir-validation/src/main/java/org/hl7/fhir/dstu3/validation/InstanceValidator.java @@ -13,8 +13,8 @@ import java.util.Map; import java.util.Set; import java.util.regex.Matcher; import java.util.regex.Pattern; - import org.apache.commons.lang3.StringUtils; + import org.hl7.fhir.dstu3.conformance.ProfileUtilities; import org.hl7.fhir.dstu3.context.IWorkerContext; import org.hl7.fhir.dstu3.context.IWorkerContext.ValidationResult; @@ -105,6 +105,7 @@ import org.w3c.dom.Node; import com.google.gson.Gson; import com.google.gson.JsonObject; +import com.google.gson.stream.JsonWriter; import ca.uhn.fhir.util.ObjectUtil; @@ -292,8 +293,6 @@ public class InstanceValidator extends BaseValidator implements IResourceValidat source = Source.InstanceValidator; } - - @Override public boolean isNoInvariantChecks() { return noInvariantChecks; @@ -1358,33 +1357,19 @@ public class InstanceValidator extends BaseValidator implements IResourceValidat checkFixedValue(errors, path + ".denominator", focus.getNamedChild("denominator"), fixed.getDenominator(), "denominator", focus); } - private Reference readAsReference(Element item) { - Reference r = new Reference(); - r.setDisplay(item.getNamedChildValue("display")); - r.setReference(item.getNamedChildValue("reference")); - List identifier = item.getChildrenByName("identifier"); - if (identifier.isEmpty() == false) { - r.setIdentifier(readAsIdentifier(identifier.get(0))); - } - return r; - } - private Identifier readAsIdentifier(Element item) { - Identifier r = new Identifier(); - r.setSystem(item.getNamedChildValue("system")); - r.setValue(item.getNamedChildValue("value")); - return r; - } - private void checkReference(Object appContext, List errors, String path, Element element, StructureDefinition profile, ElementDefinition container, String parentType, NodeStack stack) throws FHIRException, IOException { - Reference reference = readAsReference(element); - - String ref = reference.getReference(); - if (Utilities.noString(ref)) { - if (Utilities.noString(reference.getIdentifier().getSystem()) && Utilities.noString(reference.getIdentifier().getValue())) { - warning(errors, IssueType.STRUCTURE, element.line(), element.col(), path, !Utilities.noString(element.getNamedChildValue("display")), "A Reference without an actual reference or identifier should have a display"); - } - return; - } + String ref = null; + try { + // Do this inside a try because invalid instances might provide more than one reference. + ref = element.getNamedChildValue("reference"); + } catch (Error e) { + + } + if (Utilities.noString(ref)) { + // todo - what should we do in this case? + warning(errors, IssueType.STRUCTURE, element.line(), element.col(), path, !Utilities.noString(element.getNamedChildValue("display")), "A Reference without an actual reference should have a display"); + return; + } Element we = localResolve(ref, stack, errors, path); String refType; @@ -2816,6 +2801,8 @@ public class InstanceValidator extends BaseValidator implements IResourceValidat List entries = new ArrayList(); bundle.getNamedChildren("entry", entries); String type = bundle.getNamedChildValue("type"); + type = StringUtils.defaultString(type); + if (entries.size() == 0) { rule(errors, IssueType.INVALID, stack.getLiteralPath(), !(type.equals("document") || type.equals("message")), "Documents or Messages must contain at least one entry"); } else { diff --git a/hapi-fhir-validation/src/main/java/org/hl7/fhir/r4/validation/InstanceValidator.java b/hapi-fhir-validation/src/main/java/org/hl7/fhir/r4/validation/InstanceValidator.java index d5b84337ae9..854f42521ba 100644 --- a/hapi-fhir-validation/src/main/java/org/hl7/fhir/r4/validation/InstanceValidator.java +++ b/hapi-fhir-validation/src/main/java/org/hl7/fhir/r4/validation/InstanceValidator.java @@ -667,9 +667,9 @@ public class InstanceValidator extends BaseValidator implements IResourceValidat try { if (context.fetchResourceWithException(ValueSet.class, system) != null) { rule(errors, IssueType.CODEINVALID, element.line(), element.col(), path, false, "Invalid System URI: "+system+" - cannot use a value set URI as a system"); - return false; - } else - return true; + // Lloyd: This error used to prohibit checking for downstream issues, but there are some cases where that checking needs to occur. Please talk to me before changing the code back. + } + return true; } catch (Exception e) { return true; @@ -1445,7 +1445,7 @@ public class InstanceValidator extends BaseValidator implements IResourceValidat refType = "bundle"; } } - ReferenceValidationPolicy pol = refType.equals("contained") ? ReferenceValidationPolicy.CHECK_VALID : fetcher == null ? ReferenceValidationPolicy.IGNORE : fetcher.validationPolicy(hostContext.appContext, path, ref); + ReferenceValidationPolicy pol = refType.equals("contained") || refType.equals("bundle") ? ReferenceValidationPolicy.CHECK_VALID : fetcher == null ? ReferenceValidationPolicy.IGNORE : fetcher.validationPolicy(hostContext.appContext, path, ref); if (pol.checkExists()) { if (we == null) { @@ -2783,6 +2783,8 @@ public class InstanceValidator extends BaseValidator implements IResourceValidat List entries = new ArrayList(); bundle.getNamedChildren("entry", entries); String type = bundle.getNamedChildValue("type"); + type = StringUtils.defaultString(type); + if (entries.size() == 0) { rule(errors, IssueType.INVALID, stack.getLiteralPath(), !(type.equals("document") || type.equals("message")), "Documents or Messages must contain at least one entry"); } else { diff --git a/hapi-fhir-validation/src/test/java/ca/uhn/fhir/validation/FhirInstanceValidatorR4Test.java b/hapi-fhir-validation/src/test/java/ca/uhn/fhir/r4/validation/FhirInstanceValidatorR4Test.java similarity index 97% rename from hapi-fhir-validation/src/test/java/ca/uhn/fhir/validation/FhirInstanceValidatorR4Test.java rename to hapi-fhir-validation/src/test/java/ca/uhn/fhir/r4/validation/FhirInstanceValidatorR4Test.java index 1d4d1e28ae5..61688dd3d45 100644 --- a/hapi-fhir-validation/src/test/java/ca/uhn/fhir/validation/FhirInstanceValidatorR4Test.java +++ b/hapi-fhir-validation/src/test/java/ca/uhn/fhir/r4/validation/FhirInstanceValidatorR4Test.java @@ -1,4 +1,4 @@ -package ca.uhn.fhir.validation; +package ca.uhn.fhir.r4.validation; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.empty; @@ -17,7 +17,12 @@ import java.io.InputStream; import java.util.*; import java.util.zip.GZIPInputStream; +import ca.uhn.fhir.validation.FhirValidator; +import ca.uhn.fhir.validation.ResultSeverityEnum; +import ca.uhn.fhir.validation.SingleValidationMessage; +import ca.uhn.fhir.validation.ValidationResult; import org.apache.commons.io.IOUtils; +import org.hl7.fhir.dstu3.hapi.validation.FhirInstanceValidatorDstu3Test; import org.hl7.fhir.instance.model.api.IBaseResource; import org.hl7.fhir.r4.hapi.ctx.*; import org.hl7.fhir.r4.hapi.ctx.IValidationSupport.CodeValidationResult; @@ -533,6 +538,16 @@ public class FhirInstanceValidatorR4Test { assertEquals(output.toString(), 0, output.getMessages().size()); } + @Test + public void testValidateBundleWithNoType() throws Exception { + String vsContents = IOUtils.toString(FhirInstanceValidatorR4Test.class.getResourceAsStream("/r4/bundle-with-no-type.json"), "UTF-8"); + + ValidationResult output = myVal.validateWithResult(vsContents); + logResultsAndReturnNonInformationalOnes(output); + assertThat(output.getMessages().toString(), containsString("Element 'Bundle.type': minimum required = 1")); + } + + @Test public void testValidateRawXmlResourceWithEmptyPrimitive() { // @formatter:off diff --git a/hapi-fhir-validation/src/test/java/org/hl7/fhir/dstu3/hapi/validation/FhirInstanceValidatorDstu3Test.java b/hapi-fhir-validation/src/test/java/org/hl7/fhir/dstu3/hapi/validation/FhirInstanceValidatorDstu3Test.java index 32295577991..36e05359347 100644 --- a/hapi-fhir-validation/src/test/java/org/hl7/fhir/dstu3/hapi/validation/FhirInstanceValidatorDstu3Test.java +++ b/hapi-fhir-validation/src/test/java/org/hl7/fhir/dstu3/hapi/validation/FhirInstanceValidatorDstu3Test.java @@ -336,6 +336,15 @@ public class FhirInstanceValidatorDstu3Test { assertTrue(output.isSuccessful()); } + @Test + public void testValidateBundleWithNoType() throws Exception { + String vsContents = IOUtils.toString(FhirInstanceValidatorDstu3Test.class.getResourceAsStream("/dstu3/bundle-with-no-type.json"), "UTF-8"); + + ValidationResult output = myVal.validateWithResult(vsContents); + logResultsAndReturnNonInformationalOnes(output); + assertThat(output.getMessages().toString(), containsString("Element 'Bundle.type': minimum required = 1")); + } + /** * See #739 */ diff --git a/hapi-fhir-validation/src/test/resources/dstu3/bundle-with-no-type.json b/hapi-fhir-validation/src/test/resources/dstu3/bundle-with-no-type.json new file mode 100644 index 00000000000..64b109613f8 --- /dev/null +++ b/hapi-fhir-validation/src/test/resources/dstu3/bundle-with-no-type.json @@ -0,0 +1,73 @@ +{ + "resourceType": "Bundle", + "entry": [ + { + "fullUrl": "http://localhost:5555/phr/baseDstu3/Person/PERSON1", + "resource": { + "resourceType": "Person", + "id": "PERSON1", + "meta": { + "versionId": "1", + "lastUpdated": "2017-08-17T16:19:40.109+03:00" + }, + "identifier": [ + { + "value": "081181-9984" + } + ], + "name": [ + { + "text": "Anna Testi", + "family": "Testi", + "given": [ + "Anna" + ] + }, + { + "family": "asdfas" + } + ], + "telecom": [ + { + "use": "home" + }, + { + "system": "phone", + "value": "(044) 1234567", + "use": "work" + } + ], + "gender": "female", + "birthDate": "1981-11-08", + "address": [ + { + "line": [ + "Osuuspankkitie 2" + ], + "city": "Helsinki", + "postalCode": "00120", + "country": "Suomi" + }, + { + "city": "Blo-49847020" + } + ], + "active": true, + "link": [ + { + "target": { + "reference": "Patient/PATIENT1", + "display": "Anna Testi" + } + } + ] + }, + "request": { + "method": "PUT", + "url": "Person/PERSON1", + "ifMatch": "1" + } + } + ] +} + diff --git a/hapi-fhir-validation/src/test/resources/r4/bundle-with-no-type.json b/hapi-fhir-validation/src/test/resources/r4/bundle-with-no-type.json new file mode 100644 index 00000000000..64b109613f8 --- /dev/null +++ b/hapi-fhir-validation/src/test/resources/r4/bundle-with-no-type.json @@ -0,0 +1,73 @@ +{ + "resourceType": "Bundle", + "entry": [ + { + "fullUrl": "http://localhost:5555/phr/baseDstu3/Person/PERSON1", + "resource": { + "resourceType": "Person", + "id": "PERSON1", + "meta": { + "versionId": "1", + "lastUpdated": "2017-08-17T16:19:40.109+03:00" + }, + "identifier": [ + { + "value": "081181-9984" + } + ], + "name": [ + { + "text": "Anna Testi", + "family": "Testi", + "given": [ + "Anna" + ] + }, + { + "family": "asdfas" + } + ], + "telecom": [ + { + "use": "home" + }, + { + "system": "phone", + "value": "(044) 1234567", + "use": "work" + } + ], + "gender": "female", + "birthDate": "1981-11-08", + "address": [ + { + "line": [ + "Osuuspankkitie 2" + ], + "city": "Helsinki", + "postalCode": "00120", + "country": "Suomi" + }, + { + "city": "Blo-49847020" + } + ], + "active": true, + "link": [ + { + "target": { + "reference": "Patient/PATIENT1", + "display": "Anna Testi" + } + } + ] + }, + "request": { + "method": "PUT", + "url": "Person/PERSON1", + "ifMatch": "1" + } + } + ] +} + diff --git a/src/changes/changes.xml b/src/changes/changes.xml index aa82a165251..de18cd6cefa 100644 --- a/src/changes/changes.xml +++ b/src/changes/changes.xml @@ -55,6 +55,10 @@ the ID of the search, meaning that if a search returns results from the Query cache, it will reuse the ID of the previously returned Bundle + + Fix a NullPointerException when validating a Bundle (in DSTU3/R4) with no + Bundle.type]]> value +