Strict response message checking

[dennypenta]

Is your feature request related to a problem? Please describe.

When I send response message and it doesn't correct, I get empty message or several empty fields in response.

Describe the solution you'd like

Generate schema depends on .proto file and extend it with some validation rules.

[murgatroid99]

Can you share some code that shows the problem you are experiencing?

[samuela]

Let's say I have a proto message:

message PlayReply {
  int32 count = 1;
}

and a handler:

function Play(call, callback) {
  callback(null, {count: 'this is a string, not a number'});
}

Then the client will receive a message with count = 0. Or you can provide too many fields:

function Play(call, callback) {
  callback(null, {notActuallyAField: 10});
}

and the same thing happens. This is pretty scary to be completely honest. I could see lots of ways this could produce very subtle, hard-to-track-down bugs.

[nicolasnoble]

First, which codegen method did you use? Then, I'd hate to say, but this behavior is very idiomatic of JavaScript. What you really need here is typescript support for the codegen, which is being discussed over at #528

[samuela]

This was using dynamic codegen.

I could see the argument that additional fields are acceptable (width subtyping). But I don't really see how allowing strings to be passed for int fields is idiomatic JavaScript.

[nicolasnoble]

In plain JavaScript it's possible to do this:

setTimeout(() => console.log('hello world'), 'hello world again')

and it works fine, even though setTimeout is supposed to take a number as its second argument, and it's equivalent to:

setTimeout(() => console.log('hello world'), 0)

Moreover, since you're using the dynamic codegen, protobufjs will use the Number constructor, so your example can also do this:

  callback(null, { count: '1234' })

And it'll actually send the integer 1234, since Number can take a String as a constructor:

$ node
> Number('1234')
1234

So, no, this is idiomatic.

[samuela]

Fair enough, but still quite dangerous. Perhaps a better goal for this issue would be to request a "strict" mode a la "use strict" in JavaScript.

[nicolasnoble]

Right, and I strongly believe that this strict mode that you're talking about is basically TypeScript. So if we can get proper support for it in our codegen, this would be covered.

[samuela]

I'm a big fan of TypeScript but I imagine there are a number of pure JS users out there that would like stricter behavior but cannot use TS for whatever reason (boss, etc). So I think there could still be value in a JS strict mode.

FWIW I hacked together a TypeScript gRPC client/server here: https://github.com/samuela/grpc-polyglot

[murgatroid99]

I can see a way of adding stricter validation like this. There is a different way of using Protobuf.js that accepts more restricted field types. But it will likely not be as user-friendly. In particular, it wouldn't allow strings to be used to represent 64 bit integers, only numbers and Long objects. Similarly, enum values could only be represented by numbers, not the string names. There could be an option in the proto loader library to use that validation scheme.