Replace aiohttp with httpx #39
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughMigrates the external API client from aiohttp to httpx, adds optional dependency injection for an Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App
participant Client as ChargeAmpsExternalClient
participant HTTP as httpx.AsyncClient
participant API as ChargeAmps API
note over Client: Construct with optional injected httpx.AsyncClient
App->>Client: request resource
Client->>Client: check token expiry under token_lock
alt token valid
Client->>HTTP: GET /resource (Authorization)
else token expired
Client->>HTTP: POST /refresh (refresh_token)
HTTP->>API: network
API-->>HTTP: 200 {access_token, ...}
HTTP-->>Client: Response
Client->>Client: update token, set Authorization
Client->>HTTP: retry original GET /resource
end
HTTP->>API: network
API-->>HTTP: 200 JSON
HTTP-->>Client: Response
Client-->>App: parsed data
App->>Client: shutdown()
Client->>HTTP: aclose() (if _owns_client)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
chargeamps/external.py (4)
63-74: Also handle network errors on refresh.Currently only HTTPStatusError is handled; transient network errors (DNS, timeouts) raise RequestError.
- except httpx.HTTPStatusError: + except (httpx.HTTPStatusError, httpx.RequestError): self._logger.warning("Token refresh failed") self._token = None self._refresh_token = None
80-93: Also handle network errors on login.Mirror refresh handling for resilience.
- except httpx.HTTPStatusError as exc: + except (httpx.HTTPStatusError, httpx.RequestError) as exc: self._logger.error("Login failed") self._token = None self._refresh_token = None self._token_expire = 0 raise exc
227-233: Bug: sending JSON string instead of empty object.
json="{}"serializes to a JSON string, not{}. Many APIs expect an empty object.- await self._put(request_uri, json="{}") + await self._put(request_uri, json={}) @@ - await self._put(request_uri, json="{}") + await self._put(request_uri, json={})
1-233: Update CLI exception handling to catch httpx.HTTPStatusErrorchargeamps/cli.py currently imports and catches aiohttp.ClientResponseError (around line 12 and the except at ~349) while the external client uses httpx; httpx errors will bypass the handler — change the except and ensure httpx is imported.
Suggested patch:
- from aiohttp.client_exceptions import ClientResponseError + import httpx + from aiohttp.client_exceptions import ClientResponseError- except ClientResponseError as exc: + except httpx.HTTPStatusError as exc: sys.stderr.write(str(exc))
🧹 Nitpick comments (5)
chargeamps/external.py (5)
3-11: Drop unused asyncio import.
asyncioisn’t referenced; the# noqacan go too.-import asyncio # noqa import logging import time
49-57: Refresh slightly early to avoid edge-expiry.A small skew reduces 401s due to clock drift.
- if self._token_expire > time.time(): + SKEW = 30 # seconds + if self._token_expire - SKEW > time.time(): return
98-107: Be defensive when decoding JWT exp and update headers atomically.Ensure
expis int and always refresh auth headers in one shot (keeps apiKey if used).- token_payload = jwt.decode(self._token, options={"verify_signature": False}) - self._token_expire = token_payload.get("exp", 0) - - self._headers["Authorization"] = f"Bearer {self._token}" + token_payload = jwt.decode(self._token, options={"verify_signature": False}) + exp = token_payload.get("exp", 0) + self._token_expire = int(exp) if isinstance(exp, (int, float)) else 0 + + self._headers = {**self._headers, "Authorization": f"Bearer {self._token}"}
108-134: Optional: serialize default headers once into the client.If you consistently need
apiKeyandAuthorization, consider creating the client withheaders=self._headersand updating viaself._httpx_client.headers.update(...)after login. This reduces per-call header merging.
80-93: Add a lightweight token-refresh lock to avoid concurrent refresh storms.Multiple concurrent requests can race into refresh/login. A single
asyncio.Lockaround the critical section prevents duplicate work.# at __init__ self._token_lock = asyncio.Lock() # wrap core of _ensure_token async with self._token_lock: # existing logic...
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
chargeamps/external.py(9 hunks)pyproject.toml(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-12-17T22:36:20.497Z
Learnt from: yarcod
PR: kirei/python-chargeamps#30
File: chargeamps/external.py:218-223
Timestamp: 2024-12-17T22:36:20.497Z
Learning: In `chargeamps/external.py`, ensure that docstrings and error handling remain consistent with the rest of the module, avoiding the addition of detailed documentation or extensive error handling to individual functions unless it's applied uniformly.
Applied to files:
chargeamps/external.py
🧬 Code graph analysis (1)
chargeamps/external.py (4)
chargeamps/base.py (5)
shutdown(28-29)get_chargepoints(32-34)get_chargepoint_status(37-39)get_chargepoint_settings(42-44)ChargeAmpsClient(26-63)tests/test_client.py (1)
test_external_client(7-10)examples/test.py (1)
test(7-29)chargeamps/cli.py (1)
main_loop(175-358)
🔇 Additional comments (7)
pyproject.toml (1)
24-30: Do not remove aiohttp — it's still imported/used; migrate usages before dropping the dependency.Evidence:
- chargeamps/cli.py:12 —
from aiohttp.client_exceptions import ClientResponseError- pyproject.toml:24 —
"aiohttp>=3.12.15"- uv.lock contains aiohttp package entries
Either migrate the CLI usage to httpx (update exception handling) and then remove aiohttp + regenerate the lockfile, or keep aiohttp.
Likely an incorrect or invalid review comment.
chargeamps/external.py (6)
140-142: LGTM — response parsing to models is correct.
159-161: LGTM — session list parsing looks good.
169-171: LGTM — single session payload handling is fine.
176-178: LGTM — status payload handling is fine.
183-185: LGTM — settings payload handling is fine.
201-203: LGTM — connector settings payload handling is fine.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
chargeamps/external.py (1)
239-243: json="{}" sends a JSON string, not an object — use {}Passing a string results in ""{}"" on the wire. Use an empty object.
- await self._put(request_uri, json="{}") + await self._put(request_uri, json={})- await self._put(request_uri, json="{}") + await self._put(request_uri, json={})Also applies to: 244-248
🧹 Nitpick comments (5)
chargeamps/external.py (5)
63-74: Auth flow solid; consider small expiry skew to reduce edge 401sSubtract a small skew from exp to avoid just-expired tokens triggering retries.
- self._token_expire = token_payload.get("exp", 0) + # Add slight skew to avoid edge-of-expiry 401s + self._token_expire = max(0, int(token_payload.get("exp", 0)) - 30)Also applies to: 80-96, 101-110
126-133: Drop duplicate raise_for_status in callers_httpx_retry already raises; callers can just return its result.
- response = await self._httpx_retry(self._httpx_client.post, url, headers, **kwargs) - response.raise_for_status() - return response + return await self._httpx_retry(self._httpx_client.post, url, headers, **kwargs)- response = await self._httpx_retry(self._httpx_client.get, url, headers, **kwargs) - response.raise_for_status() - return response + return await self._httpx_retry(self._httpx_client.get, url, headers, **kwargs)- response = await self._httpx_retry(self._httpx_client.put, url, headers, **kwargs) - response.raise_for_status() - return response + return await self._httpx_retry(self._httpx_client.put, url, headers, **kwargs)Also applies to: 134-141, 142-149
153-157: Use comprehensions to simplify list buildingEquivalent and more concise.
- res = [] - for chargepoint in response.json(): - res.append(ChargePoint.model_validate(chargepoint)) - return res + return [ChargePoint.model_validate(cp) for cp in response.json()]- res = [] - for session in response.json(): - res.append(ChargingSession.model_validate(session)) - return res + return [ChargingSession.model_validate(s) for s in response.json()]Also applies to: 173-176
211-217: Fix misleading docstringsThese methods operate on connector settings, not “owned chargepoints.”
- """Get all owned chargepoints""" + """Get connector settings"""- """Get all owned chargepoints""" + """Set connector settings"""Also applies to: 219-229
244-248: Add type hint for reboot parameterMinor consistency polish.
- async def reboot(self, charge_point_id) -> None: + async def reboot(self, charge_point_id: str) -> None:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
chargeamps/external.py(9 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
chargeamps/external.py (2)
chargeamps/base.py (5)
shutdown(28-29)get_chargepoints(32-34)get_chargepoint_status(37-39)get_chargepoint_settings(42-44)ChargeAmpsClient(26-63)chargeamps/models.py (4)
ChargePoint(30-38)ChargingSession(79-86)ChargePointStatus(58-61)ChargePointSettings(64-67)
🔇 Additional comments (2)
chargeamps/external.py (2)
3-9: httpx migration imports — OKImports look correct for the switch to httpx.
32-41: Client DI, ownership tracking, and sane timeouts — LGTMNice DI, ownership flag, and conditional aclose().
Also applies to: 48-51
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
chargeamps/external.py (2)
218-225: Docstrings are incorrect for connector settings APIsBoth functions say “Get all owned chargepoints”; should describe connector settings.
Apply:
async def get_chargepoint_connector_settings( self, charge_point_id: str, connector_id: int ) -> ChargePointConnectorSettings: - """Get all owned chargepoints""" + """Get connector settings for a charge point""" async def set_chargepoint_connector_settings( self, settings: ChargePointConnectorSettings ) -> None: - """Get all owned chargepoints""" + """Update connector settings for a charge point"""Also applies to: 226-236
249-255: Sending JSON string instead of empty JSON objectUsing json="{}" serializes to the string literal "{}". These endpoints likely expect an empty object. Also add typing to reboot().
Apply:
async def remote_stop(self, charge_point_id: str, connector_id: int) -> None: """Remote stop chargepoint""" request_uri = f"/api/{API_VERSION}/chargepoints/{charge_point_id}/connectors/{connector_id}/remotestop" - await self._put(request_uri, json="{}") + await self._put(request_uri, json={}) -async def reboot(self, charge_point_id) -> None: +async def reboot(self, charge_point_id: str) -> None: """Reboot chargepoint""" request_uri = f"/api/{API_VERSION}/chargepoints/{charge_point_id}/reboot" - await self._put(request_uri, json="{}") + await self._put(request_uri, json={})
♻️ Duplicate comments (3)
chargeamps/external.py (3)
39-43: Client ownership + sane timeouts — LGTMGood call tracking ownership and setting conservative httpx timeouts.
51-54: Close only when owned — LGTMPrevents surprising callers that inject their own AsyncClient.
118-132: 401 retry path: missing await and stale Authorization on second attemptSecond call isn’t awaited and headers aren’t rebuilt to include the refreshed Bearer token.
Apply:
async def _httpx_retry(self, method, url, headers, **kwargs) -> httpx.Response: try: response = await method(url, headers=headers, **kwargs) response.raise_for_status() return response except httpx.HTTPStatusError as exc: if exc.response.status_code == 401: self._token = None self._token_expire = 0 await self._ensure_token() - response = method(url, headers=headers, **kwargs) + # Ensure refreshed Authorization is used; keep caller headers. + # Prefer our Authorization over any stale one. + cleaned = {k: v for k, v in headers.items() if k.lower() != "authorization"} + headers = {**cleaned, **self._headers} + response = await method(url, headers=headers, **kwargs) response.raise_for_status() return response raise
🧹 Nitpick comments (2)
chargeamps/external.py (2)
56-66: Avoid redundant refresh/login with double-checked lockingRe-check validity inside the lock to prevent queued callers from repeating refresh/login work.
Apply:
async def _ensure_token(self) -> None: - if self._token_expire - self._token_skew > time.time(): - return - if self._token is None: - self._logger.info("Token not found") - elif self._token_expire > 0: - self._logger.info("Token expired") - - async with self._token_lock: - await self._exclusive_ensure_token() + if self._token_expire - self._token_skew > time.time(): + return + if self._token is None: + self._logger.info("Token not found") + elif self._token_expire > 0: + self._logger.info("Token expired") + async with self._token_lock: + # Another task may have refreshed while we awaited the lock. + if self._token_expire - self._token_skew > time.time(): + return + await self._exclusive_ensure_token()
133-139: Drop duplicate raise_for_status() in callers_httpx_retry already raises on non-2xx. These extra calls are redundant.
Apply:
async def _post(self, path, **kwargs) -> httpx.Response: await self._ensure_token() headers = {**self._headers, **kwargs.pop("headers", {})} url = urljoin(self._base_url, path) - response = await self._httpx_retry(self._httpx_client.post, url, headers, **kwargs) - response.raise_for_status() - return response + return await self._httpx_retry(self._httpx_client.post, url, headers, **kwargs) async def _get(self, path, **kwargs) -> httpx.Response: await self._ensure_token() headers = {**self._headers, **kwargs.pop("headers", {})} url = urljoin(self._base_url, path) - response = await self._httpx_retry(self._httpx_client.get, url, headers, **kwargs) - response.raise_for_status() - return response + return await self._httpx_retry(self._httpx_client.get, url, headers, **kwargs) async def _put(self, path, **kwargs) -> httpx.Response: await self._ensure_token() headers = {**self._headers, **kwargs.pop("headers", {})} url = urljoin(self._base_url, path) - response = await self._httpx_retry(self._httpx_client.put, url, headers, **kwargs) - response.raise_for_status() - return response + return await self._httpx_retry(self._httpx_client.put, url, headers, **kwargs)Also applies to: 141-147, 149-155
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
chargeamps/external.py(8 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-12-17T22:36:20.497Z
Learnt from: yarcod
PR: kirei/python-chargeamps#30
File: chargeamps/external.py:218-223
Timestamp: 2024-12-17T22:36:20.497Z
Learning: In `chargeamps/external.py`, ensure that docstrings and error handling remain consistent with the rest of the module, avoiding the addition of detailed documentation or extensive error handling to individual functions unless it's applied uniformly.
Applied to files:
chargeamps/external.py
🧬 Code graph analysis (1)
chargeamps/external.py (5)
chargeamps/base.py (5)
shutdown(28-29)get_chargepoints(32-34)get_chargepoint_status(37-39)get_chargepoint_settings(42-44)ChargeAmpsClient(26-63)chargeamps/models.py (4)
ChargePoint(30-38)ChargingSession(79-86)ChargePointStatus(58-61)ChargePointSettings(64-67)tests/test_client.py (1)
test_external_client(7-10)examples/test.py (1)
test(7-29)chargeamps/local.py (1)
ChargeAmpsLocalClient(6-7)
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
chargeamps/external.py (1)
245-251: Fix JSON body type for empty objects (send {} not "{}")json="{}" sends a JSON string; change to json={} to send an empty object.
File: chargeamps/external.py — replace at lines 245 and 250:
@@ async def remote_stop(self, charge_point_id: str, connector_id: int) -> None: """Remote stop chargepoint""" request_uri = f"/api/{API_VERSION}/chargepoints/{charge_point_id}/connectors/{connector_id}/remotestop" - await self._put(request_uri, json="{}") + await self._put(request_uri, json={}) @@ async def reboot(self, charge_point_id) -> None: """Reboot chargepoint""" request_uri = f"/api/{API_VERSION}/chargepoints/{charge_point_id}/reboot" - await self._put(request_uri, json="{}") + await self._put(request_uri, json={})
🧹 Nitpick comments (6)
chargeamps/external.py (6)
108-116: Harden token parsing; tolerate refresh responses without refreshToken.Avoid KeyError if refreshToken isn’t returned; also coerce exp to int.
- response_payload = response.json() - - self._token = response_payload["token"] - self._refresh_token = response_payload["refreshToken"] - - token_payload = jwt.decode(self._token, options={"verify_signature": False}) - self._token_expire = token_payload.get("exp", 0) + response_payload = response.json() + self._token = response_payload["token"] + self._refresh_token = response_payload.get("refreshToken", self._refresh_token) + token_payload = jwt.decode(self._token, options={"verify_signature": False}) + self._token_expire = int(token_payload.get("exp", 0))
104-107: 不要 silently continue when no response; raise.If both refresh/login failed to produce a response, return leaves client unusable. Prefer a clear error.
- if response is None: - self._logger.error("No response") - return + if response is None: + raise RuntimeError("Token acquisition failed: no response from login/refresh")
39-43: Add connection limits to AsyncClient to avoid unbounded concurrency.httpx defaults allow many concurrent connections; set conservative limits.
- self._httpx_client = httpx_client or httpx.AsyncClient( - timeout=httpx.Timeout(connect=5.0, read=15.0, write=15.0, pool=5.0) - ) + self._httpx_client = httpx_client or httpx.AsyncClient( + timeout=httpx.Timeout(connect=5.0, read=15.0, write=15.0, pool=5.0), + limits=httpx.Limits(max_connections=20, max_keepalive_connections=20), + )
214-231: Correct method docstrings (copy-paste).Docstrings mention “Get all owned chargepoints” but these operate on connector settings.
@@ - ) -> ChargePointConnectorSettings: - """Get all owned chargepoints""" + ) -> ChargePointConnectorSettings: + """Get connector settings for a specific charge point connector""" @@ - ) -> None: - """Get all owned chargepoints""" + ) -> None: + """Set connector settings for a specific charge point connector"""
153-160: Tighten loops into comprehensions.Minor readability win.
@@ - res = [] - for chargepoint in response.json(): - res.append(ChargePoint.model_validate(chargepoint)) - return res + return [ChargePoint.model_validate(cp) for cp in response.json()] @@ - res = [] - for session in response.json(): - res.append(ChargingSession.model_validate(session)) - return res + return [ChargingSession.model_validate(s) for s in response.json()]Also applies to: 177-179
247-247: Add missing type hint.Annotate charge_point_id for consistency.
- async def reboot(self, charge_point_id) -> None: + async def reboot(self, charge_point_id: str) -> None:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
chargeamps/external.py(8 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-13T08:41:26.874Z
Learnt from: jschlyter
PR: kirei/python-chargeamps#39
File: chargeamps/external.py:43-43
Timestamp: 2025-09-13T08:41:26.874Z
Learning: In the Charge Amps External API v5, the `apiKey` header is only required for authentication endpoints (/login and /refreshToken), not for general API endpoints like chargepoints and settings which use Bearer token authorization.
Applied to files:
chargeamps/external.py
🧬 Code graph analysis (1)
chargeamps/external.py (4)
chargeamps/base.py (5)
shutdown(28-29)get_chargepoints(32-34)get_chargepoint_status(37-39)get_chargepoint_settings(42-44)ChargeAmpsClient(26-63)chargeamps/models.py (4)
ChargePoint(30-38)ChargingSession(79-86)ChargePointStatus(58-61)ChargePointSettings(64-67)tests/test_client.py (1)
test_external_client(7-10)examples/test.py (1)
test(7-29)
🔇 Additional comments (2)
chargeamps/external.py (2)
73-79: apiKey only on auth endpoints — correct.Headers for refresh/login include apiKey while regular calls use Bearer. Matches the External API behavior.
Also applies to: 90-96
51-53: Client ownership, token lock, and 401 retry look solid.
- Only closing owned client is correct.
- Token operations are serialized with asyncio.Lock.
- Single 401 refresh+retry with rebuilt headers is implemented correctly.
Also applies to: 118-133, 135-152
Summary by CodeRabbit
New Features
Refactor
Chores