search

golang / protobuf

Go support for Google's protocol buffers
BSD 3-Clause "New" or "Revised" License
9.76k stars 1.58k forks source link

types/known:structpb: support more custom types in constructors #1302

Closed torkelrogstad closed 3 years ago

torkelrogstad commented 3 years ago

Is your feature request related to a problem? Please describe. It is currently not possible to use methods from the structpb package, such as NewStruct or NewValue.

Consider this program:

package main

import (
    "fmt"
    "google.golang.org/protobuf/types/known/structpb"
)

type customString string

func main() {
    fmt.Println(structpb.NewValue(customString("hey")))
}

This errors with proto: invalid type: main.customString.

Describe the solution you'd like I believe it makes sense for the structpb methods to convert custom types to its underlying types. In this case, that'd mean that doing structpb.NewValue(customString("hey)) and structpb.NewValue("hey") would be identical.

Additional context Go playgroud for this: https://play.golang.org/p/rtT2xqXI71u

neild commented 3 years ago

I don't think this would be a good change, since it would silently discard type information. Right now you can safely write this for any type T:

var x T // some type T
v, err := structpb.NewValue(x)
if err != nil {
  return err
}
x2 := v.Interface().(T) // the type stored in the value is the type returned by `v.Interface`

Relaxing the types of values passed to structpb.NewValue would silently change the type stored in the value. Better to make the loss of type information explicit at creation time:

v, err := structpb.NewValue(string(x)) // very clear that the structpb.Value holds a string
torkelrogstad commented 3 years ago

This is fine for a string/string variant, but quickly gets hard if you're doing this with a more complext structure with structpb.NewStruct. I've been working around this for now by doing a JSON marshal/unmarshal roundtrip, which feels "wrong"

dsnet commented 3 years ago

structpb.NewValue has to draw the line somewhere for what Go types it supports. The set currently supported are clearly documented and are either:

since there is an obvious mapping for them and the implementation is pretty straight-forward.

Expanding the list of supported types to more than the above would cause the implementation of structpb to functionally have much of the same complexity as encoding/json (which is non-trivial). We're not going to maintain that (and if we did provide the feature, our implementation would probably just do a roundtrip json.Marshal and json.Unmarshal anyways).

As such, it is entirely appropriate to do a round-trip marshal and unmarshal in the application code as it clearly surfaces what is functionally happening.

dsnet commented 3 years ago

Closing as I don't think we'll be expanding the set of allowed types.