Improvement updates and fix for 8718 without GPS #59
Conversation
WalkthroughIntroduces optional GPS fields in data models, updates fixtures and tests to cover missing/augmented GPS scenarios, broadens async error handling with explicit cancellation handling in airos8.py, and bumps version and changelog to 0.2.8. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Airos8
participant Device
Caller->>Airos8: login()
Airos8->>Device: Authenticate
alt Success
Device-->>Airos8: OK
Airos8-->>Caller: token/session
else ClientError/TimeoutError
Airos8-->>Caller: raise error (logged)
else CancelledError
Airos8-->>Caller: re-raise cancellation
end
sequenceDiagram
participant Caller
participant Airos8
participant Device
Caller->>Airos8: status()
Airos8->>Device: GET /status
alt Success
Device-->>Airos8: status JSON (gps may be null)
Airos8-->>Caller: parsed data
else ClientError/TimeoutError
Airos8-->>Caller: raise error (logged as exception)
else CancelledError
Airos8-->>Caller: re-raise cancellation
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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: 6
🔭 Outside diff range comments (3)
airos/data.py (1)
522-528: Remove unused GPSMain dataclassGPSMain is only defined (airos/data.py:522–528) and never referenced elsewhere. Since AirOS8Data.gps already uses GPSData | None, GPSMain is redundant and should be removed—or, if you need to preserve backwards compatibility, marked as deprecated.
• airos/data.py:522–528 – delete the entire GPSMain class definition
• (Optional) If you can’t remove it immediately, add a deprecation warning above GPSMain and update any relevant docs or exportsairos/airos8.py (2)
383-388: provmode: also catch asyncio.TimeoutErrorMirror other endpoints.
-except (TimeoutError, aiohttp.client_exceptions.ClientError) as err: - _LOGGER.exception("Error during call to change provisioning mode: %s", err) - raise AirOSDeviceConnectionError from err +except (asyncio.TimeoutError, TimeoutError, aiohttp.client_exceptions.ClientError) as err: + _LOGGER.exception("Error during call to change provisioning mode: %s", err) + raise AirOSDeviceConnectionError from err
151-156: Bug: login returns None when CSRF header is absentEarly-returning on a missing X-CSRF-ID causes login() to return None (violates the bool return contract) even if cookies are valid. Downstream status() already tolerates absent CSRF (it conditionally sets the header). Don’t return here; proceed without a token.
Replace the else branch with a debug log, and continue:
new_csrf_token = response.headers.get("X-CSRF-ID") if new_csrf_token: self.current_csrf_token = new_csrf_token else: _LOGGER.debug("No CSRF token in login response; proceeding without it.")
🧹 Nitpick comments (4)
fixtures/airos_loco5ac_sta-ptp.json (1)
28-35: LGTM: Both top-level and remote GPS extendedThe additions are consistent across both the device and remote gps objects. This symmetry reduces parsing edge cases.
Consider documenting in a short comment (in code or tests) how time_synced should be interpreted (0/1 vs bool) to avoid ambiguity across devices/firmwares.
Also applies to: 528-536
airos/airos8.py (3)
127-149: Duplicate cookie injection; simplify to a single updateYou call update_cookies twice for the same morsel. One update with {morsel.key: morsel.value} is sufficient once the domain is set.
- self.session.cookie_jar.update_cookies( - { - morsel.key: morsel.output(header="")[len(morsel.key) + 1 :].split(";")[0].strip() - }, - response.url, - ) - # The update_cookies method can take a SimpleCookie morsel directly or a dict. - # The morsel.output method gives 'NAME=VALUE; Path=...; HttpOnly' - # We just need 'NAME=VALUE' or the morsel object itself. - # Let's use the morsel directly which is more robust. - # Alternatively: self.session.cookie_jar.update_cookies({morsel.key: morsel.value}) might work if it's simpler. - # Aiohttp's update_cookies takes a dict mapping name to value. - # To pass the full morsel with its attributes, we need to add it to the jar's internal structure. - # Simpler: just ensure the key-value pair is there for simple jar. - - # Let's try the direct update of the key-value - self.session.cookie_jar.update_cookies( - {morsel.key: morsel.value} - ) + self.session.cookie_jar.update_cookies( + {morsel.key: morsel.value}, response.url + )
160-165: Cookie jar emptiness check may be ineffectiveif not self.session.cookie_jar is likely always False because CookieJar is truthy even when empty. Consider checking length or filtered cookies.
Use a more reliable check:
if not list(self.session.cookie_jar): # or bool(self.session.cookie_jar.filter_cookies(self.base_url)) _LOGGER.exception("COOKIE JAR IS EMPTY after login POST. This is a major issue.") raise AirOSConnectionSetupError from None
312-312: Type hint: make mac_address Optional[str]Minor typing cleanup for clarity.
-async def stakick(self, mac_address: str = None) -> bool: +async def stakick(self, mac_address: str | None = None) -> bool:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
CHANGELOG.md(1 hunks)airos/airos8.py(5 hunks)airos/data.py(2 hunks)fixtures/airos_liteapgps_ap_ptmp_40mhz.json(1 hunks)fixtures/airos_loco5ac_ap-ptp.json(1 hunks)fixtures/airos_loco5ac_sta-ptp.json(1 hunks)fixtures/airos_nanobeam5ac_sta_ptmp_40mhz.json(1 hunks)fixtures/airos_nanostation_ap-ptp_8718_missing_gps.json(1 hunks)fixtures/userdata/nanostation_ap-ptp_8718_missing_gps.json(1 hunks)pyproject.toml(1 hunks)tests/test_stations.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
airos/airos8.py (2)
airos/exceptions.py (1)
AirOSDeviceConnectionError(24-25)tests/test_airos8.py (3)
test_login_connection_error(35-41)test_stakick_connection_error(157-164)test_provmode_connection_error(220-227)
tests/test_stations.py (1)
tests/test_airos8.py (1)
test_status_missing_required_key_in_json(231-260)
fixtures/airos_nanostation_ap-ptp_8718_missing_gps.json (1)
tests/test_airos8.py (2)
test_derived_data_no_br0_eth0_ath0(113-123)test_status_missing_required_key_in_json(231-260)
🔇 Additional comments (10)
fixtures/airos_nanobeam5ac_sta_ptmp_40mhz.json (1)
28-35: LGTM: Optional GPS fields added with null placeholdersThe added gps fields (alt, dim, dop, sats, time_synced) are well-formed and keep JSON valid (no trailing comma on the last field). This aligns with making GPS optional in the model.
Confirm that the target dataclass types accept both null and numeric values for these fields (e.g., Optional[float]/Optional[int]) and that time_synced can be null or an int/bool as returned by devices. If needed, I can generate a quick grep script to verify airos/data.py.
fixtures/airos_loco5ac_ap-ptp.json (1)
28-35: LGTM: Top-level GPS extended consistentlyTop-level gps now includes alt/dim/dop/sats/time_synced as null; structure and ordering consistent with other fixtures. No JSON issues spotted.
Please confirm tests cover the ap-ptp path with both present and missing GPS data for parity with sta-ptp fixtures.
airos/data.py (2)
359-361: Make GPS optional on Remote: LGTMChanging Remote.gps to GPSData | None with a default of None matches field reality on devices without GPS. Good defensive typing.
562-564: Top-level GPS optional on AirOS8Data: LGTMAligns with fixtures and prevents deserialization failures when gps is null. Good.
airos/airos8.py (1)
192-195: Cancellation semantics: LGTMExplicit asyncio.CancelledError handling and re-raise preserves cooperative cancellation. Good addition and logging consistency.
Also applies to: 309-311, 347-349, 386-388
fixtures/airos_nanostation_ap-ptp_8718_missing_gps.json (1)
27-27: Fixture: gps set to null — matches new optional typingThis correctly exercises the missing-GPS scenario for both top-level gps and nested remote.gps. Good coverage alignment with data model changes.
fixtures/userdata/nanostation_ap-ptp_8718_missing_gps.json (1)
1-1: Userdata fixture for missing GPS: LGTMComplements the AIROS fixture and tests. Structure matches dataclasses; gps_sync=false aligns with real-world behavior without GPS.
fixtures/airos_liteapgps_ap_ptmp_40mhz.json (1)
28-35: Extended GPS metadata fields: LGTMalt, dim, dop, sats, and time_synced additions map cleanly to GPSData optional fields. JSON shape remains backward-compatible.
tests/test_stations.py (2)
134-144: Good addition: covering ap-ptp with missing GPS fixtureIncluding ("ap-ptp", "nanostation_ap-ptp_8718_missing_gps") strengthens coverage for the newly optional GPS fields and ensures parsing remains robust without GPS data.
134-144: Fixtures confirmed and mode alignment verifiedBoth
nanostation_ap-ptp_8718_missing_gpsfixtures are present and valid JSON, and the userdata fixture’swireless.modeis correctly set to"ap-ptp". No further changes needed.
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #59 +/- ##
==========================================
- Coverage 95.71% 94.98% -0.73%
==========================================
Files 9 9
Lines 1283 1296 +13
==========================================
+ Hits 1228 1231 +3
- Misses 55 65 +10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1 participant
Summary by CodeRabbit
Bug Fixes
Tests
Documentation
Chores