stephenh / ts-proto

An idiomatic protobuf generator for TypeScript
Apache License 2.0
2.13k stars 345 forks source link

null Date returning 0001-01-01T00:00:00.000Z #841

Closed tomspeak closed 1 year ago

tomspeak commented 1 year ago
export interface MyData {
  myField: Date | undefined;
}

I have this type definition being generated via buf.build plugins

version: v1
plugins:
  - plugin: buf.build/community/stephenh-ts-proto
    out: gen/ts
    opt: # https://github.com/stephenh/ts-proto#supported-options and https://github.com/stephenh/ts-proto#nestjs-support
      - env=node
      - addGrpcMetadata=true
      - nestJs=true
      - forceLong=string
      - useDate=true

The issue is that when my gRPC service returns a Null Timestamp for myField, what that is serialized to is

myField: "0001-01-01T00:00:00.000Z"

Question 1: Is this a bug as I am combining useDate and forceLong=string?

The type definitions in the original toObject (toObject(message: { seconds: number; nanos: number })) made me wonder if some weird conversion is going on.

A hack around this is, in my NestJS controller I have added

wrappers['.google.protobuf.Timestamp'] = {
  ...wrappers['.google.protobuf.Timestamp'],
  toObject(message: { seconds: number; nanos: number }) {
    return message && message.seconds && message.nanos
      ? new Date(message.seconds * 1000 + message.nanos / 1e6)
      : undefined;
  },
} as any;

This now correctly does not return myField as it is undefined, the issue is that this seems to only work if I put it directly in my controller, so I would have to keep repeating this over and over.

Question 2: If this is the way to override this behaviour, is there a better way to do it?

Many thanks

stephenh commented 1 year ago

Hi @tomspeak ; I'm not sure why that is happening. It does seem like adding message && to the toObject method would be an easy fix, but fwiw I tried to reproduce the issue in this PR:

https://github.com/stephenh/ts-proto/pull/842

And the tests seem to be passing fine.

Can you tell why NestJS is calling toObject with a null message? I had assumed if the timestamp was null/undefined, it would not be put on the wire, and so the toObject method would never be called.

tomspeak commented 1 year ago

@stephenh apologies for wasting your time here, I should have inspected the payload before opening this issue 😮‍💨

After logging what was being returned:

running message:  Timestamp {
  seconds: Long { low: -2006054656, high: -15, unsigned: false }
}

I noticed that for this field, this negative value was returned every single time as if it were a const value.

After some digging it turns out that my Go gRPC service was returning Go's version of a "null time", which does not seem to play nicely at some level of this stack.

For now the cleanest fix was on the Go side, by not returning the field at all if it is null, rather than returning a field that is null.

Thank you for your time