Migrate Dev => Develop #89
Conversation
This commit introduces a robust HTTP client with optional session management. Users can now enable requests.Session to improve performance by reusing TCP connections for consecutive API calls. Adds unit tests to cover both session-based and stateless client behaviors. Refactors MpesaHttpClient to accept a use_session flag, with the default remaining as a stateless client.
…sakit into feat/add-sessions
Feat/add sessions
…(resolves review comment)
…esaHttpClient closes#44
Retry function
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughAdds retry logic with tenacity to HTTP GET/POST, centralizes error handling, and introduces optional persistent sessions in MpesaHttpClient. MpesaClient propagates a new use_session flag. Tests are refactored and expanded to cover session/non-session modes and retry behaviors. Tenacity is added as a dependency. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant MpesaClient
participant MpesaHttpClient
participant Session as requests.Session
participant Requests as requests
Caller->>MpesaClient: post/get(...)
MpesaClient->>MpesaHttpClient: post/get(path, payload, headers)
alt use_session = True
MpesaHttpClient->>Session: request (POST/GET, url, timeout)
else use_session = False
MpesaHttpClient->>Requests: request (POST/GET, url, timeout)
end
note over MpesaHttpClient: tenacity retry on timeout/connection errors
alt Response non-2xx
MpesaHttpClient->>MpesaHttpClient: handle_request_error(response)
MpesaHttpClient-->>Caller: MpesaApiException
else Response 2xx
MpesaHttpClient-->>Caller: parsed JSON
end
opt Exhausted retries
MpesaHttpClient->>MpesaHttpClient: handle_retry_exception(retry_state)
MpesaHttpClient-->>Caller: MpesaApiException (REQUEST_TIMEOUT/CONNECTION_ERROR/REQUEST_FAILED)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
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 review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (8)
pyproject.toml (1)
48-48: Consider allowing tenacity 9.x (latest fixes, same APIs used here).If compatible in CI, prefer >=9.1.2,<10.0.0.
Apply this diff:
- "tenacity>=8.2.3,<9.0.0" + "tenacity>=9.1.2,<10.0.0"Based on learnings
mpesakit/http_client/mpesa_http_client.py (2)
117-121: Make URL joining robust.Concatenation can double/miss slashes. Use urllib.parse.urljoin.
Apply these diffs:
+from urllib.parse import urljoin- full_url = f"{self.base_url}{url}" + full_url = urljoin(self.base_url + "/", url)And the same change in GET.
Also applies to: 154-159
23-44: Avoid double JSON parsing and broaden error message extraction.Only parse JSON on error, and fall back across common keys.
Apply this diff:
- try: - response_data = response.json() - except ValueError: - response_data = {"errorMessage": response.text.strip() or ""} - - if not response.ok: - error_message = response_data.get("errorMessage", "") + if not response.ok: + try: + response_data = response.json() + except ValueError: + response_data = {} + error_message = ( + response_data.get("errorMessage") + or response_data.get("message") + or response_data.get("error") + or (response.text.strip() if hasattr(response, "text") else "") + ) raise MpesaApiException( MpesaError( error_code=f"HTTP_{response.status_code}", error_message=error_message, status_code=response.status_code, raw_response=response_data, ) )mpesakit/mpesa_client.py (2)
23-28: Document the new use_session parameter.Add param doc so users discover session behavior and benefits.
Example:
def __init__(..., use_session: bool = False) -> None: """Initialize the MpesaClient with all service facades. Args: consumer_key: ... consumer_secret: ... environment: 'sandbox' or 'production'. use_session: Use a persistent requests.Session for connection pooling. """
27-27: Expose a close() to manage underlying HTTP session.Forward http_client.close() so apps can cleanly release resources.
Add:
def close(self) -> None: if hasattr(self.http_client, "close"): self.http_client.close()Optionally implement context manager to auto-close.
tests/unit/http_client/test_mpesa_http_client.py (3)
88-99: Align with “non-retryable” intent for RequestException and assert no retry.Capture the mock to assert a single call.
Apply this diff:
- with patch(patch_target, - side_effect=requests.RequestException("boom"), - ): - with pytest.raises(MpesaApiException) as exc: - client.post("/fail", json={}, headers={}) - - assert exc.value.error.error_code == "REQUEST_FAILED" + with patch(patch_target) as mock_post: + mock_post.side_effect = requests.RequestException("boom") + with pytest.raises(MpesaApiException) as exc: + client.post("/fail", json={}, headers={}) + assert exc.value.error.error_code == "REQUEST_FAILED" + mock_post.assert_called_once()Note: This assumes retries exclude RequestException as suggested in the client.
193-205: Do the same for GET: RequestException should not be retried.Assert only one attempt is made.
Apply this diff:
- with patch(patch_target, - side_effect=requests.RequestException("boom"), - ): - with pytest.raises(MpesaApiException) as exc: - - client.get("/fail") - - assert exc.value.error.error_code == "REQUEST_FAILED" + with patch(patch_target) as mock_get: + mock_get.side_effect = requests.RequestException("boom") + with pytest.raises(MpesaApiException) as exc: + client.get("/fail") + assert exc.value.error.error_code == "REQUEST_FAILED" + mock_get.assert_called_once()
39-52: Optional: Add a test for JSON decode error on success (ok=True).Covers the new JSON_DECODE_ERROR behavior on successful non-JSON responses.
Add:
def test_post_json_decode_error_on_success(client): patch_target = get_patch_target(client, "post") with patch(patch_target) as mock_post: mock_response = Mock() mock_response.ok = True mock_response.status_code = 200 mock_response.json.side_effect = ValueError("invalid json") mock_response.text = "<html>not json</html>" mock_post.return_value = mock_response with pytest.raises(MpesaApiException) as exc: client.post("/ok-but-not-json", json={}, headers={}) assert exc.value.error.error_code == "JSON_DECODE_ERROR" def test_get_json_decode_error_on_success(client): patch_target = get_patch_target(client, "get") with patch(patch_target) as mock_get: mock_response = Mock() mock_response.ok = True mock_response.status_code = 200 mock_response.json.side_effect = ValueError("invalid json") mock_response.text = "<html>not json</html>" mock_get.return_value = mock_response with pytest.raises(MpesaApiException) as exc: client.get("/ok-but-not-json") assert exc.value.error.error_code == "JSON_DECODE_ERROR"Also applies to: 144-157
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
mpesakit/http_client/mpesa_http_client.py(2 hunks)mpesakit/mpesa_client.py(1 hunks)pyproject.toml(1 hunks)tests/unit/http_client/test_mpesa_http_client.py(8 hunks)
| _session: Optional[requests.Session] = None | ||
|
|
||
| def __init__(self, env: str = "sandbox"): | ||
| """Initializes the MpesaHttpClient with the specified environment. | ||
| def __init__(self, env: str = "sandbox", use_session: bool = False): | ||
| """Initializes the MpesaHttpClient instance. | ||
| Args: | ||
| env (str): The environment to use, either 'sandbox' or 'production'. | ||
| Defaults to 'sandbox'. | ||
| env (str): The environment to connect to ('sandbox' or 'production'). | ||
| use_session (bool): Whether to use a persistent session. | ||
| """ | ||
| self.base_url = self._resolve_base_url(env) | ||
| if use_session: | ||
| self._session = requests.Session() | ||
| self._session.trust_env = False | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Close the Session to avoid resource leaks; support context management.
Provide an explicit close() and context manager to manage the persistent session.
Add these methods:
def close(self) -> None:
if self._session:
self._session.close()
def __enter__(self):
return self
def __exit__(self, exc_type, exc, tb):
self.close()Consider having MpesaClient expose/forward close() as well.
🤖 Prompt for AI Agents
In mpesakit/http_client/mpesa_http_client.py around lines 77 to 90, the
persistent requests.Session created when use_session=True is never closed and
the class lacks context-manager support; add a public close(self) method that
calls self._session.close() if present, and implement __enter__(self) returning
self and __exit__(self, exc_type, exc, tb) that calls close() to ensure sessions
are released; also update any MpesaClient wrapper to forward/Expose a close()
method so callers can explicitly close the client when not using a
with-statement.
| @retry( | ||
| retry=retry_if_exception_type(requests.exceptions.Timeout) | | ||
| retry_if_exception_type(requests.exceptions.ConnectionError)| | ||
| retry_if_exception_type(requests.exceptions.RequestException), | ||
| wait=wait_fixed(2), | ||
| stop=stop_after_attempt(3), | ||
| retry_error_callback=handle_retry_exception | ||
| ) | ||
| def post( |
There was a problem hiding this comment.
Overly broad retries; tighten to transient network errors and improve backoff/logging.
Retrying on RequestException will retry non-transient issues (e.g., InvalidURL). Restrict to Timeout and ConnectionError. Use exponential backoff with jitter and log before sleep.
Apply these diffs:
-from tenacity import (
- retry,
- stop_after_attempt,
- wait_fixed,
- retry_if_exception_type,
- RetryCallState
-)
+from tenacity import (
+ retry,
+ stop_after_attempt,
+ wait_random_exponential,
+ retry_if_exception_type,
+ RetryCallState,
+ before_sleep_log,
+) @retry(
- retry=retry_if_exception_type(requests.exceptions.Timeout) |
- retry_if_exception_type(requests.exceptions.ConnectionError)|
- retry_if_exception_type(requests.exceptions.RequestException),
- wait=wait_fixed(2),
+ retry=retry_if_exception_type(requests.exceptions.Timeout)
+ | retry_if_exception_type(requests.exceptions.ConnectionError),
+ wait=wait_random_exponential(multiplier=0.5, max=8),
stop=stop_after_attempt(3),
- retry_error_callback=handle_retry_exception
+ retry_error_callback=handle_retry_exception,
+ before_sleep=before_sleep_log(logger, logging.WARNING),
)Repeat the same change for the GET decorator below.
Also applies to: 127-135
🤖 Prompt for AI Agents
In mpesakit/http_client/mpesa_http_client.py around lines 96-104 (and repeat the
same change at 127-135), the retry decorator is too broad (includes
requests.exceptions.RequestException); restrict retries to only
requests.exceptions.Timeout and requests.exceptions.ConnectionError, replace the
fixed wait with an exponential backoff plus jitter (e.g., use
wait_exponential_jitter or compose wait_exponential with jitter) and add a
logging call before each retry/sleep to record the exception and attempt number;
update both the POST and GET decorator blocks accordingly and ensure
retry_error_callback remains wired.
| handle_request_error(response) | ||
|
|
||
| return response.json() | ||
|
|
There was a problem hiding this comment.
Handle JSON decode errors on successful responses.
response.json() can raise ValueError (e.g., 204 or text/plain). Convert to MpesaApiException.
Apply this diff to both POST and GET return paths:
- return response.json()
+ try:
+ return response.json()
+ except ValueError as e:
+ raise MpesaApiException(
+ MpesaError(
+ error_code="JSON_DECODE_ERROR",
+ error_message=str(e),
+ status_code=getattr(response, "status_code", None),
+ raw_response=getattr(response, "text", None),
+ )
+ ) from eAlso applies to: 160-162
🤖 Prompt for AI Agents
In mpesakit/http_client/mpesa_http_client.py around lines 123-126 (and similarly
160-162), the code calls response.json() directly which can raise ValueError for
non-JSON successful responses (e.g., 204 or text/plain); wrap the
response.json() call in a try/except ValueError and convert that into an
MpesaApiException that includes context (status code and response text or a
short message) so callers get a consistent error type; apply this change to both
the POST and GET return paths.
| dependencies = [ | ||
| "pydantic >=2.10.6,<3.0.0", | ||
| "pydantic[email] >=2.10.6,<3.0.0", | ||
| "requests >=2.32.3,<3.0.0", |
There was a problem hiding this comment.
Bump requests lower bound to 2.32.5 (security fix).
2.32.3 allows a vulnerable version. Use >=2.32.5 for CVE-2024-47081 fix.
Apply this diff:
- "requests >=2.32.3,<3.0.0",
+ "requests >=2.32.5,<3.0.0",Based on learnings
🤖 Prompt for AI Agents
In pyproject.toml at line 45, the requests dependency lower bound is pinned to
2.32.3 which is vulnerable; update the requirement to "requests >=2.32.5,<3.0.0"
to include the CVE-2024-47081 fix, then regenerate/update the lockfile (poetry
lock / pip-tools / your dependency manager) and run tests to ensure no
regressions.
Don’t call logging.basicConfig in a library module. This globally alters app logging. Remove basicConfig; keep module logger only. Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Migrating development branch
dev => developSummary by CodeRabbit
New Features
Refactor
Bug Fixes
Tests