Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unused message properties are being defaulted #189

Closed
respinha opened this issue Feb 21, 2018 · 10 comments
Closed

Unused message properties are being defaulted #189

respinha opened this issue Feb 21, 2018 · 10 comments

Comments

@respinha
Copy link

respinha commented Feb 21, 2018

Hello.

If I send a message via gRPC which doesn't have all of its properties set, they are being defaulted to a null/empty value. I've modified the officially provided example to simulate this issue.
I included some more properties in the HelloReply message

message HelloReply {
  string message = 1;
  double number_property = 2;
  string string_property = 3;
  repeated string array_property = 4;
  message NestedMsg {
  	string property = 1;
  }
  NestedMsg nested_property = 5;
}

and I just added a small print statement to dynamic_codegen/greeter_client.js

console.log('Total message:', JSON.stringify(response));

and it prints:

Total message: {"message":"Hello world","number_property":0,"string_property":"","array_property":[],"nested_property":null}

So, all properties are being defaulted to 0/""/[]/null. Is this the expected behaviour? I thought it could also be a protobuf.js-specific issue.
In my case, this behaviour is not desirable as in some cases I override existing data in my system with data from protobuf messages and if those properties are defaulted I might even override valid data with a null or empty value.

If this is the expected workflow for gRPC messages, can it be turned off?

@respinha respinha changed the title Non-used message properties are being defaulted Unused message properties are being defaulted Feb 21, 2018
@murgatroid99
Copy link
Member

This is the expected behavior of protobuf, as documented here. Currently, gRPC does not provide a way to change that, but I am working on a new package (proposed here) that will provide that option among other things.

@respinha
Copy link
Author

I didn't notice it in the documentation, thanks, @murgatroid99. I'll wait for the new package then.

@nicolasnoble
Copy link
Member

The new @grpc/proto-loader package has been published. Could you tell us if this helps with your problem ?

@respinha
Copy link
Author

respinha commented May 2, 2018

Hey, @nicolasnoble, thanks for the notice.

However, when I try to apply the package definition as in the example within Typescript code, I get a type definition error:

error TS2339: Property 'loadPackageDefinition' does not exist on type 'typeof "grpc"'.

@murgatroid99
Copy link
Member

We've published version 1.11.1 which includes those definitions in the TypeScript types file.

@respinha
Copy link
Author

respinha commented May 3, 2018

Hmm, I updated to version 1.11.1 and when I run tsc -d I get

node_modules/grpc/index.d.ts(1307,61): error TS2304: Cannot find name 'function'.
node_modules/grpc/index.d.ts(1309,54): error TS2304: Cannot find name 'function'.
node_modules/grpc/index.d.ts(1311,61): error TS2304: Cannot find name 'function'.
node_modules/grpc/index.d.ts(1349,82): error TS2304: Cannot find name 'function'.
node_modules/grpc/index.d.ts(1351,55): error TS2304: Cannot find name 'function'.
node_modules/grpc/index.d.ts(1353,39): error TS2304: Cannot find name 'function'.
node_modules/grpc/index.d.ts(1355,40): error TS2304: Cannot find name 'function'.
node_modules/grpc/index.d.ts(1357,41): error TS2304: Cannot find name 'function'.

@Crevil
Copy link
Contributor

Crevil commented May 3, 2018

This is fixed with #307 but not released yet

@respinha
Copy link
Author

respinha commented May 3, 2018

I fixed the typings locally in order to perform tests.
It seems to work as expected!

@nicolasnoble
Copy link
Member

@respinha-ribeiro there's an issue with node 10 (nodejs/node#20258) that prevents us from running tests with node 10 at the moment, so we can't legitimately recommend using node 10 with gRPC just yet. This will require a new release of node 10.

@respinha
Copy link
Author

respinha commented May 3, 2018

Thank you for the notice. We are using Node 9 anyway and it all seems fine.

@lock lock bot locked as resolved and limited conversation to collaborators Sep 29, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants