fix: aiohttp.ClientResponse.data should be awaited

[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

Summary by CodeRabbit

• Bug Fixes
  • Improved error handling to ensure raw response data is available for certain HTTP responses before processing exceptions.

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: cmbernard333 / name: Christian (a989bdd, 8ffdfb0, b34b275)

[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.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 50.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 70.67%. Comparing base (ea861f1) to head (8ffdfb0).

Files with missing lines	Patch %	Lines
openfga_sdk/rest.py	50.00%	1 Missing ⚠️

❌ Your project status has failed because the head coverage (70.67%) is below the target coverage (80.00%). You can increase the head coverage or adjust the target coverage.

Additional details and impacted files

@@ Coverage Diff @@ ## main #197 +/- ## ========================================== - Coverage 70.67% 70.67% -0.01%  ========================================== Files 134 134 Lines 10868 10870 +2 ========================================== + Hits 7681 7682 +1  - Misses 3187 3188 +1 

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[evansims]

Thanks for making this PR, @cmbernard333! One minor adjustment I'd suggest here, but otherwise looks good.

[coderabbitai]

Walkthrough

A conditional block was added to the asynchronous handle_response_exception method in the RESTClientObject class. This block ensures that, for aiohttp.ClientResponse objects lacking a data attribute, the response content is read and assigned to response.data before exception handling proceeds.

Changes

File(s)	Change Summary
openfga_sdk/rest.py	Added logic to assign response content to data attribute of aiohttp.ClientResponse objects in handle_response_exception if not already present.

Sequence Diagram(s)

sequenceDiagram participant Caller participant RESTClientObject participant aiohttp.ClientResponse Caller->>RESTClientObject: handle_response_exception(response) alt response is aiohttp.ClientResponse and lacks data RESTClientObject->>aiohttp.ClientResponse: read() RESTClientObject->>aiohttp.ClientResponse: set data attribute end RESTClientObject->>RESTClientObject: process exception handling logic 

Loading

---

📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a989bdd and 8ffdfb0.

📒 Files selected for processing (1)

• openfga_sdk/rest.py (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

• openfga_sdk/rest.py

✨ Finishing Touches

• 📝 Generate Docstrings

---

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>, please review it.
  • Explain this complex logic.
  • Open a follow-up GitHub issue for this discussion.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai explain this code block.
  • @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
  • @coderabbitai read src/utils.ts and explain its main purpose.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
  • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai full review to do a full review from scratch and review all the files again.
• @coderabbitai summary to regenerate the summary of the PR.
• @coderabbitai generate docstrings to generate docstrings for this PR.
• @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
• @coderabbitai help to get help.

Other keywords and placeholders

• Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
• Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
• Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• Please see the configuration documentation for more information.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

• Visit our Documentation for detailed information on how to use CodeRabbit.
• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.

[coderabbitai]

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ea861f1 and a989bdd.

📒 Files selected for processing (1)

• openfga_sdk/rest.py (1 hunks)

🧰 Additional context used

🪛 Flake8 (7.2.0)

openfga_sdk/rest.py

[error] 282-282: multiple spaces before operator

(E221)

[evansims]

Thanks again @cmbernard333, appreciate it!