Conversation
| logger.Debug("ignore non Failure field", tag.NewAnyTag("path", path.String())) | ||
| return false | ||
| } | ||
| if strings.Contains(path.String(), "/Cause") { |
There was a problem hiding this comment.
this is so we only visit the top level "Failure" type? maybe add a comment here.
There was a problem hiding this comment.
Right. We can have the repairInvalidUTF8InFailure function only handle the top-level Failure and it can descend in to the Cause field.
| return string(result) | ||
| } | ||
|
|
||
| func snakeToPascalCase[T ~string](s T) string { |
There was a problem hiding this comment.
can you provide some examples on input -> output to help understanding?
| @@ -0,0 +1,68 @@ | |||
| package main | |||
There was a problem hiding this comment.
can we have a section about the tool in CONTRIBUTING.md and how the repair works at high level?
There was a problem hiding this comment.
actually, probably best is to create an github issue for this work so we can record design choices.
There was a problem hiding this comment.
I added the contributing section (not a github issue)
| }, | ||
| ) | ||
|
|
||
| protoregistry.GlobalTypes.RangeMessages(func(mt protoreflect.MessageType) bool { |
There was a problem hiding this comment.
i think the reason is that protoreflect doesn't work with gogobuf and later we will filter types not in gogobuf. Add some comments to explain why iterates protobuf types instead of gogobuf.
| serverapireplication "github.com/temporalio/s2s-proxy/proto/1_22/server/api/replication/v1" | ||
| ) | ||
|
|
||
| func repairInvalidUTF8(vAny any) (ret bool, retErr error) { |
There was a problem hiding this comment.
is repairInvalidUTF8 hooked in the grpc codec already or will be a separate PR?
There was a problem hiding this comment.
Will be a separate PR - just splitting up PRs a little.
There was a problem hiding this comment.
Pull Request Overview
This PR introduces code generation tools for working with protobuf type hierarchies and generates code to repair invalid UTF-8 in Failure-typed fields in protobuf messages. The main purpose is to create a visitor pattern generator that can traverse protobuf type hierarchies and generate appropriate handling code.
- Adds
cmd/tools/genvisitortool to generate code based on protobuf type hierarchies - Implements UTF-8 repair functionality for
Failuretyped fields across protobuf messages - Makes function name changes to use lowercase for generated conversion functions
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| proto/compat/repair_utf8_test.go | Tests for UTF-8 repair functionality with various failure scenarios |
| proto/compat/repair_utf8_gen.go | Generated code to repair UTF-8 in Failure fields across protobuf type hierarchy |
| proto/compat/repair_utf8.go | Core UTF-8 repair logic with depth limits for failure chains |
| proto/compat/frontend_conversion_gen.go | Changes function name from uppercase to lowercase |
| proto/compat/admin_conversion_gen.go | Changes function name from uppercase to lowercase |
| cmd/tools/genvisitor/* | New code generation tool for protobuf visitor patterns |
| cmd/tools/genrpcwrappers/extra.go | Template update for consistent function naming |
| Makefile | Build target for running the genvisitor tool |
Comments suppressed due to low confidence (1)
proto/compat/repair_utf8_gen.go:131
- The package declaration is incorrect. This should be 'package compat' since the file is in the compat directory and is imported as such in the test file.
retErr = err
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
What was changed
cmd/tools/genvisitorto generate code based on protobuf type hierarchiescmd/tools/genvisitorto generate code to repair invalid utf-8 in anyFailuretyped fields in protobuf messagesproto/compatdirectory(This PR does not add the new codec)
Why?
Checklist
Closes
How was this tested:
Basic unit tests (spot checks)