Hook up repeated field encoding feature

mkruskal-google · mkruskal-google · commit 785416fd2b98 · 2025-03-19T23:00:33.000-07:00
diff --git a/src/field.js b/src/field.js
@@ -183,13 +183,6 @@ function Field(name, id, type, rule, extend, options, comment) {
      */
     this.declaringField = null;
 
-    /**
-     * Internally remembers whether this field is packed.
-     * @type {boolean|null}
-     * @private
-     */
-    this._packed = null;
-
     /**
      * Comment for this field.
      * @type {string|null}
@@ -198,26 +191,21 @@ function Field(name, id, type, rule, extend, options, comment) {
 }
 
 /**
- * Determines whether this field is packed. Only relevant when repeated and working with proto2.
+ * Determines whether this field is packed. Only relevant when repeated.
  * @name Field#packed
  * @type {boolean}
  * @readonly
  */
 Object.defineProperty(Field.prototype, "packed", {
     get: function() {
-        // defaults to packed=true if not explicity set to false
-        if (this._packed === null)
-            this._packed = this.getOption("packed") !== false;
-        return this._packed;
+        return this._features.repeated_field_encoding === "PACKED";
     }
 });
 
 /**
  * @override
  */
 Field.prototype.setOption = function setOption(name, value, ifNotSet) {
-    if (name === "packed") // clear cached before setting
-        this._packed = null;
     return ReflectionObject.prototype.setOption.call(this, name, value, ifNotSet);
 };
 
@@ -284,7 +272,7 @@ Field.prototype.resolve = function resolve() {
 
     // remove unnecessary options
     if (this.options) {
-        if (this.options.packed === true || this.options.packed !== undefined && this.resolvedType && !(this.resolvedType instanceof Enum))
+        if (this.options.packed !== undefined && this.resolvedType && !(this.resolvedType instanceof Enum))
             delete this.options.packed;
         if (!Object.keys(this.options).length)
             this.options = undefined;
@@ -319,16 +307,27 @@ Field.prototype.resolve = function resolve() {
     if (this.parent instanceof Type)
         this.parent.ctor.prototype[this.name] = this.defaultValue;
 
-
-    if (this.parent) {
-        var parentFeaturesCopy = Object.assign({}, this.parent._features);
-        this._features = Object.assign(parentFeaturesCopy, this._protoFeatures || {});
-    } else {
-        this._features = Object.assign({}, this._protoFeatures);
-    }
     return ReflectionObject.prototype.resolve.call(this);
 };
 
+/**
+ * Infers field features from legacy syntax that may have been specified differently.
+ * in older editions.
+ * @param {string|undefined} edition The edition this proto is on, or undefined if pre-editions
+ * @returns {object} The feature values to override
+ */
+Field.prototype._inferLegacyProtoFeatures = function _inferLegacyProtoFeatures(edition) {
+    if (edition) return {};
+
+    var features = {};
+    if (this.getOption("packed") === true) {
+        features.repeated_field_encoding = "PACKED";
+    } else if (this.getOption("packed") === false) {
+        features.repeated_field_encoding = "EXPANDED";
+    }
+    return features;
+}
+
 /**
  * Decorator function as returned by {@link Field.d} and {@link MapField.d} (TypeScript).
  * @typedef FieldDecorator
diff --git a/src/object.js b/src/object.js
@@ -177,31 +177,44 @@ ReflectionObject.prototype.resolve = function resolve() {
 ReflectionObject.prototype._resolveFeatures = function _resolveFeatures() {
     var defaults = {};
 
+    var edition = this.root.getOption("edition");
     if (this instanceof Root) {
         if (this.root.getOption("syntax") === "proto3") {
             defaults = Object.assign({}, proto3Defaults);
-        } else if (this.root.getOption("edition") === "2023") {
+        } else if (edition === "2023") {
             defaults = Object.assign({}, editions2023Defaults);
         } else {
             defaults = Object.assign({}, proto2Defaults);
         }
     }
 
+    var protoFeatures = Object.assign(Object.assign({}, this._protoFeatures), this._inferLegacyProtoFeatures(edition));
+
     if (this instanceof Root) {
-        this._features = Object.assign(defaults, this._protoFeatures || {});
+        this._features = Object.assign(defaults, protoFeatures || {});
     // fields in Oneofs aren't actually children of them, so we have to
     // special-case it
     } else if (this.partOf instanceof OneOf) {
         var lexicalParentFeaturesCopy = Object.assign({}, this.partOf._features);
-        this._features = Object.assign(lexicalParentFeaturesCopy, this._protoFeatures || {});
+        this._features = Object.assign(lexicalParentFeaturesCopy, protoFeatures || {});
     } else if (this.parent) {
         var parentFeaturesCopy = Object.assign({}, this.parent._features);
-        this._features = Object.assign(parentFeaturesCopy, this._protoFeatures || {});
+        this._features = Object.assign(parentFeaturesCopy, protoFeatures || {});
     } else {
-        this._features = Object.assign({}, this._protoFeatures);
+        this._features = Object.assign({}, protoFeatures);
     }
 };
 
+/**
+ * Infers features from legacy syntax that may have been specified differently.
+ * in older editions.
+ * @param {string|undefined} edition The edition this proto is on, or undefined if pre-editions
+ * @returns {object} The feature values to override
+ */
+ReflectionObject.prototype._inferLegacyProtoFeatures = function _inferLegacyProtoFeatures(edition) {
+    return {};
+}
+
 /**
  * Gets an option value.
  * @param {string} name Option name
diff --git a/src/parse.js b/src/parse.js
@@ -460,12 +460,6 @@ function parse(source, root, options) {
         } else {
             parent.add(field);
         }
-
-        // JSON defaults to packed=true if not set so we have to set packed=false explicity when
-        // parsing proto2 descriptors without the option, where applicable. This must be done for
-        // all known packable types and anything that could be an enum (= is not a basic type).
-        if (!isProto3 && field.repeated && (types.packed[type] !== undefined || types.basic[type] === undefined))
-            field.setOption("packed", false, /* ifNotSet */ true);
     }
 
     function parseGroup(parent, rule) {
diff --git a/tests/comp_packed-repeated.js b/tests/comp_packed-repeated.js
@@ -2,28 +2,96 @@ var tape = require("tape");
 
 var protobuf = require("..");
 
-var proto1 = "message Test {\
+var packedOption = "message Test {\
   repeated uint32 a = 1 [packed = true];\
 }";
 
-var proto2 = "message Test {\
+var unpackedOption = "message Test {\
   repeated uint32 a = 1 [packed = false];\
 }";
 
+var packedFeature = "edition = \"2023\";\
+message Test {\
+  repeated uint32 a = 1 [features.repeated_field_encoding = PACKED];\
+}";
+
+var unpackedFeature = "edition = \"2023\";\
+message Test {\
+  repeated uint32 a = 1 [features.repeated_field_encoding = EXPANDED];\
+}";
+
+var regular = "message Test {\
+  repeated uint32 a = 1;\
+}";
+
 var msg = {
     a: [1,2,3]
 };
 
-tape.test("packed repeated values", function(test) {
-    var root1 = protobuf.parse(proto1).root,
-        root2 = protobuf.parse(proto2).root;
+tape.test("packed repeated values roundtrip", function(test) {
+    var root1 = protobuf.parse(packedOption).root.resolveAll(),
+        root2 = protobuf.parse(unpackedOption).root.resolveAll();
     var Test1 = root1.lookup("Test"),
         Test2 = root2.lookup("Test");
-    
+
     var dec1 = Test2.decode(Test1.encode(msg).finish());
     test.same(dec1, msg, "should decode packed even if defined non-packed");
     var dec2 = Test1.decode(Test2.encode(msg).finish());
     test.same(dec2, msg, "should decode non-packed even if defined packed");
 
     test.end();
 });
+
+tape.test("packed repeated values encode", function(top) {
+    [
+        packedOption,
+        `syntax = "proto3";\n` + regular,
+        `syntax = "proto3";\n` + packedOption,
+        `edition = "2023";\n` + regular,
+        packedFeature,
+    ].forEach(function(proto, index) {
+        tape.test("proto " + index, function(test) {
+            var root = protobuf.parse(proto).root.resolveAll();
+            var Test = root.lookup("Test");
+
+            var buf = Test.encode(msg).finish();
+            test.equal(buf.length, 5, "a total of 4 bytes");
+            test.equal(buf[0], 1 << 3 | 2, "id 1, wireType 2");
+            test.equal(buf[1], 3, "length 3");
+            test.equal(buf[2], 1, "element 1");
+            test.equal(buf[3], 2, "element 2");
+            test.equal(buf[4], 3, "element 3");
+
+            test.end();
+        });
+    });
+
+    top.end()
+});
+
+tape.test("packed repeated values encode", function(top) {
+    [
+        unpackedOption,
+        regular,
+        `syntax = "proto3";\n` + unpackedOption,
+        unpackedFeature,
+    ].forEach(function(proto, index) {
+        tape.test("proto " + index, function(test) {
+            var root = protobuf.parse(proto).root.resolveAll();
+            var Test = root.lookup("Test");
+
+            var buf = Test.encode(msg).finish();
+            test.equal(buf.length, 6, "a total of 6 bytes");
+            test.equal(buf[0], 1 << 3 | 0, "id 1, wireType 0");
+            test.equal(buf[1], 1, "element 1");
+            test.equal(buf[2], 1 << 3 | 0, "id 1, wireType 0");
+            test.equal(buf[3], 2, "element 2");
+            test.equal(buf[4], 1 << 3 | 0, "id 1, wireType 0");
+            test.equal(buf[5], 3, "element 3");
+
+            test.end()
+        });
+    });
+
+    top.end()
+});
diff --git a/tests/feature_resolution_editions.js b/tests/feature_resolution_editions.js
@@ -423,16 +423,53 @@ tape.test("feature resolution editions precedence", function(test) {
         if (err)
             throw test.fail(err.message);
 
-    test.same(root.lookup("Message").lookupEnum("SomeEnumInMessage")._features,
-    {
-        enum_type: 'CLOSED',
-        field_presence: 'EXPLICIT',
-        json_format: 'LEGACY_BEST_EFFORT',
-        message_encoding: 'LENGTH_PREFIXED',
-        repeated_field_encoding: 'EXPANDED',
-        utf8_validation: 'NONE',
-        amazing_feature: 'G'
-      })
-    test.end();    
+        test.same(root.lookup("Message").lookupEnum("SomeEnumInMessage")._features,
+        {
+            enum_type: 'CLOSED',
+            field_presence: 'EXPLICIT',
+            json_format: 'LEGACY_BEST_EFFORT',
+            message_encoding: 'LENGTH_PREFIXED',
+            repeated_field_encoding: 'EXPANDED',
+            utf8_validation: 'NONE',
+            amazing_feature: 'G'
+        })
+        test.end();
     });
-})
+});
+
+tape.test("feature resolution inferred proto2 repeated encoding", function(test) {
+    var root = protobuf.parse(`syntax = "proto2";
+    message Message {
+        repeated int32 default = 1;
+        repeated int32 packed = 2 [packed = true];
+        repeated int32 unpacked = 3 [packed = false];
+    }`).root.resolveAll();
+
+    test.notOk(root.lookup("Message").fields.default.packed)
+    test.equal(root.lookup("Message").fields.default._features.repeated_field_encoding, "EXPANDED")
+    test.ok(root.lookup("Message").fields.packed.packed)
+    test.equal(root.lookup("Message").fields.packed._features.repeated_field_encoding, "PACKED")
+    test.notOk(root.lookup("Message").fields.unpacked.packed)
+    test.equal(root.lookup("Message").fields.unpacked._features.repeated_field_encoding, "EXPANDED")
+
+    test.end();
+});
+
+tape.test("feature resolution inferred proto3 repeated encoding", function(test) {
+    var root = protobuf.parse(`syntax = "proto3";
+    message Message {
+        repeated int32 default = 1;
+        repeated int32 packed = 2 [packed = true];
+        repeated int32 unpacked = 3 [packed = false];
+    }`).root.resolveAll();
+
+    test.ok(root.lookup("Message").fields.default.packed)
+    test.equal(root.lookup("Message").fields.default._features.repeated_field_encoding, "PACKED")
+    test.ok(root.lookup("Message").fields.packed.packed)
+    test.equal(root.lookup("Message").fields.packed._features.repeated_field_encoding, "PACKED")
+    test.notOk(root.lookup("Message").fields.unpacked.packed)
+    test.equal(root.lookup("Message").fields.unpacked._features.repeated_field_encoding, "EXPANDED")
+
+    test.end();
+});
+

Original file line number	Diff line number	Diff line change
`@@ -460,12 +460,6 @@ function parse(source, root, options) {`
`460`	`460`	`} else {`
`461`	`461`	`parent.add(field);`
`462`	`462`	`}`
`463`		`-`
`464`		`- // JSON defaults to packed=true if not set so we have to set packed=false explicity when`
`465`		`- // parsing proto2 descriptors without the option, where applicable. This must be done for`
`466`		`- // all known packable types and anything that could be an enum (= is not a basic type).`
`467`		`- if (!isProto3 && field.repeated && (types.packed[type] !== undefined \|\| types.basic[type] === undefined))`
`468`		`- field.setOption("packed", false, /* ifNotSet */ true);`
`469`	`463`	`}`
`470`	`464`
`471`	`465`	`function parseGroup(parent, rule) {`