status: Fix status incompatibility introduced by #6919 and move non-regeneratable proto code into /testdata #7724

arjan-bal · 2024-10-10T07:38:15Z

Problem

#6919 migrated usages of github.com/golang/protobuf to google.golang.org/protobuf/proto within status package. Prior to this, status.Details() called ptypes.UnmarshalAny which called proto.MessageV1(mt.New().Interface()) which called protoimpl.X.ProtoMessageV1Of(m) so the returned type always implemented protoadapt.MessageV1 and was always the same type given to status.WithDetails(). After the change, Status.Details() used anpb.Any.UnmarshallNew() which returns the vanilla type used while creating the anypb.Any which is only guaranteed to implement the MessageV2 API.

Effect

Users of code generated from protoc-gen-go < v1.4 (launched in 2020) would get a wrapped MessageV2 when calling Status.Details() even though they would have provided a type that only implemented MessageV1 to Status.WithDetails().

Fix

This PR brings back the call to protoimpl.X.ProtoMessageV1Of(m) by calling protoadapt.MessageV1Of before returning the Detail messages. A test is also added to catch regressions. Since the test uses generated code from protoc-gen-go v1.3, a dependency on github.com/golang/protobuf is re-introduced in go.mod.

Additional Changes

This PR also moves the generated code that is used for testing and must not be re-generated from the reflections/test module into a common directory under the parent grpc module.

RELEASE NOTES:

status: Fix regression caused by deps: move from github.com/golang/protobuf to google.golang.org/protobuf/proto #6919 in status.Details() causing it to return a wrapped type when getting proto messages generated with protoc-gen-go < v1.4.

codecov · 2024-10-10T07:45:27Z

Codecov Report

Attention: Patch coverage is 32.00000% with 17 lines in your changes missing coverage. Please review.

Project coverage is 80.25%. Comparing base (c538c31) to head (ecdbda8).
Report is 2 commits behind head on master.

Files with missing lines	Patch %	Lines
.../grpc_testing_not_regenerated/simple_message_v1.go	29.16%	17 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7724      +/-   ##
==========================================
+ Coverage   79.70%   80.25%   +0.54%     
==========================================
  Files         365      367       +2     
  Lines       36383    36622     +239     
==========================================
+ Hits        29000    29390     +390     
+ Misses       6183     6040     -143     
+ Partials     1200     1192       -8

Files with missing lines	Coverage Δ
internal/status/status.go	`90.52% <100.00%> (ø)`
testdata/grpc_testing_not_regenerated/testv3.go	`6.86% <ø> (ø)`
.../grpc_testing_not_regenerated/simple_message_v1.go	`29.16% <29.16%> (ø)`

... and 27 files with indirect coverage changes

easwars · 2024-10-10T18:48:02Z

status/status_ext_test.go

+	}
+
+	if diff := cmp.Diff(details, gotDetails, protocmp.Transform()); diff != "" {
+		t.Errorf("(%v).Details got unexpected output, diff (-got +want):\n%s", s, diff)


The order in the output, (-got +want), needs to be swapped I guess based on the order of parameters passed to cmp.Diff.

easwars · 2024-10-10T18:48:48Z

status/status_ext_test.go

+	details := []protoadapt.MessageV1{
+		&tpb.SimpleMessage{
+			Data: "abc",
+		},
+		&testpb.Empty{},
+	}


Minor nit (optional)

I feel this is more readable and compact:

details := []protoadapt.MessageV1{ &tpb.SimpleMessage{Data: "abc"}, &testpb.Empty{}, }

Changed as suggested.

easwars · 2024-10-10T18:49:42Z

testdata/grpc_testing_not_regenerate/simple.proto

@@ -0,0 +1,27 @@
+/*
+ * Copyright 2022 gRPC authors.


Should this be 2024?

dfawley · 2024-10-14T16:26:35Z

internal/status/status.go

+		// Since protoc-gen-go generates only code that implements both V1 and
+		// V2 APIs for backward compatibility, this is not a concern.


Is this guaranteed to always be the case? (Does it matter?)

Is this guaranteed to always be the case?

I don't think so. The V1 API is deprecated and is only present for backward compatibility. There is no guarantee that protoc-gen-go will always support it.

Does it matter?

protoadapt.MessageV1Of will return a wrapped message which implements the V1 API when the argument doesn't implement it already. Callers will need to unwrap it using the protoadapt.MessageV2Of to get the inner type.

This is why I updated the doc comment to say the following:

If the detail can be decoded, the proto message returned is of the same type that was given to WithDetails().

status.WithDetails accepts only V1 messages. To pass it a V2 message, it will need to be wrapped and status.Details() will return the same wrapped type.

dfawley · 2024-10-14T16:28:42Z

reflection/test/go.mod

@@ -5,12 +5,12 @@ go 1.22.7
 replace google.golang.org/grpc => ../../

 require (
-	github.com/golang/protobuf v1.5.4


IIRC, the only reason this file was here is to hide this module dependency, since it's deprecated. Let's just delete this file now. I think we're going to have to accept a huge module dependency list, which is why I created #7690 - to make sure our actual production code dependencies can be tracked.

Removed the go.mod and go.sum files from reflection/test/.

povsister · 2024-10-17T03:09:30Z

testdata/grpc_testing_not_regenerate/simple.proto

+option go_package = "google.golang.org/grpc/testdata/grpc_testing_not_regenerate";
+
+// SimpleMessage is used to hold string data.
+message SimpleMessage {


I feel it is better to name the test message as MessageV1Only, and give a short comments describing why it must not be regenerated.

I feel like "implementing the V1 API" is a property of the generated code and not the proto itself. There is a README.md describing the use of each generated file and how it was generated.

Sorry I missed that.

povsister · 2024-10-17T03:21:42Z

status/status_ext_test.go

+		gotDetails = append(gotDetails, msg)
+	}
+
+	if diff := cmp.Diff(details, gotDetails, protocmp.Transform()); diff != "" {


I just have glance on cmp.Diff, look like it is used to convert proto.Message into map-like structure, then perform a recursive "value-equal" check.
I think such examination does not satisfy "the proto message returned by Details() is of the same type that was given to WithDetails()". A reflect.Type check is preferred in such situation.

You're right, added a type comparison using reflect.TypeOf.

povsister · 2024-10-18T02:43:06Z

status/status_ext_test.go

+func (s) TestStatus_ErrorDetailsMessageV1(t *testing.T) {
+	details := []protoadapt.MessageV1{
+		&tpb.SimpleMessage{Data: "abc"},
+		&testpb.Empty{},


According to the test comments, should testpb.Empty be removed ?

Yes, removed. I wanted to keep a test case for ensuring v1 + v2 messages are also handled correctly, added a new test function for that later.

povsister · 2024-10-18T02:55:37Z

testdata/grpc_testing_not_regenerate/simple.proto

+option go_package = "google.golang.org/grpc/testdata/grpc_testing_not_regenerate";
+
+// SimpleMessage is used to hold string data.
+message SimpleMessage {


Sorry I missed that.

dfawley

Super nitpicky, but maybe this is easy to fix?

$ mv testdata/grpc_testing_not_regenerate testdata/grpc_testing_not_regenerated
$ grep -rl not_regenerate | xargs sed -i 's/grpc_testing_not_regenerate/grpc_testing_not_regenerated/g'

Maybe if it's that easy, change it? The awkward use of english always bothered me here. :)

"do_not_regenerate" or "dont_regenerate" would also be fine.

…mpatability

arjan-bal · 2024-10-21T03:59:10Z

Super nitpicky, but maybe this is easy to fix?
$ mv testdata/grpc_testing_not_regenerate testdata/grpc_testing_not_regenerated
$ grep -rl not_regenerate | xargs sed -i 's/grpc_testing_not_regenerate/grpc_testing_not_regenerated/g'
Maybe if it's that easy, change it? The awkward use of english always bothered me here. :)

"do_not_regenerate" or "dont_regenerate" would also be fine.

Renamed the directory. It required re-generating the message V1 only proto as it started panicking during registration:

panic: mismatching message name: got grpc.testdata.grpc_testing_not_regenerate.SimpleMessage, want grpc.testdata.grpc_testing_not_rege
nerated.SimpleMessage

goroutine 1 [running]:
google.golang.org/protobuf/internal/impl.legacyLoadMessageDesc({0xc54cf0, 0xb38be0}, {0xb91aa2, 0x38})
        /usr/local/google/home/arjansbal/go/pkg/mod/google.golang.org/[email protected]/internal/impl/legacy_message.go:147 +0x4b4
google.golang.org/protobuf/internal/impl.legacyLoadMessageInfo({0xc54cf0, 0xb38be0}, {0xb91aa2, 0x38})
        /usr/local/google/home/arjansbal/go/pkg/mod/google.golang.org/[email protected]/internal/impl/legacy_message.go:56 +0x6c
google.golang.org/protobuf/internal/impl.legacyLoadMessageType({0xc54cf0, 0xb38be0}, {0xb91aa2, 0x38})

…ability

…on-regeneratable proto code into /testdata (grpc#7724)

…egeneratable proto code into /testdata (#7724) (#7793)

…on-regeneratable proto code into /testdata (grpc#7724)

arjan-bal added 2 commits October 10, 2024 11:24

New directory for status proto

73e33f2

Move non-regeratable files into a common testdata directory

31d53ae

arjan-bal added the Type: Bug label Oct 10, 2024

arjan-bal added this to the 1.68 Release milestone Oct 10, 2024

arjan-bal requested review from easwars and dfawley October 10, 2024 07:38

arjan-bal assigned easwars and dfawley Oct 10, 2024

Add note about only v2 API

3b088a0

arjan-bal force-pushed the fix-status-incompatability branch from c313346 to 3b088a0 Compare October 10, 2024 07:40

Add a new proto to the test case

a0c3b84

easwars reviewed Oct 10, 2024

View reviewed changes

dfawley reviewed Oct 14, 2024

View reviewed changes

dfawley assigned arjan-bal and unassigned easwars and dfawley Oct 14, 2024

purnesh42H modified the milestones: 1.68 Release, 1.69 Release Oct 16, 2024

povsister reviewed Oct 17, 2024

View reviewed changes

address review comments

25cf51e

arjan-bal assigned easwars and dfawley and unassigned arjan-bal Oct 17, 2024

arjan-bal requested a review from povsister October 17, 2024 14:10

povsister reviewed Oct 18, 2024

View reviewed changes

fix test case

8633ca3

dfawley approved these changes Oct 18, 2024

View reviewed changes

dfawley assigned arjan-bal and unassigned dfawley Oct 18, 2024

arjan-bal added 2 commits October 21, 2024 09:10

rename directory

7dd0e15

Merge branch 'master' of github.com:grpc/grpc-go into fix-status-inco…

6009694

…mpatability

arjan-bal force-pushed the fix-status-incompatability branch from cc49daf to 6009694 Compare October 21, 2024 04:05

Merge remote-tracking branch 'source/master' into fix-status-incompat…

ecdbda8

…ability

arjan-bal merged commit 4bb0170 into grpc:master Oct 22, 2024
15 checks passed

arjan-bal deleted the fix-status-incompatability branch October 22, 2024 17:46

purnesh42H pushed a commit to purnesh42H/grpc-go that referenced this pull request Oct 29, 2024

status: Fix status incompatibility introduced by grpc#6919 and move n…

e6aad7e

…on-regeneratable proto code into /testdata (grpc#7724)

purnesh42H mentioned this pull request Oct 29, 2024

Cherry-pick #7724 to 1.68.x #7793

Merged

purnesh42H pushed a commit to purnesh42H/grpc-go that referenced this pull request Oct 30, 2024

status: Fix status incompatibility introduced by grpc#6919 and move n…

a79ba96

…on-regeneratable proto code into /testdata (grpc#7724)

purnesh42H added a commit that referenced this pull request Oct 30, 2024

status: Fix status incompatibility introduced by #6919 and move non-r…

056dc64

…egeneratable proto code into /testdata (#7724) (#7793)

misvivek pushed a commit to misvivek/grpc-go that referenced this pull request Nov 7, 2024

status: Fix status incompatibility introduced by grpc#6919 and move n…

ccf81a0

…on-regeneratable proto code into /testdata (grpc#7724)

tbg mentioned this pull request Nov 27, 2024

DEPS: upgrade grpc to v1.68.0 cockroachdb/cockroach#136278

Draft

16 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

status: Fix status incompatibility introduced by #6919 and move non-regeneratable proto code into /testdata #7724

status: Fix status incompatibility introduced by #6919 and move non-regeneratable proto code into /testdata #7724

arjan-bal commented Oct 10, 2024 •

edited

Loading

codecov bot commented Oct 10, 2024 •

edited

Loading

easwars Oct 10, 2024

arjan-bal Oct 21, 2024

easwars Oct 10, 2024

arjan-bal Oct 17, 2024

easwars Oct 10, 2024

arjan-bal Oct 17, 2024

dfawley Oct 14, 2024

arjan-bal Oct 17, 2024

dfawley Oct 14, 2024

arjan-bal Oct 17, 2024

povsister Oct 17, 2024

arjan-bal Oct 17, 2024

povsister Oct 18, 2024

povsister Oct 17, 2024

arjan-bal Oct 17, 2024

povsister Oct 18, 2024

arjan-bal Oct 18, 2024

povsister Oct 18, 2024

dfawley left a comment

arjan-bal commented Oct 21, 2024

		// Since protoc-gen-go generates only code that implements both V1 and
		// V2 APIs for backward compatibility, this is not a concern.

status: Fix status incompatibility introduced by #6919 and move non-regeneratable proto code into /testdata #7724

status: Fix status incompatibility introduced by #6919 and move non-regeneratable proto code into /testdata #7724

Conversation

arjan-bal commented Oct 10, 2024 • edited Loading

Problem

Effect

Fix

Additional Changes

codecov bot commented Oct 10, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dfawley left a comment

Choose a reason for hiding this comment

arjan-bal commented Oct 21, 2024

arjan-bal commented Oct 10, 2024 •

edited

Loading

codecov bot commented Oct 10, 2024 •

edited

Loading