Protobuf is not using correct packed option value when the syntax is set as proto2

[serkangunes]

protobuf.js version: 6.6.3

Even with the syntax = "proto2" library still packs the repeated enum fields.

TestMessage.proto

syntax = "proto2";

package test;

import "TestMessageFlag.proto";

option java_outer_classname = "TestMessageProto";

message TestMessage {
  repeated test.TestMessageFlag flags = 1;
}

TestMessageFlag.proto

syntax = "proto2";

package test;

enum TestMessageFlag {
  FLAG_1 = 1;
  FLAG_2 = 2;
  FLAG_3 = 3;
  FLAG_4 = 4;
  FLAG_5 = 5;
} 

main node js file.

var protobuf = require("protobufjs");

var proto = require('./TestProtoModule');

protobuf.load("proto/TestMessage.proto", function(err, root) {
    if (err) throw err;

    protobuf.load("proto/TestMessageFlag.proto", function(err2, root2) {
      if (err2) throw err2;

      var TestMessageFlag = root2.lookup("test.TestMessageFlag");
      var TestMessage = root.lookup("test.TestMessage");

      // Create a new message
      var message = TestMessage.create({ flags: [
        TestMessageFlag.values.FLAG_4,
        TestMessageFlag.values.FLAG_2,
        TestMessageFlag.values.FLAG_3,
        TestMessageFlag.values.FLAG_1
      ] });

      // Encode a message
      var buffer = TestMessage.encode(message).finish();
      console.log(buffer)
    });
});

Run the code and it outputs: "<Buffer 0a 04 04 02 03 01>"

Now change the field to be unpacked with an option.

syntax = "proto2";

package test;

import "TestMessageFlag.proto";

option java_outer_classname = "TestMessageProto";

message TestMessage {
  repeated test.TestMessageFlag flags = 1 [packed = false];
}

Now run the same code and it outputs: <Buffer 08 04 08 02 08 03 08 01>
Which is the correct value for unpacked repeated field.

syntax = "proto2" should by default use unpacked without the need for options.

[dcodeIO]

I wonder how your example is even working with two different root instances.

protobuf.load(["proto/TestMessage.proto", "proto/TestMessageFlag.proto"], function(err, root) {
    ...

[serkangunes]

I found the issue and submitted a pull request. Can you please review it? Using one root is exactly the same.

#683

[dcodeIO]

For context: Only repeated enum fields, which also use varint encoding, are affected, because their type is not yet (reliably) known when parsing.

Your PR fixes this by setting packed=X for all kinds of repeated fields, even those (not) using packed encoding. Not quite ideal, will have to think about that. Might be necessary to add a syntax property to message types, so this can be determined properly at runtime.

[serkangunes]

The fix only sets the flag when iProto3 is false which means we are using syntax="proto2" therefore if the packed option is undefined the default value should be false. It shouldn't matter if it is an enum or not as far as I know.

[dcodeIO]

See: #683 (comment)

[serkangunes]

Shall I close this issue now? When is it going to be released to npm?

[dcodeIO]

It's on npm now as 6.6.4.

[serkangunes]

Thanks a lot.