Fix issue where aiohttp.ClientResponse data wasn't being awaited on

[cmbernard333]

Fixes issue #184 where aiohttp.ClientResponse data wasn't being awaited on. Credit to @extreme4all and @demetere for doing the actual investigation work here.

Description

Added awaiting for the response data inside of the exception handler so that it is safe to read the response status.

References

• Python SDK error handling fails to report underlying OpenFGA exception #184

Review Checklist

• I have clicked on "allow edits by maintainers".
• I have added documentation for new/changed functionality in this PR or in a PR to openfga.dev [Provide a link to any relevant PRs in the references section above]
• The correct base branch is being used, if not main
• I have added tests to validate that the change in functionality is working as expected

[linux-foundation-easycla]

• ✅login: cmbernard333 / (b34b275)

The committers listed above are authorized under a signed CLA.

[cmbernard333]

@extreme4all and @demetere let me know if I missed something. This functionality is super important to handle http statuses correctly.

[demetere]

@cmbernard333 The two-line change itself is indeed straightforward, but the main challenge I've been facing is writing a test case that captures the issue. Right now, the tests pass both with and without the change, which suggests we're missing coverage for that edge case. I'm currently digging into the existing tests to understand what's already covered and where a new test would best fit to reflect the problem we're addressing. That's why I haven’t opened a PR just yet.

[cmbernard333]

@cmbernard333 The two-line change itself is indeed straightforward, but the main challenge I've been facing is writing a test case that captures the issue. Right now, the tests pass both with and without the change, which suggests we're missing coverage for that edge case. I'm currently digging into the existing tests to understand what's already covered and where a new test would best fit to reflect the problem we're addressing. That's why I haven’t opened a PR just yet.

Interesting. I can consistently make it fail, prior to any changes, by passing bad data to the /check api when using it in asynchronous mode in my own code. Let me see if I can’t pull a test together.

[cmbernard333]

After reviewing the tests in the repo, and unless I'm missing something entirely, there doesn't appear to be a good way to test this inside the repo itself since everything appears to be mocked. I can repeatedly reproduce this with the openfga python-sdk talking to a local openfga instance just by passing bad data to /check. It will consistently throw an exception with the message AttributeError: 'ClientResponse' object has no attribute 'data'. This makes getting errors from the python-sdk non-trivial.

Any proposal here on testing it properly is appreciated.

[cmbernard333]

@demetere one more note: the failure is reproducible most when using async, hence the need to check for aiohttp.ClientResponse. Does that help us narrow down a test case?

[cmbernard333]

@cmbernard333 The two-line change itself is indeed straightforward, but the main challenge I've been facing is writing a test case that captures the issue. Right now, the tests pass both with and without the change, which suggests we're missing coverage for that edge case. I'm currently digging into the existing tests to understand what's already covered and where a new test would best fit to reflect the problem we're addressing. That's why I haven’t opened a PR just yet.

I know I am being annoying. Its annoying this occurs constantly for me in my testing. It is definitely not an edge case when running with async. I can easily reproduce it with the SDK via the following:

    read_response: ReadResponse = await openfga_client.read(
        body=ReadRequestTupleKey(
            user="user:some-id",
            relation="admin",
            object="bad_object:some-id",
        )
    )

Then it produces this stack trace

    def __init__(self, status=None, reason=None, http_resp=None):
        if http_resp:
            try:
                headers = http_resp.headers.items()
            except AttributeError:
                headers = http_resp.getheaders().items()
   
            self.status = http_resp.status
            self.reason = http_resp.reason
>           self.body = http_resp.data
E           AttributeError: 'ClientResponse' object has no attribute 'data'

[demetere]

Sadly I do not have any leads on that, that was the sole reason for not opening PR from my side, Let ask the opinion of maintainers

[RRRajput]

Is there any update on this one?

[dyeam0]

Hi all. Thanks @cmbernard333 and @demetere for really digging into this issue. During our backlog review this week, we can assign someone to help and get more eyes on this.