Altinn / altinn-studio

Next generation open source Altinn platform and applications.
https://docs.altinn.studio
BSD 3-Clause "New" or "Revised" License
115 stars 70 forks source link

Value types with [XmlText] attributes are not nullable in generated data model #12674

Closed ivarne closed 6 months ago

ivarne commented 6 months ago

Description of the bug

As stated in JsonMetadataToCsharpConverter.cs, the C# xml library does not (and never will) support nullable value types for properties that use the [XmlText] attribute.

https://github.com/Altinn/altinn-studio/blob/6369a7b7af88030729871c6c9a2aa3a8fb4ef685/backend/src/DataModeling/Converter/Csharp/JsonMetadataToCsharpConverter.cs#L161

This is a problem for apps, because they typically want to differentiate between someone having 0 in a decimal field and someone wanting to have it empty, after it has previously held a valid value. https://github.com/Altinn/app-lib-dotnet/issues/552

I think I have identified three solutions

  1. Apps backend does lots of trickery (including a custom JsonPatch implementation for backend) to ensure that when frontend sets an XmlText property of a value type to [empty], it sets the parent property to null instead.
  2. Apps frontend checks if the json schema has @xsdText, and sets the parent object to null if the field should be empty.
  3. Altinn Studio generates models where the value type is actually nullable through a trick where two properties are combined.
[XmlRoot(ElementName = "model")]
public class YttersteObjekt
{
    [XmlElement("aarets", Order = 1)]
    [JsonPropertyName("aarets")]
    public NullableDecimalMedORID? DecimalMedOrid { get; set; }

    public bool ShouldSerializeDecimalMedOrid()
        => DecimalMedOrid?.valueNullable != null;
}

public class NullableDecimalMedORID
{
    [XmlText]
    [JsonIgnore]
    public decimal value
    {
        get => valueNullable ?? default;
        set
        {
            this.valueNullable = value;
        }
    }

    [XmlIgnore]
    [JsonPropertyName("value")]
    public decimal? valueNullable { get; set; }

    [XmlAttribute("orid")]
    [JsonPropertyName("orid")]
    [BindNever]
    public string orid => "30320";
}

For the third option it would be nice if we could add ShouldSerialize* method to the parent object so that the whole tag gets removed when the value is null https://github.com/Altinn/app-lib-dotnet/pull/590

Steps To Reproduce

  1. Type some number into a field with the [XmlText] attribute
  2. Blank the field
  3. See that a 0 value appears because the field is not nullable.

Additional Information

No response

mirkoSekulic commented 6 months ago

@ivarne In my opinion, option 2, which you mentioned, would be the best solution to this issue. However, I’m not sure about the amount of work required here.

I'm a bit skeptical if the third option is a good approach here. It's more of a hack. From data model perspective, it is possible to leave the parent field as null, which results in it not being serialized, or to assign an object with the value field defined. In essence, there are two possible outcomes.

Introducing a third option would add a new “state” where the value field is defined as null, but the end result remains unchanged. Either the object will be serialized with a defined value, or it will not be serialized at all. This simply introduces additional complexity to the data model.

Is there another use case in the apps where you need to completely clear a parent object or this is the only one? It seems like a legitimate use case.

One more note. Legacy json shcemas produced with old datamodeling tool don't have the @xsdText keyword, but those json schemas are not compatible with new datamodeling, so it's not possible to "generate" the datamodel from those schemas. In order to generate the new model, the xsd need to be re-uploaded which will produce a new json schema with @xsdText keyword defined.

ivarne commented 6 months ago

It might not have come across perfectly, but the root here is two issues, so I repeat them now for clarity.

  1. [XmlText] does not support nullable for value types. We need this for the app to not show 0 when you delete a number. This has two solutions:
    1. We must somehow detect that an operation attempts to set an [XmlText] value type property to null, and do some magic to ensure that the parent object is set to null instead. This will need to happen before deserialising into the C# model, because after that you'll only see 0 which is a valid value for the field. This can be done in backend or frontend. Errors in this magic code might cause data loss.
    2. We must find a way to get around the issue and allow Json to speak to a nullable version of the field, while xml speaks to a non nullable version.
  2. When you set the value of an [XmlText] property to null (when it is possible with a reference type like string), it does not really make sense to render the parent object if it only has fixed attributes. This leaks information about whether the user at some point had a value in a field that is now empty.

Is there another use case in the apps where you need to completely clear a parent object or this is the only one? It seems like a legitimate use case.

I don't think issue 2 is important to fix, but I bring it up in case a solution presents it self while working this issue. It simplifies handling of received data if we can transform data to a canonical form where the parent is null if all children are null.

I'm a bit skeptical if the third option is a good approach here. It's more of a hack.

I'd rather say the other solutions are hacks, because they work around the issue with extra code to handle this known structure, instead of tackling it in the serialisation libraries that causes the issue. The trouble with solution 1. and 2. is not only the time required to implement them, but the complexity of the resulting code running in a place where I really would like there to be fewer branches and conditions. The missing @xsdText keyword further complicates matters, because frontend can't reliably detect when the other properties of the class.

I updated the suggestion for the generated code with an improved version that just uses decimal? and decimal and avoids the issue of working with strings.

Introducing a third option would add a new “state” where the value field is defined as null, but the end result remains unchanged. Either the object will be serialized with a defined value, or it will not be serialized at all. This simply introduces additional complexity to the data model.

I'm not sure I understand what you talk about. My suggestion does not introduce more ambiguity that isn't there for reference types already.