fix(mappers): keep nested criteria on entry events (CorrelatedCriteria placement)

[watilde]

CorrelatedCriteria is a field of CIRCE's Criteria class, so it belongs inside the domain object — { "ConditionOccurrence": { CorrelatedCriteria } } — not on the wrapper that carries StartWindow/Occurrence for inclusion-rule criteria. The converter read and wrote it on the wrapper in both directions. Inclusion rules happen to have a wrapper, so they looked fine; entry events do not, so every nested criterion on an entry event was dropped on import and consequently missing from every export.

Read it from the domain object (falling back to the wrapper so cohorts saved by the versions that misplaced it keep loading) and write it back there. Across the 1104-cohort phenotype-library fixture set, 101 cohorts carry nested criteria on 415 entry events: all 415 were lost before, none are now.

Two pre-existing gaps remain visible inside nested criteria and are not addressed here: VisitSourceConcept and VisitDetailSourceConcept attributes still do not round-trip (1 occurrence each).

The existing nested-criteria tests asserted the wrapper placement — i.e. the bug — and are corrected to the CIRCE shape.

Fixes #131

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.30%. Comparing base (43b9e1d) to head (c05db0f).

Additional details and impacted files

@@           Coverage Diff            @@
##           develop     #136   +/-   ##
========================================
  Coverage    94.30%   94.30%           
========================================
  Files          448      448           
  Lines        96626    96629    +3     
  Branches     11356    11357    +1     
========================================
+ Hits         91120    91123    +3     
  Misses        5506     5506           

Flag	Coverage Δ
unit	94.30% <100.00%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[chrisknoll]

Great fix, thank you, I just would like to clarify:

It's strange that inclusion rules would have worked fine because Inclusion Rules are effectively the same thing as the CorelatedCriteria field in a Criteria base class.

Here is the Criteria field:
https://github.com/OHDSI/circe-be/blob/master/src/main/java/org/ohdsi/circe/cohortdefinition/Criteria.java#L59

Here is the InclusionRule:
https://github.com/OHDSI/circe-be/blob/master/src/main/java/org/ohdsi/circe/cohortdefinition/InclusionRule.java#L30

The CorrelatedCriteria and "expression' fields of each of those are both CriteriaGroup.

So, my point is that it's not about entry events vs. inclusion rules, rather it is the CriteriaGroups that are contained within each of those should have serialized and deserialized using the same 'CriteriaGroup' mapper.

But, I think you're explaining that InclusionRules were properly converting the CriteriaGroup type (the expression field) while the Criteria types were not handling the CorelatedCriteria field. If so, that makes sense.

I haven't dug very deeply into the WebAPI/Circe JSON mapping between the object models that are defined in the published API and the 'internal representation' that serves to standardize different naming conventions (and also how subclasses are serilaized in collections such as when you ahve a Criteria[] that contains different criteria types).

My concern with the mapping subsystem is that it could potentially introduce bugs like we are seeing here instead of just trying to work with the 'standard API classes'. But this is something I need to discuss with @p-hoffmann , but I think my concerns are real.

[watilde]

You're right — it's not entry-events-vs-inclusion-rules. CorrelatedCriteria is a field of the Criteria base class, so it belongs inside the domain object for every criterion, standalone or wrapped in a CorelatedCriteria inside a CriteriaGroup.

The converter read and wrote it on the wrapper, which CIRCE neither emits nor reads, so nested criteria were dropped on import in both paths. Inclusion rules only looked fine because we wrote and read the same wrong place — self-consistent round-trip, but real CIRCE/ATLAS-2 JSON lost them just the same. In the 1104-cohort phenotype library, CorrelatedCriteria sits inside the domain object 448× on entry events and 104× under inclusion rules, and on the wrapper 0×.

The fix covers both paths already (convertAtlasToEvent/convertEventToAtlas are shared), but only entry events were tested — I've added the inclusion-rule regression; both new tests fail against the pre-fix converter.

And your broader concern lands: this is exactly the failure mode of hand-rolling the mapping layer instead of using the standard API classes.

[chrisknoll]

Thanks for the heads up, and appreciate your thoughts. Regardless of what we're going to do long term with the mapping layer, this fix is needed, so I'll get this merged in. Before I can, could you resolve conflicts on this branch?

[watilde]

Rebased onto develop — conflicts resolved.

#138 had landed the same placement fix, so I dropped the overlapping part. Two things remain: an import
fallback (cohorts saved while ATLAS 3 wrote CorrelatedCriteria as a sibling of the domain object would
otherwise lose their nested criteria on load — #138 reads only the CIRCE position), and the regression
tests, now covering the inclusion-rule path as well as entry events.

[p-hoffmann]

On the mapping-layer concern: I did look into working directly with the standard API classes, and for the frontend I ran into concrete friction. The builder edits reactive state in the Pinia store, and the canonical CIRCE shape isn't a natural fit for that: criteria are discriminated by object key rather than a type field, array entries carry no stable identity for keyed rendering and targeted updates, and several fields are serialization details rather than anything the user edits. As soon as I put that shape into the store and add client ids and editing ergonomics, I effectively have an internal representation again, just an undeclared one. So from where I stand the mapping layer is hard to avoid; the more tractable question seems to be how to make it trustworthy.

That is what #140 attempts. It runs the phenotype-library cohorts through the converter and fails on any field that does not survive the round-trip, with known gaps in a registry that can only shrink. It surfaced some gaps I was not aware of, which are addressed there.

One thing worth flagging back to your concern: not all of them were converter bugs. Some were parity gaps in the model itself, things no mapping code could have round-tripped. So the converter is a real risk and you were right to flag it, but the deeper challenge looks like parity in general, across the model, the converter, and the UI. #140 is meant as the standing guard for that: anything that stops surviving the round-trip, for any reason, fails the build.

As for this PR: the regression tests are definitely worth keeping, so those we can merge. On the wrapper-position fallback, my inclination would be to drop it, since ATLAS 3 is still alpha and not widely used, but I'm open to keeping it if you see value in it.

[chrisknoll]

Ok, thanks for your thoughts. We should discuss further, but probably more efficient next time we meet.

Just a couple things:

1. I think there's a spectrum of impact on where we use the model directly and where we would wrap. So I don't think it's an all-or-nothing, but I would prefer we keep to the WebAPI model where possible just for familiarity. For example, CorrelatedCriteria is referenced as nested. This causes an unnecessary (IMO) shift in terminology when dealing with the UI.

2. I know there's a lot of friction around the discriminator of the polymorphic types (ie: Criteria) to use a wrapper vs. type field. Just to keep it clear here, the difference is:

 "ConditionOccurrence": {
    "CodesetId": 0,
    "Age": {
      "Value": 18,
      "Op": "gt"
    },
    "Gender": [
      {
        "CONCEPT_CODE": "F",
        "CONCEPT_ID": 8532,
        "CONCEPT_NAME": "FEMALE",
        "DOMAIN_ID": "Gender",
        "VOCABULARY_ID": "Gender"
      }
    ]
  }

vs.

{
  "type": "ConditionOcccurrence"
  "CodesetId": 0,
  "Age": {
    "Value": 18,
    "Op": "gt"
  },
  "Gender": [
    {
      "CONCEPT_CODE": "F",
      "CONCEPT_ID": 8532,
      "CONCEPT_NAME": "FEMALE",
      "DOMAIN_ID": "Gender",
      "VOCABULARY_ID": "Gender"
    }
  ]
}

There are several reasons I went with wrapper, but here's my top 2:

1. The content inside the {} for the wrapper is exactly the fields that are in the state of that object. I don't have to ignore a discriminator field when looking at the state of the object. I could have a typescript definition indicating what the valid fields for this object are, and it would match exactly the server side representation found here.

2. It is much harder to 'change' the type of this object. With the wraper, you have a literal field name of an object that is defining the objec type. With a discriminator field all i would have to do with he above is this and it changes it's type:

c.type = "ProcedureOccurrence".

So, I understand the friction in type field vs wrapper, but there was good reasons for the choice. If the difficulty is figuring out the type of the criteria from the wrapper, it's pretty simple:

If it has exacty one property, and that property name is in ("ConditionOccurrence", "ProcedureOccurrence"..etc) then it's a criteria type of the name of that property.

The UI elements that consume that object state just gets passed a reference to what the DiscriminatorField references. Vue.js should be able to auto-wrap that in a reactive. Here's my research on that, and if this could work like this, I'm not sure why we need any wrapping at all (except you mentioned that there was a need to identify a specific node in the object graph and the default object model doesn't make any allowance for that, but this is something we could attempt to work on separately) but here's what I thought we were getting out of Vue.js that is a huge win in my mind:

Vue 3 Reactive JSON Object Model

Vue 3 can take an arbitrary JavaScript object (including one loaded from JSON) and wrap it in a reactive proxy so that changes anywhere in the object graph can be automatically observed.

For example:

import { reactive } from 'vue';

const cohortDefinition = reactive(jsonFromServer);

or:

import { ref } from 'vue';

const cohortDefinition = ref(jsonFromServer);

Once the object is reactive, Vue can track changes to nested properties and arrays:

cohortDefinition.name = "Diabetes";

cohortDefinition.ConceptSets[0].name = "Hypertension";

cohortDefinition.InclusionRules.push(newRule);

cohortDefinition.PrimaryCriteria.CriteriaList[0].CodesetId = 123;

All of these changes can automatically update any Vue components that depend on those values.

---

How Vue Tracks Changes

Vue is not constantly scanning the object looking for changes.

Instead, Vue 3 uses JavaScript Proxy objects.

When a reactive object is created:

const cohort = reactive(json);

Vue creates a proxy around the object. When code accesses properties:

cohort.name

Vue records the dependency:

"This component depends on cohort.name."

Later, when a value changes:

cohort.name = "New Name";

the proxy intercepts the assignment and notifies only the components/effects that previously accessed that property.

This dependency tracking allows Vue to efficiently update the UI without manually managing change notifications.

---

Example with a Deep JSON Structure

This is especially useful for complex JSON models such as Atlas cohort definitions.

Example:

const cohort = reactive({
  id: 123,
  name: "My Cohort",
  conceptSets: [
    {
      id: 1,
      name: "Diabetes"
    }
  ],
  expression: {
    primaryCriteria: {
      criteriaList: []
    },
    inclusionRules: []
  }
});

Even though the structure is deeply nested, Vue can observe changes anywhere:

cohort.expression.primaryCriteria.criteriaList.push(newCriterion);

or:

cohort.conceptSets[0].name = "Hypertension";

No additional event wiring or manual notification code is required.

---

Why This Fits Atlas-Style JSON Models

A cohort definition is already a rich JSON object graph:

• Nested objects
• Arrays of criteria
• Concept sets
• Expressions
• Inclusion rules
• Parameterized settings

Vue allows the application to work directly with that domain model instead of creating a second ViewModel layer that mirrors the same structure.

For example:

const cohort = reactive(await api.loadCohort(id));

The UI can bind directly:

<v-text-field v-model="cohort.name" />

<ConceptSetEditor
    v-for="set in cohort.conceptSets"
    :key="set.id"
    :concept-set="set"
/>

A child component can edit a nested object, and other components using that same object automatically see the updated state.

---

Important Design Implication

Because Vue's reactivity works on the object graph itself, a complex domain object does not necessarily need:

• A duplicate UI model
• Manual change events
• Property-by-property watchers
• Synchronization code between models

The JSON/domain model can become the source of truth, with Vue providing the observation and update mechanism around it.

This is particularly powerful for applications like Atlas where the underlying data model is already a large, nested JSON document representing a domain object.

[p-hoffmann]

Thanks, the Vue writeup is accurate, and it helps pin down the crux. Reactivity solves observation: reactive(jsonFromServer) will track changes anywhere in the graph, no argument there. But observation was never why the mapping layer exists. It exists for the things reactivity does not provide: stable identity for keyed rendering and targeted updates (CIRCE array entries carry none, and the builder's whole job is inserting, reordering and deleting them; index keys give wrong DOM reuse there), typed dispatch over the criteria polymorphism, and draft-vs-saved separation, which mutating the fetched object in place forecloses. Solving any of those means adding client-side structure to the model, which lands us back at a mapping layer by another name.

On the wrapper vs type field: I understand the reasons and they are real on the server side, where Jackson's WRAPPER_OBJECT handling enforces the shape. On the client the trade-offs invert. TypeScript narrows discriminated unions on a type field natively, while the wrapper forces Object.keys(c)[0] dispatch at every render site, and the "exactly one property" invariant is a convention there, not something the code can enforce. Worth noting that the bug this PR fixes was precisely a violation of that invariant: we wrote CorrelatedCriteria as a sibling of the domain object, the wrapper had two properties, and nothing on the client could catch it structurally. A type field cannot be misplaced that way.

One point I will simply concede: you are right about nested vs CorrelatedCriteria. That rename bought us nothing and costs familiarity, so I will align the types and UI code with the CIRCE terminology.

So for the transform surface concern, which I share, my position is that the mapping layer stays declared and guarded rather than eliminated, with #140 as the standing guard in CI. The bigger question of converging the store shape on the CIRCE model itself is worth revisiting once 3.0 is out.