Skip to content

Conversation

@aureliar8
Copy link

Fixes #128

Contrary to what was discussed in the issue, I was not able to call ResetVT() before UnmarshallVT() because the reset method isn't always generated (it's only generated when the pool option is enabled)

Perhaps the ResetVT() method should also be generated when the unmarshal feature is enabled ?

@bhollis
Copy link
Contributor

bhollis commented Mar 18, 2024

You should do the same trick as the code does to detect the MarshalVT method - use an interface that declares ResetVT and see if the type implements that interface. That way you don't have to depend on protobuf.

@fenollp
Copy link
Contributor

fenollp commented May 18, 2024

In addition to @bhollis's comment I'd suggest to extract ResetVT generation from pool under a new option reset.

return fmt.Errorf("failed to unmarshal, message is %T (missing vtprotobuf helpers)", v)
}
//All types that implement github.com/golang/protobuf/proto.Message have a Reset method
vv, ok := v.(reseter)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
vv, ok := v.(reseter)
vv, ok := v.(interface { ResetVT() })

Copy link
Author

@aureliar8 aureliar8 May 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't require the message to implement ResetVT() because this method isn't generated by default (it requires the pool option)
Doing so would break the codec for user who just use the marshal and umarshal features

But I can follow this style and do

Suggested change
vv, ok := v.(reseter)
vv, ok := v.(interface{ Reset() })

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right :) IMO the resetvt func should be turned into a plug-in that's activated automatically with pool plugin.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't you mean that ResetVT() should be generated automatically when the unmarshal plugin is enabled ?

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 this pull request may close these issues.

gRPC codec has different semantic on Unmarshal than the default one

3 participants