Support Wrappers in Nestjs

[julien-sarazin]

Hey! Thanks for the amazing lib 👍 I'm trying to integrate it with NestJS, which works very well for 99% of my use cases, the remaining 1% is the support of Google Wrappers.

Defining the following proto file :

message Device {
  string id = 1;
  google.protobuf.Timestamp created_at = 2;
  google.protobuf.Timestamp updated_at = 3;
  google.protobuf.StringValue origin = 4;
}

Then during the object's serialization when the gRPC endpoint is called give the following error :

Device.origin: object expected

Seems to be related to

• Incompatible Wrapper Types with Nest.js (protobufjs) #69
• Any way to disable detection of wrappers type? #756

I am not yet sure to clearly understand the ts-proto, protobuf.js, @grpc/proto-loader's responsibilities. From what I understood :

• ts-proto is in charge of generating the TS/JS representation of Proto files that will be used by protobuf.js
• protobuf.js is the actual engine that serializes/deserializes using the ts-proto definitions
• @grpc/proto-loader uses protobuf.js in its "server" when gRPC calls are made.

Now focusing on my issue, I understood that when you put the nestJs option you're supposed to have customized wrappers code generated (like the example you have here).

The first issue is, (I am using buf) when I generate TS definition I don't have this custom override.
The second is, I don't see any support for StringValue.

Could you help me on that? And if needed I would be glad to add the support for more wrappers with a little guidance 😅
Thanks!

[julien-sarazin]

For reference, bellow my buf config :

...
plugins:
  - plugin: buf.build/community/stephenh-ts-proto
    out: ./packages/typescript/gen
    opt:
      - nestJs=true
      - useDate=true

[julien-sarazin]

Still looking to the lib, trying to figure it out and one question came to my mind: Is it possible that the fix for wrapper is actually available, but the version of protobuf used in my nest application being not the same than the one based on ts-proto, the patch is not applied?

> npm list protobufjs
@implicity-healthcare/device-registry-service@1.0.0 /Users/juliensarazin/workspace/implicity-healthcare/microservices/device-registry
├─┬ @grpc/proto-loader@0.7.7
│ └── protobufjs@7.2.3
├─┬ @implicity-healthcare/device-registry@0.0.29-beta
│ └── protobufjs@7.2.3 deduped
└─┬ ts-proto@1.148.2
  ├── protobufjs@6.11.3
  └─┬ ts-proto-descriptors@1.9.0
    └── protobufjs@6.11.3

[julien-sarazin]

Re-aligned the version and it does not change a thing 😭

➜  device-registry: ✗ npm list protobufjs
@implicity-healthcare/device-registry-service@1.0.0 /Users/juliensarazin/workspace/implicity-healthcare/microservices/device-registry
├─┬ @grpc/proto-loader@0.6.13
│ └── protobufjs@6.11.3
├─┬ @implicity-healthcare/device-registry@0.0.31-beta
│ └── protobufjs@6.11.3 deduped
└─┬ ts-proto@1.148.2
  ├── protobufjs@6.11.3 deduped
  └─┬ ts-proto-descriptors@1.9.0
    └── protobufjs@6.11.3 deduped

[julien-sarazin]

Tried the workaround mentioned here using :

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.StringValue'] = {
		fromObject: function (value: string | undefined) {
			return value ? { value } : undefined;
		},
		toObject: function (message: { value: string | undefined }) {
			return message.value
		},
	} as any;

	return _loadSync(filename, options);
};

but the code is never called.
Also, ensure package versions are aligned :

➜  device-registry git:(develop) ✗ npm list protobufjs
@implicity-healthcare/device-registry-service@1.0.0 /Users/juliensarazin/workspace/implicity-healthcare/microservices/device-registry
├─┬ @grpc/proto-loader@0.6.13
│ └── protobufjs@6.11.3 deduped
├─┬ @implicity-healthcare/device-registry@0.0.33-beta
│ └── protobufjs@6.11.3 deduped
├─┬ @implicity-healthcare/nest-ts-proto-loader@0.0.5
│ └── protobufjs@6.11.3 deduped
├── protobufjs@6.11.3
└─┬ ts-proto@1.148.2
  ├── protobufjs@6.11.3 deduped
  └─┬ ts-proto-descriptors@1.9.0
    └── protobufjs@6.11.3 deduped

[stephenh]

Hi @julien-sarazin , apologies for the delay, I only have the opportunity to triage ts-proto issues every few weeks or so.

I think you are right, we don't create a wrapper for StringValue atm...I think it would be pretty easy, just a matter of copy/pasting this timestamp-based one:

https://github.com/stephenh/ts-proto/blob/main/src/main.ts#L423

Or, while prototyping, you could just write a StringValue-version of this directly in the *.ts file that ts-proto generates, just to see if it's working.

If you wanted to submit a PR for this, that'd be great, and you could probably add a StringValue field to this existing nestjs integration test:

https://github.com/stephenh/ts-proto/blob/main/integration/nestjs-simple/hero.proto

The readme has an overview of contributing, but lmk if you have questions. Thanks!

[julien-sarazin]

Hi @stephenh thanks for your reply.
Instinctively this is what I've tried. First as shown here, then directly from the node_modules, trying to force the association of these custom methods when browsing the wrappers.

I reached a point digging into protobufjs and see that for some reason (still did not understand why) only Struct and Timestamp are evaluated against the "wrappers map".

// type.js
Type.prototype.setup = function setup() {
    // Sets up everything at once so that the prototype chain does not have to be re-evaluated
    // multiple times (V8, soft-deopt prototype-check).

    var fullName = this.fullName,
        types    = [];
    for (var i = 0; i < /* initializes */ this.fieldsArray.length; ++i)
        types.push(this._fieldsArray[i].resolve().resolvedType);

    // Replace setup methods with type-specific generated functions
    this.encode = encoder(this)({
        Writer : Writer,
        types  : types,
        util   : util
    });
    this.decode = decoder(this)({
        Reader : Reader,
        types  : types,
        util   : util
    });
    this.verify = verifier(this)({
        types : types,
        util  : util
    });
    this.fromObject = converter.fromObject(this)({
        types : types,
        util  : util
    });
    this.toObject = converter.toObject(this)({
        types : types,
        util  : util
    });

    // Inject custom wrappers for common types
    var wrapper = wrappers[fullName];
    if (wrapper) {
        var originalThis = Object.create(this);
        // if (wrapper.fromObject) {
            originalThis.fromObject = this.fromObject;
            this.fromObject = wrapper.fromObject.bind(originalThis);
        // }
        // if (wrapper.toObject) {
            originalThis.toObject = this.toObject;
            this.toObject = wrapper.toObject.bind(originalThis);
        // }
    }

    return this;
};

Doing what you suggested will add the .google.protobuf.StringValue to the map but no common type will never be evaluated against it. I am now trying to understand why only Timestamp and Struct are evaluated.

Thanks in advance for any help on that matter.

[stephenh]

Huh! That is really odd...

I set a breakpoint in the current Timestamp.fromObject code, and it's getting called from this call path:

(function anonymous(types,util
) {
return function Hero$fromObject(d){
  if(d instanceof this.ctor)
  return d
  var m=new this.ctor
  if(d.id!=null){
  m.id=d.id|0
  }
  if(d.name!=null){
  m.name=String(d.name)
  }
  if(d.birthDate!=null){
  if(typeof d.birthDate!=="object")
  throw TypeError(".hero.Hero.birthDate: object expected")
  m.birthDate=types[2].fromObject(d.birthDate)
  }
  return m
}
})

Which is in a generated file; honestly I did not realize that protobufjs was doing run-time generation of serialization code...oh, actually that makes sense, b/c NestJS is going through grpc-js which is going through protoloader, so it's the protoloader that assumes we're using stock protobufjs messages.

That makes sense, but yes, still really odd wrt StringValue's wrappers entry not getting hit...

Do you mind pushing up a PR of what you've got so far? If I have time I'll set a few break points here & there and maybe see if I can guess what's going on.

I did some searches for "Timestamp" in the protobufjs codebase and didn't see anything sticking out, like a LOC that says "make Struct & Timestamp look up in wrappers but not anything else". 🤔

Thanks!

[julien-sarazin]

MR is on its way 👍
Tests fails at :

  ● nestjs-simple-test nestjs › should addOneHero

    13 INTERNAL: Request message serialization failure: .hero.Hero.nickName: object expected

While cleaning my mess up, I was thinking that Google wrappers do not have their own proto files; they are all imported under google/protobuf/wrappers. Could it be related?

Sorry if I am really out of it, I'm new to protobuf ecosystem, protoc, and not used to code generation libraries!
Cheers!

[julien-sarazin]

Hello @stephenh, any chance to have a look this week?

[julien-sarazin]

Friendly 🆙
@stephenh should we consider not using primitive wrappers when integrating into nestjs?

[stephenh]

Hi @julien-sarazin ; finally got a chance to take a look at this.

Thank you for putting together this repro/initial PR! With this example, it looks like this issue is that NestJS goes through grpc.js, which goes through proto-loader, which goes through protobufjs, and specifically protobufjs's converter encoder objects, which are it's own "mini-code-generated" encoders/decoders, which it oddly it codegens at runtime.

So the code ends up looking like:

  if(d.isFamous!=null){
  if(typeof d.isFamous!=="object")
  throw TypeError(".hero.Hero.isFamous: object expected")
  m.isFamous=types[5].fromObject(d.isFamous)
  }
  if(d.experience!=null){
  if(typeof d.experience!=="object")
  throw TypeError(".hero.Hero.experience: object expected")
  m.experience=types[6].fromObject(d.experience)
  }

And the issue is that we want isFamous and experience to be primitives, and so the hard-coded typeof ... !== object from protobufjs's fails.

It turns out this is a known issue in protobufjs:

protobufjs/protobuf.js#677 (comment)

And even has a super-short PR open to fix it:

protobufjs/protobuf.js#1271

Unfortunately the PR has not been merged in the ~4 years since it's been open. I just pinged the maintainers to see if they'll merge it 🤞 :

protobufjs/protobuf.js#1271 (comment)

But currently I think we're blocked on that.

I did a little bit of exploration to see if we could side-step their runtime-generated converter/encoders, and just use the ones that ts-proto generates itself, but I'm not finding a way to slice that into proto-loader before it hits the problematic typeof check.

[julien-sarazin]

Hey @stephenh, I just wanted to give you a big thank you for looking into this! I'm really glad to know what needs to be fixed, although it's a bit disappointing that we might not see it resolved for a while.

Your time and effort on this are truly appreciated, and I want to give you a shoutout for pinging the guys from protobufjs about the PR! 🙌

In the meantime, we'll make do with the optional alternative.
Thanks again, you're awesome!