Repository: ogen-go/ogen
Pull request #469 (2022-07-08 05:15:13)

fix(parser): ensure that paths are not duplicated


Patch:
diff --git a/openapi/operation.go b/openapi/operation.go
index 28b3ed20..394d81a9 100644
--- a/openapi/operation.go
+++ b/openapi/operation.go
@@ -34,6 +34,20 @@ type Operation struct {
 // Path is an operation path.
 type Path []PathPart
 
+// ID returns path, but without parameter names.
+//
+// For example, if path is "/users/{id}", ID returns "/users/{}".
+func (p Path) ID() (path string) {
+	for _, part := range p {
+		if part.Raw != "" {
+			path += part.Raw
+			continue
+		}
+		path += "{}"
+	}
+	return
+}
+
 // String implements fmt.Stringer.
 func (p Path) String() (path string) {
 	for _, part := range p {
diff --git a/openapi/parser/parser.go b/openapi/parser/parser.go
index 27ad1e9f..7cc1da59 100644
--- a/openapi/parser/parser.go
+++ b/openapi/parser/parser.go
@@ -95,16 +95,39 @@ func Parse(spec *ogen.Spec, s Settings) (*openapi.API, error) {
 }
 
 func (p *parser) parsePathItems() error {
-	operationIDs := make(map[string]struct{})
+	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:
+		//
+		// 	Templated paths with the same hierarchy but different templated
+		//	names MUST NOT exist as they are identical.
+		//
+		paths = map[string]struct{}{}
+	)
 	for path, item := range p.spec.Paths {
-		if err := p.parsePath(path, item, operationIDs); err != nil {
+		if err := func() error {
+			id, err := pathID(path)
+			if err != nil {
+				return err
+			}
+
+			if _, ok := paths[id]; ok {
+				return errors.New("duplicate path")
+			}
+			paths[id] = struct{}{}
+
+			return p.parsePathItem(path, item, operationIDs)
+		}(); err != nil {
 			return errors.Wrapf(err, "path %q", path)
 		}
 	}
 	return nil
 }
 
-func (p *parser) parsePath(path string, item *ogen.PathItem, operationIDs map[string]struct{}) (rerr error) {
+func (p *parser) parsePathItem(path string, item *ogen.PathItem, operationIDs map[string]struct{}) (rerr error) {
 	if item == nil {
 		return errors.New("pathItem object is empty or null")
 	}
diff --git a/openapi/parser/path_parser.go b/openapi/parser/path_parser.go
index b83504f0..9a97cfc8 100644
--- a/openapi/parser/path_parser.go
+++ b/openapi/parser/path_parser.go
@@ -10,28 +10,55 @@ import (
 )
 
 type pathParser struct {
-	path   string               // immutable
-	params []*openapi.Parameter // immutable
+	path   string // immutable
+	lookup func(name string) (*openapi.Parameter, bool)
 
 	parts []openapi.PathPart // parsed parts
 	part  []rune             // current part
 	param bool               // current part is param name?
 }
 
-func parsePath(path string, params []*openapi.Parameter) ([]openapi.PathPart, error) {
+func pathID(path string) (string, error) {
+	p, err := (&pathParser{
+		path: path,
+		lookup: func(name string) (*openapi.Parameter, bool) {
+			return nil, true
+		},
+	}).Parse()
+	if err != nil {
+		return "", err
+	}
+	return p.ID(), nil
+}
+
+func parsePath(path string, params []*openapi.Parameter) (openapi.Path, error) {
 	return (&pathParser{
-		path:   path,
-		params: params,
+		path: path,
+		lookup: func(name string) (*openapi.Parameter, bool) {
+			for _, p := range params {
+				if p.Name == name && p.In == openapi.LocationPath {
+					return p, true
+				}
+			}
+			return nil, false
+		},
 	}).Parse()
 }
 
-func (p *pathParser) Parse() ([]openapi.PathPart, error) {
+func (p *pathParser) Parse() (openapi.Path, error) {
 	err := p.parse()
 	return p.parts, err
 }
 
 func (p *pathParser) parse() error {
 	// Validate and unescape path.
+	//
+	// FIXME(tdakkota): OpenAPI spec, as always, is not clear about path validation.
+	//  All we know is that it MUST start with a slash.
+	// 	At the same time, https://swagger.io/docs/specification/paths-and-operations/ says that
+	// 	paths must not include query parameters.
+	//  In summary, we do not pass URL scheme, user info, host or query string.
+	//
 	u, err := url.Parse(p.path)
 	if err != nil {
 		return err
@@ -39,12 +66,10 @@ func (p *pathParser) parse() error {
 	switch {
 	case u.IsAbs() || u.Host != "" || u.User != nil:
 		return errors.New("path MUST be relative")
-	case u.Path == "" || !strings.HasPrefix(u.Path, "/"):
+	case !strings.HasPrefix(u.Path, "/"):
 		return errors.New("path MUST begin with a forward slash")
 	case u.RawQuery != "":
 		return errors.New("path MUST NOT contain a query string")
-	case u.Fragment != "" || u.RawFragment != "":
-		return errors.New("path MUST NOT contain a fragment")
 	}
 
 	for _, r := range u.Path {
@@ -96,7 +121,7 @@ func (p *pathParser) push() error {
 		return nil
 	}
 
-	param, found := p.lookupParam(string(p.part))
+	param, found := p.lookup(string(p.part))
 	if !found {
 		return errors.Errorf("path parameter not specified: %q", string(p.part))
 	}
@@ -104,12 +129,3 @@ func (p *pathParser) push() error {
 	p.parts = append(p.parts, openapi.PathPart{Param: param})
 	return nil
 }
-
-func (p *pathParser) lookupParam(name string) (*openapi.Parameter, bool) {
-	for _, p := range p.params {
-		if p.Name == name && p.In == openapi.LocationPath {
-			return p, true
-		}
-	}
-	return nil, false
-}


Test patch:
diff --git a/_testdata/examples/superset.json b/_testdata/examples/superset.json
index d5b57088..6d74eae3 100644
--- a/_testdata/examples/superset.json
+++ b/_testdata/examples/superset.json
@@ -12172,18 +12172,16 @@
         "tags": [
           "Dashboards"
         ]
-      }
-    },
-    "/api/v1/dashboard/{id_or_slug}/charts": {
-      "get": {
-        "description": "Get the chart definitions for a given dashboard",
+      },
+      "delete": {
+        "description": "Deletes a Dashboard.",
         "parameters": [
           {
             "in": "path",
             "name": "id_or_slug",
             "required": true,
             "schema": {
-              "type": "string"
+              "type": "integer"
             }
           }
         ],
@@ -12193,24 +12191,15 @@
               "application/json": {
                 "schema": {
                   "properties": {
-                    "result": {
-                      "items": {
-                        "$ref": "#/components/schemas/ChartEntityResponseSchema"
-                      },
-                      "type": "array"
+                    "message": {
+                      "type": "string"
                     }
                   },
                   "type": "object"
                 }
               }
             },
-            "description": "Dashboard chart definitions"
-          },
-          "302": {
-            "description": "Redirects to the current digest"
-          },
-          "400": {
-            "$ref": "#/components/responses/400"
+            "description": "Dashboard deleted"
           },
           "401": {
             "$ref": "#/components/responses/401"
@@ -12220,6 +12209,12 @@
           },
           "404": {
             "$ref": "#/components/responses/404"
+          },
+          "422": {
+            "$ref": "#/components/responses/422"
+          },
+          "500": {
+            "$ref": "#/components/responses/500"
           }
         },
         "security": [
@@ -12230,43 +12225,51 @@
         "tags": [
           "Dashboards"
         ]
-      }
-    },
-    "/api/v1/dashboard/{id_or_slug}/datasets": {
-      "get": {
-        "description": "Returns a list of a dashboard's datasets. Each dataset includes only the information necessary to render the dashboard's charts.",
+      },
+      "put": {
+        "description": "Changes a Dashboard.",
         "parameters": [
           {
-            "description": "Either the id of the dashboard, or its slug",
             "in": "path",
             "name": "id_or_slug",
             "required": true,
             "schema": {
-              "type": "string"
+              "type": "integer"
             }
           }
         ],
+        "requestBody": {
+          "content": {
+            "application/json": {
+              "schema": {
+                "$ref": "#/components/schemas/DashboardRestApi.put"
+              }
+            }
+          },
+          "description": "Dashboard schema",
+          "required": true
+        },
         "responses": {
           "200": {
             "content": {
               "application/json": {
                 "schema": {
                   "properties": {
+                    "id": {
+                      "type": "number"
+                    },
+                    "last_modified_time": {
+                      "type": "number"
+                    },
                     "result": {
-                      "items": {
-                        "$ref": "#/components/schemas/DashboardDatasetSchema"
-                      },
-                      "type": "array"
+                      "$ref": "#/components/schemas/DashboardRestApi.put"
                     }
                   },
                   "type": "object"
                 }
               }
             },
-            "description": "Dashboard dataset definitions"
-          },
-          "302": {
-            "description": "Redirects to the current digest"
+            "description": "Dashboard changed"
           },
           "400": {
             "$ref": "#/components/responses/400"
@@ -12279,6 +12282,12 @@
           },
           "404": {
             "$ref": "#/components/responses/404"
+          },
+          "422": {
+            "$ref": "#/components/responses/422"
+          },
+          "500": {
+            "$ref": "#/components/responses/500"
           }
         },
         "security": [
@@ -12291,16 +12300,16 @@
         ]
       }
     },
-    "/api/v1/dashboard/{pk}": {
-      "delete": {
-        "description": "Deletes a Dashboard.",
+    "/api/v1/dashboard/{id_or_slug}/charts": {
+      "get": {
+        "description": "Get the chart definitions for a given dashboard",
         "parameters": [
           {
             "in": "path",
-            "name": "pk",
+            "name": "id_or_slug",
             "required": true,
             "schema": {
-              "type": "integer"
+              "type": "string"
             }
           }
         ],
@@ -12310,15 +12319,24 @@
               "application/json": {
                 "schema": {
                   "properties": {
-                    "message": {
-                      "type": "string"
+                    "result": {
+                      "items": {
+                        "$ref": "#/components/schemas/ChartEntityResponseSchema"
+                      },
+                      "type": "array"
                     }
                   },
                   "type": "object"
                 }
               }
             },
-            "description": "Dashboard deleted"
+            "description": "Dashboard chart definitions"
+          },
+          "302": {
+            "description": "Redirects to the current digest"
+          },
+          "400": {
+            "$ref": "#/components/responses/400"
           },
           "401": {
             "$ref": "#/components/responses/401"
@@ -12328,12 +12346,6 @@
           },
           "404": {
             "$ref": "#/components/responses/404"
-          },
-          "422": {
-            "$ref": "#/components/responses/422"
-          },
-          "500": {
-            "$ref": "#/components/responses/500"
           }
         },
         "security": [
@@ -12344,51 +12356,43 @@
         "tags": [
           "Dashboards"
         ]
-      },
-      "put": {
-        "description": "Changes a Dashboard.",
+      }
+    },
+    "/api/v1/dashboard/{id_or_slug}/datasets": {
+      "get": {
+        "description": "Returns a list of a dashboard's datasets. Each dataset includes only the information necessary to render the dashboard's charts.",
         "parameters": [
           {
+            "description": "Either the id of the dashboard, or its slug",
             "in": "path",
-            "name": "pk",
+            "name": "id_or_slug",
             "required": true,
             "schema": {
-              "type": "integer"
+              "type": "string"
             }
           }
         ],
-        "requestBody": {
-          "content": {
-            "application/json": {
-              "schema": {
-                "$ref": "#/components/schemas/DashboardRestApi.put"
-              }
-            }
-          },
-          "description": "Dashboard schema",
-          "required": true
-        },
         "responses": {
           "200": {
             "content": {
               "application/json": {
                 "schema": {
                   "properties": {
-                    "id": {
-                      "type": "number"
-                    },
-                    "last_modified_time": {
-                      "type": "number"
-                    },
                     "result": {
-                      "$ref": "#/components/schemas/DashboardRestApi.put"
+                      "items": {
+                        "$ref": "#/components/schemas/DashboardDatasetSchema"
+                      },
+                      "type": "array"
                     }
                   },
                   "type": "object"
                 }
               }
             },
-            "description": "Dashboard changed"
+            "description": "Dashboard dataset definitions"
+          },
+          "302": {
+            "description": "Redirects to the current digest"
           },
           "400": {
             "$ref": "#/components/responses/400"
@@ -12401,12 +12405,6 @@
           },
           "404": {
             "$ref": "#/components/responses/404"
-          },
-          "422": {
-            "$ref": "#/components/responses/422"
-          },
-          "500": {
-            "$ref": "#/components/responses/500"
           }
         },
         "security": [
diff --git a/_testdata/negative/paths/duplicate_path_item.json b/_testdata/negative/paths/duplicate_path_item.json
new file mode 100644
index 00000000..3a0a7c55
--- /dev/null
+++ b/_testdata/negative/paths/duplicate_path_item.json
@@ -0,0 +1,47 @@
+{
+  "openapi": "3.0.3",
+  "info": {
+    "title": "title",
+    "version": "v0.1.0"
+  },
+  "paths": {
+    "/foo/{param1}": {
+      "parameters": [
+        {
+          "name": "param1",
+          "in": "path",
+          "required": true,
+          "schema": {
+            "type": "string"
+          }
+        }
+      ],
+      "get": {
+        "responses": {
+          "200": {
+            "description": "User info"
+          }
+        }
+      }
+    },
+    "/foo/{param2}": {
+      "parameters": [
+        {
+          "name": "param2",
+          "in": "path",
+          "required": true,
+          "schema": {
+            "type": "string"
+          }
+        }
+      ],
+      "get": {
+        "responses": {
+          "200": {
+            "description": "User info"
+          }
+        }
+      }
+    }
+  }
+}
diff --git a/_testdata/negative/parameters/optional_path_parameter.json b/_testdata/negative/paths/optional_path_parameter.json
similarity index 95%
rename from _testdata/negative/parameters/optional_path_parameter.json
rename to _testdata/negative/paths/optional_path_parameter.json
index cb14a1f4..116cc321 100644
--- a/_testdata/negative/parameters/optional_path_parameter.json
+++ b/_testdata/negative/paths/optional_path_parameter.json
@@ -23,7 +23,7 @@
   "components": {
     "parameters": {
       "Parameter": {
-        "name": "parm",
+        "name": "param",
         "in": "path",
         "required": false,
         "schema": {
diff --git a/openapi/parser/path_parser_test.go b/openapi/parser/path_parser_test.go
index 83b4b74a..c02b30f6 100644
--- a/openapi/parser/path_parser_test.go
+++ b/openapi/parser/path_parser_test.go
@@ -27,13 +27,13 @@ func TestPathParser(t *testing.T) {
 	tests := []struct {
 		Path      string
 		Params    []*openapi.Parameter
-		Expect    []openapi.PathPart
+		Expect    openapi.Path
 		ExpectErr string
 	}{
 		{
 			Path:   "/foo/{bar}",
 			Params: []*openapi.Parameter{bar},
-			Expect: []openapi.PathPart{
+			Expect: openapi.Path{
 				{Raw: "/foo/"},
 				{Param: bar},
 			},
@@ -41,7 +41,7 @@ func TestPathParser(t *testing.T) {
 		{
 			Path:   "/foo.{bar}",
 			Params: []*openapi.Parameter{bar},
-			Expect: []openapi.PathPart{
+			Expect: openapi.Path{
 				{Raw: "/foo."},
 				{Param: bar},
 			},
@@ -49,7 +49,7 @@ func TestPathParser(t *testing.T) {
 		{
 			Path:   "/foo.{bar}.{baz}abc/def",
 			Params: []*openapi.Parameter{bar, baz},
-			Expect: []openapi.PathPart{
+			Expect: openapi.Path{
 				{Raw: "/foo."},
 				{Param: bar},
 				{Raw: "."},
@@ -59,7 +59,7 @@ func TestPathParser(t *testing.T) {
 		},
 		{
 			Path: "/foo%20bar",
-			Expect: []openapi.PathPart{
+			Expect: openapi.Path{
 				{Raw: "/foo bar"},
 			},
 		},
@@ -100,10 +100,6 @@ func TestPathParser(t *testing.T) {
 			Path:      "/foo?k=v",
 			ExpectErr: `path MUST NOT contain a query string`,
 		},
-		{
-			Path:      "/foo#frag",
-			ExpectErr: `path MUST NOT contain a fragment`,
-		},
 		{
 			Path:      "https://i-want-to-die.org/foo",
 			ExpectErr: `path MUST be relative`,