Elements handling during serialization

[Ivanidzo4ka]

Describe the bug
https://www.hl7.org/fhir/search.html#elements claims following:

Servers are not obliged to return just the requested elements. Servers SHOULD always return mandatory elements whether they are requested or not.

Right now if I use FhirJsonSerializer or FhirXmlSerializer and specify elements collection I only get back that collection not including mandatory elements. I probably can expand elements collection by myself, but it doesn't feel right with how serializer is design, considering it already has flag to do that, but it's hidden from public surface.

To Reproduce
Steps to reproduce the behavior:

var observation= new Observation
{
	Id = "observation",
	Status= ObservationStatus.Final,
        Code = new CodeableConcept("A", "B", "C")
};

var serializer = new FhirJsonSerializer();
var jsonText = serializer.SerializeToString(observation, elements: new[] { "id" });

Expected behavior
I would expect string to contain status and code
{"resourceType":"Observation","id":"observation","status":"final","code":{"text":"C"}}
instead I have
{"resourceType":"Observation","id":"observation"}

Screenshots
If applicable, add screenshots to help explain your problem.

Version used:

• FHIR Version: All of them
• Version: AFAIK all of them.

** How to fix:
Change code to :

        public static MaskingNode ForElements(ITypedElement node, string[] _elements) =>
            new MaskingNode(node, new MaskingNodeSettings
            {
                IncludeElements = _elements ?? new string[] { },
                IncludeMandatory = (_elements != null && _elements.Length > 0),
                PreserveBundle = MaskingNodeSettings.PreserveBundleMode.All
            });

I would add IncludeMandatory option to ForElements constructor.

[brianpos]

This is a should, not a MUST or SHALL, so is ok if it doesn't return them.

[marcovisserFurore]

In our implementation of _elements we followed the use case of the client, see also the specifcation http://hl7.org/fhir/search.html#elements:

Only elements that are listed are to be returned. Clients SHOULD list all mandatory and modifier elements in a resource as part of the list of elements.

So, the client is responsible for listing the mandatory elements. But the specification says also:

Servers are not obliged to return just the requested elements. Servers SHOULD always return mandatory elements whether they are requested or not.

This is actually contradictory, so we create an issue for this section at HL7 to make it more clear.

When you want to follow the server case, you cannot do that with the SDK at the moment. So, we see that this is a design failure in the SDK. But how to solve it?

When you add automatically the mandatory fields when using _elements, then existing users get a larger set in return, which is not backward compatible. We can solve this by adding a flag to the ForElements method to enable the new functionality, or.. we can add a flag to disable the new functionality and put the default to adding mandatory elements.

What is your opinion @Ivanidzo4ka and @brianpos ?

[Ivanidzo4ka]

I would personally root for optional flag in serializer.Serialize with default behavior set to do nothing (so it wouldn't break current code base, since it's contradictory question). That would allow to return mandatory elements if flag specified.

public void Serialize(Base instance, JsonWriter writer, SummaryType summary = SummaryType.False, string[] elements = null, ElementsBehavior elementsBehavior = ElementsBehavior.DoNothing);

public enum ElementsBehavior 
{
DoNothing=0,
AddMandatory=1,
}

or

public void Serialize(Base instance, JsonWriter writer, SummaryType summary = SummaryType.False, string[] elements = null, bool addMandatoryToElements=false);

Optional flag wouldn't force people to update their codebase, I have suspicion what not many users care about that behavior since I'm the first one who created issue about it, so it would be unfair to force people to update their codebase to provide value in function.

I would also argue against setting default to add mandatory elements, that would make stealthy change for users, and since this topic is unclear (see contradictory question) it doesn't sound like best way to handle it. You can argue what change would be described in docs and release, but how many people actually read them if your unit tests works fine after you bump nuget version? :D

[mmsmits]

Just filed the change request to provide clearer wording in FHIR spec:
https://jira.hl7.org/browse/FHIR-31655

[marcovisserFurore]

We plan to include a new boolean option IncludeMandatoryToElements in the SerializerSettings, so we don't have to change the signature of the methods. We will maintain the current behavior, so the option is set to false by default.

[Ivanidzo4ka]

Thank you for adding this flag to Serializer.
I've run it on few different resource types, and it's seems it doesn't work for CodeableConcept fields.

For example Observation from my initial request, or DiagnosticReport with code field.
code | Σ | 1..1 | CodeableConcept

1..1 seems like required for me, but if I serialize Observation I don't get code back. I get back status field, tho.

[ewoutkramer]

Mmmm.... status is a summary field, wonder whether we mixed that up.

[Ivanidzo4ka]

Code is also summary field https://hl7.org/fhir/observation-definitions.html#Observation.code

From my glance debugging it looks like for CodeableConcept doesn't have value
https://github.com/FirelyTeam/firely-net-common/blob/07fe5aa46ca30e3629e2d329e95b6d5fc4f559b7/src/Hl7.Fhir.Serialization/FhirJsonBuilder.cs#L182

while status does

I've just change observation in IncludeMandatoryInElementsSummaryTest to be

   Observation obs = new()
            {
                Status = ObservationStatus.Final,
                Code = new CodeableConcept("dsd", "dds"),
                Issued = DateTimeOffset.Now
            };

[marcovisserFurore]

The following unit test shows that code is not in the summary, while code is a mandatory attribute.

        public void IncludeMandatoryInElementsSummaryTest()
        {
            Observation obs = new()
            {
                Status = ObservationStatus.Final,
                Issued = DateTimeOffset.Now,
                Code = new CodeableConcept("system", "code"),
                BodySite = new CodeableConcept("system1", "code")
            };
            // default behavior
            var json = new FhirJsonSerializer().SerializeToDocument(obs, elements: new[] { "issued" }) as IDictionary<string, JToken>;

            json.Keys.Should().BeEquivalentTo("resourceType", "issued");

            // Adding mandatory elements to the set of elements
            json = new FhirJsonSerializer(new SerializerSettings() { IncludeMandatoryInElementsSummary = true })
                .SerializeToDocument(obs, elements: new[] { "issued" }) as IDictionary<string, JToken>;

            json.Keys.Should().BeEquivalentTo("resourceType", "issued", "status", "code");
        }

In this case the attribute code is of type CodeableConcept. And all the children of CodeableConcept are not mandatory. For example display and coding are not mandatory. That's why our serializer is not adding the CodeableConcept as a whole to the summary result. But is that correct?

I was wondering how other sorts of summary types handle this in our serializer. For example _summary=text. This type of summary should return only the "text" element, the id element, the meta element, and only top-level mandatory elements. code is a top-level mandatory element, so let's see what happens in an unit test:

        [TestMethod]
        public void IncludeSummaryTextTest()
        {
            Observation obs = new()
            {
                Id = "obs-1",
                Status = ObservationStatus.Final,
                Issued = DateTimeOffset.Now,
                Code = new CodeableConcept("system", "code"),
                BodySite = new CodeableConcept("system1", "code")
            };
       
            var json = new FhirJsonSerializer().SerializeToDocument(obs, Fhir.Rest.SummaryType.Text) as IDictionary<string, JToken>;
            json.Keys.Should().BeEquivalentTo("resourceType", "id", "meta", "status", "code");
        }

This fails for the same reason described above: fields of CodeableConcepts are not mandatory.

How do others servers implement this? I did some tests at:

• Hapi: http://hapi.fhir.org/baseDstu3
• Grahame Grieve's server: http://test.fhir.org/r3

Both the servers return the mandatory element code.

So I tend to say that our serializer is acting false here. Of course we can correct this, but that will change the behavior of several products. How to deal with that? What would be your opinion @Ivanidzo4ka ?

[brianpos]

The issue here is how far done into children does the call go?
the top level (CodeableConcept) is mandatory, but no children are - so what properties do you output?
All of them for the codeableconcept?

[marcovisserFurore]

Exactly @brianpos. How far do we go? Also the extensions of CodeableConcept, etcetera..?

[ewoutkramer]

Who's to decide on this one? Will we just look at the other servers, sort of reverse-engineer the logic and copy that? Or is this a FHIR-I discussion?

[marcovisserFurore]

I've started a discussion on chat.fhir.org

[marcovisserFurore]

I also created a ticket on HL7.org: https://jira.hl7.org/browse/FHIR-33002

[brianpos]

Might also want to check in with Gino Canessa, as he's refining the other JSON serializer that I think you'd likely be using in the MS fhir server too... (once he's done there)

[ewoutkramer]

First off, it seems our issue on including mandatory/modifier elements for _elements was a duplicate: https://jira.hl7.org/browse/FHIR-27812 / https://jira.hl7.org/browse/FHIR-31655.

The verdict is: "we should **align the advice for clients and servers (e.g., by advising both to pay attention to mandatory elements and modifier elements)." (2020-06-22)

To me, "pay attention to" means, that they servers should return mandatory and modifier elements. For the new "4.0" serializers, we should then ENABLE it by default (in contrast to the old serializers), but make it easy to disable - and mention this in the documentation of course.

And what about the children of mandatory/modifier elements? As Marco says, I think it's wrong for our serializer to leave them out. I intended for the serializer to include all children of mandatory top-level elements but that does not seem to work correctly. Now I have looked at the in summary part of the discussion (see below), the other alternative is not include all children of mandatory/modifier elements, but just the in summary ones.

We could make it work like this:

• Include the elements in the _elements, and all their children.
• Include the modifier+mandatory elements, and the summary of their children.

This way, you get what explicitly listed in _elements + a sensible minimum set of the stuff you did not ask for (but will get returned anyway).

HAPI (e.g. http://hapi.fhir.org/baseR4/Observation?_id=1178591&_elements=subject) does currently not include the mandatory top-level elements.

[ewoutkramer]

What about _summary? First _summary = true

http://hl7.org/fhir/elementdefinition-definitions.html#ElementDefinition.isSummary

(...) When a request is made with _summary=true, serailisers only include elements marked as 'isSummary = true'. Other than Attachment.data, all data type properties are included in the summary form. In resource and data type definitions, if an element is at the root or has a parent that is 'mustSupport' and the minimum cardinality is 1 or the element is a modifier, it must be marked as isSummary=true.

So, that's almost clear, but incomplete. The spec has indeed made sure that all datatypes have elements marked "in summary" (with exceptions), but see https://jira.hl7.org/browse/FHIR-33148. Most importantly, it does not mention how to handle backbones.

I don't think we have to write any additional logic on how to deal with mandatory/modifier elements as the spec itself it responsible for defining a sensible summary set. The only addition is Element.id, we agreed to add it to the summary, even if it is not marked as "in summary".

[ewoutkramer]

Then, _summary = text. The spec says:

"Return only the "text" element, the 'id' element, the 'meta' element, and only top-level mandatory elements"

After the discussion on _element, is seems that this kind of summary should also be "top-level mandatory and modifier elements". And apply the same logic as for _element (including the child data too), indeed @GinoCanessa has asked for clarification for modifier elements in the comments on https://jira.hl7.org/browse/FHIR-33002.

_summary=data. Simple, everything but the narrative text, no problem here.

[ewoutkramer]

_summary = count does not need any services from the serializer: just construct a Bundle with only the count set, and no entries.

Idem for _summary = false.

[ewoutkramer]

@marcovisserFurore @mmsmits The issues referred to in this issue have been discussed during the last WGM, we need to process the outcomes.

[mmsmits]

The resolution of https://jira.hl7.org/browse/FHIR-33002 is that only top level mandatory elements need to be returned, including the children that ensure that the minimal payload is still valid. Our newer PocoSerializers ensure this behavior. The older ITypedElement ones, include all children of the top-level mandatory items. Both are valid, since the spec also states: "servers MAY omit elements within these sub-trees as long as they ensure that the payload is valid)."

The spec doesn't says anything about including isModifier elements, this should be a effectuated using "inSummary", modifier elements that aren't specified to be included in the summary are therefor not serialized.