Alternative v6 XM approach #122
Conversation
WalkthroughAdds AirOS v6 XM login flow with cookie-based activation and api_version tracking; adjusts auth header/cookie storage; extends model mapping and v6 data parsing (frequency, throughput, CPU fields); adds v6 fixtures and tests; bumps package version; ignores Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant AirOS as AirOS.login()
participant V8 as v8 (_login_url)
participant XM as v6 (_v6_xm_login_url)
participant Form as v6 (_v6_form_url)
Caller->>AirOS: login(credentials)
AirOS->>V8: POST credentials (attempt v8)
alt v8 responds
V8-->>AirOS: auth cookie
AirOS->>Caller: connected=true, api_version=8
else v8 not found / AirOSUrlNotFoundError
AirOS->>XM: GET /login.cgi (fetch cookie)
XM-->>AirOS: Set-Cookie
AirOS->>XM: POST /login.cgi (credentials) -> expect 302
XM-->>AirOS: 302 redirect
AirOS->>Form: GET /index.cgi (activate session)
Form-->>AirOS: activation OK
AirOS->>Caller: connected=true, api_version=6
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (13)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (4)
🧰 Additional context used🧬 Code graph analysis (5)airos/airos6.py (1)
script/mashumaro-step-debug.py (3)
script/generate_ha_fixture.py (2)
airos/helpers.py (2)
airos/base.py (3)
🪛 LanguageToolCHANGELOG.md[grammar] ~10-~10: Ensure spelling is correct (QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1) ⏰ 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)
🔇 Additional comments (20)
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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #122 +/- ##
==========================================
+ Coverage 89.57% 90.41% +0.84%
==========================================
Files 8 8
Lines 681 720 +39
==========================================
+ Hits 610 651 +41
+ Misses 71 69 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
airos/base.py (2)
264-274: Handle non-JSON login responses to avoid false failures.Login endpoints may return 204/HTML. Skip JSON parsing for login URLs.
- response_text = await response.text() - _LOGGER.debug("Successfully fetched JSON from %s", url) - - # If this is the login request, we need to store the new auth data - if url in self._login_urls: - self._store_auth_data(response) - self.connected = True - - return json.loads(response_text) + _LOGGER.debug("Request to %s succeeded", url) + # Login endpoints: store auth and return immediately without JSON parsing + if url in self._login_urls: + self._store_auth_data(response) + self.connected = True + return {} + # Other endpoints: parse JSON as usual + response_text = await response.text() + return json.loads(response_text)Also applies to: 268-272
383-391: Bug: Inconsistent body vs Content-Type in update_check(force=True).You're sending JSON with form Content-Type. Align with JSON for both branches.
- if force: - return await self._request_json( - "POST", - self._update_check_url, - json_data={"force": True}, - authenticated=True, - ct_form=True, - ) + if force: + return await self._request_json( + "POST", + self._update_check_url, + json_data={"force": True}, + authenticated=True, + ct_json=True, + )Also applies to: 396-397
🧹 Nitpick comments (4)
.gitignore (1)
15-15: Anchor ignore to root (or use local exclude) to avoid unintended matches.Use "/test.py" if you only mean the repo‑root file, or prefer .git/info/exclude locally to avoid committing personal helpers.
Is this meant to ignore only the root-level test.py?
airos/base.py (3)
58-59: Minor: avoid magic number for API versions.Consider an Enum or constants (e.g., API_V8 = 8) for readability.
95-95: Use an immutable container for login URLs.Tuple (or set) communicates intent and avoids accidental mutation.
- self._login_urls = [self._login_url, self._v6_xm_login_url] + self._login_urls = (self._login_url, self._v6_xm_login_url)
293-342: _login_v6 flow LGTM; consider reuse and headers parity.Optional: call _store_auth_data on the initial GET to reuse parsing; ensure Referer/Origin are consistent across calls.
Do you want me to refactor _login_v6 to reuse _store_auth_data after we fix the cookie handling?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.gitignore(1 hunks)airos/base.py(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
airos/base.py (1)
airos/exceptions.py (3)
AirOSDeviceConnectionError(24-25)AirOSConnectionAuthenticationError(12-13)AirOSUrlNotFoundError(44-45)
🔇 Additional comments (3)
airos/base.py (3)
82-85: v6 endpoints look reasonable.LGTM; the leading slash on _v6_form_url matches f"{self.base_url}{...}" usage.
253-256: Pre-login request gating is good.Prevents accidental calls before authentication. LGTM.
347-349: Graceful fallback to v6 on 404 looks good.Clear control flow; preserves 8.x first, then v6.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
airos/base.py (2)
213-214: Past review concern has been addressed; remove commented code.The previous review flagged a critical bug about the "AIROS_" prefix being stripped. This has been resolved—line 229 now stores the full cookie as
f"{morsel.key}={morsel.value}", preserving the "AIROS_" prefix. The current implementation is correct.Remove the commented-out line for cleanliness:
if self._auth_cookie: # pragma: no cover - # headers["Cookie"] = f"AIROS_{self._auth_cookie}" headers["Cookie"] = self._auth_cookie
228-229: Past review concern has been addressed; remove commented code.The cookie is now correctly stored with the full key name (including "AIROS_" prefix), addressing the previous review's critical concern.
Remove the commented-out line:
if key.startswith("AIROS_"): - # self._auth_cookie = morsel.key[6:] + "=" + morsel.value self._auth_cookie = f"{morsel.key}={morsel.value}" break
🧹 Nitpick comments (3)
airos/base.py (3)
302-304: Optional: Simplify cookie retrieval.The cookie retrieval logic is correct but could be more readable.
Consider this slightly clearer approach:
- session_cookie = next( - (c for n, c in response.cookies.items() if n.startswith("AIROS")), None - ) + airos_cookies = [c for n, c in response.cookies.items() if n.startswith("AIROS")] + session_cookie = airos_cookies[0] if airos_cookies else None
339-340: Consider more robust session activation check.Checking for "login.cgi" in the URL string works but is fragile—it could match query parameters or other unintended parts of the URL.
Consider a more explicit check:
- if "login.cgi" in str(response.url): + if response.url.path.endswith("login.cgi"): raise AirOSConnectionAuthenticationError("Session activation failed.")
344-355: Optional: Remove unnecessary return statement.The
returnon line 354 is unnecessary since the function returnsNoneimplicitly and it's at the end of the function.Simplify:
async def login(self) -> None: """Login to AirOS device.""" payload = {"username": self.username, "password": self.password} try: await self._request_json("POST", self._login_url, json_data=payload) except AirOSUrlNotFoundError: await self._login_v6() except AirOSConnectionSetupError as err: raise AirOSConnectionSetupError("Failed to login to AirOS device") from err - else: - returnOr if you prefer explicit returns, keep it but remove the
else:async def login(self) -> None: """Login to AirOS device.""" payload = {"username": self.username, "password": self.password} try: await self._request_json("POST", self._login_url, json_data=payload) except AirOSUrlNotFoundError: await self._login_v6() except AirOSConnectionSetupError as err: raise AirOSConnectionSetupError("Failed to login to AirOS device") from err - else: - return
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
airos/base.py(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
airos/base.py (2)
tests/conftest.py (1)
base_url(18-20)airos/exceptions.py (3)
AirOSDeviceConnectionError(24-25)AirOSConnectionAuthenticationError(12-13)AirOSUrlNotFoundError(44-45)
⏰ 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: Check commit
🔇 Additional comments (4)
airos/base.py (4)
58-59: LGTM: Sensible default for API version.Setting
api_versionto 8 by default is appropriate, as it will be updated to 6 in the_login_v6()flow when needed.
95-95: LGTM: Clean centralization of login URLs.The
_login_urlslist provides a clean way to check if a URL is a login endpoint, improving maintainability.
254-254: LGTM: Correct membership checks for login URLs.The use of
self._login_urlslist for membership checks properly handles both v8 and v6 login endpoints.Also applies to: 270-270
294-343: Good implementation of v6 XM login flow.The three-step v6 login process is well-structured:
- Retrieve session cookie from login endpoint
- POST credentials with 302 redirect expectation
- Activate session via form URL
Error handling appropriately distinguishes connection errors from authentication failures, and the cookie handling is consistent with the v8 path.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/test_stations6.py (1)
77-101: Consider adding explicit__aexit__setup for async context managers.While
MagicMockmay auto-mock the missing__aexit__, explicitly setting it makes the test more robust and clear:Apply this pattern to each response mock:
get_login_response = MagicMock() get_login_response.__aenter__.return_value = get_login_response +get_login_response.__aexit__.return_value = AsyncMock(return_value=None)Repeat for
post_login_responseandget_index_response.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test_stations6.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/test_stations6.py (2)
airos/base.py (2)
status(356-380)_login_v6(294-342)airos/airos6.py (1)
AirOS6(17-82)
🪛 Gitleaks (8.28.0)
tests/test_stations6.py
[high] 116-116: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ 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: Check commit
🔇 Additional comments (3)
tests/test_stations6.py (3)
7-7: LGTM!The addition of
MagicMockto the imports is appropriate for the new test function.
104-109: Verify host parameter format and use_ssl consistency.The
hostparameter includes the scheme"http://192.168.1.3", butuse_ssldefaults toTrue, which suggests HTTPS. Ensure this is intentional and that the base class correctly handles a host with an explicit scheme, or adjust the parameters for consistency:airos6_device = AirOS6( - host="http://192.168.1.3", + host="192.168.1.3", username="ubnt", password="ubnt", session=session, + use_ssl=False, )
111-117: LGTM! Test assertions align with the v6 login flow.The test correctly validates:
- Connection state after login
- API version assignment
- Auth cookie format
- Expected number of HTTP requests
Note: The static analysis hint flagging line 116 is a false positive—this is a test assertion checking the cookie value, not an exposed secret.
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)
airos/base.py (1)
382-399: Content-Type/json mismatch in update_check(force=True).You send JSON (
json_data={"force": True}) but set form Content-Type (ct_form=True). Usect_json=Truefor consistency and correctness.- if force: - return await self._request_json( - "POST", - self._update_check_url, - json_data={"force": True}, - authenticated=True, - ct_form=True, - ) + if force: + return await self._request_json( + "POST", + self._update_check_url, + json_data={"force": True}, + authenticated=True, + ct_json=True, + )
♻️ Duplicate comments (3)
airos/base.py (3)
212-215: Cookie handling fix retained full cookie name.Keeping the full
AIROS_*cookie string in the header resolves the v6/v8 interoperability issue. LGTM.
222-230: Parsing Set-Cookie for AIROS_ and storing full pair is correct.*This matches device behavior and prior feedback. LGTM.
82-85: Minor: endpoint URL consistency.
_v6_form_urlis a path while others are absolute. It works (you prefix at use), but consider normalizing for consistency or add a clarifying comment.
🧹 Nitpick comments (4)
airos/model_map.py (1)
99-145: Good normalization; consider clarifying match semantics.The
strip()fix is spot on. The variablebest_match_is_prefixis misleading since you’re usingendswith(); consider renaming to avoid confusion and add a brief comment with an example (“devmodel is a suffix of the catalog name: 'LiteBeam 5AC' ⟶ 'airMAX LiteBeam 5AC'”).tests/test_stations6.py (2)
67-71: Remove unused cookie setup.This snippet isn’t used by assertions; drop it to reduce noise.
- cookie = SimpleCookie() - cookie["session_id"] = "test-cookie" - cookie["AIROS_TOKEN"] = "abc123"
118-118: Gitleaks false positive: replace secret-looking test token.The hardcoded
"AIROS_ABC123=xyz789"can trigger secret scanners. The diff above switches to"AIROS_TEST=nonsecret". Alternatively, build the string from variables to avoid static patterns.fixtures/airos_NanoStation_loco_M5_v6.3.16_XM_ap.json (1)
24-24: Consider adjustingcpuloadto a more realistic baseline value.The fixture specifies
cpuload: 100.0, which represents 100% CPU utilization. For a typical test fixture representing normal device state, a lower baseline value (e.g., 15–40) would be more realistic and representative. If this represents a specific high-load test scenario, consider documenting it or creating a separate fixture variant.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
CHANGELOG.md(1 hunks)airos/base.py(9 hunks)airos/model_map.py(3 hunks)fixtures/airos_NanoStation_M5_sta_v6.3.16.json(1 hunks)fixtures/airos_NanoStation_loco_M5_v6.3.16_XM_ap.json(1 hunks)fixtures/airos_NanoStation_loco_M5_v6.3.16_XM_sta.json(1 hunks)fixtures/userdata/NanoStation_loco_M5_v6.3.16_XM_ap.json(1 hunks)fixtures/userdata/NanoStation_loco_M5_v6.3.16_XM_sta.json(1 hunks)pyproject.toml(1 hunks)tests/test_stations6.py(3 hunks)
✅ Files skipped from review due to trivial changes (4)
- fixtures/airos_NanoStation_loco_M5_v6.3.16_XM_sta.json
- fixtures/userdata/NanoStation_loco_M5_v6.3.16_XM_ap.json
- fixtures/userdata/NanoStation_loco_M5_v6.3.16_XM_sta.json
- CHANGELOG.md
🧰 Additional context used
🧬 Code graph analysis (2)
tests/test_stations6.py (2)
airos/base.py (2)
status(356-380)_login_v6(294-342)airos/airos6.py (1)
AirOS6(17-82)
airos/base.py (1)
airos/exceptions.py (3)
AirOSDeviceConnectionError(24-25)AirOSConnectionAuthenticationError(12-13)AirOSUrlNotFoundError(44-45)
🪛 Gitleaks (8.28.0)
tests/test_stations6.py
[high] 118-118: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ 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: Check commit
🔇 Additional comments (4)
pyproject.toml (1)
7-7: Confirm pre-release intent and automation.Version "0.5.7a9" is a pre-release (PEP 440). Ensure your publish/CI and consumers expect pre-releases; otherwise, pinning/upgrades may skip this version.
airos/model_map.py (1)
81-88: Public mappings look good.Splitting SITE_MODELS and MANUAL_MODELS and merging into MODELS improves clarity and extensibility. No issues.
fixtures/airos_NanoStation_M5_sta_v6.3.16.json (1)
13-13: Fixture SKU correction matches mapper behavior.
"sku": "NSM5"aligns with the normalized"NanoStation M5 "->NSM5mapping. Good.fixtures/airos_NanoStation_loco_M5_v6.3.16_XM_ap.json (1)
5-15: No action required - this is not a data inconsistency.The fixture is correct.
derived.moderepresents the device's network topology configuration (point-to-point wireless link), whilewireless.moderepresents its operating mode (access point). These are two distinct concepts. Every similar fixture in the codebase (includingap-ptp,sta-ptpvariants) consistently usesderived.mode: "point_to_point"withptp: true, confirming this is the intended design pattern for devices operating in PTP topology.Likely an incorrect or invalid review comment.
| # Create a mock session | ||
| session = MagicMock() | ||
|
|
||
| # Mock response for GET /login.cgi | ||
| get_login_response = MagicMock() | ||
| get_login_response.__aenter__.return_value = get_login_response | ||
| get_login_response.status = 200 | ||
| get_login_response.cookies = { | ||
| "AIROS_ABC123": MagicMock(key="AIROS_ABC123", value="xyz789") | ||
| } | ||
|
|
||
| # Mock response for POST /login.cgi | ||
| post_login_response = MagicMock() | ||
| post_login_response.__aenter__.return_value = post_login_response | ||
| post_login_response.status = 302 | ||
|
|
||
| # Mock response for GET /index.cgi | ||
| get_index_response = MagicMock() | ||
| get_index_response.__aenter__.return_value = get_index_response | ||
| get_index_response.status = 200 | ||
| get_index_response.url = "http://192.168.1.3/index.cgi" | ||
|
|
||
| # Set side effects for session.request | ||
| session.request.side_effect = [ | ||
| get_login_response, | ||
| post_login_response, | ||
| get_index_response, | ||
| ] | ||
|
|
||
| # Create device | ||
| airos6_device = AirOS6( | ||
| host="http://192.168.1.3", | ||
| username="ubnt", | ||
| password="ubnt", | ||
| session=session, | ||
| ) | ||
|
|
There was a problem hiding this comment.
Use AsyncMock for async context manager responses (fix test flakiness).
async with session.request(...) requires awaitable __aenter__/__aexit__. Replace MagicMock with AsyncMock helpers for the three responses.
- # Create a mock session
- session = MagicMock()
+ # Create a mock session
+ session = MagicMock()
+
+ def _cm(resp):
+ aenter = AsyncMock(return_value=resp)
+ aexit = AsyncMock(return_value=None)
+ resp.__aenter__ = aenter
+ resp.__aexit__ = aexit
+ return resp
@@
- get_login_response = MagicMock()
- get_login_response.__aenter__.return_value = get_login_response
+ get_login_response = _cm(MagicMock())
@@
- post_login_response = MagicMock()
- post_login_response.__aenter__.return_value = post_login_response
+ post_login_response = _cm(MagicMock())
@@
- get_index_response = MagicMock()
- get_index_response.__aenter__.return_value = get_index_response
+ get_index_response = _cm(MagicMock())
@@
- get_login_response.cookies = {
- "AIROS_ABC123": MagicMock(key="AIROS_ABC123", value="xyz789")
- }
+ get_login_response.cookies = {
+ "AIROS_TEST": MagicMock(key="AIROS_TEST", value="nonsecret")
+ }
@@
- assert airos6_device._auth_cookie == "AIROS_ABC123=xyz789" # noqa: SLF001
+ assert airos6_device._auth_cookie == "AIROS_TEST=nonsecret" # noqa: SLF001Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In tests/test_stations6.py around lines 76 to 112, the three mocked HTTP
responses are currently MagicMock but are used in async with
session.request(...), so replace those MagicMock response objects with AsyncMock
instances and ensure their __aenter__ and __aexit__ are awaitable (e.g. set
__aenter__.return_value to the AsyncMock response itself and use AsyncMock for
any coroutine properties), then set session.request.side_effect to return those
AsyncMock response objects (or async coroutines) in order so the async context
manager awaits succeed and the test stops flaking.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
airos/data.py (1)
607-609: Consider more robust frequency parsing.The current parsing assumes a space between the number and "MHz" (e.g.,
"5180 MHz"). If the format varies (e.g.,"5180MHz"),split()[0]might not work as expected.Consider a more flexible approach:
freq = d.get("frequency") if isinstance(freq, str) and "MHz" in freq: - d["frequency"] = int(freq.split()[0]) + d["frequency"] = int(freq.replace("MHz", "").strip())This handles both
"5180 MHz"and"5180MHz"formats.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
CHANGELOG.md(1 hunks)airos/data.py(5 hunks)fixtures/airos_NanoStation_M5_sta_v6.3.16.json(4 hunks)fixtures/airos_NanoStation_loco_M5_v6.3.16_XM_ap.json(1 hunks)fixtures/airos_NanoStation_loco_M5_v6.3.16_XM_sta.json(1 hunks)pyproject.toml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- fixtures/airos_NanoStation_loco_M5_v6.3.16_XM_ap.json
🚧 Files skipped from review as they are similar to previous changes (2)
- pyproject.toml
- fixtures/airos_NanoStation_loco_M5_v6.3.16_XM_sta.json
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.md
[grammar] ~10-~10: Ensure spelling is correct
Context: ...XW already was successful) - Calculated cpuload on v6 if values available (to prevent r...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
⏰ 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: Check commit
🔇 Additional comments (5)
fixtures/airos_NanoStation_M5_sta_v6.3.16.json (1)
1-165: LGTM! Fixture aligns with v6 data model.The fixture data correctly reflects the v6 data structure changes:
- CPU metrics (cpubusy, cputotal, cpuload) align with Host6 model
- Numeric frequency (5180) and rates (216, 270) match Wireless6 type changes
- Throughput object matches the derived field structure
airos/data.py (3)
249-263: LGTM! Safe cpuload calculation with proper guards.The pre-deserialize hook correctly:
- Validates both cpubusy and cputotal are numeric
- Prevents division by zero with
cputotal > 0check- Computes percentage and rounds to 2 decimal places
574-596: LGTM! Type changes align with v6 numeric data format.The field type updates (frequency, txrate, rxrate to int, and throughput addition) correctly reflect the v6 data structure. The pre-deserialize hook handles conversion from string formats.
611-618: LGTM! Throughput derivation handles type conversions safely.The logic correctly:
- Retrieves rxrate and txrate from the data
- Uses
int(float())conversion to handle numeric strings and floats- Defaults to 0 when values are missing or falsy
- Creates properly structured throughput dictionary
CHANGELOG.md (1)
5-12: LGTM! Changelog accurately documents v6 enhancements.The version 0.5.7 entry clearly describes:
- v6 XM model login support
- CPU load calculation improvements
- Frequency and rate handling for v6 firmware
Note: The static analysis warning about "cpuload" spelling is a false positive—it's valid technical terminology for CPU load metrics.
|
1 participant
Summary by CodeRabbit
New Features
Data
Models
Tests
Chores
New Fixtures