issue #2027 review comments

Signed-off-by: Robin Arnold <robin.arnold@ibm.com>

fhir-persistence-jdbc/src/main/java/com/ibm/fhir/persistence/jdbc/domain/SearchQueryRenderer.java

@@ -138,9 +138,10 @@ public class SearchQueryRenderer implements SearchQueryVisitor<QueryData> {
      * @param identityCache
      * @param rowOffset
      * @param rowsPerPage
+     * @param includeResourceData
      */
     public SearchQueryRenderer(JDBCIdentityCache identityCache,
-        int rowOffset, int rowsPerPage, boolean includeResourceData) {
+            int rowOffset, int rowsPerPage, boolean includeResourceData) {
         this.identityCache = identityCache;
         this.rowOffset = rowOffset;
         this.rowsPerPage = rowsPerPage;

fhir-persistence-jdbc/src/main/java/com/ibm/fhir/persistence/jdbc/impl/FHIRPersistenceJDBCImpl.java

@@ -1536,10 +1536,11 @@ private List<com.ibm.fhir.persistence.jdbc.dto.Resource> getResourceDTOs(Resourc
 
     /**
      * Converts the passed Resource Data Transfer Object collection to a collection of FHIR Resource objects.
+     * @param resourceDao
      * @param resourceDTOList
      * @param resourceType
      * @param elements
-     * @param allowNullResource
+     * @param includeResourceData
      * @return
      * @throws FHIRException
      * @throws IOException

fhir-persistence/src/main/java/com/ibm/fhir/persistence/MultiResourceResult.java

@@ -47,7 +47,7 @@ public boolean isSuccess() {
     /**
      * The resource results returned from the interaction
      * @return
-     *     An unmodifiable list containing immutable objects of type {@link ResourceResource<T>}
+     *     An unmodifiable list containing immutable objects of type {@link ResourceResult<T>}
      */
     public List<ResourceResult<? extends Resource>> getResourceResults() {
         return this.resourceResults;

fhir-persistence/src/main/java/com/ibm/fhir/persistence/ResourceResult.java

@@ -14,7 +14,7 @@
 import com.ibm.fhir.model.resource.Resource;
 
 /**
- * The base wrapper result wrapper used to represent a resource being returned from a
+ * The base result wrapper used to represent a resource being returned from a
  * persistence interaction.
  * Instances are immutable and can be constructed via {@code new ResourceResult.Builder<T>()}.
  */
@@ -40,9 +40,23 @@ private ResourceResult(Builder<T> builder) {
         lastUpdated = builder.lastUpdated;
     }
 
+    @Override
+    public String toString() {
+        StringBuilder result = new StringBuilder();
+        result.append(resourceTypeName);
+        result.append("/");
+        result.append(logicalId);
+        result.append("/_history/");
+        result.append(version);
+
+        if (deleted) {
+            result.append(" [deleted]");
+        }
+        return result.toString();
+    }
+
     /**
      * Create a Builder for building instances of this class
-     * @param <T>
      * @return
      */
     public static <T extends Resource> ResourceResult.Builder<T> builder() {

fhir-persistence/src/main/java/com/ibm/fhir/persistence/util/FHIRPersistenceUtil.java

@@ -183,59 +183,6 @@ public static FHIRSystemHistoryContext parseSystemHistoryParameters(Map<String,
         return context;
     }
 
-
-    /**
-     * Create a minimal deleted resource marker from the given resource
-     *
-     * @param deletedResource
-     * @return deletedResourceMarker
-     */
-    @Deprecated
-    public static Resource createDeletedResourceMarker(Resource deletedResource) {
-        try {
-            // Build a fresh meta with only versionid/lastupdated defined
-            Meta meta = Meta.builder()
-                    .versionId(deletedResource.getMeta().getVersionId())
-                    .lastUpdated(deletedResource.getMeta().getLastUpdated())
-                    .build();
-
-            // TODO this will clone the entire resource, but we only want the minimal parameters
-            Resource deletedResourceMarker = deletedResource.toBuilder()
-                    .id(deletedResource.getId())
-                    .meta(meta)
-                    .build();
-
-            return deletedResourceMarker;
-        } catch (Exception e) {
-            throw new IllegalStateException("Error while creating deletion marker for resource of type "
-                    + deletedResource.getClass().getSimpleName());
-        }
-    }
-
-    public static Resource createDeletedResourceMarker(String resourceType, String logicalId, int version, java.time.Instant lastUpdated) {
-        // TODO do we even need this deletion marker now that we have ResourceResult?
-        try {
-            // Build a fresh meta with only versionid/lastupdated defined
-            Meta meta = Meta.builder()
-                    .versionId(Id.of(Integer.toString(version)))
-                    .lastUpdated(Instant.of(lastUpdated.atZone(ZoneOffset.UTC)))
-                    .build();
-
-            // Build a minimal instance of Resource
-            Class<? extends Resource> resourceClass = ModelSupport.getResourceType(resourceType);
-            Resource resource = resourceClass.getDeclaredConstructor().newInstance()
-                    .toBuilder()
-                    .id(logicalId)
-                    .meta(meta)
-                    .build();
-
-            return resource;
-        } catch (Exception e) {
-            throw new IllegalStateException("Error while creating deletion marker for resource of type "
-                    + resourceType);
-        }
-    }
-
     /**
      * Create a new {@link ResourceResult} instance to represent a deleted or partially
      * erased resource

fhir-persistence/src/test/java/com/ibm/fhir/persistence/search/test/AbstractPLSearchTest.java

@@ -189,7 +189,7 @@ protected boolean searchReturnsResource(Class<? extends Resource> resourceTypeTo
      * @throws Exception
      */
     protected boolean searchReturnsResourceResult(Class<? extends Resource> resourceTypeToSearch, Map<String, List<String>> queryParms, String expectedLogicalId,
-        boolean includesData) throws Exception {
+            boolean includesData) throws Exception {
         FHIRSearchContext searchContext = SearchUtil.parseQueryParameters(resourceTypeToSearch, queryParms);
         searchContext.setIncludeResourceData(includesData);
         List<ResourceResult<? extends Resource>> resourceResults = runQueryTest(
@@ -198,7 +198,7 @@ protected boolean searchReturnsResourceResult(Class<? extends Resource> resource
 
         assertNotNull(resourceResults);
 
-        // Find the logicalId in the output and check the the includesData matches
+        // Find the logicalId in the output and check that the includesData matches
         boolean result = false;
         for (ResourceResult<? extends Resource> rr: resourceResults) {
             if (rr.getLogicalId().equals(expectedLogicalId)) {

fhir-server/src/main/java/com/ibm/fhir/server/util/FHIRRestHelper.java

@@ -2234,6 +2234,10 @@ private List<Issue> performSearchReferenceChecks(String resourceType, List<Query
             // Loop through the resources, looking for versioned references and references to multiple resource types for the same logical ID
             for (ResourceResult<? extends Resource> resourceResult : matchResources) {
                 Resource resource = resourceResult.getResource();
+                if (resource == null) {
+                    log.warning("Unexpected null resource: " + resourceResult.toString());
+                    throw new FHIRPersistenceException("Search reference check contained a null resource");
+                }
 
                 // A flag that indicates whether we need to take a closer look at the reference values or not
                 boolean needsEval = false;
@@ -2242,7 +2246,7 @@ private List<Issue> performSearchReferenceChecks(String resourceType, List<Query
                 // then we'll need to check that they aren't used for chaining
                 // TODO Should we pass the previously-gathered set of references into the method instead?
                 CollectingVisitor<Reference> refCollector = new CollectingVisitor<>(Reference.class);
-                resource.accept(refCollector); // TODO protect against null resource?
+                resource.accept(refCollector);
                 List<Reference> references = refCollector.getResult();
                 for (Reference ref : references) {
                     if (ref.getReference() != null && ref.getReference().getValue() != null

fhir-server/src/test/java/com/ibm/fhir/server/util/FHIRRestHelperTest.java

@@ -1984,7 +1984,6 @@ public void testBundleSearchBundleWithNullRsrcAndNoId() throws Exception {
                     .build())
                 .build();
 
-        // TODO make this work with the new search result semantics
         List<ResourceResult<? extends Resource>> resourceResults = new ArrayList<>();
         resourceResults.add(ResourceResult.builder().build());
         resourceResults.add(ResourceResult.builder().resource(patientNoId).build());