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.

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

drive: FilesGetCall.Download did not unmarshal the body in the error response #752

Closed
aofei opened this issue Nov 18, 2020 · 3 comments
Closed

Comments

@aofei
Copy link

@aofei aofei commented Nov 18, 2020

Just now I happened to notice that the FilesGetCall.Download threw me this error:

googleapi: got HTTP response code 403 with body: {"error":{"errors":[{"domain":"usageLimits","reason":"userRateLimitExceeded","message":"User Rate Limit Exceeded. Rate of requests for user exceed configured project quota. You may consider re-evaluating expected per-user traffic to the API and adjust project quota limits accordingly. You may monitor aggregate quota usage and adjust limits in the API Console: https://console.developers.google.com/apis/api/drive.googleapis.com/quotas?project=xxxxxxxxxxxxxx","extendedHelp":"https://console.developers.google.com/apis/api/drive.googleapis.com/quotas?project=xxxxxxxxxxxxxx"}],"code":403,"message":"User Rate Limit Exceeded. Rate of requests for user exceed configured project quota. You may consider re-evaluating expected per-user traffic to the API and adjust project quota limits accordingly. You may monitor aggregate quota usage and adjust limits in the API Console: https://console.developers.google.com/apis/api/drive.googleapis.com/quotas?project=xxxxxxxxxxxxxx"}}

The returned error is still *googleapi.Error, which is fine. But in its fields, only Code and Body have values. This caused me to be unable to handle errors through the Errors field as usual.

So, does this work as designed?

@tbpg
Copy link
Contributor

@tbpg tbpg commented Nov 20, 2020

Thanks for the issue. I'm not sure why that's the case for this call. Here is what appears to be the relevant function:

// CheckMediaResponse returns an error (of type *Error) if the response
// status code is not 2xx. Unlike CheckResponse it does not assume the
// body is a JSON error document.
// It is the caller's responsibility to close res.Body.
func CheckMediaResponse(res *http.Response) error {
if res.StatusCode >= 200 && res.StatusCode <= 299 {
return nil
}
slurp, _ := ioutil.ReadAll(io.LimitReader(res.Body, 1<<20))
return &Error{
Code: res.StatusCode,
Body: string(slurp),
}
}

Note that it doesn't try to parse the body as JSON, per the function comment. It seems that all "media" requests use CheckMediaResponse, which makes sense based on naming! See https://github.com/googleapis/google-api-go-client/search?q=checkmediaresponse.

Error parsing came up for #473. So, let's wait for @codyoss. Notably, https://google.aip.dev/193 doesn't call out media requests as different.

CheckMediaResponse was added in 0a735f7 6 years ago, so it's possible that it's now safe to assume JSON.

@codyoss
Copy link
Member

@codyoss codyoss commented Dec 3, 2020

Thanks for the issue @aofei. @tbpg I assume this is a special case for storage, at least I know that api does not conform to the aip and has xml endpoints... We could keep using this method for storage and move all other apis to use CheckResponse. That way we can give the opportunity to parse out extra info for apis that conform but not slow down storage errors that we know will not parse into JSON.

@tbpg
Copy link
Contributor

@tbpg tbpg commented Dec 3, 2020

We could keep using this method for storage and move all other apis to use CheckResponse.

Sounds good to me.

codyoss added a commit to codyoss/google-api-go-client that referenced this issue Dec 7, 2020
If a method supports media downloads use CheckResponse. This method
provided extra context to errors if the api follow
https://google.aip.dev/193. The storage api will continue to use
CheckMediaResponse as it does not conform to the aip due to legacy
reasons.

Fixes: googleapis#752
gcf-merge-on-green bot pushed a commit that referenced this issue Dec 11, 2020
If a method supports media downloads use CheckResponse. This method
provided extra context to errors if the api follow
https://google.aip.dev/193. The storage api will continue to use
CheckMediaResponse as it does not conform to the aip due to legacy
reasons.

Fixes: #752
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

4 participants