Take into consideration transitive dependencies, not just root struct fields #1501

gmarcosb · 2024-12-23T20:08:17Z

Fix for #1500

src-electron/generator/helper-zcl.js

bzbarsky-apple · 2025-01-02T21:35:13Z

This PR and changeset need a much clearer explanation of what is actually being changed and why.

gmarcosb · 2025-01-02T21:56:47Z

This PR and changeset need a much clearer explanation of what is actually being changed and why.

@bzbarsky-apple I added some more to #1500 this morning after discussion in zap channel - I wasn't sure how to explain this except to point to the existing compilation fail & why it's failing.

Or did you mean you just wanted me to pull the details from #1500 over here to the PR?

src-electron/generator/helper-zcl.js

brdandu

Please update one of the xml files and add a unit test for this as well.

gmarcosb · 2025-01-03T23:10:41Z

Please update one of the xml files and add a unit test for this as well.

I added some structs to zcl-builtin/matter/data-model/chip/test-cluster.xml, as partials/cluster-objects-struct.zapt seemed to be the only test that referenced struct_contains_array

src-electron/db/query-zcl.js

brdandu · 2025-01-06T10:40:05Z

src-electron/db/db-mapping.js

@@ -378,8 +378,7 @@ exports.map = {
      isNullable: dbApi.fromDbBool(x.IS_NULLABLE),
      isOptional: dbApi.fromDbBool(x.IS_OPTIONAL),
      isFabricSensitive: dbApi.fromDbBool(x.IS_FABRIC_SENSITIVE),
-      dataTypeReference: x.TYPE,


Do not change any of the dbmappings because that any existing helper could be using these in the .zapt templates and those will get broken. If you need a new key then just add to an existing dbmap with a different key name from the ones present currently.
Also, some of the dbmaps which are named after the tables actually should have only the table columns in the dbmap. If you intend to add new keys to the dbmap then it is a good idea to create a new db map with the 'extended' postfix in the name where you can add the columns from the joined tables.

No helper is using these (I checked), and these fields are deceiving as they are calculating an already existing value

See my other comment: #1501 (comment)

The existing matter sdk may not be using this but there could be an old sdk which was using this. ZAP promises to be backwards compatible with older sdk versions. Could you keep the existing ones and add dataTypeReferenceId: x.DATA_TYPE_REF, . I know the naming here is not the best in this case but this will make sure none of the older stacks are broken.

@gmarcosb ZAP has a lot of moving parts. As @brdandu mentions, there might be some unforeseen use case this is required for.

Considering this, I fully support cleaning up unnecessary code and appreciate the intent behind your efforts.. For the sake of your PR, it makes sense to maintain a strict scope of changes related to your linked issue, then submit a follow-up PR for cleanup.

I have to push back a bit here, because I think this is precisely what leads to unmaintainable code

Yes, I understand that there can always be unforeseen cases - every code change comes with risk.

But code which makes no sense should fall in one of 2 categories:

Dead code which should be removed ASAP

Unintuitive code that should be well-tested & have good test coverage

Going the route of "cleaning things up could lead to problems, so let's just continue to build debt" is precisely what leads to unmaintainable code

The worst that will happen here is that some template will break & we'll get a compile-time error, correct? If that happens, we rollback & add the respective test coverage?

That seems like a worthwhile risk to move us towards more maintainable & cleaner code

This is not just about unused code, but code which doesn't make sense - dataTypeReference, dataTypeReferenceName, & type all hold the same value, spread across 3 fields. If that's important, it should be well-tested. If it's an error, it should be corrected ASAP.

That sounds fine to me

Here is how it could be broken:

Matter developer has SDK version x.x.x which they were promised works with the chips they bought.

SDK version x.x.x was created with ZAP version X which is older than ZAP version Y

SDK version x.x.x has external helpers referencing this variable

If Matter developers updates ZAP to version Y their external helper will not return the correct value

That is a legitimate risk but at the same time I agree with you that we should not be maintaining duplicate data. We would need to determine how relevant that risk really is.

There is a non zero chance that this would break dev environments and people will be angry.

If they're running SDK version x.x.x, seems they should stay with ZAP version X

ZAP version & SDK version should be lockstep (this is generally true, which comes back to the argument we brought up before about the issues with the repos being separate)

There doesn't seem to be a valid use-case to update ZAP while not updating the SDK version

If someone does this & are upset, send them my way & I can talk to them about how ZAP & SDK should be updated together, as they are inter-dependent

In this case specifically (as there are no refs to these other 2 fields in SDK), I can also inform them that their current use of these fields is probably not having the desired effect, because they have always had the same value as type - that should allow them to do some cleanup as well, without even doing a rollback

Effectively, we're not removing a field which we think is unused - we're removing a field which has duplicate data & can instruct users to simply use type instead (which will work both with ZAP X & Y)

If they're running SDK version x.x.x, seems they should stay with ZAP version X -> When an issue is logged by a user for zap and we fix it then we do not request the user to update the SDK. It will be better out of box experience for users to just get the latest ZAP. From personal experience, I can say this is generally appreciated by users even though it is more work for the ZAP team.
Also, Please note that we also use ZAP internally for our own SDKs(Zigbee apart from Matter for us). I had to go through every release of ours in the last 2-3 years to see if either of the 2 keys were used anywhere. Hopefully I did not miss a release due to branch naming conventions which can change sometimes. Since I could not find an issue in this case yet, I can approve it but I can assure you backwards compatibility promises are not easy to maintain and it is not always feasible to do these checks. If an issue comes because of this in an old SDK application then it is not easy to backtrack and figure this issue out because it can get pretty crazy to backtrack.
It is my request to only add new keys unless we are really doing an all out cleaning. Even before cleaning up we will need to throw a warning that these dead keys will no longer be supported from the next release.

src-electron/db/query-zcl.js

zcl-builtin/matter/data-model/chip/test-cluster.xml

src-electron/generator/helper-zcl.js

zcl-builtin/matter/data-model/chip/test-cluster.xml

…ot transitively

brdandu · 2025-01-07T11:01:18Z

src-electron/db/db-mapping.js

@@ -378,8 +378,7 @@ exports.map = {
      isNullable: dbApi.fromDbBool(x.IS_NULLABLE),
      isOptional: dbApi.fromDbBool(x.IS_OPTIONAL),
      isFabricSensitive: dbApi.fromDbBool(x.IS_FABRIC_SENSITIVE),
-      dataTypeReference: x.TYPE,


The existing matter sdk may not be using this but there could be an old sdk which was using this. ZAP promises to be backwards compatible with older sdk versions. Could you keep the existing ones and add dataTypeReferenceId: x.DATA_TYPE_REF, . I know the naming here is not the best in this case but this will make sure none of the older stacks are broken.

src-electron/db/query-zcl.js

zcl-builtin/matter/data-model/chip/test-cluster.xml

brdandu · 2025-01-07T11:12:39Z

A couple of templates using struct_contains_array that I saw were cluster-objects-struct.zapt , struct.zapt and type-by-cluster.zapt . Another place to add the struct contains array test can be to update type-by-cluster.zapt and geo-meta.test.js => epc = genResult.content['type-by-cluster.h']

brdandu

Approving because I could not find an issue with the SDKs using the dbmapping keys changed here but I request not to delete or edit existing keys in dbmapping in the future.

src-electron/generator/helper-zcl.js

Co-authored-by: Boris Zbarsky <[email protected]>

… fields (project-chip#1501) * Take into consideration transitive dependencies, not just root struct * Apply changes addressing comments in PR * Added 2 new args in previous commit: <arg name="arg7" type="DoubleNestedStructList" /> <arg name="arg8" type="DeepNestedStructList" /> * Calculate struct_has_fabric_sensitive_fields only at the top level, not transitively * Add test for struct_contains_array * Apply suggestions from code review Co-authored-by: Boris Zbarsky <[email protected]> * Fixup references after applying suggestions from code review --------- Co-authored-by: Boris Zbarsky <[email protected]>

Take into consideration transitive dependencies, not just root struct

7576739

gmarcosb requested review from paulr34, tecimovic and brdandu as code owners December 23, 2024 20:08

gmarcosb mentioned this pull request Dec 23, 2024

struct_contains_array must calculate transitive dependencies #1500

Open

bzbarsky-apple requested changes Jan 2, 2025

View reviewed changes

src-electron/generator/helper-zcl.js Outdated Show resolved Hide resolved

src-electron/generator/helper-zcl.js Outdated Show resolved Hide resolved

brdandu requested changes Jan 3, 2025

View reviewed changes

src-electron/generator/helper-zcl.js Outdated Show resolved Hide resolved

brdandu requested changes Jan 3, 2025

View reviewed changes

Apply changes addressing comments in PR

b4eb927

gmarcosb commented Jan 3, 2025

View reviewed changes

src-electron/db/query-zcl.js Show resolved Hide resolved

Added 2 new args in previous commit:

2fa580a

<arg name="arg7" type="DoubleNestedStructList" /> <arg name="arg8" type="DeepNestedStructList" />

brdandu requested changes Jan 6, 2025

View reviewed changes

bzbarsky-apple reviewed Jan 6, 2025

View reviewed changes

src-electron/generator/helper-zcl.js Outdated Show resolved Hide resolved

src-electron/generator/helper-zcl.js Show resolved Hide resolved

zcl-builtin/matter/data-model/chip/test-cluster.xml Show resolved Hide resolved

gmarcosb requested review from bzbarsky-apple and brdandu January 6, 2025 18:43

Calculate struct_has_fabric_sensitive_fields only at the top level, n…

f93714c

…ot transitively

gmarcosb force-pushed the bugfix branch from e88e7cf to f93714c Compare January 6, 2025 21:03

brdandu requested changes Jan 7, 2025

View reviewed changes

Add test for struct_contains_array

522d953

paulr34 approved these changes Jan 7, 2025

View reviewed changes

gmarcosb requested a review from brdandu January 7, 2025 18:31

brdandu approved these changes Jan 9, 2025

View reviewed changes

bzbarsky-apple approved these changes Jan 9, 2025

View reviewed changes

src-electron/generator/helper-zcl.js Outdated Show resolved Hide resolved

src-electron/generator/helper-zcl.js Outdated Show resolved Hide resolved

src-electron/generator/helper-zcl.js Outdated Show resolved Hide resolved

src-electron/generator/helper-zcl.js Outdated Show resolved Hide resolved

gmarcosb and others added 2 commits January 9, 2025 14:13

Apply suggestions from code review

56af968

Co-authored-by: Boris Zbarsky <[email protected]>

Fixup references after applying suggestions from code review

a6d5efb

paulr34 merged commit 8e4ff10 into project-chip:master Jan 10, 2025
13 checks passed

gmarcosb added a commit to gmarcosb/connectedhomeip that referenced this pull request Jan 10, 2025

Update ZAP version to include project-chip/zap#1501

d4ec66f

gmarcosb mentioned this pull request Jan 10, 2025

Update ZAP version project-chip/connectedhomeip#37039

Merged

mergify bot pushed a commit to project-chip/connectedhomeip that referenced this pull request Jan 13, 2025

Update ZAP version to include project-chip/zap#1501 (#37039)

0dd3414

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Take into consideration transitive dependencies, not just root struct fields #1501

Take into consideration transitive dependencies, not just root struct fields #1501

gmarcosb commented Dec 23, 2024

bzbarsky-apple commented Jan 2, 2025

gmarcosb commented Jan 2, 2025 •

edited

Loading

brdandu left a comment

gmarcosb commented Jan 3, 2025

brdandu Jan 6, 2025

gmarcosb Jan 6, 2025

brdandu Jan 7, 2025

paulr34 Jan 7, 2025 •

edited

Loading

gmarcosb Jan 7, 2025 •

edited

Loading

paulr34 Jan 7, 2025

paulr34 Jan 7, 2025

gmarcosb Jan 7, 2025 •

edited

Loading

paulr34 Jan 7, 2025

brdandu Jan 9, 2025

brdandu Jan 7, 2025

brdandu commented Jan 7, 2025

brdandu left a comment

Take into consideration transitive dependencies, not just root struct fields #1501

Take into consideration transitive dependencies, not just root struct fields #1501

Conversation

gmarcosb commented Dec 23, 2024

bzbarsky-apple commented Jan 2, 2025

gmarcosb commented Jan 2, 2025 • edited Loading

brdandu left a comment

Choose a reason for hiding this comment

gmarcosb commented Jan 3, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulr34 Jan 7, 2025 • edited Loading

Choose a reason for hiding this comment

gmarcosb Jan 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmarcosb Jan 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brdandu commented Jan 7, 2025

brdandu left a comment

Choose a reason for hiding this comment

gmarcosb commented Jan 2, 2025 •

edited

Loading

paulr34 Jan 7, 2025 •

edited

Loading

gmarcosb Jan 7, 2025 •

edited

Loading

gmarcosb Jan 7, 2025 •

edited

Loading