-
Notifications
You must be signed in to change notification settings - Fork 19
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Always treat groups as message fields (#132)
The relative simplicity and obviousness of this fix greatly obscures how difficult it was to determine :) Protobuf editions adds a couple "new" features to protobuf that cleverly re-use old protobuf wire and language features. In case of expanded repeated field representation, there appears to be no difference: for repeated fields, protobuf already supported both wire encodings, and protoreflect already had to treat them the same. Thus, repeated fields with both encodings work fine with protovalidate-go. In case of delimited message representation, this uses the old proto2 group encoding. This almost works, because protoreflect treats groups and submessage fields very similarly, and some of the protovalidate-go code also already handles groups as expected. Unfortunately though, there were a few places where groups and messages were not being treated identically. The failure case is rather obscure: _almost_ everything works, but some special cases are not taken correctly with the group encoding. This in and of itself is not automatically a problem but it can lead to weird issues, most notably the subtype not being discovered and registered correctly in the dynamic case. Although I did not individually test each line change to make sure that it fixes a specific bug, most of these are definitely needed and fix behavioral issues, except I am not 100% sure about the pgv migrator (Probably unnecessary but if proto2 groups work in pgv it seems fine for good measure.) I will make a separate PR to protovalidate that includes plenty of new test cases that ensure editions works; most of these pass on `main` save for some of the delimited submessage ones. cel-go seems to behave correctly already. I initially suspected a problem with how it handled delimited submessages, but it was a red herring.
- Loading branch information
1 parent
f204e44
commit ad94103
Showing
10 changed files
with
7,296 additions
and
15 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.