googleapi: parse errors and put more details into the error message

This CL adds support of errors array returned by Google API frontend. In some cases, such as invalid API key, putting more details into the error message, may help API users to debug problems quickly. Side note: this is a response to a real issue we had a couple of days ago.

[Ivan Krasin, 2014-06-18 23:12:10]

Hi Glenn, please, take a look at the CL.

[gmlewis1, 2014-06-19 01:09:13]

Thanks, Ivan! https://codereview.appspot.com/103500044/diff/40001/googleapi/googleapi.go File googleapi/googleapi.go (right): https://codereview.appspot.com/103500044/diff/40001/googleapi/googleapi.go#ne... googleapi/googleapi.go:57: fmt.Sprintf("googleapi: got HTTP response code %d with body: %v", e.Code, e.Body) s/fmt/return fmt/ https://codereview.appspot.com/103500044/diff/40001/googleapi/googleapi_test.go File googleapi/googleapi_test.go (right): https://codereview.appspot.com/103500044/diff/40001/googleapi/googleapi_test.... googleapi/googleapi_test.go:157: }, It would be great to add a test case for this situation: len(e.Errors) == 0 && e.Message == ""

[Ivan Krasin, 2014-06-19 01:16:04]

https://codereview.appspot.com/103500044/diff/40001/googleapi/googleapi.go File googleapi/googleapi.go (right): https://codereview.appspot.com/103500044/diff/40001/googleapi/googleapi.go#ne... googleapi/googleapi.go:57: fmt.Sprintf("googleapi: got HTTP response code %d with body: %v", e.Code, e.Body) On 2014/06/19 01:09:12, gmlewis1 wrote: > s/fmt/return fmt/ Done, thank you very much for the catch. Note: this was only possible, because the test does not have any expectations about error messages. I think it makes sense to add them (and I am ready to do so) Your opinion? https://codereview.appspot.com/103500044/diff/40001/googleapi/googleapi_test.go File googleapi/googleapi_test.go (right): https://codereview.appspot.com/103500044/diff/40001/googleapi/googleapi_test.... googleapi/googleapi_test.go:157: }, On 2014/06/19 01:09:12, gmlewis1 wrote: > It would be great to add a test case for this situation: > len(e.Errors) == 0 && e.Message == "" Done.

[gmlewis1, 2014-06-19 01:54:53]

LGTM Please allow bradfitz@ to review as well. https://codereview.appspot.com/103500044/diff/40001/googleapi/googleapi.go File googleapi/googleapi.go (right): https://codereview.appspot.com/103500044/diff/40001/googleapi/googleapi.go#ne... googleapi/googleapi.go:57: fmt.Sprintf("googleapi: got HTTP response code %d with body: %v", e.Code, e.Body) On 2014/06/19 01:16:04, Ivan Krasin wrote: > On 2014/06/19 01:09:12, gmlewis1 wrote: > > s/fmt/return fmt/ > > Done, thank you very much for the catch. > > Note: this was only possible, because the test does not have any expectations > about error messages. I think it makes sense to add them (and I am ready to do > so) > > Your opinion? Definitely I think this is a fantastic addition, since we have no control over the myriad API servers that will be talking to this code and how they are implemented.

[Ivan Krasin, 2014-06-19 02:31:31]

I have added expectations for error messages. Glenn, Brad, please take a look.

[gmlewis1, 2014-06-19 05:15:19]

LGTM - just a couple nits https://codereview.appspot.com/103500044/diff/80001/googleapi/googleapi_test.go File googleapi/googleapi_test.go (right): https://codereview.appspot.com/103500044/diff/80001/googleapi/googleapi_test.... googleapi/googleapi_test.go:195: if g != nil && fmt.Sprintf("%s", g) != test.errText { Why not if g != nil && string(g) != test.errText { ? https://codereview.appspot.com/103500044/diff/80001/googleapi/googleapi_test.... googleapi/googleapi_test.go:196: t.Errorf("CheckResponse: unexpected error message.\nGot: %s\nwant: %s", g, test.errText) Do we want to use "%q" for unit tests for quoting?

[Ivan Krasin, 2014-06-19 15:09:41]

PTAL https://codereview.appspot.com/103500044/diff/80001/googleapi/googleapi_test.go File googleapi/googleapi_test.go (right): https://codereview.appspot.com/103500044/diff/80001/googleapi/googleapi_test.... googleapi/googleapi_test.go:195: if g != nil && fmt.Sprintf("%s", g) != test.errText { http://play.golang.org/p/IluOuy2FNA I complains: "cannot convert err (type error) to type string" But I agree, that would be handy. https://codereview.appspot.com/103500044/diff/80001/googleapi/googleapi_test.... googleapi/googleapi_test.go:196: t.Errorf("CheckResponse: unexpected error message.\nGot: %s\nwant: %s", g, test.errText) On 2014/06/19 05:15:19, gmlewis1 wrote: > Do we want to use "%q" for unit tests for quoting? Done.

[gmlewis1, 2014-06-19 15:46:39]

https://codereview.appspot.com/103500044/diff/80001/googleapi/googleapi_test.go File googleapi/googleapi_test.go (right): https://codereview.appspot.com/103500044/diff/80001/googleapi/googleapi_test.... googleapi/googleapi_test.go:195: if g != nil && fmt.Sprintf("%s", g) != test.errText { On 2014/06/19 15:09:40, Ivan Krasin wrote: > http://play.golang.org/p/IluOuy2FNA > > I complains: "cannot convert err (type error) to type string" > But I agree, that would be handy. OK, another option is g.Error()... http://play.golang.org/p/U6jAL94pCP but your call. Brad may have advice here.

[Ivan Krasin, 2014-06-19 15:49:38]

https://codereview.appspot.com/103500044/diff/80001/googleapi/googleapi_test.go File googleapi/googleapi_test.go (right): https://codereview.appspot.com/103500044/diff/80001/googleapi/googleapi_test.... googleapi/googleapi_test.go:195: if g != nil && fmt.Sprintf("%s", g) != test.errText { On 2014/06/19 15:46:39, gmlewis1 wrote: > On 2014/06/19 15:09:40, Ivan Krasin wrote: > > http://play.golang.org/p/IluOuy2FNA > > > > I complains: "cannot convert err (type error) to type string" > > But I agree, that would be handy. > > OK, another option is g.Error()... > http://play.golang.org/p/U6jAL94pCP > but your call. Brad may have advice here. Done.

[Ivan Krasin, 2014-06-23 17:56:38]

Friendly ping.

[bradfitz, 2014-06-23 18:00:33]

LGTM Leaving for Glenn to submit. https://codereview.appspot.com/103500044/diff/120001/googleapi/googleapi.go File googleapi/googleapi.go (right): https://codereview.appspot.com/103500044/diff/120001/googleapi/googleapi.go#n... googleapi/googleapi.go:47: // ErrorItem is a detailed error found in errors array. "in the". And it's a slice, not an array. I'd just say: // ErrorItem is a detailed error code & message from the Google API // frontend. https://codereview.appspot.com/103500044/diff/120001/googleapi/googleapi.go#n... googleapi/googleapi.go:49: // Reason is the typed error code from Google API frontend. // Reason is the typed error code. For example: "some_example". https://codereview.appspot.com/103500044/diff/120001/googleapi/googleapi.go#n... googleapi/googleapi.go:65: return buf.String() strings.TrimSpace?

[Ivan Krasin, 2014-06-23 18:08:25]

Done. https://codereview.appspot.com/103500044/diff/120001/googleapi/googleapi.go File googleapi/googleapi.go (right): https://codereview.appspot.com/103500044/diff/120001/googleapi/googleapi.go#n... googleapi/googleapi.go:47: // ErrorItem is a detailed error found in errors array. On 2014/06/23 18:00:33, bradfitz wrote: > "in the". And it's a slice, not an array. > > I'd just say: > > // ErrorItem is a detailed error code & message from the Google API > // frontend. Done. https://codereview.appspot.com/103500044/diff/120001/googleapi/googleapi.go#n... googleapi/googleapi.go:49: // Reason is the typed error code from Google API frontend. On 2014/06/23 18:00:33, bradfitz wrote: > // Reason is the typed error code. For example: "some_example". Done. https://codereview.appspot.com/103500044/diff/120001/googleapi/googleapi.go#n... googleapi/googleapi.go:65: return buf.String() On 2014/06/23 18:00:33, bradfitz wrote: > strings.TrimSpace? Done.

[gmlewis1, 2014-06-23 18:21:38]

*** Submitted as https://code.google.com/p/google-api-go-client/source/detail?r=0260180239b8 *** This CL adds support of errors array returned by Google API frontend. In some cases, such as invalid API key, putting more details into the error message, may help API users to debug problems quickly. Side note: this is a response to a real issue we had a couple of days ago. R=gmlewis, bradfitz CC=golang-codereviews https://codereview.appspot.com/103500044 Committer: Glenn Lewis <gmlewis@google.com>