From e89509c3af809c0ebddb982e30937d5f7c8fc539 Mon Sep 17 00:00:00 2001 From: Frederic BIDON Date: Thu, 30 Nov 2023 22:43:54 +0100 Subject: [PATCH] Revert fix #146 A more thorough investigation on loading specs from a generated server from paths containing special characters (such as blank spaces, parenthesis) did not conclude that it was necessary to alter how cache entries are matched. Indeed, this change deeply altered how keys in the cache are managed and was therefore at best, a risky bet. * reverted commit e8e27ff * added check to ensure that the root pseudo-document is preloaded in cache under all conditions * added more unit tests to check the behavior of the normalizers against escaped characters * closes #145 Signed-off-by: Frederic BIDON --- .gitignore | 3 +- expander.go | 16 +- expander_test.go | 47 +++ .../145/Program Files (x86)/AppName/ref.json | 17 + .../AppName/todos.common.json | 103 ++++++ .../Program Files (x86)/AppName/todos.json | 336 ++++++++++++++++++ normalizer_test.go | 29 ++ schema_loader.go | 9 +- schema_loader_test.go | 33 ++ 9 files changed, 578 insertions(+), 15 deletions(-) create mode 100644 fixtures/bugs/145/Program Files (x86)/AppName/ref.json create mode 100644 fixtures/bugs/145/Program Files (x86)/AppName/todos.common.json create mode 100644 fixtures/bugs/145/Program Files (x86)/AppName/todos.json create mode 100644 schema_loader_test.go diff --git a/.gitignore b/.gitignore index dd91ed6..f47cb20 100644 --- a/.gitignore +++ b/.gitignore @@ -1,2 +1 @@ -secrets.yml -coverage.out +*.out diff --git a/expander.go b/expander.go index 012a462..fae9c12 100644 --- a/expander.go +++ b/expander.go @@ -102,15 +102,21 @@ const rootBase = ".root" // baseForRoot loads in the cache the root document and produces a fake ".root" base path entry // for further $ref resolution -// -// Setting the cache is optional and this parameter may safely be left to nil. func baseForRoot(root interface{}, cache ResolutionCache) string { + // cache the root document to resolve $ref's + normalizedBase := normalizeBase(rootBase) + if root == nil { - return "" + // ensure that we never leave a nil root: always cache the root base pseudo-document + cachedRoot, found := cache.Get(normalizedBase) + if found && cachedRoot != nil { + // the cache is already preloaded with a root + return normalizedBase + } + + root = map[string]interface{}{} } - // cache the root document to resolve $ref's - normalizedBase := normalizeBase(rootBase) cache.Set(normalizedBase, root) return normalizedBase diff --git a/expander_test.go b/expander_test.go index 719ef44..9bdeb6c 100644 --- a/expander_test.go +++ b/expander_test.go @@ -1098,6 +1098,53 @@ func TestExpand_ExtraItems(t *testing.T) { }`, jazon) } +func TestExpand_Issue145(t *testing.T) { + cwd, err := os.Getwd() + require.NoError(t, err) + pseudoRoot := normalizeBase(filepath.Join(cwd, rootBase)) + + // assert the internal behavior of baseForRoot() + t.Run("with nil root, empty cache", func(t *testing.T) { + cache := defaultResolutionCache() + require.Equal(t, pseudoRoot, baseForRoot(nil, cache)) + + t.Run("empty root is cached", func(t *testing.T) { + value, ok := cache.Get(pseudoRoot) + require.True(t, ok) // found in cache + asMap, ok := value.(map[string]interface{}) + require.True(t, ok) + require.Empty(t, asMap) + }) + }) + + t.Run("with non-nil root, empty cache", func(t *testing.T) { + cache := defaultResolutionCache() + require.Equal(t, pseudoRoot, baseForRoot(map[string]interface{}{"key": "arbitrary"}, cache)) + + t.Run("non-empty root is cached", func(t *testing.T) { + value, ok := cache.Get(pseudoRoot) + require.True(t, ok) // found in cache + asMap, ok := value.(map[string]interface{}) + require.True(t, ok) + require.Contains(t, asMap, "key") + require.Equal(t, "arbitrary", asMap["key"]) + }) + + t.Run("with nil root, non-empty cache", func(t *testing.T) { + require.Equal(t, pseudoRoot, baseForRoot(nil, cache)) + + t.Run("non-empty root is kept", func(t *testing.T) { + value, ok := cache.Get(pseudoRoot) + require.True(t, ok) // found in cache + asMap, ok := value.(map[string]interface{}) + require.True(t, ok) + require.Contains(t, asMap, "key") + require.Equal(t, "arbitrary", asMap["key"]) + }) + }) + }) +} + // PetStore20 json doc for swagger 2.0 pet store const PetStore20 = `{ "swagger": "2.0", diff --git a/fixtures/bugs/145/Program Files (x86)/AppName/ref.json b/fixtures/bugs/145/Program Files (x86)/AppName/ref.json new file mode 100644 index 0000000..10a8cb7 --- /dev/null +++ b/fixtures/bugs/145/Program Files (x86)/AppName/ref.json @@ -0,0 +1,17 @@ +{ +"definitions": { + "todo-partial": { + "title": "Todo Partial", + "type": "object", + "properties": { + "name": { + "type": "string" + }, + "completed": { + "type": ["boolean", "null"] + } + }, + "required": ["name", "completed"] + } + } +} diff --git a/fixtures/bugs/145/Program Files (x86)/AppName/todos.common.json b/fixtures/bugs/145/Program Files (x86)/AppName/todos.common.json new file mode 100644 index 0000000..1c43908 --- /dev/null +++ b/fixtures/bugs/145/Program Files (x86)/AppName/todos.common.json @@ -0,0 +1,103 @@ +{ + "swagger": "2.0", + "info": { + "version": "1.0", + "title": "To-do Demo", + "description": + "### Notes:\n\nThis OAS2 (Swagger 2) specification defines common models and responses, that other specifications may reference.\n\nFor example, check out the user poperty in the main.oas2 todo-partial model - it references the user model in this specification!\n\nLikewise, the main.oas2 operations reference the shared error responses in this common specification.", + "contact": { + "name": "Stoplight", + "url": "https://stoplight.io" + }, + "license": { + "name": "MIT" + } + }, + "host": "example.com", + "securityDefinitions": {}, + "paths": {}, + "responses": { + "401": { + "description": "", + "schema": { + "$ref": "#/definitions/error-response" + }, + "examples": { + "application/json": { + "status": "401", + "error": "Not Authorized" + } + } + }, + "403": { + "description": "", + "schema": { + "$ref": "#/definitions/error-response" + }, + "examples": { + "application/json": { + "status": "403", + "error": "Forbbiden" + } + } + }, + "404": { + "description": "", + "schema": { + "$ref": "#/definitions/error-response" + }, + "examples": { + "application/json": { + "status": "404", + "error": "Not Found" + } + } + }, + "500": { + "description": "", + "schema": { + "$ref": "#/definitions/error-response" + }, + "examples": { + "application/json": { + "status": "500", + "error": "Server Error" + } + } + } + }, + "definitions": { + "user": { + "title": "User", + "type": "object", + "properties": { + "name": { + "type": "string", + "description": "The user's full name." + }, + "age": { + "type": "number", + "minimum": 0, + "maximum": 150 + }, + "error": { + "$ref": "#/definitions/error-response" + } + }, + "required": ["name", "age"] + }, + "error-response": { + "type": "object", + "title": "Error Response", + "properties": { + "status": { + "type": "string" + }, + "error": { + "type": "string" + } + }, + "required": ["status", "error"] + } + } +} diff --git a/fixtures/bugs/145/Program Files (x86)/AppName/todos.json b/fixtures/bugs/145/Program Files (x86)/AppName/todos.json new file mode 100644 index 0000000..9c5072e --- /dev/null +++ b/fixtures/bugs/145/Program Files (x86)/AppName/todos.json @@ -0,0 +1,336 @@ +{ + "swagger": "2.0", + "info": { + "version": "1.0", + "title": "To-do Demo", + "description": "This OAS2 (Swagger 2) file represents a real API that lives at http://todos.stoplight.io.\n\nFor authentication information, click the apikey security scheme in the editor sidebar.", + "contact": { + "name": "Stoplight", + "url": "https://stoplight.io" + }, + "license": { + "name": "MIT" + } + }, + "host": "todos.stoplight.io", + "schemes": ["http"], + "consumes": ["application/json"], + "produces": ["application/json"], + "securityDefinitions": { + "Basic": { + "type": "basic" + }, + "API Key": { + "type": "apiKey", + "name": "apikey", + "in": "query" + } + }, + "paths": { + "/todos/{todoId}": { + "parameters": [{ + "name": "todoId", + "in": "path", + "required": true, + "type": "string" + }], + "get": { + "operationId": "GET_todo", + "summary": "Get Todo", + "tags": ["Todos"], + "responses": { + "200": { + "description": "", + "schema": { + "$ref": "#/definitions/todo-full" + }, + "examples": { + "application/json": { + "id": 1, + "name": "get food", + "completed": false, + "completed_at": "1955-04-23T13:22:52.685Z", + "created_at": "1994-11-05T03:26:51.471Z", + "updated_at": "1989-07-29T11:30:06.701Z" + }, + "/todos/foobar": "{\n\t\"foo\": \"bar\"\n}\n", + "/todos/chores": { + "id": 9000, + "name": "Do Chores", + "completed": false, + "created_at": "2014-08-28T14:14:28.494Z", + "updated_at": "2014-08-28T14:14:28.494Z" + }, + "new": { + "name": "esse qui proident labore", + "completed": null, + "id": 920778, + "completed_at": "2014-01-07T07:49:55.123Z", + "created_at": "1948-04-21T12:04:21.282Z", + "updated_at": "1951-12-19T11:10:34.039Z", + "user": { + "name": "irure deserunt fugiat", + "age": 121.45395681110494 + }, + "float": -47990796.228164576 + } + } + }, + "404": { + "$ref": "./todos.common.json#/responses/404" + }, + "500": { + "$ref": "./todos.common.json#/responses/500" + } + }, + "parameters": [{ + "in": "query", + "name": "", + "type": "string" + }] + }, + "put": { + "operationId": "PUT_todos", + "summary": "Update Todo", + "tags": ["Todos"], + "parameters": [{ + "name": "body", + "in": "body", + "schema": { + "$ref": "#/definitions/todo-partial", + "example": { + "name": "my todo's new name", + "completed": false + } + } + }], + "responses": { + "200": { + "description": "", + "schema": { + "$ref": "#/definitions/todo-full" + }, + "examples": { + "application/json": { + "id": 9000, + "name": "It's Over 9000!!!", + "completed": true, + "completed_at": null, + "created_at": "2014-08-28T14:14:28.494Z", + "updated_at": "2015-08-28T14:14:28.494Z" + } + } + }, + "401": { + "$ref": "./todos.common.json#/responses/401" + }, + "404": { + "$ref": "./todos.common.json#/responses/404" + }, + "500": { + "$ref": "./todos.common.json#/responses/500" + } + }, + "security": [{ + "Basic": [] + }, + { + "API Key": [] + } + ] + }, + "delete": { + "operationId": "DELETE_todo", + "summary": "Delete Todo", + "tags": ["Todos"], + "responses": { + "204": { + "description": "" + }, + "401": { + "$ref": "./todos.common.json#/responses/401" + }, + "404": { + "$ref": "./todos.common.json#/responses/404" + }, + "500": { + "$ref": "./todos.common.json#/responses/500" + } + }, + "security": [{ + "Basic": [] + }, + { + "API Key": [] + } + ] + } + }, + "/todos": { + "post": { + "operationId": "POST_todos", + "summary": "Create Todo", + "tags": ["Todos"], + "parameters": [{ + "name": "body", + "in": "body", + "schema": { + "$ref": "#/definitions/todo-partial", + "example": { + "name": "my todo's name", + "completed": false + } + } + }], + "responses": { + "201": { + "description": "", + "schema": { + "$ref": "#/definitions/todo-full" + }, + "examples": { + "application/json": { + "id": 9000, + "name": "It's Over 9000!!!", + "completed": null, + "completed_at": null, + "created_at": "2014-08-28T14:14:28.494Z", + "updated_at": "2014-08-28T14:14:28.494Z" + }, + "/todos/chores": { + "id": 9000, + "name": "Do Chores", + "completed": false, + "created_at": "2014-08-28T14:14:28.494Z", + "updated_at": "2014-08-28T14:14:28.494Z" + } + } + }, + "401": { + "$ref": "./todos.common.json#/responses/401" + }, + "500": { + "$ref": "./todos.common.json#/responses/500" + } + }, + "security": [{ + "API Key": [] + }, + { + "Basic": [] + } + ], + "description": "This creates a Todo object.\n\nTesting `inline code`." + }, + "get": { + "operationId": "GET_todos", + "summary": "List Todos", + "tags": ["Todos"], + "parameters": [{ + "$ref": "#/parameters/limit" + }, + { + "$ref": "#/parameters/skip" + } + ], + "responses": { + "200": { + "description": "", + "schema": { + "type": "array", + "items": { + "$ref": "#/definitions/todo-full" + } + }, + "examples": { + "application/json": [{ + "id": 1, + "name": "design the thingz", + "completed": true + }, + { + "id": 2, + "name": "mock the thingz", + "completed": true + }, + { + "id": 3, + "name": "code the thingz", + "completed": false + } + ], + "empty": [] + }, + "headers": { + "foo": { + "type": "string", + "default": "bar" + } + } + }, + "500": { + "$ref": "./todos.common.json#/responses/500" + } + }, + "description": "​" + } + } + }, + "parameters": { + "limit": { + "name": "limit", + "in": "query", + "description": "This is how it works.", + "required": false, + "type": "integer", + "maximum": 100 + }, + "skip": { + "name": "skip", + "in": "query", + "required": false, + "type": "string" + } + }, + "definitions": { + "todo-partial": { + "$ref": "ref.json#/definitions/todo-partial" + }, + "todo-full": { + "title": "Todo Full", + "allOf": [{ + "$ref": "#/definitions/todo-partial" + }, + { + "type": "object", + "properties": { + "id": { + "type": "integer", + "minimum": 0, + "maximum": 1000000 + }, + "completed_at": { + "type": ["string", "null"], + "format": "date-time" + }, + "created_at": { + "type": "string", + "format": "date-time" + }, + "updated_at": { + "type": "string", + "format": "date-time" + }, + "user": { + "$ref": "./todos.common.json#/definitions/user" + } + }, + "required": ["id", "user"] + } + ] + } + }, + "tags": [{ + "name": "Todos" + }] +} diff --git a/normalizer_test.go b/normalizer_test.go index 66f30bf..8d72c28 100644 --- a/normalizer_test.go +++ b/normalizer_test.go @@ -219,6 +219,30 @@ func TestNormalizer_NormalizeURI(t *testing.T) { base: `https://example.com//base/Spec.json`, expOutput: `https://example.com/base/Resources.yaml#/definitions/Pets`, }, + { + // escaped characters in base (1) + refPath: `Resources.yaml#/definitions/Pets`, + base: `https://example.com/base (x86)/Spec.json`, + expOutput: `https://example.com/base%20%28x86%29/Resources.yaml#/definitions/Pets`, + }, + { + // escaped characters in base (2) + refPath: `Resources.yaml#/definitions/Pets`, + base: `https://example.com/base [x86]/Spec.json`, + expOutput: `https://example.com/base%20%5Bx86%5D/Resources.yaml#/definitions/Pets`, + }, + { + // escaped characters in joined fragment + refPath: `Resources.yaml#/definitions (x86)/Pets`, + base: `https://example.com/base/Spec.json`, + expOutput: `https://example.com/base/Resources.yaml#/definitions%20(x86)/Pets`, + }, + { + // escaped characters in joined path + refPath: `Resources [x86].yaml#/definitions/Pets`, + base: `https://example.com/base/Spec.json`, + expOutput: `https://example.com/base/Resources%20%5Bx86%5D.yaml#/definitions/Pets`, + }, } }() @@ -423,6 +447,11 @@ func TestNormalizer_NormalizeBase(t *testing.T) { Expected: "file:///e:/base/sub/file.json", Windows: true, }, + { + // escaped characters in base (1) + Base: `file:///c:/base (x86)/spec.json`, + Expected: `file:///c:/base%20%28x86%29/spec.json`, + }, } { testCase := toPin if testCase.Windows && runtime.GOOS != windowsOS { diff --git a/schema_loader.go b/schema_loader.go index b81175a..0059b99 100644 --- a/schema_loader.go +++ b/schema_loader.go @@ -168,14 +168,7 @@ func (r *schemaLoader) load(refURL *url.URL) (interface{}, url.URL, bool, error) normalized := normalizeBase(pth) debugLog("loading doc from: %s", normalized) - unescaped, err := url.PathUnescape(normalized) - if err != nil { - return nil, url.URL{}, false, err - } - - u := url.URL{Path: unescaped} - - data, fromCache := r.cache.Get(u.RequestURI()) + data, fromCache := r.cache.Get(normalized) if fromCache { return data, toFetch, fromCache, nil } diff --git a/schema_loader_test.go b/schema_loader_test.go new file mode 100644 index 0000000..67a1e79 --- /dev/null +++ b/schema_loader_test.go @@ -0,0 +1,33 @@ +package spec + +import ( + "encoding/json" + "path/filepath" + "testing" + + "github.com/stretchr/testify/require" +) + +func TestLoader_Issue145(t *testing.T) { + t.Run("with ExpandSpec", func(t *testing.T) { + basePath := filepath.Join("fixtures", "bugs", "145", "Program Files (x86)", "AppName", "todos.json") + todosDoc, err := jsonDoc(basePath) + require.NoError(t, err) + + spec := new(Swagger) + require.NoError(t, json.Unmarshal(todosDoc, spec)) + + require.NoError(t, ExpandSpec(spec, &ExpandOptions{RelativeBase: basePath})) + }) + + t.Run("with ExpandSchema", func(t *testing.T) { + basePath := filepath.Join("fixtures", "bugs", "145", "Program Files (x86)", "AppName", "ref.json") + schemaDoc, err := jsonDoc(basePath) + require.NoError(t, err) + + sch := new(Schema) + require.NoError(t, json.Unmarshal(schemaDoc, sch)) + + require.NoError(t, ExpandSchema(sch, nil, nil)) + }) +}