Further refactoring #70
Conversation
|
Warning Rate limit exceeded@CoMPaTech has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 21 minutes and 3 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughCentralizes HTTP handling in airos/airos8.py via a new _request_json, removes the ApiResponse NamedTuple, adds CSRF/cookie and connected tracking, changes login() to return None, adds update_check(), adapts methods/tests to the new flow, and bumps version to 0.4.1a1. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant AirOS
participant Session as aiohttp.ClientSession
participant Device
Caller->>AirOS: login()
AirOS->>AirOS: _request_json(POST /api/auth/login)
AirOS->>Session: request(POST, /api/auth/login, headers, data/json)
Session->>Device: HTTP request
Device-->>Session: HTTP response
Session-->>AirOS: Response
AirOS->>AirOS: parse JSON, _store_auth_data (Set-Cookie, X-CSRF-ID), connected=True
AirOS-->>Caller: None
Caller->>AirOS: status() / update_check() / other
AirOS->>AirOS: _request_json(method, path, headers incl. auth, data/json)
AirOS->>Session: request(...)
Session->>Device: HTTP request
Device-->>Session: HTTP response
Session-->>AirOS: Response
AirOS-->>Caller: Parsed JSON or mapped AirOS error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🔭 Outside diff range comments (1)
airos/airos8.py (1)
47-56: Bug: Port is dropped when host includes scheme and port.Using parsed_host.hostname discards the port (e.g., http://192.168.1.3:8080 becomes http://192.168.1.3). Use netloc to preserve host:port.
- parsed_host = urlparse(host) - scheme = ( - parsed_host.scheme - if parsed_host.scheme - else ("https" if use_ssl else "http") - ) - hostname = parsed_host.hostname if parsed_host.hostname else host - - self.base_url = f"{scheme}://{hostname}" + parsed_host = urlparse(host) + scheme = parsed_host.scheme or ("https" if use_ssl else "http") + # Preserve port if present; fall back gracefully for schemeless hosts + if parsed_host.netloc: + netloc = parsed_host.netloc + elif parsed_host.hostname: + netloc = parsed_host.hostname + else: + netloc = host + self.base_url = f"{scheme}://{netloc}"
🧹 Nitpick comments (12)
tests/test_airos8.py (4)
14-33: Avoid disabling tests with triple-quoted strings; use pytest skip markers instead.Triple-quoted blocks leave dead code and require linter suppressions. Prefer marking tests as skipped to keep them discoverable/executable context-aware.
- Use @pytest.mark.skip(reason="...") above each test (or pytest.param(..., marks=pytest.mark.skip(...)) for parametrized cases).
- Or gate with @pytest.mark.skipif(...) based on feature flags.
This improves readability, tooling (pytest -k), and avoids pointless-string-statement suppressions.
Also applies to: 55-71, 152-166, 188-235
162-164: Remove the stray second docstring inside the test function.This is a no-op string literal and can trigger linter warnings. Keep a single docstring at the top or convert the note to a comment.
- """Test device operation using the new _request_json method.""" + # Test device operation using the new _request_json method.
188-191: Remove unused cookie setup.This dead code is not used by the test and can confuse readers.
- cookie = SimpleCookie() - cookie["session_id"] = "test-cookie" - cookie["AIROS_TOKEN"] = "abc123"
346-347: current_csrf_token is no longer used; avoid setting it in tests.The new flow uses internal _csrf_id for headers. Since you don't assert headers, just remove this line to avoid implying behavior that isn't used.
- airos_device.current_csrf_token = "mock-csrf-token"tests/test_stations.py (2)
162-164: Remove redundant docstring literal in test body.It's a no-op and can trigger linter warnings; convert to a comment.
- """Test device operation using the new _request_json method.""" + # Test device operation using the new _request_json method.
188-191: Remove unused cookie fixture remnants.They are not used by this test and can be dropped to reduce noise.
- cookie = SimpleCookie() - cookie["session_id"] = "test-cookie" - cookie["AIROS_TOKEN"] = "abc123"Note: After removal, consider dropping the unused SimpleCookie import at the top as well.
tests/test_airos_request.py (1)
130-158: Parameter/data forwarding test is on point. Consider adding a force update_check test.Given update_check(force=True) should use form data and x-www-form-urlencoded, an additional test would protect against regressions.
If helpful, I can add a test like:
- Asserts that when calling update_check(force=True):
- headers include Content-Type: application/x-www-form-urlencoded (authenticated headers path),
- session.request is invoked with data={"force": True} and json=None.
Want me to open a follow-up PR with this test?
airos/airos8.py (5)
184-246: Consolidate duplicated aiohttp.ClientError except blocks.The second except aiohttp.ClientError is unreachable due to the prior combined handler. Merge for clarity and consistent logging (consider separate TimeoutError if you want .exception).
- except (TimeoutError, aiohttp.ClientError) as err: - _LOGGER.exception("Error during API call to %s: %s", url, err) - raise AirOSDeviceConnectionError from err - except aiohttp.ClientError as err: - _LOGGER.error("Aiohttp client error for %s: %s", url, err) - raise AirOSDeviceConnectionError from err + except TimeoutError as err: + _LOGGER.exception("Error during API call to %s: %s", url, err) + raise AirOSDeviceConnectionError from err + except aiohttp.ClientError as err: + _LOGGER.exception("Error during API call to %s: %s", url, err) + raise AirOSDeviceConnectionError from err
219-219: Consider lowering success log to debug.Logging every successful request at info can be chatty in consumers. Debug is often more appropriate.
- _LOGGER.info("Successfully fetched JSON from %s", url) + _LOGGER.debug("Successfully fetched JSON from %s", url)
251-251: Send explicit Content-Type for login.Being explicit avoids device quirks when posting JSON bodies.
- await self._request_json("POST", self._login_url, json_data=payload) + await self._request_json( + "POST", self._login_url, json_data=payload, ct_json=True + )
150-169: Optional: include common headers in all requests._common_headers are defined but never used; merging them (and letting explicit headers override) can improve compatibility with stricter devices.
- request_headers = ( - self._get_authenticated_headers(ct_json=ct_json, ct_form=ct_form) - if authenticated - else {} - ) - if headers: - request_headers.update(headers) + request_headers = dict(self._common_headers) + if authenticated: + request_headers.update( + self._get_authenticated_headers(ct_json=ct_json, ct_form=ct_form) + ) + if headers: + request_headers.update(headers)
132-139: Be defensive when reading interface fields in derived_data.Direct dict indexing risks KeyError on malformed inputs. Use .get with defaults to keep failure modes consistent with AirOSKeyDataMissingError.
- for interface in interfaces: - if interface["enabled"]: # Only consider if enabled - addresses[interface["ifname"]] = interface["hwaddr"] + for interface in interfaces: + if interface.get("enabled"): # Only consider if enabled + ifname = interface.get("ifname") + hwaddr = interface.get("hwaddr") + if ifname and hwaddr: + addresses[ifname] = hwaddr
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
airos/airos8.py(4 hunks)pyproject.toml(1 hunks)tests/test_airos8.py(9 hunks)tests/test_airos_request.py(1 hunks)tests/test_stations.py(5 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
tests/test_airos_request.py (2)
airos/airos8.py (3)
AirOS(32-368)status(255-280)_request_json(184-245)airos/exceptions.py (3)
AirOSConnectionAuthenticationError(12-13)AirOSDataMissingError(16-17)AirOSDeviceConnectionError(24-25)
tests/test_stations.py (2)
tests/conftest.py (1)
airos_device(23-28)airos/airos8.py (2)
login(247-253)status(255-280)
airos/airos8.py (2)
airos/exceptions.py (4)
AirOSDeviceConnectionError(24-25)AirOSConnectionAuthenticationError(12-13)AirOSConnectionSetupError(8-9)AirOSDataMissingError(16-17)airos/data.py (1)
AirOS8Data(576-593)
🔇 Additional comments (10)
pyproject.toml (1)
7-7: Version bump looks good and is PEP 440–compliant.Pre-release version "0.4.1a0" is valid; no runtime impact.
tests/test_airos8.py (3)
173-182: Good move to patch the internal _request_json and set connected.This aligns tests with the new centralized request flow; clear and maintainable.
184-187: Assertions look correct and still validate the public data model.The checks exercise derived data and mode correctly.
279-281: Nice: verify the original cause type propagated from mashumaro.This asserts exception chaining correctness and logging path.
tests/test_stations.py (2)
166-171: Solid approach: centralize HTTP behavior by patching _request_json with side effects.Clear mapping: first call for login, second for status. Keeps test focused on transformation.
173-178: LGTM: patching _request_json and connected flag is the right level.Avoids coupling to session internals; future-proof with the new design.
tests/test_airos_request.py (4)
36-62: Happy path coverage for _request_json is clear and robust.Asserts on call signature and returned JSON are appropriate; usage of spec-ed ClientSession mock is a good guard.
64-79: Connection error mapping verified correctly.The context-manager enter side-effect is a faithful simulation; exception mapping to AirOSDeviceConnectionError matches the implementation.
81-105: Auth error handling path exercised well.Confirming raise_for_status call and mapping to AirOSConnectionAuthenticationError is valuable.
107-128: Non-JSON response path is covered; log assertion is helpful.Minor note: since the code logs at error, caplog at ERROR would be more precise, but DEBUG still captures it. No change required.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
airos/airos8.py (2)
156-168: Resolved: multiple Set-Cookie headers are now handled.Parsing all Set-Cookie headers addresses the previous bug where AIROS_* cookie could be missed.
264-281: Bug: force=True path sets form Content-Type but sends JSON body.ct_form=True with json_data=... is a header/body mismatch and can be rejected by the server.
Apply this diff:
async def update_check(self, force: bool = False) -> dict[str, Any]: """Check for firmware updates.""" if force: return await self._request_json( "POST", self._update_check_url, - json_data={"force": True}, + form_data={"force": True}, authenticated=True, ct_form=True, ) return await self._request_json( "POST", self._update_check_url, json_data={}, authenticated=True, ct_json=True, )
🧹 Nitpick comments (7)
tests/test_stations.py (2)
60-68: Align mocks with the new _request_json flow when re-enabling this test.This test patches session.post/get, but all calls are now routed through session.request via _request_json. When unskipping, patch airos_device._request_json or session.request instead to avoid false negatives.
184-187: Remove dead code (unused cookie construction).These cookie objects are not used and can be removed to reduce noise.
- cookie = SimpleCookie() - cookie["session_id"] = "test-cookie" - cookie["AIROS_TOKEN"] = "abc123"tests/test_airos8.py (2)
56-73: Replace triple-quoted disabled test with a skipped test.Keeping a whole test inside a string literal makes it harder to track in reports and tooling. Prefer @pytest.mark.skip to keep the test discoverable.
-# pylint: disable=pointless-string-statement -''' +@pytest.mark.skip(reason="broken, needs investigation") @pytest.mark.asyncio async def test_status_non_200_response(airos_device: AirOS) -> None: """Test status() with a non-successful HTTP response.""" airos_device.connected = True mock_status_response = MagicMock() mock_status_response.__aenter__.return_value = mock_status_response mock_status_response.text = AsyncMock(return_value="Error") mock_status_response.status = 500 # Simulate server error with ( - patch.object(airos_device.session, "request", return_value=mock_status_response), + patch.object(airos_device.session, "request", return_value=mock_status_response), pytest.raises(airos.exceptions.AirOSDeviceConnectionError), ): await airos_device.status() -'''
336-361: Consider adding a test for update_check(force=True) to prevent header/body mismatches.Currently only the default path is tested. Force-path should assert form submission is used (x-www-form-urlencoded with data=...), matching server expectations.
Example test you can add:
@pytest.mark.asyncio async def test_update_check_force_uses_form_request(monkeypatch) -> None: mock_session = MagicMock() airos_device = AirOS("http://192.168.1.3", "test", "test", mock_session) airos_device.connected = True mock_response = MagicMock() mock_response.__aenter__.return_value = mock_response mock_response.status = 200 mock_response.text = AsyncMock(return_value="{}") with patch.object(airos_device.session, "request", return_value=mock_response) as pr: await airos_device.update_check(force=True) # Validate method, URL and that form data was used (data=...), not json=... args, kwargs = pr.call_args assert args[0] == "POST" assert airos_device._update_check_url in args[1] assert "data" in kwargs and kwargs["data"] == {"force": True} assert "json" not in kwargs or kwargs["json"] is Noneairos/airos8.py (3)
212-219: Use str(err) for logging; err.message may be None.ClientResponseError.message is optional; logging str(err) is more robust.
- _LOGGER.error( - "Request to %s failed with status %s: %s", url, err.status, err.message - ) + _LOGGER.error( + "Request to %s failed with status %s: %s", url, err.status, str(err) + )
68-75: Tidy-up: remove unused auth fields / unify CSRF usage.
- current_csrf_token is no longer used; _csrf_id is the active field in headers.
- _use_json_for_login_post is also unused.
Remove dead attributes or wire them consistently to avoid confusion.
- self.current_csrf_token: str | None = None - - self._use_json_for_login_post = False + # Self._csrf_id is used for X-CSRF-ID header when presentIf current_csrf_token is still needed for compatibility, alias it to _csrf_id:
@property def current_csrf_token(self) -> str | None: return self._csrf_id
122-125: Nit: comment typo.“Fist” → “First”.
- # Fallback take fist alternate interface found + # Fallback: take first alternate interface found
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
CHANGELOG.md(1 hunks)airos/airos8.py(4 hunks)tests/test_airos8.py(8 hunks)tests/test_stations.py(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
🧰 Additional context used
🧬 Code Graph Analysis (2)
tests/test_stations.py (3)
airos/data.py (2)
AirOS8Data(576-593)Wireless(450-498)airos/exceptions.py (2)
AirOSDeviceConnectionError(24-25)AirOSKeyDataMissingError(20-21)airos/airos8.py (2)
login(229-235)status(237-262)
tests/test_airos8.py (2)
tests/conftest.py (1)
airos_device(23-28)airos/airos8.py (1)
login(229-235)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run mypy
🔇 Additional comments (6)
tests/test_stations.py (2)
158-168: Good adaptation to centralized _request_json.Patching the internal _request_json with a side_effect sequence is clean and mirrors the new request flow.
169-178: LGTM: Clear separation of auth call and status retrieval in the new flow.Setting connected=True in the patch context is appropriate since login is mocked. The rest of the test asserts the deserialized data path.
tests/test_airos8.py (1)
250-283: Good: Verifies MissingField is the cause of AirOSKeyDataMissingError.This increases confidence that error mapping is consistent with mashumaro’s MissingField.
airos/airos8.py (3)
136-155: Headers builder is simple and adequate.Minimal auth header construction with ct_json/ct_form flags is a good fit for the refactor.
239-241: LGTM: status now routed through _request_json with authenticated headers.This aligns with the new centralized request flow.
290-297: LGTM: migrated mutating endpoints to _request_json with form/JSON flags.stakick/provmode use form data; warnings/progress/download/install use JSON as expected. Once expect_json is added for non-JSON endpoints, these flows should be robust.
Also applies to: 306-314, 317-350, 323-339, 334-339, 345-350
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
tests/test_stations.py (1)
97-141: Update test to xfail and align expected log messageThis test is currently skipped but should be marked xfail and updated to match the real log message emitted by
_request_json.
- Change the decorator so failures are visible (xfail) rather than silently skipped.
- Adjust the assertion for the error message to use the exact string from
_LOGGER.errorinairos.airos8._request_json.- (Optional) Consider switching to
caploginstead of patching_LOGGERdirectly for less brittle message checks.Minimal diff:
- @pytest.mark.skip(reason="broken, needs investigation") + @pytest.mark.xfail(reason="awaiting refactor alignment", raises=AirOSDeviceConnectionError) # … log_args = mock_logger.error.call_args[0] - assert log_args[0] == "API call to %s failed with status %d: %s" + assert log_args[0] == "Request to %s failed with status %s: %s" assert log_args[2] == 500 assert log_args[3] == "Error"
♻️ Duplicate comments (1)
tests/test_stations.py (1)
15-15: Corrected MissingField import resolves prior failure — LGTMImporting MissingField from mashumaro.exceptions fixes the earlier test collection error flagged in a previous review.
🧹 Nitpick comments (5)
tests/test_stations.py (5)
31-37: Skipped test and mismatch: align with _request_json and fix expectation or use xfailThis test is skipped and currently mixes concerns:
- Docstring says “InvalidFieldValue” but the injected exception is MissingField.
- Patching session.post/get won’t be hit anymore because status() routes through _request_json.
Suggestions (pick one):
- Minimal: convert skip to xfail to keep it on the radar without hiding it.
- Align to refactor: patch _request_json to return the fixture and rely on the forced MissingField to exercise the logging + re-raise path. Set connected=True manually since login side effects are bypassed.
Proposed diffs:
Convert skip to xfail:
-@pytest.mark.skip(reason="broken, needs investigation") +@pytest.mark.xfail(reason="awaiting refactor alignment", raises=AirOSKeyDataMissingError)Fix docstring and patching to align with _request_json:
- """Test that the status method correctly logs redacted data when it encounters an InvalidFieldValue during deserialization.""" + """Test that status() logs redacted data when a MissingField is raised during deserialization.""" @@ - patch.object(airos_device.session, "post", return_value=mock_login_response), - patch.object(airos_device.session, "get", return_value=mock_status_response), + patch.object(airos_device, "_request_json", return_value=fixture_data), patch( "airos.airos8.AirOSData.from_dict", side_effect=MissingField( field_name="wireless", field_type=Wireless, holder_class=AirOSData ), ), @@ - await airos_device.login() - with pytest.raises(AirOSKeyDataMissingError): - await airos_device.status() + airos_device.connected = True + with pytest.raises(AirOSKeyDataMissingError): + await airos_device.status()Note: Once you adopt _request_json patching, the mock_login_response/mock_status_response and cookie/header setup in this test become dead code and can be removed.
Also applies to: 60-71, 73-77
158-168: Simplify test_ap_object: avoid redundant login and clarify intentYou’re patching _request_json and also forcing connected=True, then still calling login(). That call only consumes a side-effect without adding value and increases coupling to login internals.
Suggestion:
- Don’t call login(); just set connected=True, and have _request_json return fixture_data once.
Proposed diff:
- # Create an async mock that can return different values for different calls - mock_request_json = AsyncMock( - side_effect=[ - {}, # First call for login() - fixture_data, # Second call for status() - ] - ) + # Only exercise status() path through _request_json + mock_request_json = AsyncMock(return_value=fixture_data) @@ - # We don't need to patch the session directly anymore - await airos_device.login() - status: AirOSData = await airos_device.status() + # We don't need to patch the session directly anymore + status: AirOSData = await airos_device.status()Optional: assert the call count/arguments to verify the internal path is used:
- assert mock_request_json.await_count == 1
- mock_request_json.assert_awaited()
Also applies to: 170-177
179-183: Assertions look goodValidating Wireless.mode and derived.mac_interface remains stable across fixtures. Consider adding a small check that the mock was awaited as expected to harden the test against regressions in the HTTP path, but this is optional.
184-187: Remove unused cookie setupThese lines are dead code in this test and can confuse future readers.
Apply this diff:
- cookie = SimpleCookie() - cookie["session_id"] = "test-cookie" - cookie["AIROS_TOKEN"] = "abc123"
189-191: Prefer xfail over skip and align with _request_json for reconnect testInstead of skipping, mark as xfail to maintain visibility in CI. Also, stakick() should now route through _request_json; patch that instead of session.request for a focused unit test.
Proposed diff:
-@pytest.mark.skip(reason="broken, needs investigation") +@pytest.mark.xfail(reason="awaiting refactor alignment")Sketch of an updated approach (outside the changed lines):
- Patch airos_device._request_json to return {} and set airos_device.connected = True.
- Assert stakick() resolves truthy/successful based on the public contract.
Example:
with ( patch.object(airos_device, "_request_json", return_value={}), ): airos_device.connected = True assert await airos_device.stakick("01:23:45:67:89:aB")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
tests/test_stations.py(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/test_stations.py (5)
airos/data.py (2)
AirOS8Data(576-593)Wireless(450-498)airos/exceptions.py (2)
AirOSDeviceConnectionError(24-25)AirOSKeyDataMissingError(20-21)tests/conftest.py (1)
airos_device(23-28)airos/airos8.py (2)
status(237-262)AirOS(43-434)tests/test_airos8.py (2)
test_status_missing_interface_key_data(94-111)test_derived_data_no_br0_eth0_ath0(123-133)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run mypy
🔇 Additional comments (1)
tests/test_stations.py (1)
10-11: Imports align with refactor and usages — LGTMAirOS8Data alias, Wireless, and the new exception imports are correct and used below.
|
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
Chores