Add MQTT URI support for broker configuration#309
Conversation
WalkthroughAdds optional MQTT URI support (mqtt:// or mqtts://) across config, addon, and runtime: parsing of URI into connection parts, TLS support for the MQTT client, addon UI/schema/run-time handling, README/config examples, and tests. URI overrides per-field MQTT settings when present. Changes
Sequence Diagram(s)sequenceDiagram
participant Loader as Config Loader
participant App as MqttPowermeter
participant Broker as MQTT Broker
Loader->>Loader: read config (section MQTT / MQTT_INSIGHTS)
alt URI present
Loader->>Loader: parse_mqtt_uri(uri) -> MqttUriParts(host,port,username,password,tls)
Loader->>App: create powermeter with parsed parts (tls flag)
else URI absent
Loader->>App: create powermeter with BROKER/PORT/USERNAME/PASSWORD/TLS fields
end
App->>Broker: connect(host, port, credentials, tls?)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
README.md (1)
604-610: Clarify in docs that URI credentials are optional.Line 604 and Line 781 currently imply
user:pass@is always present. The parser accepts URIs without credentials, so documentingmqtt[s]://[user[:pass]@]host[:port]would reduce confusion.✏️ Suggested wording tweak
-Instead of `BROKER`/`PORT`/`USERNAME`/`PASSWORD`/`TLS`, you can provide a single `URI` of the form `mqtt://user:pass@host:port` (or `mqtts://...` for TLS). When `URI` is set, the individual broker fields are ignored. +Instead of `BROKER`/`PORT`/`USERNAME`/`PASSWORD`/`TLS`, you can provide a single `URI` of the form `mqtt[s]://[user[:pass]@]host[:port]`. When `URI` is set, the individual broker fields are ignored.-| `URI` | — | MQTT URI (`mqtt[s]://user:pass@host:port`); when set, overrides `BROKER`/`PORT`/`USERNAME`/`PASSWORD`/`TLS` | +| `URI` | — | MQTT URI (`mqtt[s]://[user[:pass]@]host[:port]`); when set, overrides `BROKER`/`PORT`/`USERNAME`/`PASSWORD`/`TLS` |Also applies to: 781-781
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 604 - 610, Update the README description for the MQTT URI to clarify that credentials are optional by changing the example/pattern from mqtt://user:pass@host:port to the formal pattern mqtt[s]://[user[:pass]@]host[:port] and add a short note that when the URI (URI) is set the individual BROKER/PORT/USERNAME/PASSWORD/TLS fields are ignored; ensure both occurrences (the paragraph showing the URI pattern and the example block) reflect the optional credentials formatting.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ha_addon/run.sh`:
- Around line 137-143: The emitted MQTT_INSIGHTS URI may contain embedded
credentials and is echoed into logs; update the branch that handles
bashio::config.has_value 'mqtt_uri' to redact userinfo before echoing or logging
by parsing the value from bashio::config 'mqtt_uri' (refer to the literal echo
"URI=$(bashio::config 'mqtt_uri')" and the surrounding block) and replacing the
user:pass@ segment with a masked placeholder (e.g., user:***@ or ****) or strip
credentials entirely before printing; ensure you use a small helper or shell
parameter expansion to detect userinfo (username:password@) and output the
redacted URI to the echo/log instead of the raw value.
In `@src/astrameter/config/config_loader.py`:
- Around line 86-113: parse_mqtt_uri currently silently ignores extra URI
components (path, params, query, fragment); update parse_mqtt_uri to validate
and reject any URI that contains non-empty parsed.path (other than optionally
"/"), parsed.params, parsed.query, or parsed.fragment by raising ValueError with
a clear message. Locate the validation in parse_mqtt_uri and after urlparse/host
checks add explicit guards that raise ValueError if parsed.path not in ("", "/")
or if parsed.params or parsed.query or parsed.fragment are truthy; keep existing
host/scheme/tls/port/credentials handling and add/update unit tests to assert
that URIs with path/query/fragment fail.
---
Nitpick comments:
In `@README.md`:
- Around line 604-610: Update the README description for the MQTT URI to clarify
that credentials are optional by changing the example/pattern from
mqtt://user:pass@host:port to the formal pattern
mqtt[s]://[user[:pass]@]host[:port] and add a short note that when the URI (URI)
is set the individual BROKER/PORT/USERNAME/PASSWORD/TLS fields are ignored;
ensure both occurrences (the paragraph showing the URI pattern and the example
block) reflect the optional credentials formatting.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3282c58c-d7be-41f3-b482-409327b056c1
📒 Files selected for processing (8)
README.mdconfig.ini.exampleha_addon/config.yamlha_addon/run.shha_addon/translations/en.yamlsrc/astrameter/config/config_loader.pysrc/astrameter/config/config_loader_test.pysrc/astrameter/powermeter/mqtt.py
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/astrameter/config/config_loader_test.py (2)
355-412: Add a test verifying URI takes precedence over legacy keys when both are present.The integration tests cover URI-only and legacy-keys-only scenarios well. However, the PR objective states that
BROKER/PORT/USERNAME/PASSWORD/TLS are ignored when URI is set. There's no test confirming that legacy keys are ignored when URI is present.This is important to catch regressions if the precedence logic is accidentally changed.
💡 Proposed test for URI precedence
def test_create_mqtt_powermeter_uri_overrides_legacy_keys(): """URI takes precedence; legacy BROKER/PORT/USERNAME/PASSWORD/TLS are ignored.""" config = configparser.ConfigParser() config["MQTT"] = { "URI": "mqtts://uri_user:uri_pass@uri.example.com:8884", "BROKER": "legacy.example.com", "PORT": "1883", "USERNAME": "legacy_user", "PASSWORD": "legacy_pass", "TLS": "false", "TOPIC": "home/power", } pm = create_mqtt_powermeter("MQTT", config) # All values should come from URI, not legacy keys assert pm.broker == "uri.example.com" assert pm.port == 8884 assert pm.username == "uri_user" assert pm.password == "uri_pass" assert pm.tls is True # From mqtts://, not from TLS=false def test_read_mqtt_insights_config_uri_overrides_legacy_keys(): """URI takes precedence for MQTT_INSIGHTS as well.""" config = configparser.ConfigParser() config["MQTT_INSIGHTS"] = { "URI": "mqtt://uri_user:uri_pass@uri.example.com:1885", "BROKER": "legacy.example.com", "PORT": "8883", "USERNAME": "legacy_user", "PASSWORD": "legacy_pass", "TLS": "true", } cfg = read_mqtt_insights_config(config) assert cfg is not None assert cfg.broker == "uri.example.com" assert cfg.port == 1885 assert cfg.username == "uri_user" assert cfg.password == "uri_pass" assert cfg.tls is False # From mqtt://, not from TLS=true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/astrameter/config/config_loader_test.py` around lines 355 - 412, Add tests asserting that when a URI is present it overrides legacy keys: create new tests calling create_mqtt_powermeter("MQTT", config) and read_mqtt_insights_config(config) with sections containing both "URI" and legacy keys ("BROKER","PORT","USERNAME","PASSWORD","TLS"), and assert that returned pm/cfg values (pm.broker, pm.port, pm.username, pm.password, pm.tls and cfg.broker, cfg.port, cfg.username, cfg.password, cfg.tls) come from the URI (including mqtt vs mqtts determining tls and default ports) rather than the legacy entries to ensure URI precedence in create_mqtt_powermeter and read_mqtt_insights_config.
295-352: Good test coverage for URI parsing; consider additional edge cases.The tests comprehensively cover the core parsing scenarios including percent-decoding, default ports, TLS detection, and rejection of invalid inputs. A few edge cases that could improve coverage:
- Username without password:
mqtt://alice@broker.example.com(password should beNone)- URI params rejection: The implementation checks
parsed.params, but no test verifies this (e.g.,mqtt://host;param=value)- IPv6 host:
mqtt://[::1]:1883to ensure bracket-enclosed IPv6 addresses parse correctlyThese are optional additions that would strengthen robustness.
💡 Example additional tests
def test_parse_mqtt_uri_username_only(): """Username without password is valid.""" parts = parse_mqtt_uri("mqtt://alice@broker.example.com") assert parts.username == "alice" assert parts.password is None def test_parse_mqtt_uri_rejects_params(): with pytest.raises(ValueError, match="params"): parse_mqtt_uri("mqtt://broker.example.com;param=value") def test_parse_mqtt_uri_ipv6_host(): parts = parse_mqtt_uri("mqtt://[::1]:1883") assert parts.host == "::1" assert parts.port == 1883🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/astrameter/config/config_loader_test.py` around lines 295 - 352, Add tests to cover missing-password usernames, URI params rejection, and bracketed IPv6 hosts: add a test calling parse_mqtt_uri("mqtt://alice@broker.example.com") asserting username == "alice" and password is None; add a test that expects ValueError with "params" when calling parse_mqtt_uri("mqtt://broker.example.com;param=value") to exercise the parsed.params rejection path; and add a test for parse_mqtt_uri("mqtt://[::1]:1883") asserting host == "::1" and port == 1883 so bracket-enclosed IPv6 parsing is validated. Ensure these tests reference parse_mqtt_uri from the same module so they exercise the existing params and host parsing logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/astrameter/config/config_loader_test.py`:
- Around line 355-412: Add tests asserting that when a URI is present it
overrides legacy keys: create new tests calling create_mqtt_powermeter("MQTT",
config) and read_mqtt_insights_config(config) with sections containing both
"URI" and legacy keys ("BROKER","PORT","USERNAME","PASSWORD","TLS"), and assert
that returned pm/cfg values (pm.broker, pm.port, pm.username, pm.password,
pm.tls and cfg.broker, cfg.port, cfg.username, cfg.password, cfg.tls) come from
the URI (including mqtt vs mqtts determining tls and default ports) rather than
the legacy entries to ensure URI precedence in create_mqtt_powermeter and
read_mqtt_insights_config.
- Around line 295-352: Add tests to cover missing-password usernames, URI params
rejection, and bracketed IPv6 hosts: add a test calling
parse_mqtt_uri("mqtt://alice@broker.example.com") asserting username == "alice"
and password is None; add a test that expects ValueError with "params" when
calling parse_mqtt_uri("mqtt://broker.example.com;param=value") to exercise the
parsed.params rejection path; and add a test for
parse_mqtt_uri("mqtt://[::1]:1883") asserting host == "::1" and port == 1883 so
bracket-enclosed IPv6 parsing is validated. Ensure these tests reference
parse_mqtt_uri from the same module so they exercise the existing params and
host parsing logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ae54fa7c-e17d-4e77-aab4-7d5412e9fb48
📒 Files selected for processing (4)
README.mdha_addon/run.shsrc/astrameter/config/config_loader.pysrc/astrameter/config/config_loader_test.py
✅ Files skipped from review due to trivial changes (1)
- README.md
🚧 Files skipped from review as they are similar to previous changes (2)
- ha_addon/run.sh
- src/astrameter/config/config_loader.py
Add a single `URI` option (e.g. `mqtt://user:pass@host:port` or `mqtts://...`) to the `[MQTT]` powermeter and `[MQTT_INSIGHTS]` sections, plus a matching `mqtt_uri` add-on option, so users can point MQTT Insights at an external broker instead of Home Assistant's bundled Mosquitto.
- Redact userinfo from any URI= line in print_redacted_config so the HA add-on no longer prints embedded credentials when starting up. - parse_mqtt_uri now rejects URIs that carry a path (other than "/"), params, query, or fragment to avoid silently dropping configuration. - Clarify README MQTT URI pattern to show that credentials and port are optional and that BROKER/PORT/USERNAME/PASSWORD/TLS are ignored when URI is set.
45c53a8 to
d8212fb
Compare
Regrouped the flat bullet list into Breaking / Added / Changed / Fixed subsections so readers can scan by change type. Each bullet is now a standalone diff to main (no cross-references) and cites every PR that contributed. Added Breaking entries that were missing: - CT002/CT003 ACTIVE_CONTROL default (smoothing + 15 W BALANCE_DEADBAND + saturation detection on by default) - WAIT_FOR_NEXT_MESSAGE default True (affects Shelly emulation too, not just CT002/CT003) - Async Powermeter base (out-of-tree subclasses must implement async get_powermeter_watts()) Added missing feature bullets: per-powermeter EMA smoothing/deadband wrappers (#331), Hampel outlier filter (#334), MQTT BROKER_URI (#309), exc_info on warnings (#307). Filled in previously-missing PR refs on the rebrand, CT002/CT003, MQTT Insights, web config editor, PID controller, and GIT_COMMIT_SHA bullets. https://claude.ai/code/session_01BCVmemteVXNfoTQE4De2CU
* Restructure CHANGELOG Next section as Keep-a-Changelog diff to main Regrouped the flat bullet list into Breaking / Added / Changed / Fixed subsections so readers can scan by change type. Each bullet is now a standalone diff to main (no cross-references) and cites every PR that contributed. Added Breaking entries that were missing: - CT002/CT003 ACTIVE_CONTROL default (smoothing + 15 W BALANCE_DEADBAND + saturation detection on by default) - WAIT_FOR_NEXT_MESSAGE default True (affects Shelly emulation too, not just CT002/CT003) - Async Powermeter base (out-of-tree subclasses must implement async get_powermeter_watts()) Added missing feature bullets: per-powermeter EMA smoothing/deadband wrappers (#331), Hampel outlier filter (#334), MQTT BROKER_URI (#309), exc_info on warnings (#307). Filled in previously-missing PR refs on the rebrand, CT002/CT003, MQTT Insights, web config editor, PID controller, and GIT_COMMIT_SHA bullets. https://claude.ai/code/session_01BCVmemteVXNfoTQE4De2CU * Fold same-cycle fixes into their parent Added bullets The Fixed / Changed bullets for CT002/CT003 saturation, efficiency- rotation lockup, and the MQTT_INSIGHTS empty-config crash / HA mosquitto availability check referenced features introduced in this same release cycle, so they weren't a standalone diff against main. Merged those PR refs into the CT002/CT003 and MQTT Insights Added bullets (the end state, which is what a main-viewer cares about). Modbus UNIT_ID fix stays in Fixed — Modbus existed on main. https://claude.ai/code/session_01BCVmemteVXNfoTQE4De2CU * Fold CT002/CT003 active-control defaults into Added bullet The CT002/CT003 ACTIVE_CONTROL default is not a 'changed default' vs main — CT002/CT003 don't exist on main, so the default is just part of the new feature description. Moved the default-on behavior and BALANCE_DEADBAND details into the CT002/CT003 Added bullet. Also narrowed the WAIT_FOR_NEXT_MESSAGE Breaking bullet to just the Shelly emulator (the real diff against main); the CT002/CT003 aspect is implicit in the CT002/CT003 Added bullet. Fixed a minor verb mismatch in Changed: 'Added battery activity info logs' → 'Expanded Shelly emulation logs'. https://claude.ai/code/session_01BCVmemteVXNfoTQE4De2CU --------- Co-authored-by: Claude <noreply@anthropic.com>
Summary
Added support for configuring MQTT brokers via a single URI parameter (e.g.,
mqtt://user:pass@host:1883ormqtts://...) as an alternative to specifying individualBROKER,PORT,USERNAME,PASSWORD, andTLSconfiguration options. This simplifies configuration and improves usability, especially for the Home Assistant add-on.Key Changes
New
parse_mqtt_uri()function: Parses MQTT URIs in the formatmqtt[s]://[user[:pass]@]host[:port]with support for percent-encoded credentials. Validates scheme, extracts host/port/credentials, and automatically sets TLS flag and default ports (1883 for mqtt, 8883 for mqtts).New
MqttUriPartsdataclass: Represents parsed MQTT connection details (host, port, username, password, tls).Updated
create_mqtt_powermeter(): Now checks for aURIconfig option first; if present, uses parsed URI values; otherwise falls back to individualBROKER/PORT/USERNAME/PASSWORD/TLSoptions.Updated
read_mqtt_insights_config(): Similarly supportsURIoption with fallback to individual configuration fields.TLS support in
MqttPowermeter: Addedtlsparameter to the constructor and SSL context creation in the_run()method for secure connections.Home Assistant add-on enhancements:
mqtt_uriconfiguration option toconfig.yamlrun.shto prioritize custommqtt_uriwhen provided, with appropriate loggingmqtt_urioptionDocumentation updates: Updated README and example config to document the new
URIoption for both[MQTT]and[MQTT_INSIGHTS]sections with examples.Comprehensive test coverage: Added 9 new tests covering URI parsing (valid/invalid cases), default ports, TLS detection, and integration with powermeter/insights config readers.
Implementation Details
urllib.parsewith percent-decoding for credentialsURIis specified, it takes precedence over individual broker configuration fieldsURIcontinue to work unchangedhttps://claude.ai/code/session_01DHFX8ucBEoysYSzAt9La66
Summary by CodeRabbit
New Features
Documentation
Tests