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

Allow for spec_url file in root without prefix #299

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

gdub
Copy link
Contributor

@gdub gdub commented Apr 12, 2023

Updated spec_url to allow for file in root, without a path prefix, when Configuration path set to None.

Comment on lines 120 to 122
if self.path is None:
return f"/{self.filename}"
return f"/{self.path}/{self.filename}"
Copy link
Member

Choose a reason for hiding this comment

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

Can we use urllib.parse.urljoin here? I think empty str should be more proper for self.path. Should be able to achieve the same purpose.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kemingy, sure, I can update so that empty-string self.path represents no prefix rather than None.

Given that urljoin is geared towards building URL that combines a base plus path in same way a browser builds target link with current page URL and link path, there are some "surprising" scenarios, e.g.:

>>> path='dir1'
>>> filename='file.json'
>>> urllib.parse.urljoin(path, filename)
'file.json'  # dir1 was stripped out because no trailing slash.
>>> path='dir1/'
>>> filename='file.json'
>>> urllib.parse.urljoin(path, filename)
'dir1/file.json'  # dir1 kept because it had trailing slash meas we are "in" that directory.
>>> path='dir1/dir2/dir3'
>>> filename='/file.json'
>>> urllib.parse.urljoin(path, filename)
'/file.json'  # Entire path prefix removed because filename used a leading slash (i.e. an absolute path that reset the root).

What if we use posixpath.join? Gives us what we need for joining path components, without the handling of scheme/domain stuff from urljoin that we don't use. That said, posixpath.join does has similar behavior as urljoin regarding leading/trailing slashes, so still would need to do some normalizing to have that work in a way that is similar to current implementation.

I've pushed an update here that uses posixpath.join and normalizes slashes in such a way that putting leading/trailing slashes on path value and/or putting leading slash on filename value does not produce surprising results. Added more tests for this, as well.

What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

I see. Our requirement is a bit different from the URL join. But posixpath should be used for POSIX file path. I guess this can be done in a simple way like:

def join_path(paths):
    return "/".join(x.strip("/") for x in paths if x)

f"/{join_path((self.path, self.filename))}"

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Given that we are building paths using / separator character, we are effectively using POSIX style path, right? Believe it's fairly common use, and os.path docs do note use of the posixpath and ntpath modules directly when want to deal with a particular path style.

Your snippet is simple and can work. Plugging it in, I do have a couple of the newly-added tests fail, e.g.:

>>> f"/{join_path(('/', 'openapi.json'))}"
'//openapi.json'
>>> f"/{join_path(('/', '/openapi.json'))}"
'//openapi.json'

Though, easily fixed with small tweak, such as:

def join_path(paths):
    normalized_paths = (x.strip("/") for x in paths if x)
    return "/".join(x for x in normalized_paths if x)

I tested this tweaked version and it passes all the newly added tests.

Would you rather we use posixpath or the custom implementation? Happy to use whichever you prefer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alternatively, another possibility may be to combine the path and filename options? Perhaps I'm missing something, but didn't see them used elsewhere. In which case, could perhaps use a single option that is the full path+filename in one string. Would avoid the need to join, but also would be backwards incompatible.

Copy link
Member

Choose a reason for hiding this comment

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

You remind me that config.path is widely used in the codebase. Changing config.path to empty or None may also cause some unexpected behaviors.

BTW, I would prefer to implement a path join instead of using posixpath.join even though it should work in this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pushed an update that attempts to address config.path usages, along with associated test code updates. However, having a failure with flask blueprint where api.spec isn't populated with the expected routes.

gdub added 2 commits May 5, 2023 16:00
…en Configuration path set to None.

Also made spec_url more resilient to path and filename values with leading/trailing slashes.
…n implementation.

Added join_path utility function, with tests.
assert resp.status_code == 200

resp = client.get(prefix + "/apidoc/swagger/")
resp = client.get(f"{prefix}{doc_prefix}/swagger/")
assert resp.status_code == 200

assert get_paths(api.spec) == [
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This fails for a couple of the new test cases added when using empty prefix. api.spec seems to not be getting populated. Looked at it a bit, but didn't get far enough to determine why. Any ideas?

Copy link
Member

Choose a reason for hiding this comment

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

I guess it's due to

if any(
str(rule).startswith(path)
for path in (f"/{self.config.path}", "/static")
):
continue

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.

None yet

2 participants