hapifhir / hapi-fhir

🔥 HAPI FHIR - Java API for HL7 FHIR Clients and Servers
http://hapifhir.io
Apache License 2.0
2.01k stars 1.31k forks source link

5738: Skip alternate handling if already handled by previous step #5971

Closed stefan-lindstrom closed 4 months ago

stefan-lindstrom commented 4 months ago

Hi,

first time through the code trying to fix an issue, so I hope I haven't violated too many architectural rules. :)

We saw this problem when using extensions on datatypes (using data-absent-reason, with code masked and we had an object with both a primitive and an array-property with masked values. We got this output (example from practitioner):

{
    "resourceType": "Practitioner",
    "id": "1",
    "name": [{
           "_family": {
                    "extension": [{
                            "url": "http://hl7.org/fhir/StructureDefinition/data-absent-reason",
                            "valueString": "masked"
                        }
                    ]
                }
           } ,
            "given": [null],
            "_given": [{
                    "extension": [{
                            "url": "http://hl7.org/fhir/StructureDefinition/data-absent-reason",
                            "valueString": "masked"
                        }
                    ]
                }
            ],
        }
    ]
}

My understanding is that when the object is normally handled, the parser counts alternateNamesSeen, and alternateNames Handled, and when seen > handled, a second pass is done. But instead of only doing un-handled, the second pass will go over all alternates including non-primitive ones where the alternate json.-type need not be an object but could be an array for (any non-primitive property).

Som y idea is to check, and only do the secondary handling if the alternate property in an object does not have a corresponding "real" property.

stefan-lindstrom commented 4 months ago

Tagging @jamesagnew

codecov[bot] commented 4 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 83.45%. Comparing base (497b9f2) to head (bef011d). Report is 74 commits behind head on master.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #5971 +/- ## ============================================ + Coverage 83.39% 83.45% +0.05% - Complexity 26927 27105 +178 ============================================ Files 1681 1692 +11 Lines 103965 104631 +666 Branches 13189 13253 +64 ============================================ + Hits 86702 87315 +613 - Misses 11613 11648 +35 - Partials 5650 5668 +18 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

jamesagnew commented 4 months ago

Fixes #5738

stefan-lindstrom commented 4 months ago

Thanks for the contribution!

HAPI to help. (Sorry, I'll see my self out). Is it OK to merge (once I managed to sort out authorization)?

jamesagnew commented 4 months ago

Yup, I've merged. Don't worry about the 2 failed CI checks.. Those fail on external contributions because of some permission issue we haven't figured out yet. Thanks again!