Hey everyone, I've been revamping the manual for the next 1.6 release and working with Matt on fixing up some usability bugs first. One that came up is how routing now works after a clean up of some of the tst and routing code. I've distilled the problem into a single unit test with a quiz: http://dpaste.de/DVPB/ If you can take a look and either just reply with your answer to the quiz, or rewrite the unit test to fail in the ways you think it should. Then I'll look at how to update the routing based on that. Remember though, the mongrel2 routing isn't supposed to be like the full on awesome routing you've got in your framework, it's meant to be just enough to figure out where stuff should go. Thanks! -- Zed A. Shaw http://zedshaw.com/
Zed, I would like to be able to help improve the unit tests, but I don't feel comfortable doing so until I understand the usage of mu_assert check_simple_prefix From what little I understand of the unit test, it seems to me that the pattern and the string have been reversed. Consider the following routes: / /user /user$ Here is what I expect: "/us" matches "/" "/" is the longest pattern/substring of "/us" "/user" matches "/user$" longer because EOL matches in "/user$" but not "/user" "/usersBLAAHAHAH" matches "/user" use "/user$" if you don't want the match In my opinion, "/" should match everything that is not otherwise matched; that is, it is the default route. The only way to get a 404 would be if "/" was removed. My $0.02. Henry On May 10, 2011 01:18:58 PM Zed A. Shaw wrote: > Hey everyone, > > I've been revamping the manual for the next 1.6 release and working with > Matt on fixing up some usability bugs first. One that came up is how > routing now works after a clean up of some of the tst and routing code. > I've distilled the problem into a single unit test with a quiz: > > http://dpaste.de/DVPB/ > > If you can take a look and either just reply with your answer to the > quiz, or rewrite the unit test to fail in the ways you think it should. > > Then I'll look at how to update the routing based on that. > > Remember though, the mongrel2 routing isn't supposed to be like the full > on awesome routing you've got in your framework, it's meant to be just > enough to figure out where stuff should go. > > Thanks! -- Henry Baragar Instantiated Software
On Tue, May 10, 2011 at 02:14:33PM -0400, Henry Baragar wrote: > Zed, > > I would like to be able to help improve the unit tests, but I don't feel > comfortable doing so until I understand the usage of Don't worry about it, just the description you have below is good: > Consider the following routes: > / > /user > /user$ Ok, rewriting what you have: > "/user" matches "/user$" > "/usersBLAAHAHAH" matches "/user" > "/us" matches "/" because "/" is the longest pattern/substring of "/us" Actually you can't have these because we store only prefixes and patterns. Here's how it works: 1. you have routes /user, and /user($) to match (notice the ($), that's the pattern. 2. mongrel2 loads /user into the routing table, marks it as not having a pattern, and you can only have one in there at a time. 3. mongrel2 tries to load /user($), splits it into /user prefix and ($) pattern. 4. it then hits the existing /user already in the routing table and then the routes fail to load since you can only have one. Ok, try it with these instead: / /people /user($) Then matching: /user == /user($) /userBLAHBLAH == 404 /people == /people /peopleBLAHBLAH == /people /pe == /people / == / /applesauce == / With that in mind, does that change what you think it should do? -- Zed A. Shaw http://zedshaw.com/
On May 10, 2011 03:22:11 PM Zed A. Shaw wrote: > On Tue, May 10, 2011 at 02:14:33PM -0400, Henry Baragar wrote: > > Zed, > > > > I would like to be able to help improve the unit tests, but I don't feel > > comfortable doing so until I understand the usage of > > Don't worry about it, just the description you have below is good: > > Consider the following routes: > > / > > /user > > /user$ > > Ok, rewriting what you have: > > "/user" matches "/user$" > > "/usersBLAAHAHAH" matches "/user" > > "/us" matches "/" because "/" is the longest pattern/substring of "/us" > > Actually you can't have these because we store only prefixes and > patterns. Here's how it works: > > 1. you have routes /user, and /user($) to match (notice the ($), that's > the pattern. > 2. mongrel2 loads /user into the routing table, marks it as not having a > pattern, and you can only have one in there at a time. > 3. mongrel2 tries to load /user($), splits it into /user prefix and ($) > pattern. > 4. it then hits the existing /user already in the routing table and then > the routes fail to load since you can only have one. > OK. > Ok, try it with these instead: > > / > /people > /user($) > I think I see one source of misunderstanding: there is an implicit (.*$) pattern added if there is no other pattern. So what have written really is shorthand for: /(.*$) /people(.*$) /user($) Now, most of you responses make sense... > Then matching: > > /user == /user($) > /userBLAHBLAH == 404 > /people == /people > /peopleBLAHBLAH == /people > /pe == /people ... with the exception of this one:-) It can not be explained using the rewritten patterns, nor does it make sense to me. Consider the following: /img([0-9]+) /img2001 I would expect "/img20" to give me the "20" image from my "/images" directory (a Dir handler) and would be very surprised to get the "img2001" from of my "space-odessy" directory! Henry > / == / > /applesauce == / > > With that in mind, does that change what you think it should do? -- Henry Baragar Instantiated Software
On Tue, May 10, 2011 at 04:47:32PM -0400, Henry Baragar wrote: > I think I see one source of misunderstanding: there is an implicit (.*$) > pattern added if there is no other pattern. So what have written really is > shorthand for: > > /(.*$) > /people(.*$) > /user($) Aha! That is an *excellent* way of explaining it. Why the hell didn't I think of that. > ... with the exception of this one:-) It can not be explained using the > rewritten patterns, nor does it make sense to me. > > Consider the following: > > /img([0-9]+) > /img2001 > > I would expect "/img20" to give me the "20" image from my "/images" directory I believe that works actually. I'll add it to the test and come back to you. Ok, now I think I can explain the behavior using just patterns and it'll be much clearer. Thanks man! -- Zed A. Shaw http://zedshaw.com/
On Wed, May 11, 2011 at 8:17 AM, Zed A. Shaw <zedshaw@zedshaw.com> wrote: > On Tue, May 10, 2011 at 04:47:32PM -0400, Henry Baragar wrote: > > I think I see one source of misunderstanding: there is an implicit (.*$) > > pattern added if there is no other pattern. So what have written really > is > > shorthand for: > > > > /(.*$) > > /people(.*$) > > /user($) > > Aha! That is an *excellent* way of explaining it. Why the hell didn't I > think of that. > > > ... with the exception of this one:-) It can not be explained using the > > rewritten patterns, nor does it make sense to me. > > > > Consider the following: > > > > /img([0-9]+) > > /img2001 > > > > I would expect "/img20" to give me the "20" image from my "/images" > directory > > I believe that works actually. I'll add it to the test and come back to > you. > > Ok, now I think I can explain the behavior using just patterns and it'll > be much clearer. Thanks man! > > -- > Zed A. Shaw > http://zedshaw.com/ > Is it easier to replace the / route with a proper default route? Then it becomes longest complete match or default?
On Wed, May 11, 2011 at 9:48 AM, joshua simmons <simmons.44@gmail.com>wrote: > > > On Wed, May 11, 2011 at 8:17 AM, Zed A. Shaw <zedshaw@zedshaw.com> wrote: > >> On Tue, May 10, 2011 at 04:47:32PM -0400, Henry Baragar wrote: >> > I think I see one source of misunderstanding: there is an implicit >> (.*$) >> > pattern added if there is no other pattern. So what have written really >> is >> > shorthand for: >> > >> > /(.*$) >> > /people(.*$) >> > /user($) >> >> Aha! That is an *excellent* way of explaining it. Why the hell didn't I >> think of that. >> >> > ... with the exception of this one:-) It can not be explained using the >> > rewritten patterns, nor does it make sense to me. >> > >> > Consider the following: >> > >> > /img([0-9]+) >> > /img2001 >> > >> > I would expect "/img20" to give me the "20" image from my "/images" >> directory >> >> I believe that works actually. I'll add it to the test and come back to >> you. >> >> Ok, now I think I can explain the behavior using just patterns and it'll >> be much clearer. Thanks man! >> >> -- >> Zed A. Shaw >> http://zedshaw.com/ >> > > Is it easier to replace the / route with a proper default route? Then it > becomes longest complete match or default? > What I mean would be to replace the default_route stuff with a magic route name that becomes the default. Akin to default in a switch statement. That way you don't need a weird '/' route, and ordinary routes can be matched as usual.
2011/5/10 Zed A. Shaw <zedshaw@zedshaw.com>: > Hey everyone, > > I've been revamping the manual for the next 1.6 release and working with > Matt on fixing up some usability bugs first. One that came up is how > routing now works after a clean up of some of the tst and routing code. > I've distilled the problem into a single unit test with a quiz: > > http://dpaste.de/DVPB/ > > If you can take a look and either just reply with your answer to the > quiz, or rewrite the unit test to fail in the ways you think it should. > > Then I'll look at how to update the routing based on that. > Hello zed, before the quiz (I'm still thinking about my answer...), I think these two lines on this test case are swapped: mu_assert(check_simple_prefix(routes, "/cars-fast/cadillac", route_data3), "Failed."); mu_assert(check_simple_prefix(routes, "/users/people/1234", route_data4), "Failed."); According to the routes definition: bstring route3 = bfromcstr("/users/people/([0-9]+)$"); bstring route4 = bfromcstr("/cars-fast/([a-z]-)$"); So "/cars-fast/cadillac" shoud match with route_data4 and "/users/people/1234" with route_data3. Now to the quiz: For me the most intuitive route to return is "/user" (the way it is now). Consider this possible routes: "/" "/upper" "/user" And the request path "/us". I think the intuitive way to find a matching route would be to look at each char of the request path and at each look we can discard all possible route that doesn't match that char, eg: look at "/", here we have all 3 routes as good candidates; look at "u", here we discard "/" and keep "/upper" and "/user" look at "s", here we discard "/upper", because it's third char is != 's'. Now there is no more chars to look but we still have one candidate, "/user", so we could return it. If we had eliminated all possibilities we could return 404. Don't know what to do if we end up with more than one possible routes. =) I don't know if mongrel2 already does the route matching this way (didn't study this part of the code yet). -- Dalton Barreto http://daltonmatos.wordpress.com http://wsgid.com
On Tue, May 10, 2011 at 03:01:26PM -0300, Dalton Barreto wrote: > 2011/5/10 Zed A. Shaw <zedshaw@zedshaw.com>: > So "/cars-fast/cadillac" shoud match with route_data4 and > "/users/people/1234" with route_data3. Ah, actually it's because lines 19-20 are wrong: RouteMap_insert(routes, route3, route_data4); RouteMap_insert(routes, route4, route_data3); Should be the other way around. Thanks, I'll fix that. > For me the most intuitive route to return is "/user" (the way it is > now). Consider this possible routes: Ok, so for you it is a "match the best prefix" kind of match, and if someone wants more exact they use a pattern to say it. -- Zed A. Shaw http://zedshaw.com/
2011/5/10 Zed A. Shaw <zedshaw@zedshaw.com>: > On Tue, May 10, 2011 at 03:01:26PM -0300, Dalton Barreto wrote: >> 2011/5/10 Zed A. Shaw <zedshaw@zedshaw.com>: >> So "/cars-fast/cadillac" shoud match with route_data4 and >> "/users/people/1234" with route_data3. > > Ah, actually it's because lines 19-20 are wrong: > > RouteMap_insert(routes, route3, route_data4); > RouteMap_insert(routes, route4, route_data3); > > Should be the other way around. Thanks, I'll fix that. > Great! >> For me the most intuitive route to return is "/user" (the way it is >> now). Consider this possible routes: > > Ok, so for you it is a "match the best prefix" kind of match, and if > someone wants more exact they use a pattern to say it. > Yes. If not a pattern at least another route but with a "better prefix match". -- Dalton Barreto http://daltonmatos.wordpress.com http://wsgid.com
On May 10, 2011 02:01:26 PM Dalton Barreto wrote: > 2011/5/10 Zed A. Shaw <zedshaw@zedshaw.com>: > > Hey everyone, > > > > I've been revamping the manual for the next 1.6 release and working with > > Matt on fixing up some usability bugs first. One that came up is how > > routing now works after a clean up of some of the tst and routing code. > > I've distilled the problem into a single unit test with a quiz: > > > > http://dpaste.de/DVPB/ > > > > If you can take a look and either just reply with your answer to the > > quiz, or rewrite the unit test to fail in the ways you think it should. > > > > Then I'll look at how to update the routing based on that. > > Hello zed, > > before the quiz (I'm still thinking about my answer...), I think these > two lines on this test case are swapped: > > mu_assert(check_simple_prefix(routes, "/cars-fast/cadillac", > route_data3), "Failed."); > mu_assert(check_simple_prefix(routes, "/users/people/1234", > route_data4), "Failed."); > > According to the routes definition: > > bstring route3 = bfromcstr("/users/people/([0-9]+)$"); > bstring route4 = bfromcstr("/cars-fast/([a-z]-)$"); > > So "/cars-fast/cadillac" shoud match with route_data4 and > "/users/people/1234" with route_data3. > > > Now to the quiz: > > For me the most intuitive route to return is "/user" (the way it is > now). Consider this possible routes: > > "/" > "/upper" > "/user" > > And the request path "/us". I think the intuitive way to find a > matching route would be to look at each char of the request path and > at each look we can discard all possible route that doesn't match that > char, eg: > > look at "/", here we have all 3 routes as good candidates; > look at "u", here we discard "/" and keep "/upper" and "/user" > look at "s", here we discard "/upper", because it's third char is != 's'. > > Now there is no more chars to look but we still have one candidate, > "/user", so we could return it. If we had eliminated all possibilities > we could return 404. Don't know what to do if we end up with more than > one possible routes. =) > How would you expect "/" to be routed? Doesn't it match all patterns in your algorithm? Henry > > I don't know if mongrel2 already does the route matching this way > (didn't study this part of the code yet). -- Henry Baragar Instantiated Software
On Tue, May 10, 2011 at 02:24:35PM -0400, Henry Baragar wrote: > How would you expect "/" to be routed? Doesn't it match all patterns in your > algorithm? Nope, it finds that one exactly and stops. Remember it's going by the route *and* the path it's checking. So it's going to do this: / in route, / in path -> next end of chain, return that match. -- Zed A. Shaw http://zedshaw.com/
2011/5/10 Henry Baragar <Henry.Baragar@instantiated.ca>: > On May 10, 2011 02:01:26 PM Dalton Barreto wrote: > >> 2011/5/10 Zed A. Shaw <zedshaw@zedshaw.com>: > >> > Hey everyone, > >> > > >> > I've been revamping the manual for the next 1.6 release and working with > >> > Matt on fixing up some usability bugs first. One that came up is how > >> > routing now works after a clean up of some of the tst and routing code. > >> > I've distilled the problem into a single unit test with a quiz: > >> > > >> > http://dpaste.de/DVPB/ > >> > > >> > If you can take a look and either just reply with your answer to the > >> > quiz, or rewrite the unit test to fail in the ways you think it should. > >> > > >> > Then I'll look at how to update the routing based on that. > >> > >> Hello zed, > >> > >> before the quiz (I'm still thinking about my answer...), I think these > >> two lines on this test case are swapped: > >> > >> mu_assert(check_simple_prefix(routes, "/cars-fast/cadillac", > >> route_data3), "Failed."); > >> mu_assert(check_simple_prefix(routes, "/users/people/1234", > >> route_data4), "Failed."); > >> > >> According to the routes definition: > >> > >> bstring route3 = bfromcstr("/users/people/([0-9]+)$"); > >> bstring route4 = bfromcstr("/cars-fast/([a-z]-)$"); > >> > >> So "/cars-fast/cadillac" shoud match with route_data4 and > >> "/users/people/1234" with route_data3. > >> > >> > >> Now to the quiz: > >> > >> For me the most intuitive route to return is "/user" (the way it is > >> now). Consider this possible routes: > >> > >> "/" > >> "/upper" > >> "/user" > >> > >> And the request path "/us". I think the intuitive way to find a > >> matching route would be to look at each char of the request path and > >> at each look we can discard all possible route that doesn't match that > >> char, eg: > >> > >> look at "/", here we have all 3 routes as good candidates; > >> look at "u", here we discard "/" and keep "/upper" and "/user" > >> look at "s", here we discard "/upper", because it's third char is != 's'. > >> > >> Now there is no more chars to look but we still have one candidate, > >> "/user", so we could return it. If we had eliminated all possibilities > >> we could return 404. Don't know what to do if we end up with more than > >> one possible routes. =) > >> > > How would you expect "/" to be routed? Doesn't it match all patterns in your > algorithm? > > Henry > Good point! =) That would be the case when we looked at all chars of the request path and still ended with more than one possible matching route. Maybe in this situation we could return the shortest route? Do you think that this makes any sense? -- Dalton Barreto http://daltonmatos.wordpress.com http://wsgid.com
On May 10, 2011 02:36:04 PM Dalton Barreto wrote: > Good point! =) > That would be the case when we looked at all chars of the request path > and still ended with more than one possible matching route. Maybe in > this situation we could return the shortest route? > Sounds schizophrenic:) > Do you think that this makes any sense? See my response to Zed. Henry -- Henry Baragar Instantiated Software
If the matching becomes indeterminate, as in the case with multiple matching routes, return 404? ✈ Matt On May 10, 2011, at 11:47 , Henry Baragar wrote: > On May 10, 2011 02:36:04 PM Dalton Barreto wrote: > > Good point! =) > > That would be the case when we looked at all chars of the request path > > and still ended with more than one possible matching route. Maybe in > > this situation we could return the shortest route? > > > > Sounds schizophrenic:) > > > > Do you think that this makes any sense? > > See my response to Zed. > > Henry > > -- > Henry Baragar > Instantiated Software
On Tue, May 10, 2011 at 11:57:43AM -0700, Matt Towers wrote: > If the matching becomes indeterminate, as in the case with multiple > matching routes, return 404? Tried that, and it's horribly slow. You have to build a list of all the possible matches, then if you get more than one, report each one and abort because of an ambiguous routing. BUT, I wonder if this could be a "dev guy sucks" flag. If you flip the dev run bit, then it'll do the slower lookup and throw a 500 with a big log message saying you've got amigous routes. -- Zed A. Shaw http://zedshaw.com/
I kinda like that idea. Garbage in, garbage out. ✈ Matt On May 10, 2011, at 12:13 , Zed A. Shaw wrote: > BUT, I wonder if this could be a "dev guy sucks" flag. If you flip the > dev run bit, then it'll do the slower lookup and throw a 500 with a big > log message saying you've got amigous routes.