ValueSet self reference #1503
ValueSet self reference #1503
Conversation
KaelynJefferson
requested review from
jafeltra,
cmoesel and
mint-thompson
as code owners
There was a problem hiding this comment.
Even though the Jira issue didn't link to it, I do think this resolves #1487. If you add "Fixes #1487" to the PR description, it should automatically close the issue once this PR is merged. Then again, maybe me typing this in the comment will do that too. I guess we'll see! (Edit: Present-time-Julia here to say that this comment did not link the issue to the PR, so you can add it to the description to link it and have it automatically close.)
I also left some comments inline with questions about how the behavior here works. They probably are worth discussing with the team (unless these questions have been answered last week and I just missed it!).
src/export/ValueSetExporter.ts
Outdated
| composeElement.valueSet = composeElement.valueSet.filter(vs => { | ||
| if (vs == valueSet.url) { | ||
| logger.error( | ||
| `Value set with id ${valueSet.id} has component rule with self referencing value set (by id, value set name, or url). Skipping rule.` | ||
| ); | ||
| } | ||
| return vs != valueSet.url; | ||
| }); |
There was a problem hiding this comment.
This is a question of what we want the desired output to be. What you have here might be totally correct, but I just want to double check.
Now that we're filtering the composeElement.valueSet, there's a chance that we're left with no items in composeElement.valueSet. Then on line 187, we push the composeElement object onto valueset.compose.include no matter what the value of composeElement is. Since we could have filtered out all the value sets, we might be pushing on a compose element that doesn't have any value sets, which then gets converted to a null value within an array when we create the FHIR resources. Since I might have explained that poorly, here's an example of what I'm trying to say:
If you have the following value set (and another value set defined for OtherVS)
ValueSet: MaxVS
Id: max-vs
* include codes from valueset MaxVS
* include codes from valueset OtherVS
the first rule will filter out the MaxVS value set because it is self-reference. That then results in the following compose element to be created:
"compose": {
"include": [
null,
{
"valueSet": [
"http://example.org/ValueSet/other-vs"
]
}
]
}
Since we're skipping the rule and telling authors about it, I wasn't expecting a null entry where the filtered value set was removed. Is that what we were expecting?
There was a problem hiding this comment.
After talking through this in person, we decided that the expected outcome of that FSH snippet would be
"compose": {
"include": [
{
"valueSet": [
"http://example.org/ValueSet/other-vs"
]
}
]
}
There was a problem hiding this comment.
If it is "a real pain in the neck" to get rid of the null, don't worry too much because we logged an error.
There was a problem hiding this comment.
Looking into this further with the debugger, I do not see null values added to the compose.include array. The test I added does not show resulting null values in its array either (If it did, there would be 3 null values since we show the 3 different self references in that example).
I see compose.include = null when given only self-referencing values in the value set (but null is not added to the compose.include array) - in this case there is no array.
There was a problem hiding this comment.
Hm I think the case I was describing is a little different from what you described. I agree that the test shows we skip over the self-referenced value set and no null ends up in the compose.include.valueset array it constructs. I think the other example you described where there is no array at all is happens when the only rule on the value set has only a reference to itself (I might be wrong though, so let me know if I misunderstood this case).
The case that I was trying to ask about is if you have two include rules and one has a valid reference to a value set and the other has a self-referenced value set. In that case, because the compose.include array is being created, I'm seeing a null value entered into that array for the self-referenced value set. If you use the example FSH I included and run it on this branch and look at the generated ValueSet for MaxVS, I think you should see the null value. Then again, maybe I'm doing something wrong, so let me know if you can't reproduce that.
Also, we did decide that if "it is a 'real pain in the neck' to get rid of the null", then we just would leave it. So if this is reaching pain in the neck status, let me know.
There was a problem hiding this comment.
Updated with the recent changes. Let me know if this still does not cover this case.
src/export/ValueSetExporter.ts
Outdated
| valueSet.compose.include.push(composeElement); | ||
| if (composeElement.valueSet?.length != 0) { | ||
| valueSet.compose.include.push(composeElement); | ||
| } |
There was a problem hiding this comment.
I think this condition might be a little too restrictive. Consider the following FSH:
ValueSet: SomeVS
* include codes from system http://example.org/SomeCS and valueset SomeVS
In this case, the composeElement.valueSet will be an empty array, so composeElement will not be added to the value set. But, composeElement.system has a value, so I think that composeElement should be added.
Description: This PR addresses when a component rule on a ValueSet attempts to self-reference by id, ValueSet name, or url. For this case, it logs an error and removes the self reference.
Testing Instructions: Verify with unit tests.
Related Issue: Fixes #1487