Fix multiple iterations of an artifact in ReleaseVisitor (#555)

* Add check to release visitor to prevent iterating through the same artifact multiple times * add short circuit and tests * spotless * reduce complexity --------- Co-authored-by: taha.attari@smilecdr.com <taha.attari@smilecdr.com>
cqframework · Oct 9, 2024 · 6a2cfb0 · 6a2cfb0
1 parent 0831d31
commit 6a2cfb0
Show file tree

Hide file tree

Showing 7 changed files with 259 additions and 127 deletions.
diff --git a/cqf-fhir-cr/src/main/java/org/opencds/cqf/fhir/cr/visitor/DataRequirementsVisitor.java b/cqf-fhir-cr/src/main/java/org/opencds/cqf/fhir/cr/visitor/DataRequirementsVisitor.java
@@ -11,8 +11,10 @@
 import java.io.InputStream;
 import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.HashSet;
 import java.util.List;
 import java.util.Optional;
+import java.util.Set;
 import java.util.stream.Collectors;
 import org.apache.commons.lang3.StringUtils;
 import org.cqframework.cql.cql2elm.CqlTranslator;
@@ -102,7 +104,7 @@ public IBase visit(KnowledgeArtifactAdapter adapter, Repository repository, IBas
             library.setStatus(adapter.getStatus());
             library.setType("module-definition");
         }
-        var gatheredResources = new ArrayList<String>();
+        var gatheredResources = new HashSet<String>();
         var relatedArtifacts = stripInvalid(library);
         recursiveGather(
                 adapter.get(),
@@ -203,7 +205,7 @@ protected LibraryManager createLibraryManager(Repository repository) {
 
     protected <T extends ICompositeType & IBaseHasExtensions> void recursiveGather(
             IDomainResource resource,
-            List<String> gatheredResources,
+            Set<String> gatheredResources,
             List<T> relatedArtifacts,
             Repository repository,
             List<String> capability,
@@ -212,10 +214,13 @@ protected <T extends ICompositeType & IBaseHasExtensions> void recursiveGather(
             List<String> checkArtifactVersion,
             List<String> forceArtifactVersion)
             throws PreconditionFailedException {
-        if (resource != null && !gatheredResources.contains(resource.getId())) {
-            gatheredResources.add(resource.getId());
-            var fhirVersion = resource.getStructureFhirVersionEnum();
-            var adapter = AdapterFactory.forFhirVersion(fhirVersion).createKnowledgeArtifactAdapter(resource);
+        if (resource == null) {
+            return;
+        }
+        var fhirVersion = resource.getStructureFhirVersionEnum();
+        var adapter = AdapterFactory.forFhirVersion(fhirVersion).createKnowledgeArtifactAdapter(resource);
+        if (!gatheredResources.contains(adapter.getCanonical())) {
+            gatheredResources.add(adapter.getCanonical());
             findUnsupportedCapability(adapter, capability);
             processCanonicals(adapter, artifactVersion, checkArtifactVersion, forceArtifactVersion);
             var reference = adapter.hasVersion()

diff --git a/...-utility/src/main/java/org/opencds/cqf/fhir/utility/adapter/KnowledgeArtifactAdapter.java b/...-utility/src/main/java/org/opencds/cqf/fhir/utility/adapter/KnowledgeArtifactAdapter.java
@@ -98,6 +98,17 @@ default void setVersion(String version) {
         getModelResolver().setValue(get(), "version", newStringType(get().getStructureFhirVersionEnum(), version));
     }
 
+    /**
+     * Returns the url of the artifact appended with '|' version if the artifact has a version.
+     * @return
+     */
+    default String getCanonical() {
+        if (!hasUrl()) {
+            return getId().getValueAsString();
+        }
+        return getUrl().concat(hasVersion() ? String.format("|%s", getVersion()) : "");
+    }
+
     List<IDependencyInfo> getDependencies();
 
     default String getReferenceSource() {

diff --git a/cqf-fhir-utility/src/main/java/org/opencds/cqf/fhir/utility/visitor/PackageVisitor.java b/cqf-fhir-utility/src/main/java/org/opencds/cqf/fhir/utility/visitor/PackageVisitor.java
@@ -13,8 +13,10 @@
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Date;
+import java.util.HashSet;
 import java.util.List;
 import java.util.Optional;
+import java.util.Set;
 import java.util.stream.Collectors;
 import org.apache.commons.lang3.StringUtils;
 import org.hl7.fhir.instance.model.api.IBase;
@@ -46,7 +48,6 @@ public class PackageVisitor implements IKnowledgeArtifactVisitor {
     protected final FhirContext fhirContext;
     protected final TerminologyServerClient terminologyServerClient;
     protected final ExpandHelper expandHelper;
-    protected List<String> packagedResources;
 
     public PackageVisitor(FhirContext fhirContext) {
         this.fhirContext = fhirContext;
@@ -114,7 +115,7 @@ public IBase visit(KnowledgeArtifactAdapter adapter, Repository repository, IBas
             throw new UnprocessableEntityException("'count' must be non-negative");
         }
         var resource = adapter.get();
-        packagedResources = new ArrayList<>();
+        var packagedResources = new HashSet<String>();
         // TODO: In the case of a released (active) root Library we can depend on the relatedArtifacts as a
         // comprehensive manifest
         var packagedBundle = BundleHelper.newBundle(fhirVersion);
@@ -126,6 +127,7 @@ public IBase visit(KnowledgeArtifactAdapter adapter, Repository repository, IBas
         } else {
             recursivePackage(
                     resource,
+                    packagedResources,
                     packagedBundle,
                     repository,
                     capability,
@@ -188,6 +190,7 @@ protected void handleValueSets(
 
     protected void recursivePackage(
             IDomainResource resource,
+            Set<String> packagedResources,
             IBaseBundle bundle,
             Repository repository,
             List<String> capability,
@@ -197,10 +200,13 @@ protected void recursivePackage(
             List<String> forceArtifactVersion,
             boolean isPut)
             throws PreconditionFailedException {
-        if (resource != null && !packagedResources.contains(resource.getId())) {
-            packagedResources.add(resource.getId());
-            var fhirVersion = resource.getStructureFhirVersionEnum();
-            var adapter = AdapterFactory.forFhirVersion(fhirVersion).createKnowledgeArtifactAdapter(resource);
+        if (resource == null) {
+            return;
+        }
+        var fhirVersion = resource.getStructureFhirVersionEnum();
+        var adapter = AdapterFactory.forFhirVersion(fhirVersion).createKnowledgeArtifactAdapter(resource);
+        if (!packagedResources.contains(adapter.getCanonical())) {
+            packagedResources.add(adapter.getCanonical());
             findUnsupportedCapability(adapter, capability);
             processCanonicals(adapter, artifactVersion, checkArtifactVersion, forceArtifactVersion);
             boolean entryExists = BundleHelper.getEntryResources(bundle).stream()
@@ -231,6 +237,7 @@ protected void recursivePackage(
                     .map(searchBundle -> (IDomainResource) BundleHelper.getEntryResourceFirstRep(searchBundle))
                     .forEach(component -> recursivePackage(
                             component,
+                            packagedResources,
                             bundle,
                             repository,
                             capability,