Skip to content

Add trailing_slash http route option #173

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

eugenepaniot
Copy link
Contributor

Add trailing_slash http route option.

trailing_slash defines the trailing slash behavior for the route and as a result req:stash behaviour . The initial value is true.

  1. When false, if the route path is /trailing_slash_false/a/b/*c, accessing /trailing_slash_false/a/b/c/ will result req:stash("c") as c/
  2. When true, the same path with req:stash("c") call will result req:stash("c") as c

@eugenepaniot
Copy link
Contributor Author

# .rocks/bin/luatest -v test/
...
    integration.http_server_requests.test_trailing_slash_f_get ... (0.004s) Ok
    integration.http_server_requests.test_trailing_slash_f_get_with_slash_at_begging ... (0.004s) Ok
    integration.http_server_requests.test_trailing_slash_f_get_with_slash_at_begging_and_end ... (0.005s) Ok
    integration.http_server_requests.test_trailing_slash_f_get_with_slash ... (0.004s) Ok
    integration.http_server_requests.test_trailing_slash_f_get_with_encoded_slash_begging ... (0.006s) Ok
    integration.http_server_requests.test_trailing_slash_f_get_with_encoded_slash_begging_and_end ... (0.006s) Ok
    integration.http_server_requests.test_trailing_slash_f_get_html ... (0.006s) Ok
    integration.http_server_requests.test_trailing_slash_f_get_long ... (0.004s) Ok
    integration.http_server_requests.test_trailing_slash_f_get_long_with_slash_end ... (0.002s) Ok
    integration.http_server_requests.test_trailing_slash_t_get_with_slash_at_begging_and_end ... (0.001s) Ok
    integration.http_server_requests.test_trailing_slash_t_get_with_encoded_slash_begging_and_end ... (0.001s) Ok
    integration.http_server_url_for.test_server_url_for ... (0.003s) Ok
    integration.http_server_url_match.test_server_url_match ... (0.002s) Ok
    unit.http_custom_socket.test_custom_socket ... (0.000s) Ok
    unit.http_parse_request.test_parse_request ... (0.001s) Ok
    unit.http_setcookie.test_values_escaping ... (0.002s) Ok
    unit.http_setcookie.test_values_raw ... (0.002s) Ok
    unit.http_setcookie.test_path_escaping ... (0.002s) Ok
    unit.http_setcookie.test_path_raw ... (0.001s) Ok
    unit.http_setcookie.test_set_header ... (0.001s) Ok
    unit.http_split_uri.test_split_uri ... (0.002s) Ok
    unit.http_template.test_template_1 ... (0.000s) Ok
    unit.http_template.test_template_2 ... (0.001s) Ok
    unit.http_template.test_broken_template ... (0.001s) Ok
    unit.http_template.test_rendered_template_truncated_gh_18 ... (0.004s) Ok
    unit.http_template.test_incorrect_arguments_escaping_leads_to_segfault_gh_51 ... (0.001s) Ok
=========================================================
Ran 67 tests in 0.760 seconds, 67 succeeded, 0 failed

@0x501D 0x501D force-pushed the fix-http-wrong-any-match branch from 0f5c1f5 to a4908c4 Compare November 16, 2022 08:25
@Totktonada
Copy link
Member

How Mojolicious behaves in the case?

@Totktonada
Copy link
Member

Can we stick with Mojo's behavior (without an option) and don't break existing users? If so, I would prefer to don't add the option to don't bloat the API.

@eugenepaniot
Copy link
Contributor Author

eugenepaniot commented Nov 18, 2022

@Totktonada Mojo has an option - trailing_slash - https://docs.mojolicious.org/Mojo/Path#trailing_slash
This PR suggests the same option with the same behaviour. Default value is "true" to not to affect current behaviour.

@Totktonada
Copy link
Member

Mojo has trailing_slash as a Path property (whether the path has it) and as a normalization method (add it or remove). It has no route option. Regarding router Mojo's documentation states the following:

A trailing slash in the path is always optional.

/user/admin/23/ -> /user/:role/:id -> {role => 'admin', id => 23}

I doubt that I understand the motivation of the change good enough.

Can we start from a description of your particular case and your particular problem?

@eugenepaniot
Copy link
Contributor Author

@Totktonada sure, let me try to describe the problem:
we have a key-value Tarantool Application that expects to get key from url path. As a GET parameter key like

/path/to/app/%2Fsome-key%2F

http server decodes %2Fsome-key%2F as /some-key/, and after removes last slash.

In fact, it modifies /path/to/app/%2Fsome-key%2F to /path/to/app/%2Fsome-key. What we don't expect.

In case of:

/user/admin/23/ -> /user/:role/:id -> {role => 'admin', id => 23}

I agree, its correct. But not for encoded values.

@Totktonada
Copy link
Member

http server decodes %2Fsome-key%2F as /some-key/, and after removes last slash.

In fact, it modifies /path/to/app/%2Fsome-key%2F to /path/to/app/%2Fsome-key. What we don't expect.

Wow. It is just a bug and should be fixed without any options.

@0x501D
Copy link
Member

0x501D commented Dec 8, 2023

Please rebase with master branch.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants