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

Remove references to RawMessage #311

Closed
tamird opened this issue Mar 16, 2017 · 14 comments
Closed

Remove references to RawMessage #311

tamird opened this issue Mar 16, 2017 · 14 comments

Comments

@tamird
Copy link
Contributor

tamird commented Mar 16, 2017

The code contains a few references to this type, including type raw interface { Bytes() []byte } which is supposed to be implemented by RawMessage. Unfortunately, that method signature is not as uncommon as one would hope, which can cause bad interactions when that function is implemented on generated structs or their constituent fields (e.g. via github.com/gogo/protobuf's customtype option).

My question is: is RawMessage still a thing? If yes, what is it? If no, would you accept a PR removing "support" for it?

cc @dsymonds

@bcmills
Copy link
Contributor

bcmills commented Mar 16, 2017

It's an internal thing. It is supposed to allow for certain functions (e.g. proxy servers) to work with serialized protocol messages directly without the overhead of marshaling and unmarshaling.

I agree that the interface we use to detect it is awkwardly broad.

@bcmills
Copy link
Contributor

bcmills commented Mar 16, 2017

(We can't remove it, but perhaps we can make it less prone to collisions.)

@tamird
Copy link
Contributor Author

tamird commented Mar 16, 2017

Does it need to stay an interface? I could imagine replacing raw with type RawMessage []byte. What do you think?

@bcmills
Copy link
Contributor

bcmills commented Mar 16, 2017

It has to correspond to some exported concrete type somewhere, or else nobody can use it.

It might make sense to promote to a concrete type into the proto package itself, though.

@tamird
Copy link
Contributor Author

tamird commented Mar 16, 2017

Yeah, I meant github.com/golang/protobuf/proto.RawMessage. I'll send a PR.

@bcmills
Copy link
Contributor

bcmills commented Mar 16, 2017

The proposed change would need a bit of a complicated internal migration. Unfortunately, I don't think this will work as a PR (sorry!).

@tamird
Copy link
Contributor Author

tamird commented Mar 16, 2017

No problem. What can I do to help?

@tamird
Copy link
Contributor Author

tamird commented Nov 18, 2017

Hello. Now that dev is a thing, is there something to be done to move this forward? Is it even still a problem?

@dsnet
Copy link
Member

dsnet commented Nov 20, 2017

I'm out-of-office for 2 weeks. I'm not going to think about this till I get back.

@dsnet
Copy link
Member

dsnet commented Dec 5, 2017

Support RawMessage was a gross hack that I wish was never added. I'm okay with changing the signature of the internal type to be something like ProtoRawBytes() to avoid conflicts and define that on the internal RawMessage type.

@bcmills, do you forsee any issues with that?

@bcmills
Copy link
Contributor

bcmills commented Dec 5, 2017

None that I can immediately think of.

@dsnet
Copy link
Member

dsnet commented Dec 5, 2017

Alright, I'll work on making the appropriate changes internally and push it to dev.

@dsnet dsnet self-assigned this Dec 5, 2017
dsnet added a commit that referenced this issue Jan 11, 2018
There is an internal type to Google called RawMessage that is similar
to json.RawMessage. Since there is no proper proto reflection API,
we special-cased the Bytes method of RawMessage to access the raw bytes.
This is a gross hack since Bytes() []byte is such a common method signature.

Remove this hack.

Fixes #311
dsnet added a commit that referenced this issue Jan 12, 2018
There is an internal type to Google called RawMessage that is similar
to json.RawMessage. Since there is no proper proto reflection API,
we special-cased the Bytes method of RawMessage to access the raw bytes.
This is a gross hack since Bytes() []byte is such a common method signature.

Remove this hack.

Fixes #311
@tamird
Copy link
Contributor Author

tamird commented Jan 12, 2018

(this issue has remained open because #482 was merged to dev, and master is still the primary branch).

@dsnet dsnet removed their assignment Jan 12, 2018
@dsnet dsnet changed the title What is "RawMessage"? Remove references to RawMessage Jan 12, 2018
@dsnet
Copy link
Member

dsnet commented Jan 29, 2018

Closing this as the merge work will be tracked in #479.

@dsnet dsnet closed this as completed Jan 29, 2018
@golang golang locked as resolved and limited conversation to collaborators Jun 25, 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

No branches or pull requests

3 participants