Migrate Dev => Develop

[RafaelJohn9]

Migrating development branch dev => develop

Summary by CodeRabbit

• New Features

  • Optional persistent connections (session mode) with lifecycle management for improved performance.
  • Configurable automatic retry for transient failures with clearer error translation.

• Refactor

  • Unified request flows with consistent JSON parsing and standardized error mapping.

• Bug Fixes

  • Improved handling of timeouts, connection errors, non-2xx responses, and JSON decode failures.

• Tests

  • Expanded tests covering retry behavior and both session and non-session modes.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

📝 Walkthrough

Walkthrough

Introduces tenacity-based retry logic and centralized error handling to MpesaHttpClient, adds optional persistent requests.Session via a new use_session flag, updates MpesaClient to forward use_session, and expands tests to exercise session/non-session modes and retry behaviors. pyproject.toml dependency constraints updated.

Changes

Cohort / File(s)	Summary
HTTP client core mpesakit/http_client/mpesa_http_client.py	Adds tenacity retries for POST/GET with a shared retry policy and retry_error_callback; implements handle_request_error and handle_retry_exception; adds optional persistent requests.Session controlled by use_session; introduces _raw_post/_raw_get, session lifecycle methods (close, __enter__, __exit__), logging, and URL join usage.
Client wrapper mpesakit/mpesa_client.py	MpesaClient.__init__ signature extended to accept use_session: bool = False and forwards it to MpesaHttpClient.
Tests (HTTP client) tests/unit/http_client/test_mpesa_http_client.py	Parameterizes tests for session vs non-session modes; adds get_patch_target helper to patch correct request target; expands retry tests for POST/GET (success on retry, exhausted retries, non-retryable errors), and asserts mapping of HTTP/JSON errors to MpesaApiException.
Project metadata & tooling pyproject.toml	Updates dependency constraints and metadata: tenacity constraint adjusted (now <10.0.0), requests minimum increased (>=2.32.5,<3.0.0); reorganizes project URLs, adds maintainers block, and adjusts tooling (ruff/pytest/mypy) settings.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor Caller
  participant MpesaClient
  participant MpesaHttpClient
  participant Tenacity as "tenacity (retry)"
  participant Session as "requests.Session"
  participant Requests as "requests"

  Caller->>MpesaClient: post/get(path, payload, headers)
  MpesaClient->>MpesaHttpClient: post/get(...)

  alt use_session = true
    MpesaHttpClient->>Session: request(method, url, timeout)
  else use_session = false
    MpesaHttpClient->>Requests: request(method, url, timeout)
  end

  MpesaHttpClient->>Tenacity: apply retry policy
  Tenacity->>MpesaHttpClient: return response / raise on final attempt

  alt Response 2xx
    MpesaHttpClient-->>MpesaClient: parsed JSON
    MpesaClient-->>Caller: parsed JSON
  else Response non-2xx
    MpesaHttpClient->>MpesaHttpClient: handle_request_error(response)
    MpesaHttpClient-->>MpesaClient: raise MpesaApiException
    MpesaClient-->>Caller: error
  end

  opt Retries exhausted or transport error
    MpesaHttpClient->>MpesaHttpClient: handle_retry_exception(retry_state)
    MpesaHttpClient-->>MpesaClient: raise MpesaApiException (REQUEST_TIMEOUT / CONNECTION_ERROR / REQUEST_FAILED)
    MpesaClient-->>Caller: error
  end

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Poem

I hop with retries, tenacious and spry,
A session burrow where requests can lie.
Timeouts chased, exceptions turned to code,
I nibble JSON on the sunlit road.
Hooray—stable hops, and tests that glow! 🥕

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The pull request description is extremely minimal, containing only 'Migrating development branch dev => develop', which does not match the template structure or adequately document the substantial code changes present in the changeset.	Complete the description following the provided template, including a detailed description of changes, type of change, testing methodology, and relevant checklist items to communicate the purpose and scope of modifications.
Title check	❓ Inconclusive	The title 'Migrate Dev => Develop' is vague and does not clearly reflect the actual substantial changes in the pull request, which involve adding retry mechanisms, session management, and extensive test updates.	Revise the title to accurately describe the main changes, such as 'Add retry mechanism and session management to HTTP client' or similar language that reflects the actual code modifications.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	Docstring coverage is 92.86% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

---

🧹 Recent nitpick comments

pyproject.toml (1)

50-50: Minor: Inconsistent spacing in dependency specification.

Other dependencies use a space before the version specifier (e.g., "pydantic >=2.10.6"), but tenacity omits it. Consider aligning for consistency.

Suggested fix

-  "tenacity>=9.1.2,<10.0.0",
+  "tenacity >=9.1.2,<10.0.0",

tests/unit/http_client/test_mpesa_http_client.py (2)

93-105: Add assertion to verify no retry occurred.

The test name indicates the exception "is not retried," but there's no assertion verifying that the method was called only once. Consider adding a call count assertion to strengthen this test.

Suggested improvement

     with patch(
         patch_target,
         side_effect=requests.RequestException("boom"),
-    ):
+    ) as mock_post:
         with pytest.raises(MpesaApiException) as exc:
             client.post("/fail", json={}, headers={})

+        assert mock_post.call_count == 1  # Verify no retry occurred
         assert exc.value.error.error_code == "REQUEST_FAILED"

---

200-211: Add assertion to verify no retry occurred for GET.

Same as the POST test - add a call count assertion to verify the exception was not retried.

Suggested improvement

     with patch(
         patch_target,
         side_effect=requests.RequestException("boom"),
-    ):
+    ) as mock_get:
         with pytest.raises(MpesaApiException) as exc:
             client.get("/fail")

+        assert mock_get.call_count == 1  # Verify no retry occurred
         assert exc.value.error.error_code == "REQUEST_FAILED"

mpesakit/http_client/mpesa_http_client.py (2)

156-169: JSON decode errors on success use generic error code.

The previous review suggested using a specific JSON_DECODE_ERROR code for JSON parsing failures on successful responses. The current implementation catches ValueError but uses the generic REQUEST_FAILED code.

This is functional but loses specificity. If a 200 response has non-JSON body (e.g., text/plain), the error code will be REQUEST_FAILED rather than the more descriptive JSON_DECODE_ERROR.

Suggested improvement for more specific error handling

     response: requests.Response | None = None
     try:
         response = self._raw_post(url, json, headers, timeout)
         handle_request_error(response)
-        return response.json()
-    except (requests.RequestException, ValueError) as e:
+        try:
+            return response.json()
+        except ValueError as e:
+            raise MpesaApiException(
+                MpesaError(
+                    error_code="JSON_DECODE_ERROR",
+                    error_message=str(e),
+                    status_code=response.status_code,
+                    raw_response=response.text,
+                )
+            ) from e
+    except requests.RequestException as e:
         raise MpesaApiException(
             MpesaError(
                 error_code="REQUEST_FAILED",

---

178-193: GET method has hardcoded timeout unlike POST.

The _raw_post method accepts a timeout parameter (line 131), but _raw_get has a hardcoded timeout=10. For consistency and flexibility, consider making timeout configurable for GET as well.

Suggested fix

     `@retry`(
         retry=retry_enabled(enabled=True),
         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),
     )
     def _raw_get(
         self,
         url: str,
         params: Optional[Dict[str, Any]] = None,
         headers: Optional[Dict[str, str]] = None,
+        timeout: int = 10,
     ) -> requests.Response:
         """Low-level GET request - may raise requests exceptions."""
         if headers is None:
             headers = {}
         full_url = urljoin(self.base_url, url)
         if self._session:
             return self._session.get(
-                full_url, params=params, headers=headers, timeout=10
+                full_url, params=params, headers=headers, timeout=timeout
             )
         else:
-            return requests.get(full_url, params=params, headers=headers, timeout=10)
+            return requests.get(full_url, params=params, headers=headers, timeout=timeout)

And update the public get method signature accordingly.

---

📜 Recent review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f0e98b8 and 309adad.

📒 Files selected for processing (3)

• mpesakit/http_client/mpesa_http_client.py
• pyproject.toml
• tests/unit/http_client/test_mpesa_http_client.py

🔇 Additional comments (12)

pyproject.toml (1)

47-47: Requests version bump addresses security concern.

The lower bound has been correctly updated to >=2.32.5 which includes the CVE-2024-47081 fix. Good catch from the previous review.

tests/unit/http_client/test_mpesa_http_client.py (4)

107-128: LGTM!

Good test coverage for the retry success path. The test correctly verifies that transient Timeout exceptions trigger retries and eventually succeed on the third attempt.

---

131-147: LGTM!

Correctly tests that the retry mechanism exhausts all attempts and raises the appropriate CONNECTION_ERROR exception after 3 failed attempts.

---

214-245: LGTM!

Good symmetry with the POST retry tests. The GET retry tests correctly verify both successful retry recovery and max retry exhaustion scenarios with appropriate error codes.

---

23-28: The session method patching approach is valid and standard.

The current implementation correctly patches requests.Session.post and requests.Session.get at the class level, which is the appropriate and standard way to mock instance methods in tests. When self._session.post() is called, Python's method resolution chain finds the patched class method, making this approach reliable and widely used.

Each test properly isolates patches within context managers, preventing interference. Both session and non-session modes follow consistent patching patterns and pose no additional risk.

No refactoring needed.

mpesakit/http_client/mpesa_http_client.py (7)

24-24: LGTM!

The global logging.basicConfig call has been removed as suggested in the previous review. The module now correctly uses only a module-level logger.

---

27-47: LGTM!

The handle_request_error function correctly centralizes HTTP error handling with proper JSON parsing fallback. The error structure with HTTP_{status_code} error codes provides good diagnostics.

---

50-76: LGTM!

The retry error callback correctly maps exception types to appropriate error codes and uses proper exception chaining. The fallback for missing outcome handles edge cases gracefully.

---

79-97: LGTM with a note.

The factory pattern provides flexibility for future configurability. Currently retry_enabled(enabled=True) is hardcoded at the call sites (lines 124, 172). Consider making this configurable via the constructor if clients need to disable retries in certain scenarios (e.g., testing without mocking tenacity).

---

106-116: LGTM!

Session initialization with trust_env=False is a good security practice as it prevents unintended proxy usage from environment variables.

---

226-238: LGTM!

The session lifecycle management has been properly implemented as suggested in the previous review. The close() method, __enter__, and __exit__ allow proper resource cleanup using context managers.

---

123-129: Verify wait_random_exponential parameters.

With multiplier=5 and max=8, the exponential growth is immediately capped. The formula is random(0, min(multiplier * 2^attempt, max)):

• Attempt 1: random(0, min(10, 8)) = random(0, 8)
• Attempt 2: random(0, min(20, 8)) = random(0, 8)

This effectively becomes a fixed random wait of 0-8 seconds. If true exponential backoff is desired, consider:

• wait_random_exponential(multiplier=0.5, max=8) for slower growth
• Or wait_random_exponential(multiplier=1, max=30) for more headroom

If the current behavior (random 0-8s) is intentional, this is fine.

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.}

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[RafaelJohn9]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

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

📥 Commits

Reviewing files that changed from the base of the PR and between 1fbd53b and f0e98b8.

📒 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)

[RafaelJohn9]

@coderabbitai review .

[coderabbitai]

@RafaelJohn9: I'll trigger a review of the pull request.

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.