Skip to content
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

[BUG] Groups (proto2-only feature) are not consistently handled #117

Open
jhump opened this issue Oct 24, 2023 · 3 comments
Open

[BUG] Groups (proto2-only feature) are not consistently handled #117

jhump opened this issue Oct 24, 2023 · 3 comments
Labels
Bug Something isn't working

Comments

@jhump
Copy link
Member

jhump commented Oct 24, 2023

Groups are not handled uniformly in the various implementations of protovalidate.

In particular, it looks like the Go and Java implementations do not correctly support groups and will skip validating the data inside a group field. However, Python and C++ implementations do not appear to have this defect.

Test cases should likely be added to the conformance tests to verify they work correctly across all implementations.

In particular, both Go and Java implementations only do "embedded message" validation when the field's type is "message". However, group fields will have a type of "group". The Go code uses an thin abstraction over google.protobuf.protobuf.FieldDescriptorProto.Type named protoreflect.Kind. Similarly, the Java code uses a thin abstraction in the Java enum Descriptor.FieldDescriptor.Type. But they both map to the type enum in descriptor.proto, where message and group each have a distinct value (necessary because though they are structurally the same, representing nested messages, they use different wire encoding formats).

The Python code, however, simply asks if the field descriptor has an associated message type, which it will for both groups and messages. And the C++ code instead looks at the field descriptor's cpp_type(), which will be CPPTYPE_MESSAGE for both messages and groups. So these two implementations appear to correctly support groups. (A new conformance test would be the best way to verify and assert this to be true.)

@jhump jhump added the Bug Something isn't working label Oct 24, 2023
@rodaine
Copy link
Member

rodaine commented Oct 30, 2023

Like PGV before it, PV does not officially support validation against groups (mainly due to their deprecation status that predates both of these projects). We could add support (or document/test for non-support), but over the past 5 years of these two projects, this is actually the first time they've been mentioned 😅.

@jhump
Copy link
Member Author

jhump commented Jun 20, 2024

I suspect that we'll need to revisit this. While groups are long deprecated, "delimited encoding" is a thing that can be done in Editions files, and I think such fields may look like groups when inspected via the reflection APIs.

@rodaine
Copy link
Member

rodaine commented Jun 20, 2024

Shouldn't be too big of a lift! The evaluator will likely not be too different from message fields

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants