fix(http): ApiClient.request_sync must not wrap sync result in event loop - #1367
fix(http): ApiClient.request_sync must not wrap sync result in event loop#1367Harsh23Kashyap wants to merge 2 commits into
Conversation
…loop The sync ApiClient.request_sync shared the same body as the async AsyncApiClient.request_sync: it called get_event_loop().run_until_complete on self.request(...). For the async client, self.request returns a coroutine, so the event-loop bridge is correct. For the sync client, self.request returns an ordinary value, so run_until_complete raised TypeError: An asyncio.Future, a coroutine or an awaitable is required on every successful response. The two methods live in the same file and were clearly meant to differ in exactly this respect. The async side stays unchanged; the sync side now just returns self.request(...) directly. Adds a unit regression that mocks request with a plain dict and asserts the dict is returned, plus a type_=None path check. No live server required. Fixes qdrant#1336
✅ Deploy Preview for poetic-froyo-8baba7 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 4 included reviews per hour; 2 remain after this review. 📝 WalkthroughWalkthrough
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The synchronous request wrapper now returns its synchronous result directly, restoring the documented behavior without changing its API or the asynchronous path. No actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
The 3-line regression comment for qdrant#1336 repeated the bug description that the linked issue already has. Replaced with a 2-line version that points to the bug number and the operation that was wrong. Also dropped the inline comment on the type_=None test block — the block's patch and assertion are self-evident.
1 participant
Fixes #1336
Problem
ApiClient.request_syncis a synchronous convenience wrapper, but it shared its body withAsyncApiClient.request_sync— both calledget_event_loop().run_until_complete(self.request(...)). For the async class that's correct (self.requestreturns a coroutine). For the sync class,self.requestreturns an ordinary value, so every successful response raisedTypeError: An asyncio.Future, a coroutine or an awaitable is requiredinstead of being returned.Repro from the issue:
Fix
qdrant_client/http/api_client.py—ApiClient.request_syncnow returnsself.request(...)directly. The async side (AsyncApiClient.request_sync) is unchanged — it still usesrun_until_completebecause itsself.requestis a coroutine.One-line change, 1 line of context.
Test
tests/test_qdrant_client.py::test_api_client_request_sync_returns_value— pure unit regression that patchesApiClient.requestto return a plain dict and asserts the dict flows through. Also covers thetype_=Nonepath. Fails on master (TypeError), passes with the fix. No live server required.Scope
ApiClient.request_sync)from unittest.mock import patch)No API changes (the wrapper is
request_syncand its signature is unchanged), no behavior change for the existing happy path, no dependency change. Distinct from the previous #1326/#1366 async-side timeout fix.