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; +}