Pylons / pyramid

Pyramid - A Python web framework
https://trypyramid.com/
Other
3.97k stars 887 forks source link

Modify `route_prefix_context` to preserve leading slash #3758

Open fmigneault opened 7 months ago

fmigneault commented 7 months ago

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 https://github.com/Pylons/pyramid/issues/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: https://github.com/Pylons/pyramid/blob/72f61853beda8e21b669c3520e43fe3e5b224ba3/src/pyramid/config/routes.py#L602

- 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 commented 7 months ago

Similar to what I said in https://github.com/Pylons/pyramid/issues/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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

From what I see in https://github.com/Pylons/pyramid/blob/72f61853beda8e21b669c3520e43fe3e5b224ba3/src/pyramid/config/routes.py#L413-L419 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. https://github.com/Pylons/pyramid/blob/72f61853beda8e21b669c3520e43fe3e5b224ba3/src/pyramid/config/routes.py#L602

Thoughts?