Fix for duplicate type field in ELM JSON (#1403)

* Remove choice type specifier type if empty. * Make sure edit is applied to the library itself. Allow direct ELM construction in tests. * Tidy everything up * Tidy everything up
cqframework · Aug 16, 2024 · 16daba2 · 16daba2
1 parent bc10dbe
commit 16daba2
Show file tree

Hide file tree

Showing 7 changed files with 122 additions and 19 deletions.
diff --git a/Src/java/cql-to-elm/src/main/java/org/cqframework/cql/cql2elm/CqlCompiler.java b/Src/java/cql-to-elm/src/main/java/org/cqframework/cql/cql2elm/CqlCompiler.java
@@ -18,6 +18,7 @@
 import org.antlr.v4.runtime.tree.ParseTree;
 import org.cqframework.cql.cql2elm.elm.ElmEdit;
 import org.cqframework.cql.cql2elm.elm.ElmEditor;
+import org.cqframework.cql.cql2elm.elm.IElmEdit;
 import org.cqframework.cql.cql2elm.model.CompiledLibrary;
 import org.cqframework.cql.cql2elm.preprocessor.CqlPreprocessor;
 import org.cqframework.cql.elm.IdObjectFactory;
@@ -234,7 +235,8 @@ public Library run(CharStream is) {
         var edits = allNonNull(
                 !options.contains(EnableAnnotations) ? ElmEdit.REMOVE_ANNOTATION : null,
                 !options.contains(EnableResultTypes) ? ElmEdit.REMOVE_RESULT_TYPE : null,
-                !options.contains(EnableLocators) ? ElmEdit.REMOVE_LOCATOR : null);
+                !options.contains(EnableLocators) ? ElmEdit.REMOVE_LOCATOR : null,
+                ElmEdit.REMOVE_CHOICE_TYPE_SPECIFIER_TYPE_IF_EMPTY);
 
         new ElmEditor(edits).edit(library);
 
@@ -248,7 +250,7 @@ public Library run(CharStream is) {
         return library;
     }
 
-    private List<ElmEdit> allNonNull(ElmEdit... ts) {
+    private List<IElmEdit> allNonNull(IElmEdit... ts) {
         return Arrays.stream(ts).filter(x -> x != null).collect(Collectors.toList());
     }
 }
diff --git a/Src/java/cql-to-elm/src/main/java/org/cqframework/cql/cql2elm/elm/ElmEdit.java b/Src/java/cql-to-elm/src/main/java/org/cqframework/cql/cql2elm/elm/ElmEdit.java
@@ -1,9 +1,10 @@
 package org.cqframework.cql.cql2elm.elm;
 
 import org.hl7.cql_annotations.r1.Annotation;
+import org.hl7.elm.r1.ChoiceTypeSpecifier;
 import org.hl7.elm.r1.Element;
 
-public enum ElmEdit {
+public enum ElmEdit implements IElmEdit {
     REMOVE_LOCATOR {
         @Override
         public void edit(Element element) {
@@ -38,7 +39,24 @@ public void edit(Element element) {
             element.setResultTypeName(null);
             element.setResultTypeSpecifier(null);
         }
-    };
+    },
+    REMOVE_CHOICE_TYPE_SPECIFIER_TYPE_IF_EMPTY {
+        // The ChoiceTypeSpecifier ELM node has a deprecated `type` element which, if not null, clashes with the
+        // `"type" : "ChoiceTypeSpecifier"` field in the JSON serialization of the node. This does not happen in the XML
+        // serialization which nests <type> tags inside <ChoiceTypeSpecifier>.
+        // Because the `type` element is deprecated, it is not normally populated by the compiler anymore and
+        // stays null in the ChoiceTypeSpecifier instance. It is however set to an empty list if you just call
+        // ChoiceTypeSpecifier.getType() (which we do during the ELM optimization stage in the compiler), so
+        // this edit is needed to "protect" the downstream JSON serialization if it can be done without data loss.
 
-    public abstract void edit(Element element);
+        @Override
+        public void edit(Element element) {
+            if (element instanceof ChoiceTypeSpecifier) {
+                var choice = (ChoiceTypeSpecifier) element;
+                if (choice.getType().isEmpty()) {
+                    choice.setType(null);
+                }
+            }
+        }
+    };
 }
diff --git a/Src/java/cql-to-elm/src/main/java/org/cqframework/cql/cql2elm/elm/ElmEditor.java b/Src/java/cql-to-elm/src/main/java/org/cqframework/cql/cql2elm/elm/ElmEditor.java
@@ -10,27 +10,31 @@
 
 public class ElmEditor {
 
-    private final List<ElmEdit> edits;
-    private final FunctionalElmVisitor<Void, List<ElmEdit>> visitor;
+    private final List<IElmEdit> edits;
+    private final FunctionalElmVisitor<Trackable, Void> visitor;
 
-    public ElmEditor(List<ElmEdit> edits) {
+    public ElmEditor(List<IElmEdit> edits) {
         this.edits = Objects.requireNonNull(edits);
-        this.visitor = Visitors.from(ElmEditor::applyEdits);
+        this.visitor = Visitors.from((elm, context) -> elm, this::aggregateResults);
     }
 
     public void edit(Library library) {
-        this.visitor.visitLibrary(library, edits);
+        this.visitor.visitLibrary(library, null);
+
+        // This is needed because aggregateResults is not called on the library itself.
+        this.applyEdits(library);
     }
 
-    protected static Void applyEdits(Trackable trackable, List<ElmEdit> edits) {
-        if (!(trackable instanceof Element)) {
-            return null;
-        }
+    protected Trackable aggregateResults(Trackable aggregate, Trackable nextResult) {
+        applyEdits(nextResult);
+        return aggregate;
+    }
 
-        for (ElmEdit edit : edits) {
-            edit.edit((Element) trackable);
+    protected void applyEdits(Trackable trackable) {
+        if (trackable instanceof Element) {
+            for (IElmEdit edit : edits) {
+                edit.edit((Element) trackable);
+            }
         }
-
-        return null;
     }
 }
diff --git a/Src/java/cql-to-elm/src/main/java/org/cqframework/cql/cql2elm/elm/IElmEdit.java b/Src/java/cql-to-elm/src/main/java/org/cqframework/cql/cql2elm/elm/IElmEdit.java
@@ -0,0 +1,7 @@
+package org.cqframework.cql.cql2elm.elm;
+
+import org.hl7.elm.r1.Element;
+
+public interface IElmEdit {
+    void edit(Element element);
+}
diff --git a/Src/java/cql-to-elm/src/test/java/org/cqframework/cql/cql2elm/ArchitectureTest.java b/Src/java/cql-to-elm/src/test/java/org/cqframework/cql/cql2elm/ArchitectureTest.java
@@ -4,6 +4,7 @@
 
 import com.tngtech.archunit.core.domain.JavaClasses;
 import com.tngtech.archunit.core.importer.ClassFileImporter;
+import com.tngtech.archunit.core.importer.ImportOption;
 import org.cqframework.cql.cql2elm.model.LibraryRef;
 import org.cqframework.cql.elm.IdObjectFactory;
 import org.hl7.elm.r1.Element;
@@ -14,7 +15,9 @@ class ArchitectureTest {
     @Test
     void ensureNoDirectElmConstruction() {
 
-        JavaClasses importedClasses = new ClassFileImporter().importPackages("org.cqframework.cql");
+        JavaClasses importedClasses = new ClassFileImporter()
+                .withImportOption(new ImportOption.DoNotIncludeTests())
+                .importPackages("org.cqframework.cql");
 
         constructors()
                 .that()

diff --git a/Src/java/cql-to-elm/src/test/java/org/cqframework/cql/cql2elm/elm/ElmEditTest.java b/Src/java/cql-to-elm/src/test/java/org/cqframework/cql/cql2elm/elm/ElmEditTest.java
@@ -0,0 +1,32 @@
+package org.cqframework.cql.cql2elm.elm;
+
+import static org.junit.jupiter.api.Assertions.*;
+
+import java.util.List;
+import org.hl7.elm.r1.ChoiceTypeSpecifier;
+import org.hl7.elm.r1.NamedTypeSpecifier;
+import org.hl7.elm.r1.TypeSpecifier;
+import org.junit.jupiter.api.Test;
+
+class ElmEditTest {
+
+    @Test
+    void removeChoiceTypeSpecifierTypeIfEmpty() {
+        var extChoiceTypeSpecifier = new ExtChoiceTypeSpecifier();
+
+        extChoiceTypeSpecifier.setType(List.of());
+        ElmEdit.REMOVE_CHOICE_TYPE_SPECIFIER_TYPE_IF_EMPTY.edit(extChoiceTypeSpecifier);
+        assertNull(extChoiceTypeSpecifier.getType());
+
+        var typeSpecifiers = List.of((TypeSpecifier) new NamedTypeSpecifier());
+        extChoiceTypeSpecifier.setType(typeSpecifiers);
+        ElmEdit.REMOVE_CHOICE_TYPE_SPECIFIER_TYPE_IF_EMPTY.edit(extChoiceTypeSpecifier);
+        assertSame(typeSpecifiers, extChoiceTypeSpecifier.getType());
+    }
+
+    private static class ExtChoiceTypeSpecifier extends ChoiceTypeSpecifier {
+        public List<TypeSpecifier> getType() {
+            return type;
+        }
+    }
+}
diff --git a/Src/java/cql-to-elm/src/test/java/org/cqframework/cql/cql2elm/elm/ElmEditorTest.java b/Src/java/cql-to-elm/src/test/java/org/cqframework/cql/cql2elm/elm/ElmEditorTest.java
@@ -0,0 +1,37 @@
+package org.cqframework.cql.cql2elm.elm;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+import java.util.List;
+import org.hl7.elm.r1.ChoiceTypeSpecifier;
+import org.hl7.elm.r1.Library;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+class ElmEditorTest {
+    private int editCount = 0;
+    private final ElmEditor editor = new ElmEditor(List.of(element -> editCount++));
+
+    @BeforeEach
+    void beforeEach() {
+        editCount = 0;
+    }
+
+    @Test
+    void edit() {
+        editor.edit(new Library());
+        assertEquals(editCount, 1);
+    }
+
+    @Test
+    void applyEdits1() {
+        editor.applyEdits(null);
+        assertEquals(editCount, 0);
+    }
+
+    @Test
+    void applyEdits2() {
+        editor.applyEdits(new ChoiceTypeSpecifier());
+        assertEquals(editCount, 1);
+    }
+}