why optional changes behaviour for null values

[ddcovery]

@optional attribute, thought for managing "not present" json values (undefined ones) is changing how "null" value is treated:

I have an struct that can represent, internally, Null, Undefined or Value.
This is a simplified version

struct Null {  static _ = Null();  }
struct Undefined { static _ = Undefined(); }

struct DVal!T {
   private ValKind value_kind = ValKind.is_undefined;
   private T[] value = [];
   this(T v){
      static if (isAssignable!(T, typeof(null)))
      if (v is null)
      {
        this.value_kind = ValKind.is_null;
        return;
      }
      value_kind = ValKind.is_value;
      this.value = [v];
   }
   this(Null n) {
      value_kind = ValKind.is_null;
   }
   this(Undefined n){
      value_kind = ValKind.is_undefined;
   }
   @property bool isNull(){ 
     return value_kind == ValKind.is_null;
   }
   @property bool isUndefined(){ 
     return value_kind == ValKind.is_undefined;
   }
   static DVal!T fromRepresentation(Json json) @trusted
   {
      if (json.type == Json.Type.undefined)
         return DVal!T(Undefined._);
      else if (json.type == Json.Type.null_)
        return DVal!T(Null._);
      else
        return DVal!T(deserializeJson!T(json));
  }
  Json toRepresentation() @trusted const
    {
      switch (this.value_kind)
      {
      case ValKind.is_undefined:
        return Json.undefined;
      case ValKind.is_null:
        return Json(null);
      default:
        return serializeToJson(this.value[0]);
      }
    }

}

I create a simple struct to validate how null is seriallized/deserialized

unittest
{
  static struct Person{
    DVal!string name;
  }

  Json json = Json(["name":Json(null)]);
  Person p = json.deserializeJson!Person;
  assert(p.name.isNull);
  assert(p.serializeToJson.to!string == "{\"name\":null}");
}

Then, I add the @optional attribute to manage cases where a property is not present in the json (it is the real reason to use the DVal). This cases are deserialized properly.

unittest
{
  static struct Person{
    @optional
    DVal!string name;
  }

  Json json = Json.emptyObject;
  Person p = json.deserializeJson!(Person);
  assert(p.name.isUndefined);
  assert(p.serializeToJson.to!string == "{}");
}

The problem appears when "name" is present on JSON and its value is null . The "toRepresentation" method is not called

unittest
{
  static struct Person{
    @Optional
    DVal!string name;
  }

  Json json = Json(["name":Json(null)]);
  Person p = json.deserializeJson!Person;
   // This fails, because p.name.isUndefined
  assert(p.name.isNull);
  // This fails because it has been serialized as "{}"
  assert(p.serializeToJson.to!string == "{\"name\":null}"); 
}

@optional is perfect for managing cases where a property is not present on a JSON, but - why it changes behaviour of present properties? Why null is treated in a different way when @optional is present (Null is a valid value, not an undefined one).

[schveiguy]

This is because it treats a null as not present in the case of @optional being attached:

vibe.d/data/vibe/data/serialization.d

Line 911 in 7161dc1

static if (hasPolicyAttribute!(OptionalAttribute, Policy, TypeTuple!(__traits(getMember, T, mname))[0]))

I think this is an incorrect design decision, but I'm not sure if it's changeable. Though, it might be possible to say if the type has a construction mechanism that accepts either null or a Json type, it should use that instead.

[ddcovery]

Then, the only solution to manage undefined/null as "not the same" is to avoid @optional and coding manually the Json conversion... a little annoying :-)

struct Person {
   DVal!string name;

   static Person fromRepresentation(const Json json)
   {
      Person result = {
          name: json["name"].deserializeJson!(DVal!string)
      };
      return result;
   }
   Json toRepresentation() const =>
      Json([
         "name": name.serializeToJson
      ]);
}

[s-ludwig]

I kind of missed its introduction to the code base, but it looks like using Nullable!Json toRepresentation and @embedNullable instead of @optional might be a way around this.

[ddcovery]

I will try to summarize:

I have an struct DVal!T that represents de algebraic type Undefined | Null | T. This struct implements fromRepresentation and toRepresentation.

Example:
struct Person{ DVal!string name; DVal!string surname; DVal!int tallness; }

When calling deserializeJson!Person there is 3 possible cases for each member:

Json(value) -> fromRepresentation is called
Json(null) -> fromRepresentation is called
Json.undefined -> Deserialization fails... @optional is required for missing property management

After adding @optional attribute to all three members:

Json(value) -> fromRepresentation is called
Json(null) -> fromRepresentation is not called. Default constructor/init is used instead
Json.undefined -> fromRepresentation is not called. Default constructor/init is used instead

- Why Json(null) is treated differently when @optional is present? it is a valid json value and it can be deserialized the same way it was without de @optional attribute.

Conclusion: It's impossible to implement an algebraic Undefined | Null | T type based on vibe-d Json deserialization.
Well, it is possible: implementing a custom fromRepresentation to the whole Person struct

[s-ludwig]

What I meat is something like this:

struct DNVal(T) {
    Kind kind;
    T value;
    Json toJson() const { return kind == Kind.is_null ? Json(null) : Json(value); }
    static DNVal fromJson(Json v) { return v.isNull ? DNVal(Null._) : DNVal(v.get!string); }
}

alias DVal(T) = Nullable!(DNVal!T);

struct Person { @embedNullable DVal!string name; }

So the "undefined" state wouldn't be represented by the algebraic type anymore, but by the wrapping Nullable. Of course, this may not be as nice to work with.

BTW, I agree that the handling of null is strange here and, looking at the history, it has ended up in the code in the first draft already and has just been dragged through until now. It also looks like the tryReadNull is in the wrong place and should really be between beginReadDictionaryEntry and endReadDictionaryEntry. However, I also doubt that we can safely change this behavior now. What I'd propose is to use introspection on the target type (DVal!T in this case) and decide based on that whether to make the null check upfront.

That could be an additional UDA, or it could use the presence/signature of the (from/to)Representation methods. The latter would have the disadvantage of ignoring (from/to)Json and (from/to)Bson. Another approach, which seems like the best of the three, would be to only skip the null check for struct types that satisfy either isCustomSerizalizable or Serializer.isSupportedValueType!T. That would at least minimize the surface for possible regressions.

Previous issue: #1793

[ddcovery]

Third option is what I personally expected when begun to implement DVal. Seems to be the logic one.

I finally implemented a template mixin that generates (fro/to)Representation on my DTO structs avoiding the need of @optional in its properties.

struct Person{ 
  DVal!string name; 
  DVal!string surname; 
  DVal!int tallness; 
  
  mixin JsonRepresentationMixin!Person
}

I can close this issue if you prefer (there is #1793 pointing the same problem)