add validation for variable name

mercari · Aug 16, 2024 · 5eecbe8 · 5eecbe8
1 parent 81ff065
commit 5eecbe8
Show file tree

Hide file tree

Showing 4 changed files with 147 additions and 1 deletion.
diff --git a/resolver/resolver.go b/resolver/resolver.go
@@ -1139,10 +1139,73 @@ func (r *Resolver) validateMessages(ctx *context, msgs []*Message) {
 		ctx := ctx.withFile(msg.File).withMessage(msg)
 		mb := newMessageBuilderFromMessage(msg)
 		r.validateMessageFields(ctx, msg, mb)
+		r.validateDuplicatedVariableName(ctx, msg, mb)
 		// Don't have to check msg.NestedMessages since r.allMessages(files) already includes nested messages
 	}
 }
 
+func (r *Resolver) validateDuplicatedVariableName(ctx *context, msg *Message, builder *source.MessageBuilder) {
+	if msg.Rule == nil {
+		return
+	}
+	optBuilder := builder.WithOption()
+	nameMap := make(map[string]struct{})
+	for idx, def := range msg.Rule.DefSet.Definitions() {
+		r.validateDuplicatedVariableNameWithDef(ctx, nameMap, def, optBuilder.WithDef(idx))
+	}
+	for _, field := range msg.Fields {
+		if field.Rule == nil {
+			continue
+		}
+		if field.Rule.Oneof == nil {
+			continue
+		}
+		builder := builder.WithField(field.Name).WithOption().WithOneOf()
+		for idx, def := range field.Rule.Oneof.DefSet.Definitions() {
+			r.validateDuplicatedVariableNameWithDef(ctx, nameMap, def, builder.WithDef(idx))
+		}
+	}
+}
+
+func (r *Resolver) validateDuplicatedVariableNameWithDef(ctx *context, nameMap map[string]struct{}, def *VariableDefinition, builder *source.VariableDefinitionOptionBuilder) {
+	if def.Expr == nil {
+		return
+	}
+	if def.Name != "" {
+		if _, exists := nameMap[def.Name]; exists {
+			ctx.addError(
+				ErrWithLocation(
+					fmt.Sprintf("found duplicated variable name %q", def.Name),
+					builder.WithName().Location(),
+				),
+			)
+		}
+	}
+	nameMap[def.Name] = struct{}{}
+	expr := def.Expr
+	switch {
+	case expr.Call != nil:
+		builder := builder.WithCall()
+		for idx, grpcErr := range expr.Call.Errors {
+			r.validateDuplicatedVariableNameWithGRPCError(ctx, nameMap, grpcErr, builder.WithError(idx))
+		}
+	case expr.Validation != nil:
+		r.validateDuplicatedVariableNameWithGRPCError(ctx, nameMap, expr.Validation.Error, builder.WithValidation().WithError())
+	}
+}
+
+func (r *Resolver) validateDuplicatedVariableNameWithGRPCError(ctx *context, nameMap map[string]struct{}, grpcErr *GRPCError, builder *source.GRPCErrorOptionBuilder) {
+	for idx, def := range grpcErr.DefSet.Definitions() {
+		r.validateDuplicatedVariableNameWithDef(ctx, nameMap, def, builder.WithDef(idx))
+	}
+	for detailIdx, detail := range grpcErr.Details {
+		builder := builder.WithDetail(detailIdx)
+		for defIdx, def := range detail.DefSet.Definitions() {
+			r.validateDuplicatedVariableNameWithDef(ctx, nameMap, def, builder.WithDef(defIdx))
+		}
+	}
+}
+
 func (r *Resolver) validateMessageFields(ctx *context, msg *Message, builder *source.MessageBuilder) {
 	if msg.Rule == nil {
 		return

diff --git a/source/file.go b/source/file.go
@@ -1418,7 +1418,10 @@ func (f *File) nodeInfoByGRPCError(list []*ast.MessageLiteralNode, opt *GRPCErro
 		return nil
 	}
 	node := list[opt.Idx]
-	var errDetails []*ast.MessageLiteralNode
+	var (
+		defs       []*ast.MessageLiteralNode
+		errDetails []*ast.MessageLiteralNode
+	)
 	for _, elem := range node.Elements {
 		fieldName := elem.Name.Name.AsIdentifier()
 		switch {
@@ -1440,10 +1443,15 @@ func (f *File) nodeInfoByGRPCError(list []*ast.MessageLiteralNode, opt *GRPCErro
 				return nil
 			}
 			return f.nodeInfo(value)
+		case opt.Def != nil && fieldName == "def":
+			defs = append(defs, f.getMessageListFromNode(elem.Val)...)
 		case opt.Detail != nil && fieldName == "details":
 			errDetails = append(errDetails, f.getMessageListFromNode(elem.Val)...)
 		}
 	}
+	if len(defs) != 0 {
+		return f.nodeInfoByDef(defs, opt.Def)
+	}
 	if len(errDetails) != 0 {
 		return f.nodeInfoByGRPCErrorDetail(errDetails, opt.Detail)
 	}
@@ -1456,6 +1464,7 @@ func (f *File) nodeInfoByGRPCErrorDetail(list []*ast.MessageLiteralNode, detail
 	}
 	node := list[detail.Idx]
 	var (
+		defs                 []*ast.MessageLiteralNode
 		messages             []*ast.MessageLiteralNode
 		preconditionFailures []*ast.MessageLiteralNode
 		badRequests          []*ast.MessageLiteralNode
@@ -1464,6 +1473,8 @@ func (f *File) nodeInfoByGRPCErrorDetail(list []*ast.MessageLiteralNode, detail
 	for _, elem := range node.Elements {
 		fieldName := elem.Name.Name.AsIdentifier()
 		switch {
+		case detail.Def != nil && fieldName == "def":
+			defs = append(defs, f.getMessageListFromNode(elem.Val)...)
 		case detail.If && fieldName == "if":
 			value, ok := elem.Val.(*ast.StringLiteralNode)
 			if !ok {
@@ -1486,6 +1497,9 @@ func (f *File) nodeInfoByGRPCErrorDetail(list []*ast.MessageLiteralNode, detail
 			localizedMessages = append(localizedMessages, f.getMessageListFromNode(elem.Val)...)
 		}
 	}
+	if len(defs) != 0 {
+		return f.nodeInfoByDef(defs, detail.Def)
+	}
 	if len(messages) != 0 {
 		return f.nodeInfoByDef(messages, detail.Message)
 	}

diff --git a/validator/testdata/duplicated_variable_name.proto b/validator/testdata/duplicated_variable_name.proto
@@ -0,0 +1,52 @@
+syntax = "proto3";
+
+package org.federation;
+
+import "federation.proto";
+import "echo.proto";
+
+option go_package = "example/federation;federation";
+
+service FederationService {
+  option (grpc.federation.service) = {};
+  rpc Get(GetRequest) returns (GetResponse) {};
+}
+
+message GetRequest {}
+
+message GetResponse {
+  option (grpc.federation.message) = {
+    def [
+      { name: "a" by: "0" },
+      {
+        validation {
+          name: "a"
+          error {
+            def { name: "a" by: "1" }
+            details {
+              def { name: "a" by: "2" }
+            }
+          }
+        }
+      },
+      {
+        call {
+          method: "echo.EchoService/Echo"
+          error {
+            def { name: "a" by: "3" }
+            details {
+              def { name: "a" by: "4" }
+            }
+          }
+        }
+      }
+    ]
+  };
+  oneof o {
+    string foo = 1 [(grpc.federation.field).oneof = {
+      default: true
+      def { name: "a" by: "5" }
+      by: "'foo'"
+    }];
+  }
+}
diff --git a/validator/validator_test.go b/validator/validator_test.go
@@ -24,6 +24,23 @@ func TestValidator(t *testing.T) {
 testdata/different_message_argument_type.proto:28:14: "id" argument name is declared with a different type kind. found "string" and "int64" type
 28:          args { name: "id" by: "1" }
                   ^
+`},
+		{file: "duplicated_variable_name.proto", expected: `
+testdata/duplicated_variable_name.proto:25:25: found duplicated variable name "a"
+25:              def { name: "a" by: "1" }
+                             ^
+testdata/duplicated_variable_name.proto:27:27: found duplicated variable name "a"
+27:                def { name: "a" by: "2" }
+                               ^
+testdata/duplicated_variable_name.proto:36:25: found duplicated variable name "a"
+36:              def { name: "a" by: "3" }
+                             ^
+testdata/duplicated_variable_name.proto:38:27: found duplicated variable name "a"
+38:                def { name: "a" by: "4" }
+                               ^
+testdata/duplicated_variable_name.proto:48:19: found duplicated variable name "a"
+48:        def { name: "a" by: "5" }
+                       ^
 `},
 		{file: "invalid_autobind.proto", expected: `
 testdata/invalid_autobind.proto:23:3: "id" field found multiple times in the message specified by autobind. since it is not possible to determine one, please use "grpc.federation.field" to explicitly bind it. found message names are "a" name at def and "b" name at def