fix: Remove stale token check from NavienMqttClient.__init__()#64
Conversation
The strict token validity check in __init__() prevents creating MQTT clients with restored tokens that may have expired between application restarts. However, NavienMqttClient.connect() already handles token refresh automatically, making the check in __init__() redundant and overly restrictive. This change: - Removes the has_valid_tokens check from __init__() - Relies on connect() to validate and refresh tokens when needed - Allows integrations to create MQTT clients with expired tokens - Enables proper handling of restored authentication sessions - Simplifies integration code by removing duplicate token refresh calls Fixes: MQTT connection failures when using stored tokens across restarts
Tests now verify that MQTT clients can be created with expired tokens, since token validation and refresh has moved to connect().
Updated test to reflect that MQTT init no longer rejects expired tokens. Token validation has been moved to connect() method.
- Break docstring on line 479 to stay within 80 char limit - Break comment on line 502 to stay within 80 char limit
Fixes W293 lint warnings - blank lines in docstrings had trailing whitespace.
There was a problem hiding this comment.
Pull request overview
This PR removes the strict token validation check from NavienMqttClient.__init__(), deferring all token validation to the connect() method. This simplifies integration code by eliminating redundant token checks and supports scenarios where tokens are restored from storage between application restarts.
Changes:
- Removed
has_valid_tokenscheck from MQTT client initialization - Updated docstring to clarify token validation happens in
connect() - Modified tests to verify MQTT clients can be created with expired tokens
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/nwp500/mqtt/client.py | Removed token expiration check from __init__() and updated docstring to document deferred validation |
| tests/test_mqtt_client_init.py | Updated all tests to verify MQTT client creation succeeds with expired tokens |
src/nwp500/mqtt/client.py
Outdated
| ) | ||
| # Note: Token validity is checked in connect() which also refreshes | ||
| # stale tokens automatically. This allows creating MQTT clients with | ||
| # restored tokens that may have expired between sessions. |
There was a problem hiding this comment.
The comment should explain what happens if connect() is never called with expired tokens. Consider adding guidance about the deferred validation pattern, such as: "Token validity is checked in connect() which also refreshes stale tokens automatically. This allows creating MQTT clients with restored tokens that may have expired between sessions. Note: connect() must be called to establish the connection and validate/refresh tokens."
| # restored tokens that may have expired between sessions. | |
| # restored tokens that may have expired between sessions. Token | |
| # validation and refresh are deferred until connect() is called; if | |
| # connect() is never called, tokens are not revalidated/refreshed | |
| # and no MQTT connection is established. |
tests/test_mqtt_client_init.py
Outdated
| assert not mqtt_client.is_connected | ||
|
|
||
| def test_mqtt_client_init_error_message_guidance( | ||
| def test_mqtt_client_init_no_error_message_for_expired_tokens( |
There was a problem hiding this comment.
The test name test_mqtt_client_init_no_error_message_for_expired_tokens is unclear. Consider renaming to test_mqtt_client_init_accepts_expired_tokens_without_error to better convey that the test verifies successful initialization (not just absence of error messages).
tests/test_mqtt_client_init.py
Outdated
| """Test that MQTT client works with valid tokens. | ||
|
|
||
| MQTT init no longer checks token validity - that happens in connect(). | ||
| """ |
There was a problem hiding this comment.
The docstring is misleading. The test name suggests it validates "integration" between has_valid_tokens and MQTT init, but the updated docstring states "MQTT init no longer checks token validity." Consider updating the test name to test_mqtt_client_creation_succeeds_with_valid_tokens or updating the docstring to clarify what aspect of integration is being tested.
- Expand comment in client.py to explain behavior when connect() is not called - Rename test_mqtt_client_init_no_error_message_for_expired_tokens to test_mqtt_client_init_accepts_expired_tokens_without_error for clarity - Rename test_has_valid_tokens_integration_with_mqtt_init to test_mqtt_client_creation_succeeds_with_valid_tokens with improved docstring
Description
Removes the strict
has_valid_tokenscheck fromNavienMqttClient.__init__()since token validation and refresh is already handled inconnect().Fixes #63
Changes
if not auth_client.has_valid_tokenscheck from init()Why This Fix
The current design forces integrations to:
ensure_valid_token()before creating the MQTT clientensure_valid_token()again in connect()This creates redundancy and prevents valid use cases where tokens are restored from storage between application restarts.
By deferring all token validation to
connect(), we:Testing