Fix IG/$data-requirements key ValueSet binding discovery (#1032)

* Fix IG/$data-requirements key ValueSet binding discovery

Make IG/$data-requirements emit the same key ValueSet bindings the IG-publisher shows in its "Key Elements Table". For several IGs like hl7.fhir.uv.cql the operation previously surfaced zero ValueSets in the relatedArtifact list and zero entries with crmi-dependencyRole=key even though the IG-publisher's published key elements analysis identifies several per profile.

* Preserve unversioned stub CodeSystem refs in $data-requirements

Author: c-schuler

cqf-fhir-cr/src/main/java/org/opencds/cqf/fhir/cr/visitor/DataRequirementsVisitor.java

@@ -165,6 +165,14 @@ public IBase visit(IKnowledgeArtifactAdapter adapter, IBaseParameters operationP
 
             gatherDependenciesWithClassification(adapter, new ArrayList<>(), relatedArtifacts, ctx);
 
+            // Surface key bindings the dependency walk missed. Some bindings (e.g., on
+            // sliced elements whose binding lives on a complex element type) aren't
+            // reachable through differential + baseDefinition walking, but the snapshot-based
+            // KeyElementAnalyzer correctly identifies them. Promote each transitiveKeyCanonical
+            // not already in relatedArtifacts to a synthetic depends-on entry with role=key,
+            // matching what the IG-publisher's Key Elements Table would show.
+            synthesizeMissingKeyDependencies(relatedArtifacts, transitiveKeyCanonicals, conformanceResolver);
+
             // Add root IG as a composed-of entry
             if (adapter.get() instanceof org.hl7.fhir.r4.model.ImplementationGuide
                     || adapter.get() instanceof org.hl7.fhir.r5.model.ImplementationGuide) {
@@ -281,6 +289,17 @@ private String enrichReference(
             IKnowledgeArtifactAdapter dependencyAdapter,
             ConformanceResourceResolver conformanceResolver) {
         if (dependencyAdapter != null) {
+            var url = dependencyAdapter.getUrl();
+            // External CodeSystems shipped as "stub" entries (CodeSystem.content = not-present)
+            // carry a version field that reflects the publishing package, not the actual
+            // terminology version. NpmRepository excludes these from the version index, so a
+            // present URL with a missing indexed version identifies a stub. Skip the pin.
+            if (conformanceResolver != null
+                    && url != null
+                    && conformanceResolver.getResourceType(url) != null
+                    && conformanceResolver.getVersion(url) == null) {
+                return url;
+            }
             return dependencyAdapter.getCanonical();
         }
         var reference = dependency.getReference();
@@ -424,6 +443,53 @@ private <T extends ICompositeType & IBaseHasExtensions> void addCrmiExtensions(
         }
     }
 
+    @SuppressWarnings("unchecked")
+    private <T extends ICompositeType & IBaseHasExtensions> void synthesizeMissingKeyDependencies(
+            List<T> relatedArtifacts,
+            Set<String> transitiveKeyCanonicals,
+            ConformanceResourceResolver conformanceResolver) {
+        if (transitiveKeyCanonicals == null || transitiveKeyCanonicals.isEmpty()) {
+            return;
+        }
+
+        var existingUrls = relatedArtifacts.stream()
+                .map(IKnowledgeArtifactAdapter::getRelatedArtifactReference)
+                .filter(StringUtils::isNotBlank)
+                .map(Canonicals::getUrl)
+                .collect(Collectors.toSet());
+
+        for (var keyCanonical : transitiveKeyCanonicals) {
+            var url = Canonicals.getUrl(keyCanonical);
+            if (url == null || existingUrls.contains(url)) {
+                continue;
+            }
+
+            var indexedVersion = conformanceResolver != null ? conformanceResolver.getVersion(url) : null;
+            var enrichedRef = indexedVersion != null ? url + "|" + indexedVersion : url;
+
+            var newDep = (T) IKnowledgeArtifactAdapter.newRelatedArtifact(
+                    fhirVersion(), Constants.RELATEDARTIFACT_TYPE_DEPENDSON, enrichedRef, null);
+
+            var extensionList = (List<IBaseExtension<?, ?>>) newDep.getExtension();
+            extensionList.add(ExtensionBuilders.buildDependencyRoleExt(fhirVersion(), "key"));
+
+            if (conformanceResolver != null) {
+                var resourceType = conformanceResolver.getResourceType(url);
+                if (resourceType != null) {
+                    addResourceTypeExtension(resourceType, newDep);
+                }
+                var pkgInfo = conformanceResolver.getPackageInfo(url);
+                if (pkgInfo != null) {
+                    extensionList.add(ExtensionBuilders.buildComplexPackageSourceExt(
+                            fhirVersion(), pkgInfo.packageId(), pkgInfo.version(), pkgInfo.canonical()));
+                }
+            }
+
+            relatedArtifacts.add(newDep);
+            existingUrls.add(url);
+        }
+    }
+
     private void addResourceTypeExtension(String resourceType, ICompositeType relatedArtifact) {
         if (fhirVersion().equals(FhirVersionEnum.R4)) {
             ((org.hl7.fhir.r4.model.RelatedArtifact) relatedArtifact)

cqf-fhir-cr/src/main/java/org/opencds/cqf/fhir/cr/visitor/DependencyRoleClassifier.java

@@ -90,13 +90,32 @@ public static List<String> classifyDependencyRoles(
     /**
      * Checks if a dependency is in the set of transitive key canonicals discovered
      * via ValueSet compose chain walking.
+     * <p>
+     * Normalizes both sides on version: dependency references and key-set entries are
+     * compared on URL only. The set may contain version-pinned URLs (FHIR core writes
+     * bindings inconsistently — e.g. {@code publication-status|4.0.1} pinned but
+     * {@code library-type} not), and dep references are version-stripped at lookup, so
+     * symmetric stripping is required to avoid spurious misses.
      */
     private static boolean isTransitiveKeyDependency(IDependencyInfo dependency, Set<String> transitiveKeyCanonicals) {
         if (transitiveKeyCanonicals == null || transitiveKeyCanonicals.isEmpty()) {
             return false;
         }
         var canonical = getDependencyCanonical(dependency.getReference());
-        return canonical != null && transitiveKeyCanonicals.contains(canonical);
+        if (canonical == null) {
+            return false;
+        }
+        // Fast path: exact URL hit (covers entries that are already version-stripped).
+        if (transitiveKeyCanonicals.contains(canonical)) {
+            return true;
+        }
+        // Fallback: strip versions on the key-set side and compare URLs.
+        for (var keyEntry : transitiveKeyCanonicals) {
+            if (canonical.equals(getDependencyCanonical(keyEntry))) {
+                return true;
+            }
+        }
+        return false;
     }
 
     /**

cqf-fhir-cr/src/test/java/org/opencds/cqf/fhir/cr/visitor/DependencyRoleClassifierTest.java

@@ -524,4 +524,46 @@ void testTransitiveKeyCanonicals_emptySet_noEffect() {
         assertFalse(roles.contains("key"));
         assertTrue(roles.contains("default"));
     }
+
+    @Test
+    void testTransitiveKeyMatchesWhenSetHasVersionPinAndDepDoesNot() {
+        // FHIR core writes some bindings with version pins (e.g.
+        // "http://hl7.org/fhir/ValueSet/publication-status|4.0.1") and others without.
+        // When KeyElementAnalyzer collects these into the transitive key set, dep
+        // references discovered through other paths may be version-stripped. The
+        // classifier must normalize so set membership is URL-only.
+        var resolver = createResolver();
+
+        var library = new Library();
+        library.setUrl("http://example.org/Library/main");
+        var libraryAdapter = (IKnowledgeArtifactAdapter) adapterFactory.createKnowledgeArtifactAdapter(library);
+
+        var transitiveKeySet = Set.of("http://hl7.org/fhir/ValueSet/publication-status|4.0.1");
+        var dependency = createDependency("http://hl7.org/fhir/ValueSet/publication-status");
+
+        var roles = DependencyRoleClassifier.classifyDependencyRoles(
+                dependency, libraryAdapter, null, resolver, transitiveKeySet);
+
+        assertTrue(
+                roles.contains("key"), "Should match version-pinned set entry against version-stripped dep reference");
+    }
+
+    @Test
+    void testTransitiveKeyMatchesWhenDepHasVersionPinAndSetDoesNot() {
+        // Symmetric case: set entry has no pin, dep reference does. Classifier
+        // strips dep on lookup, so this branch already worked, but lock it down.
+        var resolver = createResolver();
+
+        var library = new Library();
+        library.setUrl("http://example.org/Library/main");
+        var libraryAdapter = (IKnowledgeArtifactAdapter) adapterFactory.createKnowledgeArtifactAdapter(library);
+
+        var transitiveKeySet = Set.of("http://hl7.org/fhir/ValueSet/library-type");
+        var dependency = createDependency("http://hl7.org/fhir/ValueSet/library-type|4.0.1");
+
+        var roles = DependencyRoleClassifier.classifyDependencyRoles(
+                dependency, libraryAdapter, null, resolver, transitiveKeySet);
+
+        assertTrue(roles.contains("key"));
+    }
 }

cqf-fhir-utility/src/main/java/org/opencds/cqf/fhir/utility/Canonicals.java

@@ -38,7 +38,10 @@ public static String getResourceType(String canonical) {
             return null;
         }
 
-        canonical = canonical.replace(canonical.substring(canonical.lastIndexOf("/")), "");
+        // Drop only the trailing /<id> segment. Using substring rather than replace,
+        // since replace is global and would mangle self-referential URLs like
+        // http://hl7.org/fhir/StructureDefinition/StructureDefinition.
+        canonical = canonical.substring(0, canonical.lastIndexOf("/"));
         return canonical.contains("/") ? canonical.substring(canonical.lastIndexOf("/") + 1) : canonical;
     }
 

cqf-fhir-utility/src/main/java/org/opencds/cqf/fhir/utility/repository/NpmRepository.java

@@ -1,7 +1,10 @@
 package org.opencds.cqf.fhir.utility.repository;
 
 import ca.uhn.fhir.context.FhirContext;
+import ca.uhn.fhir.model.api.IQueryParameterType;
+import ca.uhn.fhir.rest.param.UriParam;
 import ca.uhn.fhir.rest.server.exceptions.ResourceNotFoundException;
+import com.google.common.collect.Multimap;
 import java.io.File;
 import java.io.InputStream;
 import java.util.ArrayList;
@@ -10,13 +13,17 @@
 import java.util.LinkedList;
 import java.util.List;
 import java.util.Map;
+import java.util.Objects;
 import java.util.Queue;
 import java.util.Set;
+import org.hl7.fhir.instance.model.api.IBaseBundle;
 import org.hl7.fhir.instance.model.api.IBaseResource;
 import org.hl7.fhir.instance.model.api.IIdType;
 import org.hl7.fhir.utilities.npm.FilesystemPackageCacheManager;
 import org.hl7.fhir.utilities.npm.NpmPackage;
+import org.opencds.cqf.fhir.utility.BundleHelper;
 import org.opencds.cqf.fhir.utility.Canonicals;
+import org.opencds.cqf.fhir.utility.Ids;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -41,6 +48,7 @@ public class NpmRepository extends InMemoryFhirRepository {
     private Map<String, String> resourceTypeIndex; // canonical URL → FHIR resource type
     private Map<String, String> versionIndex; // canonical URL → version
     private Map<String, PackageInfo> packageIndex; // canonical URL → owning package info
+    private Map<String, String> idIndex; // canonical URL → resource id (for canonical-by-URL search)
 
     /** Package provenance information for a resource. */
     public record PackageInfo(String packageId, String version, String canonical) {}
@@ -90,6 +98,70 @@ public <T extends IBaseResource, I extends IIdType> T read(
         }
     }
 
+    /**
+     * Canonical-by-URL search support: when an in-memory search comes up empty and the
+     * search includes a {@code url} parameter, look up the URL in the package index and
+     * trigger {@link #read} to lazy-load the resource. Then re-run the in-memory search.
+     *
+     * <p>Without this override, callers that resolve canonicals via search (e.g.
+     * {@code SearchHelper.searchRepositoryByCanonicalWithPaging}) silently miss
+     * NPM-only resources because lazy loading is only wired into {@code read()}.
+     */
+    @Override
+    public <B extends IBaseBundle, T extends IBaseResource> B search(
+            Class<B> bundleType,
+            Class<T> resourceType,
+            Multimap<String, List<IQueryParameterType>> searchParameters,
+            Map<String, String> headers) {
+
+        var result = super.search(bundleType, resourceType, searchParameters, headers);
+
+        if (!BundleHelper.getEntry(result).isEmpty() || dependsOnPackages.isEmpty()) {
+            return result;
+        }
+
+        var url = extractUrlSearchParam(searchParameters);
+        if (url == null) {
+            return result;
+        }
+
+        ensurePackagesLoaded();
+
+        var indexedType = resourceTypeIndex.get(url);
+        if (indexedType == null || !indexedType.equals(resourceType.getSimpleName())) {
+            return result;
+        }
+
+        var indexedId = idIndex.get(url);
+        if (indexedId == null) {
+            return result;
+        }
+
+        try {
+            IIdType id = Ids.newId(fhirContext(), indexedType, indexedId);
+            read(resourceType, id, headers);
+        } catch (Exception e) {
+            logger.debug("NPM lazy-load by URL '{}' failed: {}", url, e.getMessage());
+            return result;
+        }
+
+        return super.search(bundleType, resourceType, searchParameters, headers);
+    }
+
+    private static String extractUrlSearchParam(Multimap<String, List<IQueryParameterType>> searchParameters) {
+        if (searchParameters == null || !searchParameters.containsKey("url")) {
+            return null;
+        }
+        return searchParameters.get("url").stream()
+                .flatMap(List::stream)
+                .filter(UriParam.class::isInstance)
+                .map(UriParam.class::cast)
+                .map(UriParam::getValue)
+                .filter(Objects::nonNull)
+                .findFirst()
+                .orElse(null);
+    }
+
     /**
      * Returns the loaded NPM packages, triggering lazy loading if needed.
      */
@@ -142,17 +214,19 @@ private synchronized void ensurePackagesLoaded() {
         var typeIndex = new HashMap<String, String>();
         var verIndex = new HashMap<String, String>();
         var pkgIndex = new HashMap<String, PackageInfo>();
+        var idMap = new HashMap<String, String>();
         logger.info("Loading NPM packages from {} seed(s)", dependsOnPackages.size());
         try {
             var mgr = createPackageCacheManager();
-            loadPackagesTransitively(mgr, result, typeIndex, verIndex, pkgIndex);
+            loadPackagesTransitively(mgr, result, typeIndex, verIndex, pkgIndex, idMap);
             logger.info("NPM package loading complete: {} package(s) loaded", result.size());
         } catch (Exception e) {
             logger.warn("Could not initialize NPM package cache manager: {}", e.getMessage(), e);
         }
         resourceTypeIndex = typeIndex;
         versionIndex = verIndex;
         packageIndex = pkgIndex;
+        idIndex = idMap;
         loadedPackages = result;
     }
 
@@ -170,7 +244,8 @@ private void loadPackagesTransitively(
             List<NpmPackage> result,
             Map<String, String> typeIndex,
             Map<String, String> verIndex,
-            Map<String, PackageInfo> pkgIndex) {
+            Map<String, PackageInfo> pkgIndex,
+            Map<String, String> idMap) {
         Set<String> loaded = new HashSet<>();
         Queue<String[]> toLoad = new LinkedList<>(dependsOnPackages);
         while (!toLoad.isEmpty()) {
@@ -182,7 +257,7 @@ private void loadPackagesTransitively(
             if (!loaded.add(key)) {
                 continue;
             }
-            loadSinglePackage(mgr, pkgInfo, key, result, typeIndex, verIndex, pkgIndex, toLoad);
+            loadSinglePackage(mgr, pkgInfo, key, result, typeIndex, verIndex, pkgIndex, idMap, toLoad);
         }
     }
 
@@ -194,6 +269,7 @@ private void loadSinglePackage(
             Map<String, String> typeIndex,
             Map<String, String> verIndex,
             Map<String, PackageInfo> pkgIndex,
+            Map<String, String> idMap,
             Queue<String[]> toLoad) {
         try {
             var npmPkg = mgr.loadPackage(pkgInfo[0], pkgInfo[1]);
@@ -204,7 +280,7 @@ private void loadSinglePackage(
             result.add(npmPkg);
             logger.info("Loaded NPM package: {}", key);
 
-            indexPackageResources(npmPkg, key, typeIndex, verIndex, pkgIndex);
+            indexPackageResources(npmPkg, key, typeIndex, verIndex, pkgIndex, idMap);
 
             for (var dep : npmPkg.dependencies()) {
                 var parts = dep.split("#", 2);
@@ -222,7 +298,8 @@ private void indexPackageResources(
             String key,
             Map<String, String> typeIndex,
             Map<String, String> verIndex,
-            Map<String, PackageInfo> pkgIndex) {
+            Map<String, PackageInfo> pkgIndex,
+            Map<String, String> idMap) {
         try {
             var owningPackage = new PackageInfo(npmPkg.id(), npmPkg.version(), npmPkg.canonical());
             for (var info : npmPkg.listIndexedResources()) {
@@ -232,6 +309,9 @@ private void indexPackageResources(
                 if (info.getResourceType() != null) {
                     typeIndex.putIfAbsent(info.getUrl(), info.getResourceType());
                 }
+                if (info.getId() != null) {
+                    idMap.putIfAbsent(info.getUrl(), info.getId());
+                }
                 // Stub CodeSystems (content: not-present) are placeholders —
                 // their version and package provenance are not meaningful.
                 var isStub = "CodeSystem".equals(info.getResourceType()) && "not-present".equals(info.getContent());

cqf-fhir-utility/src/test/java/org/opencds/cqf/fhir/utility/CanonicalsTest.java

@@ -58,6 +58,17 @@ void partialCanonicalType() {
         assertNull(Canonicals.getFragment(testUrl));
     }
 
+    @Test
+    void selfReferentialUrlResolvesResourceType() {
+        // Regression: previously String.replace() stripped both occurrences of
+        // "/StructureDefinition" and returned "fhir" for this self-referential URL.
+        String selfRef = "http://hl7.org/fhir/StructureDefinition/StructureDefinition";
+
+        assertEquals("StructureDefinition", Canonicals.getResourceType(selfRef));
+        assertEquals("StructureDefinition", Canonicals.getIdPart(selfRef));
+        assertEquals(selfRef, Canonicals.getUrl(selfRef));
+    }
+
     @Test
     void canonicalParts() {
         CanonicalType testUrl = new CanonicalType("http://fhir.acme.com/Questionnaire/example|1.0#vs1");