Skip to content

Fix openapi3.referencedDocumentPath#248

Merged
fenollp merged 3 commits into
getkin:masterfrom
kshlm:fix-referencedDocumentPath
Sep 4, 2020
Merged

Fix openapi3.referencedDocumentPath#248
fenollp merged 3 commits into
getkin:masterfrom
kshlm:fix-referencedDocumentPath

Conversation

@kshlm

@kshlm kshlm commented Sep 2, 2020

Copy link
Copy Markdown
Contributor

openapi3.referencedDocument tried to path.Join two URLs, and then
generate a new url.URL by parsing the obtained path. The URL
obtained at the end would be invalid, when the base path is HTTP URL.

Given base path as 'http://example.com/schemas' and ref as '/schema1',
the obtained end URL would be 'http:/example.com/schemas'. This happens
because path.Join cleans the path it generates and replaces repeated
'//' with a single '/'.

This is fixed by doing a path.Join on the base URL.Path and the ref,
and setting this result as the URL.Path of the new URL.

@fenollp

fenollp commented Sep 2, 2020

Copy link
Copy Markdown
Collaborator

Hi there, thanks for the patch!
Could you add a few tests on this function? Especially one that would not pass before your fix.
Thanks

`openapi3.referencedDocument` tried to `path.Join` two URLs, and then
generate a new `url.URL` by parsing the obtained path. The URL
obtained at the end would be invalid when the base path is HTTP URL.

Given base path as 'http://example.com/schemas' and ref as '/schema1',
the obtained end URL would be 'http:/example.com/schemas'. This happens
because `path.Join` cleans the path it generates and replaces repeated
'//' with a single '/'.

This is fixed by doing a `path.Join` on the base URL.Path and the ref,
and setting this result as the URL.Path of the new URL.
@kshlm kshlm force-pushed the fix-referencedDocumentPath branch from 0317a9f to 33ca746 Compare September 3, 2020 13:20
@kshlm

kshlm commented Sep 3, 2020

Copy link
Copy Markdown
Contributor Author

I've added the tests.

I'm not a 100% sure on the about the expected result of referencedDocumentPath. I would have expected it to be path.Dir(basePath)+refPath. But other tests seem to expect it to be path.Dir(basePath)+path.Dir(refPath)+"/". Which is what I've implemented the changes and the tests for.

@fenollp fenollp left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the tests :) I have a few nitpicks but after that it'll be merged.

Comment thread openapi3/swagger_loader_referenced_document_path_test.go Outdated
Comment thread openapi3/swagger_loader_referenced_document_path_test.go
Comment thread openapi3/swagger_loader_referenced_document_path_test.go Outdated
Comment thread openapi3/swagger_loader_referenced_document_path_test.go Outdated
@kshlm

kshlm commented Sep 4, 2020

Copy link
Copy Markdown
Contributor Author

Not sure why tests are failing. They're unrelated to my change in the openapi3 package.

@fenollp fenollp merged commit ceae068 into getkin:master Sep 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants