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

Configurable trailing slash behaviour for NormalizePath #1639

Merged
merged 5 commits into from Aug 19, 2020
Merged

Configurable trailing slash behaviour for NormalizePath #1639

merged 5 commits into from Aug 19, 2020

Conversation

ljoonal
Copy link
Contributor

@ljoonal ljoonal commented Aug 18, 2020

PR Type

Feature

PR Checklist

Check your PR fulfills the following:

  • Tests for the changes have been added / updated.
  • Documentation comments have been added / updated.
  • A changelog entry has been made for the appropriate packages.
  • Format code with the latest stable rustfmt

Overview

Currently NormalizePath always adds a trailing slash. This change would make it configurable to either follow that style, or to always trim the trailing slash instead.

Will be a breaking change to NormalizePath usage, since middleware::NormalizePath will need to be replaced by middleware::NormalizePath::default() or ::new(...) when creating the middleware.

Closes #1612

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.

Looks really good so far. Just left some initial thoughts and obviously the checks are failing from the doc comment in condition.rs.

src/middleware/normalize.rs Outdated Show resolved Hide resolved
src/middleware/normalize.rs Outdated Show resolved Hide resolved
src/middleware/normalize.rs Show resolved Hide resolved
src/middleware/normalize.rs Show resolved Hide resolved
src/middleware/normalize.rs Outdated Show resolved Hide resolved
src/middleware/normalize.rs Show resolved Hide resolved
ljoonal and others added 2 commits August 18, 2020 14:15
Made TrailingSlash be non_exhaustive with a default of Always, and changed NormalizePath back to derive. Started storing the TrailingSlash enum in NormalizePathNormalization instead of a boolean. Added one more test request without a trailing slash. And also tidied up a bit.
@ljoonal ljoonal marked this pull request as ready for review August 18, 2020 11:58
@robjtede robjtede requested review from a team August 18, 2020 12:37
@robjtede robjtede added C-improvement Category: an improvement to existing functionality B-semver-major breaking change requiring a major version bump labels Aug 18, 2020
@robjtede
Copy link
Member

Thanks for those updates. Add a CHANGELOG.md entry and a note to MIGRATION.md. Also check the job failure to see what needs updating.

@codecov-commenter
Copy link

Codecov Report

Merging #1639 into master will increase coverage by 0.01%.
The diff coverage is 89.28%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1639      +/-   ##
==========================================
+ Coverage   53.21%   53.22%   +0.01%     
==========================================
  Files         124      126       +2     
  Lines       11760    11796      +36     
==========================================
+ Hits         6258     6279      +21     
- Misses       5502     5517      +15     
Impacted Files Coverage Δ
src/middleware/condition.rs 86.11% <ø> (ø)
src/middleware/normalize.rs 93.93% <89.28%> (-2.07%) ⬇️
actix-http/src/header/common/mod.rs 7.69% <0.00%> (-2.31%) ⬇️
actix-http/src/header/common/accept.rs 0.00% <0.00%> (ø)
actix-http/src/header/common/expires.rs
actix-http/src/header/common/if_none_match.rs
actix-http/src/header/common/last_modified.rs 0.00% <0.00%> (ø)
actix-http/src/header/common/accept_language.rs 0.00% <0.00%> (ø)
actix-http/src/header/common/content_language.rs 0.00% <0.00%> (ø)
actix-http/src/header/common/allow.rs 0.00% <0.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 3892a95...653cfee. Read the comment docs.

@robjtede robjtede merged commit 75d86a6 into actix:master Aug 19, 2020
@robjtede
Copy link
Member

Great work, thanks 👍

@patrickelectric
Copy link

patrickelectric commented Aug 28, 2020

Hey, any plan to release it ? It's an awesome feature, I was able to have a /path|path/ to the same get method, and now with 2.0.0 appears to be not possible at all sadly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B-semver-major breaking change requiring a major version bump C-improvement Category: an improvement to existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

customization of NormalizePath behavior
4 participants