Templates: pass bool params as real booleans - #33153
Open
ickeundso wants to merge 4 commits into
Open
Conversation
Bool params were passed to the render as strings, so templates had to compare
against "true" and plain `{{ if .param }}` was always true - the non-empty
string "false" is truthy. Bool params are now real booleans, all comparisons
use the boolean form, and a new test renders every template with each bool
param set to true and false in every usage, so a reintroduced string
comparison fails the build instead of silently misrendering.
Fixes toggles that had no effect because the template used truthiness:
demo-battery controllable, demo-meter curtailable, deye-hybrid-3p
includegenport, openems-modbus battery, vehicle-api geofence_enabled, and
messenger/homeassistant critical (ttl/priority were sent unconditionally).
Two templates needed an explicit default to keep their current behavior:
batterx externalpv (unset previously matched `ne "false"`) and flobz
wakeup_alt, which was used as a bool but never declared as one.
The config UI seeded bool defaults as strings, which made "field is empty"
checks see a filled field; defaults are now booleans there as well.
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="util/templates/template.go" line_range="431-439" />
<code_context>
}
}
+ // bool params are rendered as real booleans so templates can use
+ // `{{ if .param }}` instead of comparing against "true"/"false"
+ for _, p := range t.Params {
+ if p.Type == TypeBool {
+ if s, ok := res[p.Name].(string); ok {
+ res[p.Name] = s == "true"
+ }
+ }
+ }
+
// class-local includes take precedence over the global ones
</code_context>
<issue_to_address>
**issue (broader_impact):** Boolean conversion is limited to declared `t.Params`, so predefined boolean-like properties such as `tcpip`, `udp`, `rs485serial`, and `rs485tcpip` remain strings in the render context. The `modbus.tpl` truthiness checks therefore still treat the stored string "false" as true and select the wrong Modbus branch.
**Triggers:** When a Modbus configuration contains one of the predefined interface properties explicitly set to the string "false".
**Suggested fix:** Convert the predefined boolean properties before rendering as well, or update `modbus.tpl` to compare their values using the same normalized boolean representation.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 1 finding to address first, and this changes boolean parameter semantics in the shared renderer and across many device templates, so an incorrect conversion or migrated condition could produce a wrong device configuration or feature set. Reverting prevents future incorrect renders, but configurations already generated or applied would need to be regenerated or repaired.
Blocking findings: util/templates/template.go:439
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
andig
reviewed
Aug 26, 2026
andig
reviewed
Aug 26, 2026
Validate the bool example as well as the default, it is used as the default in docs and unit test mode. Vary every choice param in TestAllTemplatesRenderWithBoolParams, not just usage. `and`/`or` short-circuit, so a string comparison behind another choice param was never reached, e.g. deye-hybrid-3p only evaluates `.firmware1098` for `batterytype: hv`. Coerce stored string values in applyDefaultsFromTemplate, not just defaults. Configs written before bool params became real booleans hold "true"/"false", and the boolean SelectGroup options match neither. Revert the unrelated res[out] type assertion guard in RenderResult.
mergeMaps folds keys with strings.EqualFold, the same comparison ParamByName uses, so a declared param cannot end up under two keys and the conversion does not have to wait for the best-value selection to settle. Predefined properties still can, but none of them is a declared bool, hence the ParamByName guard. Name the offending field in the bool validation error.
ParamType is an int enum with TypeString as its zero value, so a predefined property, which has no param and therefore a zero Param, can never compare equal to TypeBool.
Member
|
Great PR, thanks for drilling into this one. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to the discussion in #32219.
Bool params reached the template render as strings, so templates had to compare
eq .param "true"and a plain{{ if .param }}was always true — the non-empty string"false"is truthy. Bool params are now real booleans, every comparison uses the boolean form, and a bool default is validated to betrue/false.This fixes six toggles that never had an effect, because their template used truthiness:
meter/openems-modbusbatterybatterymodeblock always renderedmessenger/homeassistantcriticalttl: 0/priority: highalways sentmeter/demo-batterycontrollablebatterymodeblock always renderedmeter/demo-metercurtailablecurtailblock always renderedmeter/deye-hybrid-3pincludegenportcharger/vehicle-apigeofence_enabledEvery template was rendered with each of its bool params unset,
trueandfalsebefore and after the change and the outputs diffed — the table is the result of that comparison.Note for existing installations: openems-modbus users relying on the unintended always-on battery control, and HA-messenger users relying on unconditional
ttl: 0/priority: high, have to enable the respective toggle now.Otherwise backward compatible. String
"true"/"false"from stored configs keeps working, and an unset bool renders asfalseinstead of empty, which decodes to the same zero value — verified that no affected Go struct pre-sets those fields. Two templates needed an explicit default to pin current behaviour:meter/batterxexternalpv(unset previously matchedne "false") andvehicle/flobzwakeup_alt, which was used as a bool but declared untyped.The config UI seeded bool defaults as strings, which made its "field is empty" checks see a filled field; defaults are booleans there as well now.
TestAllTemplatesRenderWithBoolParamsrenders every template across all classes with each bool param set both ways in every usage, so a reintroduced string comparison fails the build. It earned its keep during the rebase:demo-meter curtailable,deye-hybrid-3p includegenportandfirmware1098had picked up the string form in the meantime and were caught immediately.Not covered:
util/templates/modbus.tpluses the same truthiness pattern on.tcpip/.udp/.rs485serial/.rs485tcpip. Those arepredefinedTemplatePropertiesrather than declared params, so this change does not reach them and a config withtcpip: falsestill selects the wrong branch. Same bug class, but it needs its own fix.