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
            };