Index canonical references (#1728)

* Index canonical references

* Add changelog

* Fix compile error

* Add whitespace

hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/4_3_0/1728-index-canonical.yaml

@@ -0,0 +1,5 @@
+---
+type: add
+issue: 1728
+title: "Fields of type `canonical` were not previously indexed by the JPA server, meaning that some
+   default search parameters could not be honoured (e.g. StructureDefinition:valueset). This is now corrected."

hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4SearchNoFtTest.java

@@ -3,7 +3,14 @@ package ca.uhn.fhir.jpa.dao.r4;
 import ca.uhn.fhir.context.RuntimeResourceDefinition;
 import ca.uhn.fhir.jpa.dao.DaoConfig;
 import ca.uhn.fhir.jpa.entity.Search;
-import ca.uhn.fhir.jpa.model.entity.*;
+import ca.uhn.fhir.jpa.model.entity.ResourceIndexedSearchParamDate;
+import ca.uhn.fhir.jpa.model.entity.ResourceIndexedSearchParamNumber;
+import ca.uhn.fhir.jpa.model.entity.ResourceIndexedSearchParamQuantity;
+import ca.uhn.fhir.jpa.model.entity.ResourceIndexedSearchParamString;
+import ca.uhn.fhir.jpa.model.entity.ResourceIndexedSearchParamToken;
+import ca.uhn.fhir.jpa.model.entity.ResourceIndexedSearchParamUri;
+import ca.uhn.fhir.jpa.model.entity.ResourceLink;
+import ca.uhn.fhir.jpa.model.entity.ResourceTable;
 import ca.uhn.fhir.jpa.searchparam.MatchUrlService;
 import ca.uhn.fhir.jpa.searchparam.SearchParameterMap;
 import ca.uhn.fhir.jpa.searchparam.SearchParameterMap.EverythingModeEnum;
@@ -33,7 +40,11 @@ import org.hl7.fhir.r4.model.Enumerations.AdministrativeGender;
 import org.hl7.fhir.r4.model.Observation.ObservationStatus;
 import org.hl7.fhir.r4.model.Subscription.SubscriptionChannelType;
 import org.hl7.fhir.r4.model.Subscription.SubscriptionStatus;
-import org.junit.*;
+import org.junit.After;
+import org.junit.AfterClass;
+import org.junit.Before;
+import org.junit.Ignore;
+import org.junit.Test;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.transaction.TransactionStatus;
 import org.springframework.transaction.support.TransactionCallback;
@@ -44,12 +55,30 @@ import javax.servlet.http.HttpServletRequest;
 import java.io.IOException;
 import java.math.BigDecimal;
 import java.nio.charset.StandardCharsets;
-import java.util.*;
+import java.util.Collections;
+import java.util.Date;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+import java.util.TreeSet;
 import java.util.stream.Collectors;
 
 import static org.hamcrest.CoreMatchers.is;
-import static org.hamcrest.Matchers.*;
+import static org.hamcrest.Matchers.contains;
-import static org.junit.Assert.*;
+import static org.hamcrest.Matchers.containsInAnyOrder;
+import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.empty;
+import static org.hamcrest.Matchers.endsWith;
+import static org.hamcrest.Matchers.hasItem;
+import static org.hamcrest.Matchers.hasItems;
+import static org.hamcrest.Matchers.hasSize;
+import static org.hamcrest.Matchers.not;
+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.junit.Assert.fail;
 import static org.mockito.Mockito.mock;
 
 @SuppressWarnings({"unchecked", "Duplicates"})
@@ -73,6 +102,28 @@ public class FhirResourceDaoR4SearchNoFtTest extends BaseJpaR4Test {
 		myDaoConfig.setReuseCachedSearchResultsForMillis(null);
 	}
 
+	@Test
+	public void testCanonicalReference() {
+		StructureDefinition sd = new StructureDefinition();
+		sd.getSnapshot().addElement().getBinding().setValueSet("http://foo");
+		String id = myStructureDefinitionDao.create(sd).getId().toUnqualifiedVersionless().getValue();
+
+		{
+			SearchParameterMap map = new SearchParameterMap();
+			map.setLoadSynchronous(true);
+			map.add(StructureDefinition.SP_VALUESET, new ReferenceParam("http://foo"));
+			List<String> ids = toUnqualifiedVersionlessIdValues(myStructureDefinitionDao.search(map));
+			assertThat(ids, contains(id));
+		}
+		{
+			SearchParameterMap map = new SearchParameterMap();
+			map.setLoadSynchronous(true);
+			map.add(StructureDefinition.SP_VALUESET, new ReferenceParam("http://foo2"));
+			List<String> ids = toUnqualifiedVersionlessIdValues(myStructureDefinitionDao.search(map));
+			assertThat(ids, empty());
+		}
+	}
+
 	@Test
 	public void testHasConditionAgeCompare() {
 		Patient patient = new Patient();
@@ -4365,7 +4416,6 @@ public class FhirResourceDaoR4SearchNoFtTest extends BaseJpaR4Test {
 	}
 
 
-
 	private String toStringMultiline(List<?> theResults) {
 		StringBuilder b = new StringBuilder();
 		for (Object next : theResults) {

hapi-fhir-jpaserver-model/src/main/java/ca/uhn/fhir/jpa/model/entity/ResourceLink.java

@@ -84,22 +84,6 @@ public class ResourceLink extends BaseResourceIndex {
 		super();
 	}
 
-	public ResourceLink(String theSourcePath, ResourceTable theSourceResource, IIdType theTargetResourceUrl, Date theUpdated) {
-		super();
-		setSourcePath(theSourcePath);
-		setSourceResource(theSourceResource);
-		setTargetResourceUrl(theTargetResourceUrl);
-		setUpdated(theUpdated);
-	}
-
-	public ResourceLink(String theSourcePath, ResourceTable theSourceResource, ResourceTable theTargetResource, Date theUpdated) {
-		super();
-		setSourcePath(theSourcePath);
-		setSourceResource(theSourceResource);
-		setTargetResource(theTargetResource);
-		setUpdated(theUpdated);
-	}
-
 	@Override
 	public boolean equals(Object theObj) {
 		if (this == theObj) {
@@ -158,18 +142,25 @@ public class ResourceLink extends BaseResourceIndex {
 		Validate.isTrue(theTargetResourceUrl.hasResourceType());
 
 //		if (theTargetResourceUrl.hasIdPart()) {
-			// do nothing
+		// do nothing
 //		} else {
-			// Must have set an url like http://example.org/something
+		// Must have set an url like http://example.org/something
-			// We treat 'something' as the resource type because of fix for #659. Prior to #659 fix, 'something' was
+		// We treat 'something' as the resource type because of fix for #659. Prior to #659 fix, 'something' was
-			// treated as the id and 'example.org' was treated as the resource type
+		// treated as the id and 'example.org' was treated as the resource type
-			// TODO: log a warning?
+		// TODO: log a warning?
 //		}
 
 		myTargetResourceType = theTargetResourceUrl.getResourceType();
 		myTargetResourceUrl = theTargetResourceUrl.getValue();
 	}
 
+	public void setTargetResourceUrlCanonical(String theTargetResourceUrl) {
+		Validate.notBlank(theTargetResourceUrl);
+
+		myTargetResourceType = "(unknown)";
+		myTargetResourceUrl = theTargetResourceUrl;
+	}
+
 	public Date getUpdated() {
 		return myUpdated;
 	}
@@ -216,4 +207,34 @@ public class ResourceLink extends BaseResourceIndex {
 		return b.toString();
 	}
 
+	public static ResourceLink forAbsoluteReference(String theSourcePath, ResourceTable theSourceResource, IIdType theTargetResourceUrl, Date theUpdated) {
+		ResourceLink retVal = new ResourceLink();
+		retVal.setSourcePath(theSourcePath);
+		retVal.setSourceResource(theSourceResource);
+		retVal.setTargetResourceUrl(theTargetResourceUrl);
+		retVal.setUpdated(theUpdated);
+		return retVal;
+	}
+
+	/**
+	 * Factory for canonical URL
+	 */
+	public static ResourceLink forLogicalReference(String theSourcePath, ResourceTable theSourceResource, String theTargetResourceUrl, Date theUpdated) {
+		ResourceLink retVal = new ResourceLink();
+		retVal.setSourcePath(theSourcePath);
+		retVal.setSourceResource(theSourceResource);
+		retVal.setTargetResourceUrlCanonical(theTargetResourceUrl);
+		retVal.setUpdated(theUpdated);
+		return retVal;
+	}
+
+	public static ResourceLink forLocalReference(String theSourcePath, ResourceTable theSourceResource, ResourceTable theTargetResource, Date theUpdated) {
+		ResourceLink retVal = new ResourceLink();
+		retVal.setSourcePath(theSourcePath);
+		retVal.setSourceResource(theSourceResource);
+		retVal.setTargetResource(theTargetResource);
+		retVal.setUpdated(theUpdated);
+		return retVal;
+	}
+
 }

hapi-fhir-jpaserver-searchparam/src/main/java/ca/uhn/fhir/jpa/searchparam/extractor/BaseSearchParamExtractor.java

@@ -125,23 +125,29 @@ public abstract class BaseSearchParamExtractor implements ISearchParamExtractor
 				case "uri":
 				case "canonical":
 					String typeName = toTypeName(value);
+					IPrimitiveType<?> valuePrimitive = (IPrimitiveType<?>) value;
+					IBaseReference fakeReference = (IBaseReference) myContext.getElementDefinition("Reference").newInstance();
+					fakeReference.setReference(valuePrimitive.getValueAsString());
 
 					// Canonical has a root type of "uri"
 					if ("canonical".equals(typeName)) {
-						IPrimitiveType<?> valuePrimitive = (IPrimitiveType<?>) value;
-						IBaseReference fakeReference = (IBaseReference) myContext.getElementDefinition("Reference").newInstance();
-						fakeReference.setReference(valuePrimitive.getValueAsString());
 
 						/*
 						 * See #1583
 						 * Technically canonical fields should not allow local references (e.g.
 						 * Questionnaire/123) but it seems reasonable for us to interpret a canonical
-						 * containing a local reference for what it is, and allow people to seaerch
+						 * containing a local reference for what it is, and allow people to search
 						 * based on that.
 						 */
 						IIdType parsed = fakeReference.getReferenceElement();
 						if (parsed.hasIdPart() && parsed.hasResourceType() && !parsed.isAbsolute()) {
-							PathAndRef ref = new PathAndRef(searchParam.getName(), path, fakeReference);
+							PathAndRef ref = new PathAndRef(searchParam.getName(), path, fakeReference, false);
+							params.add(ref);
+							break;
+						}
+
+						if (parsed.isAbsolute()) {
+							PathAndRef ref = new PathAndRef(searchParam.getName(), path, fakeReference, true);
 							params.add(ref);
 							break;
 						}
@@ -165,7 +171,7 @@ public abstract class BaseSearchParamExtractor implements ISearchParamExtractor
 						return;
 					}
 
-					PathAndRef ref = new PathAndRef(searchParam.getName(), path, valueRef);
+					PathAndRef ref = new PathAndRef(searchParam.getName(), path, valueRef, false);
 					params.add(ref);
 					break;
 				default:

hapi-fhir-jpaserver-searchparam/src/main/java/ca/uhn/fhir/jpa/searchparam/extractor/PathAndRef.java

@@ -27,15 +27,21 @@ public class PathAndRef {
 	private final String myPath;
 	private final IBaseReference myRef;
 	private final String mySearchParamName;
+	private final boolean myCanonical;
 
 	/**
 	 * Constructor
 	 */
-	public PathAndRef(String theSearchParamName, String thePath, IBaseReference theRef) {
+	public PathAndRef(String theSearchParamName, String thePath, IBaseReference theRef, boolean theCanonical) {
 		super();
 		mySearchParamName = theSearchParamName;
 		myPath = thePath;
 		myRef = theRef;
+		myCanonical = theCanonical;
+	}
+
+	public boolean isCanonical() {
+		return myCanonical;
 	}
 
 	public String getSearchParamName() {

hapi-fhir-jpaserver-searchparam/src/main/java/ca/uhn/fhir/jpa/searchparam/extractor/ResourceLinkExtractor.java

@@ -87,8 +87,10 @@ public class ResourceLinkExtractor {
 
 		theParams.myPopulatedResourceLinkParameters.add(thePathAndRef.getSearchParamName());
 
-		if (LogicalReferenceHelper.isLogicalReference(myModelConfig, nextId)) {
+		boolean canonical = thePathAndRef.isCanonical();
-			ResourceLink resourceLink = new ResourceLink(thePathAndRef.getPath(), theEntity, nextId, theUpdateTime);
+		if (LogicalReferenceHelper.isLogicalReference(myModelConfig, nextId) || canonical) {
+			String value = nextId.getValue();
+			ResourceLink resourceLink = ResourceLink.forLogicalReference(thePathAndRef.getPath(), theEntity, value, theUpdateTime);
 			if (theParams.myLinks.add(resourceLink)) {
 				ourLog.debug("Indexing remote resource reference URL: {}", nextId);
 			}
@@ -130,7 +132,7 @@ public class ResourceLinkExtractor {
 				String msg = myContext.getLocalizer().getMessage(BaseSearchParamExtractor.class, "externalReferenceNotAllowed", nextId.getValue());
 				throw new InvalidRequestException(msg);
 			} else {
-				ResourceLink resourceLink = new ResourceLink(thePathAndRef.getPath(), theEntity, nextId, theUpdateTime);
+				ResourceLink resourceLink = ResourceLink.forAbsoluteReference(thePathAndRef.getPath(), theEntity, nextId, theUpdateTime);
 				if (theParams.myLinks.add(resourceLink)) {
 					ourLog.debug("Indexing remote resource reference URL: {}", nextId);
 				}
@@ -165,7 +167,7 @@ public class ResourceLinkExtractor {
 			return null;
 		}
 
-		return new ResourceLink(nextPathAndRef.getPath(), theEntity, targetResource, theUpdateTime);
+		return ResourceLink.forLocalReference(nextPathAndRef.getPath(), theEntity, targetResource, theUpdateTime);
 	}
 
 }

hapi-fhir-jpaserver-searchparam/src/test/java/ca/uhn/fhir/jpa/searchparam/extractor/ResourceIndexedSearchParamsTest.java

@@ -32,7 +32,7 @@ public class ResourceIndexedSearchParamsTest {
 		myTarget.setResourceType("Organization");
 
 		myParams = new ResourceIndexedSearchParams(source);
-		ResourceLink link = new ResourceLink("organization", source, myTarget, new Date());
+		ResourceLink link = ResourceLink.forLocalReference("organization", source, myTarget, new Date());
 		myParams.getResourceLinks().add(link);
 	}