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.

Sign up for GitHub

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

feat: introduce base client that utilizes context #196

Merged
merged 3 commits into from
Nov 8, 2022
Merged

feat: introduce base client that utilizes context #196

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0bad2da
add *WithCtx methods to base client & request handler
natebrennand Oct 30, 2022
ca4bc43
add default client method & test
natebrennand Oct 30, 2022
fa94b6c
further document wrapperClient
natebrennand Nov 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 35 additions & 0 deletions client/base_client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package client

import (
"context"
"net/http"
"net/url"
"time"
Expand All @@ -12,3 +13,37 @@ type BaseClient interface {
SendRequest(method string, rawURL string, data url.Values,
headers map[string]interface{}) (*http.Response, error)
}

// BaseClientWithCtx is an extension of BaseClient with the ability to associate a contex with
// the request
type BaseClientWithCtx interface {
BaseClient
SendRequestWithCtx(ctx context.Context, method string, rawURL string, data url.Values,
headers map[string]interface{}) (*http.Response, error)
}

// wrapperClient wraps the lower level BaseClient to fulfill the BaseClientWithCtx interface. This
// allows the SDK to utilize the BaseClientWithCtx method throughout the codebase.
//
// All *WithCtx methods of a wrapped client will not actually use their context.Context argument.
type wrapperClient struct {
// embed the BaseClient so the functions remain accessible
BaseClient
}

// SendRequestWithCtx passes the request through to the underlying BaseClient. The context.Context
// argument is not utilized.
func (w wrapperClient) SendRequestWithCtx(ctx context.Context, method string, rawURL string, data url.Values,
headers map[string]interface{}) (*http.Response, error) {
return w.SendRequest(method, rawURL, data, headers)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be calling SendRequestWithCtx instead? Otherwise the context is swallowed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, there really should've been a comment over wrapperClient to make this more explicit.

wrapperClient is wrapping the older BaseClient implementation. So this method is the new interface passing though to the context-free client.
This enables the SDK to use the BaseClientWithCtx interface everywhere since we can transform the older BaseClient

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, understanding better now the intent. I'm thinking it would be a little easier to understand the implications by actually having the wrapperClient send through the provided context (which is what I was expecting in the initial review).

wrapping/upgrading is adding the ability to an existing client/handler to pass a context through (by adding an implementation for SendRequestWithCtx to it). So it no longer becomes a noop that swallows the context. That way if I wanted to take an existing integration and start adding context to API calls, all I'd need to do is switch to use the context-based version of the API method (no initialization changes needed to the client/service/handler).

Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So there are a few different ways for folks to instantiate a twiliogo client, in all but one the *WithCtx methods will operate as expected. There's just one scenario where it will be misleading to the integration developer, but I think it's the most sophisticated setup so there's the highest likelihood they will identify this.

Integrations where the client will "just work" with *WithCtx methods:

The only scenario where a context may be provided but not used is when the integration is using client.RequestHandler and has provided a custom BaseClient. If this is the case, there's no way for the library to pass the context in unless we introduce breaking changes, since the specified client by the integration (which is opaque to the SDK) has no method available to ingest a context.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, now I I understand. Thanks for bearing with me.

}

// wrapBaseClientWithNoopCtx "upgrades" a BaseClient to BaseClientWithCtx so that requests can be
// send with a request context.
func wrapBaseClientWithNoopCtx(c BaseClient) BaseClientWithCtx {
childish-sambino marked this conversation as resolved.
Show resolved Hide resolved
// the default library client has SendRequestWithCtx, use it if available.
if typedClient, ok := c.(BaseClientWithCtx); ok {
return typedClient
}
return wrapperClient{BaseClient: c}
}
11 changes: 9 additions & 2 deletions client/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
package client

import (
"context"
"encoding/json"
"fmt"
"net/http"
Expand Down Expand Up @@ -44,7 +45,7 @@ func defaultHTTPClient() *http.Client {
}
}

func (c *Client) basicAuth() (string, string) {
func (c *Client) basicAuth() (username, password string) {
return c.Credentials.Username, c.Credentials.Password
}

Expand Down Expand Up @@ -89,6 +90,12 @@ func (c *Client) doWithErr(req *http.Request) (*http.Response, error) {

// SendRequest verifies, constructs, and authorizes an HTTP request.
func (c *Client) SendRequest(method string, rawURL string, data url.Values,
headers map[string]interface{}) (*http.Response, error) {
return c.SendRequestWithCtx(context.TODO(), method, rawURL, data, headers)
}

// SendRequestWithCtx verifies, constructs, and authorizes an HTTP request.
func (c *Client) SendRequestWithCtx(ctx context.Context, method string, rawURL string, data url.Values,
headers map[string]interface{}) (*http.Response, error) {
u, err := url.Parse(rawURL)
if err != nil {
Expand All @@ -112,7 +119,7 @@ func (c *Client) SendRequest(method string, rawURL string, data url.Values,
valueReader = strings.NewReader(data.Encode())
}

req, err := http.NewRequest(method, u.String(), valueReader)
req, err := http.NewRequestWithContext(ctx, method, u.String(), valueReader)
if err != nil {
return nil, err
}
Expand Down
28 changes: 28 additions & 0 deletions client/client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package client_test

import (
"context"
"encoding/json"
"io"
"net/http"
Expand Down Expand Up @@ -210,6 +211,33 @@ func TestClient_SetTimeoutTimesOut(t *testing.T) {
assert.Error(t, err)
}

func TestClient_SetTimeoutTimesOutViaContext(t *testing.T) {
handlerDelay := 100 * time.Microsecond
clientTimeout := 10 * time.Microsecond
assert.True(t, clientTimeout < handlerDelay)

timeoutServer := httptest.NewServer(http.HandlerFunc(
func(writer http.ResponseWriter, _ *http.Request) {
d := map[string]interface{}{
"response": "ok",
}
time.Sleep(100 * time.Microsecond)
encoder := json.NewEncoder(writer)
err := encoder.Encode(&d)
if err != nil {
t.Error(err)
}
writer.WriteHeader(http.StatusOK)
}))
defer timeoutServer.Close()

c := NewClient("user", "pass")
ctx, cancel := context.WithTimeout(context.TODO(), 10*time.Microsecond)
defer cancel()
_, err := c.SendRequestWithCtx(ctx, "GET", timeoutServer.URL, nil, nil) //nolint:bodyclose
assert.Error(t, err)
}

func TestClient_SetTimeoutSucceeds(t *testing.T) {
timeoutServer := httptest.NewServer(http.HandlerFunc(
func(writer http.ResponseWriter, request *http.Request) {
Expand Down
10 changes: 10 additions & 0 deletions client/page_util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package client

import (
"context"
"encoding/json"
"fmt"
"strings"
Expand Down Expand Up @@ -34,6 +35,15 @@ func GetNext(baseUrl string, response interface{}, getNextPage func(nextPageUri
return getNextPage(nextPageUrl)
}

func GetNextWithCtx(ctx context.Context, baseUrl string, response interface{}, getNextPage func(ctx context.Context, nextPageUri string) (interface{}, error)) (interface{}, error) {
nextPageUrl, err := getNextPageUrl(baseUrl, response)
if err != nil {
return nil, err
}

return getNextPage(ctx, nextPageUrl)
}

func toMap(s interface{}) (map[string]interface{}, error) {
var payload map[string]interface{}
data, err := json.Marshal(s)
Expand Down
118 changes: 88 additions & 30 deletions client/request_handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,38 +2,14 @@
package client

import (
"context"
"net/http"
"net/url"
"os"
"strings"
)

type RequestHandler struct {
Client BaseClient
Edge string
Region string
}

func NewRequestHandler(client BaseClient) *RequestHandler {
return &RequestHandler{
Client: client,
Edge: os.Getenv("TWILIO_EDGE"),
Region: os.Getenv("TWILIO_REGION"),
}
}

func (c *RequestHandler) sendRequest(method string, rawURL string, data url.Values,
headers map[string]interface{}) (*http.Response, error) {
parsedURL, err := c.BuildUrl(rawURL)
if err != nil {
return nil, err
}

return c.Client.SendRequest(method, parsedURL, data, headers)
}

// BuildUrl builds the target host string taking into account region and edge configurations.
func (c *RequestHandler) BuildUrl(rawURL string) (string, error) {
func buildUrlInternal(overrideEdge, overrideRegion, rawURL string) (string, error) {
u, err := url.Parse(rawURL)
if err != nil {
return "", err
Expand Down Expand Up @@ -63,12 +39,12 @@ func (c *RequestHandler) BuildUrl(rawURL string) (string, error) {
region = pieces[2]
}

if c.Edge != "" {
edge = c.Edge
if overrideEdge != "" {
edge = overrideEdge
}

if c.Region != "" {
region = c.Region
if overrideRegion != "" {
region = overrideRegion
} else if region == "" && edge != "" {
region = "us1"
}
Expand All @@ -83,14 +59,96 @@ func (c *RequestHandler) BuildUrl(rawURL string) (string, error) {
return u.String(), nil
}

type RequestHandler struct {
Client BaseClient
Edge string
Region string
}

func NewRequestHandler(client BaseClient) *RequestHandler {
return &RequestHandler{
Client: client,
Edge: os.Getenv("TWILIO_EDGE"),
Region: os.Getenv("TWILIO_REGION"),
}
}

func (c *RequestHandler) sendRequest(method string, rawURL string, data url.Values,
headers map[string]interface{}) (*http.Response, error) {
parsedURL, err := c.BuildUrl(rawURL)
if err != nil {
return nil, err
}

return c.Client.SendRequest(method, parsedURL, data, headers)
}

// BuildUrl builds the target host string taking into account region and edge configurations.
func (c *RequestHandler) BuildUrl(rawURL string) (string, error) {
return buildUrlInternal(c.Edge, c.Region, rawURL)
}

// deprecated
func (c *RequestHandler) Post(path string, bodyData url.Values, headers map[string]interface{}) (*http.Response, error) {
return c.sendRequest(http.MethodPost, path, bodyData, headers)
}

// deprecated
func (c *RequestHandler) Get(path string, queryData url.Values, headers map[string]interface{}) (*http.Response, error) {
return c.sendRequest(http.MethodGet, path, queryData, headers)
}

// deprecated
func (c *RequestHandler) Delete(path string, nothing url.Values, headers map[string]interface{}) (*http.Response, error) {
return c.sendRequest(http.MethodDelete, path, nil, headers)
}

func UpgradeRequestHandler(h *RequestHandler) *RequestHandlerWithCtx {
return &RequestHandlerWithCtx{
// wrapped client will supply context.TODO() to all API calls
Client: wrapBaseClientWithNoopCtx(h.Client),
Edge: h.Edge,
Region: h.Region,
}
}

type RequestHandlerWithCtx struct {
Client BaseClientWithCtx
Edge string
Region string
}

func (c *RequestHandlerWithCtx) sendRequest(ctx context.Context, method string, rawURL string, data url.Values,
headers map[string]interface{}) (*http.Response, error) {
parsedURL, err := c.BuildUrl(rawURL)
if err != nil {
return nil, err
}

return c.Client.SendRequestWithCtx(ctx, method, parsedURL, data, headers)
}

func NewRequestHandlerWithCtx(client BaseClientWithCtx) *RequestHandlerWithCtx {
return &RequestHandlerWithCtx{
Client: client,
Edge: os.Getenv("TWILIO_EDGE"),
Region: os.Getenv("TWILIO_REGION"),
}
}

// BuildUrl builds the target host string taking into account region and edge configurations.
func (c *RequestHandlerWithCtx) BuildUrl(rawURL string) (string, error) {
return buildUrlInternal(c.Edge, c.Region, rawURL)
}

func (c *RequestHandlerWithCtx) Post(ctx context.Context, path string, bodyData url.Values, headers map[string]interface{}) (*http.Response, error) {
return c.sendRequest(ctx, http.MethodPost, path, bodyData, headers)
}

func (c *RequestHandlerWithCtx) Get(ctx context.Context, path string, queryData url.Values, headers map[string]interface{}) (*http.Response, error) {
return c.sendRequest(ctx, http.MethodGet, path, queryData, headers)
}

func (c *RequestHandlerWithCtx) Delete(ctx context.Context, path string, nothing url.Values, headers map[string]interface{}) (*http.Response, error) {
return c.sendRequest(ctx, http.MethodDelete, path, nil, headers)
}