feat: add feature resolution for protobuf editions #2029

sofisl · 2024-09-10T23:22:12Z

No description provided.

tests/feature_resolution_editions.js

src/parse.js

src/object.js

tests/feature_resolution_editions.js

….js into protobufEditionsWork

src/parse.js

sofisl · 2024-09-19T18:46:00Z

tests/comp_options.js

@@ -30,7 +30,7 @@ message Message {
 tape.test("complex options", function (test) {
  var root = protobuf.parse(proto).root;

-  test.deepEqual(root.parsedOptions[0], {
+  test.deepEqual(root.parsedOptions[1], {


Since we now have defaults as the first parsed option, this test needs to look for the second parsed option.

Do we need to put defaults into "parsedOptions"? That seems like the wrong place to store them, since they're global

parsedOptions are just options that have been properly parsed - in other words, a step after options. Since features are just options, and they need to be parsed just like options, they do need to go through this step. If you want I can delete them from options after they've been copied over into _features.

My confusion is why defaults end up here. Parsed features make sense to me, but you're explicitly adding edition defaults to parsedOptions

Gotcha - yeah it seems it is related. I think the point is that we're taking advantage of the parsing setParsedOptions offers, which ultimately is needed in feature resolution. If we move it outside of setting parsedOptions into some other feature resolution-specific method, we're essentially copying over reusable code. Same reason why we're caling setParsedOption here too, since it takes advantage of all the parsing already offered in that method.

tests/api_object.js

mkruskal-google · 2024-09-23T20:45:06Z

src/object.js

@@ -186,6 +207,7 @@ ReflectionObject.prototype.setParsedOption = function setParsedOption(name, valu
    if (!this.parsedOptions) {
        this.parsedOptions = [];
    }
+    var isFeature = /features/.test(name);


This looks like it might have some false positive. Do we want to make sure that it starts with features.?

I think you're still missing an ., this would pass for an option named featuresFoo

mkruskal-google · 2024-09-23T20:45:44Z

src/object.js

@@ -195,12 +217,13 @@ ReflectionObject.prototype.setParsedOption = function setParsedOption(name, valu
        });
        if (opt) {
            // If we found an existing option - just merge the property value
+            // (If it's a feature, will just write over)


Why do we need to special-case features? They should all be scalar so we should never be trying to merge during parse

This feature really only gets called when parsing Root, since root gets initialized with defaults, and then can be overwritten if there's options specified at the root level. For example, if we start off with editions syntax, and then immediately add the following at the root level:

option features.json_format = LEGACY_BEST_EFFORT;

Since the previous experience is to merge at each child, then the features-specific behavior gets lost, since we'd end up with an array of values (features.json_format = [LEGACY_BEST_EFFORT, ALLOW]);

One option is to simply initialize defaults at the time that we set root-level options, so there's no overwriting, just 'filling in the blanks'. However this would require much of the same logic that setParsedOption already does (i.e., searching for a property within the_proto_features object), and it seems pretty un-DRY to rewrite it for features only because the setProperty value is different.

Instead, I'll change the isFeature parameter to overwrite in setProperty to make it more general.

This sounds related to my confusion about calling setParsedOption with defaults. Alternatively, you could just initialize _features of the root to the relevant defaults during feature resolution right? i.e. if _parent is null, initialize with the edition defaults

Right, but what if parent isn't null? In that case we're rewriting a lot of the code that is in setParsedOption to confirm whether or not a value exists and if to rewrite it, or to create a new property, etc.

src/object.js

src/parse.js

mkruskal-google · 2024-09-23T20:55:42Z

src/parse.js

@@ -338,13 +371,17 @@ function parse(source, root, options) {

                case "required":
                case "repeated":
+                    if (edition)
+                        throw illegal(token);
                    parseField(type, token);
                    break;

                case "optional":


In addition to optional and required, we have the following grammar changes I don't see covered:

"group" keyword is removed

reserved field and enum value names are specified as identifiers, rather than string literals (i.e. no quotes)

Added tests for the grammar changes. As for the second point, I believe this is already covered by feature resolution tests since there are no quotes.

I don't think that second case is covered, since you have no reserved fields. What I'm talking about is that for a proto2 message:

message Foo { reserved "bar", "baz"; }

becomes this in edition 2023:

message Foo { reserved bar, baz; }

Added tests.

mkruskal-google · 2024-09-23T20:56:28Z

src/parse.js

+        dummy.setParsedOption = function(name, value, propName) {
+            // In order to not change existing behavior, only calling
+            // this for features
+            if (/features/.test(name)) {


Similar issue with this regex I think

tests/api_object.js

mkruskal-google · 2024-09-23T20:58:49Z

tests/comp_options.js

@@ -30,7 +30,7 @@ message Message {
 tape.test("complex options", function (test) {
  var root = protobuf.parse(proto).root;

-  test.deepEqual(root.parsedOptions[0], {
+  test.deepEqual(root.parsedOptions[1], {


Do we need to put defaults into "parsedOptions"? That seems like the wrong place to store them, since they're global

mkruskal-google · 2024-09-23T21:01:07Z

tests/feature_resolution_editions.js

+// Tests precedence for different levels of feature resolution
+tape.test("feature resolution editions precedence", function(test) {
+
+    protobuf.load("tests/data/feature-resolution.proto", function(err, root) {


I think it might be helpful to split up this proto into smaller ones, and define them inline per-test. I regret using monolithic protos in our tests, as it's much harder to understand what's being tests

I hear you, but I think in this case it helps the test reader understand the overall precedence of options - at least it does for me. It's clear by the alphabetical order what has precedence in the grand scheme of things, rather than having to piece it together in each test. As long as it doesn't get used for other development, I think it's helpful to get the greater overall picture of precedence.

The problem is that to understand this test you need to constantly be switching back and forth between two different files (as a reader, I find this test difficult to review for correctness). If you had much smaller protos defined inline in each test, it would be much clearer what the expectation is

OK, switched it.

mkruskal-google · 2024-09-24T23:40:54Z

src/object.js

-    if (this.root instanceof Root)
-        this.resolved = true; // only if part of a root
+    this._resolveFeatures();
+    if (this.root instanceof Root || this.parent)


Is this sufficient?

We probably don't want to call _resolveFeatures() unless this condition is true

What if the parent is itself orphaned?

Moved _resolveFeatures, I think you're right.

If the parent itself is orphaned, then this wouldn't get called recursively since this usually only gets called under resolveAll.

mkruskal-google · 2024-09-24T23:42:28Z

src/object.js

@@ -186,6 +207,7 @@ ReflectionObject.prototype.setParsedOption = function setParsedOption(name, valu
    if (!this.parsedOptions) {
        this.parsedOptions = [];
    }
+    var isFeature = /features/.test(name);


I think you're still missing an ., this would pass for an option named featuresFoo

mkruskal-google · 2024-09-24T23:45:10Z

src/object.js

@@ -195,12 +217,13 @@ ReflectionObject.prototype.setParsedOption = function setParsedOption(name, valu
        });
        if (opt) {
            // If we found an existing option - just merge the property value
+            // (If it's a feature, will just write over)


This sounds related to my confusion about calling setParsedOption with defaults. Alternatively, you could just initialize _features of the root to the relevant defaults during feature resolution right? i.e. if _parent is null, initialize with the edition defaults

sofisl added 7 commits August 23, 2024 16:31

feat: api_converters_editions tests added and run successfully"

b4b5ca4

remove logging

d8eb1b4

feat: add feature resolution for protobuf editions

65d3ed1

cleanup PR

0a7bbd1

see diff

68e4480

fix broken tests

759cc45

add tests

8dabd5e

sofisl requested a review from mkruskal-google September 10, 2024 23:22

mkruskal-google reviewed Sep 10, 2024

View reviewed changes

sofisl added 11 commits September 13, 2024 18:55

feat: add feature resolution and tests

68b5339

undo lockfile update

dae1b3e

add comment

f204288

cleanup

f0491be

remove only

d9c2ec4

save for now

b802c42

undo bundle changes

73590cc

revert package-lock changes

a70c6a4

feat: add feature resolution

a9ffc8a

revert package-lock updates/

43a703a

cleanup

2fabe04

sofisl requested a review from mkruskal-google September 19, 2024 16:39

sofisl and others added 5 commits September 19, 2024 11:21

chore: run lint

1bd72ff

Merge branch 'master' into protobufEditionsWork

5d35287

typo

225fddb

Merge branch 'protobufEditionsWork' of github.com:protobufjs/protobuf…

840907e

….js into protobufEditionsWork

typo

43183bd

sofisl commented Sep 19, 2024

View reviewed changes

sofisl commented Sep 20, 2024

View reviewed changes

tests/api_object.js Outdated Show resolved Hide resolved

Update api_enum.js

c4c486d

mkruskal-google reviewed Sep 23, 2024

View reviewed changes

respond to comments

1ca7586

sofisl requested a review from mkruskal-google September 24, 2024 18:46

sofisl added 4 commits September 24, 2024 13:22

chore: add grammar tests

e73bb3c

run lint

e1abb31

address comments

10dae84

remove comment

087a33e

mkruskal-google reviewed Sep 24, 2024

View reviewed changes

sofisl requested a review from mkruskal-google September 25, 2024 00:31

sofisl added 4 commits September 26, 2024 10:42

respond to comments

608009b

fix test

062caa5

add default feature resolution at the end

c7cee06

run lint

da2df8a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add feature resolution for protobuf editions #2029

feat: add feature resolution for protobuf editions #2029

sofisl commented Sep 10, 2024

sofisl Sep 19, 2024

mkruskal-google Sep 23, 2024

sofisl Sep 24, 2024

mkruskal-google Sep 24, 2024

sofisl Sep 25, 2024

mkruskal-google Sep 23, 2024

mkruskal-google Sep 24, 2024

mkruskal-google Sep 23, 2024

sofisl Sep 24, 2024 •

edited

Loading

mkruskal-google Sep 24, 2024

sofisl Sep 25, 2024

mkruskal-google Sep 23, 2024

sofisl Sep 24, 2024

mkruskal-google Sep 24, 2024

sofisl Sep 26, 2024

mkruskal-google Sep 23, 2024

mkruskal-google Sep 23, 2024

mkruskal-google Sep 23, 2024

sofisl Sep 24, 2024 •

edited

Loading

mkruskal-google Sep 24, 2024 •

edited

Loading

sofisl Sep 26, 2024

mkruskal-google Sep 24, 2024

sofisl Sep 25, 2024

mkruskal-google Sep 24, 2024

mkruskal-google Sep 24, 2024

feat: add feature resolution for protobuf editions #2029

Are you sure you want to change the base?

feat: add feature resolution for protobuf editions #2029

Conversation

sofisl commented Sep 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sofisl Sep 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sofisl Sep 24, 2024 • edited Loading

Choose a reason for hiding this comment

mkruskal-google Sep 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sofisl Sep 24, 2024 •

edited

Loading

sofisl Sep 24, 2024 •

edited

Loading

mkruskal-google Sep 24, 2024 •

edited

Loading