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

bugfix proxy and rewrite, updated test with actual call settings #1630

Merged
merged 1 commit into from
Nov 5, 2020
Merged

bugfix proxy and rewrite, updated test with actual call settings #1630

merged 1 commit into from
Nov 5, 2020

Conversation

arun0009
Copy link
Contributor

@arun0009 arun0009 commented Sep 3, 2020

Issue: Proxy and Rewrite Middleware don't set URL appropriately in tests and this causes different behavior when running tests vs when proxy and rewrite middleware are used in real server mode.

We should use url Parse method instead.

Check how path, raw path is set when using URL.Parse in this playground example. This is how url path and raw path is set when running echo server and invoking API endpoints.

After updating tests, was able to find a bug in proxy and rewrite and was able to fix it (and made sure it passes in real scenario calls as well).

Also, refactored some common code that's shared between proxy with rewrite rules and rewrite middleware rules. This also fixed one other bug e.g. the regex rules for rewrite is different from proxy regex rules when they should be same (rewrite regex rules works as expected).

After refactoring and using common function the below test works in proxy and rewrite (in current master code, proxy test for below scenario would fail):

u, _:= url.Parse("/api/new users")
req.URL = u
rec = httptest.NewRecorder()
e.ServeHTTP(rec, req)
assert.Equal(t, "/new%20users", req.URL.EscapedPath()) 

@codecov
Copy link

codecov bot commented Sep 3, 2020

Codecov Report

Merging #1630 into master will increase coverage by 0.11%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1630      +/-   ##
==========================================
+ Coverage   85.28%   85.40%   +0.11%     
==========================================
  Files          28       28              
  Lines        2216     2206      -10     
==========================================
- Hits         1890     1884       -6     
+ Misses        212      208       -4     
  Partials      114      114              
Impacted Files Coverage Δ
middleware/middleware.go 100.00% <100.00%> (ø)
middleware/proxy.go 63.82% <100.00%> (-2.51%) ⬇️
middleware/rewrite.go 76.47% <100.00%> (+4.05%) ⬆️

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 151ed6b...f6dfcbe. Read the comment docs.

@lammel
Copy link
Contributor

lammel commented Sep 3, 2020

I could not check on it yet. Will look into next week.

@arun0009
Copy link
Contributor Author

arun0009 commented Sep 9, 2020

@lammel - I updated the issue description and fixed code accordingly. Let me know if you have any questions.

@lammel lammel self-assigned this Sep 10, 2020
@arun0009
Copy link
Contributor Author

@lammel - resolved merge conflicts.

@arun0009
Copy link
Contributor Author

can someone review this PR? @lammel ?

@lammel
Copy link
Contributor

lammel commented Oct 30, 2020

I'm running out of time currently. Will try to look into it in the next days.

@lammel
Copy link
Contributor

lammel commented Nov 5, 2020

PR looks fine. No overall performance regression seen (no dedicated benchmark exists yet).
Thanks, merged.

@arun0009
Copy link
Contributor Author

arun0009 commented Nov 5, 2020

@lammel - Thanks! Did you forget to click merge? :)

@lammel lammel merged commit ceffc10 into labstack:master Nov 5, 2020
@lammel
Copy link
Contributor

lammel commented Nov 5, 2020

@lammel - Thanks! Did you forget to click merge? :)

Could be... worked better on the second attempt ;-)

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.

2 participants