From 3cbf669007fe902e5ffee75fe1e9e2f23ecea171 Mon Sep 17 00:00:00 2001 From: James Agnew Date: Sat, 3 Feb 2018 15:47:48 -0500 Subject: [PATCH] Don't crash on startup if an invalid subscription is in the database --- .../ca/uhn/fhir/util/SubscriptionUtil.java | 64 ++++++++ .../ca/uhn/fhir/jpa/dao/BaseHapiFhirDao.java | 21 ++- .../FhirResourceDaoSubscriptionDstu3.java | 14 ++ .../dao/r4/FhirResourceDaoSubscriptionR4.java | 21 ++- .../BaseSubscriptionInterceptor.java | 4 +- .../BaseSubscriptionSubscriber.java | 17 +- .../SubscriptionActivatingSubscriber.java | 26 ++- ...sourceDaoDstu3InvalidSubscriptionTest.java | 150 ++++++++++++++++++ ...rResourceDaoR4InvalidSubscriptionTest.java | 150 ++++++++++++++++++ .../ResponseHighlighterInterceptor.java | 8 +- .../ResponseHighlightingInterceptorTest.java | 144 +++++++++-------- src/changes/changes.xml | 10 ++ 12 files changed, 533 insertions(+), 96 deletions(-) create mode 100644 hapi-fhir-base/src/main/java/ca/uhn/fhir/util/SubscriptionUtil.java create mode 100644 hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/dstu3/FhirResourceDaoDstu3InvalidSubscriptionTest.java create mode 100644 hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4InvalidSubscriptionTest.java diff --git a/hapi-fhir-base/src/main/java/ca/uhn/fhir/util/SubscriptionUtil.java b/hapi-fhir-base/src/main/java/ca/uhn/fhir/util/SubscriptionUtil.java new file mode 100644 index 00000000000..a542c85e917 --- /dev/null +++ b/hapi-fhir-base/src/main/java/ca/uhn/fhir/util/SubscriptionUtil.java @@ -0,0 +1,64 @@ +package ca.uhn.fhir.util; + +import ca.uhn.fhir.context.BaseRuntimeChildDefinition; +import ca.uhn.fhir.context.BaseRuntimeElementDefinition; +import ca.uhn.fhir.context.FhirContext; +import ca.uhn.fhir.context.RuntimeResourceDefinition; +import org.hl7.fhir.instance.model.api.IBase; +import org.hl7.fhir.instance.model.api.IBaseResource; +import org.hl7.fhir.instance.model.api.IPrimitiveType; +import org.thymeleaf.util.Validate; + +import java.util.List; + +/* + * #%L + * HAPI FHIR - Core Library + * %% + * Copyright (C) 2014 - 2018 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% + */ + +/** + * Utilities for working with the subscription resource + */ +public class SubscriptionUtil { + + private static void populatePrimitiveValue(FhirContext theContext, IBaseResource theSubscription, String theChildName, String theValue) { + RuntimeResourceDefinition def = theContext.getResourceDefinition(theSubscription); + Validate.isTrue(def.getName().equals("Subscription"), "theResource is not a subscription"); + BaseRuntimeChildDefinition statusChild = def.getChildByName(theChildName); + List entries = statusChild.getAccessor().getValues(theSubscription); + IPrimitiveType instance; + if (entries.size() == 0) { + BaseRuntimeElementDefinition statusElement = statusChild.getChildByName(theChildName); + instance = (IPrimitiveType) statusElement.newInstance(statusChild.getInstanceConstructorArguments()); + statusChild.getMutator().addValue(theSubscription, instance); + } else { + instance = (IPrimitiveType) entries.get(0); + } + + instance.setValueAsString(theValue); + } + + public static void setReason(FhirContext theContext, IBaseResource theSubscription, String theMessage) { + populatePrimitiveValue(theContext, theSubscription, "reason", theMessage); + } + + public static void setStatus(FhirContext theContext, IBaseResource theSubscription, String theStatus) { + populatePrimitiveValue(theContext, theSubscription, "status", theStatus); + } + +} diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseHapiFhirDao.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseHapiFhirDao.java index 4255e1edb75..e970a5b3df6 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseHapiFhirDao.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseHapiFhirDao.java @@ -9,9 +9,9 @@ package ca.uhn.fhir.jpa.dao; * 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. @@ -50,6 +50,7 @@ import ca.uhn.fhir.rest.server.interceptor.IServerInterceptor; import ca.uhn.fhir.rest.server.interceptor.IServerInterceptor.ActionRequestDetails; import ca.uhn.fhir.rest.server.servlet.ServletRequestDetails; import ca.uhn.fhir.util.*; +import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Charsets; import com.google.common.collect.ArrayListMultimap; import com.google.common.collect.Sets; @@ -75,8 +76,6 @@ import javax.xml.stream.events.Characters; import javax.xml.stream.events.XMLEvent; import java.io.CharArrayWriter; import java.io.UnsupportedEncodingException; -import java.io.Writer; -import java.nio.CharBuffer; import java.text.Normalizer; import java.util.*; import java.util.Map.Entry; @@ -104,6 +103,7 @@ public abstract class BaseHapiFhirDao implements IDao { private static final org.slf4j.Logger ourLog = org.slf4j.LoggerFactory.getLogger(BaseHapiFhirDao.class); private static final Map ourRetrievalContexts = new HashMap(); private static final String PROCESSING_SUB_REQUEST = "BaseHapiFhirDao.processingSubRequest"; + private static boolean ourValidationDisabledForUnitTest; static { Map> resourceMetaParams = new HashMap>(); @@ -1213,6 +1213,7 @@ public abstract class BaseHapiFhirDao implements IDao { retVal.removeAll(theToRemove); return retVal; } + private void setUpdatedTime(Collection theParams, Date theUpdateTime) { for (BaseResourceIndexedSearchParam nextSearchParam : theParams) { nextSearchParam.setUpdated(theUpdateTime); @@ -1377,7 +1378,9 @@ public abstract class BaseHapiFhirDao implements IDao { */ if (theResource != null) { if (thePerformIndexing) { - validateResourceForStorage((T) theResource, theEntity); + if (!ourValidationDisabledForUnitTest) { + validateResourceForStorage((T) theResource, theEntity); + } } String resourceType = myContext.getResourceDefinition(theResource).getName(); if (isNotBlank(theEntity.getResourceType()) && !theEntity.getResourceType().equals(resourceType)) { @@ -2105,6 +2108,14 @@ public abstract class BaseHapiFhirDao implements IDao { return b.toString(); } + /** + * Do not call this method outside of unit tests + */ + @VisibleForTesting + public static void setValidationDisabledForUnitTest(boolean theValidationDisabledForUnitTest) { + ourValidationDisabledForUnitTest = theValidationDisabledForUnitTest; + } + private static List toBaseCodingList(List theSecurityLabels) { ArrayList retVal = new ArrayList(theSecurityLabels.size()); for (IBaseCoding next : theSecurityLabels) { 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 48ac336e755..40cb9a0e9d2 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 @@ -29,6 +29,7 @@ import ca.uhn.fhir.jpa.entity.SubscriptionTable; import ca.uhn.fhir.parser.DataFormatException; import ca.uhn.fhir.rest.api.EncodingEnum; import ca.uhn.fhir.rest.server.exceptions.UnprocessableEntityException; +import org.apache.commons.lang3.ObjectUtils; import org.hl7.fhir.dstu3.model.Subscription; import org.hl7.fhir.dstu3.model.Subscription.SubscriptionChannelType; import org.hl7.fhir.dstu3.model.Subscription.SubscriptionStatus; @@ -108,6 +109,16 @@ public class FhirResourceDaoSubscriptionDstu3 extends FhirResourceDaoDstu3 dao = getDao(resDef.getImplementingClass()); if (dao == null) { diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoSubscriptionR4.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoSubscriptionR4.java index fd5ae30ad36..5fe3c8d8eaf 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoSubscriptionR4.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoSubscriptionR4.java @@ -29,6 +29,7 @@ import ca.uhn.fhir.jpa.entity.SubscriptionTable; import ca.uhn.fhir.parser.DataFormatException; import ca.uhn.fhir.rest.api.EncodingEnum; import ca.uhn.fhir.rest.server.exceptions.UnprocessableEntityException; +import org.apache.commons.lang3.ObjectUtils; import org.hl7.fhir.instance.model.api.IBaseResource; import org.hl7.fhir.instance.model.api.IIdType; import org.hl7.fhir.r4.model.Subscription; @@ -37,20 +38,16 @@ import org.hl7.fhir.r4.model.Subscription.SubscriptionStatus; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.transaction.PlatformTransactionManager; +import javax.annotation.Nullable; import java.util.Date; import static org.apache.commons.lang3.StringUtils.isBlank; public class FhirResourceDaoSubscriptionR4 extends FhirResourceDaoR4 implements IFhirResourceDaoSubscription { - private static final org.slf4j.Logger ourLog = org.slf4j.LoggerFactory.getLogger(FhirResourceDaoSubscriptionR4.class); - @Autowired private ISubscriptionTableDao mySubscriptionTableDao; - @Autowired - private PlatformTransactionManager myTxManager; - private void createSubscriptionTable(ResourceTable theEntity, Subscription theSubscription) { SubscriptionTable subscriptionEntity = new SubscriptionTable(); subscriptionEntity.setCreated(new Date()); @@ -108,7 +105,18 @@ public class FhirResourceDaoSubscriptionR4 extends FhirResourceDaoR4 dao = getDao(resDef.getImplementingClass()); if (dao == null) { diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/subscription/BaseSubscriptionInterceptor.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/subscription/BaseSubscriptionInterceptor.java index 4c7e4baca83..e80b6e39d24 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/subscription/BaseSubscriptionInterceptor.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/subscription/BaseSubscriptionInterceptor.java @@ -9,9 +9,9 @@ package ca.uhn.fhir.jpa.subscription; * 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. diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/subscription/BaseSubscriptionSubscriber.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/subscription/BaseSubscriptionSubscriber.java index 27c7ffc0fa9..a5c9f5c2b34 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/subscription/BaseSubscriptionSubscriber.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/subscription/BaseSubscriptionSubscriber.java @@ -26,7 +26,6 @@ import org.hl7.fhir.instance.model.api.IBaseResource; import org.hl7.fhir.instance.model.api.IPrimitiveType; import org.hl7.fhir.r4.model.Subscription; import org.springframework.messaging.MessageHandler; -import org.springframework.messaging.MessagingException; public abstract class BaseSubscriptionSubscriber implements MessageHandler { @@ -60,14 +59,6 @@ public abstract class BaseSubscriptionSubscriber implements MessageHandler { } - /** - * Does this subscription type (e.g. rest hook, websocket, etc) apply to this interceptor? - */ - protected boolean subscriptionTypeApplies(IBaseResource theSubscription) { - FhirContext ctx = mySubscriptionDao.getContext(); - return subscriptionTypeApplies(ctx, theSubscription); - } - /** * Does this subscription type (e.g. rest hook, websocket, etc) apply to this interceptor? */ @@ -80,10 +71,12 @@ public abstract class BaseSubscriptionSubscriber implements MessageHandler { * Does this subscription type (e.g. rest hook, websocket, etc) apply to this interceptor? */ static boolean subscriptionTypeApplies(FhirContext theCtx, IBaseResource theSubscription, Subscription.SubscriptionChannelType theChannelType) { - IPrimitiveType status = theCtx.newTerser().getSingleValueOrNull(theSubscription, BaseSubscriptionInterceptor.SUBSCRIPTION_TYPE, IPrimitiveType.class); + IPrimitiveType subscriptionType = theCtx.newTerser().getSingleValueOrNull(theSubscription, BaseSubscriptionInterceptor.SUBSCRIPTION_TYPE, IPrimitiveType.class); boolean subscriptionTypeApplies = false; - if (theChannelType.toCode().equals(status.getValueAsString())) { - subscriptionTypeApplies = true; + if (subscriptionType != null) { + if (theChannelType.toCode().equals(subscriptionType.getValueAsString())) { + subscriptionTypeApplies = true; + } } return subscriptionTypeApplies; } diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/subscription/SubscriptionActivatingSubscriber.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/subscription/SubscriptionActivatingSubscriber.java index 972883619a1..66fb005fe0a 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/subscription/SubscriptionActivatingSubscriber.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/subscription/SubscriptionActivatingSubscriber.java @@ -23,6 +23,9 @@ package ca.uhn.fhir.jpa.subscription; import ca.uhn.fhir.context.FhirContext; import ca.uhn.fhir.jpa.dao.IFhirResourceDao; import ca.uhn.fhir.rest.api.RestOperationTypeEnum; +import ca.uhn.fhir.rest.server.exceptions.UnprocessableEntityException; +import ca.uhn.fhir.util.SubscriptionUtil; +import org.apache.commons.lang3.time.DateUtils; import org.hl7.fhir.instance.model.api.IBaseResource; import org.hl7.fhir.instance.model.api.IIdType; import org.hl7.fhir.instance.model.api.IPrimitiveType; @@ -30,12 +33,14 @@ import org.hl7.fhir.r4.model.Subscription; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.springframework.messaging.MessagingException; +import org.springframework.scheduling.TaskScheduler; import org.springframework.transaction.PlatformTransactionManager; import org.springframework.transaction.TransactionStatus; -import org.springframework.transaction.support.TransactionCallbackWithoutResult; -import org.springframework.transaction.support.TransactionSynchronizationAdapter; -import org.springframework.transaction.support.TransactionSynchronizationManager; -import org.springframework.transaction.support.TransactionTemplate; +import org.springframework.transaction.support.*; + +import java.util.Date; +import java.util.concurrent.Callable; +import java.util.concurrent.ExecutorService; @SuppressWarnings("unchecked") public class SubscriptionActivatingSubscriber { @@ -92,11 +97,18 @@ public class SubscriptionActivatingSubscriber { } } - private void activateSubscription(IPrimitiveType theStatus, String theActiveStatus, IBaseResource theSubscription, String theRequestedStatus) { + private void activateSubscription(IPrimitiveType theStatus, String theActiveStatus, final IBaseResource theSubscription, String theRequestedStatus) { theStatus.setValueAsString(theActiveStatus); ourLog.info("Activating and registering subscription {} from status {} to {}", theSubscription.getIdElement().toUnqualified().getValue(), theRequestedStatus, theActiveStatus); - mySubscriptionDao.update(theSubscription); - mySubscriptionInterceptor.registerSubscription(theSubscription.getIdElement(), theSubscription); + try { + mySubscriptionDao.update(theSubscription); + } catch (final UnprocessableEntityException e) { + ourLog.info("Changing status of {} to ERROR", theSubscription.getIdElement()); + IBaseResource subscription = mySubscriptionDao.read(theSubscription.getIdElement()); + SubscriptionUtil.setStatus(myCtx, subscription, "error"); + SubscriptionUtil.setReason(myCtx, subscription, e.getMessage()); + mySubscriptionDao.update(subscription); + } } diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/dstu3/FhirResourceDaoDstu3InvalidSubscriptionTest.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/dstu3/FhirResourceDaoDstu3InvalidSubscriptionTest.java new file mode 100644 index 00000000000..ec3bd9dc58a --- /dev/null +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/dstu3/FhirResourceDaoDstu3InvalidSubscriptionTest.java @@ -0,0 +1,150 @@ +package ca.uhn.fhir.jpa.dao.dstu3; + +import ca.uhn.fhir.jpa.dao.BaseHapiFhirDao; +import ca.uhn.fhir.jpa.dao.DaoConfig; +import ca.uhn.fhir.jpa.subscription.resthook.SubscriptionRestHookInterceptor; +import ca.uhn.fhir.rest.server.exceptions.UnprocessableEntityException; +import ca.uhn.fhir.util.TestUtil; +import org.hl7.fhir.dstu3.model.Subscription; +import org.hl7.fhir.instance.model.api.IIdType; +import org.junit.After; +import org.junit.AfterClass; +import org.junit.Test; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.transaction.TransactionStatus; +import org.springframework.transaction.support.TransactionCallbackWithoutResult; +import org.springframework.transaction.support.TransactionTemplate; + +import javax.persistence.Query; + +import static org.junit.Assert.*; + +public class FhirResourceDaoDstu3InvalidSubscriptionTest extends BaseJpaDstu3Test { + + @Autowired + private SubscriptionRestHookInterceptor myInterceptor; + + @After + public void afterResetDao() { + myDaoConfig.setResourceServerIdStrategy(new DaoConfig().getResourceServerIdStrategy()); + BaseHapiFhirDao.setValidationDisabledForUnitTest(false); + } + + @Test + public void testCreateInvalidSubscriptionOkButCanNotActivate() { + Subscription s = new Subscription(); + s.setStatus(Subscription.SubscriptionStatus.OFF); + s.setCriteria("FOO"); + IIdType id = mySubscriptionDao.create(s).getId().toUnqualified(); + + s = mySubscriptionDao.read(id); + assertEquals("FOO", s.getCriteria()); + + s.setStatus(Subscription.SubscriptionStatus.REQUESTED); + try { + mySubscriptionDao.update(s); + fail(); + } catch (UnprocessableEntityException e) { + assertEquals("Subscription.criteria must be in the form \"{Resource Type}?[params]", e.getMessage()); + } + } + + /** + * Make sure that bad data in the database doesn't prevent startup + */ + @Test + public void testSubscriptionMarkedDeleted() { + BaseHapiFhirDao.setValidationDisabledForUnitTest(true); + + Subscription s = new Subscription(); + s.setStatus(Subscription.SubscriptionStatus.REQUESTED); + s.getChannel().setEndpoint("http://foo"); + s.getChannel().setPayload("application/fhir+json"); + s.setCriteria("Patient?foo"); + final IIdType id = mySubscriptionDao.create(s).getId().toUnqualifiedVersionless(); + assertNotNull(id.getIdPart()); + + BaseHapiFhirDao.setValidationDisabledForUnitTest(false); + + new TransactionTemplate(myTransactionMgr).execute(new TransactionCallbackWithoutResult() { + @Override + protected void doInTransactionWithoutResult(TransactionStatus status) { + Query q = myEntityManager.createNativeQuery("UPDATE HFJ_RESOURCE SET RES_DELETED_AT = RES_UPDATED WHERE RES_ID = " + id.getIdPart()); + q.executeUpdate(); + } + }); + + myEntityManager.clear(); + + myInterceptor.start(); + } + + /** + * Make sure that bad data in the database doesn't prevent startup + */ + @Test + public void testSubscriptionWithInvalidCriteria() { + BaseHapiFhirDao.setValidationDisabledForUnitTest(true); + + Subscription s = new Subscription(); + s.setStatus(Subscription.SubscriptionStatus.REQUESTED); + s.getChannel().setType(Subscription.SubscriptionChannelType.RESTHOOK); + s.getChannel().setEndpoint("http://foo"); + s.getChannel().setPayload("application/fhir+json"); + s.setCriteria("BLAH"); + IIdType id = mySubscriptionDao.create(s).getId().toUnqualifiedVersionless(); + assertNotNull(id.getIdPart()); + + BaseHapiFhirDao.setValidationDisabledForUnitTest(false); + + myInterceptor.start(); + + } + + /** + * Make sure that bad data in the database doesn't prevent startup + */ + @Test + public void testSubscriptionWithNoStatus() { + BaseHapiFhirDao.setValidationDisabledForUnitTest(true); + + Subscription s = new Subscription(); + s.getChannel().setType(Subscription.SubscriptionChannelType.RESTHOOK); + s.getChannel().setEndpoint("http://foo"); + s.getChannel().setPayload("application/fhir+json"); + s.setCriteria("Patient?active=true"); + IIdType id = mySubscriptionDao.create(s).getId().toUnqualifiedVersionless(); + + BaseHapiFhirDao.setValidationDisabledForUnitTest(false); + + myInterceptor.start(); + + } + + /** + * Make sure that bad data in the database doesn't prevent startup + */ + @Test + public void testSubscriptionWithNoType() { + BaseHapiFhirDao.setValidationDisabledForUnitTest(true); + + Subscription s = new Subscription(); + s.setStatus(Subscription.SubscriptionStatus.REQUESTED); + s.getChannel().setEndpoint("http://foo"); + s.getChannel().setPayload("application/fhir+json"); + s.setCriteria("Patient?foo"); + IIdType id = mySubscriptionDao.create(s).getId().toUnqualifiedVersionless(); + assertNotNull(id.getIdPart()); + + BaseHapiFhirDao.setValidationDisabledForUnitTest(false); + + myInterceptor.start(); + + } + + @AfterClass + public static void afterClassClearContext() { + TestUtil.clearAllStaticFieldsForUnitTest(); + } + +} diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4InvalidSubscriptionTest.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4InvalidSubscriptionTest.java new file mode 100644 index 00000000000..5446252d716 --- /dev/null +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4InvalidSubscriptionTest.java @@ -0,0 +1,150 @@ +package ca.uhn.fhir.jpa.dao.r4; + +import ca.uhn.fhir.jpa.dao.BaseHapiFhirDao; +import ca.uhn.fhir.jpa.dao.DaoConfig; +import ca.uhn.fhir.jpa.subscription.resthook.SubscriptionRestHookInterceptor; +import ca.uhn.fhir.rest.server.exceptions.UnprocessableEntityException; +import ca.uhn.fhir.util.TestUtil; +import org.hl7.fhir.instance.model.api.IIdType; +import org.hl7.fhir.r4.model.Subscription; +import org.junit.After; +import org.junit.AfterClass; +import org.junit.Test; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.transaction.TransactionStatus; +import org.springframework.transaction.support.TransactionCallbackWithoutResult; +import org.springframework.transaction.support.TransactionTemplate; + +import javax.persistence.Query; + +import static org.junit.Assert.*; + +public class FhirResourceDaoR4InvalidSubscriptionTest extends BaseJpaR4Test { + + @Autowired + private SubscriptionRestHookInterceptor myInterceptor; + + @After + public void afterResetDao() { + myDaoConfig.setResourceServerIdStrategy(new DaoConfig().getResourceServerIdStrategy()); + BaseHapiFhirDao.setValidationDisabledForUnitTest(false); + } + + @Test + public void testCreateInvalidSubscriptionOkButCanNotActivate() { + Subscription s = new Subscription(); + s.setStatus(Subscription.SubscriptionStatus.OFF); + s.setCriteria("FOO"); + IIdType id = mySubscriptionDao.create(s).getId().toUnqualified(); + + s = mySubscriptionDao.read(id); + assertEquals("FOO", s.getCriteria()); + + s.setStatus(Subscription.SubscriptionStatus.REQUESTED); + try { + mySubscriptionDao.update(s); + fail(); + } catch (UnprocessableEntityException e) { + assertEquals("Subscription.criteria must be in the form \"{Resource Type}?[params]", e.getMessage()); + } + } + + /** + * Make sure that bad data in the database doesn't prevent startup + */ + @Test + public void testSubscriptionMarkedDeleted() { + BaseHapiFhirDao.setValidationDisabledForUnitTest(true); + + Subscription s = new Subscription(); + s.setStatus(Subscription.SubscriptionStatus.REQUESTED); + s.getChannel().setEndpoint("http://foo"); + s.getChannel().setPayload("application/fhir+json"); + s.setCriteria("Patient?foo"); + final IIdType id = mySubscriptionDao.create(s).getId().toUnqualifiedVersionless(); + assertNotNull(id.getIdPart()); + + BaseHapiFhirDao.setValidationDisabledForUnitTest(false); + + new TransactionTemplate(myTransactionMgr).execute(new TransactionCallbackWithoutResult() { + @Override + protected void doInTransactionWithoutResult(TransactionStatus status) { + Query q = myEntityManager.createNativeQuery("UPDATE HFJ_RESOURCE SET RES_DELETED_AT = RES_UPDATED WHERE RES_ID = " + id.getIdPart()); + q.executeUpdate(); + } + }); + + myEntityManager.clear(); + + myInterceptor.start(); + } + + /** + * Make sure that bad data in the database doesn't prevent startup + */ + @Test + public void testSubscriptionWithInvalidCriteria() throws InterruptedException { + BaseHapiFhirDao.setValidationDisabledForUnitTest(true); + + Subscription s = new Subscription(); + s.setStatus(Subscription.SubscriptionStatus.REQUESTED); + s.getChannel().setType(Subscription.SubscriptionChannelType.RESTHOOK); + s.getChannel().setEndpoint("http://foo"); + s.getChannel().setPayload("application/fhir+json"); + s.setCriteria("BLAH"); + IIdType id = mySubscriptionDao.create(s).getId().toUnqualifiedVersionless(); + assertNotNull(id.getIdPart()); + + BaseHapiFhirDao.setValidationDisabledForUnitTest(false); + + myInterceptor.start(); + + } + + /** + * Make sure that bad data in the database doesn't prevent startup + */ + @Test + public void testSubscriptionWithNoStatus() { + BaseHapiFhirDao.setValidationDisabledForUnitTest(true); + + Subscription s = new Subscription(); + s.getChannel().setType(Subscription.SubscriptionChannelType.RESTHOOK); + s.getChannel().setEndpoint("http://foo"); + s.getChannel().setPayload("application/fhir+json"); + s.setCriteria("Patient?active=true"); + IIdType id = mySubscriptionDao.create(s).getId().toUnqualifiedVersionless(); + + BaseHapiFhirDao.setValidationDisabledForUnitTest(false); + + myInterceptor.start(); + + } + + /** + * Make sure that bad data in the database doesn't prevent startup + */ + @Test + public void testSubscriptionWithNoType() { + BaseHapiFhirDao.setValidationDisabledForUnitTest(true); + + Subscription s = new Subscription(); + s.setStatus(Subscription.SubscriptionStatus.REQUESTED); + s.getChannel().setEndpoint("http://foo"); + s.getChannel().setPayload("application/fhir+json"); + s.setCriteria("Patient?foo"); + IIdType id = mySubscriptionDao.create(s).getId().toUnqualifiedVersionless(); + assertNotNull(id.getIdPart()); + + BaseHapiFhirDao.setValidationDisabledForUnitTest(false); + + myInterceptor.start(); + + } + + @AfterClass + public static void afterClassClearContext() { + TestUtil.clearAllStaticFieldsForUnitTest(); + } + +} diff --git a/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/interceptor/ResponseHighlighterInterceptor.java b/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/interceptor/ResponseHighlighterInterceptor.java index 30e49acce57..3045580b603 100644 --- a/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/interceptor/ResponseHighlighterInterceptor.java +++ b/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/interceptor/ResponseHighlighterInterceptor.java @@ -318,7 +318,13 @@ public class ResponseHighlighterInterceptor extends InterceptorAdapter { boolean force = false; String[] formatParams = theRequestDetails.getParameters().get(Constants.PARAM_FORMAT); if (formatParams != null && formatParams.length > 0) { - String formatParam = formatParams[0]; + String formatParam = defaultString(formatParams[0]); + int semiColonIdx = formatParam.indexOf(';'); + if (semiColonIdx != -1) { + formatParam = formatParam.substring(0, semiColonIdx); + } + formatParam = trim(formatParam); + if (Constants.FORMATS_HTML.contains(formatParam)) { // this is a set force = true; } else if (Constants.FORMATS_HTML_XML.equals(formatParam)) { diff --git a/hapi-fhir-structures-dstu2/src/test/java/ca/uhn/fhir/rest/server/interceptor/ResponseHighlightingInterceptorTest.java b/hapi-fhir-structures-dstu2/src/test/java/ca/uhn/fhir/rest/server/interceptor/ResponseHighlightingInterceptorTest.java index f3192703315..2bea5344959 100644 --- a/hapi-fhir-structures-dstu2/src/test/java/ca/uhn/fhir/rest/server/interceptor/ResponseHighlightingInterceptorTest.java +++ b/hapi-fhir-structures-dstu2/src/test/java/ca/uhn/fhir/rest/server/interceptor/ResponseHighlightingInterceptorTest.java @@ -1,29 +1,34 @@ package ca.uhn.fhir.rest.server.interceptor; -import static org.hamcrest.Matchers.containsString; -import static org.hamcrest.Matchers.containsStringIgnoringCase; -import static org.hamcrest.Matchers.matchesPattern; -import static org.hamcrest.Matchers.not; -import static org.hamcrest.Matchers.stringContainsInOrder; -import static org.junit.Assert.assertArrayEquals; -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertNull; -import static org.junit.Assert.assertThat; -import static org.junit.Assert.assertTrue; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.when; - -import java.io.PrintWriter; -import java.io.StringWriter; -import java.nio.charset.StandardCharsets; -import java.util.*; -import java.util.concurrent.TimeUnit; - -import javax.servlet.http.HttpServletRequest; -import javax.servlet.http.HttpServletResponse; - +import ca.uhn.fhir.context.FhirContext; +import ca.uhn.fhir.context.api.BundleInclusionRule; +import ca.uhn.fhir.model.api.IResource; +import ca.uhn.fhir.model.dstu2.composite.HumanNameDt; +import ca.uhn.fhir.model.dstu2.composite.IdentifierDt; +import ca.uhn.fhir.model.dstu2.resource.Binary; +import ca.uhn.fhir.model.dstu2.resource.OperationOutcome; +import ca.uhn.fhir.model.dstu2.resource.OperationOutcome.Issue; +import ca.uhn.fhir.model.dstu2.resource.Organization; +import ca.uhn.fhir.model.dstu2.resource.Patient; +import ca.uhn.fhir.model.dstu2.valueset.IdentifierUseEnum; +import ca.uhn.fhir.model.primitive.IdDt; +import ca.uhn.fhir.model.primitive.UriDt; +import ca.uhn.fhir.rest.annotation.IdParam; +import ca.uhn.fhir.rest.annotation.Read; +import ca.uhn.fhir.rest.annotation.RequiredParam; +import ca.uhn.fhir.rest.annotation.Search; +import ca.uhn.fhir.rest.api.Constants; +import ca.uhn.fhir.rest.api.EncodingEnum; +import ca.uhn.fhir.rest.api.RequestTypeEnum; import ca.uhn.fhir.rest.api.server.ResponseDetails; +import ca.uhn.fhir.rest.server.IResourceProvider; +import ca.uhn.fhir.rest.server.RestfulServer; +import ca.uhn.fhir.rest.server.exceptions.ResourceNotFoundException; +import ca.uhn.fhir.rest.server.servlet.ServletRequestDetails; +import ca.uhn.fhir.util.PortUtil; +import ca.uhn.fhir.util.TestUtil; +import ca.uhn.fhir.util.UrlUtil; +import com.phloc.commons.collections.iterate.ArrayEnumeration; import org.apache.commons.io.IOUtils; import org.apache.http.HttpResponse; import org.apache.http.client.methods.CloseableHttpResponse; @@ -34,30 +39,26 @@ import org.apache.http.impl.conn.PoolingHttpClientConnectionManager; import org.eclipse.jetty.server.Server; import org.eclipse.jetty.servlet.ServletHandler; import org.eclipse.jetty.servlet.ServletHolder; -import org.junit.*; +import org.junit.AfterClass; +import org.junit.Before; +import org.junit.BeforeClass; import org.junit.Test; import org.mockito.invocation.InvocationOnMock; import org.mockito.stubbing.Answer; import org.springframework.web.cors.CorsConfiguration; -import com.phloc.commons.collections.iterate.ArrayEnumeration; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; +import java.io.PrintWriter; +import java.io.StringWriter; +import java.nio.charset.StandardCharsets; +import java.util.*; +import java.util.concurrent.TimeUnit; -import ca.uhn.fhir.context.FhirContext; -import ca.uhn.fhir.context.api.BundleInclusionRule; -import ca.uhn.fhir.model.api.IResource; -import ca.uhn.fhir.model.dstu2.composite.HumanNameDt; -import ca.uhn.fhir.model.dstu2.composite.IdentifierDt; -import ca.uhn.fhir.model.dstu2.resource.*; -import ca.uhn.fhir.model.dstu2.resource.OperationOutcome.Issue; -import ca.uhn.fhir.model.dstu2.valueset.IdentifierUseEnum; -import ca.uhn.fhir.model.primitive.IdDt; -import ca.uhn.fhir.model.primitive.UriDt; -import ca.uhn.fhir.rest.annotation.*; -import ca.uhn.fhir.rest.api.*; -import ca.uhn.fhir.rest.server.*; -import ca.uhn.fhir.rest.server.exceptions.ResourceNotFoundException; -import ca.uhn.fhir.rest.server.servlet.ServletRequestDetails; -import ca.uhn.fhir.util.*; +import static org.hamcrest.Matchers.*; +import static org.junit.Assert.*; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; public class ResponseHighlightingInterceptorTest { @@ -88,7 +89,7 @@ public class ResponseHighlightingInterceptorTest { assertEquals(200, status.getStatusLine().getStatusCode()); assertEquals("foo", status.getFirstHeader("content-type").getValue()); assertEquals("Attachment;", status.getFirstHeader("Content-Disposition").getValue()); - assertArrayEquals(new byte[] { 1, 2, 3, 4 }, responseContent); + assertArrayEquals(new byte[]{1, 2, 3, 4}, responseContent); } @Test @@ -117,7 +118,7 @@ public class ResponseHighlightingInterceptorTest { assertEquals(200, status.getStatusLine().getStatusCode()); assertEquals("foo", status.getFirstHeader("content-type").getValue()); assertEquals("Attachment;", status.getFirstHeader("Content-Disposition").getValue()); - assertArrayEquals(new byte[] { 1, 2, 3, 4 }, responseContent); + assertArrayEquals(new byte[]{1, 2, 3, 4}, responseContent); } @@ -253,6 +254,23 @@ public class ResponseHighlightingInterceptorTest { ourLog.info(responseContent); } + @Test + public void testForceHtmlJsonWithAdditionalParts() throws Exception { + HttpGet httpGet = new HttpGet("http://localhost:" + ourPort + "/Patient/1?_format=" + UrlUtil.escapeUrlParam("html/json; fhirVersion=1.0")); + httpGet.addHeader("User-Agent", "Mozilla/5.0 (Windows NT 6.1; WOW64; rv:40.0) Gecko/20100101 Firefox/40.1"); + + HttpResponse status = ourClient.execute(httpGet); + String responseContent = IOUtils.toString(status.getEntity().getContent(), StandardCharsets.UTF_8); + IOUtils.closeQuietly(status.getEntity().getContent()); + assertEquals(200, status.getStatusLine().getStatusCode()); + assertEquals("text/html;charset=utf-8", status.getFirstHeader("content-type").getValue().replace(" ", "").toLowerCase()); + assertThat(responseContent, containsString("html")); + assertThat(responseContent, containsString(">{<")); + assertThat(responseContent, not(containsString("<"))); + + ourLog.info(responseContent); + } + @Test public void testForceHtmlXml() throws Exception { HttpGet httpGet = new HttpGet("http://localhost:" + ourPort + "/Patient/1?_format=html/xml"); @@ -309,7 +327,7 @@ public class ResponseHighlightingInterceptorTest { assertThat(responseContent, stringContainsInOrder("OperationOutcome", "Unknown resource type 'Foobar' - Server knows how to handle")); } - + @Test public void testGetInvalidResourceNoAcceptHeader() throws Exception { HttpGet httpGet = new HttpGet("http://localhost:" + ourPort + "/Foobar/123"); @@ -403,7 +421,7 @@ public class ResponseHighlightingInterceptorTest { ServletRequestDetails reqDetails = new TestServletRequestDetails(); reqDetails.setRequestType(RequestTypeEnum.GET); HashMap params = new HashMap(); - params.put(Constants.PARAM_FORMAT, new String[] { Constants.FORMAT_HTML }); + params.put(Constants.PARAM_FORMAT, new String[]{Constants.FORMAT_HTML}); reqDetails.setParameters(params); reqDetails.setServer(new RestfulServer(ourCtx)); reqDetails.setServletRequest(req); @@ -437,7 +455,7 @@ public class ResponseHighlightingInterceptorTest { ServletRequestDetails reqDetails = new TestServletRequestDetails(); reqDetails.setRequestType(RequestTypeEnum.GET); HashMap params = new HashMap(); - params.put(Constants.PARAM_FORMAT, new String[] { Constants.CT_HTML }); + params.put(Constants.PARAM_FORMAT, new String[]{Constants.CT_HTML}); reqDetails.setParameters(params); reqDetails.setServer(new RestfulServer(ourCtx)); reqDetails.setServletRequest(req); @@ -468,9 +486,9 @@ public class ResponseHighlightingInterceptorTest { ServletRequestDetails reqDetails = new TestServletRequestDetails(); reqDetails.setRequestType(RequestTypeEnum.GET); HashMap params = new HashMap(); - params.put(Constants.PARAM_PRETTY, new String[] { Constants.PARAM_PRETTY_VALUE_TRUE }); - params.put(Constants.PARAM_FORMAT, new String[] { Constants.CT_XML }); - params.put(ResponseHighlighterInterceptor.PARAM_RAW, new String[] { ResponseHighlighterInterceptor.PARAM_RAW_TRUE }); + params.put(Constants.PARAM_PRETTY, new String[]{Constants.PARAM_PRETTY_VALUE_TRUE}); + params.put(Constants.PARAM_FORMAT, new String[]{Constants.CT_XML}); + params.put(ResponseHighlighterInterceptor.PARAM_RAW, new String[]{ResponseHighlighterInterceptor.PARAM_RAW_TRUE}); reqDetails.setParameters(params); reqDetails.setServer(new RestfulServer(ourCtx)); reqDetails.setServletRequest(req); @@ -536,7 +554,7 @@ public class ResponseHighlightingInterceptorTest { ServletRequestDetails reqDetails = new TestServletRequestDetails(); reqDetails.setRequestType(RequestTypeEnum.GET); HashMap params = new HashMap(); - params.put(Constants.PARAM_PRETTY, new String[] { Constants.PARAM_PRETTY_VALUE_TRUE }); + params.put(Constants.PARAM_PRETTY, new String[]{Constants.PARAM_PRETTY_VALUE_TRUE}); reqDetails.setParameters(params); reqDetails.setServer(new RestfulServer(ourCtx)); reqDetails.setServletRequest(req); @@ -586,7 +604,7 @@ public class ResponseHighlightingInterceptorTest { ourLog.info(output); assertThat(output, containsString("resourceType")); } - + @Test public void testHighlightProducesDefaultJsonWithBrowserRequest2() throws Exception { ResponseHighlighterInterceptor ic = ourInterceptor; @@ -702,7 +720,7 @@ public class ResponseHighlightingInterceptorTest { assertThat(responseContent, not(containsStringIgnoringCase("Accept"))); assertThat(responseContent, not(containsStringIgnoringCase("Content-Type"))); } - + @Test public void testShowRequest() throws Exception { ourInterceptor.setShowRequestHeaders(true); @@ -719,7 +737,7 @@ public class ResponseHighlightingInterceptorTest { assertThat(responseContent, (containsStringIgnoringCase("Accept"))); assertThat(responseContent, not(containsStringIgnoringCase("Content-Type"))); } - + @Test public void testShowRequestAndResponse() throws Exception { ourInterceptor.setShowRequestHeaders(true); @@ -768,7 +786,7 @@ public class ResponseHighlightingInterceptorTest { ServletHandler proxyHandler = new ServletHandler(); ourServlet = new RestfulServer(ourCtx); - + /* * Enable CORS */ @@ -783,15 +801,15 @@ public class ResponseHighlightingInterceptorTest { config.addAllowedOrigin("*"); config.addExposedHeader("Location"); config.addExposedHeader("Content-Location"); - config.setAllowedMethods(Arrays.asList("GET","POST","PUT","DELETE","OPTIONS")); + config.setAllowedMethods(Arrays.asList("GET", "POST", "PUT", "DELETE", "OPTIONS")); ourServlet.registerInterceptor(corsInterceptor); - + ourServlet.registerInterceptor(ourInterceptor); ourServlet.setResourceProviders(patientProvider, new DummyBinaryResourceProvider()); ourServlet.setBundleInclusionRule(BundleInclusionRule.BASED_ON_RESOURCE_PRESENCE); ServletHolder servletHolder = new ServletHolder(ourServlet); proxyHandler.addServletWithMapping(servletHolder, "/*"); - + ourServer.setHandler(proxyHandler); ourServer.start(); @@ -801,7 +819,7 @@ public class ResponseHighlightingInterceptorTest { ourClient = builder.build(); } - + class TestServletRequestDetails extends ServletRequestDetails { @Override public String getServerBaseForRequest() { @@ -820,7 +838,7 @@ public class ResponseHighlightingInterceptorTest { public Binary read(@IdParam IdDt theId) { Binary retVal = new Binary(); retVal.setId("1"); - retVal.setContent(new byte[] { 1, 2, 3, 4 }); + retVal.setContent(new byte[]{1, 2, 3, 4}); retVal.setContentType(theId.getIdPart()); return retVal; } @@ -829,7 +847,7 @@ public class ResponseHighlightingInterceptorTest { public List search() { Binary retVal = new Binary(); retVal.setId("1"); - retVal.setContent(new byte[] { 1, 2, 3, 4 }); + retVal.setContent(new byte[]{1, 2, 3, 4}); retVal.setContentType("text/plain"); return Collections.singletonList(retVal); } @@ -895,8 +913,7 @@ public class ResponseHighlightingInterceptorTest { /** * Retrieve the resource by its identifier * - * @param theId - * The resource identity + * @param theId The resource identity * @return The resource */ @Read() @@ -909,8 +926,7 @@ public class ResponseHighlightingInterceptorTest { /** * Retrieve the resource by its identifier * - * @param theId - * The resource identity + * @param theId The resource identity * @return The resource */ @Search() diff --git a/src/changes/changes.xml b/src/changes/changes.xml index 62e32f3e024..ee91912f251 100644 --- a/src/changes/changes.xml +++ b/src/changes/changes.xml @@ -104,6 +104,16 @@ caused an exception if the client made a request with no count parameter included. Thanks to Viviana Sanz for reporting! + + A bug in the JPA server was fixed where a Subscription incorrectly created + without a status or with invalid criteria would cause a crash during + startup. + + + ResponseHighlightingInterceptor now properly parses _format + parameters that include additional content (e.g. + _format=html/json;fhirVersion=1.0]]>) +