Skip to content

Commit

Permalink
Fix problems when setting values on child elements in primitive arrays (
Browse files Browse the repository at this point in the history 
#1302)

* Don't create extra elements on primitive arrays

When checking the existing slices on a primitive array, the slice names
are on the objects within the underscore-prefixed array.

* Maintain parallel arrays for primitives

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.

* Code cleanup and additional tests

In places where code coverage reported uncovered code, add tests where
possible. Some uncovered code represented logically impossible
scenarios, so that code is removed.

* Fill up new array when creating other half of primitive array

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.

* Refactor to eliminate "underscored" helper variable

* Check for empty error log at end of slice tests

* Revert changes to strict soft index resolution

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.
  • Loading branch information
@mint-thompson
mint-thompson authored Jul 21, 2023
1 parent 7d9cbc8 commit 8ef06be
Show file tree
Hide file tree
Showing 7 changed files with 843 additions and 92 deletions.
11 changes: 9 additions & 2 deletions src/export/InstanceExporter.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -480,10 +480,17 @@ export class InstanceExporter implements Fishable {
// 2 - The child element is n..m, but it has k < n elements
// there's a special exception for the "value" child of a primitive,
// since the actual value may be present on the parent primitive element.
// if the parent primitive is an object, the value will be in the "assignedValue" attribute.
// otherwise, the value is the parent primitive itself.
if (
(child.min > 0 &&
instanceChild == null &&
!(child.id.endsWith('.value') && parentPrimitive != null)) ||
!(
child.id.endsWith('.value') &&
(typeof parentPrimitive === 'object'
? parentPrimitive?.assignedValue
: parentPrimitive) != null
)) ||
(Array.isArray(instanceChild) && instanceChild.length < child.min)
) {
// Can't point to any specific rule, so give sourceInfo of entire instance
Expand Down Expand Up @@ -608,7 +615,7 @@ export class InstanceExporter implements Fishable {
' See: https://hl7.org/fhir/uv/shorthand/reference.html#sliced-array-paths\n' +
` Path: ${slicingEl.path}\n` +
` Slice: ${matchedNames.join(' or ')}\n` +
` Value: ${valueString}}`,
` Value: ${valueString}`,
fshDef.sourceInfo
);
} else {
Expand Down
139 changes: 103 additions & 36 deletions src/fhirtypes/common.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -101,16 +101,22 @@ export function createUsefulSlices(

// If this is a primitive and the path continues to a nested element of the primitive,
// then we need to look at the special property that starts with _ instead.
const key =
pathPart.primitive && i < pathParts.length - 1 ? `_${pathPart.base}` : pathPart.base;

let key: string;
if (pathPart.primitive && i < pathParts.length - 1) {
key = `_${pathPart.base}`;
} else {
key = pathPart.base;
}
const ruleIndex = getArrayIndex(pathPart);
let effectiveIndex = ruleIndex;
let sliceName: string;
if (ruleIndex != null) {
// If the array doesn't exist, create it
if (current[key] == null) {
current[key] = [];
if (pathPart.primitive) {
current[pathPart.base] ??= [];
current[`_${pathPart.base}`] ??= [];
} else {
current[key] ??= [];
}
sliceName = pathPart.brackets ? getSliceName(pathPart) : null;
if (sliceName) {
Expand Down Expand Up @@ -145,15 +151,15 @@ export function createUsefulSlices(
* So we should put the rule at the end of the component, which is effectiveIndex = 3
*/
if (ruleIndex >= sliceIndices.length) {
effectiveIndex = ruleIndex - sliceIndices.length + current[key].length;
effectiveIndex = ruleIndex - sliceIndices.length + current[pathPart.base].length;
} else {
effectiveIndex = sliceIndices[ruleIndex];
}
} else {
// This is an array entry that does not have a named slice (so a typical numeric index)
knownSlices.set(
currentPath,
Math.max(ruleIndex + 1, knownSlices.get(currentPath) ?? 0)
Math.max(effectiveIndex + 1, knownSlices.get(currentPath) ?? 0)
);
}
if (pathPart.brackets != null) {
Expand All @@ -170,15 +176,35 @@ export function createUsefulSlices(
j === effectiveIndex &&
current[key][effectiveIndex] == null
) {
current[key][effectiveIndex] = {};
if (pathPart.primitive) {
current[pathPart.base][effectiveIndex] = {};
current[`_${pathPart.base}`][effectiveIndex] = {};
} else {
current[key][effectiveIndex] = {};
}
} else if (j >= current[key].length) {
if (sliceName) {
// _sliceName is used to later differentiate which slice an element represents
current[key].push({ _sliceName: sliceName });
if (pathPart.primitive) {
current[pathPart.base].push({ _sliceName: sliceName });
current[`_${pathPart.base}`].push({ _sliceName: sliceName });
} else {
current[key].push({ _sliceName: sliceName });
}
} else if (j === effectiveIndex) {
current[key].push({});
if (pathPart.primitive) {
current[pathPart.base].push({});
current[`_${pathPart.base}`].push({});
} else {
current[key].push({});
}
} else {
current[key].push(null);
if (pathPart.primitive) {
current[pathPart.base].push(null);
current[`_${pathPart.base}`].push(null);
} else {
current[key].push(null);
}
}
}
}
Expand Down Expand Up @@ -402,8 +428,23 @@ export function setImpliedPropertiesOnInstance(
}
// check the children for instance of this element
const children = currentElement.def.children(true);
// special handling: if our current element has no slice name, we need guarantee the defined minimum
// this is the only place where we do this, in order to accomodate cases where some named slices already exist
let existingSliceCount = 0;
if (finalMin < currentElement.def.min && currentElement.def.sliceName == null) {
// our final min was lowered by slices, so add that to our indices.
const slicePaths = currentElement.def
.getSlices()
.map(el => `${tracePath}[${el.sliceName}]`);
slicePaths.forEach(slicePath => {
if (knownSlices.has(slicePath)) {
existingSliceCount += knownSlices.get(slicePath);
}
});
}
for (let idx = 0; idx < finalMin; idx++) {
const newHistory = traceParts.slice(-1)[0] + (idx > 0 ? `[${idx}]` : '');
const effectiveIdx = idx + existingSliceCount;
const newHistory = traceParts.slice(-1)[0] + (effectiveIdx > 0 ? `[${effectiveIdx}]` : '');
elementsToCheck.push(
...children.map(
child =>
Expand Down Expand Up @@ -502,7 +543,7 @@ export function setImpliedPropertiesOnInstance(
}
sortedRulePaths.forEach(path => {
const { pathParts } = instanceOfStructureDefinition.validateValueAtPath(path, null, fisher);
setPropertyOnInstance(instanceDef, pathParts, sdRuleMap.get(path), fisher, manualSliceOrdering);
setPropertyOnInstance(instanceDef, pathParts, sdRuleMap.get(path), fisher);
});
}

Expand Down Expand Up @@ -544,8 +585,7 @@ export function setPropertyOnInstance(
instance: StructureDefinition | ElementDefinition | InstanceDefinition | ValueSet | CodeSystem,
pathParts: PathPart[],
assignedValue: any,
fisher: Fishable,
manualSliceOrdering = false
fisher: Fishable
): void {
if (assignedValue != null) {
// If we can assign the value on the StructureDefinition StructureDefinition, then we can set the
Expand All @@ -554,14 +594,33 @@ export function setPropertyOnInstance(
for (const [i, pathPart] of pathParts.entries()) {
// When a primitive has child elements, a _ is appended to the name of the primitive
// According to https://www.hl7.org/fhir/json.html#primitive
const key =
pathPart.primitive && i < pathParts.length - 1 ? `_${pathPart.base}` : pathPart.base;
let key: string;
if (pathPart.primitive && i < pathParts.length - 1) {
key = `_${pathPart.base}`;
} else {
key = pathPart.base;
}
// If this part of the path indexes into an array, the index will be the last bracket
let index = getArrayIndex(pathPart);
let sliceName: string;
if (index != null) {
// If the array doesn't exist, create it
if (current[key] == null) current[key] = [];
if (pathPart.primitive) {
// we may need to create or update one or both arrays
if (current[pathPart.base] == null) {
current[pathPart.base] =
current[`_${pathPart.base}`]?.map((x: any) =>
x?._sliceName != null ? { _sliceName: x._sliceName } : null
) ?? [];
}
if (current[`_${pathPart.base}`] == null) {
current[`_${pathPart.base}`] = current[pathPart.base].map((x: any) =>
x?._sliceName != null ? { _sliceName: x._sliceName } : null
);
}
} else if (current[key] == null) {
// if the array doesn't exist, create it
current[key] = [];
}
sliceName = getSliceName(pathPart);
if (sliceName) {
const sliceIndices: number[] = [];
Expand All @@ -581,33 +640,41 @@ export function setPropertyOnInstance(
} else {
index = sliceIndices[index];
}
} else if (manualSliceOrdering) {
const sliceIndices: number[] = [];
current[pathPart.base]?.forEach((el: any, i: number) => {
if (el?._sliceName == null) {
sliceIndices.push(i);
}
});
// Convert the index in terms of the slice to the corresponding index in the overall array
if (index >= sliceIndices.length) {
index = index - sliceIndices.length + current[key].length;
} else {
index = sliceIndices[index];
}
}
// If the index doesn't exist in the array, add it and lesser indices
// Empty elements should be null, not undefined, according to https://www.hl7.org/fhir/json.html#primitive
for (let j = 0; j <= index; j++) {
if (j < current[key].length && j === index && current[key][index] == null) {
current[key][index] = {};
if (pathPart.primitive) {
// a value may already exist on one of the arrays, so only assign an empty object if it is nullish
current[pathPart.base][index] ??= {};
current[`_${pathPart.base}`][index] ??= {};
} else {
current[key][index] = {};
}
} else if (j >= current[key].length) {
if (sliceName) {
// _sliceName is used to later differentiate which slice an element represents
current[key].push({ _sliceName: sliceName });
if (pathPart.primitive) {
current[pathPart.base].push({ _sliceName: sliceName });
current[`_${pathPart.base}`].push({ _sliceName: sliceName });
} else {
current[key].push({ _sliceName: sliceName });
}
} else if (j === index) {
current[key].push({});
if (pathPart.primitive) {
current[pathPart.base].push({});
current[`_${pathPart.base}`].push({});
} else {
current[key].push({});
}
} else {
current[key].push(null);
if (pathPart.primitive) {
current[pathPart.base].push(null);
current[`_${pathPart.base}`].push(null);
} else {
current[key].push(null);
}
}
}
}
Expand Down
9 changes: 8 additions & 1 deletion src/utils/Processing.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -474,8 +474,15 @@ export function checkNullValuesOnArray(resource: any, parentName = '', priorPath
}
if (Array.isArray(property)) {
const nullIndexes: number[] = [];
const hasUnderscoreArray = Array.isArray(resource[`_${propertyKey}`]);
property.forEach((element: any, index: number) => {
if (element === null) nullIndexes.push(index);
// if property is a primitive array, also check the corresponding index in the underscore property
if (
element === null &&
(!hasUnderscoreArray || resource[`_${propertyKey}`][index] == null)
) {
nullIndexes.push(index);
}
if (isPlainObject(element)) {
// If we encounter an object property, we'll want to check its properties as well
checkNullValuesOnArray(element, resourceName, `${currentPath}[${index}]`);
Expand Down
Loading

0 comments on commit 8ef06be

Please sign in to comment.