Filter unversioned canonical refs from $package expansion parameters (#1033)

* Filter unversioned canonical refs from $package expansion parameters

* Use Canonicals utility for canonical-URL detection in expansion-param filter

* Spotless

Author: c-schuler

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

@@ -303,6 +303,13 @@ protected void handleValueSets(
         var params = (IParametersAdapter) createAdapterForResource(
                 createAdapterForResource(expansionParams).copy());
 
+        // Drop any expansion-parameter whose value is an unversioned canonical URL.
+        // Terminology servers reject expansions when system-version / canonicalVersion
+        // entries lack a `|version` pin (the expansion would be non-deterministic),
+        // so excluding them here keeps the Tx call viable. Non-URL-valued params
+        // (booleans, codes, etc.) pass through unchanged.
+        filterUnversionedCanonicalParams(params);
+
         var valueSets = BundleHelper.getEntryResources(packagedBundle).stream()
                 .filter(r -> r.fhirType().equals(VALUESET_FHIR_TYPE))
                 .map(v -> (IValueSetAdapter) createAdapterForResource(v))
@@ -359,6 +366,41 @@ protected void handleValueSets(
         });
     }
 
+    /**
+     * Removes expansion-parameter entries whose values are unversioned canonical URLs.
+     * Logs a warning per dropped entry. Non-canonical-valued params (booleans, codes,
+     * language tags, etc.) are kept. A value is treated as canonical when
+     * {@link Canonicals#getUrl(String)} parses it (handles http/https URLs plus
+     * urn:uuid / urn:oid forms) and versioned when {@link Canonicals#getVersion(String)}
+     * returns non-null.
+     */
+    private void filterUnversionedCanonicalParams(IParametersAdapter params) {
+        if (params == null || params.getParameter() == null) {
+            return;
+        }
+        var kept = params.getParameter().stream()
+                .filter(p -> {
+                    if (!p.hasValue()) {
+                        return true;
+                    }
+                    var value = p.getPrimitiveValue();
+                    if (value == null || Canonicals.getUrl(value) == null) {
+                        return true;
+                    }
+                    if (Canonicals.getVersion(value) != null) {
+                        return true;
+                    }
+                    myLogger.warn(
+                            "Excluding unversioned canonical from $package expansion parameters: {}={}",
+                            p.getName(),
+                            value);
+                    return false;
+                })
+                .map(IParametersParameterComponentAdapter::get)
+                .toList();
+        params.setParameter(kept);
+    }
+
     // Helper to surface expansion parameter warnings as OperationOutcome issues
     private void addExpansionWarningsToOperationOutcome(IValueSetAdapter valueSetAdapter) {
         if (valueSetAdapter == null || valueSetAdapter.getExpansion() == null) {

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

@@ -1416,4 +1416,59 @@ private static String decodeCanonicalFromCvId(String id) {
         byte[] bytes = Base64.getDecoder().decode(base64);
         return new String(bytes, StandardCharsets.UTF_8);
     }
+
+    @Test
+    void filterUnversionedCanonicalParams_dropsUrlsWithoutPin_keepsOthers() throws Exception {
+        var visitor = new PackageVisitor(repo);
+
+        var params = parameters();
+        params.addParameter("system-version", "http://snomed.info/sct"); // unversioned canonical — drop
+        params.addParameter("system-version", "http://loinc.org|2.81"); // versioned — keep
+        params.addParameter("canonicalVersion", "http://example.org/ValueSet/foo"); // unversioned canonical — drop
+        params.addParameter("canonicalVersion", "http://example.org/ValueSet/bar|1.0.0"); // versioned — keep
+        params.addParameter("system-version", "urn:oid:1.2.3.4"); // unversioned urn canonical — drop
+        params.addParameter("system-version", "urn:oid:1.2.3.4|2024"); // versioned urn canonical — keep
+        params.addParameter("displayLanguage", "en-US"); // non-canonical — keep
+        params.addParameter("activeOnly", true); // boolean — keep
+
+        var adapter = (IParametersAdapter)
+                IAdapterFactory.forFhirVersion(FhirVersionEnum.R4).createParameters(params);
+
+        var method =
+                PackageVisitor.class.getDeclaredMethod("filterUnversionedCanonicalParams", IParametersAdapter.class);
+        method.setAccessible(true);
+        method.invoke(visitor, adapter);
+
+        var remainingValues = adapter.getParameter().stream()
+                .map(p -> p.getName() + "=" + (p.hasValue() ? p.getPrimitiveValue() : "<no-value>"))
+                .toList();
+
+        assertEquals(5, remainingValues.size(), "should drop all three unversioned canonical entries");
+        assertTrue(remainingValues.contains("system-version=http://loinc.org|2.81"));
+        assertTrue(remainingValues.contains("canonicalVersion=http://example.org/ValueSet/bar|1.0.0"));
+        assertTrue(remainingValues.contains("system-version=urn:oid:1.2.3.4|2024"));
+        assertTrue(remainingValues.contains("displayLanguage=en-US"));
+        assertTrue(remainingValues.contains("activeOnly=true"));
+        assertFalse(remainingValues.stream().anyMatch(v -> v.equals("system-version=http://snomed.info/sct")));
+        assertFalse(
+                remainingValues.stream().anyMatch(v -> v.equals("canonicalVersion=http://example.org/ValueSet/foo")));
+        assertFalse(remainingValues.stream().anyMatch(v -> v.equals("system-version=urn:oid:1.2.3.4")));
+    }
+
+    @Test
+    void filterUnversionedCanonicalParams_handlesNullAndEmptyInputs() throws Exception {
+        var visitor = new PackageVisitor(repo);
+        var method =
+                PackageVisitor.class.getDeclaredMethod("filterUnversionedCanonicalParams", IParametersAdapter.class);
+        method.setAccessible(true);
+
+        // Null adapter — should not throw
+        method.invoke(visitor, new Object[] {null});
+
+        // Empty parameters — should not throw, no-op
+        var empty = (IParametersAdapter)
+                IAdapterFactory.forFhirVersion(FhirVersionEnum.R4).createParameters(parameters());
+        method.invoke(visitor, empty);
+        assertTrue(empty.getParameter().isEmpty());
+    }
 }