-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Comments
Something along the lines of this as a start: would this be useful? |
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. |
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. |
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. |
Just to leave some more notes on this issue: Python (with weaker typing) supports this type of convenience: And you can achieve the intended result (very inefficiently) by marshaling through jsonpb to json, and then unmarshal back to map[string]interface{} |
Leaving more notes :) A helper to convert map[string]interface{} to google.protobuf.Struct: |
I found a ptypes.Struct <-> map[string]interface{} converter in GoogleCloudPlatform/google-cloud-go:
|
It's a shame this has been closed - having some helpers to/from |
It makes no sense not to have these helpers. |
We recently came across a need for something similar and this is what we came up with: Not the most performant way but we chose simplicity over performance. |
Reopening to take a second look when we consider ptypes in |
Directing people towards or expecting people to use marshal/unmarshal through 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. |
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: |
This thread is not about providing an arbitrary “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 |
Fixed by https://golang.org/cl/225298. It will be available in the next release of the |
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.
The text was updated successfully, but these errors were encountered: