From 16daba2fa28ceffff28c30357078d123f8ba8c27 Mon Sep 17 00:00:00 2001 From: Anton Vasetenkov Date: Sat, 17 Aug 2024 00:41:38 +1200 Subject: [PATCH] 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/cql/cql2elm/CqlCompiler.java | 6 ++- .../cqframework/cql/cql2elm/elm/ElmEdit.java | 24 ++++++++++-- .../cql/cql2elm/elm/ElmEditor.java | 30 ++++++++------- .../cqframework/cql/cql2elm/elm/IElmEdit.java | 7 ++++ .../cql/cql2elm/ArchitectureTest.java | 5 ++- .../cql/cql2elm/elm/ElmEditTest.java | 32 ++++++++++++++++ .../cql/cql2elm/elm/ElmEditorTest.java | 37 +++++++++++++++++++ 7 files changed, 122 insertions(+), 19 deletions(-) create mode 100644 Src/java/cql-to-elm/src/main/java/org/cqframework/cql/cql2elm/elm/IElmEdit.java create mode 100644 Src/java/cql-to-elm/src/test/java/org/cqframework/cql/cql2elm/elm/ElmEditTest.java create mode 100644 Src/java/cql-to-elm/src/test/java/org/cqframework/cql/cql2elm/elm/ElmEditorTest.java 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 index 9b4862e5a..a99fbbac6 100644 --- 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 allNonNull(ElmEdit... ts) { + private List 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 index 912b97f16..81212314d 100644 --- 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 tags inside . + // 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 index 28dd8e23d..4ecd00113 100644 --- 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 edits; - private final FunctionalElmVisitor> visitor; + private final List edits; + private final FunctionalElmVisitor visitor; - public ElmEditor(List edits) { + public ElmEditor(List 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 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 new file mode 100644 index 000000000..ee6ba3d5d --- /dev/null +++ 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 index 3671456e1..6b8195e77 100644 --- 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 new file mode 100644 index 000000000..b86a8b878 --- /dev/null +++ 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 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 new file mode 100644 index 000000000..6209127ae --- /dev/null +++ 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); + } +}