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

Modify route_prefix_context to preserve leading slash #3758

Open
fmigneault opened this issue Apr 6, 2024 · 4 comments · May be fixed by #3759
Open

Modify route_prefix_context to preserve leading slash #3758

fmigneault opened this issue Apr 6, 2024 · 4 comments · May be fixed by #3759

Comments

@fmigneault
Copy link

Feature Request

Is your feature request related to an issue? Please describe.

When route_prefix_context is used to apply a prefix on various routes, the supplied route_prefix gets stripped to remove any leading/training slashes. Provided that Pyramid handles /prefix/route and prefix/route the same way, it should not strip the leading slash (it is fine to strip the training one to allow addition of more routes after).

This was highlighted a while back in #2143, but not addressed further.

When working with utilities like https://github.com/Cornices/cornice and https://github.com/Cornices/cornice.ext.swagger, it is possible to define a Service that uses the prefix_route to obtain the correct path for the OpenAPI rendering. Because route_prefix_context strips the path, it is not generating the intended path.

Describe the solution you'd like

Modify this line:

route_prefix = route_prefix.strip('/')

- route_prefix = route_prefix.strip('/')
+ route_prefix = route_prefix.rstrip('/')

When using rstrip and with config.route_prefix_context("/ogcapi"), the routes below get rendered correctly.
They behave and respond exactly the same way as if the path were stripped from the leading /.

image

With the current strip, all leading / are lost, creating inconsistent paths:

image

Describe alternatives you've considered

Override the pyramind.config.Configurator.route_prefix_context method.
While this works, it is a dirty workaround as I have to duplicate the code to keep all the other self.begin(), etc. calls.

Additional context
This is where the supplied route_prefix is relevant in the package that uses it for eventual OpenAPI generation :
https://github.com/Cornices/cornice/blob/abfcfd9eeefaf281237ba84a3b90ef5ee0698aa9/src/cornice/pyramidhook.py#L169-L183

@mmerickel
Copy link
Member

Similar to what I said in #2143 if you wish to submit a fix to normalize pyramid's internal representation of the path I think that's fine to do. Considering WSGI PATH_INFO begins with a / or is an empty string, again it seems to make sense that pyramid would normalize on that internally as well.

I would recommend instead that cornice just normalize it itself since pyramid isn't making that guarantee right now that IRoute.pattern begins with a slash. I'm probably missing why that's such a difficult thing to do... However like I said I'd accept a PR to sort that out within pyramid's IRoute itself but no one contributed a PR last time.

@fmigneault
Copy link
Author

I'm not sure if there are any use cases where users would rely on routes that would not have the prefix slash, for the same reason that it is not guaranteed right now. At the very least, I couldn't see any side effect by replacing strip by rstrip. I'm not sure what/where else to look for adjustments to normalize the /. Could this be done gradually?

@mmerickel
Copy link
Member

It’s a general problem with any route, not just the route prefix iirc. You can just add_route('foo', 'bar') and it’ll be stored as pattern bar internally. I suspect the solution is just to fix it in add_route but I’m speaking from memory and haven’t dove into it to confirm.

@fmigneault
Copy link
Author

From what I see in

elif self.route_prefix:
if pattern == '' and inherit_slash:
pattern = self.route_prefix
else:
pattern = (
self.route_prefix.rstrip('/') + '/' + pattern.lstrip('/')
)

Any necessary / striping is already handled internally by add_route.

Therefore, it seems even more appropriate to avoid striping in route_prefix_context, and let add_route do its job.

route_prefix = route_prefix.strip('/')

Thoughts?

@fmigneault fmigneault linked a pull request Apr 12, 2024 that will close this issue
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