lantanagroup / FHIR.js

Node.JS library for serializing/deserializing FHIR resources between JS/JSON and XML using various node.js XML libraries
Apache License 2.0
102 stars 29 forks source link

Field validation error for required attributes when choice types #39

Closed jessicamrbr closed 3 years ago

jessicamrbr commented 3 years ago

fhir/parseConformance.js:167

                    const anySliceRequired = structureDefinition.snapshot.element
                        .filter((e) => e.id.includes(elementId) && e.min >= 1)
                        .length > 0;

This code can find other attributes outside the correct context. Example.: Observation.value[x] Observation.otherScope.value[x]

Moreover, and it seeks to validate a cardinality not included in the standard.

image

Removing this validation should resolve.

                    elementId = elementId.substring(0, elementId.length - 3);
                    // const anySliceRequired = structureDefinition.snapshot.element
                    //     .filter((e) => e.id.includes(elementId) && e.min >= 1)
                    //     .length > 0;
                    for (let y in element.type) {
                        let choiceType = element.type[y].code;
                        choiceType = choiceType.substring(0, 1).toUpperCase() + choiceType.substring(1);
                        const choiceElementId = elementId + choiceType;
                        const newProperty = {
                            _name: choiceElementId,
                            _choice: elementId,
                            _type: element.type[y].code,
                            _multiple: element.max !== '1',
                            _required: element.min === 1  //|| anySliceRequired
                        };
                        this.populateValueSet(element, newProperty);
                        parsedStructureDefinition._properties.push(newProperty);
                    }
seanmcilvenna commented 3 years ago

I didn't remove the logic, but made it more complex so that it checks based on the entire id/path, not just e.id.includes('value') (for example).

seanmcilvenna commented 3 years ago

I committed that change and published it as 4.8.2 on NPM

jessicamrbr commented 3 years ago

You must solve the problem. But what is the purpose of making this validation more complex, if by definition the choiceTypes children did not have a cardinality in the structureDefinition? Won't that make the code unnecessarily more complex and less performance?

seanmcilvenna commented 3 years ago

The reason this validation needs to be more complex is because there may be custom profiles that constrain individual choices further. For example, you might have a customized profile that says "Observation.valueBoolean is required".

jessicamrbr commented 3 years ago

It is about this scenario that I commented. Even in a custom profile, the cardinality does not affect each type of a ChoiceType attribute, you can only change the attribute's cardinality ("root").

The types of a ChoiceTypes attribute do not represent sub attributes, but different types of the same attribute.

seanmcilvenna commented 3 years ago

I'll have to study the subject a bit more against the FHIR spec. I understand what you're saying and will take it under consideration for future improvements. For now, though, I think it is ok to keep this issue closed, since the fix (although maybe overly complicated) still addresses your initial problem.

lschmierer commented 3 years ago

Hey, there this issue is on me 🙈

It was introduced in commit https://github.com/lantanagroup/FHIR.js/pull/36/commits/1297cb5e356f10c485c781249a61d2c215711ce0

Actually @jessicamrbr you are not completely right. The spec is not entirely clear on the min cardinality of sliced elements. Take the BMI Profile for example.

bmi1

Observation.value[x] has min cardinality of 0.

bmi2

While the min cardinality of Observation.value[x]:valueQuantity is set to 1 .

In the Profiling Examples the cardinality of the sliced element (net cardinality) is always set correctly. However, as the BMI Profile demonstrates, this behaviour can not allways be assumed.

Thanks @seanmcilvenna for fixing this issue! These changes should propably also added to populateBackboneElement.

It might further make sense to also add this behaviour to the handling of non-choice types (in if (element.type.length === 1 && !elementId.includes('[x]') && !elementId.includes(':'))) as I do not expect all custom profiles to have the sliced elements min cardinalty to be allways set correct.

seanmcilvenna commented 3 years ago

Good discussion. I'll re-open this ticket. I'm not sure when I'll be able to get to these suggestions. Of course, I'd love to see a pull request... :)

jessicamrbr commented 3 years ago

This case and discussion is interesting.

In R4, slicing by type was added, commonly it should be used to treat types in references. It could also be applied to change "must support", "usage notes", "vocabulary bindings", etc. from a Choice Type. But structurally, it is also possible to change the cardinality.

However, this is very risky. Consider a case where:

This structure would say that mandatory you should have two types contradicting the rules https://www.hl7.org/fhir/formats.html#choice

Following the example of https://www.hl7.org/fhir/bmi.profile.json.html

Instead bmi could be done in the following way achieving the same goal.

But considering bmi.profile is considered an officially recognized profile, so my view should not be complete and it makes sense to treat this case where slicing occurs.

Thanks @seanmcilvenna and @lschmierer for the discussion.