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

gRPC codec has different semantic on Unmarshal than the default one #128

Open
aureliar8 opened this issue Jan 3, 2024 · 1 comment · May be fixed by #131
Open

gRPC codec has different semantic on Unmarshal than the default one #128

aureliar8 opened this issue Jan 3, 2024 · 1 comment · May be fixed by #131

Comments

@aureliar8
Copy link

aureliar8 commented Jan 3, 2024

The README state than an easy way to use the vtprotobuf generated code is to change the default grpc codec to use github.com/planetscale/vtprotobuf/codec/grpc.Codec

However the unmarshal operation of this custom codec has different semantics than the default one: it performs a merge rather than an override.
This is documented in the UnmarshalVT doc but it's easy to miss.

This can be an issue when using grpc streams, because some valid code with the default codec will change behavior:

func (Server) SomeStreamRPC(stream pb.Test_StreamServer) error {
	msg := new(pb.SomeMsg)
	for {
                // msg.ResetVT() <- This is required to keep the same behavior after changing the codec 
		err := stream.RecvMsg(msg)
		if err != nil {
			return err
		}
		// Do something with msg ...
	}
}

I'd suggest that the codec is this repo calls msg.ResetVT() before UnmarhsalVT to keep the semantics of the default codec.
Advanced user can still pass their own codec that does not call ResetVT if they require it.

@vmg
Copy link
Member

vmg commented Jan 29, 2024

Good catch. I think this is a sensible change for the default implementation, since it can be overriden by performance-conscious users. Would you mind opening a PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants