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

Change NormalizePath to append trailing slash #1433

Merged
merged 4 commits into from
Apr 4, 2020
Merged

Change NormalizePath to append trailing slash #1433

merged 4 commits into from
Apr 4, 2020

Conversation

stevemk14ebr
Copy link
Contributor

@stevemk14ebr stevemk14ebr commented Mar 28, 2020

For paths like /blah/ requests like /////blah now work. Previously this would 404 as the trailing slash was missing and NormalizePath only replaces multiples and doesn't add the trailing slash when missing.

@robjtede
Copy link
Member

Can you add tests so we never see regressions.

@codecov-io
Copy link

codecov-io commented Mar 28, 2020

Codecov Report

Merging #1433 into master will increase coverage by 0.12%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1433      +/-   ##
==========================================
+ Coverage   47.91%   48.03%   +0.12%     
==========================================
  Files         137      137              
  Lines       12805    12826      +21     
==========================================
+ Hits         6135     6161      +26     
+ Misses       6670     6665       -5     
Impacted Files Coverage Δ
src/middleware/normalize.rs 95.65% <100.00%> (+1.90%) ⬆️
actix-http/src/header/map.rs 54.31% <0.00%> (-0.87%) ⬇️
actix-http/src/h1/dispatcher.rs 55.88% <0.00%> (+0.25%) ⬆️
actix-http/src/h1/codec.rs 70.83% <0.00%> (+1.38%) ⬆️
actix-http/src/h1/payload.rs 73.52% <0.00%> (+5.88%) ⬆️

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 fcb1dec...e9e0746. Read the comment docs.

Copy link
Member

@robjtede robjtede left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the contribution

otavio
otavio previously approved these changes Mar 30, 2020
@robjtede
Copy link
Member

Can you update changelog also?

@stevemk14ebr
Copy link
Contributor Author

done, merge it?

@robjtede robjtede requested a review from a team April 4, 2020 15:49
CHANGES.md Outdated Show resolved Hide resolved
Copy link
Member

@JohnTitor JohnTitor left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@JohnTitor JohnTitor merged commit aaff68b into actix:master Apr 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants