arkhn / fhir-river

Live ETL pipeline to standardize Health Data into FHIR.
Apache License 2.0
42 stars 4 forks source link

[Fix] Node pending state when choice is attribute ancestor #721

Closed BPierrick closed 2 years ago

BPierrick commented 2 years ago

Fixes

Definition of Done

simonvadee commented 2 years ago

I don't understand the fix sorry, can you explain a bit more ? maybe a small unit test would have helped me to understand

BPierrick commented 2 years ago

I don't understand the fix sorry, can you explain a bit more ? maybe a small unit test would have helped me to understand

The problem was not related to the text length, but to the useIsNodePending algorithm.

image

simonvadee commented 2 years ago

ok thanks for the clarification. I have one question though: don't you think that multiple attributes can start with the same string without sharing the same ancestor ? Sorry I don't have a real world example but I can make up one:

BPierrick commented 2 years ago

This example wouldn't raise any issue in my opinion. Moreover I'm not sure I completely got it because Observation.value[x] and Observation.valueAbsentReason do share a common ancestor, which is Observation.value[x] right..?

This one though would be a problem :

I don't really identify any other edge case. And I don't think this one is likely to happen..?

simonvadee commented 2 years ago

no I was talking of the case where Observation.value[x] and Observation.valueAbsentReason would not share a common ancestor (be 2 different attributes) --> the same case as your example. I don't know if this is likely to happen but I would say it's possible

BPierrick commented 2 years ago

Well yes, in that case if Observation.valueAbsentReason is not a choice of Observation.value[x], then this would lead to a bug.

I think either we consider this algorithm and we suppose this would never happen. Or we would need to perform a tree search to effectively check if the nodes are common ancestors, but this solution would be more expensive.