Fix index matching with trailing slashes #16

Open
wants to merge 7 commits into
from

Projects

None yet

2 participants

@melo
Collaborator

Mason 2.20 fails in this situation: with a index.mc inside a foo/ directory:

  • GET /foo: the index.mc is found and used;
  • GET /foo/: no match is found.

This pull request has the failing test cases for now. Working on a fix at the moment.

melo added some commits Jan 7, 2013
@melo melo Revert "Add resolver logic to deal with the presence of trailing slas…
…hes"

This reverts commit 7a17fe0.

Signed-off-by: Pedro Melo <melo@simplicidade.org>
baa1643
@melo melo Added two tests to check index matching with trailing slashes
Signed-off-by: Pedro Melo <melo@simplicidade.org>
1446b5e
@melo melo Make sure our test descriptions include the =1 flag
Signed-off-by: Pedro Melo <melo@simplicidade.org>
06a6c25
@melo melo Redo fix for #8, respecting the index matches without allow_path_info
Signed-off-by: Pedro Melo <melo@simplicidade.org>
2a58680
@melo
Collaborator

Ok, works now.

I've started by reverting the previous #8 fix, reverting the code changes from PR #11, and then added the new tests, and finally made the tests from #11 (most of them, some were just wrong) work.

@jonswar I'll double check the tests, and test it with my app; then I'll update the docs (in this branch right now, they are wrong) and add them to this PR.

@melo melo Document new trailing slashes behaviour
Added the documentation test cases to the test suite.

Signed-off-by: Pedro Melo <melo@simplicidade.org>
bdd2292
@melo
Collaborator

Done, all test pass, documentation up-to-date.

pstadt and others added some commits Jan 8, 2013
@pstadt pstadt Fix: component.mc wins over index/dhandlers without trailing slash
Signed-off-by: Pedro Melo <melo@simplicidade.org>
233f45b
@melo melo Improve readability of the @candidate_paths selection process
Signed-off-by: Pedro Melo <melo@simplicidade.org>
c6fb692
@melo
Collaborator

Done.

Did a bit of cleanup, building @candidate_paths was getting messy.

@pstadt comments?

@jonswar You might need to tidy up later. I tried to use your tidyall.ini but you are using a private JS::Code::TidyAll class

@pstadt

Testcases are still not complete, it feels like opening pandora's box ;-)

Can you please have a look at test_decline a few lines later and the expected decline-order?
As far as I can see

$try->( '/foo.mc',              'bar' );
$try->( '/foo/dhandler.mc',     'bar' );

is not correct the correct order according to the docs but it matches successful.

So here are the new tests:

$try->( '/news/sports', ['/dhandler','/news.mc=1'], '/news.mc', 'sports');
$try->( '/news/sports', ['/news/dhandler','/news.mc=1'], '/news/dhandler', 'sports');
$try->( '/news/sports/', ['/news/dhandler','/news.mc=1'], '/news/dhandler', 'sports/');

Gets more and more confusing.
Check order in general has to be @idx_dhandler, @filtered_autoext.
Check order has to be @filtered_autoext. @idx_dhandler for the first loop ($path_info eq '') only if $trailing_slash eq ''.
I this correct?
Then this would be the correct code:

if ($trailing_slash eq '' && path_info eq '') {
    @candidate_paths = (@filtered_autoext, @idx_dhandler);
} else {
    @candidate_paths = (@idx_dhandler, @filtered_autoext);
}

Now I will have a closer look on

if ($path_info eq '' && !@autoextensions)

as path_info was set to '/' in the old code but now it is always empty at the first loop.

@melo
Collaborator

I need to take a break and step back from it all...

With #8, I only wanted to make sure that path_info included the trailing slash if I requested id with allow_path_info => 1. That is all.

I would like to run the current test suite on this PR with 2.19, and see if those tests that @pstadt mentions fail or not. If not, then they are correct, and the documentation is misleading, and needs to be updated. I don't want to change the matching semantics, just want to capture the trailing slash in path_info.

@jonswar 2.20 is a broken release due to my #11 pull request. I think it should be reverted ASAP and release 2.21, and afterwards, if this PR is stable and makes sense, you can if you wish make a 2.22 release with it.

If you agree, tell me and I'll prepare a new PR to revert #11. It should be as simple as cherry pick baa1643 from this PR and then remove the documentation (this patch melo@7a17fe0#L1R71 from lib/Mason/Manual/RequestDispatch.pod).

I need to work now, I'll get back to this tomorrow with a fresher mind.

@pstadt

I think we are on a good way.

I am not sure if 2.19 is a good basis to work on, as it has contradictory statements about matching in code comments and RequestDispatch.pod (and an implementation). And 2.19 does not define how to deal with trailing slashes except in code.

I think best is to go back to drawing board what should be the matching order.

As far as I can see there might be only one incompatible change between new and old: thats what the decline_test complains about with latest changes.

And it might not be necessary to do a quick 2.21 release because noone complained for half a year about the 2.20 release

@melo
Collaborator

I don't know right now.

If I ever see someone with /foo.mc and one of /foo/index.mc or /foo/dhandler.mc, my reaction would probably be:

No, no, no, no, no, no...

So...

I don't mind doing the work, but clearly, at least for me, a step back is required, and that means 2.19, because 2.20 with #11 clearly broke more that it fixed.

@pstadt

Strictly spoken a dhandler is identical to an index component with the full meaning of allow_path_info if set: intercept on every level and return path_info.
But I think this would go to far ;-)

@pstadt

Played around a little with the test and 2.19, these are all successful:

    $try->( '/news/sports', ['/news/dhandler','/news/sports.mc'], '/news/sports.mc', '');
    $try->( '/news/sports', ['/news/dhandler','/news/sports.mc=1'], '/news/sports.mc', '');
    $try->( '/news/sports/', ['/news/dhandler','/news/sports.mc=1'], '/news/dhandler', 'sports');
    $try->( '/news/sports/', ['/news/dhandler','/news/sports.mc'], '/news/dhandler', 'sports');
    $try->( '/news/sports/hockey', ['/news/dhandler','/news/sports.mc=1'], '/news/sports.mc', 'hockey');
    $try->( '/news/sports/hockey', ['/news/dhandler','/news/sports.mc'], '/news/dhandler', 'sports/hockey');

I think the result of the 3rd test is a bug (only about components, not about path_info).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment