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

add regular expression matching to --output-http-rewrite-url #127

Merged
merged 3 commits into from
Oct 29, 2014

Conversation

jcw9930
Copy link
Contributor

@jcw9930 jcw9930 commented Oct 28, 2014

I extended --output-http-rewrite-url to also accept regular expressions for matching.

@buger
Copy link
Owner

buger commented Oct 28, 2014

Hi! Thank you for this change! But i think it will not be full without allowing to use matched parts in resulting url, for example:

/v1/user/([^\/])/ping:/v2/user/$1/ping

So you can use regexp with match groups on left side, and use them on right side.

@jcw9930
Copy link
Contributor Author

jcw9930 commented Oct 28, 2014

I have now switched to ReplaceAllString and tested with the modified url of
yours (notice the added '+' in the test). Note that when specifiying the
regex on commandline you need to escape $ and put the parameter in quotes.

2014-10-28 15:10 GMT+01:00 Leonid Bugaev notifications@github.com:

Hi! Thank you for this change! But i think it will not be full without
allowing to use matched parts in resulting url, for example:

/v1/user/([^\/])/ping:/v2/user/$1/ping

So you can use regexp with match groups on left side, and use them on
right side.


Reply to this email directly or view it on GitHub
#127 (comment).

t.Error("Should not error on /v1/user/([^\\/]+)/ping:/v2/user/$1/ping")
}

url = "/v1/user/joe/ping"
Copy link
Owner

Choose a reason for hiding this comment

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

Pls also add tests that explicitly checks if rewrites.Rewrite(url) == /v2/user/joe/ping

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pushed. does that work for you?

2014-10-28 18:24 GMT+01:00 Leonid Bugaev notifications@github.com:

In settings_url_map_test.go:

@@ -25,3 +24,24 @@ func TestUrlRewriteMap(t *testing.T) {
t.Error("Request url should not have been rewritten, was")
}
}
+
+func TestUrlRewriteMap_2(t *testing.T) {

  • var url string
  • rewrites := UrlRewriteMap{}
  • err := rewrites.Set("/v1/user/([^\/]+)/ping:/v2/user/$1/ping")
  • if err != nil {
  •   t.Error("Should not error on /v1/user/([^\/]+)/ping:/v2/user/$1/ping")
    
  • }
  • url = "/v1/user/joe/ping"

Pls also add tests that explicitly checks if rewrites.Rewrite(url) ==
/v2/user/joe/ping


Reply to this email directly or view it on GitHub
https://github.com/buger/gor/pull/127/files#r19487611.

@buger
Copy link
Owner

buger commented Oct 28, 2014

Thanks! Looks great! I will review it one more time tomorrow and will merge.

if f.src == path {
return f.target
if f.src.MatchString(path) {
return f.src.ReplaceAllString(path, f.target)
Copy link
Owner

Choose a reason for hiding this comment

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

Did not know about ReplaceAllString function, love go standard lib! :)

buger added a commit that referenced this pull request Oct 29, 2014
add regular expression matching to --output-http-rewrite-url
@buger buger merged commit b6977df into buger:master Oct 29, 2014
@jcw9930 jcw9930 deleted the url_regexp_rewrite branch October 29, 2014 10:35
@jcw9930
Copy link
Contributor Author

jcw9930 commented Nov 26, 2014

Please add this small patch, it fixes multiple occurrences in the same path

diff --git a/settings_url_map.go b/settings_url_map.go
index e8fa48a..5453610 100644
--- a/settings_url_map.go
+++ b/settings_url_map.go
@@ -34,7 +34,7 @@ func (r *UrlRewriteMap) Set(value string) error {
 func (r *UrlRewriteMap) Rewrite(path string) string {
        for _, f := range *r {
                if f.src.MatchString(path) {
-                       return f.src.ReplaceAllString(path, f.target)
+                       path = f.src.ReplaceAllString(path, f.target)
                }
        }
        return path

buger added a commit that referenced this pull request Nov 26, 2014
@buger
Copy link
Owner

buger commented Nov 26, 2014

Done eb7885c!

Thanks for pointing.

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.

3 participants