[BUG] Conformance test suite has test cases that are unspecified by the proto3 spec

[andrewparmet]

Description

While implementing protovalidate for protokt, which is a fully proto3-compliant generator and runtime[1], I couldn't get several conformance tests to pass. I believe this is because the conformance tests are taking advantage of convenient-but-unspecified behavior implemented by the runtimes supported by the other protovalidate implementations.

[1] Protokt doesn't support message merging but that doesn't matter here.

Steps to Reproduce

Run the conformance suite against https://github.com/andrewparmet/protovalidate-protokt/pull/9 without adversarial short-circuiting and observe the failures.

These tests check for ignoring validation rules when a non-optional scalar field is not present on the wire for a proto3 message:

buf.validate.conformance.cases.Fixed64Ignore
buf.validate.conformance.cases.Fixed32Ignore
buf.validate.conformance.cases.SFixed64Ignore
buf.validate.conformance.cases.SFixed32Ignore
buf.validate.conformance.cases.SInt64Ignore
buf.validate.conformance.cases.SInt32Ignore
buf.validate.conformance.cases.UInt64Ignore
buf.validate.conformance.cases.UInt32Ignore
buf.validate.conformance.cases.Int64Ignore
buf.validate.conformance.cases.Int32Ignore
buf.validate.conformance.cases.FloatIgnore
buf.validate.conformance.cases.DoubleIgnore
buf.validate.conformance.cases.IgnoreEmptyProto3Scalar
buf.validate.conformance.cases.BytesIPv6Ignore

This test checks that a non-optional scalar field was present on the wire for a proto3 message:

buf.validate.conformance.cases.RequiredProto3Scalar

Expected Behavior

The proto3 spec explicitly calls out that presence tracking is not (necessarily) implemented by compliant runtimes, and their conformance checks do not verify behavior like this.

Actual Behavior

These tests fail unless I specifically address them.

Possible Solution

These conformance tests should be removed and replaced with a test that verifies that ignore_empty cannot be used on non-optional proto3 scalar fields.

Additional Context

See bufbuild/protovalidate-java#80 for the proposal to extend protovalidate-java to support protokt.

[rodaine]

Hey @andrewparmet, and thanks for the very complete report! Both required and ignore_empty (and the soon to be implemented ignore) deal with presence, but aren't really concerned about presence tracking as described in the spec. Rather, they are more related to the concept of whether or not a field is serialized (which is the only way we can establish user intent to set a field). For fields that don't have explicit presence discipline, this typically applies to fields that are the default value for the type (zero or empty). For a deeper dive on presence, I recommend reading this guide.

validate.proto documents this explicitly, though this section has probably been the hardest to express in the clearest language possible. Definitely open to improvements on the definitions there to help with clarity, but the behavior is what is expected and useful to describing expectations of API semantics. If protokt does not support a Has method or similar, I think that might be more important to report upstream to that project to be consistent with the official libraries.

[andrewparmet]

report upstream to that project to be consistent with the official libraries

That would be me!

Part of the implementation was an implementation of reflective field lookup and it wasn't correct for fields that didn't track presence - precisely those tested by those conformance tests.

This was just an oversight - sorry for the false alarm!