grpc / grpc-dart

The Dart language implementation of gRPC.
https://pub.dev/packages/grpc
Apache License 2.0
860 stars 271 forks source link

Nullable WellKnownTypes - Timestamp and Duration #502

Closed zs-dima closed 3 years ago

zs-dima commented 3 years ago

google accounts Timestamp and Duration (WellKnownTypes) as nullable types, but grpc-dart library account them as non-nullable types.

How I could pass null for the Timestamp or Duration WellKnownTypes in this case without adding own types ?

Could be nice to make this type nullable.

https://github.com/protocolbuffers/protobuf/issues/2168

Thanks in advance

mraleph commented 3 years ago

Sorry, I don't understand the question. Could you provide some code to illustrate what you are asking?

From Dart language perspective (post NNBD) a type like Duration can't be nullable. If you want to store null it has to be Duration?.

From protobuf perspective - there is no concept of nullable Duration either AFAIK.

zs-dima commented 3 years ago

@mraleph thanks for reply

For the protobuf and other languages proto google.protobuf.Duration means Duration? in dart. But it is Duration currently - so I could not pass Null as Duration or Timestamp to the backend or etc

Could be nice to make google.protobuf.Duration as Duration? in dart and make google.protobuf.Timestamp as DateTime? in dart

zs-dima commented 3 years ago

@mraleph I send you protobuf protocol discussion where google.protobuf.Timestamp mentioned as nulluble type. so google.protobuf.Timestamp have to be DateTime? in dart

however protobuf protocol does not have non-nulluble alternatives for this types (same as int32<>Int32Value, string<>StringValue and etc) but it is question to the protobuf protocol.

mraleph commented 3 years ago

I looked a bit into this. It seems that C# protobuf implementation has a really different behavior from how other protobuf implementations behave. Most specifically in C# you can actually have null stored in a field even though you are not supposed to be able to distinguish between missing fields and fields initialized with default values. Given a message:

message Msg {
    google.protobuf.Timestamp update = 1;
}

C# protoc backend generates:

  private global::Google.Protobuf.WellKnownTypes.Timestamp update_;
  public global::Google.Protobuf.WellKnownTypes.Timestamp Update {
    get { return update_; }
    set {
      update_ = value;
    }
  }

While Java backend generates:

    private com.google.protobuf.Timestamp update_;
    /**
     * <code>.google.protobuf.Timestamp update = 1;</code>
     * @return Whether the update field is set.
     */
    @java.lang.Override
    public boolean hasUpdate() {
      return update_ != null;
    }
    /**
     * <code>.google.protobuf.Timestamp update = 1;</code>
     * @return The update.
     */
    @java.lang.Override
    public com.google.protobuf.Timestamp getUpdate() {
      return update_ == null ? com.google.protobuf.Timestamp.getDefaultInstance() : update_;
    }

Java seems to match behavior prescribed by wire format, but C# seems to expose whether field is present or absent through nullability. It is not specific to Timestamp in anyway - just a generic behaviour for all fields.

If you want to mimic C# behaviour (e.g. if you are communicating with C# server which is misusing protobufs due to, what I think is essentially a bug in original C# protoc implementation) you have to manually clear the field, e.g. for a Msg class generated from protobuf above you could add an extension:

extension NullableTimestamp on Msg {
  void set UpdateOrNull(DateTime? value) {
    if (value != null) {
      Update = Timestamp.fromDateTime(value);
    } else {
      clearUpdate();
    }
  }

  DateTime? get UpdateOrNull => hasUpdate() ? update.toDateTime() : null;
}

Unfortunately you will have to write these extensions manually.

zs-dima commented 3 years ago

@mraleph thanks a lot for the workaround at least. I agree that it is protobuf protocol issue and it have to be Timestamp <> TimestampValue and Duration <> DurationValue same as other nullable types.

zs-dima commented 3 years ago

@mraleph I created related protobuf protocol issue about https://github.com/protocolbuffers/protobuf/issues/8822