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

Support "position" slicing discriminator #1538

Merged
merged 2 commits into from
Dec 2, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 7 additions & 6 deletions npm-shrinkwrap.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

31 changes: 27 additions & 4 deletions src/fhirtypes/ElementDefinition.ts
Original file line number Diff line number Diff line change
Expand Up @@ -386,13 +386,27 @@ export class ElementDefinition {
validationErrors.push(
this.validateIncludes(slicing.rules, ALLOWED_SLICING_RULES, 'slicing.rules')
);

const isR4 = this.structDef.fhirVersion.startsWith('4.');
slicing.discriminator?.forEach((d, i) => {
const discriminatorPath = `slicing.discriminator[${i}]`;
validationErrors.push(this.validateRequired(d.type, `${discriminatorPath}.type`));
validationErrors.push(
this.validateIncludes(d.type, ALLOWED_DISCRIMINATOR_TYPES, `${discriminatorPath}.type`)
);
if (isR4) {
validationErrors.push(
this.validateIncludes(d.type, ALLOWED_DISCRIMINATOR_TYPES, `${discriminatorPath}.type`)
);
} else {
validationErrors.push(
this.validateIncludes(d.type, ALLOWED_DISCRIMINATOR_TYPES_R5, `${discriminatorPath}.type`)
);
if (d.type === 'position' && slicing.ordered !== true) {
validationErrors.push(
new ValidationError(
'Slicing ordering must be true when a position discriminator is used.',
'slicing.ordered'
)
);
}
Comment on lines +401 to +408
Copy link
Member

Choose a reason for hiding this comment

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

Here's something to think about: If the user uses a position discriminator type, should we just set ordered to true for them? I.e., we know what it should be, so... just do it? Putting the "short" in "shorthand"?

I'm torn on it. I feel like if they don't set the ordered property themself using a caret rule, then it makes sense to set it for them. But if they had set it themself to the wrong value (false), then I'm not sure if silently setting it to true is the right thing to do...

}
validationErrors.push(this.validateRequired(d.path, `${discriminatorPath}.path`));
});
return validationErrors.filter(e => e);
Expand Down Expand Up @@ -3076,6 +3090,15 @@ export type ElementDefinitionSlicingDiscriminator = {
// Cannot constrain ElementDefinitionSlicingDiscriminator to have these values as a type
// since we want to process other string values, but log an error
const ALLOWED_DISCRIMINATOR_TYPES = ['value', 'exists', 'pattern', 'type', 'profile'];
// R5 introduced the 'position' discriminator
const ALLOWED_DISCRIMINATOR_TYPES_R5 = [
'value',
'exists',
'pattern',
'type',
'profile',
'position'
];
Comment on lines +3094 to +3101
Copy link
Member

Choose a reason for hiding this comment

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

I don't have a strong opinion on this, but... it could be:

Suggested change
const ALLOWED_DISCRIMINATOR_TYPES_R5 = [
'value',
'exists',
'pattern',
'type',
'profile',
'position'
];
const ALLOWED_DISCRIMINATOR_TYPES_R5 = [...ALLOWED_DISCRIMINATOR_TYPES, 'position'];


export type ElementDefinitionBase = {
path: string;
Expand Down
64 changes: 64 additions & 0 deletions test/fhirtypes/ElementDefinition.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1112,5 +1112,69 @@ describe('ElementDefinition', () => {
/slicing.discriminator\[0\].type: Invalid value: #foo. Value must be selected from one of the following: #value, #exists, #pattern, #type, #profile/
);
});

it('should be invalid when an element has a position discriminator, ordered is true, and the FHIR version is older than R5', () => {
const clone = valueX.clone(false);
clone.slicing = {
rules: 'open',
ordered: true,
discriminator: [{ path: '$this', type: 'position' }]
};
const validationErrors = clone.validate();
expect(validationErrors).toHaveLength(1);
expect(validationErrors[0].message).toMatch(
/slicing.discriminator\[0\].type: Invalid value: #position. Value must be selected from one of the following: #value, #exists, #pattern, #type, #profile/
);
});
});
});

describe('ElementDefinition R5', () => {
let defs: FHIRDefinitions;
let jsonObservation: any;
let jsonValueX: any;
let observation: StructureDefinition;
let valueX: ElementDefinition;
let fisher: TestFisher;

beforeAll(() => {
defs = new FHIRDefinitions();
loadFromPath(path.join(__dirname, '..', 'testhelpers', 'testdefs'), 'r5-definitions', defs);
fisher = new TestFisher().withFHIR(defs);
// resolve observation once to ensure it is present in defs
observation = fisher.fishForStructureDefinition('Observation');
jsonObservation = defs.fishForFHIR('Observation', Type.Resource);
jsonValueX = jsonObservation.snapshot.element[21];
});

beforeEach(() => {
observation = StructureDefinition.fromJSON(jsonObservation);
valueX = ElementDefinition.fromJSON(jsonValueX);
valueX.structDef = observation;
});

it('should be valid when an element has a position discriminator, ordered is true, and the FHIR version is R5 is newer', () => {
const clone = valueX.clone(false);
clone.slicing = {
rules: 'open',
ordered: true,
discriminator: [{ path: '$this', type: 'position' }]
};
const validationErrors = clone.validate();
expect(validationErrors).toHaveLength(0);
});

it('should be invalid when an element has a position discriminator, ordered is not true, and the FHIR version is R5 or newer', () => {
const clone = valueX.clone(false);
clone.structDef.fhirVersion = '5.0.0';
clone.slicing = {
rules: 'open',
discriminator: [{ path: '$this', type: 'position' }]
};
const validationErrors = clone.validate();
expect(validationErrors).toHaveLength(1);
expect(validationErrors[0].message).toMatch(
/slicing\.ordered: Slicing ordering must be true when a position discriminator is used/
);
});
});
Loading