From 1f11e18c7a061668ae1c3ae916090803ff03147d Mon Sep 17 00:00:00 2001 From: Sergey Vilgelm Date: Sun, 1 Oct 2023 21:39:53 -0700 Subject: [PATCH] Benchmarks and Improvements for parseRequestURL function (#711) * Benchmarks for applying PathParams in parseRequestURL function ```shell % go test -benchmem -bench=. -run=^Benchmark goos: darwin goarch: amd64 pkg: github.com/go-resty/resty/v2 cpu: Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz Benchmark_parseRequestURL_PathParams-16 524658 2260 ns/op 448 B/op 9 allocs/op PASS ok github.com/go-resty/resty/v2 2.327s ``` * Benchmarks for applying QueryParams in parseRequestURL function ```shell % go test -benchmem -bench=. -run=^Benchmark goos: darwin goarch: amd64 pkg: github.com/go-resty/resty/v2 cpu: Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz Benchmark_parseRequestURL_QueryParams-16 865923 1371 ns/op 416 B/op 13 allocs/op PASS ok github.com/go-resty/resty/v2 2.491s ``` * improve the performance of applying the path parameters * Use the map to collect all replacements and use replace all path parameters using O(1) logic * Add additional unit tests to cover empty `{}` and not closed `{bar` path parameters ```shell % go test -benchmem -bench=. -run=^Benchmark goos: darwin goarch: amd64 pkg: github.com/go-resty/resty/v2 cpu: Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz Benchmark_parseRequestURL_PathParams-16 785971 1410 ns/op 320 B/op 6 allocs/op PASS ok github.com/go-resty/resty/v2 1.445s ``` * improve the performance of applying the query parameters * improve the loging by adding the query parameters from the request first, then adding the parameters from the client and skip if already exists * additional unit tests for the query parameters ```shell % go test -benchmem -bench=. -run=^Benchmark goos: darwin goarch: amd64 pkg: github.com/go-resty/resty/v2 cpu: Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz Benchmark_parseRequestURL_QueryParams-16 1000000 1158 ns/op 352 B/op 9 allocs/op PASS ok github.com/go-resty/resty/v2 2.473s ``` * using acquireBuffer reusing a buffer from the pool decreases the allocs and memory usage ```shell % go test -benchmem -bench=. -run=^Benchmark goos: darwin goarch: amd64 pkg: github.com/go-resty/resty/v2 cpu: Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz Benchmark_parseRequestURL_PathParams-16 753834 1367 ns/op 256 B/op 5 allocs/op Benchmark_parseRequestURL_QueryParams-16 1000000 1167 ns/op 352 B/op 9 allocs/op PASS ok github.com/go-resty/resty/v2 2.373s ``` * using reflect.DeepEqual to compare the expected and actual QueryParams * update r.QueryParam isntead of creating new variable * remove unneeded if --- middleware.go | 115 +++++++++++++++++++++++++++++-------------- middleware_test.go | 120 +++++++++++++++++++++++++++++++++++++++------ 2 files changed, 183 insertions(+), 52 deletions(-) diff --git a/middleware.go b/middleware.go index b614af4c..15cbc49a 100644 --- a/middleware.go +++ b/middleware.go @@ -27,27 +27,76 @@ const debugRequestLogKey = "__restyDebugRequestLog" //_______________________________________________________________________ func parseRequestURL(c *Client, r *Request) error { - // GitHub #103 Path Params - if len(r.PathParams) > 0 { + if l := len(c.PathParams) + len(c.RawPathParams) + len(r.PathParams) + len(r.RawPathParams); l > 0 { + params := make(map[string]string, l) + + // GitHub #103 Path Params for p, v := range r.PathParams { - r.URL = strings.Replace(r.URL, "{"+p+"}", url.PathEscape(v), -1) + params[p] = url.PathEscape(v) } - } - if len(c.PathParams) > 0 { for p, v := range c.PathParams { - r.URL = strings.Replace(r.URL, "{"+p+"}", url.PathEscape(v), -1) + if _, ok := params[p]; !ok { + params[p] = url.PathEscape(v) + } } - } - // GitHub #663 Raw Path Params - if len(r.RawPathParams) > 0 { + // GitHub #663 Raw Path Params for p, v := range r.RawPathParams { - r.URL = strings.Replace(r.URL, "{"+p+"}", v, -1) + if _, ok := params[p]; !ok { + params[p] = v + } } - } - if len(c.RawPathParams) > 0 { for p, v := range c.RawPathParams { - r.URL = strings.Replace(r.URL, "{"+p+"}", v, -1) + if _, ok := params[p]; !ok { + params[p] = v + } + } + + if len(params) > 0 { + var prev int + buf := acquireBuffer() + defer releaseBuffer(buf) + // search for the next or first opened curly bracket + for curr := strings.Index(r.URL, "{"); curr > prev; curr = prev + strings.Index(r.URL[prev:], "{") { + // write everything form the previous position up to the current + if curr > prev { + buf.WriteString(r.URL[prev:curr]) + } + // search for the closed curly bracket from current position + next := curr + strings.Index(r.URL[curr:], "}") + // if not found, then write the remainder and exit + if next < curr { + buf.WriteString(r.URL[curr:]) + prev = len(r.URL) + break + } + // special case for {}, without parameter's name + if next == curr+1 { + buf.WriteString("{}") + } else { + // check for the replacement + key := r.URL[curr+1 : next] + value, ok := params[key] + /// keep the original string if the replacement not found + if !ok { + value = r.URL[curr : next+1] + } + buf.WriteString(value) + } + + // set the previous position after the closed curly bracket + prev = next + 1 + if prev >= len(r.URL) { + break + } + } + if buf.Len() > 0 { + // write remainder + if prev < len(r.URL) { + buf.WriteString(r.URL[prev:]) + } + r.URL = buf.String() + } } } @@ -82,32 +131,26 @@ func parseRequestURL(c *Client, r *Request) error { } // Adding Query Param - query := make(url.Values) - for k, v := range c.QueryParam { - for _, iv := range v { - query.Add(k, iv) - } - } - - for k, v := range r.QueryParam { - // remove query param from client level by key - // since overrides happens for that key in the request - query.Del(k) + if len(c.QueryParam)+len(r.QueryParam) > 0 { + for k, v := range c.QueryParam { + // skip query parameter if it was set in request + if _, ok := r.QueryParam[k]; ok { + continue + } - for _, iv := range v { - query.Add(k, iv) + r.QueryParam[k] = v[:] } - } - // GitHub #123 Preserve query string order partially. - // Since not feasible in `SetQuery*` resty methods, because - // standard package `url.Encode(...)` sorts the query params - // alphabetically - if len(query) > 0 { - if IsStringEmpty(reqURL.RawQuery) { - reqURL.RawQuery = query.Encode() - } else { - reqURL.RawQuery = reqURL.RawQuery + "&" + query.Encode() + // GitHub #123 Preserve query string order partially. + // Since not feasible in `SetQuery*` resty methods, because + // standard package `url.Encode(...)` sorts the query params + // alphabetically + if len(r.QueryParam) > 0 { + if IsStringEmpty(reqURL.RawQuery) { + reqURL.RawQuery = r.QueryParam.Encode() + } else { + reqURL.RawQuery = reqURL.RawQuery + "&" + r.QueryParam.Encode() + } } } diff --git a/middleware_test.go b/middleware_test.go index 6ec91f96..f4e5c698 100644 --- a/middleware_test.go +++ b/middleware_test.go @@ -105,6 +105,46 @@ func Test_parseRequestURL(t *testing.T) { }, expectedURL: "https://example.com/4%2F5/6/7", }, + { + name: "empty path parameter in URL", + init: func(c *Client, r *Request) { + r.SetPathParams(map[string]string{ + "bar": "4", + }) + r.URL = "https://example.com/{}/{bar}" + }, + expectedURL: "https://example.com/%7B%7D/4", + }, + { + name: "not closed path parameter in URL", + init: func(c *Client, r *Request) { + r.SetPathParams(map[string]string{ + "foo": "4", + }) + r.URL = "https://example.com/{foo}/{bar/1" + }, + expectedURL: "https://example.com/4/%7Bbar/1", + }, + { + name: "extra path parameter in URL", + init: func(c *Client, r *Request) { + r.SetPathParams(map[string]string{ + "foo": "1", + }) + r.URL = "https://example.com/{foo}/{bar}" + }, + expectedURL: "https://example.com/1/%7Bbar%7D", + }, + { + name: " path parameter with remainder", + init: func(c *Client, r *Request) { + r.SetPathParams(map[string]string{ + "foo": "1", + }) + r.URL = "https://example.com/{foo}/2" + }, + expectedURL: "https://example.com/1/2", + }, { name: "using BaseURL with absolute URL in request", init: func(c *Client, r *Request) { @@ -189,13 +229,32 @@ func Test_parseRequestURL(t *testing.T) { "foo": "1", // ignored, because of the "foo" parameter in request "bar": "2", }) - c.SetQueryParams(map[string]string{ + r.SetQueryParams(map[string]string{ "foo": "3", }) r.URL = "https://example.com/" }, expectedURL: "https://example.com/?foo=3&bar=2", }, + { + name: "adding query parameters by request to URL with existent", + init: func(c *Client, r *Request) { + r.SetQueryParams(map[string]string{ + "bar": "2", + }) + r.URL = "https://example.com/?foo=1" + }, + expectedURL: "https://example.com/?foo=1&bar=2", + }, + { + name: "adding query parameters by request with multiple values", + init: func(c *Client, r *Request) { + r.QueryParam.Add("foo", "1") + r.QueryParam.Add("foo", "2") + r.URL = "https://example.com/" + }, + expectedURL: "https://example.com/?foo=1&foo=2", + }, } { t.Run(tt.name, func(t *testing.T) { c := New() @@ -216,27 +275,55 @@ func Test_parseRequestURL(t *testing.T) { if expectedURL.String() != actualURL.String() { t.Errorf("r.URL = %q does not match expected %q", r.URL, tt.expectedURL) } - if len(expectedQuery) != len(actualQuery) { + if !reflect.DeepEqual(expectedQuery, actualQuery) { t.Errorf("r.URL = %q does not match expected %q", r.URL, tt.expectedURL) } - for name, expected := range expectedQuery { - actual, ok := actualQuery[name] - if !ok { - t.Errorf("r.URL = %q does not match expected %q", r.URL, tt.expectedURL) - } - if len(expected) != len(actual) { - t.Errorf("r.URL = %q does not match expected %q", r.URL, tt.expectedURL) - } - for i, v := range expected { - if v != actual[i] { - t.Errorf("r.URL = %q does not match expected %q", r.URL, tt.expectedURL) - } - } - } }) } } +func Benchmark_parseRequestURL_PathParams(b *testing.B) { + c := New().SetPathParams(map[string]string{ + "foo": "1", + "bar": "2", + }).SetRawPathParams(map[string]string{ + "foo": "3", + "xyz": "4", + }) + r := c.R().SetPathParams(map[string]string{ + "foo": "5", + "qwe": "6", + }).SetRawPathParams(map[string]string{ + "foo": "7", + "asd": "8", + }) + b.ResetTimer() + for i := 0; i < b.N; i++ { + r.URL = "https://example.com/{foo}/{bar}/{xyz}/{qwe}/{asd}" + if err := parseRequestURL(c, r); err != nil { + b.Errorf("parseRequestURL() error = %v", err) + } + } +} + +func Benchmark_parseRequestURL_QueryParams(b *testing.B) { + c := New().SetQueryParams(map[string]string{ + "foo": "1", + "bar": "2", + }) + r := c.R().SetQueryParams(map[string]string{ + "foo": "5", + "qwe": "6", + }) + b.ResetTimer() + for i := 0; i < b.N; i++ { + r.URL = "https://example.com/" + if err := parseRequestURL(c, r); err != nil { + b.Errorf("parseRequestURL() error = %v", err) + } + } +} + func Test_parseRequestHeader(t *testing.T) { for _, tt := range []struct { name string @@ -865,6 +952,7 @@ func Benchmark_parseRequestBody_reader_with_SetContentLength(b *testing.B) { } } } + func Benchmark_parseRequestBody_reader_without_SetContentLength(b *testing.B) { c := New() r := c.R()