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

types/known: functions for working with Struct #370

Closed
ptone opened this issue Jun 11, 2017 · 15 comments
Closed

types/known: functions for working with Struct #370

ptone opened this issue Jun 11, 2017 · 15 comments

Comments

@ptone
Copy link

ptone commented Jun 11, 2017

The helper files for some of the ptype proto types such as Duration and Timestamp are useful.

It would be great to have a ptypes.Struct <-> map[string]interface{} converter helper - as this would seem to me to be the closest analog native go representation.

@ptone
Copy link
Author

ptone commented Jun 12, 2017

Something along the lines of this as a start:

ptone@7ee10a8

would this be useful?

@cybrcodr
Copy link
Contributor

Just my opinion. With map[string]interface{}, user will still need to type assert on the map value. That's pretty similar to accessing ptypes.Struct.Fields which is of type map[string]*Value, i.e. need to type assert to one of *Value_X. There is also the advantage of having the *Value getter methods if using ptypes.Struct.Fields.

@ptone
Copy link
Author

ptone commented Jun 23, 2017

There are plenty of 3rd party libraries that work with a type of map[string]interface{} and handle that reflection and assertion in their implementation. These will not work with the struct ptype, and so this adds more burden on users to always handle that, hence the suggestion for an optional utility function.

@cybrcodr
Copy link
Contributor

Fair enough. I'm not convinced though that this should belong here, partly being cautious about expanding the API surface. I'm going to close this for now, but if you have a really good reason and feel strongly that this should be part of this package, feel free to reopen. Thanks.

@ptone
Copy link
Author

ptone commented Jul 3, 2017

Just to leave some more notes on this issue:

Python (with weaker typing) supports this type of convenience:
https://developers.google.com/protocol-buffers/docs/reference/python-generated#struct

And you can achieve the intended result (very inefficiently) by marshaling through jsonpb to json, and then unmarshal back to map[string]interface{}

@jsmouret
Copy link

jsmouret commented Aug 3, 2017

Leaving more notes :)

A helper to convert map[string]interface{} to google.protobuf.Struct:
https://gist.github.com/jsmouret/2bc876e8def6c63410556350eca3e43d

@xuyang2
Copy link

xuyang2 commented Jul 30, 2018

I found a ptypes.Struct <-> map[string]interface{} converter in GoogleCloudPlatform/google-cloud-go:

func DecodeToMap(s *pb.Struct) map[string]interface{}

@catkins
Copy link

catkins commented Sep 19, 2018

It's a shame this has been closed - having some helpers to/from pb.Struct would make these types much easier to work with.

@awfm9
Copy link

awfm9 commented Feb 18, 2019

It makes no sense not to have these helpers.

@hiromis
Copy link

hiromis commented Mar 6, 2020

We recently came across a need for something similar and this is what we came up with:
https://play.golang.org/p/-_o7hLUSWhV

Not the most performant way but we chose simplicity over performance.

@neild neild reopened this Mar 6, 2020
@neild
Copy link
Contributor

neild commented Mar 6, 2020

Reopening to take a second look when we consider ptypes in google.golang.org/protobuf.

@puellanivis
Copy link
Collaborator

Directing people towards or expecting people to use marshal/unmarshal through encoding/json is probably a bad idea, because package json will sorts the keys on a map[string]interface{} before emitting them. I’ve seen a server spin on max CPU for a couple minutes because it kept going through marshal/unmarshal rather than porting values directly.

I think we could easily do a better job in this package. And if we make it convenient enough, then people are far less likely to fall into one of the many gaping pitfalls on performance or memory just trying to get a solution out the door.

@cstrahan
Copy link

cstrahan commented Mar 6, 2020

Directing people towards or expecting people to use marshal/unmarshal through encoding/json is probably a bad idea, because package json will sorts the keys on a map[string]interface{} before emitting them.

In principle, I agree... but in practice, one must currently decide between maintaining a (intricate, and possibly buggy) implementation via reflection, or going the sure-fire way of round tripping through JSON (de)serialization.

As you suggest, I think this would be best solved by adding direct support in the protobuf lib, that way everyone achieves better performance, and the logic has more eyeballs on it (to prevent subtle edge-case bugs).

As a final note, I'll mention that even a project as large and successful as Envoy still uses JSON round-tripping:

https://github.com/envoyproxy/go-control-plane/blob/b0ba75c9c5574133fecebe557aa8d126e69f5ca7/pkg/conversion/struct.go#L27-L60

@puellanivis
Copy link
Collaborator

This thread is not about providing an arbitrary proto.Message to map[string]interface{}, but rather strictly a structpb.Struct to map[string]interface{}, and back. This should not require anything more than a type switch.

“But what if they’re using a named type of concrete type XY? That won’t work in a type switch.” We should not support that. We should solely and narrowly support only what structpb.Struct supports, and no more.

@dsnet dsnet changed the title ptype.Struct helpers types/known: functions for working with Struct Mar 7, 2020
@dsnet
Copy link
Member

dsnet commented Jun 9, 2020

Fixed by https://golang.org/cl/225298. It will be available in the next release of the google.golang.org/protobuf module, to be released within a week or so.

@dsnet dsnet closed this as completed Jun 9, 2020
@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