Cannot deserialize empty strings as Map values in JavaScript

[paambaati]

What version of protobuf and what language are you using?
Version: v3.9.1 and v3.10.0
Language: Javascript

What operating system (Linux, Windows, ...) and version?
macOS Mojave 10.14.6

What runtime / compiler are you using (e.g., python version or gcc version)
Node.js v12.9.1
Apple LLVM version 10.0.1 (clang-1001.0.46.4)
Target: x86_64-apple-darwin18.7.0

What did you do?
I'm using generated static bindings from https://github.com/liftbridge-io/liftbridge-grpc/blob/5694b15f251d2ff16d7d4c3e8d944aab327d3ef0/api.proto#L93-L104 to create a Liftbridge Node.js client that will create a stream and publish some messages and subscribe to them (Liftbridge is a kind of lightweight Kafka), and I run into an Failed to parse server response error consistently whenever I try to consume the Subscribe stream.

Steps to reproduce the behavior

1. docker run -p 4222:4222 -ti nats:latest --debug --trace in a window.
2. go get github.com/liftbridge-io/go-liftbridge and then $GOPATH/bin/liftbridge --raft-bootstrap-seed --nats-servers nats://localhost:4222 --level debug in another window.
3. Clone my repo from https://github.com/paambaati/node-liftbridge.git in yet another window.
   1. yarn install or npm install
   2. yarn run debug or npm run debug to run the debug script that reproduces this issue.

The debug script attempts to create a new stream, then publish a few messages, then subscribes to the same stream (subject) and then publishes a few more messages.

What did you expect to see
Each published message should be printed to console (see relevant lines).

What did you see instead?

[AssertionError]: Assertion failed
    at new goog.asserts.AssertionError ~/node-liftbridge/node_modules/google-protobuf/google-protobuf.js:79:876)
    at Object.goog.asserts.doAssertFailure_ ~/node-liftbridge/node_modules/google-protobuf/google-protobuf.js:80:257)
    at Object.goog.asserts.assert [as assert] ~/node-liftbridge/node_modules/google-protobuf/google-protobuf.js:81:83)
    at Function.jspb.Map.deserializeBinary ~/node-liftbridge/node_modules/google-protobuf/google-protobuf.js:275:277)
    at ~/node-liftbridge/grpc/generated/api_pb.js:2259:18
    at jspb.BinaryReader.readMessage ~/node-liftbridge/node_modules/google-protobuf/google-protobuf.js:249:329)
    at Function.proto.proto.Message.deserializeBinaryFromReader ~/node-liftbridge/grpc/generated/api_pb.js:2258:14)
    at Function.proto.proto.Message.deserializeBinary ~/node-liftbridge/grpc/generated/api_pb.js:2214:30)
    at deserialize_proto_Message ~/node-liftbridge/grpc/generated/api_grpc_pb.js:59:25)
    at ~/node-liftbridge/node_modules/grpc/src/common.js:38:12
    at ~/node-liftbridge/node_modules/grpc/src/client_interceptors.js:689:22 {
  message: 'Assertion failed',
  reportErrorToServer: true,
  messagePattern: 'Assertion failed'
}

The problem is when the headers map has an undefined value. The Go client reads it as an empty string (headers:<key:"reply" value:"" >) while protobuf reads it as undefined, and hence the assertion error.

The block of code where the error is being thrown is —

jspb.Map.deserializeBinary = function(a, b, c, d, e, f) {
    for (var g = void 0; b.nextField() && !b.isEndGroup();) {
        var h = b.getFieldNumber();
        1 == h ? f = c.call(b) : 2 == h && (a.valueCtor_ ? (goog.asserts.assert(e), g = new a.valueCtor_, d.call(b, g, e)) : g = d.call(b))
    }
    /* CUSTOM HACK */ if (g === undefined) g = '';
    goog.asserts.assert(void 0 != f);
    goog.asserts.assert(void 0 != g);
    a.set(f, g)
};

Note the /* CUSTOM HACK */; adding it seems to fix the issue for now.

I am guessing this is generated off of —https://github.com/protocolbuffers/protobuf/blob/8e6141a63dd2c54baff48d284bdede6a9ec56266/js/map.js#L474-L510

Related issues

1. Failed to parse server response when I try to read a stream grpc/grpc-node#1022
2. Empty strings read as undefined in Map fields protobufjs/protobuf.js#1293

Both are the same issue, reported as I was debugging this issue. They both contain additional context and some responses from maintainers & others facing the same issue.

Anything else we should know about your project / environment

1. The static bindings were generated by a script - see relevant command here. The bindings are checked-in at grpc/generated.

2. The Go version of the client I'm developing (see go-liftbridge) can in fact Subscribe to the messages Published by my Node.js debug script and print all the Messages correctly! I used the example subscribe program at liftbridge-io/go-liftbridge:example/lift-sub/subscribe.go@master to verify this 😮

3. I ran a full verbose trace of my debug script, and here's the output — https://gist.github.com/paambaati/7884b119eee47fafa436f74db8b59edc. I've padded the debug script's stdout with a lot of new lines so it is a little easier to separate them from the GRPC traces.

[paambaati]

The snippet below works though, so maybe this depends on how it is read from the wire?

const tmpMessage = new Message();
tmpMessage.getHeadersMap().set('test', '');
tmpMessage.getHeadersMap().set('test2', null);
tmpMessage.getHeadersMap().set('test3', undefined);
const bytes = tmpMessage.serializeBinary();
const tmpMessage2 = Message.deserializeBinary(bytes);
console.log(tmpMessage2.toObject());

/*
{
  offset: '0',
  key: '',
  value: '',
  timestamp: '0',
  subject: '',
  reply: '',
  headersMap: [ [ 'test', Uint8Array [] ], [ 'test2', '' ], [ 'test3', '' ] ],
  ackinbox: '',
  correlationid: '',
  ackpolicy: 0
}
*/

[paambaati]

After some more debugging, it looks like the issue is because the method parameters in the generated _pb.js file are different from what's expected by protobuf.

proto.proto.Message.deserializeBinaryFromReader = function(msg, reader) {
// ...
   case 7:
      var value = msg.getHeadersMap();
      reader.readMessage(value, function(message, reader) {
        // NOTICE THE NUMBER OF ARGUMENTS HERE -->>
        jspb.Map.deserializeBinary(message, reader, jspb.BinaryReader.prototype.readString, jspb.BinaryReader.prototype.readBytes, null, "");
      });
      break;
// ...
}

Note that the call to jspb.Map.deserializeBinary only has 6 arguments in the generated _pb.js file, while https://github.com/protocolbuffers/protobuf/blob/8e6141a63dd2c54baff48d284bdede6a9ec56266/js/map.js#L474-L476 expects 7. The 7th argument is the default for the map's value field.

Is this a bug, or is there a way to coerce the default value to an empty Buffer/Uint8Array from the .proto file? AFAIK, there's no default values in Proto3.

[paambaati]

@anandolee @TeBoring @BSBandme @xfxyjwf @haberman Thoughts? If you can help me find where the _pb.js generation from the map.js file happens, I will happily send a PR.

[paambaati]

protobufjs/protobuf.js#1348 should fix this.

[dibenede]

This is similar to #27