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

Support real regex rules for rewrite and proxy middleware #1767

Merged
merged 12 commits into from
Feb 8, 2021

Conversation

lammel
Copy link
Contributor

@lammel lammel commented Feb 8, 2021

Based on a real-world application using a workaround to ignore a part of the URL using .+? is not working anymore.
Using /prefix/*/*is also problematic due to the greedy nature of our regex conversion.

The behavior for handling regex (using QuoteMeta now) has been changed with PR #1630.

With this PR the rewrite middleware will be enhanced to also allow real regular expressions with capture groups.
The existing Rules can still be used and will be converted to the now public RegexRules. Both can be used together, as long as the resulting regex is not exactly the same as a regex generated from a rules section.

Example for usage:

e.Pre(RewriteWithConfig(RewriteConfig{
		Rules: map[string]string{
			"^/a/*":     "/v1/$1",
			"^/b/*/c/*": "/v2/$2/$1",
			"^/c/*/*":   "/v3/$2",
		},
		RegexRules: map[*regexp.Regexp]string{
			regexp.MustCompile("^/x/.+?/(.*)"):   "/v4/$1",
			regexp.MustCompile("^/y/(.+?)/(.*)"): "/v5/$2/$1",
		},
	}))

A separate PR for documentation will be created in https://github.com/labstack/echox

@codecov
Copy link

codecov bot commented Feb 8, 2021

Codecov Report

Merging #1767 (51df801) into master (7c8592a) will increase coverage by 0.01%.
The diff coverage is 83.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1767      +/-   ##
==========================================
+ Coverage   89.72%   89.74%   +0.01%     
==========================================
  Files          32       32              
  Lines        2667     2672       +5     
==========================================
+ Hits         2393     2398       +5     
  Misses        175      175              
  Partials       99       99              
Impacted Files Coverage Δ
middleware/rewrite.go 76.47% <66.66%> (+3.13%) ⬆️
middleware/middleware.go 100.00% <100.00%> (ø)
middleware/proxy.go 62.35% <100.00%> (+1.37%) ⬆️

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 7c8592a...51df801. Read the comment docs.

@lammel lammel requested review from aldas and pafuent February 8, 2021 00:25
@lammel lammel added this to In progress in Echo v4.2 Feb 8, 2021
@lammel lammel changed the title Feature/real regex rules Support real regex rules for rewrite and proxy middleware Feb 8, 2021
Copy link
Contributor

@aldas aldas left a comment

Choose a reason for hiding this comment

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

Other than that comment copy paste looks good to me.

p.s. this is my observation
And we really should use more table driven tests and even maybe refactor some old tests to tables when touching different parts. Those sequential page length tests could have side-effect and pass only because some earlier scenario/testcase already changed some internal state that next test case touches by reusing objects.

middleware/middleware.go Show resolved Hide resolved
middleware/proxy.go Show resolved Hide resolved
middleware/proxy_test.go Show resolved Hide resolved
@lammel
Copy link
Contributor Author

lammel commented Feb 8, 2021

@aldas Feel free to merge, if Goland is happy now too.
I'll update the release notes, once it's merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants