clinical_quality_language icon indicating copy to clipboard operation
clinical_quality_language copied to clipboard

Dynamic Object Factories for Elm and Annotation

Open vitorpamplona opened this issue 3 years ago • 1 comments

This is a proposal to address https://github.com/DBCG/cql_engine/issues/586 and builds on top of https://github.com/cqframework/clinical_quality_language/pull/760

The idea is to make all ObjectFactories part of the LibraryManager setup and to use that setup throughout the compilation process (including dependent libraries). This would allow the engine to compile directly into executable elm while keeping both layers of ELM separate from one another.

To avoid confusion, it's important to force users of the engine to not use the default LibraryManager constructor for compilation processes (since it creates non-executable elm by default). This could be a major source of runtime inconsistencies and I am not really sure how to solve it. One idea is to break the LibraryManager into 3 classes, remove the default ObjectFactories from the main class and require users to choose a subclass:

public abstract class LibraryManager {
  public LibraryManager(
    ModelManager modelManager, 
    ObjectFactory elmFactory, 
    org.hl7.cql_annotations.r1.ObjectFactory annotationsFactory) {
    //...
  }   
  //...
} 
public class LinkedLibraryManager extends LibraryManager {
    public LinkedLibraryManager(ModelManager modelManager) {
        this(modelManager, new ObjectFactory(), new org.hl7.cql_annotations.r1.ObjectFactory());
    }
}

An then on the Engine:

public class ExecutableLibraryManager extends LibraryManager {
    public ExecutableLibraryManager(ModelManager modelManager) {
        this(modelManager, new ObjectFactoryEx(), new org.hl7.cql_annotations.r1.ObjectFactory());
    }
} 

I am not sure if it is worth breaking backward compatibility to improve clarity in this case.

vitorpamplona avatar Jun 29 '22 16:06 vitorpamplona

@JPercival @brynrhodes I am wondering if I should close this PR or if it's worth merging it as is. I think the ability to set up one final ObjectFactory for the entire stack is a good feature to have. We are just not using it right now

vitorpamplona avatar Jul 11 '22 19:07 vitorpamplona

We're working to merge the ELM graphs in #1165. We'll revisit this after that's stabilized.

JPercival avatar Jun 02 '23 17:06 JPercival