diff --git a/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_4_0/4485-subscription-cross-partition-custom-interceptor-put-update-error-hapi-2010.yaml b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_4_0/4485-subscription-cross-partition-custom-interceptor-put-update-error-hapi-2010.yaml new file mode 100644 index 00000000000..1af0a7c49ab --- /dev/null +++ b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_4_0/4485-subscription-cross-partition-custom-interceptor-put-update-error-hapi-2010.yaml @@ -0,0 +1,5 @@ +type: fix +issue: 4485 +jira: SMILE-5561 +title: "Cross-partition subscription PUT with a custom interceptor will fail on validation because the read partition ID is used. + This has been fixed by skipping validation if the validator invoked during an update operation" diff --git a/hapi-fhir-jpaserver-subscription/src/main/java/ca/uhn/fhir/jpa/subscription/submit/interceptor/SubscriptionValidatingInterceptor.java b/hapi-fhir-jpaserver-subscription/src/main/java/ca/uhn/fhir/jpa/subscription/submit/interceptor/SubscriptionValidatingInterceptor.java index 15e5a4a9af7..12f7d275214 100644 --- a/hapi-fhir-jpaserver-subscription/src/main/java/ca/uhn/fhir/jpa/subscription/submit/interceptor/SubscriptionValidatingInterceptor.java +++ b/hapi-fhir-jpaserver-subscription/src/main/java/ca/uhn/fhir/jpa/subscription/submit/interceptor/SubscriptionValidatingInterceptor.java @@ -70,12 +70,12 @@ public class SubscriptionValidatingInterceptor { @Hook(Pointcut.STORAGE_PRESTORAGE_RESOURCE_CREATED) public void resourcePreCreate(IBaseResource theResource, RequestDetails theRequestDetails, RequestPartitionId theRequestPartitionId) { - validateSubmittedSubscription(theResource, theRequestDetails, theRequestPartitionId); + validateSubmittedSubscription(theResource, theRequestDetails, theRequestPartitionId, Pointcut.STORAGE_PRESTORAGE_RESOURCE_CREATED); } @Hook(Pointcut.STORAGE_PRESTORAGE_RESOURCE_UPDATED) - public void resourcePreCreate(IBaseResource theOldResource, IBaseResource theResource, RequestDetails theRequestDetails, RequestPartitionId theRequestPartitionId) { - validateSubmittedSubscription(theResource, theRequestDetails, theRequestPartitionId); + public void resourceUpdated(IBaseResource theOldResource, IBaseResource theResource, RequestDetails theRequestDetails, RequestPartitionId theRequestPartitionId) { + validateSubmittedSubscription(theResource, theRequestDetails, theRequestPartitionId, Pointcut.STORAGE_PRESTORAGE_RESOURCE_UPDATED); } @Autowired @@ -83,14 +83,30 @@ public class SubscriptionValidatingInterceptor { myFhirContext = theFhirContext; } + // This will be deleted once the next snapshot (6.3.15) is published @Deprecated public void validateSubmittedSubscription(IBaseResource theSubscription) { - validateSubmittedSubscription(theSubscription, null, null); + validateSubmittedSubscription(theSubscription, null, null, Pointcut.STORAGE_PRESTORAGE_RESOURCE_CREATED); } + // This will be deleted once the next snapshot (6.3.15) is published + @Deprecated(since="6.3.14") public void validateSubmittedSubscription(IBaseResource theSubscription, RequestDetails theRequestDetails, RequestPartitionId theRequestPartitionId) { + + validateSubmittedSubscription(theSubscription, theRequestDetails, theRequestPartitionId, Pointcut.STORAGE_PRESTORAGE_RESOURCE_CREATED); + } + + @VisibleForTesting + void validateSubmittedSubscription(IBaseResource theSubscription, + RequestDetails theRequestDetails, + RequestPartitionId theRequestPartitionId, + Pointcut thePointcut) { + if (Pointcut.STORAGE_PRESTORAGE_RESOURCE_CREATED != thePointcut && Pointcut.STORAGE_PRESTORAGE_RESOURCE_UPDATED != thePointcut) { + throw new UnprocessableEntityException(Msg.code(2267) + "Expected Pointcut to be either STORAGE_PRESTORAGE_RESOURCE_CREATED or STORAGE_PRESTORAGE_RESOURCE_UPDATED but was: " + thePointcut); + } + if (!"Subscription".equals(myFhirContext.getResourceType(theSubscription))) { return; } @@ -117,7 +133,7 @@ public class SubscriptionValidatingInterceptor { break; } - validatePermissions(theSubscription, subscription, theRequestDetails, theRequestPartitionId); + validatePermissions(theSubscription, subscription, theRequestDetails, theRequestPartitionId, thePointcut); mySubscriptionCanonicalizer.setMatchingStrategyTag(theSubscription, null); @@ -151,13 +167,18 @@ public class SubscriptionValidatingInterceptor { protected void validatePermissions(IBaseResource theSubscription, CanonicalSubscription theCanonicalSubscription, RequestDetails theRequestDetails, - RequestPartitionId theRequestPartitionId) { + RequestPartitionId theRequestPartitionId, + Pointcut thePointcut) { // If the subscription has the cross partition tag if (SubscriptionUtil.isCrossPartition(theSubscription) && !(theRequestDetails instanceof SystemRequestDetails)) { if (!myDaoConfig.isCrossPartitionSubscriptionEnabled()){ throw new UnprocessableEntityException(Msg.code(2009) + "Cross partition subscription is not enabled on this server"); } + if (theRequestPartitionId == null && Pointcut.STORAGE_PRESTORAGE_RESOURCE_UPDATED == thePointcut) { + return; + } + // if we have a partition id already, we'll use that // otherwise we might end up with READ and CREATE pointcuts // returning conflicting partitions (say, all vs default) diff --git a/hapi-fhir-jpaserver-subscription/src/test/java/ca/uhn/fhir/jpa/subscription/submit/interceptor/SubscriptionValidatingInterceptorTest.java b/hapi-fhir-jpaserver-subscription/src/test/java/ca/uhn/fhir/jpa/subscription/submit/interceptor/SubscriptionValidatingInterceptorTest.java index 5421ceed55d..681a8ba99ac 100644 --- a/hapi-fhir-jpaserver-subscription/src/test/java/ca/uhn/fhir/jpa/subscription/submit/interceptor/SubscriptionValidatingInterceptorTest.java +++ b/hapi-fhir-jpaserver-subscription/src/test/java/ca/uhn/fhir/jpa/subscription/submit/interceptor/SubscriptionValidatingInterceptorTest.java @@ -1,12 +1,14 @@ package ca.uhn.fhir.jpa.subscription.submit.interceptor; import ca.uhn.fhir.context.FhirContext; +import ca.uhn.fhir.interceptor.api.Pointcut; import ca.uhn.fhir.jpa.api.config.DaoConfig; import ca.uhn.fhir.i18n.Msg; import ca.uhn.fhir.jpa.api.dao.DaoRegistry; import ca.uhn.fhir.jpa.partition.IRequestPartitionHelperSvc; import ca.uhn.fhir.jpa.subscription.match.matcher.matching.SubscriptionStrategyEvaluator; import ca.uhn.fhir.jpa.subscription.match.registry.SubscriptionCanonicalizer; +import ca.uhn.fhir.jpa.subscription.model.CanonicalSubscription; import ca.uhn.fhir.rest.server.exceptions.UnprocessableEntityException; import org.hl7.fhir.r4.model.Subscription; import org.junit.jupiter.api.BeforeEach; @@ -20,6 +22,8 @@ import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; import org.springframework.test.context.junit.jupiter.SpringExtension; +import javax.annotation.Nonnull; + import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.is; import static org.junit.jupiter.api.Assertions.fail; @@ -50,7 +54,7 @@ public class SubscriptionValidatingInterceptorTest { public void testEmptySub() { try { Subscription badSub = new Subscription(); - mySubscriptionValidatingInterceptor.validateSubmittedSubscription(badSub); + mySubscriptionValidatingInterceptor.resourcePreCreate(badSub, null, null); fail(); } catch (UnprocessableEntityException e) { assertThat(e.getMessage(), is(Msg.code(8) + "Can not process submitted Subscription - Subscription.status must be populated on this server")); @@ -63,7 +67,7 @@ public class SubscriptionValidatingInterceptorTest { try { Subscription badSub = new Subscription(); badSub.setStatus(Subscription.SubscriptionStatus.ACTIVE); - mySubscriptionValidatingInterceptor.validateSubmittedSubscription(badSub); + mySubscriptionValidatingInterceptor.resourcePreCreate(badSub, null, null); fail(); } catch (UnprocessableEntityException e) { assertThat(e.getMessage(), is(Msg.code(11) + "Subscription.criteria must be populated")); @@ -76,7 +80,7 @@ public class SubscriptionValidatingInterceptorTest { Subscription badSub = new Subscription(); badSub.setStatus(Subscription.SubscriptionStatus.ACTIVE); badSub.setCriteria("Patient"); - mySubscriptionValidatingInterceptor.validateSubmittedSubscription(badSub); + mySubscriptionValidatingInterceptor.resourcePreCreate(badSub, null, null); fail(); } catch (UnprocessableEntityException e) { assertThat(e.getMessage(), is(Msg.code(14) + "Subscription.criteria must be in the form \"{Resource Type}?[params]\"")); @@ -89,7 +93,7 @@ public class SubscriptionValidatingInterceptorTest { Subscription badSub = new Subscription(); badSub.setStatus(Subscription.SubscriptionStatus.ACTIVE); badSub.setCriteria("Patient?"); - mySubscriptionValidatingInterceptor.validateSubmittedSubscription(badSub); + mySubscriptionValidatingInterceptor.resourcePreCreate(badSub, null, null); fail(); } catch (UnprocessableEntityException e) { assertThat(e.getMessage(), is(Msg.code(20) + "Subscription.channel.type must be populated")); @@ -104,7 +108,7 @@ public class SubscriptionValidatingInterceptorTest { badSub.setCriteria("Patient?"); Subscription.SubscriptionChannelComponent channel = badSub.getChannel(); channel.setType(Subscription.SubscriptionChannelType.MESSAGE); - mySubscriptionValidatingInterceptor.validateSubmittedSubscription(badSub); + mySubscriptionValidatingInterceptor.resourcePreCreate(badSub, null, null); fail(); } catch (UnprocessableEntityException e) { assertThat(e.getMessage(), is(Msg.code(16) + "No endpoint defined for message subscription")); @@ -121,7 +125,7 @@ public class SubscriptionValidatingInterceptorTest { channel.setEndpoint("foo"); try { - mySubscriptionValidatingInterceptor.validateSubmittedSubscription(badSub); + mySubscriptionValidatingInterceptor.resourcePreCreate(badSub, null, null); fail(); } catch (UnprocessableEntityException e) { assertThat(e.getMessage(), is(Msg.code(17) + "Only 'channel' protocol is supported for Subscriptions with channel type 'message'")); @@ -129,7 +133,7 @@ public class SubscriptionValidatingInterceptorTest { channel.setEndpoint("channel"); try { - mySubscriptionValidatingInterceptor.validateSubmittedSubscription(badSub); + mySubscriptionValidatingInterceptor.resourcePreCreate(badSub, null, null); fail(); } catch (UnprocessableEntityException e) { assertThat(e.getMessage(), is(Msg.code(17) + "Only 'channel' protocol is supported for Subscriptions with channel type 'message'")); @@ -137,7 +141,7 @@ public class SubscriptionValidatingInterceptorTest { channel.setEndpoint("channel:"); try { - mySubscriptionValidatingInterceptor.validateSubmittedSubscription(badSub); + mySubscriptionValidatingInterceptor.resourcePreCreate(badSub, null, null); fail(); } catch (UnprocessableEntityException e) { assertThat(e.getMessage(), is(Msg.code(19) + "Invalid subscription endpoint uri channel:")); @@ -145,7 +149,36 @@ public class SubscriptionValidatingInterceptorTest { // Happy path channel.setEndpoint("channel:my-queue-name"); - mySubscriptionValidatingInterceptor.validateSubmittedSubscription(badSub); + mySubscriptionValidatingInterceptor.resourcePreCreate(badSub, null, null); + } + + @Test + public void testSubscriptionUpdate() { + final Subscription subscription = createSubscription(); + + // Assert there is no Exception thrown here. + mySubscriptionValidatingInterceptor.resourceUpdated(subscription, subscription, null, null); + } + + @Test + public void testInvalidPointcut() { + try { + mySubscriptionValidatingInterceptor.validateSubmittedSubscription(createSubscription(), null, null, Pointcut.TEST_RB); + fail(); + } catch (UnprocessableEntityException e) { + assertThat(e.getMessage(), is(Msg.code(2267) + "Expected Pointcut to be either STORAGE_PRESTORAGE_RESOURCE_CREATED or STORAGE_PRESTORAGE_RESOURCE_UPDATED but was: " + Pointcut.TEST_RB)); + } + } + + @Nonnull + private static Subscription createSubscription() { + final Subscription subscription = new Subscription(); + subscription.setStatus(Subscription.SubscriptionStatus.REQUESTED); + subscription.setCriteria("Patient?"); + final Subscription.SubscriptionChannelComponent channel = subscription.getChannel(); + channel.setType(Subscription.SubscriptionChannelType.RESTHOOK); + channel.setEndpoint("channel"); + return subscription; } @Configuration diff --git a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/subscription/SubscriptionValidatingInterceptorTest.java b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/subscription/SubscriptionValidatingInterceptorTest.java index 7adc542bba5..dd132f88c7f 100644 --- a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/subscription/SubscriptionValidatingInterceptorTest.java +++ b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/subscription/SubscriptionValidatingInterceptorTest.java @@ -6,6 +6,7 @@ import ca.uhn.fhir.interceptor.model.RequestPartitionId; import ca.uhn.fhir.jpa.api.config.DaoConfig; import ca.uhn.fhir.jpa.api.dao.DaoRegistry; import ca.uhn.fhir.jpa.partition.IRequestPartitionHelperSvc; +import ca.uhn.fhir.jpa.subscription.match.matcher.matching.SubscriptionMatchingStrategy; import ca.uhn.fhir.rest.api.server.SystemRequestDetails; import ca.uhn.fhir.jpa.subscription.match.matcher.matching.SubscriptionStrategyEvaluator; import ca.uhn.fhir.jpa.subscription.match.registry.SubscriptionCanonicalizer; @@ -15,6 +16,7 @@ import ca.uhn.fhir.rest.api.server.RequestDetails; import ca.uhn.fhir.rest.server.exceptions.UnprocessableEntityException; import ca.uhn.fhir.rest.server.servlet.ServletRequestDetails; import ca.uhn.fhir.util.HapiExtensions; +import org.hl7.fhir.instance.model.api.IIdType; import org.hl7.fhir.r4.model.BooleanType; import org.hl7.fhir.r4.model.Patient; import org.hl7.fhir.r4.model.Subscription; @@ -30,10 +32,16 @@ import static org.hamcrest.Matchers.containsString; import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.fail; +import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.ArgumentMatchers.isA; +import static org.mockito.ArgumentMatchers.nullable; +import static org.mockito.Mockito.lenient; +import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; +import static org.mockito.Mockito.spy; import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @ExtendWith(MockitoExtension.class) @@ -49,11 +57,13 @@ public class SubscriptionValidatingInterceptorTest { private IRequestPartitionHelperSvc myRequestPartitionHelperSvc; @Mock private DaoConfig myDaoConfig; + private SubscriptionCanonicalizer mySubscriptionCanonicalizer; @BeforeEach public void before() { mySvc = new SubscriptionValidatingInterceptor(); - mySvc.setSubscriptionCanonicalizerForUnitTest(new SubscriptionCanonicalizer(myCtx)); + mySubscriptionCanonicalizer = spy(new SubscriptionCanonicalizer(myCtx)); + mySvc.setSubscriptionCanonicalizerForUnitTest(mySubscriptionCanonicalizer); mySvc.setDaoRegistryForUnitTest(myDaoRegistry); mySvc.setSubscriptionStrategyEvaluatorForUnitTest(mySubscriptionStrategyEvaluator); mySvc.setFhirContext(myCtx); @@ -66,7 +76,7 @@ public class SubscriptionValidatingInterceptorTest { Subscription subscription = new Subscription(); try { - mySvc.validateSubmittedSubscription(subscription); + mySvc.resourcePreCreate(subscription, null, null); fail(); } catch (UnprocessableEntityException e) { assertThat(e.getMessage(), containsString("Subscription.status must be populated on this server")); @@ -84,7 +94,7 @@ public class SubscriptionValidatingInterceptorTest { subscription.getChannel().setPayload("application/fhir+json"); subscription.getChannel().setEndpoint("http://foo"); - mySvc.validateSubmittedSubscription(subscription); + mySvc.resourcePreCreate(subscription, null, null); } @Test @@ -99,7 +109,7 @@ public class SubscriptionValidatingInterceptorTest { subscription.getChannel().setEndpoint("http://foo"); try { - mySvc.validateSubmittedSubscription(subscription); + mySvc.resourcePreCreate(subscription, null, null); fail(); } catch (UnprocessableEntityException e) { assertThat(e.getMessage(), containsString("Subscription.criteria contains invalid/unsupported resource type: Patient")); @@ -118,7 +128,7 @@ public class SubscriptionValidatingInterceptorTest { subscription.getChannel().setEndpoint("http://foo"); try { - mySvc.validateSubmittedSubscription(subscription); + mySvc.resourcePreCreate(subscription, null, null); fail(); } catch (UnprocessableEntityException e) { assertThat(e.getMessage(), containsString("Subscription.criteria contains invalid/unsupported resource type: Patient")); @@ -136,7 +146,7 @@ public class SubscriptionValidatingInterceptorTest { subscription.getChannel().setPayload("application/fhir+json"); try { - mySvc.validateSubmittedSubscription(subscription); + mySvc.resourcePreCreate(subscription, null, null); fail(); } catch (UnprocessableEntityException e) { assertThat(e.getMessage(), containsString("Rest-hook subscriptions must have Subscription.channel.endpoint defined")); @@ -155,7 +165,7 @@ public class SubscriptionValidatingInterceptorTest { subscription.getChannel().setEndpoint("http://foo"); try { - mySvc.validateSubmittedSubscription(subscription); + mySvc.resourcePreCreate(subscription, null, null); fail(); } catch (UnprocessableEntityException e) { assertThat(e.getMessage(), containsString("Subscription.channel.type must be populated")); @@ -172,7 +182,7 @@ public class SubscriptionValidatingInterceptorTest { subscription.getChannel().setType(Subscription.SubscriptionChannelType.RESTHOOK); subscription.getChannel().setEndpoint("http://foo"); - mySvc.validateSubmittedSubscription(subscription); + mySvc.resourcePreCreate(subscription, null, null); } @Test @@ -184,7 +194,7 @@ public class SubscriptionValidatingInterceptorTest { subscription.getChannel().setEndpoint("http://foo"); try { - mySvc.validateSubmittedSubscription(subscription); + mySvc.resourcePreCreate(subscription, null, null); fail(); } catch (UnprocessableEntityException e) { assertThat(e.getMessage(), containsString("Subscription.criteria must be populated")); @@ -210,7 +220,7 @@ public class SubscriptionValidatingInterceptorTest { // No asserts here because the function should throw an UnprocessableEntityException exception if the subscription // is invalid - assertDoesNotThrow(() -> mySvc.validateSubmittedSubscription(subscription, requestDetails, null)); + assertDoesNotThrow(() -> mySvc.resourcePreCreate(subscription, requestDetails, null)); Mockito.verify(myDaoConfig, times(1)).isCrossPartitionSubscriptionEnabled(); Mockito.verify(myDaoRegistry, times(1)).isResourceTypeSupported(eq("Patient")); Mockito.verify(myRequestPartitionHelperSvc, times(1)).determineCreatePartitionForRequest(isA(RequestDetails.class), isA(Subscription.class), eq("Subscription")); @@ -233,7 +243,7 @@ public class SubscriptionValidatingInterceptorTest { requestDetails.setRestOperationType(RestOperationTypeEnum.CREATE); try { - mySvc.validateSubmittedSubscription(subscription, requestDetails, null); + mySvc.resourcePreCreate(subscription, requestDetails, null); fail(); } catch (UnprocessableEntityException theUnprocessableEntityException) { assertEquals(Msg.code(2010) + "Cross partition subscription must be created on the default partition", theUnprocessableEntityException.getMessage()); @@ -256,7 +266,7 @@ public class SubscriptionValidatingInterceptorTest { requestDetails.setRestOperationType(RestOperationTypeEnum.CREATE); try { - mySvc.validateSubmittedSubscription(subscription, requestDetails, null); + mySvc.resourcePreCreate(subscription, requestDetails, null); fail(); } catch (UnprocessableEntityException theUnprocessableEntityException) { assertEquals(Msg.code(2009) + "Cross partition subscription is not enabled on this server", theUnprocessableEntityException.getMessage()); @@ -280,10 +290,36 @@ public class SubscriptionValidatingInterceptorTest { // No asserts here because the function should throw an UnprocessableEntityException exception if the subscription // is invalid - mySvc.validateSubmittedSubscription(subscription, requestDetails, null); + mySvc.resourcePreCreate(subscription, requestDetails, null); Mockito.verify(myDaoConfig, never()).isCrossPartitionSubscriptionEnabled(); Mockito.verify(myDaoRegistry, times(1)).isResourceTypeSupported(eq("Patient")); Mockito.verify(myRequestPartitionHelperSvc, never()).determineCreatePartitionForRequest(isA(RequestDetails.class), isA(Patient.class), eq("Patient")); } + @Test + public void testSubscriptionUpdate() { + when(myDaoRegistry.isResourceTypeSupported(eq("Patient"))).thenReturn(true); + when(myDaoConfig.isCrossPartitionSubscriptionEnabled()).thenReturn(true); + lenient() + .when(myRequestPartitionHelperSvc.determineReadPartitionForRequestForRead(isA(RequestDetails.class), isA(String.class), isA(IIdType.class))) + .thenReturn(RequestPartitionId.allPartitions()); + + final Subscription subscription = new Subscription(); + subscription.setId("customId1"); + subscription.setStatus(Subscription.SubscriptionStatus.REQUESTED); + subscription.setCriteria("Patient?identifier=foo"); + subscription.getChannel().setType(Subscription.SubscriptionChannelType.RESTHOOK); + subscription.getChannel().setEndpoint("http://foo"); + + subscription.addExtension().setUrl(HapiExtensions.EXTENSION_SUBSCRIPTION_CROSS_PARTITION).setValue(new BooleanType(true)); + + final RequestDetails requestDetails = mock(RequestDetails.class); + lenient() + .when(requestDetails.getRestOperationType()).thenReturn(RestOperationTypeEnum.UPDATE); + + mySvc.resourceUpdated(subscription, subscription, requestDetails, null); + + verify(mySubscriptionStrategyEvaluator).determineStrategy(anyString()); + verify(mySubscriptionCanonicalizer, times(2)).setMatchingStrategyTag(eq(subscription), nullable(SubscriptionMatchingStrategy.class)); + } }