feat: Allow passing external aiohttp.ClientSession to OpenEVSE#508
feat: Allow passing external aiohttp.ClientSession to OpenEVSE#508
Conversation
…enEVSEWebsocket for improved session management and lifecycle control.
📝 WalkthroughWalkthroughThis PR adds optional external Changes
Sequence DiagramsequenceDiagram
actor User
participant OpenEVSE
participant aiohttp.ClientSession
participant WebSocket
participant TargetServer
rect rgba(100, 150, 200, 0.5)
Note over User,TargetServer: With external session
User->>aiohttp.ClientSession: create session
User->>OpenEVSE: __init__(host, session=my_session)
OpenEVSE->>OpenEVSE: store session, set _session_external = True
User->>OpenEVSE: update()
OpenEVSE->>aiohttp.ClientSession: send HTTP request
aiohttp.ClientSession->>TargetServer: HTTP GET/POST
TargetServer-->>aiohttp.ClientSession: response
aiohttp.ClientSession-->>OpenEVSE: data
OpenEVSE-->>User: status updated
User->>OpenEVSE: ws_disconnect()
OpenEVSE->>WebSocket: close (do not close external session)
User->>aiohttp.ClientSession: close (user responsibility)
end
rect rgba(150, 100, 200, 0.5)
Note over User,TargetServer: Without external session
User->>OpenEVSE: __init__(host)
OpenEVSE->>OpenEVSE: session=None, _session_external=False
User->>OpenEVSE: update()
OpenEVSE->>aiohttp.ClientSession: create temporary session
aiohttp.ClientSession->>TargetServer: HTTP GET/POST
TargetServer-->>aiohttp.ClientSession: response
aiohttp.ClientSession-->>OpenEVSE: data
OpenEVSE->>aiohttp.ClientSession: close temporary session
OpenEVSE-->>User: status updated
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Pylint (4.0.4)openevsehttp/websocket.pyopenevsehttp/__main__.pytests/test_main.py
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: 3
🤖 Fix all issues with AI agents
In `@example_external_session.py`:
- Line 13: The example is importing OpenEVSE from the non-public module
openevsehttp.__main__; update the import to use the public package root by
replacing the import line to import OpenEVSE from openevsehttp (e.g. change
"from openevsehttp.__main__ import OpenEVSE" to "from openevsehttp import
OpenEVSE") so the example uses the documented public entrypoint.
In `@EXTERNAL_SESSION.md`:
- Around line 74-105: The API headings use underscores which the markdown linter
treats as strong-style; update the two headings by wrapping the function names
in inline code spans instead of bolding—replace the bolded "__init__"
occurrences with `OpenEVSE.__init__` and `OpenEVSEWebsocket.__init__` (or just
`__init__` inside the respective section) so the headings use backticks for the
function names and the MD050 lint error is resolved.
In `@tests/test_external_session.py`:
- Around line 130-132: The test function name
test_firmware_check_with_external_session is misleading because the test does
not pass a session; rename the test function to reflect that it does not use an
external session (e.g., test_firmware_check_without_session or
test_firmware_check_uses_internal_session) and update any references/imports or
test markers accordingly so the name matches the test behavior; locate the
function definition test_firmware_check_with_external_session in
tests/test_external_session.py and perform the rename there and in any calls or
fixtures that reference it.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openevsehttp/__main__.py (1)
124-233: 🛠️ Refactor suggestion | 🟠 MajorSignificant code duplication between external and internal session paths.
The
if self._session:andelse:branches contain nearly identical logic (~100 lines duplicated). This creates a maintenance burden where any future changes must be applied in two places.Consider extracting the common request logic into a helper method:
♻️ Proposed refactor to reduce duplication
+ async def _execute_request( + self, + session: aiohttp.ClientSession, + url: str, + method: str, + data: Any, + rapi: Any, + auth: aiohttp.BasicAuth | None, + ) -> dict[str, str] | dict[str, Any]: + """Execute HTTP request with the given session.""" + http_method = getattr(session, method) + _LOGGER.debug( + "Connecting to %s with data: %s rapi: %s using method %s", + url, data, rapi, method, + ) + try: + async with http_method(url, data=rapi, json=data, auth=auth) as resp: + try: + message = await resp.text() + except UnicodeDecodeError: + _LOGGER.debug("Decoding error") + message = await resp.read() + message = message.decode(errors="replace") + + try: + message = json.loads(message) + except ValueError: + _LOGGER.warning("Non JSON response: %s", message) + + if resp.status == 400: + index = "" + if "msg" in message.keys(): + index = "msg" + elif "error" in message.keys(): + index = "error" + _LOGGER.error("Error 400: %s", message[index]) + raise ParseJSONError + if resp.status == 401: + _LOGGER.error("Authentication error: %s", message) + raise AuthenticationError + if resp.status in [404, 405, 500]: + _LOGGER.warning("%s", message) + + if method == "post" and "config_version" in message: + await self.update() + return message + + except (TimeoutError, ServerTimeoutError) as err: + _LOGGER.error("%s: %s", ERROR_TIMEOUT, url) + raise + except ContentTypeError as err: + _LOGGER.error("Content error: %s", err.message) + raise async def process_request( self, url: str, method: str = "", data: Any = None, rapi: Any = None, ) -> dict[str, str] | dict[str, Any]: """Return result of processed HTTP request.""" auth = None if method is None: raise MissingMethod if self._user and self._pwd: auth = aiohttp.BasicAuth(self._user, self._pwd) - # Use provided session or create a temporary one - if self._session: - session = self._session - # ... 50+ lines of duplicated code ... - else: - async with aiohttp.ClientSession() as session: - # ... 50+ lines of duplicated code ... + if self._session: + return await self._execute_request( + self._session, url, method, data, rapi, auth + ) + else: + async with aiohttp.ClientSession() as session: + return await self._execute_request( + session, url, method, data, rapi, auth + )
🤖 Fix all issues with AI agents
In `@openevsehttp/__main__.py`:
- Around line 232-233: The final await session.close() and return message are
unreachable because the try block always returns (and exceptions are re-raised);
fix by ensuring the session is closed and the message returned via a finally or
context manager: move session cleanup into a finally block (or use "async with"
for the session) and perform the return from after the try/finally so that
"session.close()" (reference the session variable) always runs and "message" is
returned reliably instead of being skipped by the early return in the try block.
In `@tests/test_external_session.py`:
- Around line 20-48: The test function test_external_session_provided has an
unused fixture parameter mock_aioclient; remove mock_aioclient from the function
signature so the test only accepts parameters it uses (i.e., change async def
test_external_session_provided(mock_aioclient): to async def
test_external_session_provided():) and run the tests to confirm no other
fixtures depend on it; ensure references to mock_aioclient are not used
elsewhere in that test.
In `@tests/test_main.py`:
- Around line 2576-2582: The test function test_firmware_check_no_config
currently declares an unused fixture parameter mock_aioclient; remove the unused
parameter from the test signature so it becomes async def
test_firmware_check_no_config(): to avoid lingering unused fixtures, keeping the
test focused on calling OpenEVSE(SERVER_URL).firmware_check() without HTTP
interactions.
🧹 Nitpick comments (1)
openevsehttp/__main__.py (1)
639-674: Similar code duplication in firmware_check.The same duplication pattern exists here between the external session path (lines 639-656) and internal session path (lines 657-674). Consider applying the same refactoring pattern.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/test_main.py`:
- Around line 3223-3226: The side_effect coroutine defines unused parameters
`data` and `error` which triggers the linter; rename them to `_data` and
`_error` (or prefix with underscores) in the async def signature for side_effect
to silence Ruff ARG001 while leaving the body unchanged where it sets ws._state
= "stopped" and references ws._state.
🧹 Nitpick comments (2)
openevsehttp/__main__.py (1)
124-230: Consider factoring duplicated request handling.
Line 124 onward repeats nearly identical request/response logic for external vs internal sessions; extracting a small helper would reduce drift and keep error handling consistent (and can be reused by thefirmware_checkflow).tests/test_main.py (1)
2890-2894: Drop duplicate pytestmark/constants.
Line 2890 redefines values already declared at the top of the file; this is redundant and risks drift.♻️ Proposed cleanup
-pytestmark = pytest.mark.asyncio -SERVER_URL = "openevse.test.tld" -TEST_URL_STATUS = "http://openevse.test.tld/status"
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
openevsehttp/__main__.py (2)
124-230: 🛠️ Refactor suggestion | 🟠 MajorSignificant code duplication between external and internal session branches.
The entire request handling logic (~100 lines) is copy-pasted between the
if self._sessionandelsebranches. The only difference is whether the session isself._sessionor created viaasync with aiohttp.ClientSession(). This violates DRY and makes maintenance error-prone—any bug fix or behavior change must be applied in both places.Consider extracting the common logic into a helper, or restructuring to conditionally create/use the session before a single request-handling block:
♻️ Suggested approach
async def process_request(self, url, method="", data=None, rapi=None): """Return result of processed HTTP request.""" auth = None if method is None: raise MissingMethod if self._user and self._pwd: auth = aiohttp.BasicAuth(self._user, self._pwd) - if self._session: - session = self._session - # ... ~50 lines of request logic ... - else: - async with aiohttp.ClientSession() as session: - # ... same ~50 lines duplicated ... + if self._session is not None: + return await self._do_request(self._session, url, method, data, rapi, auth) + else: + async with aiohttp.ClientSession() as session: + return await self._do_request(session, url, method, data, rapi, auth)Then move the shared logic into
_do_request(self, session, url, method, data, rapi, auth).
636-671: 🛠️ Refactor suggestion | 🟠 MajorSame duplication pattern in
firmware_check.The GitHub API call logic is duplicated between the external and internal session branches. Apply the same extract-helper approach suggested for
process_request.
🤖 Fix all issues with AI agents
In `@openevsehttp/__main__.py`:
- Around line 106-107: The code sets self._session_external = session is not
None but later checks session truthiness with "if self._session:", causing
inconsistent semantics; find the conditional that currently reads "if
self._session:" and change it to "if self._session is not None" so the presence
check matches how self._session_external is initialized (refer to the attributes
self._session and self._session_external in __main__.py and update the
conditional that inspects self._session accordingly).
In `@openevsehttp/websocket.py`:
- Around line 37-38: The None-checks are inconsistent: change the session
assignment in the constructor to use an identity check like the external flag so
a falsy-but-non-None session is preserved; update the line that sets
self.session to use "session if session is not None else
aiohttp.ClientSession()" (matching the existing _session_external = session is
not None) so the behavior of self.session and _session_external stays consistent
(look for the constructor/initializer where self.session and
self._session_external are set).
In `@tests/test_main.py`:
- Around line 2890-2893: Remove the duplicate module-level redefinitions of
pytestmark, SERVER_URL, and TEST_URL_STATUS: locate the redundant block that
reassigns pytestmark = pytest.mark.asyncio and SERVER_URL/TEST_URL_STATUS to the
same values, and delete that duplicate block so only the original definitions
(the ones earlier in the module) remain; ensure no other references rely on the
second definitions and run tests to confirm.
🧹 Nitpick comments (2)
tests/test_main.py (2)
3170-3170: Unusederrorparameter triggers Ruff ARG001.Prefix with underscore for consistency with the fix at line 3223.
Proposed fix
- async def side_effect(msgtype, data, error): + async def side_effect(msgtype, data, _error):
2672-2888: Consider consolidating external session error-handling tests.Tests
test_external_session_*(lines 2672–2888) duplicate the same error scenarios already covered bytest_process_request_*(lines 2349–2526) but with an external session. While this provides coverage for both code paths, it reinforces the point that extracting the shared request logic inprocess_requestwould eliminate the need for duplicate test suites.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
openevsehttp/__main__.py (2)
124-230: 🛠️ Refactor suggestion | 🟠 MajorSignificant code duplication in
process_request— extract session acquisition instead.The entire request handling logic (lines 135–177 vs 188–230) is duplicated verbatim across the two branches. The only difference is how the session is obtained. This violates DRY and doubles the maintenance surface for any future bug fix or feature change.
♻️ Proposed refactor to eliminate duplication
async def process_request( self, url: str, method: str = "", data: Any = None, rapi: Any = None, ) -> dict[str, str] | dict[str, Any]: """Return result of processed HTTP request.""" auth = None if method is None: raise MissingMethod if self._user and self._pwd: auth = aiohttp.BasicAuth(self._user, self._pwd) - # Use provided session or create a temporary one - if self._session is not None: - session = self._session - http_method = getattr(session, method) - ... # ~50 duplicated lines - else: - async with aiohttp.ClientSession() as session: - http_method = getattr(session, method) - ... # ~50 duplicated lines + # Use provided session or create a temporary one + owned_session = None + session = self._session + if session is None: + owned_session = aiohttp.ClientSession() + session = owned_session + + try: + http_method = getattr(session, method) + _LOGGER.debug( + "Connecting to %s with data: %s rapi: %s using method %s", + url, data, rapi, method, + ) + try: + async with http_method( + url, data=rapi, json=data, auth=auth, + ) as resp: + try: + message = await resp.text() + except UnicodeDecodeError: + _LOGGER.debug("Decoding error") + message = await resp.read() + message = message.decode(errors="replace") + + try: + message = json.loads(message) + except ValueError: + _LOGGER.warning("Non JSON response: %s", message) + + if resp.status == 400: + index = "" + if "msg" in message.keys(): + index = "msg" + elif "error" in message.keys(): + index = "error" + _LOGGER.error("Error 400: %s", message[index]) + raise ParseJSONError + if resp.status == 401: + _LOGGER.error("Authentication error: %s", message) + raise AuthenticationError + if resp.status in [404, 405, 500]: + _LOGGER.warning("%s", message) + + if method == "post" and "config_version" in message: + await self.update() + return message + + except (TimeoutError, ServerTimeoutError) as err: + _LOGGER.error("%s: %s", ERROR_TIMEOUT, url) + raise err + except ContentTypeError as err: + _LOGGER.error("Content error: %s", err.message) + raise err + finally: + if owned_session is not None: + await owned_session.close()
636-671: 🛠️ Refactor suggestion | 🟠 MajorSame duplication problem in
firmware_check.The two branches (lines 636–653 vs 654–671) are identical aside from session acquisition. The same refactoring pattern (acquire-or-create +
try/finally) would eliminate this duplication.
🧹 Nitpick comments (1)
tests/test_main.py (1)
2671-2886: External session test variants mirror production code duplication.These tests (e.g.,
test_external_session_400_error_with_msgvstest_process_request_400_error_with_msg) duplicate assertions for every error path across both session modes. If the productionprocess_requestis refactored to eliminate branching (as suggested), these paired tests can be consolidated with a@pytest.mark.parametrizeoversession=Nonevs an external session, cutting ~200 lines.
1 participant
potentially replaces #505
Summary by CodeRabbit
New Features
Documentation
Tests
Style