Added UnimplementedServer for server interface by prannayk · Pull Request #785 · golang/protobuf

prannayk · 2019-01-15T12:23:21Z

Solves the issues raised in #784 @dfawley

googlebot · 2019-01-15T12:23:25Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

googlebot · 2019-01-15T12:23:25Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

prannayk · 2019-01-15T12:24:55Z

I signed it!

googlebot · 2019-01-15T12:24:58Z

CLAs look good, thanks!

googlebot · 2019-01-15T12:24:59Z

CLAs look good, thanks!

dfawley · 2019-01-15T18:24:33Z

cc @lyuxuan

puellanivis · 2019-01-20T08:48:11Z

It seems the SafeImplementation has been changed to UnimplementedServer, does it make sense to rename the PR title?

dfawley · 2019-01-22T18:59:09Z

Also, “…can be embedded…” would sound better? Saying “should” here suggests that the full implications must be understood and carefully weighed before choosing a different course. Meanwhile, this is more of a nice optional feature.

FWIW, I would argue embedding this should be considered a best practice for all services. Otherwise, newly added methods will break your code - but adding methods to proto services is not supposed to be a breaking change.

prannayk · 2019-01-22T19:18:18Z

Also, “…can be embedded…” would sound better? Saying “should” here suggests that the full implications must be understood and carefully weighed before choosing a different course. Meanwhile, this is more of a nice optional feature.

FWIW, I would argue embedding this should be considered a best practice for all services. Otherwise, newly added methods will break your code - but adding methods to proto services is not supposed to be a breaking change.

True, but its an optional feature for now, and gives added functionality to those who would want the said safety. Therefore the said language is better.

prannayk · 2019-02-02T12:51:12Z

Hey @dfawley @puellanivis does this look good now?

prannayk · 2019-02-25T08:22:04Z

Let me know if any other changes need to be done @dfawley @puellanivis

dfawley

Looks good to me, with one minor change requested. @dsnet are you OK with merging this after that is addressed?

dfawley · 2019-03-04T18:14:45Z

@dsnet - any concerns with this PR? If not, I think it is ready to be merged.

dsnet · 2019-03-05T21:19:20Z

I'm out of the office till beginning of next week. I can take a look on next Monday.

dfawley · 2019-03-13T23:12:41Z

Friendly ping @dsnet. grpc-go was just broken by a method we didn't implement; this PR would have prevented that.

PR fixes space removed in return line change errUnimplemented to real function Fixed PR issues errUnimplemented changed to real function, PR fixes

…re#784

### Release notes - https://github.com/grpc/grpc-go/releases/tag/v1.23.0 - much stuff! a bunch of HTTP2-related fixes -- we don't _expose_ go-grpc's listener, but we use it interally all over the place - there's `*satus.Is` now: grpc/grpc-go#2868 -- we could make some use of that, I suppose. - a bunch of perf improvements, always welcome :) - https://github.com/golang/protobuf/releases/tag/v1.3.2 - small fry, but nice stuff: golang/protobuf#785 adds an Unimplemented method to each server, allowing old servers to be used when there's proto changes. * deps: bump protobuf (1.3.2) and go-grpc (1.23.0) * deps: bump protobuf (1.3.2) and go-grpc (1.23.0) [revendor] * deps: bump protobuf (1.3.2) and go-grpc (1.23.0) [regenerate] Signed-off-by: Stephan Renatus <srenatus@chef.io>

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>

Also add deprecation comments on methods. Forward port: golang/protobuf#785 golang/protobuf#952 Fixes golang/protobuf#816 Change-Id: Id4d9f08b39fe16eaf57fb7a92fb8ada222b5cbf4 Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/205246 Reviewed-by: Joe Tsai <thebrokentoaster@gmail.com>

prannayk added 2 commits January 15, 2019 15:44

Added SafeImplementation

065f78d

Implemented server side

06c69e2

Fixed pb.go files

5f6df4d

dfawley reviewed Jan 15, 2019

View reviewed changes

Comment thread protoc-gen-go/grpc/grpc.go Outdated

Comment thread protoc-gen-go/grpc/grpc.go Outdated

Comment thread protoc-gen-go/grpc/grpc.go Outdated

Comment thread protoc-gen-go/grpc/grpc.go Outdated

prannayk added 2 commits January 16, 2019 00:26

Slight fixes for integration

056a1c0

Small Fixes

b2977b0

lyuxuan suggested changes Jan 16, 2019

View reviewed changes

Error impl changed, fixed spells

f84145c

puellanivis reviewed Jan 20, 2019

View reviewed changes

Comment thread protoc-gen-go/grpc/grpc.go Outdated

Comment thread protoc-gen-go/grpc/grpc.go Outdated

Comment thread protoc-gen-go/grpc/grpc.go Outdated

Comment thread protoc-gen-go/grpc/grpc.go Outdated

Comment thread protoc-gen-go/grpc/grpc.go Outdated

gofmt fixes, etc.

07adc54

prannayk changed the title ~~Added SafeImplementation for Server & Client interface~~ Added UnimplementedServer for server interface Jan 20, 2019

dfawley reviewed Jan 22, 2019

View reviewed changes

Comment thread protoc-gen-go/grpc/grpc.go Outdated

Comment thread protoc-gen-go/grpc/grpc.go Outdated

Review fixes

99dbe8a

lyuxuan suggested changes Jan 23, 2019

View reviewed changes

Comment thread protoc-gen-go/grpc/grpc.go Outdated

Comment thread protoc-gen-go/grpc/grpc.go Outdated

PR fixes

52633e3

lyuxuan approved these changes Jan 30, 2019

View reviewed changes

puellanivis reviewed Feb 3, 2019

View reviewed changes

Comment thread protoc-gen-go/grpc/grpc.go Outdated

space removed in return line

055c917

dfawley suggested changes Feb 27, 2019

View reviewed changes

Comment thread protoc-gen-go/grpc/grpc.go Outdated

change errUnimplemented to real function

fa4915e

dfawley approved these changes Mar 1, 2019

View reviewed changes

dsnet reviewed Mar 14, 2019

View reviewed changes

Comment thread protoc-gen-go/testdata/grpc/grpc.go Outdated

prannayk added 3 commits March 15, 2019 09:45

Review fixes

adc53a7

PR fixes space removed in return line change errUnimplemented to real function Fixed PR issues errUnimplemented changed to real function, PR fixes

Merge branch 'feature#784' of github.com:prannayk/protobuf into featu…

204d621

…re#784

remove grpc.go

88b5eab

dsnet merged commit 318d17d into golang:master Mar 15, 2019

dsnet mentioned this pull request Mar 15, 2019

cmd/protoc-gen-go-grpc: forward port UnimplementedServer #816

Closed

dfawley mentioned this pull request Mar 28, 2019

Feature request: Provide a way to make sure that their code does not break when services add new rpcs. grpc/grpc-go#2318

Closed

dsnet mentioned this pull request Jun 6, 2019

Feature Request: Ensure that code does not break when services add new RPCs. #784

Closed

prannayk deleted the feature#784 branch August 4, 2019 20:40

srenatus mentioned this pull request Aug 14, 2019

deps: bump go-grpc (1.23.0) and protobuf (1.3.2) chef/automate#1237

Merged

dfawley mentioned this pull request Sep 17, 2019

adding a new rpc to a proto is a breaking api change grpc/grpc-go#3024

Closed

urandom2 mentioned this pull request Sep 19, 2019

proposal: cmd/doc: add "// Unstable:" prefix convention golang/go#34409

Open

dsnet mentioned this pull request Dec 12, 2019

protoc-gen-go: Implement UnimplementedServer with value instead of pointer #997

Closed

asv mentioned this pull request Feb 6, 2020

Implicit dependencies for grpc Status / Code break a custom compilers bazel-contrib/rules_go#2362

Closed

melindalu mentioned this pull request Mar 10, 2020

Adds deps for imports in generated code stackb/rules_proto#147

Merged

dfawley mentioned this pull request Jun 11, 2020

protoc... missing UnimplementedGreeterServer grpc/grpc-go#3670

Closed

golang locked and limited conversation to collaborators Jun 26, 2020

Conversation

prannayk commented Jan 15, 2019

Uh oh!

googlebot commented Jan 15, 2019

What to do if you already signed the CLA

Individual signers

Corporate signers

Uh oh!

googlebot commented Jan 15, 2019

What to do if you already signed the CLA

Individual signers

Corporate signers

Uh oh!

prannayk commented Jan 15, 2019

Uh oh!

googlebot commented Jan 15, 2019

Uh oh!

googlebot commented Jan 15, 2019

Uh oh!

dfawley commented Jan 15, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

puellanivis commented Jan 20, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dfawley commented Jan 22, 2019

Uh oh!

Uh oh!

Uh oh!

prannayk commented Jan 22, 2019

Uh oh!

Uh oh!

Uh oh!

prannayk commented Feb 2, 2019

Uh oh!

Uh oh!

prannayk commented Feb 25, 2019

Uh oh!

dfawley left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dfawley commented Mar 4, 2019

Uh oh!

dsnet commented Mar 5, 2019

Uh oh!

dfawley commented Mar 13, 2019

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants