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

HttpError doesn't include JSON error response with the "pipe_to" option enabled #120

Open
jedrazb opened this issue Jun 22, 2023 · 1 comment
Labels
bug Something isn't working

Comments

@jedrazb
Copy link

jedrazb commented Jun 22, 2023

Hi, I came across inconsistent behaviour with the reason in the HttpError exception. It seems to depend on the pipe_to function argument being present or not. If we have a pipe_to argument, the non-200 response doesn't have the error reason field populated.

Why is it important?

According to google docs there are at least 8 possible reasons for getting a 403 response. The recommended approach to dealing with 403s depends on the returned reason in the non-200 response.

Code example to reproduce

import asyncio
from aiofiles.tempfile import NamedTemporaryFile
from aiogoogle import Aiogoogle
from aiogoogle.auth.creds import ServiceAccountCreds

creds = {...}

async def example_api_call(**kwargs):
    async with Aiogoogle(
        service_account_creds=ServiceAccountCreds(
            scopes=["https://www.googleapis.com/auth/drive.readonly"],
            **creds,
        )
    ) as google_client:
        drive = await google_client.discover(
            api_name="drive", api_version="v3"
        )
        first_page_with_next_attached = (
            await google_client.as_service_account(
                drive.files.export(**kwargs),
                full_res=True
            )
        )
        async for page_items in first_page_with_next_attached:
            yield page_items


async def example():
    await anext(example_api_call(
          fileId="...",
          mimeType="application/vnd.openxmlformats-officedocument.spreadsheetml.sheet",
        ))

In this scenario, when calling the client directly, the 403 response will cause a HttpError to be thrown with a reason:

Forbidden

Content:
{'code': 403,
 'errors': [{'domain': 'global',
             'message': 'This file is too large to be exported.',
             'reason': 'exportSizeLimitExceeded'}],
 'message': 'This file is too large to be exported.'}

Request URL:
https://www.googleapis.com/drive/v3/files/...

But when using a pipe_to argument, the HttpError won't contain the error reason.

async def example():
    async with NamedTemporaryFile(mode="wb", delete=False) as async_buffer:
        await anext(example_api_call(
              fileId="...",
              mimeType="application/vnd.openxmlformats-officedocument.spreadsheetml.sheet",
              pipe_to=async_buffer,
            ))

Thrown error:

Forbidden

Request URL:
https://www.googleapis.com/drive/v3/files/...

Next steps

I suppose it's not expected behaviour. The reason in the error response should (always?) be populated, meaning we can access it e.g. via e.res.reason in case we encounter HttpError e.

I'm happy to contribute with a PR if you feel like this issue should be addressed.

Also, thank you for building and maintaining this library. 🥇

@omarryhan
Copy link
Owner

Hi, thank you for taking the time to file the issue in such detail!

I'm happy to contribute with a PR if you feel like this issue should be addressed.

That would be amazing 🙏

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants