Repository: ogen-go/ogen
Pull request #614 (2022-10-11 21:46:44)

fix(parser): check `operationId` uniqueness among all operations


Patch:
diff --git a/openapi/parser/parse_path_item.go b/openapi/parser/parse_path_item.go
index 86fd5b20..352fb4b8 100644
--- a/openapi/parser/parse_path_item.go
+++ b/openapi/parser/parse_path_item.go
@@ -14,7 +14,6 @@ type pathItem = []*openapi.Operation
 func (p *parser) parsePathItem(
 	path string,
 	item *ogen.PathItem,
-	operationIDs map[string]struct{},
 	ctx *jsonpointer.ResolveCtx,
 ) (_ pathItem, rerr error) {
 	if item == nil {
@@ -26,7 +25,7 @@ func (p *parser) parsePathItem(
 	}()
 
 	if ref := item.Ref; ref != "" {
-		ops, err := p.resolvePathItem(path, ref, operationIDs, ctx)
+		ops, err := p.resolvePathItem(path, ref, ctx)
 		if err != nil {
 			return nil, p.wrapRef(ctx.LastLoc(), locator, err)
 		}
@@ -46,11 +45,11 @@ func (p *parser) parsePathItem(
 		}()
 
 		if id := op.OperationID; id != "" {
-			if _, ok := operationIDs[id]; ok {
+			if _, ok := p.operationIDs[id]; ok {
 				err := errors.Errorf("duplicate operationId: %q", id)
 				return p.wrapField("operationId", ctx.LastLoc(), locator, err)
 			}
-			operationIDs[id] = struct{}{}
+			p.operationIDs[id] = struct{}{}
 		}
 
 		parsedOp, err := p.parseOp(path, method, op, itemParams, ctx)
diff --git a/openapi/parser/parse_webhook.go b/openapi/parser/parse_webhook.go
index 69f6e52d..4c8a9d1b 100644
--- a/openapi/parser/parse_webhook.go
+++ b/openapi/parser/parse_webhook.go
@@ -10,9 +10,8 @@ import (
 )
 
 func (p *parser) parseWebhook(name string, item *ogen.PathItem, ctx *jsonpointer.ResolveCtx) (openapi.Webhook, error) {
-	operations := make(map[string]struct{})
 	// FIXME(tdakkota): we are passing "/" path, but webhook has no path.
-	pi, err := p.parsePathItem("/", item, operations, ctx)
+	pi, err := p.parsePathItem("/", item, ctx)
 	if err != nil {
 		return openapi.Webhook{}, errors.Wrap(err, "parse pathItem")
 	}
diff --git a/openapi/parser/parser.go b/openapi/parser/parser.go
index 33337b7a..b25960ce 100644
--- a/openapi/parser/parser.go
+++ b/openapi/parser/parser.go
@@ -34,6 +34,14 @@ type parser struct {
 	}
 	// securitySchemes contains security schemes defined in the root spec.
 	securitySchemes map[string]*ogen.SecurityScheme
+	// operationIDs holds operation IDs of already parsed operations.
+	//
+	// Spec says:
+	//
+	// 	The id MUST be unique among all operations described in the API.
+	//
+	// Used to detect duplicates.
+	operationIDs map[string]struct{}
 
 	external   jsonschema.ExternalResolver
 	schemas    map[string]*yaml.Node
@@ -52,8 +60,9 @@ func Parse(spec *ogen.Spec, s Settings) (*openapi.API, error) {
 
 	s.setDefaults()
 	p := &parser{
-		spec:       spec,
-		operations: nil,
+		spec:         spec,
+		operations:   nil,
+		operationIDs: map[string]struct{}{},
 		refs: struct {
 			requestBodies   map[refKey]*openapi.RequestBody
 			responses       map[refKey]*openapi.Response
@@ -127,8 +136,6 @@ func Parse(spec *ogen.Spec, s Settings) (*openapi.API, error) {
 
 func (p *parser) parsePathItems() error {
 	var (
-		// Maps to check for duplicate operationIds and paths.
-		operationIDs = map[string]struct{}{}
 		// paths contains simple paths, e.g. "/users/{id}" -> "/users/{}".
 		//
 		// OpenAPI spec says:
@@ -159,7 +166,7 @@ func (p *parser) parsePathItems() error {
 			return p.wrapLocation("", pathsLoc.Key(path), err)
 		}
 
-		ops, err := p.parsePathItem(path, item, operationIDs, jsonpointer.NewResolveCtx(p.depthLimit))
+		ops, err := p.parsePathItem(path, item, jsonpointer.NewResolveCtx(p.depthLimit))
 		if err != nil {
 			err := errors.Wrapf(err, "path %q", path)
 			return p.wrapLocation("", pathsLoc.Field(path), err)
diff --git a/openapi/parser/resolve.go b/openapi/parser/resolve.go
index a90bb2f0..dc8a8c38 100644
--- a/openapi/parser/resolve.go
+++ b/openapi/parser/resolve.go
@@ -223,7 +223,6 @@ func (p *parser) resolveSecurityScheme(ref string, ctx *jsonpointer.ResolveCtx)
 
 func (p *parser) resolvePathItem(
 	itemPath, ref string,
-	operationIDs map[string]struct{},
 	ctx *jsonpointer.ResolveCtx,
 ) (pathItem, error) {
 	const prefix = "#/components/pathItems/"
@@ -232,7 +231,7 @@ func (p *parser) resolvePathItem(
 		components: p.spec.Components.PathItems,
 		cache:      p.refs.pathItems,
 		parse: func(raw *ogen.PathItem, ctx *jsonpointer.ResolveCtx) (pathItem, error) {
-			return p.parsePathItem(itemPath, raw, operationIDs, ctx)
+			return p.parsePathItem(itemPath, raw, ctx)
 		},
 	}, ref, ctx)
 	if err != nil {


Test patch:
diff --git a/openapi/parser/_testdata/negative/operationId/duplicate_operationId_within_spec.json b/openapi/parser/_testdata/negative/operationId/duplicate_operationId_within_spec.json
new file mode 100644
index 00000000..d8d90fda
--- /dev/null
+++ b/openapi/parser/_testdata/negative/operationId/duplicate_operationId_within_spec.json
@@ -0,0 +1,31 @@
+{
+  "openapi": "3.1.0",
+  "info": {
+    "title": "title",
+    "version": "v0.1.0"
+  },
+  "webhooks": {
+    "usersGet": {
+      "get": {
+        "operationId": "usersGet",
+        "responses": {
+          "200": {
+            "description": "User info"
+          }
+        }
+      }
+    }
+  },
+  "paths": {
+    "/usesrs/get": {
+      "get": {
+        "operationId": "usersGet",
+        "responses": {
+          "200": {
+            "description": "User info"
+          }
+        }
+      }
+    }
+  }
+}
diff --git a/openapi/parser/_testdata/negative/paths/duplicate_operationId_within_pathItem.json b/openapi/parser/_testdata/negative/paths/duplicate_operationId_within_pathItem.json
new file mode 100644
index 00000000..00b68074
--- /dev/null
+++ b/openapi/parser/_testdata/negative/paths/duplicate_operationId_within_pathItem.json
@@ -0,0 +1,27 @@
+{
+  "openapi": "3.0.3",
+  "info": {
+    "title": "title",
+    "version": "v0.1.0"
+  },
+  "paths": {
+    "/users/get": {
+      "get": {
+        "operationId": "usersGet",
+        "responses": {
+          "200": {
+            "description": "User info"
+          }
+        }
+      },
+      "post": {
+        "operationId": "usersGet",
+        "responses": {
+          "200": {
+            "description": "User info"
+          }
+        }
+      }
+    }
+  }
+}
diff --git a/openapi/parser/_testdata/negative/paths/duplicate_operationId.json b/openapi/parser/_testdata/negative/paths/duplicate_operationId_within_paths.json
similarity index 100%
rename from openapi/parser/_testdata/negative/paths/duplicate_operationId.json
rename to openapi/parser/_testdata/negative/paths/duplicate_operationId_within_paths.json
diff --git a/openapi/parser/_testdata/negative/webhook/duplicate_operationId.json b/openapi/parser/_testdata/negative/webhook/duplicate_operationId_within_pathItem.json
similarity index 100%
rename from openapi/parser/_testdata/negative/webhook/duplicate_operationId.json
rename to openapi/parser/_testdata/negative/webhook/duplicate_operationId_within_pathItem.json
diff --git a/openapi/parser/_testdata/negative/webhook/duplicate_operationId_within_webhooks.json b/openapi/parser/_testdata/negative/webhook/duplicate_operationId_within_webhooks.json
new file mode 100644
index 00000000..051289f7
--- /dev/null
+++ b/openapi/parser/_testdata/negative/webhook/duplicate_operationId_within_webhooks.json
@@ -0,0 +1,29 @@
+{
+  "openapi": "3.1.0",
+  "info": {
+    "title": "title",
+    "version": "v0.1.0"
+  },
+  "webhooks": {
+    "usersGet": {
+      "get": {
+        "operationId": "usersGet",
+        "responses": {
+          "200": {
+            "description": "User info"
+          }
+        }
+      }
+    },
+    "usersPost": {
+      "post": {
+        "operationId": "usersGet",
+        "responses": {
+          "200": {
+            "description": "User info"
+          }
+        }
+      }
+    }
+  }
+}