-
Notifications
You must be signed in to change notification settings - Fork 1
support proto serialization #509
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
base: main
Are you sure you want to change the base?
Conversation
|
|
👋 krehermann, thanks for creating this pull request! To help reviewers, please consider creating future PRs as drafts first. This allows you to self-review and make any final changes before notifying the team. Once you're ready, you can mark it as "Ready for review" to request feedback. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds support for Protocol Buffer (protobuf) message serialization to the IsSerializable validation function. The change recognizes that all protobuf messages are inherently serializable and should be treated as such in the validation logic.
- Added protobuf message detection to the IsSerializable function
- Included a new test case to verify protobuf serialization support
- Reorganized imports to follow Go conventions
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| operations/validation.go | Added protobuf message detection logic and updated function documentation |
| operations/validation_test.go | Added test case for protobuf message serialization and reorganized imports |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| } | ||
|
|
||
| func isProtoMessage(v reflect.Value) bool { | ||
| protoMsgType := reflect.TypeOf((*proto.Message)(nil)).Elem() |
Copilot
AI
Oct 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function doesn't handle nil pointer values correctly. If v represents a nil pointer to a proto message, v.Type() will return the pointer type but the check may fail. Consider adding a nil check and handling the case where v.Kind() == reflect.Ptr && v.IsNil().
| protoMsgType := reflect.TypeOf((*proto.Message)(nil)).Elem() | |
| protoMsgType := reflect.TypeOf((*proto.Message)(nil)).Elem() | |
| // Handle nil pointer: check the type the pointer points to | |
| if v.Kind() == reflect.Ptr && v.IsNil() { | |
| elemType := v.Type().Elem() | |
| return elemType.Implements(protoMsgType) || reflect.PointerTo(elemType).Implements(protoMsgType) | |
| } |
| if isProtoMessage(v) { | ||
| return true | ||
| } |
Copilot
AI
Oct 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The proto message check should be placed after the nil value check but before dereferencing pointers to maintain consistent logic flow. Currently, it may incorrectly handle pointer-to-proto cases that are checked later in the function.
af3c39c to
308b763
Compare
Co-authored-by: Copilot <[email protected]>
308b763 to
f98c206
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| protoMsgType := reflect.TypeOf((*proto.Message)(nil)).Elem() | ||
| return v.Type().Implements(protoMsgType) || reflect.PointerTo(v.Type()).Implements(protoMsgType) |
Copilot
AI
Oct 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reflect.TypeOf((*proto.Message)(nil)).Elem() call is executed on every invocation. Consider extracting this to a package-level variable to avoid repeated reflection operations.
| { | ||
| name: "should serialize proto message pointer", | ||
| // this is arbitrary proto message just to test the code path | ||
| v: &pb.AddressReferenceEditResponse{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm i dont think this is enough, even though this serialized without error, it doesnt mean we dont have data lost.
My understanding that for protos , we should at least be using "google.golang.org/protobuf/encoding/protojson" as using encoding/json will cause data lost as it doesnt know how to handle proto message.
I want to record responses from the JD in the operation report, and it's failing on IsSerializable.
All proto messages are, by defn, serializable, so add support