Add context support, ErrUnauthorized and Doer interface for http.Client #4

anpryl · 2016-09-19T14:05:12Z

Extend API for context usage.
Make Errors an error instance.

coveralls · 2016-09-19T14:12:44Z

Coverage decreased (-7.6%) to 59.603% when pulling 0cc56ad on anpryl:master into 73e6538 on tambet:master.

dmitshur

Thank you for contributing.

Aside from the fmt.Println(resp.StatusCode, string(b)) change, which looks unintended, all other code changes look good.

However, I see two main issues/potential improvements for this PR.

All code changes are in a single commit. This is harder to review, and not a good practice. Would you mind splitting each logical change like "add context support, "add Doer interface", "add http post form", etc., into a separate commit?
No documentation. I know a lot of the code here isn't perfect, but let's try to make things better. Exported symbols need to be documented. This is a good practice and consistent with Go style. Try running golint on your changes.

Aside from those 2 things, these changes look great and make sense to me. I'd be happy to accept them. I'm guessing @tambet would like them too.

Are you willing to address the two suggestions I've made above?

dmitshur · 2016-09-20T06:21:26Z

asana/asana.go

+	fmt.Println("---------------------------")
+	fmt.Println(resp.StatusCode, string(b))
+	fmt.Println("---------------------------")
+	err = json.NewDecoder(bytes.NewBuffer(b)).Decode(res)


Did you forget to remove this change after testing?

I'm so sorry for this debugging printing
I will add documentation and split commits at this weekend.
Thank you for your feedback!

…asana

coveralls · 2016-09-20T18:57:32Z

Coverage decreased (-0.09%) to 67.105% when pulling 98380c1 on anpryl:master into 73e6538 on tambet:master.

coveralls · 2016-09-20T18:57:43Z

Coverage decreased (-0.09%) to 67.105% when pulling 98380c1 on anpryl:master into 73e6538 on tambet:master.

anpryl · 2016-09-20T19:00:46Z

Tried to split commits as much as possible, add documentation, also add tests this time.
If I could make it better, please, don't hesitate to comment it.
Thank you!

coveralls · 2016-09-20T19:17:27Z

Coverage decreased (-0.09%) to 67.105% when pulling 12d49ff on anpryl:master into 73e6538 on tambet:master.

dmitshur

This looks great. I've left minor comments and questions in my review. Thanks!

dmitshur · 2016-09-23T07:52:22Z

asana/asana.go

+
+	//DoerFunc implements Doer interface.
+	//Allow to transform any appropriate function "f" to Doer instance: DoerFunc(f).
+	DoerFunc func(req *http.Request) (resp *http.Response, err error)


Here, and elsewhere, the comments don't have a space after the two slashes. There should be a single space there.

See here for rationale.

Fix this, thank you for explanations.

dmitshur · 2016-09-23T07:52:59Z

asana/asana.go

+	//Use it as point of setting Auth header or custom status code error handling.
+	Doer interface {
+		Do(req *http.Request) (*http.Response, error)
+	}


I have a guess, but could you please elaborate on why you decided to make the Doer interface instead of sticking with an *http.Client?

It allows me to use single instance of my configured http.Client and set different Auth headers on each request.

I have configured http.Client and it is property of my own Client struct.

type Client struct { httpClient *http.Client }

My service receiving auth token as an argument to request and with Doer interface it is easy to add Auth header with token to request like this:

func (c *Client) withToken(token string) *asana.Client { f := func(req *http.Request) (*http.Response, error) { req.Header.Set("Authorization", "Bearer "+token) return c.httpClient.Do(req) } return asana.NewClient(asana.DoerFunc(f)) } // Usage example func (c *Client) Issue(ctx context.Context, token string, issueID int64) (*Issue, error) { t, err := c.withToken(token).GetTask(ctx, issueID, issueFieldsFilter) if err != nil { return nil, err } issue := toIssue(t) return &issue, nil }

Thanks for the explanation.

This sounds reasonable, but I also slightly worry this might not be the most optimal solution for the problem you're trying to solve. Having Doer be an interface lets you pass in a middleware-augmented *http.Client of sorts, but you still have to create a new asana.NewClient for each one...

It's also interesting that go-github, a very popular library, manages to get by without Doer interface. /cc @willnorris, do you know how one would do that with go-github?

I'm not opposed to this going in @anpryl, I just wanted to say that it seems a little more questionable to me, and perhaps we can find a better way to effectively be able to augment certain *http.Requests. But maybe not, and this is fine.

Is this client fully taking on responsibility for authentication, or only for certain calls, like in your c.Issue method? go-github's Client knows nothing about authentication at all. It is always punted to the underlying http.Client, which typically comes from golang.org/x/oauth2, or the basic auth client that go-github does provide (but is otherwise completely separated from the Client struct).

I think the best analogy to this in go-github would be the custom accept headers we set on various API calls. That happens by first building the http.Request, then adding whatever custom headers we want before passing it off to client.Do. As long as the underlying http.Client leaves those headers intact, then you're fine. If your underlying http.Client is clobbering your auth header because it thinks it should be responsible for auth, then you do have a problem.

This is exactly why go-github's Client struct has its own Do and NewRequest methods, so that you can modify the request before calling Do. Not separating those, as go-asana does with its single Request method, results in these weird workarounds.

Thanks for the explanation @willnorris, that makes sense.

I agree that the Do and NewRequest separation makes more sense and is cleaner/safer, but I'd want to think more and understand it better (outside of this PR). I think we should consider changing this library in that direction, rather than the Doer, which I think is a lot harder for people to use (at all, and also correctly).

What are your thoughts on this @anpryl?

We still need Doer to reuse http.Client with different tokens.
But this change will decrease cost of NewClient function call.
We could also include withToken function into package and make it public.

Well, okay. It's been a while and I have no better ideas than Doer interface, so let's go with that for now. If really good ideas come in the future, we can probably still change things.

Can I move url.URL to global variable and initialize it in init() funciton?

It's better to split up orthogonal changes across multiple PRs and keep each one smaller.

Mind making it a separate followup PR? This one is already huge. :)

Well, you are right, I will prepare new PR after merge.

dmitshur · 2016-09-23T07:55:10Z

asana/asana.go

 	}

 	Error struct {
 		Phrase  string `json:"phrase,omitempty"`
 		Message string `json:"message,omitempty"`
 	}
+
+	Errors []Error


Are there any restrictions on Errors? Can it contain 0 elements? 1 element?

dmitshur · 2016-09-23T07:55:49Z

asana/asana.go

@@ -272,7 +283,7 @@ func (c *Client) request(method string, path string, data interface{}, opt *Filt
 	res := &Response{Data: v}
 	err = json.NewDecoder(resp.Body).Decode(res)
 	if len(res.Errors) > 0 {
-		return res.Errors[0]
+		return res.Errors


According to this, at least 1 element. I think it's worth documenting that for the Errors type, so users will know to always expect either at least one error there.

You are correct, add godoc for Errors type

dmitshur · 2016-09-23T07:57:30Z

asana/asana_test.go

@@ -14,6 +15,7 @@ var (
 	client *Client
 	mux    *http.ServeMux
 	server *httptest.Server
+	ctx    = context.Background()


I know it's a test, but still, this isn't saving much typing. Instead of this variable here, just use context.Background() directly at the call sites. E.g.:

workspaces, err := client.ListWorkspaces(context.Background()) ... users, err := client.ListUsers(context.Background(), nil) ...

I prefer to write 3 letters instead of context.Background() in tests, but it's not a big deal. I remove this global variable and put context.Background().

dmitshur · 2016-09-23T08:00:55Z

asana/asana.go

 }

 // request makes a request to Asana API, using method, at path, sending data with opt filter.
 // The response is populated into v, and any error is returned.
-func (c *Client) request(ctx context.Context, method string, path string, data interface{}, opt *Filter, v interface{}) error {
+func (c *Client) request(ctx context.Context, method string, path string, data interface{}, form url.Values, opt *Filter, v interface{}) error {


Now request has 2 ways of specifying the data. Either by setting data, or by setting form. I think it might be helpful to document that at most one can be used at the same time (or none), and that data takes higher precedence.

What do you think?

Improve request documentation

coveralls · 2016-09-25T18:01:58Z

Coverage decreased (-0.09%) to 67.105% when pulling 67b650e on anpryl:master into 73e6538 on tambet:master.

coveralls · 2016-09-25T18:13:27Z

Coverage decreased (-0.09%) to 67.105% when pulling 84ac3b0 on anpryl:master into 73e6538 on tambet:master.

dmitshur

Thanks for addressing all my feedback @anpryl.

This LGTM.

@tambet, any comments on this? If this looks good to you, or if you don't have any objections, I'll merge this in a few days.

dmitshur · 2016-09-30T08:32:11Z

asana/asana.go

+	//Use it as point of setting Auth header or custom status code error handling.
+	Doer interface {
+		Do(req *http.Request) (*http.Response, error)
+	}


Thanks for the explanation.

This sounds reasonable, but I also slightly worry this might not be the most optimal solution for the problem you're trying to solve. Having Doer be an interface lets you pass in a middleware-augmented *http.Client of sorts, but you still have to create a new asana.NewClient for each one...

It's also interesting that go-github, a very popular library, manages to get by without Doer interface. /cc @willnorris, do you know how one would do that with go-github?

I'm not opposed to this going in @anpryl, I just wanted to say that it seems a little more questionable to me, and perhaps we can find a better way to effectively be able to augment certain *http.Requests. But maybe not, and this is fine.

This is more consistent with the rest of the code.

coveralls · 2016-09-30T08:47:17Z

Coverage decreased (-0.09%) to 67.105% when pulling 84152d5 on anpryl:master into 73e6538 on tambet:master.

dmitshur

LGTM. See #4 (comment).

I'll let this sit for a day and merge tomorrow if no further comments. /cc @tambet

Thanks for your work @anpryl!

dmitshur · 2016-10-15T03:06:34Z

Merged. This is a breaking API change, but it includes some necessary API improvements such as adding support for passing a context. Updating should be easy, and those who don't want to will have this library vendored anyway.

Thanks for your awesome contribution @anpryl!

See tambet/go-asana#4.

dmitshur reviewed Sep 20, 2016

View reviewed changes

Prylutskyi Anatolii added 6 commits September 20, 2016 19:52

Add Doer interface, DoerFunc type and replace http.Client with Doer

6b4902a

Add Errors type and made it error instance to return all errors from …

9a6f5cb

…asana

Add ErrUnauthorized on 401 response status code and test it

d3f78fd

Add context.Context support for all operations and fix tests

6db1851

Extend request function to allow do x-www-form-urlencoded requests

e210494

Add CreateTask function and test it

549df79

anpryl force-pushed the master branch from 0cc56ad to 549df79 Compare September 20, 2016 18:55

Change go version to 1.7 in travis.yml

98380c1

Add DueOn and DueAt fields to Task struct

12d49ff

dmitshur requested changes Sep 23, 2016

View reviewed changes

Prylutskyi Anatolii added 3 commits September 25, 2016 20:58

Add space after two slashes for all godocs

4ad4491

Add godoc for Errors type

5a94976

Improve godoc for request function, add info about form argument

67b650e

Remove global context in tests

84ac3b0

dmitshur approved these changes Sep 30, 2016

View reviewed changes

Add missing periods to end of sentences.

84152d5

This is more consistent with the rest of the code.

dmitshur approved these changes Oct 14, 2016

View reviewed changes

dmitshur merged commit 6c0cb70 into tambet:master Oct 15, 2016

dmitshur added a commit to shurcooL/users that referenced this pull request Oct 15, 2016

Update for go-asana API change.

0f44743

See tambet/go-asana#4.

dmitshur added a commit to shurcooL/issues that referenced this pull request Oct 15, 2016

asanaapi: Update for go-asana API change.

8bc4323

See tambet/go-asana#4.

dmitshur added a commit to shurcooL/users that referenced this pull request Oct 15, 2016

asanaapi: Update for go-asana API change.

30bdb6a

See tambet/go-asana#4.

dmitshur added a commit to shurcooL/users that referenced this pull request Oct 15, 2016

asanaapi: Update for go-asana API change.

d1ef8a7

See tambet/go-asana#4.

dmitshur mentioned this pull request Jan 21, 2017

support the context package google/go-github#526

Closed

dmitshur mentioned this pull request Feb 8, 2017

Add support for context package. google/go-github#529

Merged

alindeman mentioned this pull request Mar 6, 2017

Adds support for context.Context interagent/schematic#41

Merged

Add context support, ErrUnauthorized and Doer interface for http.Client #4

Add context support, ErrUnauthorized and Doer interface for http.Client #4

Conversation

anpryl commented Sep 19, 2016

coveralls commented Sep 19, 2016 • edited Loading

dmitshur left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Sep 20, 2016

coveralls commented Sep 20, 2016 • edited Loading

anpryl commented Sep 20, 2016

coveralls commented Sep 20, 2016 • edited Loading

dmitshur left a comment

Choose a reason for hiding this comment

dmitshur Sep 23, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitshur Sep 30, 2016 • edited Loading

Choose a reason for hiding this comment

willnorris Sep 30, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitshur Oct 14, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Sep 25, 2016 • edited Loading

coveralls commented Sep 25, 2016 • edited Loading

dmitshur left a comment

Choose a reason for hiding this comment

dmitshur Sep 30, 2016 • edited Loading

Choose a reason for hiding this comment

coveralls commented Sep 30, 2016 • edited Loading

dmitshur left a comment

Choose a reason for hiding this comment

dmitshur commented Oct 15, 2016

coveralls commented Sep 19, 2016 •

edited

Loading

coveralls commented Sep 20, 2016 •

edited

Loading

coveralls commented Sep 20, 2016 •

edited

Loading

dmitshur Sep 23, 2016 •

edited

Loading

dmitshur Sep 30, 2016 •

edited

Loading

willnorris Sep 30, 2016 •

edited

Loading

dmitshur Oct 14, 2016 •

edited

Loading

coveralls commented Sep 25, 2016 •

edited

Loading

coveralls commented Sep 25, 2016 •

edited

Loading

dmitshur Sep 30, 2016 •

edited

Loading

coveralls commented Sep 30, 2016 •

edited

Loading