From 32f2af884ebbef2632de35098887bbdfe9dcd5cb Mon Sep 17 00:00:00 2001 From: Giannis Katsanos Date: Fri, 2 Feb 2024 11:53:10 +0200 Subject: [PATCH] fix: Params interface (#210) Changed the Queryable interface to Params. Instead of operating on a url.Values object, implementing types should return a url.Values object. When we set the params to the querystring, we'll use the new interface method. Added a custom implementation to join URL paths. The new implementation will preserve any query parameters in the URL. We can now replace url.JoinPath, which required us to use Go v1.19 and upwards. --- actortoken/actor_token.go | 3 +- clerk.go | 87 ++++++++++++++++++++++++++++++--------- clerk_test.go | 27 +++++++++--- domain/domain.go | 5 +-- 4 files changed, 92 insertions(+), 30 deletions(-) diff --git a/actortoken/actor_token.go b/actortoken/actor_token.go index acb44421..d4c8efe5 100644 --- a/actortoken/actor_token.go +++ b/actortoken/actor_token.go @@ -5,7 +5,6 @@ import ( "context" "encoding/json" "net/http" - "net/url" "github.com/clerk/clerk-sdk-go/v2" ) @@ -32,7 +31,7 @@ func Create(ctx context.Context, params *CreateParams) (*clerk.ActorToken, error // Revoke revokes a pending actor token. func Revoke(ctx context.Context, id string) (*clerk.ActorToken, error) { token := &clerk.ActorToken{} - path, err := url.JoinPath(path, id, "revoke") + path, err := clerk.JoinPath(path, id, "revoke") if err != nil { return token, err } diff --git a/clerk.go b/clerk.go index 87009a91..99c13e60 100644 --- a/clerk.go +++ b/clerk.go @@ -12,7 +12,9 @@ import ( "io" "net/http" "net/url" + "regexp" "strconv" + "strings" "sync" "time" ) @@ -53,11 +55,13 @@ func (r *APIResource) Read(response *APIResponse) { type APIParams struct { } -// Add can be used to set parameters to url.Values. The method -// is currently a no-op, but is defined so that all types that -// describe API operation parameters implement the Queryable +// ToQuery can be used to transform the params to querystring +// values. +// It is currently a no-op, but is defined so that all types +// that describe API operation parameters implement the Params // interface. -func (params *APIParams) Add(q url.Values) { +func (params *APIParams) ToQuery() url.Values { + return nil } // APIResponse describes responses coming from the Clerk API. @@ -98,11 +102,11 @@ func NewAPIResponse(resp *http.Response, body json.RawMessage) *APIResponse { type APIRequest struct { Method string Path string - Params Queryable + Params Params } // SetParams sets the APIRequest.Params. -func (req *APIRequest) SetParams(params Queryable) { +func (req *APIRequest) SetParams(params Params) { req.Params = params } @@ -127,10 +131,10 @@ type ResponseReader interface { Read(*APIResponse) } -// Queryable can add parameters to url.Values. +// Params can add parameters to url.Values. // Useful for constructing a request query string. -type Queryable interface { - Add(url.Values) +type Params interface { + ToQuery() url.Values } // BackendConfig is used to configure a new Clerk Backend. @@ -205,7 +209,7 @@ func (b *defaultBackend) Call(ctx context.Context, apiReq *APIRequest, setter Re } func (b *defaultBackend) newRequest(ctx context.Context, apiReq *APIRequest) (*http.Request, error) { - path, err := url.JoinPath(b.URL, clerkAPIVersion, apiReq.Path) + path, err := JoinPath(b.URL, clerkAPIVersion, apiReq.Path) if err != nil { return nil, err } @@ -222,7 +226,7 @@ func (b *defaultBackend) newRequest(ctx context.Context, apiReq *APIRequest) (*h return req, nil } -func (b *defaultBackend) do(req *http.Request, params Queryable, setter ResponseReader) error { +func (b *defaultBackend) do(req *http.Request, params Params, setter ResponseReader) error { err := setRequestBody(req, params) if err != nil { return err @@ -255,13 +259,11 @@ func (b *defaultBackend) do(req *http.Request, params Queryable, setter Response // Sets the params in either the request body, or the querystring // for GET requests. -func setRequestBody(req *http.Request, params Queryable) error { +func setRequestBody(req *http.Request, params Params) error { // GET requests don't have a body, but we will pass the params // in the query string. - if req.Method == http.MethodGet && params != nil { - q := req.URL.Query() - params.Add(q) - req.URL.RawQuery = q.Encode() + if req.Method == http.MethodGet { + setRequestQuery(req, params) return nil } @@ -277,7 +279,24 @@ func setRequestBody(req *http.Request, params Queryable) error { return nil } -// Error response handling +// Sets the params in the request querystring. Any existing values +// will be preserved, unless overriden. Keys in the params querystring +// always win. +func setRequestQuery(req *http.Request, params Params) { + if params == nil { + return + } + q := req.URL.Query() + paramsQuery := params.ToQuery() + for k, values := range paramsQuery { + for _, v := range values { + q.Set(k, v) + } + } + req.URL.RawQuery = q.Encode() +} + +// Error API response handling. func handleError(resp *APIResponse, body []byte) error { apiError := &APIErrorResponse{ HTTPStatusCode: resp.StatusCode, @@ -347,14 +366,44 @@ type ListParams struct { Offset *int64 `json:"offset,omitempty"` } -// Add sets list params to the passed in url.Values. -func (params ListParams) Add(q url.Values) { +// ToQuery returns url.Values with the ListParams values in the +// querystring. +func (params ListParams) ToQuery() url.Values { + q := url.Values{} if params.Limit != nil { q.Set("limit", strconv.FormatInt(*params.Limit, 10)) } if params.Offset != nil { q.Set("offset", strconv.FormatInt(*params.Offset, 10)) } + return q +} + +// Regular expression that matches multiple backslashes in a row. +var extraBackslashesRE = regexp.MustCompile("([^:])//+") + +// JoinPath returns a URL string with the provided path elements joined +// with the base path. +func JoinPath(base string, elem ...string) (string, error) { + // Concatenate all paths. + var sb strings.Builder + sb.WriteString(base) + for _, el := range elem { + sb.WriteString("/") + sb.WriteString(el) + } + // Trim leading and trailing backslashes, replace all occurrences of + // multiple backslashes in a row with one backslash, preserve the + // protocol's two backslashes. + // e.g. http://foo.com//bar/ will become http://foo.com/bar + res := extraBackslashesRE.ReplaceAllString(strings.Trim(sb.String(), "/"), "$1/") + + // Make sure we have a valid URL. + u, err := url.Parse(res) + if err != nil { + return "", err + } + return u.String(), nil } // String returns a pointer to the provided string value. diff --git a/clerk_test.go b/clerk_test.go index 75792f78..223e25db 100644 --- a/clerk_test.go +++ b/clerk_test.go @@ -124,13 +124,22 @@ type testResourceList struct { type testResourceListParams struct { APIParams ListParams - Name string `json:"name"` + Name string + Overriden string } -// We need to implement the Queryable interface. -func (params testResourceListParams) Add(q url.Values) { +// We need to implement the Params interface. +func (params testResourceListParams) ToQuery() url.Values { + q := url.Values{} q.Set("name", params.Name) - params.ListParams.Add(q) + q.Set("overriden", params.Overriden) + listQ := params.ListParams.ToQuery() + for k, values := range listQ { + for _, v := range values { + q.Add(k, v) + } + } + return q } func TestBackendCall_RequestHeaders(t *testing.T) { @@ -219,6 +228,7 @@ func TestBackendCall_SuccessfulResponse_PostRequest(t *testing.T) { func TestBackendCall_SuccessfulResponse_GetRequest(t *testing.T) { ctx := context.Background() name := "the-name" + overriden := "true" limit := 1 rawJSON := `{"data": [{"id":"res_123","object":"resource"}], "total_count": 1}` @@ -231,6 +241,10 @@ func TestBackendCall_SuccessfulResponse_GetRequest(t *testing.T) { // Optional query parameters are omitted. _, ok := q["offset"] assert.False(t, ok) + // Existing query parameters are preserved + assert.Equal(t, "still-here", q.Get("existing")) + // Existing query parameters can be overriden + assert.Equal(t, overriden, q.Get("overriden")) _, err := w.Write([]byte(rawJSON)) require.NoError(t, err) @@ -246,12 +260,13 @@ func TestBackendCall_SuccessfulResponse_GetRequest(t *testing.T) { // Simulate usage for an API operation on a testResourceList. // We need to initialize a request and use the Backend to send it. resource := &testResourceList{} - req := NewAPIRequest(http.MethodGet, "/resources") + req := NewAPIRequest(http.MethodGet, "/resources?existing=still-here&overriden=false") req.SetParams(&testResourceListParams{ ListParams: ListParams{ Limit: Int64(int64(limit)), }, - Name: name, + Name: name, + Overriden: overriden, }) err := GetBackend().Call(ctx, req, resource) require.NoError(t, err) diff --git a/domain/domain.go b/domain/domain.go index 717a7faf..5da7ca7d 100644 --- a/domain/domain.go +++ b/domain/domain.go @@ -4,7 +4,6 @@ package domain import ( "context" "net/http" - "net/url" "github.com/clerk/clerk-sdk-go/v2" ) @@ -36,7 +35,7 @@ type UpdateParams struct { // Update updates a domain's properties. func Update(ctx context.Context, id string, params *UpdateParams) (*clerk.Domain, error) { - path, err := url.JoinPath(path, id) + path, err := clerk.JoinPath(path, id) if err != nil { return nil, err } @@ -50,7 +49,7 @@ func Update(ctx context.Context, id string, params *UpdateParams) (*clerk.Domain // Delete removes a domain. func Delete(ctx context.Context, id string) (*clerk.DeletedResource, error) { - path, err := url.JoinPath(path, id) + path, err := clerk.JoinPath(path, id) if err != nil { return nil, err }