Refactor $cql operation processor to be version agnostic#922
Refactor $cql operation processor to be version agnostic#922
Conversation
|
Formatting check succeeded! |
barhodes
force-pushed
the
refactor-cql-processor-2
branch
from
1877799 to
c1708a9
Compare
|
c-schuler
left a comment
c-schuler
left a comment
There was a problem hiding this comment.
This looks really good! A few minor recommendations/comments. A bigger concern is the gaps in testing. The processor is now version agnostic, but it appears as though the tests are R4-only.
| translatedLibrary.getIdentifier() == null | ||
| ? null | ||
| : translatedLibrary.getIdentifier().getId()) | ||
| .withVersion(translatedLibrary.getIdentifier().getVersion()); |
There was a problem hiding this comment.
Null check for withId(), but not for withVersion()?
| IRepository dataRepository, | ||
| IRepository contentRepository, | ||
| IRepository terminologyRepository) { | ||
| repository = proxy(repository, useServerData, dataRepository, contentRepository, terminologyRepository); |
There was a problem hiding this comment.
Hmm. This looks like a preexisting project-wide problem, but we should really be using a local variable here, right? Won't this cause a cascading wrap scenario if a CqlProcessor is reused across multiple evaluate() calls? For example, on construction this.repository would be the "originalRepo", then the first call would be ProxyRepository(originalRepo,...), then the second call would be ProxyRepository(ProxyRepository(originalRepo...)), and so on with subsequent calls. Seems like a lot of potential issues could spring from this pattern (performance, stale endpoint references, etc).
Not saying this needs to be addressed in this PR, but maybe we should create an issue to call this out?
| fhirVersion = libraryEngine.getRepository().fhirContext().getVersion().getVersion(); | ||
| this.libraryEngine = libraryEngine; | ||
| this.subjectId = subjectId; | ||
| if (expression == null && content == null) { |
There was a problem hiding this comment.
Should we check for empty expression string? If expression is "", would it fail downstream?
3 participants
Refactored the processor for the $cql operation to be version agnostic and aligned with the other CR processors. Added support for the prefetchData parameter for both the $cql and Library/$evaluate operations.