fix(node): filter getDefaults() to conformant members - #4380
Draft
lboue wants to merge 3 commits into
Draft
Conversation
…onType DayEntryStruct.RandomizationType declared both conformance "[RNDM]" (only allowed when the Randomization feature is supported) and default: 0. Writing a dayEntries list entry that omits RandomizationType auto-filled it with that default before validating conformance, then rejected the very same fill: Conformance "[RNDM]": Matter does not allow you to set this attribute A bare (non-list) struct attribute with the identical field shape (e.g. CurrentDayEntry) did not trip this — the auto-fill-then-reject only happens along the list-of-struct write path (Endpoint.setStateOf -> supervisor.patch -> StructManager), not a plain state property assignment. Per the spec text for both RandomizationOffset and RandomizationType (Matter 1.6 Application Cluster Spec, cluster section 9.12.5.10.4-5, as transcribed in commodity-tariff.resource.ts): "If this field is not indicated, randomization shall use the value in the Default* attribute." Absence is spec-meaningful (fall back to the cluster's own DefaultRandomizationType), not "value is None" — RandomizationOffset already has no default in the model, matching that; RandomizationType's default: 0 was inconsistent with its sibling field and collapsed that fallback semantic even when Randomization was enabled, on top of causing the conformance crash when it wasn't. Root-caused and reported against Luligu/matterbridge#625 (their ElectricalUtilityMeter.addElectricalMeter()'s energyTariff option hits this exact path). Full crash log and analysis in the linked issue. Related issue: matter-js#4374 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…default
The actual bug isn't in the model: the spec's own DayEntryStruct field table
(Application Cluster Spec § 9.12.5.10, page 798-800 of 23-27350-010) gives
RandomizationType a Fallback of "None" — matching what CHIP's data model XML
encodes (default="None") and what the matter.js model already had. My first
pass (previous commit) removed that default to make the crash go away, but
that also removes the spec-mandated fallback for when Randomization *is*
supported and a day entry omits the field.
Root cause is in ValuePatcher.ts's getDefaults(): it calls
`supervisor.membersOf(schema)` with no conformance filter, so it considers
every declared member's `default" regardless of whether that member is even
conformant given the currently active features. SelectDefaultValue() (in
@matter/model) already guards this correctly with
`scope.hasOperationalSupport(member)`; getDefaults() has its own, separate
member-default computation that skipped that check. The fix passes
`{ conformance: "conformant" }` to `membersOf()`, matching the filter
StructManager.ts already uses for the (working) mandatory-default synthesis
path.
Restores commodity-tariff.element.ts's `default: 0` and adds a second test
confirming the fix preserves the spec's "None" fallback once Randomization
is enabled and the field is omitted, alongside the existing regression test
for the conformance-disabled crash.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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
1 participant
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.
Type of change
Description
Supersedes #4375, which was correctly closed: that first pass hand-edited the generated
commodity-tariff.element.tsto dropRandomizationType'sdefault: 0/"None"— but that default isspec-mandated (Application Cluster Specification § 9.12.5.10,
DayEntryStructfield table:RandomizationType'sFallback column is
None), so removing it "fixed" the crash by breaking the spec-correct read fallback forwhen
Randomizationis supported and the field is omitted.Actual root cause:
ValuePatcher.ts'sgetDefaults()callssupervisor.membersOf(schema)with noconformance filter, so it considers every declared member's
defaultregardless of whether that member iseven conformant given the currently active features. Writing a
dayEntrieslist entry that omitsRandomizationTypesynthesizes its default (None) even whenRandomizationis unsupported — where thefield can't exist at all — then rejects that very same synthesized value for violating the conformance gate
it was never asked to satisfy:
SelectDefaultValue()(@matter/model) already guards this correctly withscope.hasOperationalSupport(member);StructManager.ts's mandatory-default synthesis path already callsmembersOf(member, { conformance: "conformant" })for the same reason.
getDefaults()has its own, separate member-default computation that skipped thatfilter. This only surfaces on the list-of-struct write path — a bare, non-list struct attribute with the
identical field shape (e.g.
CurrentDayEntry) doesn't trip it.Fix: pass
{ conformance: "conformant" }tomembersOf()ingetDefaults(), matching the filter alreadyused elsewhere. No model files touched;
commodity-tariff.element.ts'sdefault: 0stays as originallydeclared.
Backing evidence
(Log attached on #4374, from the real-world crash in a Matterbridge plugin.) Two tests in
CommodityTariffServerTest.tscover both sides:Randomizationunsupported (the crash — omittingRandomizationTypemust be accepted) andRandomizationsupported (omitting it must still read back as thespec's
Nonefallback, notundefined) — both fail without this fix, both pass with it.Checklist
npm testpasses — ran@matter/model's build,@matter/node's fulltest/behaviors/**suite (391 tests, including both new regression tests) and@matter/general's suite (1330/1330).npm run format-verifyandnpm run lintpass (both run at the repo root, not just on the touched files)