Skip to content

Core path does not support swagger basePath #78

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

Closed
deckar01 opened this issue Jun 6, 2016 · 8 comments · Fixed by #92
Closed

Core path does not support swagger basePath #78

deckar01 opened this issue Jun 6, 2016 · 8 comments · Fixed by #92
Milestone

Comments

@deckar01
Copy link
Member

deckar01 commented Jun 6, 2016

Related to #69.

Flask allows configuring the APPLICATION_ROOT, which provides the base path for the routes. #70 fixed this issue so that the flask extension reports the fully qualified path.

When the specs are dumped as swagger specs using to_dict(), the full path is shown on every route, even though the swagger specification supports the basePath field on the root object.

I think it is important to keep the meaningful portion of the route URL separate from the base path to maintain readable documentation. #70 was the right fix for the flask extension since the apispec core assumes paths are absolute, but the fact that both flask and swagger support a configurable base path indicates that the apispec core should probably internalize the concept of base paths and expose them to extensions.

Examples of base path in similar libraries:

Edit: Updated to refect that generating swagger specs is a core functionality of apispec, not an extension.

@deckar01 deckar01 changed the title Marshmallow swagger extension does not support basePath Core path does not support swagger basePath Jun 6, 2016
@deckar01
Copy link
Member Author

deckar01 commented Jun 6, 2016

After looking into the implementation, I realized that the APISpec constructor allows supplying additional options that are applied directly to the specs in to_dict().

A work around for #69 would have been to pass basePath to the APISpec constructor, but after #70 there is no way to get back to relative paths on the individual routes.

If anyone was using flask configured with APPLICATION_ROOT and explicitly passing it as basePath to the APISpec constructor, #70 will break their code, because the flask extension unconditionally expands route paths.

It might be a good idea to roll back #70 and yank the releases.

A better design would be to have the flask extension apply the basePath option to the spec instance in setup(). This would allow the flask extension to take advantage of the feature and prevent breaking existing usage of the the basePath option.

In summary, basePath was already implicitly supported by the apispec core, but #70 broke it for the flask extension.

@deckar01
Copy link
Member Author

deckar01 commented Jun 7, 2016

Upon further investigation, APISpec.add_path() is supposed to strip off the basePath from the path if it matches, but since this logic is applied before the extensions, the path instance returned by the flask extension never has its basePath prefix removed.

@lafrech
Copy link
Member

lafrech commented Oct 22, 2018

I'm discovering this and I find it a bit strange that we add the basePath prefix in the flask ext then strip it in the core with the normalize_path function.

Shouldn't we revert #70 already to not add it in the first place?

I think that's what @deckar01 means in

It might be a good idea to roll back #70 and yank the releases.

I guess now would be a good time. Note that this is an apispec-webframeworks issue, rather than an apispec issue.

Back to apispec, I'm not sure about normalize_path. Maybe we ought to leave it in case it is useful for other helpers. It probably doesn't hurt to leave it (unless some app has a relative path that is the same as the basePath, which is unlikely to happen but could be a bit painful to debug)

@lafrech
Copy link
Member

lafrech commented Oct 22, 2018

Also if basePath is / (default for OpenAPI and for flask's APPLICATION_ROOT) then normalize_path strips leading / from the paths.

What about replacing it with this (untested) ?

        def normalize_path(path):
            if path and 'basePath' in self.options:
                path.lstrip(self.options['basePath'])
                if not path.starstwith('/'):
                    path = '/' + path
            return path

@lafrech
Copy link
Member

lafrech commented Oct 22, 2018

Let's settle this before v1.0.

@lafrech lafrech reopened this Oct 22, 2018
@lafrech lafrech added this to the 1.0 milestone Oct 22, 2018
@lafrech
Copy link
Member

lafrech commented Oct 22, 2018

Pinging @asteinlein, who worked on this.

@lafrech
Copy link
Member

lafrech commented Oct 24, 2018

Removing APPLICATION_ROOT stuff from Flask plugin and removing normalize_path only breaks the tests that were added when the features were added.

I think we should do the Flask plugin change (undo #70) in apispec-webframeworks. And document the need for the user to specify the base path (how to do it differs between OpenAPI v2 and v3).

Regarding normalize_path, it only works for OpenAPI v2 as basePath does not exist in OpenAPI v3. I think it is useless for Flask once #70 is reverted. I can't comment about the other web frameworks. I'm surprised that it was added (by @jmcarp in 5563054) before #70. There was probably a good reason. But isn't this a client issue that the whole path is passed in the first place?

I can only talk about Flask, but from my experience, the app doesn't know its base path until told via a parameter (APPLICATION_ROOT) so having the base path in the route passed to apispec requires a deliberate action (prefixing APPLICATION_ROOT) in the first place that should be avoided.

Ultimately, OpenAPI v3 breaks the whole thing by allowing multiple base paths...

From there, I lean towards removing normalize_path rather than fixing it.

lafrech added a commit to marshmallow-code/flask-smorest that referenced this issue Oct 24, 2018
Discussed in marshmallow-code/apispec#78

It is the user's responsability to set the correct "basePath" in the spec.

Blueprint.register_views_in_doc does not pass app to spec.add_path anymore.
@lafrech
Copy link
Member

lafrech commented Dec 16, 2018

Closing as changes in #78 (comment) were implemented.

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 a pull request may close this issue.

2 participants