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

proto: deprecate {Unm,M}arshalMessageSet{JSON} #741

Merged
merged 1 commit into from Nov 8, 2018
Merged

Conversation

dsnet
Copy link
Member

@dsnet dsnet commented Oct 31, 2018

Despite the naming, these are "internal-only" functions that are intended to
only be called from generated code for MessageSets.
Furthermore, MessageSets are themselves a deprecated feature from proto1 days,
such that descriptor.proto even warns against their use since the initial
open-source release of protocol buffers in 2008. Within Google,
there are no direct usages of these functions, and all existing
usages of MessageSets go through the new table-driven implementation.

In addition to deprecating the {Unm,M}arshalMessageSet{JSON} top-level functions,
also modify the generator to stop emitting MarshalJSON and UnmarshalJSON methods
for messages sets. The UnmarshalJSON method is not implemented and the
MarshalJSON method does not seem to be called anywhere in Google (verified by
making the method panic and doing a global test). The jsonpb package continues
to work with MessageSets.

I should note that when the table-driven implementation was open sourced
in late 2017 (see 8cc9e46), it accidentally
removed generation of the Marshal and Unmarshal method. However, no one seemed
to have noticed that those methods were no longer generated.

@dsnet dsnet requested a review from neild October 31, 2018 20:12
@dsnet
Copy link
Member Author

dsnet commented Oct 31, 2018

According to a quick GitHub code search, there are no known external MessageSet messages:

https://github.com/search?l=Go&q=MarshalMessageSet+NOT+extMap+NOT+messageSetDesc+NOT+Request_Color_value+NOT+GenerateAlias&type=Code

@dsnet
Copy link
Member Author

dsnet commented Oct 31, 2018

\cc @jmarais @donaldgraham (maintainers over at gogo/protobuf).

Let me know if you have reason to believe people depend on these. MessageSets were already deprecated upon the first open source release of protobufs in 2008 and I'm aware of zero real usages externally.

Despite the naming, these are "internal-only" functions that are intended to
only be called from generated code for MessageSets.
Furthermore, MessageSets are themselves a deprecated feature from proto1 days,
such that descriptor.proto even warns against their use since the initial
open-source release of protocol buffers in 2008. Within Google,
there are no direct usages of these functions, and all existing
usages of MessageSets go through the new table-driven implementation.

In addition to deprecating the {Unm,M}arshalMessageSet{JSON} top-level functions,
also modify the generator to stop emitting MarshalJSON and UnmarshalJSON methods
for messages sets. The UnmarshalJSON method is not implemented and the
MarshalJSON method does not seem to be called anywhere in Google (verified by
making the method panic and doing a global test). The jsonpb package continues
to work with MessageSets.

I should note that when the table-driven implementation was open sourced
in late 2017 (see 8cc9e46), it accidentally
removed generation of the Marshal and Unmarshal method. However, no one seemed
to have noticed that those methods were no longer generated.
@dsnet dsnet merged commit 951a149 into master Nov 8, 2018
@dsnet dsnet deleted the deprecate-messageset branch November 8, 2018 17:20
dsnet added a commit that referenced this pull request Nov 27, 2018
Despite the naming, these are "internal-only" functions that are intended to
only be called from generated code for MessageSets.
Furthermore, MessageSets are themselves a deprecated feature from proto1 days,
such that descriptor.proto even warns against their use since the initial
open-source release of protocol buffers in 2008. Within Google,
there are no direct usages of these functions, and all existing
usages of MessageSets go through the new table-driven implementation.

In addition to deprecating the {Unm,M}arshalMessageSet{JSON} top-level functions,
also modify the generator to stop emitting MarshalJSON and UnmarshalJSON methods
for messages sets. The UnmarshalJSON method is not implemented and the
MarshalJSON method does not seem to be called anywhere in Google (verified by
making the method panic and doing a global test). The jsonpb package continues
to work with MessageSets.

I should note that when the table-driven implementation was open sourced
in late 2017 (see 8cc9e46), it accidentally
removed generation of the Marshal and Unmarshal method. However, no one seemed
to have noticed that those methods were no longer generated.

Change-Id: I5fa3ddc452ff1906a4d7c31c6e0a2902702784ca
Cherry-Pick: github.com/golang/protobuf@951a149f90371fb8858c6c979d03bb2583611052
Original-Author: Joe Tsai <joetsai@digital-static.net>
Reviewed-on: https://go-review.googlesource.com/c/151435
Reviewed-by: Damien Neil <dneil@google.com>
thaJeztah added a commit to thaJeztah/swarmkit that referenced this pull request Oct 20, 2019
full diff: golang/protobuf@v1.2.0...v1.3.2

protobuf v1.3.2:

- golang/protobuf#785: grpc code generation: add an UnimplementedServer type implementing each server interface, returning an unimplemented error for each method
- golang/protobuf#851: convert prints to os.Stderr to use log.Printf
- golang/protobuf#883: jsonpb: fix marshaling of Duration with negative nanoseconds

protobuf v1.3.1:

- The set of dependencies specified in go.mod has now been reduced to only the standard library.

protobuf v1.3.0:

- golang/protobuf#699: add a go.mod module file
- golang/protobuf#701: stop generating package "// import" comment
- golang/protobuf#741: deprecate {Unm,M}arshalMessageSet{JSON}
- golang/protobuf#760: different internal implementation of oneofs
    - `.pb.go` files generated by protoc-gen-go@v1.3.0 will require the proto@v1.3.0 package to work
- various minor changes to code generation

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@golang golang locked and limited conversation to collaborators Jun 26, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants