Repository: getkin/kin-openapi
Pull request #611 (2022-09-22 10:53:06)

Correctly resolve path of yaml resource if double referenced.
Where previously following path was resolved `testdata/components/models/error.yaml`, missing `recursiveRef` folder.

d-sauer: Could there be some concurrency problem with tests? 
When I run this test individually, there are no errors, same as well when I run all of them, there are no errors.

If someone could give me a hint to try to wrap up this fix?
https://github.com/getkin/kin-openapi/actions/runs/3104857599/jobs/5029805927#step:16:17
fenollp: Hi! Could you describe a bit more what this is fixing? A real-world example would be nice, thanks.

Also, please add back the newlines at the end of YAML files.

I can reproduce the test failure locally. Try testing with `-count=10`.

You may want to rebase on top of latest master: #602 might help.
d-sauer: I faced a problem when parsing the OpenAPI spec we are using across the company, which we based on our template, which works with [openapi-generator](https://github.com/OpenAPITools/openapi-generator) to generate JavaSDK for example.

Example [OpenAPI spec](https://github.com/d-sauer/exploring-golang/tree/development/hello-oas/openapi-spec/ServiceHelloV1)

Where referencing [paths.path.get.response](https://github.com/d-sauer/exploring-golang/blob/development/hello-oas/openapi-spec/ServiceHelloV1/specification/openapi.yaml#L66) to [components.responses.response(https://github.com/d-sauer/exploring-golang/blob/development/hello-oas/openapi-spec/ServiceHelloV1/specification/openapi.yaml#L89), which [schema](https://github.com/d-sauer/exploring-golang/blob/development/hello-oas/openapi-spec/ServiceHelloV1/specification/openapi.yaml#L94) is ref to [components.schema](https://github.com/d-sauer/exploring-golang/blob/development/hello-oas/openapi-spec/ServiceHelloV1/specification/openapi.yaml#L79), which have ref to [yaml object](https://github.com/d-sauer/exploring-golang/blob/development/hello-oas/openapi-spec/ServiceHelloV1/specification/common/models/error.yaml). 

Merged latest changes from master, but still resolved path is wrong.
`testdata/recursiveRef/paths/components/models/error.yaml`
while it should be `testdata/recursiveRef/components/models/error.yaml`


Thank you for the hint, now I can reproduce it as well when using `-count`.
Still a bit nondeterministic, but let me try to troubleshoot it.

d-sauer: I went back to my previous analysis.
Based on a short test, I assume the expectation is correct.
```
func TestResolvePath(t *testing.T) {
	var b = &url.URL{Path: "testdata/recursiveRef"}
	var c = &url.URL{Path: "./components/models/error.yaml"}
	u, err := resolvePath(b, c)

	require.NoError(t, err)
	require.Equal(t, &url.URL{Path: "testdata/recursiveRef/components/models/error.yaml"}, u)
}
```

Getting
```
        	Error:      	Not equal: 
        	            	expected: &url.URL{Scheme:"", Opaque:"", User:(*url.Userinfo)(nil), Host:"", Path:"testdata/recursiveRef/components/models/error.yaml", RawPath:"", OmitHost:false, ForceQuery:false, RawQuery:"", Fragment:"", RawFragment:""}
        	            	actual  : &url.URL{Scheme:"", Opaque:"", User:(*url.Userinfo)(nil), Host:"", Path:"testdata/components/models/error.yaml", RawPath:"", OmitHost:false, ForceQuery:false, RawQuery:"", Fragment:"", RawFragment:""}

```
fenollp: I agree with your expectation with regards to `TestResolvePath`.

Can you try and root-cause that bit of non-determinism? I am not seeing any filesystem manipulation (from a quick glance) so this is very puzzling to me. My hunch is this is the crux of the issue. Would love for you to continue sharing your progress.

Thanks!
d-sauer: I resolved undeterministic part by implementing a slightly different solution than the first time.

As the root cause of my initial problem was in [loader.join](https://github.com/getkin/kin-openapi/blob/62f85cfe75b720f25179e5272a7c551146bf7dd9/openapi3/loader.go#L252) function, where [loader.documentPathForRecursiveRef](https://github.com/getkin/kin-openapi/blob/62f85cfe75b720f25179e5272a7c551146bf7dd9/openapi3/loader.go#L423) would return folder, and in loader.join `path.Dir` would return the parent folder, whereas in other (most) situations loader.join would receive the relative path to the actual specification file (e.g. openapi.yml).


The current fix is trying to overcome this by detecting if the path contains `.` after last `/`.
There could be a problem if the folder name contains `.`.
Maybe there is a better way, but I could go deeper to understand why and when `loader.rootDir` suppose to be used.

A similar could be done with `url.ResolveReference`, but that function will always return leading `/`, which won't work for relative paths as a plain string.
fenollp: > There could be a problem if the folder name contains `.`.

Indeed! Please add related tests.
d-sauer: > > There could be a problem if the folder name contains `.`.
> 
> Indeed! Please add related tests.

Let me go bit deeper to see why folder reference (`loader.rootDir `) is passed in first place from `[loader.documentPathForRecursiveRef](https://github.com/getkin/kin-openapi/blob/62f85cfe75b720f25179e5272a7c551146bf7dd9/openapi3/loader.go#L423), and to see how [loader.join](https://github.com/getkin/kin-openapi/blob/62f85cfe75b720f25179e5272a7c551146bf7dd9/openapi3/loader.go#L252) is used from other functions.

As there is no easy way to determine if path/url as string is pointing to folder or specification.

d-sauer: I went a bit deeper and needed to do a slightly different implementation.
Which in this case is removing enhancements of `resolvePath`, and by that, removing the need to test it.

I hope implementation is now a bit more straight forward.
Where previously main root cause was to use `loader.rootDir` while `join` function is using `path.Dir`, causing to return the parent folder of the spec folder.
So I added `loader.rootLocation` to be used if `resolvedRef` argument in `documentPathForRecursiveRef` is empty.

Patch:
diff --git a/openapi3/loader.go b/openapi3/loader.go
index 28836f3..9ed8e30 100644
--- a/openapi3/loader.go
+++ b/openapi3/loader.go
@@ -36,7 +36,8 @@ type Loader struct {
 
 	Context context.Context
 
-	rootDir string
+	rootDir      string
+	rootLocation string
 
 	visitedPathItemRefs map[string]struct{}
 
@@ -148,6 +149,7 @@ func (loader *Loader) LoadFromDataWithPath(data []byte, location *url.URL) (*T,
 func (loader *Loader) loadFromDataWithPathInternal(data []byte, location *url.URL) (*T, error) {
 	if loader.visitedDocuments == nil {
 		loader.visitedDocuments = make(map[string]*T)
+		loader.rootLocation = location.Path
 	}
 	uri := location.String()
 	if doc, ok := loader.visitedDocuments[uri]; ok {
@@ -420,6 +422,11 @@ func (loader *Loader) documentPathForRecursiveRef(current *url.URL, resolvedRef
 	if loader.rootDir == "" {
 		return current
 	}
+
+	if resolvedRef == "" {
+		return &url.URL{Path: loader.rootLocation}
+	}
+
 	return &url.URL{Path: path.Join(loader.rootDir, resolvedRef)}
 }
 


Test patch:
diff --git a/openapi3/internalize_refs_test.go b/openapi3/internalize_refs_test.go
index b1ca846..fe6b29d 100644
--- a/openapi3/internalize_refs_test.go
+++ b/openapi3/internalize_refs_test.go
@@ -2,7 +2,7 @@ package openapi3
 
 import (
 	"context"
-	"io/ioutil"
+	"os"
 	"regexp"
 	"testing"
 
@@ -41,25 +41,25 @@ func TestInternalizeRefs(t *testing.T) {
 			err = doc.Validate(ctx)
 			require.NoError(t, err, "validating internalized spec")
 
-			data, err := doc.MarshalJSON()
+			actual, err := doc.MarshalJSON()
 			require.NoError(t, err, "marshalling internalized spec")
 
 			// run a static check over the file, making sure each occurence of a
 			// reference is followed by a #
-			numRefs := len(regexpRef.FindAll(data, -1))
-			numInternalRefs := len(regexpRefInternal.FindAll(data, -1))
+			numRefs := len(regexpRef.FindAll(actual, -1))
+			numInternalRefs := len(regexpRefInternal.FindAll(actual, -1))
 			require.Equal(t, numRefs, numInternalRefs, "checking all references are internal")
 
-			// load from data, but with the path set to the current directory
-			doc2, err := sl.LoadFromData(data)
+			// load from actual, but with the path set to the current directory
+			doc2, err := sl.LoadFromData(actual)
 			require.NoError(t, err, "reloading spec")
 			err = doc2.Validate(ctx)
 			require.NoError(t, err, "validating reloaded spec")
 
 			// compare with expected
-			data0, err := ioutil.ReadFile(test.filename + ".internalized.yml")
+			expected, err := os.ReadFile(test.filename + ".internalized.yml")
 			require.NoError(t, err)
-			require.JSONEq(t, string(data), string(data0))
+			require.JSONEq(t, string(expected), string(actual))
 		})
 	}
 }
diff --git a/openapi3/loader_recursive_ref_test.go b/openapi3/loader_recursive_ref_test.go
index 5b7c150..924cb6b 100644
--- a/openapi3/loader_recursive_ref_test.go
+++ b/openapi3/loader_recursive_ref_test.go
@@ -14,6 +14,8 @@ func TestLoaderSupportsRecursiveReference(t *testing.T) {
 	err = doc.Validate(loader.Context)
 	require.NoError(t, err)
 	require.Equal(t, "bar", doc.Paths["/foo"].Get.Responses.Get(200).Value.Content.Get("application/json").Schema.Value.Properties["foo2"].Value.Properties["foo"].Value.Properties["bar"].Value.Example)
+	require.Equal(t, "ErrorDetails", doc.Paths["/foo"].Get.Responses.Get(400).Value.Content.Get("application/json").Schema.Value.Title)
+	require.Equal(t, "ErrorDetails", doc.Paths["/double-ref-foo"].Get.Responses.Get(400).Value.Content.Get("application/json").Schema.Value.Title)
 }
 
 func TestIssue447(t *testing.T) {
diff --git a/openapi3/testdata/recursiveRef/components/models/error.yaml b/openapi3/testdata/recursiveRef/components/models/error.yaml
new file mode 100644
index 0000000..b4d4047
--- /dev/null
+++ b/openapi3/testdata/recursiveRef/components/models/error.yaml
@@ -0,0 +1,2 @@
+type: object
+title: ErrorDetails
diff --git a/openapi3/testdata/recursiveRef/openapi.yml b/openapi3/testdata/recursiveRef/openapi.yml
index 675722a..9f884c7 100644
--- a/openapi3/testdata/recursiveRef/openapi.yml
+++ b/openapi3/testdata/recursiveRef/openapi.yml
@@ -5,6 +5,13 @@ info:
 paths:
   /foo:
     $ref: ./paths/foo.yml
+  /double-ref-foo:
+    get:
+      summary: Double ref response
+      description: Reference response with double reference.
+      responses:
+        "400":
+          $ref: "#/components/responses/400"
 components:
   schemas:
     Foo:
@@ -15,3 +22,12 @@ components:
       $ref: ./components/Bar.yml
     Cat:
       $ref: ./components/Cat.yml
+    Error:
+      $ref: ./components/models/error.yaml
+  responses:
+    "400":
+      description: 400 Bad Request
+      content:
+        application/json:
+          schema:
+            $ref: "#/components/schemas/Error"
diff --git a/openapi3/testdata/recursiveRef/openapi.yml.internalized.yml b/openapi3/testdata/recursiveRef/openapi.yml.internalized.yml
index 0730590..0d50852 100644
--- a/openapi3/testdata/recursiveRef/openapi.yml.internalized.yml
+++ b/openapi3/testdata/recursiveRef/openapi.yml.internalized.yml
@@ -9,11 +9,27 @@
         }
       }
     },
+    "responses": {
+      "400": {
+        "content": {
+          "application/json": {
+            "schema": {
+              "$ref": "#/components/schemas/Error"
+            }
+          }
+        },
+        "description": "400 Bad Request"
+      }
+    },
     "schemas": {
       "Bar": {
         "example": "bar",
         "type": "string"
       },
+      "Error":{
+        "title":"ErrorDetails",
+        "type":"object"
+      },
       "Foo": {
         "properties": {
           "bar": {
@@ -30,6 +46,10 @@
         },
         "type": "object"
       },
+      "error":{
+        "title":"ErrorDetails",
+        "type":"object"
+      },
       "Cat": {
         "properties": {
           "cat": {
@@ -46,6 +66,17 @@
   },
   "openapi": "3.0.3",
   "paths": {
+    "/double-ref-foo": {
+      "get": {
+        "description": "Reference response with double reference.",
+        "responses": {
+          "400": {
+            "$ref": "#/components/responses/400"
+          }
+        },
+        "summary": "Double ref response"
+      }
+    },
     "/foo": {
       "get": {
         "responses": {
@@ -63,6 +94,9 @@
               }
             },
             "description": "OK"
+          },
+          "400": {
+            "$ref": "#/components/responses/400"
           }
         }
       },
diff --git a/openapi3/testdata/recursiveRef/paths/foo.yml b/openapi3/testdata/recursiveRef/paths/foo.yml
index 43e03b7..4c845b5 100644
--- a/openapi3/testdata/recursiveRef/paths/foo.yml
+++ b/openapi3/testdata/recursiveRef/paths/foo.yml
@@ -11,3 +11,5 @@ get:
             properties:
               foo2:
                 $ref: ../openapi.yml#/components/schemas/Foo2
+    "400":
+      $ref: "../openapi.yml#/components/responses/400"