MaxSize computation for sized proto messages#3619
Conversation
PR SummaryMedium Risk Overview For messages marked The plugin registers wireguard schemas for all messages in generated files (not only the old annotation closure), refreshes Reviewed by Cursor Bugbot for commit 025002f. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 2299c65. Configure here.
| } | ||
| genFileName := strings.TrimSuffix(filepath.Base(file.Desc.Path()), ".proto") + ".wireguard.go" | ||
| g := p.NewGeneratedFile(filepath.Join(genDir, genFileName), file.GoImportPath) | ||
| g := p.NewGeneratedFile(filepath.Join(string(file.GoImportPath), genFileName), file.GoImportPath) |
There was a problem hiding this comment.
Ignored module breaks output paths
High Severity
emit passes the full protobuf go_package import path into NewGeneratedFile instead of a path relative to the plugin module option. parseConfig reads module from the request but the result is discarded, so buf’s module=… opt no longer strips the prefix. Regeneration writes .wireguard.go files under nested github.com/… directories instead of beside the existing packages.
Reviewed by Cursor Bugbot for commit 2299c65. Configure here.
There was a problem hiding this comment.
why is module param dropped here?
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3619 +/- ##
==========================================
- Coverage 59.01% 58.27% -0.75%
==========================================
Files 2224 2167 -57
Lines 182814 175202 -7612
==========================================
- Hits 107893 102100 -5793
+ Misses 65220 64094 -1126
+ Partials 9701 9008 -693
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
| size := 1 | ||
| values := fd.Enum().Values() | ||
| for i := range values.Len() { | ||
| size = max(size, protowire.SizeVarint(uint64(int64(values.Get(i).Number())))) // nolint: gosec // WAI, negative enums are encoded as uint64 |
There was a problem hiding this comment.
nit: this is the size known to receiving side, but the sending side could define a much larger enum value and send it over?
| } | ||
| genFileName := strings.TrimSuffix(filepath.Base(file.Desc.Path()), ".proto") + ".wireguard.go" | ||
| g := p.NewGeneratedFile(filepath.Join(genDir, genFileName), file.GoImportPath) | ||
| g := p.NewGeneratedFile(filepath.Join(string(file.GoImportPath), genFileName), file.GoImportPath) |
There was a problem hiding this comment.
why is module param dropped here?
| for fullName := range inSchema { | ||
| d := byName[fullName] | ||
| func validateRuleValues(byName map[protoreflect.FullName]protoreflect.MessageDescriptor, exts wireguardExts) error { | ||
| for _, d := range byName { |
There was a problem hiding this comment.
would this iterate on all messages including external importas as well?
3 participants
For each message with "wireguard.sized = true", we generate "MaxSize() int" method. It will be used to bound buffers intended for caching those messages. Additionally I have made wireguard apply implicit max_count = 1 to every singular field, because proto.Unmarshal silently merges recursively instances of singular fields (if there are many), therefore making Scan() useless (which applies bounds field arity by instance). We have no use case for encoding singular fields as multiple partitions, so we simply disallow such encoding instead.