-
Notifications
You must be signed in to change notification settings - Fork 886
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
route_prefix doesn't allow the prefix pattern to match without a trailing slash #406
Comments
Thanks Michael for responding to my question on pylons-discuss here. My impression from the docs was that Pyramid considers empty routes to be "bad" and wants to correct your mistake - I don't see anything wrong with that assertion (which benefits from decisiveness) provided we can overrule:
The semantics of If we can do something like the above then the Furthermore, if someone wanted to make a particular 'modules' routes available from the web-root (for whatever reason), this would be expected:
|
Avoiding the real issue, I'll just say that your last example already works. |
Quite so! |
I'm may well not be understanding the issue as well as Michael, but I might well consider patching something like this: # pyramid/config/routes.py
class RoutesConfiguratorMixin(object):
@action_method
def add_route(self,
name,
# ..
allow_empty_pattern=False
):
# ..
if self.route_prefix:
route_prefix_join = '/'
if allow_empty_pattern and pattern == '' and self.route_prefix != '/':
route_prefix_join = ''
pattern = self.route_prefix.rstrip('/') + route_prefix_join + pattern.lstrip('/') Checking that the path genuinely is empty when we've flagged the |
Instead of doing anything more complicated, I'd probably suggest that you use http://docs.pylonsproject.org/projects/pyramid/en/1.3-branch/narr/urldispatch.html#redirecting-to-slash-appended-routes |
From your solution we have the following results (assuming the empty string cases have Case 1 - I appreciate the effort, but we've thought about this and a flag on I think the |
Where in the docs it "sound like empty routes are bad", out of curiosity? They've always been treated as the equivalent of a route with a |
I'm closing this, because I think we've reached an impasse. If I'm doing this in error, please reopen. |
I don't think there is an impasse. Sorry for slow reply - it's not an indication that I don't see a way to resolve this. In fact I just wanted to make sure I had properly considered all the information before responding further. If you can re-open I will post a snippet with my suggested design and documentation/rationale.
For now here is my revised code and tests - I will follow with the documentation/rationale. def route_prefix(
route_prefix='',
pattern='',
):
""" Proposed re-design for route-prefix re Issue #406 """
if route_prefix and pattern:
pattern = route_prefix.rstrip('/') + '/' + pattern.lstrip('/')
else:
pattern = route_prefix + pattern
return pattern
# Test
# (test #, prefix, pattern, goal, )
cases = (
(u'1.1', u'/p', u'', u'/p', ),
(u'1.2', u'/p', u'1', u'/p/1', ),
(u'1.3', u'/p', u'/', u'/p/', ),
(u'1.4', u'/p', u'/1', u'/p/1', ),
(u'1.5', u'/p/', u'' , u'/p/', ),
(u'1.6', u'/p/', u'1', u'/p/1', ),
(u'1.7', u'/p/', u'/', u'/p/', ),
(u'1.8', u'/p/', u'/1', u'/p/1', ),
(u'2.1', u'p', u'', u'p', ),
(u'2.2', u'p', u'1', u'p/1', ),
(u'2.3', u'p', u'/', u'p/', ),
(u'2.4', u'p', u'/1', u'p/1', ),
(u'2.5', u'p/', u'' , u'p/', ),
(u'2.6', u'p/', u'1', u'p/1', ),
(u'2.7', u'p/', u'/', u'p/', ),
(u'2.8', u'p/', u'/1', u'p/1', ),
(u'3.1', u'', u'', u'', ),
(u'3.2', u'', u'1', u'1', ),
(u'3.3', u'', u'/', u'/', ),
(u'3.4', u'', u'/1', u'/1', ),
(u'4.1', u'/', u'', u'/', ),
(u'4.2', u'/', u'1', u'/1', ),
(u'4.3', u'/', u'/', u'/', ),
(u'4.4', u'/', u'/1', u'/1', ),
)
if __name__ == '__main__':
colw = 8
print
print "ROUTE PREFIX TEST"
print
print "%s %s %s %s %s %s" % ('Test'.ljust(colw),
'Prefix'.ljust(colw),
'Pattern'.ljust(colw),
'Result'.ljust(colw),
'Goal'.ljust(colw),
'Success'.ljust(colw),
)
for c in cases:
output = route_prefix(route_prefix=c[1],pattern=c[2])
success = 'ok' if (output == c[3]) else 'FAIL'
print "%s %s %s %s %s %s" % (c[0].ljust(colw),
c[1].ljust(colw),
c[2].ljust(colw),
output.ljust(colw),
c[3].ljust(colw),
success.ljust(colw),
)
|
The below function is how I arrived at my proposed solution (above), I believe this is the easiest way for me to communicate my reasoning. My criticism of Pyramid's current route-prefix design is that:
If my suggestion were to be adopted, projects affected would be those relying on the current implicit configuration - that is, using empty routes with route-prefixes and expecting a slash ("/prefix" + "" = "/prefix/". The solution is pretty painless, either: a.) make empty patterns explicit (either empty or "/") b.) change the route-prefix to "/prefix/" which explicitly configures any mounted routes with the current "implicit behaviour" ("/prefix/" + "" = "/prefix/"). From this point on, I believe the route-prefix and pattern relationship would be completely consistent and 100% explicitly configurable. def route_prefix_logic(
route_prefix='',
pattern=None,
):
""" Logic/rationale of proposed re-design for route-prefix re Issue
#406, excluding simplification through coercion """
if route_prefix.endswith('/') and pattern.startswith('/'):
# explicit slash to right and left of join
# - act to prevent a double-slash at the join
pattern = route_prefix.rstrip('/') + pattern
elif route_prefix.endswith('/') or pattern.startswith('/'):
# explicit slash to right or left of the join (but not both)
# - the natural join is ok
pattern = route_prefix + pattern
else:
# no slashes specified, but there are only two possible scenarios
if route_prefix and pattern:
# act to prevent a join without a slash on either side
pattern = route_prefix + '/' + pattern
else:
# the route is the route_prefix or the pattern
pattern = route_prefix + pattern
return pattern |
A link to the original issue which added route prefixes for reference: #222 |
I'll note that it's just as impossible even without any prefix involved to make a sane distinction between an empty route pattern and a single '/'. This is because user agents are required to send '/' as the path even when a user explictly leaves off the trailing slash from a root URL. As a result, users currently don't expect that visiting "http://localhost" will behave any differently from "http://localhost/". The rationale for this is present in the HTTP 1.0 spec in http://www.w3.org/Protocols/HTTP/1.0/spec.html#Request-Line :
When a broken user agent does not send a "/" as the path and sends an empty string instead (e.g. "GET HTTP/1.0") CGI and WSGI servers tend to convert the empty path to "/" as an implicit fix. Eg. https://github.com/Pylons/waitress/blob/master/waitress/task.py#L449 . Every CGI and WSGI server has similar logic in it. |
Thinking a little further on my approach, I think I'm trying to say that: the remit of route-prefix should be limited to forming a successful join of a prefix and a pattern; that is to say, one in which the right-side of the prefix and the left-side of the pattern make sense.
I think I understand - although empty route and '/' are equivalent from a match-perspective (and docs have always made this very clear), I believe 'empty patterns' are essential from a configuration perspective. By allowing an empty route in configuration you achieve 100% explicitly configurable routes where the right-edge of prefix and left-edge of pattern are a means of supplying configuration intentions, and meet @mmerickel's comment to me that route_prefix must be able to mount any path without preparatory configuration. Older code may be mounted and 'work' without modification. One imagines module developers would be inclined to make their module-root '' (empty) to allow maximum flexibility in integration ("/mounted-module"), but they could just as well assert that their module-root always be "/mounted-module/" by making their module-root "/". |
While I'm explicitly not passing judgment on the utility of the proposal, in general, for any change to be seriously considered, the benefit of the change is going to need to greatly outweigh the potential misery for existing users caused by a backwards incompatibility. The change you're proposing would break any application which currently uses an include with a route_prefix where the set of routes being included contains a route with a pattern of '/' or ''. In the meantime, the current behavior is indeed logically consistent (the empty route pattern at the root represents '/', a prefixed-and-included empty route pattern represents '/prefix/'). Additionally, a mechanism exists for resolving an included empty route pattern (AppendSlashNotFoundView). So I think this is eventually going to boil down to a judgment between the benefits of retaining bw compat and the benefits of making the change. We're (for better or worse) very conservative when it comes to breaking bw compatibility, so at the risk of irritating you, such a change is very unlikely unless it can be shown to be in some way logically inconsistent. |
Correction: The change you're proposing would break any application which currently uses an include with a route_prefix where the set of routes being included contains a route with a pattern of ''. The pattern '/' would work. |
Pyramid makes no distinction between The goal of The question is how will My proposed solution is to make the behavior only dependent on Case 1 -
For the case of view-relative includes, it seems to me that there is no good solution via Alternatively, this approach gives the user more control over the URLs supported by their application. In general, the behavior most people probably want in their application is that of the Input would be great on this issue, because to me the solution is not obvious given Pyramid's history and the fact that no one can agree on the format for URLs on the internet in general. |
Wrt to @mmerickel proposal, here are my thoughts... I'll likely be eventually irritated when I need to remember to append a slash to a prefix when mounting an arbitrary third-party app that has a root view which depends on view-relative images/css. With the status quo, I don't need to remember anything, because the system makes the choice for me, and there's one unambiguous way it works. When I use e.g. rsync, or other systems where trailing slashes have meaning, I can never remember when appending a slash is necessary without reading the docs, and I usually succumb to that particular gotcha at least once before reading the docs. I think it's highly likely that people are going to want both '/prefix/' and '/prefix' to work. Adding the proposed feature is going to make it slightly harder to document how to get that behavior, because it will depend on the prefix they're using to include the external routes. Currently it's pretty easy; we just tell them to use AppendSlashNotFoundView. And if AppendSlashNotFoundView is undesirable, it's not strictly necessary to have one. Something like this would work too:
Those things said, I'm OK with the proposal @mmerickel mentions above if he believes it's the Right Thing and the requisite documentation is added, even if it would entail a minor bw incompatibility. I'd also be quite happy to have the status quo. |
I've put my efforts into a pull-request rather than try to describe further. @mmerickel I think your requirements are contained within what I'm proposing here and hope I have understood you correctly. I think module authors should be considering that the empty-route gives the user doing the include the most flexibility in how they mount it, but that the author can be opinionated if there is some reason for doing so. Likewise, with my solution above a user can overrule the author and use a slash-prepended prefix to insist that an empty-route becomes slash-appended. I think the system works well as it is with the exception of the issue with prefix mounting and empty pattern being ambiguous, so just solve the ambiguity with an explicit configuration setting. @mcdonc it seems to me it's a matter of opinion what is more logical, from either the HTTP spec or the configuration utility perspective. As @mmerickel says "no one can agree on a format anyway". I'm proposing to solve the problem comprehensively by doing what Pyramid does best - let people choose, and ship the best default as and when the web evolves. I believe the above solution creates more flexibility in this edge-case and doesn't break bw compatibility re:
@mcdonc not irritated in the slightest! And delighted to be having a conversation about this. I really need this flexibility for my project, and our marketing has very strong opinions about slash-appended paths. Personally I think it creates a dissonance between what a user sees in literature and what they see in the browser; we want to publish I'm not specifically hassling for you guys to accept this, but I will be using this patch (or something very like it) and am very pleased to contribute - even if my code helps by showing you what you don't want! :) Naturally, I would much prefer just to enable |
I'm -1 on any extra knobs (a global "allow_empty_pattern" setting or an "allow_empty_pattern" arg to include). Raydeo's proposal of making the slash at the end of a prefix meaningful makes more sense to me, although with the caveats I mentioned in my last comment (particularly, I'm not enthused by the idea of needing to explain to folks how both to append a slash and how to remove a slash depending on their prefix, it'll get awful confusing). I recognize that there's an aesthetic value to non-slash-appended routes, and that you're trying to make use of a route prefix for code factoring. At the same time, non-slash-appended URLs aren't universally sensible in the context of the original use case for route_prefix, which was to allow people to mount arbitrary third-party applications at arbitrary prefixes. Root views of such apps will always work at slash-appended URLs but they might or might not work at non-slash-appended URLs. For better or worse, I lean towards making the default something that has the highest likelihood of working independent of aesthetics. At the risk of stating the obvious, you don't have to use include() here at all if you're not concerned about composing multiple apps together (you don't need the conflict resolution behavior of include, presumably). Something like this would be just as sensible in that context:
|
Thanks Chris. I am seeking to make good-use of code factoring, but I was also hoping to present a highly logical approach to joining routes, which Pyramid currently does aside from #406. To me, At the risk of beating you over the head with it, I'll just add the my personal note that I chose Pyramid having matched your design defence against our requirements, particularly:
Sure, I know I can fudge-it but I want to be 'Pyramidic' and architect a pretty and logical application that is easy to understand for our team. I feel the current state of affairs excludes my approach, and I was hoping for:
I worry that if Pyramid starts excluding certain approaches on something as fundamental as route-design, what other features will be closed to me in future? Re your comment @mcdonc:
I was working on the basis of your design defence:
I don't think I understand why the discussion is about how 'third-party-apps' can be mounted and 'just work' when Pyramid is not designed to be pluggable with third-party modules, since this is seen to hamstring other frameworks. This requirement seems like it is causing a headache over what to do, rather than letting us decide how we want to architect our applications. Just advise module developers on how to provide maximum flexibility, and provided If my input is still welcome, I'll gladly try to follow along or re-think my solution re the -1 for a knob, otherwise I look forward to seeing what you come up with :) |
Fair point. "apps" is probably the wrong word here, sorry and I'm lazy to use it, particularly given the design defense stuff. Pyramid's isn't a platform for integrating "reusable apps". But Pyramid does allow a single app to be composed from multiple configuration sources, where some of those sources were not necessarily written by the person doing the composing. In particular, it should be possible for someone to write an includeme that provides routes and views that are consumable by a third party. And, in fact, if the includeme has views that work at the Pyramid root, if he doesn't use absolute literal URLs in the views, ideally, the original author shouldn't really need to think much about whether it will be included by somebody. It should just work when it's included, and currently does.
We're really talking about a very small subset of includeables that will break. Currently it's possible for a consumer to pull an arbitrary set of routes into a Pyramid application's configuration as long as those routes are factored out into an includeme-style include function as long as views attached to the routes use 1) completely relative URLs (urls without any leading slash) 2) absolute qualified URLs generated via route_url or 3) absolute nonqualified urls generated via route_path. However, when the root view is served without a slash appended, URLs it generates via #1 will not work. I'm nominally OK with nixing #1 as something that people who create includeables are allowed to do, although it makes the chances that using an include with a route pattern will work against arbitrary includemes slightly less likely. I'm not really OK with making it a global flag, because I don't think it's necessary if we do what @mmerickel has suggested and just make the slash at the end of the route prefix meaningful. Even though this is a backwards incompatibility, I think it's more sensible. My remaining concern is being able to provide enough documentation to someone whom:
I'm loath to add a RemoveSlashNotFoundView to do this, in particular, but given that it's bound to come up, it needs to be addressed. |
Got it. @mmerickel's proposal is very appealing, and since route-prefixes are the point at which a user has control (but having no other usage) they would be the natural configuration mechanism. The code I was presenting was coming from the perspective that you'd probably prefer the module's authors to have the ultimate say (to be strongly opinionated in enforcing the appended-slash, OR allow the prefix-right-edge to decide). @mmerickel sorry for banging the stick the other way, I misunderstood that it was commonly preferred to let the user/implementer have ultimate control of how empty-routes should be handled, and your approach solves my particular issue in any case. |
I'm going to implement this before the next alpha release. We'll see how it pans out. It's just odd to me to allow the |
Adding to your proposal - If you make
# configure '/users/' + '' = '/users/' and slash-appended-route-patterns
include('module', route_prefix='/users/', route_suffix='/')
# configure '/users' + '/' = '/users' and non-slash-appended-route-patterns
include('module', route_prefix'/users', route_suffix='')
# do whatever module author wants for slash-appending
include('module', route_prefix='/users', route_suffix=None)
Do you mean possibility of configuring both routes within the module code? Or that they should both serve the same view? (bad www? although github does it) |
Actually.. does this comes under "there is no difference between |
I think we can eliminate |
Personally I think this whole ticket is just a shortcoming of the design where |
I came to that conclusion, because if the implementer is going to control the slash on empty routes they should really control the slash on non-empty routes too. I've put my thinking into a pull re https://github.com/simonyarde/pyramid/compare/master...route-prefix-issue-406
The direction of the solution seems to be that callables cannot assume anything about how they will be mounted, that is if the design goal is met where any route can be mounted, and any level of nesting. What I was aiming for was to address that a when a third-party module is used on its own (top-level) then its internal prefix and suffix config works as normal, but when a module becomes a sub-module its prefix and suffix are ignored (in order to create joins) and the top-level controls empty-route and slash-appending. This assumes of course the route_suffix option is supplied; if it's empty then the sub-modules internal config wins. |
Please see alternative approach passing route_prefixes as lists instead of strings, I think it's neater and you can centralise all the route-pattern string creation. Route-suffixes make no difference to the slash-style, because the top-level include wins (user implementation level). |
Regarding this issue, it's going to get tabled for now. The modification of route_prefix to support mutating a route that was defined elsewhere was hinting at something, and the route_suffix feature that has been suggested makes it clear that the whole idea of mutating routes defined by "other people" is a whole new feature that needs to be more properly discussed API-wise if it's to be included. Pyramid's current implementation of route_prefix, while arguably limiting, is consistent and predictable. I've also looked at other frameworks that implement a similar feature and they also do it this way (automatic / appended), requiring you to use the Anyway, if you feel strongly enough about this I think it should be rephrased in terms of modifying route patterns in addons, which I will argue is out-of-scope for Pyramid. |
So here's the plan wrt to this issue:
If it turns out there's enough problems with this decision to indicate a change needs to be made, we'll reconsider this in 1.4. In the meantime, I'm going to make AppendSlashNotFoundView a little less crappy to actually use. |
Reread the thread 3 times. Now understand.
Right now, In all 4 cases from first post, end result is the same 'some_prefix/'
After this patch 0169301:
@mcdonc suggested using append_slash option for people from second group. But this option is convenient for people from first group, b/c their patterns contains slash at the end. And they most likely will use this option for convenience for their users. For people from 1 group this option needed only in connection with this #406 issue. So may be it is wise to switch to second behavior, so both group will be satisfied. |
@mmerickel When you going to fix this so that #819 will just work? =) |
I have a feeling that this new bug report is related: #2143 |
Regarding the 2.0 (#2362) possibility: I think @mmerickel's original notion of making things dependent on the existence of a trailing slash in the I think think the simplest option is to addd a kwarg to
Keep in mind that different plugins will be built and want to be implemented differently. One plugin might not even have a '' route, others might. Some people might not even be using the The best workaround I've found in the meantime is to explicitly declare whatever the "index" route is named to be just the prefix:
That requires no 3rd party module, and does not introduce a global not-found behavior. An implementation detail of routes that I noticed (but haven't had time to look into), is that this method will always result in the |
fixed via #3420 |
The issue here is an ambiguity in handling
route_prefix
. In general, things work correctly:The ambiguity arises when the included function attempts to add a route using the pattern
'/'
or''
(an empty string). Pyramid elects to treat the prefix as a container.Remember that in Pyramid's
config.add_route
there is no difference between prepending a'/'
or leaving no prefix. Thus, the two route definitions below result in the same url being matched and generated.There are 4 possible use cases to account for when looking at the issue.
Case 1
Case 2
Case 3
Case 4
Workaround
Pyramid currently treats the route prefix as a container, thus the resulting route will always be appended with a
'/'
. This may not be ideal, but it at least allows for a workaround where the user can add their own route (at the level of the include):If there is a clear way to handle each use-case, and document it as such, then it may be possible to change this behavior but to me it's hard to expect a user to "do the right thing" with case 2 due to the way pyramid currently handles
add_route
patterns with and without a prefixed slash.The text was updated successfully, but these errors were encountered: