Incompatible Wrapper Types with Nest.js (protobufjs)

[ssilve1989]

When using Google's Well-Known Types with Nest (or just plain protobufjs) the types generated from ts-proto fail to be parsed.

Given this protobuf

syntax = "proto2"
import "google/protobuf/wrappers.proto";

message MonitorHelloRequest {
    optional google.protobuf.StringValue name_prefix = 2;
    optional google.protobuf.Int64Value limit = 3;
}

we get a typescript interface of

export interface MonitorHelloRequest {
  namePrefix: string | undefined;
  limit: number | undefined;
}

which seems fine. But when attempting to use that as a message that is parsed by a server using protobufjs (nest in this case)

You get the error

TypeError: MonitorHelloRequest.namePrefix: object expected

This is because protobufjs is actually expecting that interface to be

export interface MonitorHelloRequest {
  namePrefix: { value: string } | undefined
  limit: { value: number } | undefined;
}

it is unclear to me who is right here as I can see the argument for either. This was referenced on protobufjs/protobuf.js#1042 and several other issues but there does not seem to be any update in a very long time from protobufjs on the matter

[ssilve1989]

@stephenh I've opened up a reproduction repo here: https://github.com/ssilve1989/ts-proto-issue-69

[stephenh]

Ah shoot. Yeah...

So, one of my pet goals with ts-proto was to get some resemblance of optional fields back into protobuf.

I did this by treating proto primitives as required (b/c they come back with default values if unset) and then the well-known types I purposefully make into string | unknown.

Then in the Message.encode/decode methods that ts-proto controls, I can turn these string | unknown types into the { value: string } wrappers that protobuf wants on the wire.

However, you're using NestJS's protobuf support to do the actual serde, which doesn't know about my "haha well-known types are my hack to get optional fields back into protobuf".

So, not sure the best solution here. You either need to:

1. Teach ts-proto to not output well-known types as the string | optional, which is "more correct", but also a generally worse API,
2. Teach NestJS's protobuf serializer about ts-proto's "different" approach to well-known types

I don't know much about how NestJS's protobuf internals works. Obviously my bias would be to teach it about ts-proto's approach because I think it makes a better API for application programmers. :-)

[stephenh]

I need a way to "@" the regular NestJS contributors, i.e. @toonvanstrijp @iangregsondev @lukas-andre @jessequinn to get your thoughts/input on things like this as actual-NestJS-users.

Are you guys following all ts-proto issues, and so already see comments/issues like this? If so that's great.

Otherwise maybe time for a gitter/spectrum/slack/something setup?

Also, let me know if you have any ideas about this particular issue, i.e. is adding this string | unknown support into NestJS protobuf directly something that would be doable... (I'm guessing it's only like 20% likely we'll do that, and that for now NestJS users will have to opt-out of ts-proto's fancier well-known type support...which kinda sucks but is probably simpler.)

[ssilve1989]

Teach NestJS's protobuf serializer about ts-proto's "different" approach to well-known types

There seems to be an option in Nest's grpc options to use a different proto-loader, but there's no documentation on what implementing a custom-loader would look like.

[jessequinn]

I have ran into a similar issue when using google timestamp.

May i suggest Discord as NestJS has a large community already there.

[jessequinn]

There seems to be an option in Nest's grpc options to use a different proto-loader, but there's no documentation on what implementing a custom-loader would look like.

This is definitely an issue. A lack of documentation in some cases. Again, many of the NestJS contributors are on Discord.

[timostamm]

According to the google docs,

The JSON representation for StringValue is JSON string.

See wrappers.proto and the reference.

The proto3 language guide has a compact table showing all JSON mappings.

[stephenh]

@timostamm I believe that ts-proto itself faithfully implements that JSON mapping you reference, specifically for the StringValue/etc. wrapper types (but do point something out if it doesn't).

AFAIU this issue isn't about JSON serialization, it's that the NestJS binary protobuf encoders/decoders don't know about ts-proto's internal decision to "not wrap" the wrapper types.

I haven't had time to look into this, but I'm hoping that the Nest grpc custom proto loader option that @ssilve1989 mentioned would allow the ts-proto-generated clients to tell Nest's grpc internals defer to ts-proto's own Message.encode/Message.decode methods for handling the binary bytes <-> JS object translation.

[timostamm]

@stephenh My bad, I thought this was about the JSON rep.

Still getting to know ts-proto, but so far I am positively surprised by the Protobuf representation.
Don't have a good picture of the JSON rep yet, will let you know in case something catches my eye.

[charlie430]

The following Google well known types also do not serialize correctly.

• google.protobuf.Struct
• google.protobuf.ListValue
• google.protobuf.Value

[tomer-friedman]

@ssilve1989 I just ran into the exact same issue. Did you find a working solution? (custom proto-loader or anything)

[ssilve1989]

@tomer-friedman No we are just avoiding using wrapper types for now.

[ofekshalom]

@stephenh hi, I have the same issue as @ssilve1989. I didn’t understand your answer:
“Then in the Message.encode/decode methods that ts-proto controls, I can turn these string | unknown types into the { value: string } wrappers that protobuf wants on the wire.”
Can you explain (add an example🙏) how to turn for example string | undefined to {value:string} and for other types like floatValue.
Thanks :)

[stephenh]

@ofekshalom the gist is that ts-proto already turns string | undefined to { value: string } for you in its Message.encode methods, i.e.:

export const SimpleWithWrappers = {
  encode(message: SimpleWithWrappers, writer: Writer = Writer.create()): Writer {
    if (message.name !== undefined && message.name !== undefined) {
      StringValue.encode({ value: message.name! }, writer.uint32(10).fork()).ldelim();
    }
    if (message.age !== undefined && message.age !== undefined) {
      Int32Value.encode({ value: message.age! }, writer.uint32(18).fork()).ldelim();
    }

From the simple.ts file in the ts-proto integration tests.

For non-NestJS users of ts-proto this is just fine, b/c those users manually call SimpleWithWrappers.encode or SimpleWithWrappers.decode which know to do "the right thing".

The "bug" is that NestJS doesn't know to call these ts-proto encode/decode methods, and instead uses the generic grpc-node serialization logic, which is not aware of how ts-proto handles the well known types in a different (and asserted better) way.

So ideally we'd find a way to tell NestJS to rely on ts-proto's serialization logic, instead of grpc-nodes.

I don't use NestJS personally so am not sure how to do this or if its possible.

[aodpi]

Any updates on this? Is it possible to have an option that will generate for example for an optional

google.protobuf.Int64Value propertyId = 1

to generate a typescript as follows:

propertyId?: Int64Value;

I've manually set the property to this type and it works as expected. Although I have to manually check if the input property has value sent the Int64Value object and send null if otherwise.

[radarsu]

Could we get option to turn off ts-proto flattening wrapper types?

[stephenh]

@radarsu I'm not against that per se, other than worrying how much code might need to change (maybe not a lot, not sure), and ts-proto's already-a-lot-of-flags slippery slope.

Although I wonder if what we're actually doing at that point is: NestJS assumes "basically protobufjs objects", and so while it seems like we're asking ts-proto to "not flatten wrapper types", fundamentally what we're doing is asking ts-proto to match one-for-one the protobufjs objects, so that NestJS is none the wiser.

At which point, why use ts-proto and instead just use protobufjs which is what NestJS expects?

(Disclaimer I'm not a NestJS expert, so could have ^ wrong/be missing something.)

Basically I'd rather teach NestJS to use ts-proto's own encode / decode methods, which is what we've done for other framework integrations like grpc-web. Disclaimer I don't personally have time to work on this, but I think that's the tact we should take.

[tuananhlai]

Are there any workaround for this issue yet?

[lucasmaehn]

I think i finally found a workaround for this problem after spending the whole day on that:D
We can update the wrapper object from protobufjs like the following:

import { wrappers } from 'protobufjs';
wrappers['.google.protobuf.Timestamp'] = {
  fromObject: function (value) {
    return {
      seconds: value.getTime() / 1000,
      nanos: (value.getTime() % 1000) * 1e6,
    };
  },
  toObject: function (message: { seconds: number; nanos: number }, options) {
    return new Date(message.seconds * 1000 + message.nanos / 1e6);
  },
} as any; // <- dirty workaround :D

This can, of course, be extended to incorporate other well known protobuf messages.

To automatically run this code, I created a small package, with only one file (index.ts) looking like the following:

import {
  loadSync as _loadSync,
  Options,
  PackageDefinition,
} from '@grpc/proto-loader';
import { wrappers } from 'protobufjs';

export const loadSync = (
  filename: string | string[],
  options?: Options,
): PackageDefinition => {
  wrappers['.google.protobuf.Timestamp'] = {
    fromObject: function (value) {
      return {
        seconds: value.getTime() / 1000,
        nanos: (value.getTime() % 1000) * 1e6,
      };
    },
    toObject: function (message: { seconds: number; nanos: number }, options) {
      return new Date(message.seconds * 1000 + message.nanos / 1e6);
    },
  } as any;

  return _loadSync(filename, options);
};

this package can then be used in nest, by setting the protoLoader setting to the name of the package
Not sure if this makes sense to implement in this package though, since it's tackling a problem very specific to nest...

[stephenh]

@lucasmaehn awesome! Yes, this is the key we've been waiting for someone to just spend enough time digging to find. :-)

In retrospect, that actually looks pretty easy to get at.

I don't have time to look in to it / work on it, but it certainly seems feasible that ts-proto should be able to generate some sort of:

registerWrapperTypes()

That would install the Timestamp/etc. functions into protobufjs's wrappers...

It might be a little tricky to decide where in the output to put this... Like instead of a single registerWrappers.ts or something that knows each of the wrapper types the user's schema is actually using...

So, a few things to solve, but seems doable. If anyone wants to poke at it, that'd be great.

[stephenh]

Fwiw I think with #762 the wrapper types of Timestamp and Struct / Value / ListValue should be working in NestJS, so I'm going to close this out.

Feel free to open issues for other specific wrapper types if they don't work. Like FieldMask probably, but I'm not sure.

Also ts-proto's wrapper types could use some love in general, just to clean up the internal handling of them to be easier to reason about and maintain, but that's unfortunately not something I have time to do at the moment.

Thanks all!

[prenaissance]

The issue still persists, at least for DoubleValue and Int32Value.