From cc0810429db7c4aba2d0b923ec690516532a216f Mon Sep 17 00:00:00 2001 From: Josh Humphries <2035234+jhump@users.noreply.github.com> Date: Tue, 12 May 2026 15:09:19 -0400 Subject: [PATCH] Lint MessageRules.oneof in PROTOVALIDATE check Validate that each MessageOneofRule entry names at least one field, contains no duplicate names, references only fields defined in the message, and that no field appears in more than one entry. --- .../buflintvalidate/buflintvalidate.go | 1 + .../internal/buflintvalidate/message.go | 120 ++++++++++++++++++ private/bufpkg/bufcheck/lint_test.go | 5 + .../lint/protovalidate/proto/oneof.proto | 47 +++++++ 4 files changed, 173 insertions(+) create mode 100644 private/bufpkg/bufcheck/bufcheckserver/internal/buflintvalidate/message.go diff --git a/private/bufpkg/bufcheck/bufcheckserver/internal/buflintvalidate/buflintvalidate.go b/private/bufpkg/bufcheck/bufcheckserver/internal/buflintvalidate/buflintvalidate.go index cbe7d72cae..2be8dc2b75 100644 --- a/private/bufpkg/bufcheck/bufcheckserver/internal/buflintvalidate/buflintvalidate.go +++ b/private/bufpkg/bufcheck/bufcheckserver/internal/buflintvalidate/buflintvalidate.go @@ -38,6 +38,7 @@ func CheckMessage( if messageRules == nil { return nil } + checkOneofRulesForMessage(addAnnotationFunc, messageRules, messageDescriptor, message) return checkCELForMessage( addAnnotationFunc, messageRules, diff --git a/private/bufpkg/bufcheck/bufcheckserver/internal/buflintvalidate/message.go b/private/bufpkg/bufcheck/bufcheckserver/internal/buflintvalidate/message.go new file mode 100644 index 0000000000..740918045d --- /dev/null +++ b/private/bufpkg/bufcheck/bufcheckserver/internal/buflintvalidate/message.go @@ -0,0 +1,120 @@ +// Copyright 2020-2026 Buf Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package buflintvalidate + +import ( + "buf.build/gen/go/bufbuild/protovalidate/protocolbuffers/go/buf/validate" + "github.com/bufbuild/buf/private/bufpkg/bufprotosource" + "google.golang.org/protobuf/reflect/protoreflect" +) + +const ( + // https://buf.build/bufbuild/protovalidate/docs/main:buf.validate#buf.validate.MessageRules + oneofFieldNumberInMessageRules = 4 + // https://buf.build/bufbuild/protovalidate/docs/main:buf.validate#buf.validate.MessageOneofRule + fieldsFieldNumberInMessageOneofRule = 1 +) + +func checkOneofRulesForMessage( + add func(bufprotosource.Descriptor, bufprotosource.Location, []bufprotosource.Location, string, ...any), + messageRules *validate.MessageRules, + messageDescriptor protoreflect.MessageDescriptor, + message bufprotosource.Message, +) { + oneofRules := messageRules.GetOneof() + if len(oneofRules) == 0 { + return + } + // Track which oneof entries reference each field name, so we can report + // fields that appear in more than one entry. + fieldNameToOneofIndices := make(map[string][]int) + messageFields := messageDescriptor.Fields() + for i, oneofRule := range oneofRules { + oneofLocation := message.OptionExtensionLocation( + validate.E_Message, + oneofFieldNumberInMessageRules, + int32(i), + ) + fields := oneofRule.GetFields() + if len(fields) == 0 { + add( + message, + oneofLocation, + nil, + "Message %q has a (buf.validate.message).oneof entry with no fields. Each oneof entry must specify at least one field.", + message.Name(), + ) + continue + } + seenInThisEntry := make(map[string]struct{}, len(fields)) + for j, fieldName := range fields { + fieldLocation := message.OptionExtensionLocation( + validate.E_Message, + oneofFieldNumberInMessageRules, + int32(i), + fieldsFieldNumberInMessageOneofRule, + int32(j), + ) + if _, ok := seenInThisEntry[fieldName]; ok { + add( + message, + fieldLocation, + nil, + "Message %q has a (buf.validate.message).oneof entry that references field %q more than once. Duplicate field names are not permitted.", + message.Name(), + fieldName, + ) + continue + } + seenInThisEntry[fieldName] = struct{}{} + if messageFields.ByName(protoreflect.Name(fieldName)) == nil { + add( + message, + fieldLocation, + nil, + "Message %q has a (buf.validate.message).oneof entry that references field %q, which is not defined in the message.", + message.Name(), + fieldName, + ) + continue + } + fieldNameToOneofIndices[fieldName] = append(fieldNameToOneofIndices[fieldName], i) + } + } + for fieldName, indices := range fieldNameToOneofIndices { + if len(indices) < 2 { + continue + } + locations := make([]bufprotosource.Location, 0, len(indices)) + for _, index := range indices { + locations = append(locations, message.OptionExtensionLocation( + validate.E_Message, + oneofFieldNumberInMessageRules, + int32(index), + )) + } + locations = deduplicateLocations(locations) + for _, location := range locations { + add( + message, + location, + nil, + "Message %q references field %q in more than one (buf.validate.message).oneof entry. A field may only appear in one oneof entry.", + message.Name(), + fieldName, + ) + } + } +} diff --git a/private/bufpkg/bufcheck/lint_test.go b/private/bufpkg/bufcheck/lint_test.go index 43ac25ee09..6069c4a6a0 100644 --- a/private/bufpkg/bufcheck/lint_test.go +++ b/private/bufpkg/bufcheck/lint_test.go @@ -743,6 +743,11 @@ func TestRunProtovalidate(t *testing.T) { bufanalysistesting.NewFileAnnotation(t, "number.proto", 329, 79, 329, 117, "PROTOVALIDATE"), bufanalysistesting.NewFileAnnotation(t, "oneof.proto", 13, 7, 13, 43, "PROTOVALIDATE"), bufanalysistesting.NewFileAnnotation(t, "oneof.proto", 19, 7, 19, 43, "PROTOVALIDATE"), + bufanalysistesting.NewFileAnnotation(t, "oneof.proto", 48, 3, 48, 75, "PROTOVALIDATE"), + bufanalysistesting.NewFileAnnotation(t, "oneof.proto", 55, 3, 55, 54, "PROTOVALIDATE"), + bufanalysistesting.NewFileAnnotation(t, "oneof.proto", 62, 3, 62, 67, "PROTOVALIDATE"), + bufanalysistesting.NewFileAnnotation(t, "oneof.proto", 70, 3, 70, 62, "PROTOVALIDATE"), + bufanalysistesting.NewFileAnnotation(t, "oneof.proto", 71, 3, 71, 62, "PROTOVALIDATE"), bufanalysistesting.NewFileAnnotation(t, "repeated.proto", 25, 5, 25, 48, "PROTOVALIDATE"), bufanalysistesting.NewFileAnnotation(t, "repeated.proto", 27, 5, 27, 48, "PROTOVALIDATE"), bufanalysistesting.NewFileAnnotation(t, "repeated.proto", 45, 5, 45, 48, "PROTOVALIDATE"), diff --git a/private/bufpkg/bufcheck/testdata/lint/protovalidate/proto/oneof.proto b/private/bufpkg/bufcheck/testdata/lint/protovalidate/proto/oneof.proto index 83d13b2728..57207f61da 100644 --- a/private/bufpkg/bufcheck/testdata/lint/protovalidate/proto/oneof.proto +++ b/private/bufpkg/bufcheck/testdata/lint/protovalidate/proto/oneof.proto @@ -26,3 +26,50 @@ message OneofTest { optional string f5 = 5 [(buf.validate.field).required = true]; optional google.protobuf.Duration f6 = 6 [(buf.validate.field).required = true]; } + +// MessageOneofRuleValid exercises the buf.validate.message.oneof rule for the +// success case: the rule references real fields with no duplicates and no +// overlap between entries. +message MessageOneofRuleValid { + option (buf.validate.message).oneof = {fields: ["a", "b"]}; + option (buf.validate.message).oneof = { + fields: ["c", "d"], + required: true + }; + string a = 1; + string b = 2; + string c = 3; + string d = 4; +} + +// MessageOneofRuleUnknownField references a field that is not defined in +// the message. +message MessageOneofRuleUnknownField { + option (buf.validate.message).oneof = {fields: ["a", "does_not_exist"]}; + string a = 1; + string b = 2; +} + +// MessageOneofRuleEmpty has an entry with no field names. +message MessageOneofRuleEmpty { + option (buf.validate.message).oneof = {fields: []}; + string a = 1; + string b = 2; +} + +// MessageOneofRuleDuplicateField has the same field name twice in one entry. +message MessageOneofRuleDuplicateField { + option (buf.validate.message).oneof = {fields: ["a", "b", "a"]}; + string a = 1; + string b = 2; +} + +// MessageOneofRuleFieldInMultipleEntries has the same field in two +// different entries. +message MessageOneofRuleFieldInMultipleEntries { + option (buf.validate.message).oneof = {fields: ["a", "b"]}; + option (buf.validate.message).oneof = {fields: ["b", "c"]}; + string a = 1; + string b = 2; + string c = 3; +}