Contained device in narrowing (#6537)

* Tests

* Changelog

* remove shims

* more tests

* upgrade

* Remove fixme

* Update hapi-fhir-base/src/main/java/ca/uhn/fhir/context/ModelScanner.java

Co-authored-by: James Agnew <jamesagnew@gmail.com>

---------

Co-authored-by: James Agnew <jamesagnew@gmail.com>

hapi-fhir-base/src/main/java/ca/uhn/fhir/context/ModelScanner.java

@@ -441,6 +441,18 @@ class ModelScanner {
 					providesMembershipInCompartments.add(name);
 				}
 
+				/**
+				 * In the base FHIR R4 specification, the Device resource is not a part of the Patient compartment.
+				 * However, it is a patient-specific resource that most users expect to be, and several derivative
+				 * specifications including g(10) testing expect it to be, and the fact that it is not has led to many
+				 * bug reports in HAPI FHIR. As of HAPI FHIR 8.0.0 it is being manually added in response to those
+				 * requests.
+				 * See https://github.com/hapifhir/hapi-fhir/issues/6536 for more information.
+				 */
+				if (searchParam.name().equals("patient") && searchParam.path().equals("Device.patient")) {
+					providesMembershipInCompartments.add("Patient");
+				}
+
 				List<RuntimeSearchParam.Component> components = null;
 				if (paramType == RestSearchParameterTypeEnum.COMPOSITE) {
 					components = new ArrayList<>();

hapi-fhir-docs/src/main/java/ca/uhn/hapi/fhir/docs/AuthorizationInterceptors.java

@@ -262,7 +262,6 @@ public class AuthorizationInterceptors {
 			}
 		};
 		// END SNIPPET: advancedCompartment
-
 	}
 
 	@SuppressWarnings("InnerClassMayBeStatic")

hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_8_0/6536-make-device-part-of-patient-compartment.yaml

@@ -0,0 +1,14 @@
+---
+type: add
+jira: SMILE-9260
+title: "The `patient` search parameter for the `Device` resource has been added to the Patient Compartment for the purposes of: 
+
+- AuthorizationInterceptor
+- SearchNarrowingInterceptor
+- $everything Operation
+- Patient/Group Bulk Export
+
+This means that a search for $everything on a patient will return devices associated to this patient. This used to be possible
+via the [Advanced Compartment Authorization](/hapi-fhir/docs/security/authorization_interceptor.html#advanced-compartment-authorization), but that would only apply to authorization, 
+and not these other use cases. This solution adds this Search Parameter to all places where compartment membership would be checked. 
+"

hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_8_0/upgrade.md

@@ -1,4 +1,4 @@
-# Upgrade Notes
+## Resource Provenance
 
 The JPA server stores values for the field `Resource.meta.source` in dedicated columns in its database so that they can be indexes and searched for as needed, using the `_source` Search Parameter.
 
@@ -15,6 +15,19 @@ If you do have such data, you should follow the following steps:
 
 * When this reindex operation has successfully completed, the setting above can be disabled. Disabling this setting avoids an extra database round-trip when loading data, so this change will have a positive performance impact on your server.
 
-# Fulltext Search with _lastUpdated Filter
+## Device membership in Patient Compartment
+
+As of 8.0.0, versions of FHIR below R5 now consider the `Device` resource's `patient` Search Parameter to be in the Patient Compartment. The following features are affected:
+
+- Patient Search with `_revInclude=*`
+- Patient instance-level `$everything` operation
+- Patient type-level `$everything` operation
+- Automatic Search Narrowing
+- Bulk Export
+
+Previously, there were various shims in the code that permitted similar behaviour in these features. Those shims have been removed. The only remaining component is [Advanced Compartment Authorization](/hapi-fhir/docs/security/authorization_interceptor.html#advanced-compartment-authorization), which can still be used 
+to add other Search Parameters into a given compartment.
+
+## Fulltext Search with _lastUpdated Filter
 
 Fulltext searches have been updated to support `_lastUpdated` search parameter. If you are using Advanced Hibernate Search indexing and wish to use the `_lastUpdated` search parameetr with this feature, a full reindex of your repository is required.

hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/bulk/BulkExportUseCaseTest.java

@@ -113,8 +113,6 @@ public class BulkExportUseCaseTest extends BaseResourceProviderR4Test {
 	public class SpecConformanceTests {
 
 
-
-
 		@Test
 		public void testBulkExportJobsAreMetaTaggedWithJobIdAndExportId() throws IOException {
 			//Given a patient exists

hapi-fhir-storage-batch2-jobs/src/main/java/ca/uhn/fhir/batch2/jobs/export/BulkDataExportProvider.java

@@ -360,10 +360,6 @@ public class BulkDataExportProvider {
 		if (myCompartmentResources == null) {
 			myCompartmentResources =
 					new HashSet<>(SearchParameterUtil.getAllResourceTypesThatAreInPatientCompartment(theFhirContext));
-			if (isDeviceResourceSupportedForPatientCompartmentForFhirVersion(
-					theFhirContext.getVersion().getVersion())) {
-				myCompartmentResources.add("Device");
-			}
 		}
 		return myCompartmentResources;
 	}
@@ -819,9 +815,4 @@ public class BulkDataExportProvider {
 			throw new InvalidRequestException(Msg.code(513) + "Must request async processing for " + theOperationName);
 		}
 	}
-
-	private static boolean isDeviceResourceSupportedForPatientCompartmentForFhirVersion(
-			FhirVersionEnum theFhirVersionEnum) {
-		return PATIENT_COMPARTMENT_FHIR_VERSIONS_SUPPORT_DEVICE.contains(theFhirVersionEnum);
-	}
 }

hapi-fhir-storage-batch2-jobs/src/test/java/ca/uhn/fhir/batch2/jobs/export/BulkDataExportProviderTest.java

@@ -38,7 +38,7 @@ class BulkDataExportProviderTest {
 	@MethodSource("fhirContexts")
 	void checkDeviceIsSupportedInPatientCompartment(FhirContext theFhirContext) {
 		Set<String> resourceNames = new BulkDataExportProvider().getPatientCompartmentResources(theFhirContext);
-		if (PATIENT_COMPARTMENT_FHIR_VERSIONS_SUPPORT_DEVICE.contains(theFhirContext.getVersion().getVersion())) {
+		if (theFhirContext.getVersion().getVersion().isOlderThan(FhirVersionEnum.R5)) {
 			assertThat(resourceNames).contains("Device");
 		} else {
 			assertThat(resourceNames).doesNotContain("Device");

hapi-fhir-structures-r4/src/test/java/ca/uhn/fhir/rest/server/interceptor/auth/SearchNarrowingInterceptorTest.java

@@ -39,6 +39,7 @@ import org.hl7.fhir.instance.model.api.IAnyResource;
 import org.hl7.fhir.instance.model.api.IBaseResource;
 import org.hl7.fhir.instance.model.api.IIdType;
 import org.hl7.fhir.r4.model.Bundle;
+import org.hl7.fhir.r4.model.Device;
 import org.hl7.fhir.r4.model.IdType;
 import org.hl7.fhir.r4.model.Observation;
 import org.hl7.fhir.r4.model.Parameters;
@@ -100,6 +101,7 @@ public class SearchNarrowingInterceptorTest {
 	private RestfulServerExtension myRestfulServerExtension = new RestfulServerExtension(ourCtx)
 		.registerProvider(new DummyObservationResourceProvider())
 		.registerProvider(new DummyPatientResourceProvider())
+		.registerProvider(new DummyDeviceResourceProvider())
 		.registerProvider(new DummySystemProvider())
 		.withPagingProvider(new FifoMemoryPagingProvider(100));
 
@@ -283,6 +285,42 @@ public class SearchNarrowingInterceptorTest {
 		assertNull(ourLastCodeParam);
 	}
 
+	@Test
+	public void testNarrowCompartment_DevicesByPatientContext_ClientRequestedNoParams() {
+		ourNextAuthorizedList = new AuthorizedList()
+			 .addCompartments("Patient/123", "Patient/456");
+
+		myClient
+			 .search()
+			 .forResource("Device")
+			 .execute();
+
+		assertEquals("Device.search", ourLastHitMethod);
+		assertNull(ourLastIdParam);
+		assertNull(ourLastCodeParam);
+		assertNull(ourLastSubjectParam);
+		assertNull(ourLastPerformerParam);
+		assertThat(toStrings(ourLastPatientParam)).containsExactly("Patient/123,Patient/456");
+	}
+
+	@Test
+	public void testNarrowCompartment_DevicesByPatientContext_ClientRequestedWithParams() {
+		ourNextAuthorizedList = new AuthorizedList()
+			 .addCompartments("Patient/123");
+
+		myClient
+			 .search()
+			 .byUrl("Device?patient=Patient/123")
+			 .execute();
+
+		assertEquals("Device.search", ourLastHitMethod);
+		assertNull(ourLastIdParam);
+		assertNull(ourLastCodeParam);
+		assertNull(ourLastSubjectParam);
+		assertNull(ourLastPerformerParam);
+		assertThat(toStrings(ourLastPatientParam)).containsExactly("Patient/123");
+	}
+
 	@Test
 	public void testNarrowCompartment_ObservationsByPatientContext_ClientRequestedNoParams() {
 		ourNextAuthorizedList = new AuthorizedList()
@@ -950,7 +988,110 @@ public class SearchNarrowingInterceptorTest {
 			return new MethodOutcome(new IdType("Patient/123"), true);
 		}
 
+		@SuppressWarnings("unused")
+		public static class DummyObservationResourceProvider implements IResourceProvider {
+
+			@Override
+			public Class<? extends IBaseResource> getResourceType() {
+				return Observation.class;
+			}
+
+
+			@Search()
+			public List<Resource> search(
+				 @OptionalParam(name = "_id") TokenAndListParam theIdParam,
+				 @OptionalParam(name = Observation.SP_SUBJECT) ReferenceAndListParam theSubjectParam,
+				 @OptionalParam(name = Observation.SP_PATIENT) ReferenceAndListParam thePatientParam,
+				 @OptionalParam(name = Observation.SP_PERFORMER) ReferenceAndListParam thePerformerParam,
+				 @OptionalParam(name = Observation.SP_CODE) TokenAndListParam theCodeParam
+			) {
+				ourLastHitMethod = "Observation.search";
+				ourLastIdParam = theIdParam;
+				ourLastSubjectParam = theSubjectParam;
+				ourLastPatientParam = thePatientParam;
+				ourLastPerformerParam = thePerformerParam;
+				ourLastCodeParam = theCodeParam;
+				return ourReturn;
+			}
+
+			@Create
+			public MethodOutcome create(@ResourceParam IBaseResource theResource, @ConditionalUrlParam String theConditionalUrl) {
+				ourLastHitMethod = "Observation.create";
+				ourLastConditionalUrl = theConditionalUrl;
+				return new MethodOutcome(new IdType("Observation/123"), true);
+			}
+
+			@Update
+			public MethodOutcome update(@ResourceParam IBaseResource theResource, @ConditionalUrlParam String theConditionalUrl) {
+				ourLastHitMethod = "Observation.update";
+				ourLastConditionalUrl = theConditionalUrl;
+				return new MethodOutcome(new IdType("Observation/123"), true);
+			}
+
+			@Delete
+			public MethodOutcome delete(@IdParam IIdType theId, @ConditionalUrlParam String theConditionalUrl) {
+				ourLastHitMethod = "Observation.delete";
+				ourLastConditionalUrl = theConditionalUrl;
+				return new MethodOutcome(new IdType("Observation/123"), true);
+			}
+
+			@Patch
+			public MethodOutcome patch(@IdParam IIdType theId, @ResourceParam IBaseResource theResource, @ConditionalUrlParam String theConditionalUrl, PatchTypeEnum thePatchType) {
+				ourLastHitMethod = "Observation.patch";
+				ourLastConditionalUrl = theConditionalUrl;
+				return new MethodOutcome(new IdType("Observation/123"), true);
+			}
+		}
 	}
+	@SuppressWarnings("unused")
+	public static class DummyDeviceResourceProvider implements IResourceProvider {
+
+		@Override
+		public Class<? extends IBaseResource> getResourceType() {
+			return Device.class;
+		}
+
+
+		@Search()
+		public List<Resource> search(
+			 @OptionalParam(name = "_id") TokenAndListParam theIdParam,
+			 @OptionalParam(name = Device.SP_PATIENT) ReferenceAndListParam thePatientParam
+		) {
+			ourLastHitMethod = "Device.search";
+			ourLastIdParam = theIdParam;
+			ourLastPatientParam = thePatientParam;
+			return ourReturn;
+		}
+
+		@Create
+		public MethodOutcome create(@ResourceParam IBaseResource theResource, @ConditionalUrlParam String theConditionalUrl) {
+			ourLastHitMethod = "Device.create";
+			ourLastConditionalUrl = theConditionalUrl;
+			return new MethodOutcome(new IdType("Device/123"), true);
+		}
+
+		@Update
+		public MethodOutcome update(@ResourceParam IBaseResource theResource, @ConditionalUrlParam String theConditionalUrl) {
+			ourLastHitMethod = "Device.update";
+			ourLastConditionalUrl = theConditionalUrl;
+			return new MethodOutcome(new IdType("Device/123"), true);
+		}
+
+		@Delete
+		public MethodOutcome delete(@IdParam IIdType theId, @ConditionalUrlParam String theConditionalUrl) {
+			ourLastHitMethod = "Device.delete";
+			ourLastConditionalUrl = theConditionalUrl;
+			return new MethodOutcome(new IdType("Device/123"), true);
+		}
+
+		@Patch
+		public MethodOutcome patch(@IdParam IIdType theId, @ResourceParam IBaseResource theResource, @ConditionalUrlParam String theConditionalUrl, PatchTypeEnum thePatchType) {
+			ourLastHitMethod = "Device.patch";
+			ourLastConditionalUrl = theConditionalUrl;
+			return new MethodOutcome(new IdType("Device/123"), true);
+		}
+	}
+
 
 	@SuppressWarnings("unused")
 	public static class DummyObservationResourceProvider implements IResourceProvider {

hapi-fhir-validation/src/test/java/ca/uhn/fhir/rest/server/interceptor/auth/AuthorizationInterceptorR4Test.java

@@ -415,23 +415,19 @@ public class AuthorizationInterceptorR4Test extends BaseValidationTestWithInline
 		extractResponseAndClose(status);
 		assertEquals(200, status.getStatusLine().getStatusCode());
 		assertTrue(ourHitMethod);
-
 	}
 
 	@Test
-	public void testCustomCompartmentSpsOnMultipleInstances() throws Exception {
+	public void testDeviceIsNativelyInPatientCompartmentForAuthorizationPurposes() throws Exception {
 		//Given
 		ourServer.registerInterceptor(new AuthorizationInterceptor(PolicyEnum.DENY) {
 			@Override
 			public List<IAuthRule> buildRuleList(RequestDetails theRequestDetails) {
-				AdditionalCompartmentSearchParameters additionalCompartmentSearchParameters = new AdditionalCompartmentSearchParameters();
-				additionalCompartmentSearchParameters.addSearchParameters("Device:patient");
 				List<IdType> relatedIds = new ArrayList<>();
 				relatedIds.add(new IdType("Patient/123"));
-				relatedIds.add(new IdType("Patient/456"));
 				return new RuleBuilder()
 					.allow().read().allResources()
-					.inCompartmentWithAdditionalSearchParams("Patient", relatedIds, additionalCompartmentSearchParameters)
+					.inCompartment("Patient", relatedIds)
 					.andThen().denyAll()
 					.build();
 			}
@@ -459,19 +455,19 @@ public class AuthorizationInterceptorR4Test extends BaseValidationTestWithInline
 		assertEquals(200, status.getStatusLine().getStatusCode());
 	}
 
+
 	@Test
-	public void testCustomSearchParamsDontOverPermit() throws Exception {
+	public void testCustomCompartmentSpsOnMultipleInstances() throws Exception {
 		//Given
 		ourServer.registerInterceptor(new AuthorizationInterceptor(PolicyEnum.DENY) {
 			@Override
 			public List<IAuthRule> buildRuleList(RequestDetails theRequestDetails) {
-				AdditionalCompartmentSearchParameters additionalCompartmentSearchParameters = new AdditionalCompartmentSearchParameters();
-				additionalCompartmentSearchParameters.addSearchParameters("Encounter:patient");
 				List<IdType> relatedIds = new ArrayList<>();
 				relatedIds.add(new IdType("Patient/123"));
+				relatedIds.add(new IdType("Patient/456"));
 				return new RuleBuilder()
 					.allow().read().allResources()
-					.inCompartmentWithAdditionalSearchParams("Patient", relatedIds, additionalCompartmentSearchParameters)
+					.inCompartment("Patient", relatedIds)
 					.andThen().denyAll()
 					.build();
 			}
@@ -494,12 +490,11 @@ public class AuthorizationInterceptorR4Test extends BaseValidationTestWithInline
 		status = ourClient.execute(httpGet);
 		extractResponseAndClose(status);
 
-		//then
+		//Then
-		assertFalse(ourHitMethod);
+		assertTrue(ourHitMethod);
-		assertEquals(403, status.getStatusLine().getStatusCode());
+		assertEquals(200, status.getStatusLine().getStatusCode());
 	}
 
-
 	@Test
 	public void testNonsenseParametersThrowAtRuntime() throws Exception {
 		//Given
@@ -531,7 +526,7 @@ public class AuthorizationInterceptorR4Test extends BaseValidationTestWithInline
 		ourReturn = Collections.singletonList(d);
 
 		//When
-		httpGet = new HttpGet(ourServer.getBaseUrl() + "/Device/124456");
+		httpGet = new HttpGet(ourServer.getBaseUrl() + "/Device/");
 		status = ourClient.execute(httpGet);
 		extractResponseAndClose(status);