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

Enums should default to first defined value not the one with tag = 0 #613

Closed
robin-anil opened this issue Jan 3, 2017 · 7 comments
Closed

Comments

@robin-anil
Copy link
Contributor

robin-anil commented Jan 3, 2017

On the client side I am encoding

{
   operation: 'AND',
   valueSet: [{
      dimension: 'DIMENSION_1',
      value: [2]
   }]
}

On the server (in java) I am receiving

{
    valueSet: {
      value: [605]
    }
}

Here is the proto used. Stripped down for simplicity

message Filter {
  enum Operation {
    AND = 0;
  }

  optional Operation operation = 1;
  repeated ValueSet value_set = 2;
  repeated Filter filter = 5;
}

enum Dimension {
   DIMENSION_1 = 0;
}

message ValueSet {
  optional Dimension dimension = 1;
  repeated int64 value = 2;
}
@robin-anil
Copy link
Contributor Author

robin-anil commented Jan 3, 2017

On the client side converting to buffer and parsing the buffer back yields the correct proto/json (1st one). Since the server side is using Google's protobuf library, I suspect protobufJS to be having the bug, but we could be wrong.

@dcodeIO
Copy link
Member

dcodeIO commented Jan 3, 2017

{
   "operation": "AND",
   "valueSet": [{
      "dimension": "DIMENSION_1",
      "value": [ 2 ]
   }]
}

Buffer:

08	id 1, wireType 0
00	value 0 -> Filter#operation = 0 (AND)
12	id 2, wireType 2
04	length 4 -> Filter#value_set[0] =
 08	id 1, wireType 0
 00	value 0 -> ValueSet#dimension = 0 (DIMENSION_1)
 10	id 2, wireType 0
 02	value 2 -> ValueSet#value = 2

Looks ok to me. The optional enum defaults are most likely encoded on the wire because these are set as strings without using Message.from to convert them properly. Encoders just silently convert them to numbers - either use Message.verify (returns "Filter.operation: enum value expected" here) to check or .from to coerce them.

See:

{
   "operation": 0,
   "valueSet": [{
      "dimension": 0,
      "value": [ 2 ]
   }]
}

Buffer:

12	id 2, wireType 2
02	length 2
 10	id 2, wireType 0
 02	value 2

Which is the same without optional enum defaults.

Correct usage example:

var protobuf = require("..");

var msg = {
   operation: "AND",
   valueSet: [{
      dimension: "DIMENSION_1",
      value: [2]
   }]
};

var root = protobuf.loadSync(require.resolve("./issue.proto"));

var Filter = root.lookup("Filter");

var reason = Filter.verify(msg);
if (reason !== null)
  console.log("message is invalid when calling just encode because: " + reason);

var buf = Filter.encode(Filter.from(msg)).finish();
console.log(buf);

@robin-anil
Copy link
Contributor Author

robin-anil commented Jan 3, 2017

ha. it turns out the enum is a bit weird. number 18 is defined at the top and on the server side it is being defaulted to DIMENSION_18 when you call valueSet.getDimension() as the parsed object does not seem to have the enum set.

  valueSet: {
     value: [605]
  }
enum Dimension {
   DIMENSION_18 = 18;
   DIMENSION_1 = 0;
}

@dcodeIO
Copy link
Member

dcodeIO commented Jan 3, 2017

I see. Optional enums with default values are not encoded on the wire, so this seems to be the underlying issue.

@robin-anil
Copy link
Contributor Author

robin-anil commented Jan 3, 2017

Hmm yeah, in this case there is no [default = DIMENSION_1] specified. Could be a quirk in the format.

Or Google's internal java code could be tickling this bug, Oh dear!

@dcodeIO
Copy link
Member

dcodeIO commented Jan 3, 2017

From Google's developer guide:

For enums, the default value is the first defined enum value, which must be 0.

https://developers.google.com/protocol-buffers/docs/proto3#default

Seems that the official implementation just takes the first defined, while protobuf.js uses 0. This still shouldn't result in an invalid wire format, though. Hmm.

@robin-anil
Copy link
Contributor Author

robin-anil commented Jan 3, 2017

The enum issue is fixed on the server side by sorting our enum defs for now, but yes you should use the first defined as the default in js as well. The int64 wire type issue is red-herring, that seems to be a bug in our code which was sending wrong data. I will rename the bug

@robin-anil robin-anil changed the title Invalid byte stream created by protobufjs Enums should default to first defined value not the one with tag = 0 Jan 3, 2017
@dcodeIO dcodeIO closed this as completed in b52089e Jan 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants