Skip to content

Fixed extends #1202

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fixed extends #1202

wants to merge 1 commit into from

Conversation

jroper
Copy link
Contributor

@jroper jroper commented Apr 16, 2019

Extended fields were being named using the full name, which meant they always had a dot before them, eg:

message A {
  extensions 1000 to max;
}

extends A {
  optional string extended = 1000;
}

would decode to:

{
  ".extended": "value"
}

This fixes it, using the fields name, rather than the fields full name.

@JustinBeckwith
Copy link
Contributor

Thanks for this fix @jroper! We had a little issue when this PR was submitted, and CI did not get a chance to run. Would you mind rebasing and pushing again to trigger Travis? Thank you!

Extended fields were being named using the full name, which meant they
always had a dot before them, eg:

message A {
  extensions 1000 to max;
}

extends A {
  optional string extended = 1000;
}

would decode to:

{
  ".extended": "value"
}

This fixes it, using the fields name, rather than the fields full name.
@jroper
Copy link
Contributor Author

jroper commented Apr 22, 2019

Actually, I'm now reconsidering whether this change is right... I'm not sure. I don't think it should be always prefixed with a dot, but should it always contain the namespace? Possibly? Maybe if the extension comes from another file (package)? I don't know.

It gets even more complex when considering extensions for options fields in protobuf, eg:

package com.example;
extends google.protobuf.FieldOptions {
  optional string my_extension;
}

When this actually gets used, it has to be fully qualified:

message A {
  optional string my_string [(com.example.my_extension) = "foo"];
}

Maybe that's a different thing and should be treated differently. Not sure. Anyway, I've rebased as requested.

@dcodeIO
Copy link
Member

dcodeIO commented Apr 22, 2019

Using the full name is actually intended btw, because extended field names are not unique. There might be multiple extensions using the same field name but differing ids. The official API solves this by getting extended field values via the extension, while we are solving it by using the full name. As such, this is not a fix but breaks it.

@jroper
Copy link
Contributor Author

jroper commented Apr 23, 2019

@dcodeIO but should it be prepended with a .? That was the thing that surprised me, I can understand it being fully qualified, but to my knowledge there's no precedent for using a naming convention anywhere of preceding a fully qualified name with a dot, there's no documentation about it, it's completely surprising.

@dcodeIO
Copy link
Member

dcodeIO commented Apr 23, 2019

The leading dot is mostly a mechanism to tell a resolve step to start at the root, so .Foo and Foo can resolve to different things if there's a Foo sibling and a Foo top-level. Iirc I didn't make this up but it's indeed somewhere in the official implementation. Ofc the leading dot could be stripped for looks, but that'll most likely involve a bit of substring here and concat there when resolving - not sure if that's worth it.

Edit, found it: https://developers.google.com/protocol-buffers/docs/proto#packages-and-name-resolution

A leading '.' (for example, .foo.bar.Baz) means to start from the outermost scope instead.

@nicolasnoble
Copy link
Member

We can try and clarify this with the protobuf team, but it looks to me that the current behavior is correct.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants