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

Bug in parsing path with dots #36

Closed
TroyKomodo opened this issue Jan 1, 2024 · 5 comments · Fixed by #37
Closed

Bug in parsing path with dots #36

TroyKomodo opened this issue Jan 1, 2024 · 5 comments · Fixed by #37

Comments

@TroyKomodo
Copy link
Contributor

#[test]
fn test_dots_no_ext() {
    let mut tree = PathTree::new();
    let _ = tree.insert("/:name", 1);

    let result = tree.find("/abc.xyz.123");
    assert!(result.is_some());

    let (value, params) = result.unwrap();
    assert_eq!(value, &1);

    assert_eq!(params.params(), &[("name", "abc.xyz.123")]);
}

#[test]
fn test_dots_ext() {
    let mut tree = PathTree::new();
    let _ = tree.insert("/:name.123", 1);

    let result = tree.find("/abc.xyz.123");
    assert!(result.is_some());

    let (value, params) = result.unwrap();
    assert_eq!(value, &1);

    assert_eq!(params.params(), &[("name", "abc.xyz")]);
}

I would expect both tests to pass, however only the test_dots_no_ext passes.

@fundon fundon added bug Something isn't working help wanted Extra attention is needed and removed help wanted Extra attention is needed bug Something isn't working labels Jan 1, 2024
fundon added a commit that referenced this issue Jan 1, 2024
@fundon
Copy link
Member

fundon commented Jan 1, 2024

Hi @TroyKomodo,

I added the test cases, use tree.insert("/:name*.123", 1);.

The :name pattern does not match -, ., ~, /,\\,: with a splitter in a piece.
Both :name+ and :name* match it.

So we can use :name+ or :name* to handle it.

@TroyKomodo
Copy link
Contributor Author

TroyKomodo commented Jan 1, 2024

Oh I see I did not expect that. Is there any reason the : splitter doesn't match .?
Also, what causes the 2nd test to fail if it's not matching the dot. I assume it's using the abc.123 as the name of the parameter.

@fundon
Copy link
Member

fundon commented Jan 1, 2024

PR welcome.

Sorry, it's too long, I need to read codes.

@TroyKomodo
Copy link
Contributor Author

PR welcome.

Sorry, it's too long, I need to read codes.

No it's not a problem, I was just asking if you knew why what the matcher was doing when it matched. I am indifferent on the syntax if you think it's a feature worth adding I would be happy to PR for it.

@fundon
Copy link
Member

fundon commented Jan 1, 2024

I think that case should be matched. Your expectation was right.

TroyKomodo added a commit to TroyKomodo/path-tree that referenced this issue Jan 1, 2024
This commit adds a feature which allows the normal matcher paths like
`/:name.js` or `/:name.xyz.js`

Previously this would have found no routes on paths like
`/frame.js` or `/frame.xyz.js`

Now the matcher is able to find the paths correctly.

fixes: viz-rs#36
TroyKomodo added a commit to TroyKomodo/path-tree that referenced this issue Jan 1, 2024
This commit adds a feature which allows the normal matcher paths like
`/:file.gz` or `/:lib.js.gz`

Previously this would have found no routes on paths like
`/node.js.gz`

Now `/:file.gz` matches `/node.js.gz` where "name" => "node.js"
And `/:lib.js.gz` matches `/node.js.gz` where "lib" => "node"

Which to me seems like the correct result in these cases.

fixes: viz-rs#36
TroyKomodo added a commit to TroyKomodo/path-tree that referenced this issue Jan 1, 2024
This commit adds a feature which allows the normal matcher to match
paths like `/:file.gz` or `/:lib.js.gz`

Previously this would have found no routes on paths like
`/node.js.gz`

Now `/:file.gz` matches `/node.js.gz` where "name" => "node.js"
And `/:lib.js.gz` matches `/node.js.gz` where "lib" => "node"

Which to me seems like the correct result in these cases.

fixes: viz-rs#36
@fundon fundon closed this as completed in #37 Jan 2, 2024
fundon added a commit that referenced this issue Jan 2, 2024
* feat: normal match repeated pattern

This commit adds a feature which allows the normal matcher to match
paths like `/:file.gz` or `/:lib.js.gz`

Previously this would have found no routes on paths like
`/node.js.gz`

Now `/:file.gz` matches `/node.js.gz` where "name" => "node.js"
And `/:lib.js.gz` matches `/node.js.gz` where "lib" => "node"

Which to me seems like the correct result in these cases.

fixes: #36

* chore: remove iter copied

* fix: revert iter enumerate

* chore: remove unless comments

* chore: check keeprunning first

---------

Co-authored-by: Fangdun Tsai <fundon@pindash.io>
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