Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

paths: fix Operation request urljoin #85

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

olichtne
Copy link

@olichtne olichtne commented Sep 13, 2022

When adding operation path to the base_url, the use of the + operator can lead to doubling of / characters if the base_url ends with a /.

Using urljoin should fix this. This is also a cleaner way to work with url paths.

This causes some minor issues for us when interacting with our rest api server where the doubled / character is not matched correctly and results in 404 errors.

Even though the doubled / characters are defined in RFCs as something that should be simply skipped and don't effectively change the path, some http servers can be configured either way.

An example of a reproducer is to use a schema that looks as follows:

openapi: 3.0.2
info:
  title: LNST Dashboard
  version: ''
  description: API to the interact with the dashboard db directly - upload or get
    and process data as you wish
servers:
  - url: https://api.example.com/
paths:
  /artifacts/:
    get:
      operationId: listArtifacts
...

vs.

openapi: 3.0.2
info:
  title: LNST Dashboard
  version: ''
  description: API to the interact with the dashboard db directly - upload or get
    and process data as you wish
servers:
  - url: https://api.example.com
paths:
  /artifacts/:
    get:
      operationId: listArtifacts
...

Note that the difference is the final / in the server url.

For the first schema with the current code of openapi3 the request api.call_listArtifacts() would call:

https://api.example.com//artifacts

Whereas with the fix the same request would call:

https://api.example.com/artifacts

When adding operation `path` to the base_url, the use of the `+`
operator can lead to doubling of `/` characters if the base_url ends
with a `/`.

Using urljoin should fix this. This is also a cleaner way to work with
url paths.

Signed-off-by: Ondrej Lichtner <olichtne@redhat.com>
@Dorthu Dorthu self-assigned this Oct 14, 2022
Copy link
Owner

@Dorthu Dorthu left a comment

Choose a reason for hiding this comment

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

This behaves unexpectedly when the servers listed include a path fragment. For example:

openapi: 3.0.2
info:
  title: LNST Dashboard
  version: ''
  description: API to the interact with the dashboard db directly - upload or get
    and process data as you wish
servers:
  - url: https://api.example.com/v2
paths:
  /artifacts/:
    get:
      operationId: listArtifacts
...

A request to the listArtifacts operation here would result in a request to https://api.example.com/artifacts/ instead of https://api.example.com/v2/artifacts/; this appears to be the behavior of urljoin:

>>> from urllib.parse import urljoin
>>> urljoin("https://api.example.com/v2", "/artifacts/")
'https://api.example.com/artifacts/'
>>> urljoin("https://api.example.com/v2/", "/artifacts/")
'https://api.example.com/artifacts/'

Maybe stripping the extra / if it's present in both the server name and the path would be a simpler solution?

@olichtne
Copy link
Author

hmmm, good point

stripping the extra /

i feel like there should be a better way to do this, doing string editing operations on path fragments for a simple join operation feels weird to me.

i'll think about this next week when i have some time for this.

thanks for looking at the PR.

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