A followup to pr #127#128
Conversation
There was a problem hiding this comment.
Hey @watersRand can we make the Commit message more informative of what we want to be achieved ?
The PR was never merged thus is hard to understand what this commit does without referencing a GitHub PR
And we also have some failing checks
|
Tip For best results, initiate chat on the files or code changes.
Just let me know! 🐇 |
|
@coderabbitai Review the PR |
|
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughRefactored async HTTP client to centralize retry/error utilities, add Tenacity-based retries and idempotency-key support, expand Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant AsyncClient as MpesaAsyncHttpClient
participant Retry as Tenacity (retry wrapper)
participant HTTPX as httpx.AsyncClient
participant MPESA as Mpesa API
Client->>AsyncClient: post(url, json, headers?, timeout=10, idempotent?)
alt idempotent == true
AsyncClient->>Retry: _retryable_post(...) (ensure X-Idempotency-Key)
Retry->>HTTPX: _raw_post(...)
HTTPX->>MPESA: HTTP POST
MPESA-->>HTTPX: response
HTTPX-->>Retry: response
Retry-->>AsyncClient: response
else
AsyncClient->>HTTPX: _raw_post(...)
HTTPX->>MPESA: HTTP POST
MPESA-->>HTTPX: response
HTTPX-->>AsyncClient: response
end
AsyncClient->>AsyncClient: handle_request_error(response)
AsyncClient-->>Client: response.json() or raise MpesaApiException
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/unit/http_client/test_mpesa_async_http_client.py (1)
162-174: Assert the retry attempt count in these GET failure tests.These assertions only pin the final translated exception. They still pass if
_raw_get()stops retrying but preserves the last error mapping. Add anawait_count == 3assertion to matchstop_after_attempt(3)so the new retry contract is actually covered.Also applies to: 178-190, 194-206
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/http_client/test_mpesa_async_http_client.py` around lines 162 - 174, The test only checks the final mapped MpesaApiException but not that retries occurred; update test_get_timeout (and the other similar tests at the referenced ranges) to assert the mocked request was awaited the expected number of times by adding an assertion like async_client._client.get.await_count == 3 after the existing exception assertions to match stop_after_attempt(3); apply the same await_count == 3 check in the two other GET failure tests mentioned.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mpesakit/http_client/mpesa_async_http_client.py`:
- Around line 50-57: The POST retry policy risks duplicate transactions; update
the retry behavior in _raw_post (and its decorator/usage) so POST requests do
not automatically retry on network timeouts unless the caller explicitly opts
into idempotency/deduplication; specifically, change the retry predicate used on
_raw_post to only retry for safe/idempotent methods or when an idempotency
token/header is provided, and leave _raw_get unchanged; ensure callers like the
STK Push flow (stk_push.py processrequest call) either disable retries or pass
an idempotency key before allowing retries.
In `@tests/unit/mpesa_express/test_stk_push.py`:
- Around line 164-183: The test is calling MpesaHttpClient.post("/test")
directly which bypasses the StkPush flow; instead construct a StkPush (and
AsyncStkPush for the other test) using a mocked token manager and the real
client, patch the underlying httpx.Client.post used by StkPush to return the
mocked response, then loop 100 times calling StkPush.push() so the token lookup,
Authorization header construction, request serialization and service-layer reuse
are exercised; ensure you reference StkPush.push (and AsyncStkPush.push) and the
token manager mock when updating the test and assert both the push success count
and the mocked httpx post call_count == 100.
---
Nitpick comments:
In `@tests/unit/http_client/test_mpesa_async_http_client.py`:
- Around line 162-174: The test only checks the final mapped MpesaApiException
but not that retries occurred; update test_get_timeout (and the other similar
tests at the referenced ranges) to assert the mocked request was awaited the
expected number of times by adding an assertion like
async_client._client.get.await_count == 3 after the existing exception
assertions to match stop_after_attempt(3); apply the same await_count == 3 check
in the two other GET failure tests mentioned.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: acaf4488-58de-4e2b-9858-ce73371b628f
📒 Files selected for processing (3)
mpesakit/http_client/mpesa_async_http_client.pytests/unit/http_client/test_mpesa_async_http_client.pytests/unit/mpesa_express/test_stk_push.py
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
mpesakit/http_client/mpesa_async_http_client.py (1)
79-91: Remove duplicated POST transport logic in_retryable_post.
_retryable_post()duplicates_raw_post()and can drift over time. Delegate to_raw_post()so retry behavior wraps a single transport implementation.♻️ Proposed refactor
async def _retryable_post( self, url: str, json: Dict[str, Any], headers: Dict[str, str], timeout: int = 10, ) -> httpx.Response: - return await self._client.post( - url, - json=json, - headers=headers, - timeout=timeout, - ) + return await self._raw_post(url=url, json=json, headers=headers, timeout=timeout)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mpesakit/http_client/mpesa_async_http_client.py` around lines 79 - 91, _refiable_post currently duplicates the HTTP transport by directly calling self._client.post; change it to delegate to the single transport implementation _raw_post so retries wrap one place. Replace the body of _retryable_post to call and return await self._raw_post(url=url, json=json, headers=headers, timeout=timeout) (preserve the same parameters and return type httpx.Response) so all POST logic lives in _raw_post and retry behavior uses that single implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mpesakit/http_client/mpesa_async_http_client.py`:
- Around line 113-117: The code mutates the caller-provided headers dict when
adding an idempotency key, which can leak the key into subsequent requests; fix
by making a shallow copy of headers before any modification (e.g., assign
headers = dict(headers) when headers is not None or headers = {} otherwise) and
then add the "X-Idempotency-Key" (using uuid.uuid4()) to that copy so the
original dict passed into the method remains unchanged; update the logic around
the idempotent check and "X-Idempotency-Key" assignment (referencing the headers
variable, the idempotent flag, and uuid.uuid4()) to use the copied headers.
In `@tests/unit/mpesa_express/test_stk_push.py`:
- Around line 164-210: The test test_stk_push_multiple_times should ensure
MpesaHttpClient is closed even on early failures: wrap the loop/body that
constructs MpesaHttpClient(env="sandbox", use_session=use_session) and uses stk
= StkPush(http_client=client, token_manager=mock_token_manager) in a try/finally
(or use a context manager) and call client.close() (and if use_session=True
ensure any underlying session is closed) in the finally block so that the httpx
client/session is cleaned up regardless of assertion errors; apply the same
pattern to the other similar test at lines 341-386.
---
Nitpick comments:
In `@mpesakit/http_client/mpesa_async_http_client.py`:
- Around line 79-91: _refiable_post currently duplicates the HTTP transport by
directly calling self._client.post; change it to delegate to the single
transport implementation _raw_post so retries wrap one place. Replace the body
of _retryable_post to call and return await self._raw_post(url=url, json=json,
headers=headers, timeout=timeout) (preserve the same parameters and return type
httpx.Response) so all POST logic lives in _raw_post and retry behavior uses
that single implementation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 67c4458d-11a3-4571-b9b8-d15414df130c
📒 Files selected for processing (3)
mpesakit/http_client/mpesa_async_http_client.pytests/unit/http_client/test_mpesa_async_http_client.pytests/unit/mpesa_express/test_stk_push.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unit/http_client/test_mpesa_async_http_client.py
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
mpesakit/http_client/mpesa_async_http_client.py (1)
79-91: Have_retryable_post()delegate to_raw_post().The retryable and non-retryable POST paths currently duplicate the same low-level request construction. Reusing
_raw_post()keeps that behavior in one place and avoids the two paths drifting.Suggested refactor
async def _retryable_post( self, url: str, json: Dict[str, Any], headers: Dict[str, str], timeout: int = 10, ) -> httpx.Response: - return await self._client.post( - url, - json=json, - headers=headers, - timeout=timeout, - ) + return await self._raw_post(url, json, headers, timeout)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mpesakit/http_client/mpesa_async_http_client.py` around lines 79 - 91, _refactor:_retryable_post currently duplicates the low-level request logic; change it to call and return self._raw_post(...) instead of calling self._client.post directly. Pass the same parameters (url, json, headers, timeout) through to _raw_post so the retryable path reuses the centralized request construction in _raw_post (ensure the return type remains httpx.Response).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mpesakit/http_client/mpesa_async_http_client.py`:
- Around line 115-116: The branch that sets an idempotency key can add a
duplicate header if the caller provided a differently-cased key; change the
check in the idempotent handling (where `idempotent` and `headers` are used in
mpesa_async_http_client.py) to look for any header whose lowercased name equals
"x-idempotency-key" (e.g., any(k.lower() == "x-idempotency-key" for k in
headers)) and only generate and insert a new `X-Idempotency-Key` when no such
key exists; keep the canonical "X-Idempotency-Key" casing when inserting so
downstream servers see a consistent header name.
In `@mpesakit/mpesa_express/schemas.py`:
- Line 92: The schema contains a literal Base64-looking password
"bXlwYXNzd29yZA==" with a "#nosec B105" suppression; remove the suppression and
replace the value for the "Password" field with a clearly fake placeholder (e.g.
"BASE64_PASSWORD_PLACEHOLDER" or "<PASSWORD_PLACEHOLDER>") so it no longer
resembles credentials, and make the same replacement for the other occurrence
referenced (the second "Password" example). Ensure you do not reintroduce any
secret-suppression comments.
---
Nitpick comments:
In `@mpesakit/http_client/mpesa_async_http_client.py`:
- Around line 79-91: _refactor:_retryable_post currently duplicates the
low-level request logic; change it to call and return self._raw_post(...)
instead of calling self._client.post directly. Pass the same parameters (url,
json, headers, timeout) through to _raw_post so the retryable path reuses the
centralized request construction in _raw_post (ensure the return type remains
httpx.Response).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f0433efa-38e7-494e-8986-09d0ba850e8a
📒 Files selected for processing (3)
mpesakit/http_client/mpesa_async_http_client.pympesakit/mpesa_express/schemas.pytests/unit/mpesa_express/test_stk_push.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unit/mpesa_express/test_stk_push.py
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
mpesakit/http_client/mpesa_async_http_client.py (2)
79-91: Inconsistent indentation in method signature and body.The method parameters and return statement have non-standard indentation that differs from the rest of the file.
♻️ Suggested formatting fix
async def _retryable_post( - self, - url: str, - json: Dict[str, Any], - headers: MutableMapping[str, str], - timeout: int = 10, -) -> httpx.Response: - return await self._raw_post( + self, + url: str, + json: Dict[str, Any], + headers: MutableMapping[str, str], + timeout: int = 10, + ) -> httpx.Response: + return await self._raw_post( url, json, headers, timeout, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mpesakit/http_client/mpesa_async_http_client.py` around lines 79 - 91, The _retryable_post method has inconsistent indentation for its signature and the return statement; adjust the function signature and body to match the project's style (align parameters under the opening parenthesis or use a single-line signature) and indent the return statement one level inside the method body, making the call to _raw_post consistently formatted (refer to _retryable_post and the call to _raw_post) so the parameters and closing parenthesis line up with the rest of the file.
3-24: Redundant exception types in retry condition.
httpx.ConnectTimeoutandhttpx.ReadTimeoutare subclasses ofhttpx.TimeoutException, so listing them explicitly is redundant. Including onlyhttpx.TimeoutExceptionandhttpx.ConnectErrorwould be sufficient and clearer.♻️ Suggested simplification
mpesa_retry_condition = retry_if_exception_type( - (httpx.ConnectError, httpx.ConnectTimeout,httpx.TimeoutException ,httpx.ReadTimeout) + (httpx.ConnectError, httpx.TimeoutException) )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mpesakit/http_client/mpesa_async_http_client.py` around lines 3 - 24, The retry condition mpesa_retry_condition currently lists httpx.ConnectTimeout and httpx.ReadTimeout along with httpx.TimeoutException, which is redundant because those classes subclass httpx.TimeoutException; simplify the retry_if_exception_type call used to build mpesa_retry_condition to include only httpx.ConnectError and httpx.TimeoutException (remove explicit ConnectTimeout and ReadTimeout) so the intent is clearer and behavior remains the same.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/unit/mpesa_express/test_stk_push.py`:
- Around line 344-386: The test redundantly calls await client.aclose() after
using the context manager; remove the explicit await client.aclose() call at the
end of the test so the MpesaAsyncHttpClient context manager (its __aexit__)
handles closing automatically—locate the test block that creates
MpesaAsyncHttpClient(), AsyncStkPush, and the loop (references:
MpesaAsyncHttpClient, AsyncStkPush, mock_async_token_manager) and delete the
final await client.aclose() statement.
---
Nitpick comments:
In `@mpesakit/http_client/mpesa_async_http_client.py`:
- Around line 79-91: The _retryable_post method has inconsistent indentation for
its signature and the return statement; adjust the function signature and body
to match the project's style (align parameters under the opening parenthesis or
use a single-line signature) and indent the return statement one level inside
the method body, making the call to _raw_post consistently formatted (refer to
_retryable_post and the call to _raw_post) so the parameters and closing
parenthesis line up with the rest of the file.
- Around line 3-24: The retry condition mpesa_retry_condition currently lists
httpx.ConnectTimeout and httpx.ReadTimeout along with httpx.TimeoutException,
which is redundant because those classes subclass httpx.TimeoutException;
simplify the retry_if_exception_type call used to build mpesa_retry_condition to
include only httpx.ConnectError and httpx.TimeoutException (remove explicit
ConnectTimeout and ReadTimeout) so the intent is clearer and behavior remains
the same.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0c3a8dac-3760-47f6-97db-b076d9ba3188
📒 Files selected for processing (2)
mpesakit/http_client/mpesa_async_http_client.pytests/unit/mpesa_express/test_stk_push.py
| before_sleep_log, | ||
| ) | ||
|
|
||
| from .mpesa_http_client import retry_enabled, handle_request_error, handle_retry_exception |
There was a problem hiding this comment.
I don't think it would be wise for an async client depending on functions found in a sync client.
It would be better to store them in the common base file http_client.py as methods or functions.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
mpesakit/http_client/mpesa_async_http_client.py (1)
93-100: Mirror these new parameters inAsyncHttpClient.
MpesaAsyncHttpClient.post()/get()now exposetimeoutandidempotent, butAsyncHttpClientinmpesakit/http_client/http_client.pystill only advertises(url, json, headers)and(url, params, headers). Callers typed against the abstraction won't see the new knobs, and static checks will keep rejecting valid calls.Also applies to: 187-193
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mpesakit/http_client/mpesa_async_http_client.py` around lines 93 - 100, The abstract interface AsyncHttpClient must mirror the new parameters introduced on MpesaAsyncHttpClient: add timeout: int = 10 and idempotent: bool = False to the AsyncHttpClient.post(...) signature (which currently lists url, json, headers) and likewise add timeout and idempotent to AsyncHttpClient.get(...) (currently url, params, headers); update the method type hints/docs to include these parameters so callers typed against AsyncHttpClient can pass them, and ensure any other concrete implementations of AsyncHttpClient implement the new parameters as well to preserve compatibility with MpesaAsyncHttpClient.post/get.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mpesakit/http_client/http_client.py`:
- Around line 71-83: The code assumes response.json() returns a dict and calls
response_data.get(...), which will raise AttributeError for non-object JSON
(list/string/null); update the error handling in the response parsing block that
assigns response_data to first check isinstance(response_data, dict) before
calling .get(), and for non-dict values convert/serialize them to a string
(e.g., use response.text or str(response_data)) to populate
MpesaError.error_message and raw_response so that raise
MpesaApiException(MpesaError(...)) always runs safely; touch the block around
response.json(), response_data, MpesaApiException and MpesaError to implement
this guard and fallback.
---
Nitpick comments:
In `@mpesakit/http_client/mpesa_async_http_client.py`:
- Around line 93-100: The abstract interface AsyncHttpClient must mirror the new
parameters introduced on MpesaAsyncHttpClient: add timeout: int = 10 and
idempotent: bool = False to the AsyncHttpClient.post(...) signature (which
currently lists url, json, headers) and likewise add timeout and idempotent to
AsyncHttpClient.get(...) (currently url, params, headers); update the method
type hints/docs to include these parameters so callers typed against
AsyncHttpClient can pass them, and ensure any other concrete implementations of
AsyncHttpClient implement the new parameters as well to preserve compatibility
with MpesaAsyncHttpClient.post/get.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d3fa35c1-7a88-4620-8474-6344dda92aad
📒 Files selected for processing (4)
mpesakit/http_client/http_client.pympesakit/http_client/mpesa_async_http_client.pympesakit/http_client/mpesa_http_client.pympesakit/mpesa_express/schemas.py
✅ Files skipped from review due to trivial changes (1)
- mpesakit/mpesa_express/schemas.py
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
tests/unit/mpesa_express/test_stk_push.py (1)
341-384: Minor: assertion placement differs between sync and async variants.In
test_stk_push_multiple_timestheassertstatements sit inside the innermostwith patch.object(httpx.Client, "send", ...)block, but here they sit outside thewith patch.object(httpx.AsyncClient, "send", ...)block (after the patch has been torn down).mock_sendis still in scope somock_send.call_countstill works, but for symmetry and to ensure the assertion failure message points at the patched object in its active state, consider moving these two asserts into the innerwith— matching the sync test.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/mpesa_express/test_stk_push.py` around lines 341 - 384, Move the two final assertions into the active patch context so failures point to the mocked object: inside test_async_stk_push_multiple_times, after the loop that calls await stk.push(...), place the asserts assert success_count == 100 and assert mock_send.call_count == 100 immediately inside the inner with patch.object(httpx.AsyncClient, "send", AsyncMock(...)) as mock_send block (where mock_send is in scope), mirroring the sync test structure and keeping the assertions while the patch is active.mpesakit/http_client/http_client.py (1)
46-66: Abstract signature drifts from concrete implementation.The abstract
AsyncHttpClient.postdeclaresheaders: Dict[str, str](required, non-Optional), butMpesaAsyncHttpClient.postdeclaresheaders: Optional[Dict[str, str]] = None. Either make the abstract contract match (Optional[Dict[str, str]] = None) to allow headerless calls likeAsyncStkPush.push-style callers, or tighten the concrete implementation. Same minor whitespace nit:timeout: int =10should betimeout: int = 10for consistency with the rest of the file.Proposed alignment
`@abstractmethod` async def post( self, url: str, json: Dict[str, Any], - headers: Dict[str, str], - timeout: int =10, - idempotent: bool = False + headers: Optional[Dict[str, str]] = None, + timeout: int = 10, + idempotent: bool = False, ) -> Dict[str, Any]: """Sends an asynchronous POST request.""" pass `@abstractmethod` async def get( self, url: str, params: Optional[Dict[str, Any]] = None, headers: Optional[Dict[str, str]] = None, - timeout: int =10 + timeout: int = 10, ) -> Dict[str, Any]:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mpesakit/http_client/http_client.py` around lines 46 - 66, The abstract AsyncHttpClient.post signature currently requires headers: Dict[str, str] while the concrete MpesaAsyncHttpClient.post accepts headers: Optional[Dict[str, str]] = None and the timeout default spacing is inconsistent; update the abstract method signature in AsyncHttpClient.post to use headers: Optional[Dict[str, str]] = None and change timeout: int =10 to timeout: int = 10 so it matches MpesaAsyncHttpClient.post (or conversely change MpesaAsyncHttpClient.post to a non-optional headers param if you prefer tightening the contract), ensuring both AsyncHttpClient.post and MpesaAsyncHttpClient.post use the same headers typing and consistent timeout formatting.mpesakit/http_client/mpesa_http_client.py (1)
52-101: Sync/async retry behavior is now asymmetric for POST.After this PR,
MpesaAsyncHttpClient.postonly retries whenidempotent=True(addressing the duplicate-transaction concern from earlier review), butMpesaHttpClient.poststill unconditionally retries via@retry(... retry_enabled(enabled=True) ...)on_raw_post. That meansStkPush.push()(sync) can still double-submit an STK Push on a timeout between request send and response receipt, whileAsyncStkPush.push()(async) won't because it doesn't opt in to idempotent mode. Consider mirroring the async design in the sync client so both paths share the same safety guarantees — either add anidempotentflag to the syncpost, or split the sync path into_raw_post+_retryable_postas done for async.mpesakit/http_client/mpesa_async_http_client.py (2)
118-158: Exception mapping works, but is redundant withhandle_retry_exception.The four cascading
exceptblocks duplicate the code-mapping logic inhandle_retry_exception(timeout →REQUEST_TIMEOUT, connect →CONNECTION_ERROR, fallback →REQUEST_FAILED). Once thereraise=Trueissue above is resolved and the callback runs reliably, you can collapse these to a singleexcept (httpx.RequestError, ValueError)block (matchingget) and let the shared helper own the mapping — reducing drift between the two methods. Also noteresponseis guaranteedNoneon everyexceptbranch here (thehttpxcalls raise before assignment), sogetattr(response, "status_code", None)on line 155 will always beNone.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mpesakit/http_client/mpesa_async_http_client.py` around lines 118 - 158, The multiple specific except blocks in the async POST flow should be collapsed to a single except (httpx.RequestError, ValueError) block in the method that calls _retryable_post/_raw_post (the code around response = await self._retryable_post/_raw_post and handle_request_error), so that exception-to-MpesaError mapping is delegated to the shared helper handle_retry_exception (and avoid duplicating timeout/connect/fallback logic); remove using getattr(response, "status_code", None) / getattr(response, "text", None) inside the except (since response is None on exceptions) and raise MpesaApiException(MpesaError(...)) only with the information provided by handle_retry_exception or the caught exception as appropriate (keep MpesaApiException and MpesaError usage). Ensure references to _retryable_post, _raw_post, handle_request_error, and handle_retry_exception are used to locate and update the code.
22-24: Simplify redundant exception types in retry predicate.
httpx.ConnectTimeoutandhttpx.ReadTimeoutare subclasses ofhttpx.TimeoutException, so including all three is redundant — the basehttpx.TimeoutExceptionalready covers both. Additionally,_raw_getbelow uses the sharedretry_enabled(enabled=True)helper for similar retry logic, while_retryable_postuses this custom condition. Standardize on one mechanism for consistency.Simplification
-mpesa_retry_condition = retry_if_exception_type( - (httpx.ConnectError, httpx.ConnectTimeout,httpx.TimeoutException ,httpx.ReadTimeout) -) +mpesa_retry_condition = retry_if_exception_type( + (httpx.ConnectError, httpx.TimeoutException) +)Alternatively, switch
_retryable_postto useretry=retry_enabled(enabled=True)to match_raw_get.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mpesakit/http_client/mpesa_async_http_client.py` around lines 22 - 24, The mpesa_retry_condition currently lists redundant exceptions (httpx.ConnectTimeout and httpx.ReadTimeout are subclasses of httpx.TimeoutException) and _retryable_post uses this custom predicate while _raw_get uses retry_enabled(enabled=True); simplify by removing the redundant types (keep only httpx.ConnectError and httpx.TimeoutException) or, better, change _retryable_post to use retry=retry_enabled(enabled=True) to standardize behavior with _raw_get; update references to mpesa_retry_condition and the _retryable_post call accordingly so both methods share the same retry mechanism.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mpesakit/http_client/mpesa_async_http_client.py`:
- Around line 71-91: The decorated async function _retryable_post has misaligned
indentation between its parameter list and body; adjust the indentation so the
"async def _retryable_post(" line, its parameter lines (self, url, json,
headers, timeout) and the body "return await self._raw_post(...)" are
consistently indented (same column) under the `@retry` decorator, or run the
project's formatter (e.g., black) to normalize formatting; ensure references to
_raw_post remain unchanged and the function signature and return remain intact.
---
Nitpick comments:
In `@mpesakit/http_client/http_client.py`:
- Around line 46-66: The abstract AsyncHttpClient.post signature currently
requires headers: Dict[str, str] while the concrete MpesaAsyncHttpClient.post
accepts headers: Optional[Dict[str, str]] = None and the timeout default spacing
is inconsistent; update the abstract method signature in AsyncHttpClient.post to
use headers: Optional[Dict[str, str]] = None and change timeout: int =10 to
timeout: int = 10 so it matches MpesaAsyncHttpClient.post (or conversely change
MpesaAsyncHttpClient.post to a non-optional headers param if you prefer
tightening the contract), ensuring both AsyncHttpClient.post and
MpesaAsyncHttpClient.post use the same headers typing and consistent timeout
formatting.
In `@mpesakit/http_client/mpesa_async_http_client.py`:
- Around line 118-158: The multiple specific except blocks in the async POST
flow should be collapsed to a single except (httpx.RequestError, ValueError)
block in the method that calls _retryable_post/_raw_post (the code around
response = await self._retryable_post/_raw_post and handle_request_error), so
that exception-to-MpesaError mapping is delegated to the shared helper
handle_retry_exception (and avoid duplicating timeout/connect/fallback logic);
remove using getattr(response, "status_code", None) / getattr(response, "text",
None) inside the except (since response is None on exceptions) and raise
MpesaApiException(MpesaError(...)) only with the information provided by
handle_retry_exception or the caught exception as appropriate (keep
MpesaApiException and MpesaError usage). Ensure references to _retryable_post,
_raw_post, handle_request_error, and handle_retry_exception are used to locate
and update the code.
- Around line 22-24: The mpesa_retry_condition currently lists redundant
exceptions (httpx.ConnectTimeout and httpx.ReadTimeout are subclasses of
httpx.TimeoutException) and _retryable_post uses this custom predicate while
_raw_get uses retry_enabled(enabled=True); simplify by removing the redundant
types (keep only httpx.ConnectError and httpx.TimeoutException) or, better,
change _retryable_post to use retry=retry_enabled(enabled=True) to standardize
behavior with _raw_get; update references to mpesa_retry_condition and the
_retryable_post call accordingly so both methods share the same retry mechanism.
In `@tests/unit/mpesa_express/test_stk_push.py`:
- Around line 341-384: Move the two final assertions into the active patch
context so failures point to the mocked object: inside
test_async_stk_push_multiple_times, after the loop that calls await
stk.push(...), place the asserts assert success_count == 100 and assert
mock_send.call_count == 100 immediately inside the inner with
patch.object(httpx.AsyncClient, "send", AsyncMock(...)) as mock_send block
(where mock_send is in scope), mirroring the sync test structure and keeping the
assertions while the patch is active.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c8098c2e-0cae-49fe-a14a-a3c0f33d40b1
📒 Files selected for processing (5)
mpesakit/http_client/http_client.pympesakit/http_client/mpesa_async_http_client.pympesakit/http_client/mpesa_http_client.pympesakit/mpesa_express/schemas.pytests/unit/mpesa_express/test_stk_push.py
✅ Files skipped from review due to trivial changes (1)
- mpesakit/mpesa_express/schemas.py
| @retry( | ||
| retry=mpesa_retry_condition, | ||
| wait=wait_random_exponential(multiplier=5, max=8), | ||
| stop=stop_after_attempt(3), | ||
| retry_error_callback=handle_retry_exception, | ||
| before_sleep=before_sleep_log(logger, logging.WARNING), | ||
| reraise=True, | ||
| ) | ||
| async def _retryable_post( | ||
| self, | ||
| url: str, | ||
| json: Dict[str, Any], | ||
| headers: MutableMapping[str, str], | ||
| timeout: int = 10, | ||
| ) -> httpx.Response: | ||
| return await self._raw_post( | ||
| url, | ||
| json, | ||
| headers, | ||
| timeout, | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
tenacity reraise True retry_error_callback interaction
💡 Result:
In the Python Tenacity library, retry_error_callback and reraise=True serve distinct purposes in error handling after all retries fail, and they interact such that retry_error_callback takes precedence if provided. - Without retry_error_callback: Tenacity raises a RetryError wrapping the last exception. With reraise=True, it re-raises the original last exception instead (at the top of the stack trace for better visibility). - With retry_error_callback: The callback is invoked with the final retry_state instead of raising RetryError. The callback can perform custom actions (e.g., logging) and return a value (suppressing the exception) or explicitly re-raise (e.g., raise RetryError(retry_state.outcome) or raise retry_state.outcome.exception). Providing retry_error_callback prevents the automatic RetryError raise, even if reraise=True is set—reraise does not alter this behavior. Thus, reraise=True has no effect when retry_error_callback is specified; to combine logging with exception propagation, re-raise manually in the callback. Examples: from tenacity import retry, stop_after_attempt, RetryError def on_error(retry_state): # Log or other actions raise RetryError(retry_state.outcome) # Re-raises wrapped RetryError @retry(stop=stop_after_attempt(3), retry_error_callback=on_error) def failing_fn: raise ValueError("fail") Or for original exception: def on_error(retry_state): raise retry_state.outcome.exception Official docs confirm retry_error_callback allows custom handling "without raising an exception (or you can re-raise or do anything really)", independent of reraise.
Citations:
- 1: Re-raising RetryError from retry_error_callback jd/tenacity#296
- 2: https://github.com/jd/tenacity/
- 3: https://tenacity.readthedocs.io/en/latest
- 4: https://tenacity.readthedocs.io/en/stable/
- 5: https://tenacity.readthedocs.io/en/stable/api.html
- 6: https://io.traffine.com/en/articles/retry-with-tenacity
🏁 Script executed:
# Find the file and examine the overall structure
find . -type f -name "mpesa_async_http_client.py" | head -5Repository: Byte-Barn/mpesakit
Length of output: 111
🏁 Script executed:
# Also search for the related sync client to understand the comparison
find . -type f -name "*http_client*.py" | grep -v __pycache__Repository: Byte-Barn/mpesakit
Length of output: 301
🏁 Script executed:
# Search for handle_retry_exception function
rg -n "handle_retry_exception" --type pyRepository: Byte-Barn/mpesakit
Length of output: 838
🏁 Script executed:
# Search for _raw_get and _raw_post methods
rg -n "_raw_get|_raw_post" --type py -A 5Repository: Byte-Barn/mpesakit
Length of output: 11120
🏁 Script executed:
# Look for the get and post methods around lines 207-220
rg -n "def get|def post" --type py -A 15Repository: Byte-Barn/mpesakit
Length of output: 14225
🏁 Script executed:
# Read handle_retry_exception implementation
sed -n '94,120p' mpesakit/http_client/http_client.pyRepository: Byte-Barn/mpesakit
Length of output: 1077
🏁 Script executed:
# Read the async post method error handling
sed -n '117,150p' mpesakit/http_client/mpesa_async_http_client.pyRepository: Byte-Barn/mpesakit
Length of output: 1162
🏁 Script executed:
# Read the async get method error handling
sed -n '204,220p' mpesakit/http_client/mpesa_async_http_client.pyRepository: Byte-Barn/mpesakit
Length of output: 674
🏁 Script executed:
# Read the sync post method error handling for comparison
sed -n '88,102p' mpesakit/http_client/mpesa_http_client.pyRepository: Byte-Barn/mpesakit
Length of output: 654
🏁 Script executed:
# Read the sync get method error handling for comparison
sed -n '149,165p' mpesakit/http_client/mpesa_http_client.pyRepository: Byte-Barn/mpesakit
Length of output: 741
🏁 Script executed:
# Check indentation of _retryable_post method (lines 70-91)
sed -n '70,92p' mpesakit/http_client/mpesa_async_http_client.py | cat -ARepository: Byte-Barn/mpesakit
Length of output: 640
🏁 Script executed:
# Search for httpx exception definitions to understand hierarchy
rg -n "class TimeoutException|class ConnectError|class RequestError" --type pyRepository: Byte-Barn/mpesakit
Length of output: 44
🏁 Script executed:
# Let's see what exceptions httpx raises for timeouts
python3 << 'EOF'
import httpx
# Check exception hierarchy
print("TimeoutException bases:", httpx.TimeoutException.__bases__)
print("ConnectError bases:", httpx.ConnectError.__bases__)
print("ConnectTimeout bases:", httpx.ConnectTimeout.__bases__)
print("RequestError bases:", httpx.RequestError.__bases__)
print("\nIsinstance checks:")
print("TimeoutException is RequestError:", issubclass(httpx.TimeoutException, httpx.RequestError))
print("ConnectError is RequestError:", issubclass(httpx.ConnectError, httpx.RequestError))
print("ConnectTimeout is TimeoutException:", issubclass(httpx.ConnectTimeout, httpx.TimeoutException))
EOFRepository: Byte-Barn/mpesakit
Length of output: 413
🏁 Script executed:
# Check if _retryable_get exists and its decorator
sed -n '155,170p' mpesakit/http_client/mpesa_async_http_client.pyRepository: Byte-Barn/mpesakit
Length of output: 573
🏁 Script executed:
# Check if there's a requirements.txt or setup.py with tenacity version
find . -type f \( -name "requirements*.txt" -o -name "setup.py" -o -name "pyproject.toml" \) | xargs grep -l tenacity 2>/dev/null | head -3Repository: Byte-Barn/mpesakit
Length of output: 78
🏁 Script executed:
# Get the actual tenacity version constraint
rg "tenacity" --type py --type txt | grep -i versionRepository: Byte-Barn/mpesakit
Length of output: 44
🏁 Script executed:
# Verify that post() with explicit handlers and _retryable_post with decorator don't double-catch
# by checking what _raw_post actually raises
sed -n '56,72p' mpesakit/http_client/mpesa_async_http_client.pyRepository: Byte-Barn/mpesakit
Length of output: 521
🏁 Script executed:
# Check pyproject.toml for tenacity version
cat pyproject.toml | grep -A 3 -B 3 tenacityRepository: Byte-Barn/mpesakit
Length of output: 217
Fix indentation of _retryable_post parameter list and body.
The parameter list (lines 80-85) is dedented to column 4 while the body (lines 86-91) is at column 5. Run your code formatter to align with standard Python conventions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mpesakit/http_client/mpesa_async_http_client.py` around lines 71 - 91, The
decorated async function _retryable_post has misaligned indentation between its
parameter list and body; adjust the indentation so the "async def
_retryable_post(" line, its parameter lines (self, url, json, headers, timeout)
and the body "return await self._raw_post(...)" are consistently indented (same
column) under the `@retry` decorator, or run the project's formatter (e.g., black)
to normalize formatting; ensure references to _raw_post remain unchanged and the
function signature and return remain intact.
|
@RafaelJohn9 ,I may need help here. |
RafaelJohn9
left a comment
RafaelJohn9
left a comment
There was a problem hiding this comment.
Hey @watersRand , Let's do this step by step, we will only focus on the issue at hand.
- Remove all idempotency logic for now, we will manage this later on.
- Fix all Syntax/Indentation problems (if any.)
- Remove all idempotency tests
- Make sure the rest of the tests are passing
Then we will be good to go. For now I'll disable CodeRabbit, it seems to never stop.
| async def _raw_post( | ||
| self, | ||
| url: str, | ||
| json: Dict[str, Any], | ||
| headers: MutableMapping[str, str], | ||
| timeout: int = 10, | ||
| ) -> httpx.Response: | ||
| """Low-level asynchronous POST request - may raise httpx exceptions.""" | ||
| return await self._client.post( | ||
| url, | ||
| json=json, | ||
| headers=headers, | ||
| timeout=timeout, | ||
| ) | ||
|
|
||
| @retry( | ||
| retry=mpesa_retry_condition, | ||
| wait=wait_random_exponential(multiplier=5, max=8), | ||
| stop=stop_after_attempt(3), | ||
| retry_error_callback=handle_retry_exception, | ||
| before_sleep=before_sleep_log(logger, logging.WARNING), | ||
| reraise=True, | ||
| ) | ||
| async def _retryable_post( | ||
| self, | ||
| url: str, | ||
| json: Dict[str, Any], | ||
| headers: MutableMapping[str, str], | ||
| timeout: int = 10, | ||
| ) -> httpx.Response: | ||
| return await self._raw_post( | ||
| url, | ||
| json, | ||
| headers, | ||
| timeout, |
There was a problem hiding this comment.
Is this the correct indentation ? perhaps CodeRabbit is wrong ?
watersRand
force-pushed
the
test/stk-push-limit
branch
from
77c5ae1 to
adc45a0
Compare
2 participants
Description
This PR is a follow-up on PR #127.
It aligns the
MpesaAsyncHttpClientwith the synchronous client to support unified retry functionality using thetenacitylogic.Key Changes:
@retrydecorators inMpesaAsyncHttpClientmatching the sync implementation.requests.SessionPerformance test when and without using it. #116: Added simulation for 100 STK pushes to verify performance and session handling._raw_postand_raw_getmethods to handle low-level exceptions.Type of Change
How Has This Been Tested?
tests/unit/http_client/pass.httpx.AsyncClientsession.Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation