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

Add github.com/golang/protobuf/proto.MessageV2 equivalent to google.golang.org/protobuf #1442

Closed
dfawley opened this issue May 3, 2022 · 13 comments

Comments

@dfawley
Copy link
Contributor

dfawley commented May 3, 2022

Also we need a protoiface.MessageV1 that is allowed to be referenced by outside packages.

Background: we have an API that accepts v1 proto.Messages as a parameter. We would like to stop importing the old github.com/golang/protobuf since it is deprecated and our users keep complaining about it.

@dsnet suggested the proto team might be interested in adding this in grpc/grpc-go#4330 (comment).

@dfawley
Copy link
Contributor Author

dfawley commented May 10, 2022

@neild what are your thoughts on this?

@neild
Copy link
Contributor

neild commented May 10, 2022

What would the use case be for a "github.com/golang/protobuf/proto".MessageV2? You can just reference "google.golang.org/protobuf/proto".Message directly. This won't add any new transitive dependencies, since "github.com/golang/protobuf/proto" already depends on "google.golang.org/protobuf/proto".

Defining a public version of MessageV1 somewhere in the "google.golang.org/protobuf" module seems reasonable to me, although I'm not sure what the best place for it would be. A new package? (protocompat? Something under types/?) Maybe we should just say users can reference the one in protoiface if they need it, although that would be unfortunate from an API cleanliness standpoint.

If gRPC wants to refer to protoiface.MessageV1 directly while this is being worked out, that package does have a stable API.

@dfawley
Copy link
Contributor Author

dfawley commented May 10, 2022

What would the use case be for a "github.com/golang/protobuf/proto".MessageV2? You can just reference "google.golang.org/protobuf/proto".Message directly.

github.com/golang/protobuf/proto.MessageV2 is a conversion function that turns a proto v1 message into a proto v2 message.

The goal here is to stop importing github.com/golang/protobuf/... from anywhere our library, since it is marked as deprecated.

If gRPC wants to refer to protoiface.MessageV1 directly while this is being worked out, that package does have a stable API.

This wording from pkg.go.dev was scary to me:

WARNING: This package should only be imported by message implementations. The functionality found in this package should be accessed through higher-level abstractions provided by the proto package.

I don't think a temporary solution is necessary here as long as there's buy-in for a more permanent option. We can just wait longer to stop referencing the deprecated-but-can-never-be-deleted github.com/golang/protobuf/... packages.

@neild
Copy link
Contributor

neild commented May 10, 2022

Sorry, I was confusing the MessageV2 function with an alias to the Message type.

It might make sense to add a new package to google.golang.org/protobuf that contains the type conversion functions. I believe the implementations of these functions are all in the new module anyway, so this shouldn't change the module dependency graph. I don't know what the new package would be named; google.golang.org/protoconvert maybe? It's a shame that google.golang.org/runtime/protolegacy took the protolegacy name.

@dsnet
Copy link
Member

dsnet commented May 10, 2022

google.golang.org/protobuf/protoadapt?

@puellanivis
Copy link
Collaborator

I also liked the proposed protocompat

@dfawley
Copy link
Contributor Author

dfawley commented May 12, 2022

It's a shame that google.golang.org/runtime/protolegacy took the protolegacy name.

github.com/golang/protobuf/v2/runtime/protolegacy's documentation makes it sound like it's effectively an internal-only package, so I don't personally see any harm in reusing the name for a public-facing package if that's the best name to use here.

Maybe the package name will be easier to choose if we define the symbols it will contain, and see how it will look when used?

package protoadapt / protocompat / protoconvert / protolegacy

type MessageV1 = protoiface.MessageV1

func ToMessageV2(MessageV1 /* or GeneratedMessage to mirror proto.MessageV2()? */) proto.Message

E.g.

func foo(v1 protoadapt.MessageV1) {
	v2 := protoadapt.ToMessageV2(v1)
	// ...
}

@dfawley
Copy link
Contributor Author

dfawley commented Oct 25, 2022

We have an increasing number of users (see new contributed PR to migrate to the newer proto packages grpc/grpc-go#5740) impacted by this. Could this be prioritized?

@stpabhi
Copy link

stpabhi commented Nov 11, 2022

@dsnet I'm happy to send a change for the same via Gerrit.

@stpabhi
Copy link

stpabhi commented Nov 11, 2022

Here's the draft change to review.

stpabhi added a commit to stpabhi/protobuf-go that referenced this issue Nov 15, 2022
…1 or v2 message.

Fixes golang/protobuf#1442

Change-Id: I7ec60982be81d5dc060094ccec51d819b17a3a8f
@BenjaminNolan
Copy link

Is there more needed for grpc/credentials to be upgraded, or is it just the code that @stpabhi has provided in the PR above? If it's just this code, can someone please merge this in.

stpabhi added a commit to stpabhi/protobuf-go that referenced this issue Mar 14, 2023
… or v2 message.

Fixes golang/protobuf#1442

Change-Id: I7ec60982be81d5dc060094ccec51d819b17a3a8f
@chikafred
Copy link

please, how do I solve this issue:

go: module github.com/golang/protobuf is deprecated: Use the "google.golang.org/protobuf" module instead.

@dsnet
Copy link
Member

dsnet commented Apr 22, 2023

Take a look at https://pkg.go.dev/google.golang.org/protobuf/protoadapt, but you'll need v1.30.0 of the module

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

7 participants