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

fix(CompiledRouter): support URI templates with a trailing slash #1751

Merged
merged 11 commits into from
Aug 25, 2020

Conversation

vytas7
Copy link
Member

@vytas7 vytas7 commented Aug 2, 2020

Summary of Changes

Previously, the default CompiledRouter was erroneously stripping trailing slashes from URI templates.
This has been fixed so that it is now possible to add two different routes for a path with and without a trailing forward slash.

Related Issues

Closes #1544

Open Questions

I'm a bit divided what to do with add_route() in case RequestOptions.strip_url_path_trailing_slash is set to True, and the provided template ends with a trailing slash?

  • Do nothing special, effectively adding a non-functional route that is never hit (this may be a breaking change is someone had such a route).
  • Raise a ValueError asking to only add routes without a trailing slash (this may be a breaking change is someone had such a route).
  • Strip the trailing slash from the template before submitting it to the router's add_route().
  • Strip the trailing slash from the template before submitting it to the router's add_route(), and emit a deprecation warning.
  • Something else?

I have now proceeded with the first option, documenting this as a breaking change, and sprinkling some warnings throughout the docs. Thanks @CaselIT for input!

@codecov
Copy link

codecov bot commented Aug 2, 2020

Codecov Report

Merging #1751 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master     #1751   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           52        52           
  Lines         4295      4295           
  Branches       690       690           
=========================================
  Hits          4295      4295           
Impacted Files Coverage Δ
falcon/routing/compiled.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ff4d231...96df0f7. Read the comment docs.

@CaselIT
Copy link
Member

CaselIT commented Aug 2, 2020

Regarding the question, the problem I see is that we cannot assume that an user has set the options before adding the routes. Also the options may be dynamically updated.
Taking this into consideration, I don't think a warning should be raised, we should just document, maybe with a warning, the behaviour of the router wrt that request option.

@vytas7
Copy link
Member Author

vytas7 commented Aug 2, 2020

Ah, good point wrt the configuration order @CaselIT !
RequestOptions.strip_url_path_trailing_slash is by default False, so if the user hasn't set that to True yet, we obviously cannot do anything about it. But if the user has already overridden to True, I am inclined to emit a warning though, that might be helpful in debugging... 🤔

Edit: these routes with trailing slashes might actually be reachable with middleware path rewrite though... 🤔

@vytas7 vytas7 marked this pull request as ready for review August 2, 2020 16:40
@vytas7 vytas7 requested review from kgriffs and nZac August 2, 2020 16:40
@CaselIT
Copy link
Member

CaselIT commented Aug 2, 2020

The documentation looks good 👍

@vytas7 vytas7 requested a review from jmvrbanac August 7, 2020 19:22
CaselIT
CaselIT previously approved these changes Aug 7, 2020
Copy link
Member

@kgriffs kgriffs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is now possible to add two different routes for a path with and without a trailing forward slash

Is there a test for this specific use case? Otherwise LGTM. 👍

@vytas7
Copy link
Member Author

vytas7 commented Aug 25, 2020

@kgriffs good point; I have now added a dedicated test to better exercise this scenario.

@vytas7 vytas7 requested review from kgriffs and CaselIT August 25, 2020 18:53
Copy link
Member

@kgriffs kgriffs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

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 this pull request may close these issues.

Routes with trailing slash do not work under default API Options
4 participants