mfucci / node-matter

Matter protocol client / server for node.js
Apache License 2.0
119 stars 10 forks source link

Initialize all Attribute fields for Server #223

Closed Apollon77 closed 1 year ago

Apollon77 commented 1 year ago

While playing around with subscriptions I noticed that we only initialize the attributes that are defined with default values for a cluster. Fields that are not in the "attributesInitialValues" are potentially never initialized correctly.

To fix this I now iterate over all attribute fields. But this means that we now could have undefined as value which basically is better then mitting attributeservers. For this I added filter in the relevant places to filter out undefined values when sending read responses or DataReports on subscriptions.

mfucci commented 1 year ago

Typescript compiler forces to define all mandatory attributes in attributesInitialValues. Optional attributes can optionally also be defined in attributesInitialValues. So I think initializing only attributesInitialValues defined attributes is correct, since this is:

Commands only have access to mandatory attributes, so they are all initialized.

Can you point out which are the case were an attribute might not be initialized correctly?

Apollon77 commented 1 year ago

OptionalWritable attribute that is written later? Would be missed now and return an error. And it felt wrong to then "initialize" it dynamically.

And I'm not that sure that a command handler would never need/try to access an optionalAttribute - I did not yet read all specs. I assume we would then search a while ;-)

And last but not least the ClusterClient for use As controller initialize all attributes (yes he needs to, but so behaves slightly different).

But also to prepare a consistent high level api it would be hard when the available get methods differ based on the fact which data were set with initial values.

Apollon77 commented 1 year ago

I checked again and yes I stumbled over this topci while implementing the "WriteRequests" - not "TS compiler warning wise" but semantically. The "getAttribute" in IntractionServer.ts would not find such an attribute because it iterates over the attributes that got initialized by CLusterServer ... so all thee would be missing for write requests. At minimum there it is unexpected because we would return an error about an unsupported Write.

So I have decided to not add "special handling and dynamic delayed Attribute cluster initialization" on WriteRequests but fix the underlying issue directly ... This PR is for that.

mfucci commented 1 year ago

I still don't understand what you are trying to fix, can you walk me through the flow that creates the problem?

Here is how I implemented it.

Let's say, we have a cluster C with attributes:

When initializing the cluster, A initial values needs to be provided, and optionally B / C. The command handlers can only access A which is always defined. If a command needs to access the optional attribute B, and can be redeclared as mandatory, using UseOptionalAttributes

Let's say that A and B are initialized on the cluster. read A => valueA (mandatory and always defined) read B => valueB (optional but always defined since initialized at cluster creation) read C => nothing (optional and not present on the cluster)

write A => works write B => works since the AttributeServer has been created write C => error since C doesn't exist (the optional AttributeServer was not created)

Optional attributes can't be initialized dynamically: they are either initialized at the creation of the cluster or they don't exist