[ACL][WriteClient] Encoding non-empty ReplaceAll when writing to AccessControl Cluster #38263
Conversation
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
Closed
5 tasks
|
PR #38263: Size comparison from 2a9cb23 to 6eb1bb6 Increases above 0.2%:
Full report (75 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
cecille
reviewed
Apr 4, 2025
There was a problem hiding this comment.
I looked mostly through the test portion and only skimmed the write interaction. Tests look correct with a minor comment re: disabling test steps. @andreilitvin - I think you're familiar with the write client. If you are OK with that, we could call this a google tag-team and checkmark it.
|
PR #38263: Size comparison from f20194e to 055af80 Increases above 0.2%:
Full report (75 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
3 tasks
|
PR #38263: Size comparison from f20194e to c5d6fa2 Increases above 0.2%:
Full report (75 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
src/app/clusters/access-control-server/access-control-server.cpp
Outdated
Show resolved
Hide resolved
1b5e9d1 to
9feb584
Compare
|
PR #38263: Size comparison from c0a7934 to 9feb584 Increases above 0.2%:
Full report (46 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, nrfconnect, psoc6, qpg, stm32, telink, tizen)
|
|
PR #38263: Size comparison from c0a7934 to ddf5138 Increases above 0.2%:
Full report (75 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
PR #38263: Size comparison from c0a7934 to 5d7908f Increases above 0.2%:
Full report (75 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
j-ororke
added a commit
to j-ororke/connectedhomeip
that referenced
this pull request
Apr 17, 2025
- Following test steps mentioned in test plan PR #[5061](CHIP-Specifications/chip-test-plans#5061) from Amine - Using base branch of PR #[38263](project-chip#38263)
Apollon77
added a commit
to matter-js/matter.js
that referenced
this pull request
Apr 18, 2025
... until we adjust for test excluded till we adjusted for project-chip/connectedhomeip#38263 (which is Matter 1.5 anyway)
mergify bot
pushed a commit
to matter-js/matter.js
that referenced
this pull request
Apr 18, 2025
* Patch/Exclude some ACL tests ... until we adjust for test excluded till we adjusted for project-chip/connectedhomeip#38263 (which is Matter 1.5 anyway) * ok needs more excludes * ok needs more excludes * ok needs more excludes * ok needs more excludes * ok needs more excludes * ok needs more excludes * ok adjustment to one OPSTATE test
j-ororke
added a commit
to j-ororke/connectedhomeip
that referenced
this pull request
Apr 18, 2025
- Following test steps mentioned in test plan PR #[5061](CHIP-Specifications/chip-test-plans#5061) from Amine - Using base branch of PR #[38263](project-chip#38263)
j-ororke
added a commit
to j-ororke/connectedhomeip
that referenced
this pull request
Apr 19, 2025
- Adding python3 ACL_2_4 test module - This uses base branch of Amine's PR project-chip#38263 to create this test module: This is a follow-up PR to that PR from Amine, once Amine's PR is merged this one can come out of WIP state.
j-ororke
added a commit
to j-ororke/connectedhomeip
that referenced
this pull request
Apr 20, 2025
- Added TC_ACL_2_3 python3 test module - This is a follow up PR to Amine's PR: project-chip#38263 - Removed Test_TC_ACL_2_3.yaml script
dsavitsky-dsr
pushed a commit
to popovdg/connectedhomeip
that referenced
this pull request
Apr 22, 2025
…ssControl Cluster (project-chip#38263) * Modifying how ACL Attribute is written: making it use a non-empty initial ReplaceAll list * removing Test modifications on ACL Extensions * adding reference to issue in TODO comment * integrating comments * integrating comments * Revert "removing Test modifications on ACL Extensions" This reverts commit 055af80. * Clarifying AttributeDataIB Checkpoint * adding Valid CAT values * Integrating comments * Add comments clarifying AttributeDataIB rollback * Integrating comments again * change method order * integrating new comments * revert change to IM Error Code return from AccessControl cluster * changing constants use to reserveBuffer for TLV writing * Integrating more Boris Comments * removing state related to IsWriteRequestChunked * Create TryToStartList that dynamically starts message in case of NO Memory
mergify bot
pushed a commit
that referenced
this pull request
Apr 24, 2025
* Modifying how ACL Attribute is written: making it use a non-empty initial ReplaceAll list * removing Test modifications on ACL Extensions * adding reference to issue in TODO comment * integrating comments * integrating comments * Revert "removing Test modifications on ACL Extensions" This reverts commit 055af80. * Testing removing one of the template functions * WriteClient: Encoding as many items as possible into a single list as part of ReplaceAll item operation * fixing failures * Fix to Error Handling in User-Label Cluster: return RESOURCE_EXHAUSTED when the number of items in list exceeds kMaxUserLabelListLength * chip-repl: support for preencoded attributes * Fixing ACL 2.3 and 2.5 * Validating ACLs Upfront: This is done to make sure we reject the whole list if any of the entries within it are non conforming * more yaml test changes * Restyled by prettier-yaml * Fix Unit Testing Cluster (Test Cluster Server) to process ReplaceAll operations * fix PutPreencodedAttribute * Cleaning code * More Code Clean Up * Unit Test Cleanup * more cleanup * Adding testcases to TestAccessControlCluster * Fixing comments * Making IsValid method in AccessControl Static * Adding comments * Make IsValid a member function of AccessControl::Entry * Adding WriteClient class member mIsWriteRequestChunked * making TestWriteChunkig more Robust * Adding ACL_2_4 python3 test module: - Adding python3 ACL_2_4 test module - This uses base branch of Amine's PR #38263 to create this test module: This is a follow-up PR to that PR from Amine, once Amine's PR is merged this one can come out of WIP state. * Restyled by autopep8 * Updating TC_ACL_2_4 python3 test module: - Changing expect clause for test step 29 * Updating TC_ACL_2_4: - Resolving linting errors * Apply commit suggestions from code review from Amine Updating to include commit suggestions to test steps and comments from @Alami-Amine. These suggestions made the code cleaner and more readable, thank you! Co-authored-by: Amine Alami <[email protected]> * Updating TC_ACL_2_4 python3 test module to resolve commits from Amine, making the code cleaner and better: - Updating to change type of ID from "fabric-scoped node ID" with correct type "invalid Group Node ID" - Updating test step 4 description to be more detailed. * Restyled by autopep8 * Update src/python_testing/TC_ACL_2_4.py Changing hex values for CAT1-CAT4 in test step 21 as suggested by Amine who gathered these from the test spec Co-authored-by: Amine Alami <[email protected]> * Updating TC_ACL_2_4 python3 test module: - Replacing duplicate CAT4 value in test step 21 and replaced it with value from Amine that did not replicate CAT3's value * Updating TC_ACL_2_4 test module: - Updating to match test steps in test plan PR: CHIP-Specifications/chip-test-plans#5052 * Pulling in files from upstream master to make sure that everything is in sync * Updating TC_ACL_2_4 python3 test module: - Removing try except blocks - Replacing self.print_step() with logging.info() * Restyled by autopep8 * Updated TC_ACL_2_4 python3 test module: - Added new test step method to step 29 - Updated test steps after test step 29 include the test step ordering - Updated test 29 and 30 to include the correct verbiage for the definitions and expected results * Updated TC_ACL_2_4 test module: - Actually updated TC_ACL_2_4 python3 test module to correct expected results verbiage for test steps 32-44 * Restyled by autopep8 * Update src/python_testing/TC_ACL_2_4.py Co-authored-by: Amine Alami <[email protected]> * Removing Test_TC_ACL_2_4.yaml script * Updating TC_ACL_2_4 python3 test module: - Replaced enumerate blocks with asserts.assert_in() checks to verify that structs are read correctly throughout the test - Removed some unneeded comments in the test module * Restyled by autopep8 * Updating TC_ACL_2_4 python3 test module: - Added in validation of admin entry only to end of test steps 32-44 - Removed duplicate validation of admin entry only in test step 31 --------- Co-authored-by: Alami-Amine <[email protected]> Co-authored-by: Restyled.io <[email protected]> Co-authored-by: Amine Alami <[email protected]>
mergify bot
pushed a commit
that referenced
this pull request
Apr 24, 2025
* Modifying how ACL Attribute is written: making it use a non-empty initial ReplaceAll list * removing Test modifications on ACL Extensions * adding reference to issue in TODO comment * integrating comments * integrating comments * Revert "removing Test modifications on ACL Extensions" This reverts commit 055af80. * Clarifying AttributeDataIB Checkpoint * adding Valid CAT values * Integrating comments * Add comments clarifying AttributeDataIB rollback * Integrating comments again * change method order * Creating ACL_2_6 python3 test module: - Following test steps mentioned in test plan PR #[5061](CHIP-Specifications/chip-test-plans#5061) from Amine - Using base branch of PR #[38263](#38263) * Restyled by autopep8 * Restyled by isort * Updating TC_ACL_2_6 python3 test module: - Resolving linting errors - Added additional checks for test step 3 and 5 to include struct validation * Updated TC_ACL_2_6 python test module: - Rebased branch with upstream master - Changed to checking for 2 new elements during test step 5 - Changed print_step() to logging.info() * Restyled by autopep8 * Updating to the upstream master branch version of WriteClient.cpp, WriteClient.h, Test_TC_ACL_2_5.yaml, and TestWriteChunking.cpp, to remove changes included in commit on this branch * Update src/python_testing/TC_ACL_2_6.py Co-authored-by: Amine Alami <[email protected]> * Removing Test_TC_ACL_2_6 yaml script * Updating TC_ACL_2_6 python3 test module: - Updating endpoint to 0 for REPL Linux CI checks to pass as AccessControl cluster events are on endpoint 0 * Updating TC_ACL_2_6 python3 test module: - Updating to using a simple for loop to validate the ReadEvent() response data - Removed unneeded asyncio.wait() from prior debugging - Created a short follow-up task PR to add subscription to this test module later * Update TC_ACL_2_6.py Resolving linting errros --------- Co-authored-by: Alami-Amine <[email protected]> Co-authored-by: Restyled.io <[email protected]> Co-authored-by: Amine Alami <[email protected]>
mergify bot
pushed a commit
that referenced
this pull request
Apr 24, 2025
* Modifying how ACL Attribute is written: making it use a non-empty initial ReplaceAll list * removing Test modifications on ACL Extensions * adding reference to issue in TODO comment * integrating comments * integrating comments * Revert "removing Test modifications on ACL Extensions" This reverts commit 055af80. * Clarifying AttributeDataIB Checkpoint * adding Valid CAT values * Integrating comments * Add comments clarifying AttributeDataIB rollback * Integrating comments again * change method order * Creating ACL_2_3 python3 test module: - Added TC_ACL_2_3 python3 test module - This is a follow up PR to Amine's PR: #38263 - Removed Test_TC_ACL_2_3.yaml script * Actually removing Test_TC_ACL_2_3.yaml script * Resolving restylzer/autopep8 issue with script manually as provided CI patch failed * Resolving linting errors * Apply suggestions from Amine's code review Co-authored-by: Amine Alami <[email protected]> --------- Co-authored-by: Alami-Amine <[email protected]> Co-authored-by: Amine Alami <[email protected]>
dsavitsky-dsr
pushed a commit
to popovdg/connectedhomeip
that referenced
this pull request
Apr 29, 2025
* Modifying how ACL Attribute is written: making it use a non-empty initial ReplaceAll list * removing Test modifications on ACL Extensions * adding reference to issue in TODO comment * integrating comments * integrating comments * Revert "removing Test modifications on ACL Extensions" This reverts commit 055af80. * Testing removing one of the template functions * WriteClient: Encoding as many items as possible into a single list as part of ReplaceAll item operation * fixing failures * Fix to Error Handling in User-Label Cluster: return RESOURCE_EXHAUSTED when the number of items in list exceeds kMaxUserLabelListLength * chip-repl: support for preencoded attributes * Fixing ACL 2.3 and 2.5 * Validating ACLs Upfront: This is done to make sure we reject the whole list if any of the entries within it are non conforming * more yaml test changes * Restyled by prettier-yaml * Fix Unit Testing Cluster (Test Cluster Server) to process ReplaceAll operations * fix PutPreencodedAttribute * Cleaning code * More Code Clean Up * Unit Test Cleanup * more cleanup * Adding testcases to TestAccessControlCluster * Fixing comments * Making IsValid method in AccessControl Static * Adding comments * Make IsValid a member function of AccessControl::Entry * Adding WriteClient class member mIsWriteRequestChunked * making TestWriteChunkig more Robust * Adding ACL_2_4 python3 test module: - Adding python3 ACL_2_4 test module - This uses base branch of Amine's PR project-chip#38263 to create this test module: This is a follow-up PR to that PR from Amine, once Amine's PR is merged this one can come out of WIP state. * Restyled by autopep8 * Updating TC_ACL_2_4 python3 test module: - Changing expect clause for test step 29 * Updating TC_ACL_2_4: - Resolving linting errors * Apply commit suggestions from code review from Amine Updating to include commit suggestions to test steps and comments from @Alami-Amine. These suggestions made the code cleaner and more readable, thank you! Co-authored-by: Amine Alami <[email protected]> * Updating TC_ACL_2_4 python3 test module to resolve commits from Amine, making the code cleaner and better: - Updating to change type of ID from "fabric-scoped node ID" with correct type "invalid Group Node ID" - Updating test step 4 description to be more detailed. * Restyled by autopep8 * Update src/python_testing/TC_ACL_2_4.py Changing hex values for CAT1-CAT4 in test step 21 as suggested by Amine who gathered these from the test spec Co-authored-by: Amine Alami <[email protected]> * Updating TC_ACL_2_4 python3 test module: - Replacing duplicate CAT4 value in test step 21 and replaced it with value from Amine that did not replicate CAT3's value * Updating TC_ACL_2_4 test module: - Updating to match test steps in test plan PR: https://github.com/CHIP-Specifications/chip-test-plans/pull/5052 * Pulling in files from upstream master to make sure that everything is in sync * Updating TC_ACL_2_4 python3 test module: - Removing try except blocks - Replacing self.print_step() with logging.info() * Restyled by autopep8 * Updated TC_ACL_2_4 python3 test module: - Added new test step method to step 29 - Updated test steps after test step 29 include the test step ordering - Updated test 29 and 30 to include the correct verbiage for the definitions and expected results * Updated TC_ACL_2_4 test module: - Actually updated TC_ACL_2_4 python3 test module to correct expected results verbiage for test steps 32-44 * Restyled by autopep8 * Update src/python_testing/TC_ACL_2_4.py Co-authored-by: Amine Alami <[email protected]> * Removing Test_TC_ACL_2_4.yaml script * Updating TC_ACL_2_4 python3 test module: - Replaced enumerate blocks with asserts.assert_in() checks to verify that structs are read correctly throughout the test - Removed some unneeded comments in the test module * Restyled by autopep8 * Updating TC_ACL_2_4 python3 test module: - Added in validation of admin entry only to end of test steps 32-44 - Removed duplicate validation of admin entry only in test step 31 --------- Co-authored-by: Alami-Amine <[email protected]> Co-authored-by: Restyled.io <[email protected]> Co-authored-by: Amine Alami <[email protected]>
dsavitsky-dsr
pushed a commit
to popovdg/connectedhomeip
that referenced
this pull request
Apr 29, 2025
* Modifying how ACL Attribute is written: making it use a non-empty initial ReplaceAll list * removing Test modifications on ACL Extensions * adding reference to issue in TODO comment * integrating comments * integrating comments * Revert "removing Test modifications on ACL Extensions" This reverts commit 055af80. * Clarifying AttributeDataIB Checkpoint * adding Valid CAT values * Integrating comments * Add comments clarifying AttributeDataIB rollback * Integrating comments again * change method order * Creating ACL_2_6 python3 test module: - Following test steps mentioned in test plan PR #[5061](https://github.com/CHIP-Specifications/chip-test-plans/pull/5061) from Amine - Using base branch of PR #[38263](project-chip#38263) * Restyled by autopep8 * Restyled by isort * Updating TC_ACL_2_6 python3 test module: - Resolving linting errors - Added additional checks for test step 3 and 5 to include struct validation * Updated TC_ACL_2_6 python test module: - Rebased branch with upstream master - Changed to checking for 2 new elements during test step 5 - Changed print_step() to logging.info() * Restyled by autopep8 * Updating to the upstream master branch version of WriteClient.cpp, WriteClient.h, Test_TC_ACL_2_5.yaml, and TestWriteChunking.cpp, to remove changes included in commit on this branch * Update src/python_testing/TC_ACL_2_6.py Co-authored-by: Amine Alami <[email protected]> * Removing Test_TC_ACL_2_6 yaml script * Updating TC_ACL_2_6 python3 test module: - Updating endpoint to 0 for REPL Linux CI checks to pass as AccessControl cluster events are on endpoint 0 * Updating TC_ACL_2_6 python3 test module: - Updating to using a simple for loop to validate the ReadEvent() response data - Removed unneeded asyncio.wait() from prior debugging - Created a short follow-up task PR to add subscription to this test module later * Update TC_ACL_2_6.py Resolving linting errros --------- Co-authored-by: Alami-Amine <[email protected]> Co-authored-by: Restyled.io <[email protected]> Co-authored-by: Amine Alami <[email protected]>
dsavitsky-dsr
pushed a commit
to popovdg/connectedhomeip
that referenced
this pull request
Apr 29, 2025
* Modifying how ACL Attribute is written: making it use a non-empty initial ReplaceAll list * removing Test modifications on ACL Extensions * adding reference to issue in TODO comment * integrating comments * integrating comments * Revert "removing Test modifications on ACL Extensions" This reverts commit 055af80. * Clarifying AttributeDataIB Checkpoint * adding Valid CAT values * Integrating comments * Add comments clarifying AttributeDataIB rollback * Integrating comments again * change method order * Creating ACL_2_3 python3 test module: - Added TC_ACL_2_3 python3 test module - This is a follow up PR to Amine's PR: project-chip#38263 - Removed Test_TC_ACL_2_3.yaml script * Actually removing Test_TC_ACL_2_3.yaml script * Resolving restylzer/autopep8 issue with script manually as provided CI patch failed * Resolving linting errors * Apply suggestions from Amine's code review Co-authored-by: Amine Alami <[email protected]> --------- Co-authored-by: Alami-Amine <[email protected]> Co-authored-by: Amine Alami <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
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.
Modifying how ACL (and Extensions) Attribute is written: making it use a non-empty initial ReplaceAll list.
Cause of Issue
ReplaceAll List Operation: Currently, an Empty List is always Encoded first, This triggers a clearing of all Entries on the server side.AppendItem List OperationFollowed by all the Items that are meant to be written; Those Items are Appended one by one on the Server's side.The Fix
WriteClientEncodes an ACL ListReplaceAlllist that used to clear all List Entries), and then following it up with Items sent seperately.ReplaceAllList, and will only chunk if needed.ReplaceAlllist are valid, before starting to Update or Delete any Entries.Scope of Fix
WriteClient::EncodeAttribute(): used by Linux-based WriteClientsWriteClient::PutPreencodedAttribute(): used by Python and Android WriteClientsTesting
AccessControlCluster.yaml