fix: Use Jinja-safe property access in HA discovery templates#31930
Conversation
yo3gnd
force-pushed
the
yo3gnd-fix-ha-jinja-property-access
branch
from
02b29f0 to
304730b
Compare
| const valueJsonProperty = (property: string): string => { | ||
| const first = property.charCodeAt(0); | ||
|
|
||
| return first >= 48 && first <= 57 ? `value_json[${JSON.stringify(property)}]` : `value_json.${property}`; |
There was a problem hiding this comment.
Why not always use value_json["property"]?
There was a problem hiding this comment.
It would be valid, but it would rewrite every generated HA discovery template rather than only the broken cases. I kept dot notation for identifier-safe properties so existing discovery payloads stay unchanged where they already work, and bracket notation is only used where dot notation falls over. It also keeps JSON.stringify(...) out of the common path after the perf concern above.
There was a problem hiding this comment.
Fair enough. I originally kept stringify because this is generating a string literal inside a template, and I was being defensive about possible awkward property names. After checking the exposed property names from the herdsman converters, they are much more disciplined than I first assumed.
So for the narrowed digit-prefix case, plain quotes are enough. Patched.
There was a problem hiding this comment.
but it would rewrite every generated HA discovery template
I don't think that's really a problem, it would simplify the code a bit.
There was a problem hiding this comment.
I changed the helper to always emit bracket notation for dynamic expose properties.
This makes the test diff larger because exact HA discovery payloads move from value_json.foo to value_json["foo"], but entity identity is unchanged: topics, object IDs, unique IDs and command topics stay the same. Fixed bridge/internal templates still use the old, existing hard-coded dot notation.
Focused HA discovery coverage for the changed paths passes: 13 selected tests green. I kept validation scoped because the broader HA discovery file currently has unrelated group/effect-list expectation drift on dev.
Edit: 🤦♂️
yo3gnd
force-pushed
the
yo3gnd-fix-ha-jinja-property-access
branch
3 times, most recently
from
05a01e3 to
2e61fd6
Compare
yo3gnd
force-pushed
the
yo3gnd-fix-ha-jinja-property-access
branch
from
2e61fd6 to
3d0423f
Compare
| expect(duplicated).toStrictEqual([]); | ||
| }); | ||
|
|
||
| it("Should use bracket notation for discovery templates with numeric property names", () => { |
There was a problem hiding this comment.
I think these test can be removed? It will be tested by the other tests already.
yo3gnd
force-pushed
the
yo3gnd-fix-ha-jinja-property-access
branch
from
3d0423f to
c2426d6
Compare
|
thanks! |
3 participants
Related to #31888.
Home Assistant discovery templates were being generated as
{{ value_json.<property> }}across a fair few entity types. That is fine for ordinary identifiers, but it goes sideways once the property name is not a valid Jinja identifier, for example 24_hours_records.This change switches generated expose-property access to bracket notation, e.g.
{{ value_json["24_hours_records"] }}.The important bit is that this does not rename entities, discovery object IDs, MQTT payload keys, or any of the surrounding discovery structure. It only changes how Home Assistant reads the same values back out of value_json. The regression tests I used while narrowing this down have since been dropped again, as the existing Home Assistant discovery coverage already exercises these paths well enough.