Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Complex options cannot be parsed #50

Closed
bufdev opened this issue Jan 3, 2018 · 7 comments
Closed

Complex options cannot be parsed #50

bufdev opened this issue Jan 3, 2018 · 7 comments

Comments

@bufdev
Copy link
Contributor

bufdev commented Jan 3, 2018

I think the logic in Option.parseAggregate is not complicated enough - it assumes simple key/value for options. However, the following is a valid option:

syntax = "proto3";

import "google/protobuf/descriptor.proto";

package foo;

message Foo {
  int64 hello = 1;
}

message Bar {
  int64 woot = 1;
  Foo foo = 2;
}

extend google.protobuf.FileOptions {
  Bar bar = 80001;
}

Then you can have:

syntax = "proto3";

package baz;

option (foo.bar) = {
  woot: 100
  foo: {
    hello: 200
  }
};

This is completely valid, but due to the lack of recursion in parseAggregate, and the issue that it only stops on a tSEMICOLON (see #49), this will not be parsed.

@bufdev
Copy link
Contributor Author

bufdev commented Feb 27, 2018

There is actually an alternate syntax without the ':' after 'foo' that is also valid in Protobuf:

syntax = "proto3";

package baz;

option (foo.bar) = {
  woot: 100
  foo {
    hello: 200
  }
};

See https://github.com/googleapis/googleapis/blob/master/google/privacy/dlp/v2beta2/dlp.proto#L96

Here's a diff on service_test.go that breaks:

$ git diff
diff --git a/service_test.go b/service_test.go
index 058c885..0a4fc5d 100644
--- a/service_test.go
+++ b/service_test.go
@@ -86,6 +86,10 @@ func TestRPCWithOptionAggregateSyntax(t *testing.T) {
                        option (test_ident) = {
                                test: "test"
                                test2:"test2"
+                               addtional_bindings {
+                                       hello: "value"
+                                       hello2: "value2"
+                               }
                        }; // inline test_ident
                }
        }`

This results in found "{" but expected [option aggregate key colon :]

@emicklei
Copy link
Owner

ATM aggregations are stored in AggregatedConstants. With this recursive example, this is no longer valid. Need to think about this a bit more

@emicklei
Copy link
Owner

in the PR, Issue #60 , the constants are flattened.
in your example given, there will be a constant called foo.hello

@bufdev
Copy link
Contributor Author

bufdev commented Feb 27, 2018

This won't work for other arbitrary cases. For example, what if I have a repeated field in the message? so foo.hello is a repeated string? What if it has a oneof, etc?

This might be fine to start, but just of note :-)

emicklei added a commit that referenced this issue Feb 28, 2018
* allow nested aggregated constants
@bufdev
Copy link
Contributor Author

bufdev commented Mar 2, 2018

This is still broken, will write a test.

@bufdev
Copy link
Contributor Author

bufdev commented Mar 2, 2018

See the PR I just wrote, two things need to be handled:

  1. Having a : after the key before a complex option
  2. Having the value be on a separate line

@emicklei
Copy link
Owner

emicklei commented Mar 4, 2018

closing this as #60 is merged

@emicklei emicklei closed this as completed Mar 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants