[BUG] Inconsistent behavior for required and ignore_empty

[jhump]

This issue represents a few different defects:

1. The current documentation does not fully specify required and ignore_empty field constraints. It is unclear how message fields are handled (is an empty message one that is unset/null or one that is a default/empty message instance?) or how this interacts with field presence and explicitly configured default values (which may be non-zero).
2. The current implementations actually disagree on the meaning of "empty" and how that is applied to the required and ignore_empty field constraints. So once a clear specification is provided for these constraints, some implementations will likely need to be updated to comply with the revised spec.
3. The current conformance tests do not have any cases which enforce how these constraints are applied, which is how we find ourselves with inconsistent implementations.

In addition to resulting in inconsistency, some of the implementations are also written in a way that will likely not work correctly with Protobuf Editions. In particular, some implementations do not fully lean on the reflection support of the relevant Protobuf runtime, which will correctly consider "features" in source files that use Protobuf Editions. This means that the new representation for things like implicit vs. explicit field presence is likely to confuse the implementation logic.

Below is a summary of how the various runtimes actually implement these constraints:

• Go: The Go implementation fully leverages Protobuf reflection to interpret the data and apply these constraints. This is close to an ideal implementation; the only caveat is that some methods in the runtime's reflection API are not well-specified under all conditions, and the implementation is using these methods even for these unclear conditions. This is likely remedied with a few unit tests to make sure the result is expected, even under these conditions.

  To be specific, the implementation leans on FieldDescriptor.HasPresence(), FieldDescriptor.Default(), and Message.Has(FieldDescriptor) to decide if a value is present/non-empty and enforce the required and ignore_empty constraints. The downside is that FieldDescriptor.Default() is being used unconditionally, but the docs for that method do not specify its behavior for non-scalar fields (e.g. lists, maps, and messages).

  Relevant code snippets:

  • https://github.com/bufbuild/protovalidate-go/blob/main/internal/evaluator/field.go#L52
  • https://github.com/bufbuild/protovalidate-go/blob/main/internal/evaluator/builder.go#L255

• Java: The Java implementation is very similar to the Go implementation in structure and also leverages Protobuf reflection. However, it uses some conditions to clarify the case of empty/default message values. (It considers an explicitly present message value to be "empty" as long as it equals the empty/default message instance.)

  Strangely, it does not use the field's default value if it's type is an unsigned integer: it explicitly considers only zero to be empty. This is an awkward inconsistency with the Go implementation and weird "hybrid" between the Go and Python/C++ approaches. (I suspect this may have roots in the fact that the JVM does not actually have unsigned integer types.)

  Relevant code snippets:

  • https://github.com/bufbuild/protovalidate-java/blob/main/src/main/java/build/buf/protovalidate/internal/evaluator/EvaluatorBuilder.java#L175
  • https://github.com/bufbuild/protovalidate-java/blob/main/src/main/java/build/buf/protovalidate/internal/evaluator/FieldEvaluator.java#L66
  • https://github.com/bufbuild/protovalidate-java/blob/main/src/main/java/build/buf/protovalidate/internal/evaluator/ValueEvaluator.java#L54-L59

• C++: The C++ implementation has some quirks. For one, instead of using FieldDescriptor.has_presence(), it only considers fields with message types and fields in oneofs to be "explicit presence". This is a proto3-specific interpretation and must change to properly support proto2 and editions.

  Furthermore, this does not use FieldDescriptor's accessors for the field's default value. Instead, it is hard-coded to only consider zero values (0 numeric values, empty string/bytes, and false) to be "empty", with regards to the ignore_empty constraint. This is inconsistent with the Go and Java implementations and also leads to the potential paradox that a non-zero default value for a field could be considered an invalid value if explicitly set.

  Relevant code snippets:

  • https://github.com/bufbuild/protovalidate-cc/blob/main/buf/validate/internal/constraints.cc#L106-L109
  • https://github.com/bufbuild/protovalidate-cc/blob/main/buf/validate/internal/constraints.cc#L115-L175

• Python: The Python implementation looks almost exactly like the C++ implementation. It does not use FieldDescriptor.has_presence or FieldDescriptor.default_value properties. Instead it has logic similar to the C++ implementation for deciding if a field has explicit presence. Though its implementation is more specific and only considers non-repeated message fields to have explicit presence (this is more correct than the C++ implementation, but FieldDescriptor.has_presence would still be a marked improvement).

  Also like C++, it only considers zero values to be empty and does not consider a field's potential non-zero default value.

  Different from C++: it will compare the value to the zero/empty value to decide if a required field is present. This seems wrong since it implies that an explicitly set field may still be considered absent. It only uses message.HasField(name) when the field is not in a oneof (which seems like it's overly-specific to the awkward implementation detail used in proto3 optional fields).

  Relevant code snippets:

  • https://github.com/bufbuild/protovalidate-python/blob/main/protovalidate/internal/constraints.py#L115
  • https://github.com/bufbuild/protovalidate-python/blob/main/protovalidate/internal/constraints.py#L335

In general, the implementations should really look much more like one another. Even if we add conformance tests to better detect inconsistencies like above, it would instill greater confidence if the implementation code also looked more uniform, using the same conditions and reflection APIs across all implementations, where possible. (FWIW, the Protobuf runtimes do provide reasonably consistent APIs for reflection, across languages.)

In my opinion, the implementation in the Go runtime is closest to what I would intuitively expect -- that a default value, even if set explicitly, is not checked when ignore_empty is true. However, I would argue that the name of this constraint is very confusing since "empty" doesn't intuitively apply to scalar fields at all. I think ignore_default would be more clear, especially since it ignores other constraints not only when the field is absent but also when explicitly set to the default value. The only down-sides to the Go implementation is that its unclear how its usage of FieldDescriptor.Default() interacts with non-scalar fields. Is an explicitly-set empty message considered "empty"? And, either way, should it? My opinion is that an explicitly set field is not empty, only an absent one. But I can see this interpretation being debatable (even if the constraint were named ignore_default).

Some of the inconsistencies are a bit subtle, such that devising a conformance test to distinguish one's behavior from another might be tricky. But we should at least have conformance tests that cover the following scenarios, which should have teased out issues in the above implementations:

• A proto2 optional field is "present" and thus valid per the required constraint as long as it is explicitly set, even if set to its zero or default value. (This would catch potential issues in the Python implementation.)
• We should have test cases that use messages that define default values for all scalar field types, and then verify that they are correctly considered empty. This can be done with a validation that would otherwise fail with the field's default or zero value, which should be skipped. Before such cases can be formulated, we need to first agree on and fully specify the meaning of ignore_empty. (This would catch the inconsistencies between using a field's configured default vs. hard-coding to zero.)
• We should have cases that verify (both positive and negative outcomes) whether absent and present-but-empty message values are considered empty.

[jhump]

cc: @Alfus, @rodaine

[rodaine]

Thanks @jhump. The conformance tests are indeed limited in capturing the full breadth of behaviors. required, for instance, used to be exclusively on the MessageConstraints but now has adopted a wider meaning. The Go implementation is unofficially our official reference implementation so we will likely use its behavior as the guidepost of what is expected and should test & document accordingly.

[rodaine]

@jhump, #124 improves on the documentation for these fields and adds conformance tests to cover the expected behavior of required (with a fast follow for ignore_empty if approved).