Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix problems when setting values on child elements in primitive arrays #1302

Merged
merged 9 commits into from
Jul 21, 2023

Conversation

mint-thompson
Copy link
Collaborator

Completes task CIMPL-1116.

Child elements of primitives are represented in JSON by using the name of the primitive element prefixed with an underscore. When the primitive element is also an array, create both the regular and underscore-prefixed arrays at the same time. When adding an element to one array, add a corresponding element to the other. Both arrays track slice names for sliced arrays. While this will sometimes result in the creation of arrays that contain no meaningful elements, these arrays are already cleaned up during export.

This PR changes the way soft index resolution works with manualSliceOrdering enabled to make it more accurately fulfill the requirement of "no access to elements with slice names without using the slice name." As a result, while the index values returned from convertSoftIndicesStrict have changed, the resulting instances are the same. A similar change is made in createUsefulSlices, which is used when manualSliceOrdering is enabled.

The useful outcomes of this PR are:

  • The correct number of array elements are created when assigning values to a primitive array and its children.
  • Cardinality requirements that apply to a primitive array and its children are correctly enforced. This has been observed in cases where the primitive array's value child is required on a defined slice, but not required in general.

As there are a lot of potential paths through the new code depending on the order in which primitive elements and their children are assigned, I tried to write tests as well as I could to achieve code coverage. Additionally, there were some places in my initial implementation where code coverage was logically impossible, and the unreachable code was removed. In particular, there are places in createUsefulSlices and setPropertyOnInstance where I think it is logically impossible to reach the code when sliceName is not null. Therefore, these code blocks do not attempt to use sliceName. I would appreciate a careful review of these sections in particular.

No changes appeared in the partial regression set. I'm going to be running a full regression tonight, and I will post the results in this thread.

When checking the existing slices on a primitive array, the slice names
are on the objects within the underscore-prefixed array.
Primitive arrays are represented with two arrays: one that stores the
primitive values, and one that stores child elements. When creating an
element at a given index in one array, also create an element at the
same index in the other. Slice names at a given index should be equal.

When resoving soft indices under manual slice ordering, treat "no slice
name" as its own group of slices. This helps guarantee correct array
indices when assigning child properties of a primitive array.
In places where code coverage reported uncovered code, add tests where
possible. Some uncovered code represented logically impossible
scenarios, so that code is removed.
Copy link
Member

@cmoesel cmoesel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have not fully reviewed all the tests, and I'm not sure when I'll be able to, so I thought I'd at least submit some of my comments/questions for the code...

src/fhirtypes/common.ts Outdated Show resolved Hide resolved
src/fhirtypes/common.ts Outdated Show resolved Hide resolved
src/fhirtypes/common.ts Outdated Show resolved Hide resolved
src/fhirtypes/common.ts Outdated Show resolved Hide resolved
@mint-thompson
Copy link
Collaborator Author

In the full regression set, I've discovered some problems with handling caret rules on a type.profile.extension and type.targetProfile.extension. I don't think this will require a complete overhaul, but some of the suggestions @cmoesel made may help with those scenarios.

A primitive array may start out containing some elements, such as when
working with ElementDefinition.type.profile. Fill up the array so that
each half is the same size.
@mint-thompson
Copy link
Collaborator Author

I've made some updates that resolve problems that occurred when assigning extensions on type.targetProfile. The PACarePlan profile does this, so that's how I caught it. The array fill on creation was something in my original implementation, but I removed it when I couldn't think of a test case to make it important. I'll have to keep extensions on types in mind for the future. These updates also make use of @cmoesel's suggestions for simplifying some of the branching logic.

Copy link
Member

@cmoesel cmoesel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's looking good, but this might be the gift that keeps on giving, as I've left at least one sort of significant thing for you to consider... perhaps it is worth discussing as a team.

test/export/InstanceExporter.test.ts Show resolved Hide resolved
test/export/InstanceExporter.test.ts Show resolved Hide resolved
test/utils/PathUtils.test.ts Outdated Show resolved Hide resolved
Guarantees about not accessing named slices without the slice name only
apply when soft indexing is used. The resulting numeric indices,
therefore, may be spaced out in order to account for the presence of
named slices.
Copy link
Member

@cmoesel cmoesel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it complicated? Yup. Is it working? Yup. Is it working well? For sure!

I just tried this with the original examples from CIMPL-1116, and wow -- they were messed up before; but now they are perfect. This is a good change and I'm glad to get those pretty nasty bugs out of the way. Thanks, @mint-thompson!

Copy link
Collaborator

@jafeltra jafeltra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think all this makes sense. I followed along with the tests, and I think the changes here make sense and don't add too much extra complexity. Nice job wrangling some very tricky code.

src/export/InstanceExporter.ts Show resolved Hide resolved
@mint-thompson mint-thompson merged commit 8ef06be into master Jul 21, 2023
14 checks passed
@mint-thompson mint-thompson deleted the cimpl-1116-primitive-array branch July 21, 2023 12:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants