Validate MessageOneofRule (#311)

Supports cases in bufbuild/protovalidate#379

---------

Signed-off-by: Sri Krishna <[email protected]>
Co-authored-by: Steve Ayers <[email protected]>

Author: srikrsna-buf

gradle.properties

@@ -1,5 +1,5 @@
 # Version of buf.build/bufbuild/protovalidate to use.
-protovalidate.version = v0.13.0
+protovalidate.version = v0.13.1
 
 # Arguments to the protovalidate-conformance CLI
 protovalidate.conformance.args = --strict_message --strict_error --expected_failures=expected-failures.yaml

src/main/java/build/buf/protovalidate/EvaluatorBuilder.java

@@ -37,9 +37,11 @@
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.HashMap;
+import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Objects;
+import java.util.Set;
 import org.jspecify.annotations.Nullable;
 
 /** A build-through cache of message evaluators keyed off the provided descriptor. */
@@ -208,16 +210,26 @@ private void processMessageOneofRules(
         Descriptor desc, MessageRules msgRules, MessageEvaluator msgEval)
         throws CompilationException {
       for (MessageOneofRule rule : msgRules.getOneofList()) {
-        List<FieldDescriptor> fields = new ArrayList<>(rule.getFieldsCount());
+        if (rule.getFieldsCount() == 0) {
+          throw new CompilationException(
+              String.format(
+                  "at least one field must be specified in oneof rule for the message %s",
+                  desc.getFullName()));
+        }
+        Set<FieldDescriptor> fields = new LinkedHashSet<>(rule.getFieldsCount());
         for (String name : rule.getFieldsList()) {
           FieldDescriptor field = desc.findFieldByName(name);
           if (field == null) {
             throw new CompilationException(
-                String.format("field \"%s\" not found in %s", name, desc.getFullName()));
+                String.format("field %s not found in %s", name, desc.getFullName()));
+          }
+          if (!fields.add(field)) {
+            throw new CompilationException(
+                String.format(
+                    "duplicate %s in oneof rule for the message %s", name, desc.getFullName()));
           }
-          fields.add(field);
         }
-        msgEval.append(new MessageOneofEvaluator(fields, rule.getRequired()));
+        msgEval.append(new MessageOneofEvaluator(new ArrayList<>(fields), rule.getRequired()));
       }
     }
 

src/main/resources/buf/validate/validate.proto

@@ -150,17 +150,18 @@ message MessageRules {
   //      fields have explicit presence. This means that, for the purpose of determining
   //      how many fields are set, explicitly setting such a field to its zero value is
   //      effectively the same as not setting it at all.
-  //   3. This will generate validation errors when unmarshalling, even from the binary
-  //      format. With a Protobuf oneof, if multiple fields are present in the serialized
-  //      form, earlier values are usually silently ignored when unmarshalling, with only
-  //      the last field being present when unmarshalling completes.
+  //   3. This will always generate validation errors for a message unmarshalled from
+  //      serialized data that sets more than one field. With a Protobuf oneof, when
+  //      multiple fields are present in the serialized form, earlier values are usually
+  //      silently ignored when unmarshalling, with only the last field being set when
+  //      unmarshalling completes.
   //
   //
   // ```proto
   // message MyMessage {
   //   // Only one of `field1` or `field2` _can_ be present in this message.
   //   option (buf.validate.message).oneof = { fields: ["field1", "field2"] };
-  //   // Only one of `field3` or `field4` _must_ be present in this message.
+  //   // Exactly one of `field3` or `field4` _must_ be present in this message.
   //   option (buf.validate.message).oneof = { fields: ["field3", "field4"], required: true };
   //   string field1 = 1;
   //   bytes field2 = 2;
@@ -173,7 +174,8 @@ message MessageRules {
 
 message MessageOneofRule {
   // A list of field names to include in the oneof. All field names must be
-  // defined in the message.
+  // defined in the message. At least one field must be specified, and
+  // duplicates are not permitted.
   repeated string fields = 1;
   // If true, one of the fields specified _must_ be set.
   optional bool required = 2;