strict erroring for custom marshalers trying to use a deterministic b… #650

jmarais · 2018-07-10T09:34:03Z

…uffer as discussed in #648

googlebot · 2018-07-10T09:34:06Z

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 · 2018-07-10T09:34:06Z

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.

jmarais · 2018-07-10T09:35:53Z

I signed it!

googlebot · 2018-07-10T09:35:55Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

…uffer as discussed in golang#648

googlebot · 2018-07-10T09:44:53Z

CLAs look good, thanks!

googlebot · 2018-07-10T09:44:53Z

CLAs look good, thanks!

jmarais · 2018-07-10T09:57:04Z

I am not sure why the following would fail in travis. Any recommendation?

go build ./protoc-gen-go/testdata/grpc/grpc.pb.go
# golang.org/x/net/http2
../../../golang.org/x/net/http2/go111.go:10:30: trace.WroteHeaderField undefined (type *clientTrace has no field or method WroteHeaderField)
../../../golang.org/x/net/http2/go111.go:14:26: trace.WroteHeaderField undefined (type *clientTrace has no field or method WroteHeaderField)
../../../golang.org/x/net/http2/go111.go:15:8: trace.WroteHeaderField undefined (type *clientTrace has no field or method WroteHeaderField)

dsnet · 2018-07-10T19:11:54Z

proto/table_marshal.go

@@ -225,6 +225,9 @@ func (u *marshalInfo) marshal(b []byte, ptr pointer, deterministic bool) ([]byte
 	// If the message can marshal itself, let it do it, for compatibility.
 	// NOTE: This is not efficient.
 	if u.hasmarshaler {
+		if deterministic {
+			return nil, errors.New("proto: deterministic not supported by the Marshal method of " + u.typ.Name())


s/Name/String/

reflect.Type.Name only prints the type name, while reflect.Type.String provides the package as well.

Thanks for the suggestion.

dsnet · 2018-07-10T19:31:32Z

You can ignore the travis failure. After some digging, it seems that WroteHeaderField was added on June 27th in https://go-review.googlesource.com/c/go/+/67430. However, travis is running with go1.11beta1, which was cut on June 26th, so lacks that function.

dsnet · 2018-07-12T04:20:56Z

Thanks for the change!

…shalers PR #650 added a check to error out when a custom marshaler was called and deterministic marshaling was also specified. That change performed the check in a relatively obscure location that did not catch all code paths. Since determinism can only be enabled on the Buffer type, we check it up front in Buffer.Marshal. Also, change the test to avoid code injection into generated sources.

…shalers (#656) PR #650 added a check to error out when a custom marshaler was called and deterministic marshaling was also specified. That change performed the check in a relatively obscure location that did not catch all code paths. Since determinism can only be enabled on the Buffer type, we check it up front in Buffer.Marshal. Also, change the test to avoid code injection into generated sources.

PR #650 added strict checking of the use of deterministic with custom marshalers since it is impossible to know whether a custom marshalers actually do produce deterministic output or not. However, this check is breaking hundreds of targets that already rely on determinism along with custom marshalers. In every case, the custom marshaler already produced deterministic output, so it did not really matter. If deterministic is specified *and* a custom marshaler is not actually deterministic, then the output is obviously not deterministic, and setting the flag was a lie. However, there is not much we can do with the current API. A redesign of the proto API will resolve this tension.

…658) PR #650 added strict checking of the use of deterministic with custom marshalers since it is impossible to know whether a custom marshalers actually do produce deterministic output or not. However, this check is breaking hundreds of targets that already rely on determinism along with custom marshalers. In every case, the custom marshaler already produced deterministic output, so it did not really matter. If deterministic is specified *and* a custom marshaler is not actually deterministic, then the output is obviously not deterministic, and setting the flag was a lie. However, there is not much we can do with the current API. A redesign of the proto API will resolve this tension.

strict erroring for custom marshalers trying to use a deterministic b…

b0e1c3c

…uffer as discussed in golang#648

jmarais force-pushed the issue-648 branch from bae48a3 to b0e1c3c Compare July 10, 2018 09:44

dsnet reviewed Jul 10, 2018

View reviewed changes

dsnet mentioned this pull request Jul 10, 2018

Resolved issue #635 Markdown Format Issues #652

Merged

use reflect.Type.String to include package name in error message

4fed7d8

dsnet merged commit 70b3af3 into golang:master Jul 12, 2018

dsnet mentioned this pull request Jul 15, 2018

jsonpb: avoid copying string-valued map-keys #654

Merged

dsnet mentioned this pull request Jul 20, 2018

proto: fix and cleanup test for deterministic marshal with custom marshalers #656

Merged

dsnet mentioned this pull request Jul 24, 2018

proto: revert strict erroring of deterministic and custom marshalers #658

Merged

golang locked and limited conversation to collaborators Jun 26, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

strict erroring for custom marshalers trying to use a deterministic b… #650

strict erroring for custom marshalers trying to use a deterministic b… #650

Uh oh!

jmarais commented Jul 10, 2018

Uh oh!

googlebot commented Jul 10, 2018

Uh oh!

googlebot commented Jul 10, 2018

Uh oh!

jmarais commented Jul 10, 2018

Uh oh!

googlebot commented Jul 10, 2018

Uh oh!

googlebot commented Jul 10, 2018

Uh oh!

googlebot commented Jul 10, 2018

Uh oh!

jmarais commented Jul 10, 2018

Uh oh!

dsnet Jul 10, 2018

Uh oh!

jmarais Jul 11, 2018

Uh oh!

dsnet commented Jul 10, 2018

Uh oh!

dsnet commented Jul 12, 2018

Uh oh!

Uh oh!

strict erroring for custom marshalers trying to use a deterministic b… #650

strict erroring for custom marshalers trying to use a deterministic b… #650

Uh oh!

Conversation

jmarais commented Jul 10, 2018

Uh oh!

googlebot commented Jul 10, 2018

What to do if you already signed the CLA

Individual signers

Corporate signers

Uh oh!

googlebot commented Jul 10, 2018

What to do if you already signed the CLA

Individual signers

Corporate signers

Uh oh!

jmarais commented Jul 10, 2018

Uh oh!

googlebot commented Jul 10, 2018

Uh oh!

googlebot commented Jul 10, 2018

Uh oh!

googlebot commented Jul 10, 2018

Uh oh!

jmarais commented Jul 10, 2018

Uh oh!

dsnet Jul 10, 2018

Choose a reason for hiding this comment

Uh oh!

jmarais Jul 11, 2018

Choose a reason for hiding this comment

Uh oh!

dsnet commented Jul 10, 2018

Uh oh!

dsnet commented Jul 12, 2018

Uh oh!

Uh oh!