Align config-flow validation with GooglePollenApiClient, add quota exception, bump to v1.9.5#76
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the robustness and maintainability of the Google Pollen integration by standardizing API interaction during configuration. It ensures that setup validation mirrors runtime behavior, provides clearer error feedback for users, and centralizes API communication logic within the client. The changes also expand test coverage to prevent regressions in error handling and input normalization. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
||||||||||||||||||||||||
PR Code Suggestions ✨Explore these optional code suggestions:
|
|||||||||||
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively refactors the config flow to use the GooglePollenApiClient, which centralizes network and error handling logic, improving maintainability. The introduction of a dedicated PollenQuotaExceededError is a good enhancement for clearer error semantics. My review includes a high-severity comment on the test implementation, suggesting a refactor to better align with the new decoupled architecture by mocking the client's interface rather than its implementation, consistent with best practices for mocking. I've also included a medium-severity suggestion to improve code conciseness in the new exception handling blocks.
| class _StubValidationClient: | ||
| def __init__(self, responses: list[_StubResponse], api_key: str) -> None: | ||
| self._responses = responses | ||
| self._api_key = api_key | ||
| self.calls: list[dict[str, object]] = [] | ||
|
|
||
| async def read(self) -> bytes: | ||
| return self._body | ||
| async def async_fetch_pollen_data( | ||
| self, | ||
| *, | ||
| latitude: float, | ||
| longitude: float, | ||
| days: int, | ||
| language_code: str | None, | ||
| ) -> dict: | ||
| self.calls.append( | ||
| { | ||
| "latitude": latitude, | ||
| "longitude": longitude, | ||
| "days": days, | ||
| "language_code": language_code, | ||
| } | ||
| ) | ||
| response = self._responses.pop(0) | ||
|
|
||
| if response.status == 401: | ||
| raise cf.ConfigEntryAuthFailed("HTTP 401: API key *** not valid") | ||
|
|
||
| if response.status == 403: | ||
| body_text = response._body.decode() | ||
| if "API key not valid" in body_text: | ||
| raise cf.ConfigEntryAuthFailed(f"HTTP 403: {body_text}") | ||
| raise cf.UpdateFailed(f"HTTP 403: {body_text}") | ||
|
|
||
| if response.status == 429: | ||
| raise cf.PollenQuotaExceededError("HTTP 429: Quota exceeded") | ||
|
|
||
| if response.status != 200: | ||
| raise cf.UpdateFailed(f"HTTP {response.status}: backend error") | ||
|
|
||
| async def json(self): | ||
| import json as _json | ||
|
|
||
| return _json.loads(self._body.decode()) | ||
| return _json.loads(response._body.decode()) | ||
|
|
||
| async def text(self) -> str: | ||
| return self._body.decode() | ||
|
|
||
| class _PatchedClientFactory: | ||
| def __init__(self, responses: list[_StubResponse]): | ||
| self._responses = responses | ||
| self.instances: list[_StubValidationClient] = [] | ||
|
|
||
| class _SequenceSession: | ||
| def __init__(self, responses: list[_StubResponse]) -> None: | ||
| self.responses = responses | ||
| self.calls: list[tuple[tuple, dict]] = [] | ||
| @property | ||
| def calls(self) -> list[dict[str, object]]: | ||
| if not self.instances: | ||
| return [] | ||
| return self.instances[0].calls | ||
|
|
||
| def get(self, *args, **kwargs): | ||
| self.calls.append((args, kwargs)) | ||
| return self.responses.pop(0) | ||
| def __call__(self, session, api_key: str) -> _StubValidationClient: | ||
| client = _StubValidationClient(self._responses, api_key) | ||
| self.instances.append(client) | ||
| return client |
There was a problem hiding this comment.
The _StubValidationClient re-implements logic for interpreting HTTP responses and raising exceptions. This duplicates the responsibility of GooglePollenApiClient, which this PR aims to centralize. The tests for config_flow should be decoupled from the client's internal implementation (i.e., how it handles HTTP status codes). Instead, the mock client should directly raise the expected exceptions (ConfigEntryAuthFailed, PollenQuotaExceededError, etc.) to test how config_flow handles them. The _TimeoutClient and _ClientErroringClient classes in this file are great examples of this preferred approach. Refactoring the tests to follow this pattern will make them more robust and better aligned with the goal of this PR.
References
- Mocks should accurately reflect the synchronicity and overall behavior of the function being mocked. By directly raising expected exceptions, the mock accurately simulates the outcome of the real client's methods, aligning with the principle of mocking the interface rather than internal implementation details.
| except ConfigEntryAuthFailed as err: | ||
| errors["base"] = "invalid_auth" | ||
| redacted = redact_api_key(err, api_key) | ||
| if redacted: | ||
| placeholders["error_message"] = redacted | ||
| except PollenQuotaExceededError as err: | ||
| redacted = redact_api_key(err, api_key) | ||
| if redacted: | ||
| placeholders["error_message"] = redacted | ||
| errors["base"] = "quota_exceeded" | ||
| except UpdateFailed as err: | ||
| redacted = redact_api_key(err, api_key) | ||
| if redacted: | ||
| placeholders["error_message"] = redacted | ||
| errors["base"] = "cannot_connect" |
There was a problem hiding this comment.
To reduce code duplication and improve conciseness, you can use assignment expressions (the walrus operator :=) to both check for and assign the redacted error message. This is supported since your project requires Python >= 3.14.
| except ConfigEntryAuthFailed as err: | |
| errors["base"] = "invalid_auth" | |
| redacted = redact_api_key(err, api_key) | |
| if redacted: | |
| placeholders["error_message"] = redacted | |
| except PollenQuotaExceededError as err: | |
| redacted = redact_api_key(err, api_key) | |
| if redacted: | |
| placeholders["error_message"] = redacted | |
| errors["base"] = "quota_exceeded" | |
| except UpdateFailed as err: | |
| redacted = redact_api_key(err, api_key) | |
| if redacted: | |
| placeholders["error_message"] = redacted | |
| errors["base"] = "cannot_connect" | |
| except ConfigEntryAuthFailed as err: | |
| errors["base"] = "invalid_auth" | |
| if redacted := redact_api_key(err, api_key): | |
| placeholders["error_message"] = redacted | |
| except PollenQuotaExceededError as err: | |
| errors["base"] = "quota_exceeded" | |
| if redacted := redact_api_key(err, api_key): | |
| placeholders["error_message"] = redacted | |
| except UpdateFailed as err: | |
| errors["base"] = "cannot_connect" | |
| if redacted := redact_api_key(err, api_key): | |
| placeholders["error_message"] = redacted |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8aaf555232
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
|
|
||
| update_coordinator_mod.UpdateFailed = _StubUpdateFailed | ||
| _force_module("homeassistant.helpers.update_coordinator", update_coordinator_mod) |
There was a problem hiding this comment.
Forcing homeassistant.helpers.update_coordinator into sys.modules with only UpdateFailed makes the test process order-dependent: if this file is imported before other test modules, later imports of DataUpdateCoordinator fail during collection. In this repo, pytest -q now errors with ImportError: cannot import name 'DataUpdateCoordinator' from tests/test_init.py/tests/test_sensor.py, so this change breaks the full suite rather than just isolating config-flow tests.
Useful? React with 👍 / 👎.
|
|
||
| client_mod.GooglePollenApiClient = _StubGooglePollenApiClient | ||
| client_mod.PollenQuotaExceededError = _StubPollenQuotaExceededError | ||
| _force_module("custom_components.pollenlevels.client", client_mod) |
There was a problem hiding this comment.
Injecting a fake custom_components.pollenlevels.client module globally causes downstream tests to run against the stub instead of the real client whenever this module is imported first. That leak breaks sensor/coordinator tests that rely on real client attributes/behavior (for example, tests patching client_mod.asyncio fail with AttributeError), so the suite becomes import-order dependent and no longer validates production client logic.
Useful? React with 👍 / 👎.
eXPerience83
deleted the
codex/refactor-input-validation-in-config-flow
branch
1 participant
User description
Motivation
quota_exceededmapping is stable.CHANGELOG.mdand package metadata.Description
PollenQuotaExceededErrorincustom_components/pollenlevels/client.pyand raise it when the API returns HTTP 429 instead of a text-basedUpdateFailed.custom_components/pollenlevels/config_flow.pyto callGooglePollenApiClient.async_fetch_pollen_dataand mapConfigEntryAuthFailed,PollenQuotaExceededError,UpdateFailed,TimeoutError, andClientErrorto form error keys (invalid_auth,quota_exceeded,cannot_connect, etc.).tests/test_config_flow.pyto stubGooglePollenApiClient, add tests for client-timeout and client-transport error mapping, adapt helper factories, and validate that request args (likedaysandlanguage_code) are normalized.1.9.5inmanifest.jsonandpyproject.toml, and added the correspondingCHANGELOG.mdentry.Testing
pytest tests/test_config_flow.pyto exercise config-flow validation and client stubs, and the test suite passed.quota_exceeded), clientTimeoutErrormapping tocannot_connect, client transport errors mapping tocannot_connect, and the happy-path normalization, all of which succeeded.Codex Task
PR Type
Enhancement, Tests
Description
Added
PollenQuotaExceededErrorexception for dedicated HTTP 429 handlingRefactored config-flow validation to use
GooglePollenApiClientdirectlyCentralized error mapping for auth, quota, timeout, and connectivity failures
Expanded test coverage with client-based validation and error scenarios
Bumped version to 1.9.5 with changelog documentation
Diagram Walkthrough
File Walkthrough
client.py
Add dedicated quota exceeded exceptioncustom_components/pollenlevels/client.py
PollenQuotaExceededErrorexception class inheriting fromUpdateFailedPollenQuotaExceededErrorinsteadof generic
UpdateFailedconfig_flow.py
Refactor validation to use GooglePollenApiClientcustom_components/pollenlevels/config_flow.py
GooglePollenApiClient.async_fetch_pollen_dataConfigEntryAuthFailed,PollenQuotaExceededError,UpdateFailed,TimeoutError, andClientErrorjson,aiohttp,extract_error_message,is_invalid_api_key_message)test_config_flow.py
Update tests for client-based validationtests/test_config_flow.py
homeassistant.exceptionsandhomeassistant.helpers.update_coordinator_StubGooglePollenApiClientand_StubPollenQuotaExceededErrorfor testing
_SequenceSessionwith_StubValidationClientand_PatchedClientFactoryfor client-based testingto
cannot_connectdays,language_code)CHANGELOG.md
Document version 1.9.5 release changesCHANGELOG.md
with client semantics
error mapping
manifest.json
Bump integration version to 1.9.5custom_components/pollenlevels/manifest.json
pyproject.toml
Bump package version to 1.9.5pyproject.toml