feat: add support for Struct in NestJS #762

ZimGil · 2023-01-23T15:30:11Z

This PR is related to #69

After investigation of how Timestamp was solved with wrappers I was able to solve the issue we're seeing with Struct.

Notice that I'm considering that values of Stract are of type Value which is what's actually stated in the Docs

Field name Type Description

fields map<string, Value> Map of dynamically typed values.

While researching the wrappers solution I noticed that when wrapping google.protobuf.Struct we can wrap and unwrap but google.protobuf.Value can not be wrapped as protobufjs is not reading this value of wrappers. But due to the fact that Struct maps to Value I could use it's wrap and unwrap methods in order to wrap and unwrap Struct itself.

Same goes for ListValue

Had to bump @grpc/proto-loader to support NestJS useage of fromJSON()

stephenh

Hey @ZimGil , thanks for the PR! Apologies I just haven't had a time to get to it yet. I think it looks good. Neither NestJS or structs are things I've personally used so I really appreciate contributions to make them work!

I made a few small comments, and it looks like there is a failing test in CI, but overall this looks great, and would be great to get this merged. Thanks!

stephenh · 2023-01-29T15:18:12Z

integration/nestjs-simple/hero.ts

+      if (nested) {
+        res.structValue!.fields[field] = wrapStruct(value[field], true);
+      } else {
+        res.fields![field] = wrapStruct(value[field], true);


Question: Should this true be a false? Just thinking that we are in the nested = false branch of the it, so should we pass along false as well?

The only case we actually need for nested to be false is the base object, we should only get here initially and from here it should be always true

Gotcha, thanks!

stephenh · 2023-01-29T15:23:31Z

src/main.ts

@@ -1867,15 +1925,20 @@ function generateUnwrap(ctx: Context, fullProtoTypeName: string, fieldNames: Str
      chunks.push(code`unwrap(message: Struct): {[key: string]: any} {
        const object: { [key: string]: any } = {};
        [...message.fields.keys()].forEach((key) => {
-          object[key] = message.fields.get(key);
+          const unwrappedValue = Value.unwrap(message.fields.get(key));
+          object[key] = unwrappedValue !== undefined ? unwrappedValue : message.fields.get(key);


Question: Just curious, but instead of : message.fields.get(key), should this be just : undefined? Like if Value.unwrap can't unwrap the value, is there really a potentially valid-but-not-unwrappable value we need to get?

Can't recall why I put it there but there was something - I'll look into what happens if we remove it

The reason we have that here is for the case where the result of message.fields.get(key) is not necessarily actually wrapped and this cause the result of Value.unwrap() to return undefined

Returning the actual value instead help us avoid these.
Removing this broke the tests

Ah okay, happy to keep it then.

Out of curiosity, is it expected that we'll end up with not-actually-wrapped values in should-be-wrapped cases, or was an existing test just being particularly ocd?

The second part :)

Hm, I think I realized what was happening ...

In the findOneHero test, the server was only invoking Struct.wrap(...) once, but never invoking ListValue.wrap or Value.wrap on any of the inner contents.

Even after adding wrapper entries for them:

wrappers[".google.protobuf.Struct"] = { fromObject: Struct.wrap, toObject: Struct.unwrap } as any; wrappers[".google.protobuf.Value"] = { fromObject: Value.wrap, toObject: Value.unwrap } as any; wrappers[".google.protobuf.ListValue"] = { fromObject: ListValue.wrap, toObject: ListValue.unwrap } as any;

Value.wrap / ListValue.wrap are never invoked.

So I think that's why, inside the client .unwraps, we saw things that were "hey, wait a sec, not wrapped".

I was able to fix this by making Struct.wrap deeply wrap, and Struct.unwrap deeply unwrap...which AFAICT fixed the issue.

But it broke ts-proto's non-NestJS Struct.encode / Struct.decode methods, which specifically assumed that wrap/unwrap were shallow.

So now I need to go back and teach the encode/decode methods about the deepness of wrap/unwrap...or maybe for now we just have two versions of wrap/unwrap; one that is deep for NestJS, and one that is shallow for not-NestJS...

I'm going to go ahead and push up the deep-wrap/unwrap changes up to this PR, which will break some non-NestJS tests, but AFAICT that's the right path forward for NestJS's Struct support.

Does that seem right?

I agree that deeply wrapping and unwrapping is the way to go

Don't want to break the tests.

Well, I cheated and just copy/pasted both deep & shallow versions of the wrappers, but it seems to be working for now. :-)

I really haven't personally used the Struct/List/ListValue types, and helping on this PR was the closest I've been to the code... I feel like something is still a little off...like how in the regular/non-NestJs output, Struct.encode takes a Struct but it should take an any...

Anyway, I'm going to punt on that for now. Tests are passing so I'll merge and 🤞 seems like it should work.

Thanks for the PR!

stephenh · 2023-01-29T15:30:10Z

src/main.ts

  }

-  if (options.outputEncodeMethods || options.outputJsonMethods || options.outputTypeRegistry) {
+  if (options.outputEncodeMethods || options.outputJsonMethods || options.outputTypeRegistry || options.nestJs) {


Suggestion: Ah okay, we add the || options.nestJs b/c we need it to generate any wrap/unwrap methods...

I wonder if we could make this a little more nuanced, by doing something like:

const wrap = this.generateWrap(...); const unwrap = this.generateUnwrap(...); if (options.outputEncodeMethods || ... || wrap.length > 0 || unwrap.length > 0)

Pretty minor, but I think it'd avoid outputting some of the empty export const Hero = {} lines that are unnecessary.

I'll look into that

We can't seem to do that as props of generateWrap() and generateUnwrap() are in the scope of the callback of visit inside that if

Any other suggestion to clean up the output?

Ah shoot, you're right. I had to poke around a little bit, but I think pushed up a few more conditionals that cleaned up some of the unnecessary output.

ZimGil · 2023-01-30T13:45:23Z

@stephenh I've pushed a fix the the tests - replied to the comments after playing around.

stephenh · 2023-01-30T22:09:26Z

src/main.ts

-      chunks.push(code`unwrap(message: Value): string | number | boolean | Object | null | Array<any> | undefined {
-        if (message?.${fieldNames.stringValue} !== undefined) {
+      chunks.push(code`unwrap(message: any): string | number | boolean | Object | null | Array<any> | undefined {
+        if (message?.hasOwnProperty('${fieldNames.stringValue}') && message?.${fieldNames.stringValue} !== undefined) {


Are the hasOwnProperty checks necessary here? Just curious if they are actually fixing something or just being extra.

Without it, it's looking at the prototype which actually has all other props set to null for some reason

At least in some cases

# [1.139.0](v1.138.0...v1.139.0) (2023-01-31) ### Features * add support for Struct in NestJS ([#762](#762)) ([e8c6d8b](e8c6d8b)) ### Performance Improvements * generate switch statement for oneof union encode ([#767](#767)) ([c3fd1e3](c3fd1e3))

stephenh · 2023-01-31T19:35:45Z

🎉 This PR is included in version 1.139.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

feat: add support for Struct in NestJS

5249801

ZimGil force-pushed the nestjs-struct branch from 0061dcc to 5249801 Compare January 23, 2023 15:52

Add another codegen update.

301ebb0

stephenh reviewed Jan 29, 2023

View reviewed changes

Fix tests

13d1ce9

stephenh added 2 commits January 30, 2023 15:32

Format.

0253e48

Make const/base output more conditional.

9956fed

stephenh reviewed Jan 30, 2023

View reviewed changes

stephenh added 4 commits January 30, 2023 16:12

Use isStructType.

6c76cf6

Consistently wrap & unwrap.

6aa03cc

Use conditional shallow/deep wrapping.

1bfd10d

Fix oneofs.

ffb21dc

stephenh merged commit e8c6d8b into stephenh:main Jan 31, 2023

stephenh mentioned this pull request Jan 31, 2023

Incompatible Wrapper Types with Nest.js (protobufjs) #69

Closed

stephenh added the released label Jan 31, 2023

jokerttu mentioned this pull request Feb 15, 2023

Maximum call stack size exceeded while encoding google.protobuf.Struct type #778

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add support for Struct in NestJS #762

feat: add support for Struct in NestJS #762

ZimGil commented Jan 23, 2023 •

edited

Loading

stephenh left a comment

stephenh Jan 29, 2023

ZimGil Jan 29, 2023

stephenh Jan 30, 2023

stephenh Jan 29, 2023

ZimGil Jan 29, 2023

ZimGil Jan 30, 2023

stephenh Jan 30, 2023

ZimGil Jan 30, 2023

stephenh Jan 31, 2023

ZimGil Jan 31, 2023

stephenh Jan 31, 2023

stephenh Jan 29, 2023

ZimGil Jan 29, 2023

ZimGil Jan 30, 2023

stephenh Jan 30, 2023

ZimGil commented Jan 30, 2023

stephenh Jan 30, 2023

ZimGil Jan 30, 2023 •

edited

Loading

ZimGil Jan 30, 2023

stephenh commented Jan 31, 2023

feat: add support for Struct in NestJS #762

feat: add support for Struct in NestJS #762

Conversation

ZimGil commented Jan 23, 2023 • edited Loading

stephenh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZimGil commented Jan 30, 2023

Choose a reason for hiding this comment

ZimGil Jan 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephenh commented Jan 31, 2023

ZimGil commented Jan 23, 2023 •

edited

Loading

ZimGil Jan 30, 2023 •

edited

Loading