Use dedicated httpx Client #275
Conversation
WalkthroughThe pull request introduces enhancements to the Polestar API integration, focusing on improving error handling, resource management, and authentication processes. Changes span multiple files in the custom components, including modifications to the configuration flow, HTTP client creation, authentication mechanisms, and the addition of a logout functionality. The updates aim to make the API interaction more robust and provide better control over session management. Changes
Sequence DiagramsequenceDiagram
participant User
participant ConfigFlow
participant PolestarApi
participant PolestarAuth
User->>ConfigFlow: Initiate Credential Test
ConfigFlow->>PolestarApi: Create Client
PolestarApi->>PolestarAuth: Authenticate
alt Successful Authentication
PolestarApi-->>ConfigFlow: Validation Success
else Authentication Failure
PolestarApi->>PolestarAuth: async_logout
PolestarAuth-->>ConfigFlow: Return Error
end
ConfigFlow-->>User: Configuration Result
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
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: 0
🔭 Outside diff range comments (1)
custom_components/polestar_api/pypolestar/polestar.py (1)
Line range hint
249-286: Enhance error handling in GraphQL query executionThe current error handling could be more comprehensive to handle network-related issues and ensure proper resource cleanup.
async def _query_graph_ql( self, query: DocumentNode, operation_name: str | None = None, variable_values: dict | None = None, ): if self.gql_session is None: raise RuntimeError("GraphQL not connected") self.logger.debug("GraphQL URL: %s", self.api_url) try: result = await self.gql_session.execute( query, operation_name=operation_name, variable_values=variable_values, extra_args={ "headers": {"Authorization": f"Bearer {self.auth.access_token}"} }, ) except TransportQueryError as exc: self.logger.debug("GraphQL TransportQueryError: %s", str(exc)) if ( exc.errors and exc.errors[0].get("extensions", {}).get("code") == "UNAUTHENTICATED" ): self.latest_call_code = 401 raise PolestarNotAuthorizedException(exc.errors[0]["message"]) from exc self.latest_call_code = 500 raise PolestarApiException from exc + except httpx.ConnectError as exc: + self.logger.error("Failed to connect to GraphQL endpoint: %s", str(exc)) + self.latest_call_code = 503 + raise PolestarApiException("Failed to connect to API") from exc + except httpx.TimeoutException as exc: + self.logger.error("GraphQL request timed out: %s", str(exc)) + self.latest_call_code = 504 + raise PolestarApiException("Request timed out") from exc except Exception as exc: self.logger.debug("GraphQL Exception: %s", str(exc)) raise exc
🧹 Nitpick comments (5)
custom_components/polestar_api/config_flow.py (1)
89-102: LGTM! Good improvement in resource management.The addition of the try-finally block ensures proper cleanup of resources by calling
async_logout()regardless of the outcome. This is a good practice for managing HTTP client resources.Consider adding a debug log before raising exceptions to help with troubleshooting.
if found_vins := api_client.get_available_vins(): _LOGGER.debug("Found %d VINs for %s", len(found_vins), username) else: _LOGGER.warning("No VINs found for %s", username) + _LOGGER.debug("Available VINs: %s", found_vins) raise NoCarsFoundException if vin and vin not in found_vins: _LOGGER.warning("VIN %s not found for %s", vin, username) + _LOGGER.debug("Available VINs: %s", found_vins) raise VinNotFoundExceptioncustom_components/polestar_api/pypolestar/auth.py (1)
60-76: LGTM! Comprehensive logout implementation.The logout implementation thoroughly cleans up all resources including cookies and tokens. The domain-aware cookie cleanup is a good practice.
Consider adding a debug log for token cleanup similar to cookie cleanup.
self.access_token = None self.id_token = None self.refresh_token = None +self.logger.debug("Cleared all tokens") self.token_lifetime = None self.token_expiry = Nonecustom_components/polestar_api/polestar.py (1)
Line range hint
165-187: LGTM! Comprehensive error handling with appropriate retry intervals.The error handling is well-structured with different retry intervals based on the type of error. Consider adding constants for the retry intervals to make them more maintainable.
+# Constants for retry intervals +RETRY_INTERVAL_SHORT = 5 # seconds +RETRY_INTERVAL_MEDIUM = 15 # seconds +RETRY_INTERVAL_LONG = 60 # seconds except PolestarApiException as e: _LOGGER.warning("API Exception on update data %s", str(e)) - self.polestar_api.next_update = datetime.now() + timedelta(seconds=5) + self.polestar_api.next_update = datetime.now() + timedelta(seconds=RETRY_INTERVAL_SHORT) except PolestarAuthException as e: _LOGGER.warning("Auth Exception on update data %s", str(e)) await self.polestar_api.auth.get_token() - self.polestar_api.next_update = datetime.now() + timedelta(seconds=5) + self.polestar_api.next_update = datetime.now() + timedelta(seconds=RETRY_INTERVAL_SHORT)custom_components/polestar_api/pypolestar/polestar.py (2)
93-94: Add error handling to async_logout methodWhile the implementation is clean and follows single responsibility, consider adding error handling to ensure proper cleanup even if the auth logout fails.
async def async_logout(self) -> None: - await self.auth.async_logout() + try: + await self.auth.async_logout() + except Exception as exc: + self.logger.warning("Failed to logout: %s", str(exc)) + finally: + # Ensure cleanup of local session state + self.gql_session = None
Line range hint
36-39: Refactor client management to align with PR objectivesThe current implementation uses a single shared httpx client for all operations. To meet the PR objectives:
- Separate authentication and API clients
- Implement cookie cleanup after credential testing
- Create dedicated clients per API interaction
Consider this architectural approach:
class PolestarApi: def __init__( self, username: str, password: str, auth_client: httpx.AsyncClient | None = None, vins: list[str] | None = None, unique_id: str | None = None, ) -> None: """Initialize the Polestar API.""" # Dedicated client for auth with cookie management self.auth_client = auth_client or httpx.AsyncClient() # Separate client for API calls self.api_client = httpx.AsyncClient() self.auth = PolestarAuth(username, password, self.auth_client, unique_id)This separation allows for:
- Cookie cleanup after auth operations
- Independent client lifecycle management
- Better alignment with the PR objectives
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
custom_components/polestar_api/config_flow.py(1 hunks)custom_components/polestar_api/polestar.py(2 hunks)custom_components/polestar_api/pypolestar/auth.py(5 hunks)custom_components/polestar_api/pypolestar/const.py(1 hunks)custom_components/polestar_api/pypolestar/polestar.py(1 hunks)
🔇 Additional comments (4)
custom_components/polestar_api/pypolestar/const.py (1)
17-17: Add documentation for OIDC cookie constants
The addition of OIDC cookie names is appropriate for managing authentication state during logout. Consider adding a docstring to document the purpose of these cookies.
+# Cookies set by the Polestar Identity Provider that need to be cleared during logout
OIDC_COOKIES = ["PF", "PF.PERSISTENT"]Let's verify these are the correct cookie names by checking the authentication response:
✅ Verification successful
OIDC cookie names are correctly implemented for logout functionality
The verification confirms that the OIDC cookie names are correctly implemented and used in the logout flow:
- The cookies are used in
async_logout()method ofPolestarAuthclass - The implementation properly cleans up both "PF" and "PF.PERSISTENT" cookies using the domain from the OIDC provider URL
- The cookie names align with Ping Federate (PF) cookies, which is the identity provider used by Polestar (evident from the API endpoints using "pf." prefixed parameters)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the OIDC cookie names in the authentication response
# Expected: Response headers should contain Set-Cookie with PF and PF.PERSISTENT
curl -I -s "${OIDC_PROVIDER_BASE_URL}/as/authorization.oauth2" \
-H "Accept: application/json" | grep -i "set-cookie"
Length of output: 88
Script:
#!/bin/bash
# Let's check the codebase for any references to these cookie names
# and their usage in authentication/logout flows
# Search for cookie names in the codebase
echo "=== Searching for PF cookie references ==="
rg "PF\." -A 2 -B 2
echo -e "\n=== Searching for cookie-related code ==="
ast-grep --pattern 'cookie'
echo -e "\n=== Checking auth/logout implementation ==="
fd "auth|login|logout" --type f --exec cat {}
Length of output: 9929
custom_components/polestar_api/pypolestar/auth.py (2)
50-52: LGTM! Good improvement in attribute naming.
The renaming of attributes with the 'oidc_' prefix improves code clarity by explicitly indicating their relationship with OpenID Connect.
271-274: LGTM! Consistent attribute usage.
The changes maintain consistency with the renamed OIDC attributes while preserving the core token handling logic.
custom_components/polestar_api/polestar.py (1)
11-11: LGTM! Using recommended HTTP client creation method.
The switch to create_async_httpx_client aligns with Home Assistant's best practices for HTTP client management.
Also applies to: 231-231
|
@magnushacker can you test this PR? |
|
Works fine! |
httpx.Clientfor each APIhttpx.Clientwhen testing credentials, but remove all cookies after testingUpdates #272
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor