Merge pull request #70 from mercari/remove_unnecessary_name_checks

Remove unnecessary reference name checks
mercari · Dec 1, 2023 · 5011a5f · 5011a5f
2 parents 1806b8a + 718b4bc
commit 5011a5f
Show file tree

Hide file tree

Showing 6 changed files with 75 additions and 245 deletions.
diff --git a/_examples/13_map/go.mod b/_examples/13_map/go.mod
@@ -13,7 +13,6 @@ require (
 	go.opentelemetry.io/otel/sdk v1.19.0
 	go.opentelemetry.io/otel/trace v1.19.0
 	golang.org/x/sync v0.4.0
-	google.golang.org/genproto/googleapis/rpc v0.0.0-20231012201019-e917dd12ba7a
 	google.golang.org/grpc v1.59.0
 	google.golang.org/protobuf v1.31.0
 )
@@ -34,4 +33,5 @@ require (
 	golang.org/x/sys v0.13.0 // indirect
 	golang.org/x/text v0.13.0 // indirect
 	google.golang.org/genproto/googleapis/api v0.0.0-20231002182017-d307bd883b97 // indirect
+	google.golang.org/genproto/googleapis/rpc v0.0.0-20231012201019-e917dd12ba7a // indirect
 )
diff --git a/resolver/graph.go b/resolver/graph.go
@@ -358,7 +358,7 @@ func CreateMessageDependencyGraph(ctx *context, baseMsg *Message) *MessageDepend
 	rule := baseMsg.Rule
 
 	var rootDefNodes []*MessageDependencyGraphNode
-	for idx, varDef := range rule.VariableDefinitions {
+	for _, varDef := range rule.VariableDefinitions {
 		varDefNode := newMessageDependencyGraphNodeByVariableDefinition(baseMsg, varDef)
 		if varDef.Name != "" {
 			nameToNode[varDef.Name] = varDefNode
@@ -382,12 +382,9 @@ func CreateMessageDependencyGraph(ctx *context, baseMsg *Message) *MessageDepend
 			}
 			node, exists := nameToNode[ref]
 			if !exists {
-				ctx.addError(
-					ErrWithLocation(
-						fmt.Sprintf(`%q name does not exist`, ref),
-						source.VariableDefinitionLocation(ctx.fileName(), baseMsg.Name, idx),
-					),
-				)
+				// It can be either a reference registered directly from a file descriptor such as an enum value
+				// or just an unknown reference name. cel-go detects the later case and returns an error, so we
+				// can safely ignore it here
 				continue
 			}
 			if _, exists := node.ChildrenMap[varDefNode]; !exists {
@@ -438,7 +435,7 @@ func CreateMessageDependencyGraphByFieldOneof(ctx *context, baseMsg *Message, fi
 	}
 
 	var rootDefNodes []*MessageDependencyGraphNode
-	for idx, varDef := range fieldOneof.VariableDefinitions {
+	for _, varDef := range fieldOneof.VariableDefinitions {
 		varDefNode := newMessageDependencyGraphNodeByVariableDefinition(baseMsg, varDef)
 		if varDef.Name != "" {
 			nameToNode[varDef.Name] = varDefNode
@@ -458,17 +455,9 @@ func CreateMessageDependencyGraphByFieldOneof(ctx *context, baseMsg *Message, fi
 				continue
 			}
 			if _, exists := allRefMap[ref]; !exists {
-				ctx.addError(
-					ErrWithLocation(
-						fmt.Sprintf(`%q name does not exist`, ref),
-						source.MessageFieldOneofDefLocation(
-							ctx.fileName(),
-							baseMsg.Name,
-							field.Name,
-							idx,
-						),
-					),
-				)
+				// It can be either a reference registered directly from a file descriptor such as an enum value
+				// or just an unknown reference name. cel-go detects the later case and returns an error, so we
+				// can safely ignore it here
 				continue
 			}
 
@@ -517,57 +506,7 @@ func CreateMessageDependencyGraphByFieldOneof(ctx *context, baseMsg *Message, fi
 	return graph
 }
 
-func CreateMessageDependencyGraphByValidationErrorDetailMessages(ctx *context, baseMsg *Message, messages VariableDefinitions) *MessageDependencyGraph {
-	allReferences := baseMsg.ReferenceNames()
-	allRefMap := make(map[string]struct{})
-	for _, ref := range allReferences {
-		allRefMap[ref] = struct{}{}
-	}
-
-	for msgIdx, msg := range messages {
-		for argIdx, arg := range msg.Expr.Message.Args {
-			refs := arg.Value.ReferenceNames()
-			if len(refs) == 0 {
-				continue
-			}
-			for _, ref := range refs {
-				if _, exists := allRefMap[ref]; !exists {
-					message := fmt.Sprintf(`%q name does not exist`, ref)
-					if arg.Value.Inline {
-						ctx.addError(
-							ErrWithLocation(
-								message,
-								source.VariableDefinitionValidationDetailMessageArgumentInlineLocation(
-									ctx.fileName(),
-									baseMsg.Name,
-									ctx.defIndex(),
-									ctx.errDetailIndex(),
-									msgIdx,
-									argIdx,
-								),
-							),
-						)
-					} else {
-						ctx.addError(
-							ErrWithLocation(
-								message,
-								source.VariableDefinitionValidationDetailMessageArgumentByLocation(
-									ctx.fileName(),
-									baseMsg.Name,
-									ctx.defIndex(),
-									ctx.errDetailIndex(),
-									msgIdx,
-									argIdx,
-								),
-							),
-						)
-					}
-					continue
-				}
-			}
-		}
-	}
-
+func CreateMessageDependencyGraphByValidationErrorDetailMessages(baseMsg *Message, messages VariableDefinitions) *MessageDependencyGraph {
 	// Each node won't depend on each other, so they all should be a root
 	var roots []*MessageDependencyGraphNode
 	for _, msg := range messages {

diff --git a/resolver/resolver.go b/resolver/resolver.go
@@ -2903,7 +2903,7 @@ func (r *Resolver) resolveMessageDependencies(ctx *context, files []*File) {
 			ctx := ctx.withDefIndex(defIdx)
 			for detIdx, detail := range def.Expr.Validation.Error.Details {
 				ctx := ctx.withErrDetailIndex(detIdx)
-				if graph := CreateMessageDependencyGraphByValidationErrorDetailMessages(ctx, msg, detail.Messages); graph != nil {
+				if graph := CreateMessageDependencyGraphByValidationErrorDetailMessages(msg, detail.Messages); graph != nil {
 					detail.DependencyGraph = graph
 					detail.Resolvers = graph.MessageResolverGroups(ctx)
 				}

diff --git a/source/location.go b/source/location.go
@@ -497,22 +497,6 @@ func MessageFieldByLocation(fileName, msgName, fieldName string) *Location {
 	}
 }
 
-// MessageFieldAliasLocation creates location for alias in grpc.federation.field option.
-func MessageFieldAliasLocation(fileName, msgName, fieldName string) *Location {
-	return &Location{
-		FileName: fileName,
-		Message: &Message{
-			Name: msgName,
-			Field: &Field{
-				Name: fieldName,
-				Option: &FieldOption{
-					Alias: true,
-				},
-			},
-		},
-	}
-}
-
 // MessageFieldOneofLocation creates location for oneof in grpc.federation.field option.
 func MessageFieldOneofLocation(fileName, msgName, fieldName string) *Location {
 	return &Location{
@@ -581,26 +565,6 @@ func MessageFieldOneofByLocation(fileName, msgName, fieldName string) *Location
 	}
 }
 
-// MessageFieldOneofDefLocation creates location for def in grpc.federation.field.oneof option.
-func MessageFieldOneofDefLocation(fileName, msgName, fieldName string, idx int) *Location {
-	return &Location{
-		FileName: fileName,
-		Message: &Message{
-			Name: msgName,
-			Field: &Field{
-				Name: fieldName,
-				Option: &FieldOption{
-					Oneof: &FieldOneof{
-						VariableDefinitions: &VariableDefinitionOption{
-							Idx: idx,
-						},
-					},
-				},
-			},
-		},
-	}
-}
-
 // MessageFieldOneofDefMessageLocation creates location for def[*].message in grpc.federation.field.oneof.
 func MessageFieldOneofDefMessageLocation(fileName, msgName, fieldName string, idx int) *Location {
 	return &Location{
@@ -647,83 +611,6 @@ func MessageFieldOneofDefMessageArgumentLocation(fileName, msgName, fieldName st
 	}
 }
 
-// MessageFieldOneofDefMessageArgumentNameLocation creates location for def[*].message.args[].name in grpc.federation.field.oneof.
-func MessageFieldOneofDefMessageArgumentNameLocation(fileName, msgName, fieldName string, idx, argIdx int) *Location {
-	return &Location{
-		FileName: fileName,
-		Message: &Message{
-			Name: msgName,
-			Field: &Field{
-				Option: &FieldOption{
-					Oneof: &FieldOneof{
-						VariableDefinitions: &VariableDefinitionOption{
-							Idx: idx,
-							Message: &MessageExprOption{
-								Args: &ArgumentOption{
-									Idx:  argIdx,
-									Name: true,
-								},
-							},
-						},
-					},
-				},
-			},
-		},
-	}
-}
-
-// MessageFieldOneofDefMessageArgumentByLocation creates location for def[*].message.args[].by in grpc.federation.field.oneof.
-func MessageFieldOneofDefMessageArgumentByLocation(fileName, msgName, fieldName string, idx, argIdx int) *Location {
-	return &Location{
-		FileName: fileName,
-		Message: &Message{
-			Name: msgName,
-			Field: &Field{
-				Name: fieldName,
-				Option: &FieldOption{
-					Oneof: &FieldOneof{
-						VariableDefinitions: &VariableDefinitionOption{
-							Idx: idx,
-							Message: &MessageExprOption{
-								Args: &ArgumentOption{
-									Idx: argIdx,
-									By:  true,
-								},
-							},
-						},
-					},
-				},
-			},
-		},
-	}
-}
-
-// MessageFieldOneofDefMessageArgumentInlineLocation creates location for def[*].message.args[].inline in grpc.federation.field.oneof.
-func MessageFieldOneofDefMessageArgumentInlineLocation(fileName, msgName, fieldName string, idx, argIdx int) *Location {
-	return &Location{
-		FileName: fileName,
-		Message: &Message{
-			Name: msgName,
-			Field: &Field{
-				Name: fieldName,
-				Option: &FieldOption{
-					Oneof: &FieldOneof{
-						VariableDefinitions: &VariableDefinitionOption{
-							Idx: idx,
-							Message: &MessageExprOption{
-								Args: &ArgumentOption{
-									Idx:    argIdx,
-									Inline: true,
-								},
-							},
-						},
-					},
-				},
-			},
-		},
-	}
-}
-
 // MessageOptionLocation creates location for grpc.federaiton.message option.
 func MessageOptionLocation(fileName, msgName string) *Location {
 	return &Location{
@@ -1427,61 +1314,3 @@ func VariableDefinitionValidationDetailMessageArgumentLocation(fileName, msgName
 		},
 	}
 }
-
-// VariableDefinitionValidationDetailMessageArgumentByLocation creates location for def.validation[*].error.details[*].message[*].args[*].by in grpc.federation.message.
-func VariableDefinitionValidationDetailMessageArgumentByLocation(fileName, msgName string, defIdx, detIdx, msgIdx, argIdx int) *Location {
-	return &Location{
-		FileName: fileName,
-		Message: &Message{
-			Name: msgName,
-			Option: &MessageOption{
-				VariableDefinitions: &VariableDefinitionOption{
-					Idx: defIdx,
-					Validation: &ValidationExprOption{
-						Detail: &ValidationDetailOption{
-							Idx: detIdx,
-							Message: &ValidationDetailMessageOption{
-								Idx: msgIdx,
-								Message: &MessageExprOption{
-									Args: &ArgumentOption{
-										Idx: argIdx,
-										By:  true,
-									},
-								},
-							},
-						},
-					},
-				},
-			},
-		},
-	}
-}
-
-// VariableDefinitionValidationDetailMessageArgumentInlineLocation creates location for def.validation[*].error.details[*].message[*].args[*].inline in grpc.federation.message.
-func VariableDefinitionValidationDetailMessageArgumentInlineLocation(fileName, msgName string, defIdx, detIdx, msgIdx, argIdx int) *Location {
-	return &Location{
-		FileName: fileName,
-		Message: &Message{
-			Name: msgName,
-			Option: &MessageOption{
-				VariableDefinitions: &VariableDefinitionOption{
-					Idx: defIdx,
-					Validation: &ValidationExprOption{
-						Detail: &ValidationDetailOption{
-							Idx: detIdx,
-							Message: &ValidationDetailMessageOption{
-								Idx: msgIdx,
-								Message: &MessageExprOption{
-									Args: &ArgumentOption{
-										Idx:    argIdx,
-										Inline: true,
-									},
-								},
-							},
-						},
-					},
-				},
-			},
-		},
-	}
-}
diff --git a/validator/testdata/valid_enum_value_reference.proto b/validator/testdata/valid_enum_value_reference.proto
@@ -0,0 +1,54 @@
+syntax = "proto3";
+
+package org.federation;
+
+import "federation.proto";
+import "nested_post.proto";
+
+option go_package = "example/federation;federation";
+
+service FederationService {
+  option (grpc.federation.service) = {
+    dependencies: [
+      { name: "post_service", service: "org.post.PostService" }
+    ]
+  };
+  rpc GetPost(GetPostRequest) returns (GetPostResponse) {};
+}
+
+message GetPostRequest {
+  string id = 1;
+}
+
+message GetPostResponse {
+  option (grpc.federation.message) = {
+    def {
+      name: "post"
+      message {
+        name: "Post"
+        args { name: "id", by: "$.id" }
+      }
+    }
+  };
+  Post post = 1 [(grpc.federation.field).by = "post"];
+}
+
+message Post {
+  option (grpc.federation.message) = {
+    def {
+      name: "res"
+      call {
+        method: "org.post.PostService/GetPost"
+        request { field: "id", by: "$.id" }
+      }
+    }
+    def { name: "post", by: "res.post", autobind: true }
+  };
+  string id = 1;
+  PostType type = 2 [(grpc.federation.field).by = "org.federation.PostType.FICTION"];
+}
+
+enum PostType {
+  UNKNOWN = 0;
+  FICTION = 1;
+}