JavaScript parser fails to parse packed repeated field as not packed (and vice versa)

[cygery]

Message definitions

proto2_test.proto:

syntax = "proto2";

package test;

message Test2Int32RepPackedTrue {
    repeated int32 repeated_int32 = 1 [packed = true];
}

message Test2Int32RepPackedFalse {
    repeated int32 repeated_int32 = 1 [packed = false];
}

proto3_test.proto (same as above but with proto3 instead of proto2 and Test3* instead of Test2:

syntax = "proto3";

package test;

message Test3Int32RepPackedTrue {
    repeated int32 repeated_int32 = 1 [packed = true];
}

message Test3Int32RepPackedFalse {
    repeated int32 repeated_int32 = 1 [packed = false];
}

Compiled via:

$ protoc --version
libprotoc 3.0.0
$ protoc --js_out=import_style=commonjs,binary:. *.proto

JavaScript Test

Test script which first serializes examples of the four message types defined above and then tries to deserialize each of them using all four message types. Expected behavior: deserialization works in all cases.

packed_test.js:

var test2_pb = require('./test/proto2_test_pb');
var test3_pb = require('./test/proto3_test_pb');

var plRepInt32 = [42];

functions = [test2_pb.Test2Int32RepPackedTrue, test2_pb.Test2Int32RepPackedFalse, test3_pb.Test3Int32RepPackedTrue, test3_pb.Test3Int32RepPackedFalse]
names = ["Proto2 packed=true", "Proto2 packed=false", "Proto3 packed=true", "Proto3 packed=false"]

for (var i = 0; i < functions.length; i++) {
    var src = functions[i];
    for (var j = 0; j < functions.length; j++) {
        var dst = functions[j];
        var msg = new src();
        msg.setRepeatedInt32List(plRepInt32);
        var bytes = msg.serializeBinary();
        try {
            dst.deserializeBinary(bytes);
        } catch (e) {
            console.log("deserialize failed src: " + names[i] + " dst: " + names[j]);
        }
    }
}

Using google-protobuf version 3.0.0-alpha.6 via npm.

Output

$ node packed_test.js 
deserialize failed src: Proto2 packed=true dst: Proto2 packed=false
deserialize failed src: Proto2 packed=true dst: Proto3 packed=false
deserialize failed src: Proto2 packed=false dst: Proto2 packed=true
deserialize failed src: Proto2 packed=false dst: Proto3 packed=true
deserialize failed src: Proto3 packed=true dst: Proto2 packed=false
deserialize failed src: Proto3 packed=true dst: Proto3 packed=false
deserialize failed src: Proto3 packed=false dst: Proto2 packed=true
deserialize failed src: Proto3 packed=false dst: Proto3 packed=true

Result

Parsing (not) packed as (not) packed messages works correctly.
Parsing packed as not packed messages (and vice versa) does not work although it should.

Stacktraces

Parse packed as not packed:

AssertionError: Assertion failed
    at new goog.asserts.AssertionError (/home/epg/halde/protobuf_test/node_modules/google-protobuf/google-protobuf.js:62:603)
    at Object.goog.asserts.doAssertFailure_ (/home/epg/halde/protobuf_test/node_modules/google-protobuf/google-protobuf.js:63:126)
    at Object.goog.asserts.assert [as assert] (/home/epg/halde/protobuf_test/node_modules/google-protobuf/google-protobuf.js:63:385)
    at jspb.BinaryReader.readInt32 (/home/epg/halde/protobuf_test/node_modules/google-protobuf/google-protobuf.js:234:371)
    at Function.proto.test.Test2Int32RepPackedFalse.deserializeBinaryFromReader (/home/epg/halde/protobuf_test/test/proto2_test_pb.js:453:49)
    at Function.proto.test.Test2Int32RepPackedFalse.deserializeBinary (/home/epg/halde/protobuf_test/test/proto2_test_pb.js:434:46)
    at Object.<anonymous> (/home/epg/halde/protobuf_test/packed_test.js:17:17)
    at Module._compile (module.js:541:32)
    at Object.Module._extensions..js (module.js:550:10)
    at Module.load (module.js:458:32)

Parse not packed as packed:

AssertionError: Assertion failed
    at new goog.asserts.AssertionError (/home/epg/halde/protobuf_test/node_modules/google-protobuf/google-protobuf.js:62:603)
    at Object.goog.asserts.doAssertFailure_ (/home/epg/halde/protobuf_test/node_modules/google-protobuf/google-protobuf.js:63:126)
    at Object.goog.asserts.assert [as assert] (/home/epg/halde/protobuf_test/node_modules/google-protobuf/google-protobuf.js:63:385)
    at jspb.BinaryReader.readPackedField_ (/home/epg/halde/protobuf_test/node_modules/google-protobuf/google-protobuf.js:243:71)
    at jspb.BinaryReader.readPackedInt32 (/home/epg/halde/protobuf_test/node_modules/google-protobuf/google-protobuf.js:243:357)
    at Function.proto.test.Test2Int32RepPackedTrue.deserializeBinaryFromReader (/home/epg/halde/protobuf_test/test/proto2_test_pb.js:280:58)
    at Function.proto.test.Test2Int32RepPackedTrue.deserializeBinary (/home/epg/halde/protobuf_test/test/proto2_test_pb.js:261:45)
    at Object.<anonymous> (/home/epg/halde/protobuf_test/packed_test.js:17:17)
    at Module._compile (module.js:541:32)
    at Object.Module._extensions..js (module.js:550:10)

[cygery]

I had a look at the generators for C++ and JavaScript. Based on that I assume that patching the JS generator to create the following code for repeated fields should be sufficient, however, being not familiar enough with Protobuf I can't tell for sure.

The current generator creates the following two code snippets in deserializeBinaryFromReader for packed/not packed repeated fields, respectively:

Packed:

    case 1:
      var value = /** @type {!Array.<number>} */ (reader.readPackedInt32());
      msg.setRepeatedInt32List(value);
      break;

Not packed:

    case 1:
      var value = /** @type {number} */ (reader.readInt32());
      msg.getRepeatedInt32List().push(value);
      msg.setRepeatedInt32List(msg.getRepeatedInt32List());
      break;

Merging these two and selecting the one to use based on the wire type appears sufficient to me:

    case 1:
      var wireType = reader.getWireType();
      if (wireType == jspb.BinaryConstants.WireType.VARINT) {
          var value = /** @type {number} */ (reader.readInt32());
          msg.getRepeatedInt32List().push(value);
          msg.setRepeatedInt32List(msg.getRepeatedInt32List());
      } else if (wireType == jspb.BinaryConstants.WireType.DELIMITED) {
          var value = /** @type {!Array.<number>} */ (reader.readPackedInt32());
          msg.setRepeatedInt32List(msg.getRepeatedInt32List().concat(value));
      }
      break;

This should correctly concatenate any combinations of (multiple) occurring packed and not packed fields.
Additionally, this approach requires jspb.BinaryConstants to be exported.

Any feedback appreciated :)

[haberman]

This appears to still be a bug. PR's welcome. I'm not sure when we will get to it.

[ArvoGuo]

Any progress?

[TeBoring]

Added conformance test, but haven’t fixed it yet.

…

On Mon, Jul 29, 2019 at 21:00 ArvoGuo ***@***.***> wrote: Any progress? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#1701?email_source=notifications&email_token=ABHUPZNABQI66ZWPMNIZNRTQB64FXA5CNFSM4CHKKWB2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD3CV4DY#issuecomment-516251151>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABHUPZMPTZXYFUHSQWOJG3TQB64FXANCNFSM4CHKKWBQ> .

[qnighy]

I made #7379 to fix this.