External well-known structures support

[pomo-mondreganto]

I've added support for marshalling, unmarshalling and sizing of external well-known structures (currently tested with google.protobuf.Duration and google.protobuf.Timestamp, which we heavily use in production. My benchmarks show up to x3 speedup for marshalling structures with timestamp fields and up to x1.5 speedup for unmarshalling.

Adding other well-known structures should not be a problem, too (just extend the switch-case MapWellKnown(message *protogen.Message) with the fully-qualified import path for the external structure).

I believe we can even make the well known structures' set dynamic in the future, as Rust's prost does using the extern_path option.

[pomo-mondreganto]

It's also worth noticing that unknownFields for the external structures are not accounted for when marshalling & sizing, however, the support can be added using the ProtoReflect() accessor.

[biosvs]

Should we make a step back and discuss the overall approach for well known types?

I'm not sure if I get idea right, but it seems like you want to add wkt support as "features".

There is much simpler approach:

1. WKT should be generated as general proto: go+vtProto (we don't care about nature of this proto at this point).
2. Generated code should be placed in this repository (exactly as it made for protobuf-go).
3. Whenever you use wkt in your proto, use import replacements, e.g.:
   --go_opt=Mgoogle/protobuf/wrappers.proto=github.com/planetscale/vtprotobuf/types/wrapperspb

This approach doesn't require any changes in the existing code at all.

[vmg]

@biosvs: thanks for the insight. I quite like this idea from an implementation point of view; my concern is that the developer experience is not ideal, as you'd have to manually add the import replacements to your protoc-gen-go invocations.

I've just noticed that Vitess actually uses a couple WKTs, so I'm gonna attempt this approach to see how it works out in practice.

[vmg]

Short update: turns out that Vitess uses its own custom types instead of Pb Well-known Types, so I don't have any input to offer here. I'd like to hear some other people's opinions before I close this PR for @biosvs's approach.

[nockty]

👋 I'm back from vacations, it's nice seeing activity on this topic again because well known types are currently holding off vtprotobuf capabilities for messages that use a lot of them. I would love to see this support added!

(Disclaimer: I haven't reviewed this specific PR thoroughly, just adding my thoughts on the general discussion.)

Re: @biosvs' approach -- unless I'm mistaken, the developer experience impact would not be limited to import replacements in the protoc-gen-go invocations, right? IIUC, this approach would also require replacing all imports in application code from "google.golang.org/protobuf/types/known/timestamppb" (timestamppb being an example) to something like "github.com/planetscale/vtprotobuf/types/known/timestamppb" when using these types. For instance in this kind of code:

        // change the message timestamp to now
	ts := time.Now()
	someMessage.Timestamp = timestamppb.New(ts)

It may be an acceptable tradeoff, I just want to make sure that we acknowledge what the developer experience will be.

There are a couple other approaches mentioned in #54 that could avoid these import replacements. For instance, one approach would be to have public global helper function for WKT (example: UnmarshalTimestampVT(ts *timestamppb.Timestamp, data []byte)) that are referenced by the generated code and that can be used in the application code as well if needed.

• pro: users can continue using regular WKT instead of relying on planetscale/vtprotobuf ones;
• con: the interface for these types is different from others, users need to use functions instead of methods (since we cannot add methods to WKT without redefining them).

I'm curious to have your thoughts about all of this. On my side, I can find some time to try different approaches.

[biosvs]

@nockty yes, you're right. Here's an example of how it may look like: main...biosvs:vtprotobuf:add-wkt-types

[pomo-mondreganto]

You also could use type aliases so that existing codebases wouldn't need to migrate to vtproto's implementations. Pseudocode for illustration:

// vtproto/timestamppb/generated.go
type Timestamp timestamppb.Timestamp

func (t *Timestamp) MarshalVT() ...

func (t *Timestamp) UnmarshalVT() ...

// my-project-proto/generated.go
func (s *MyStruct) UnmarshalVT() {
  if isWKTimestamp(m.field) {
    m.field = timestamppb.Timestamp(vtproto_timestamppb.Timestamp(m.field).UnmarshalVT())
  }  
} 

[biosvs]

@pomo-mondreganto generated messages have unexported fields like unknownFields, and from the top of mind I can't imagine how to deal with it.

[vmg]

Merged an alternative implementation here: #99

Thank you for getting the ball rolling with this feature!