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

proto: store extension values according to protobuf data model #746

Merged
merged 1 commit into from Nov 15, 2018

Conversation

Projects
None yet
3 participants
@dsnet
Member

dsnet commented Nov 8, 2018

The current API represents scalar extension fields as *T and
repeated extension fields as []T.
However, this is not an accurate reflection of the protobuf data model.

For scalars, pointers are usually used to represent nullability. However,
in the case of extension scalars, there is no need to do so since presence
information is captured by checking whether the field is in the extension map.
For this reason, presence on extension scalars is not determined by checking
whether the returned pointer is nil, but whether HasExtension reports true.

For repeated fields, using []T means that the returned value is only a partially
mutable value. You can swap out elements, but you cannot change the length of
the original field value. On the other hand, the reflective API provides methods
on repeated field values that permit operations that do change the length.
Thus, using *[]T is a closer match to the protobuf data model.

This CL changes the implementation of extension fields to always store T
for scalars, and *[]T for repeated fields. However, for backwards compatibility,
the API continues to provide *T and []T. In theory, this could break anyone
that relies on memory aliasing for *T. However, this is unlikely since:

  • use of extensions themselves are relatively rare
  • if extensions are used, it is recommended practice to use a message as the
    field extension and not a scalar
  • relying on memory aliasing is generally not a good idiom to follow.
    The expected pattern is to call SetExtension to make it explicit that a mutation
    is happening on the message.
  • analysis within Google demonstrates that no one is relying on this behavior.

@dsnet dsnet requested review from neild and cybrcodr Nov 8, 2018

if err := proto.SetExtension(cloneTestMessage, pb.E_Ext_Text, proto.String("hello")); err != nil {
panic("SetExtension: " + err.Error())
}
if err := proto.SetExtension(cloneTestMessage, pb.E_Greeting, []string{"one", "two"}); err != nil {

This comment has been minimized.

@cybrcodr

cybrcodr Nov 14, 2018

Contributor

Do you also need to verify that clone makes a deep copy on this?

This comment has been minimized.

@dsnet

dsnet Nov 15, 2018

Member

Done.

This comment has been minimized.

@dsnet

dsnet Nov 15, 2018

Member

Done

Show resolved Hide resolved proto/extensions.go
proto: store extension values according to protobuf data model
The current API represents scalar extension fields as *T and
repeated extension fields as []T.
However, this is not an accurate reflection of the protobuf data model.

For scalars, pointers are usually used to represent nullability. However,
in the case of extension scalars, there is no need to do so since presence
information is captured by checking whether the field is in the extension map.
For this reason, presence on extension scalars is not determined by checking
whether the returned pointer is nil, but whether HasExtension reports true.

For repeated fields, using []T means that the returned value is only a partially
mutable value. You can swap out elements, but you cannot change the length of
the original field value. On the other hand, the reflective API provides methods
on repeated field values that permit operations that do change the length.
Thus, using *[]T is a closer match to the protobuf data model.

This CL changes the implementation of extension fields to always store T
for scalars, and *[]T for repeated fields. However, for backwards compatibility,
the API continues to provide *T and []T. In theory, this could break anyone
that relies on memory aliasing for *T. However, this is unlikely since:
* use of extensions themselves are relatively rare
* if extensions are used, it is recommended practice to use a message as the
field extension and not a scalar
* relying on memory aliasing is generally not a good idiom to follow.
The expected pattern is to call SetExtension to make it explicit that a mutation
is happening on the message.
* analysis within Google demonstrates that no one is relying on this behavior.
@neild

neild approved these changes Nov 15, 2018

@dsnet dsnet merged commit 5213254 into master Nov 15, 2018

3 checks passed

cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@dsnet dsnet deleted the extension branch Nov 15, 2018

dsnet added a commit that referenced this pull request Nov 15, 2018

internal/impl: remove hack in legacy_extension_hack.go
This hack has now been upstreamed in the v1 codebase;
there is no longer any need to maintain the code in v2.
See #746

Change-Id: I2c59d11303f5465e65190d893c422c088c647691
Reviewed-on: https://go-review.googlesource.com/c/149659
Reviewed-by: Herbie Ong <herbie@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment