Separate DataModel APIs for encoding fabric-sensitive structs #15284
Conversation
This file contains 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
pullapprove
bot
requested review from
andy31415,
anush-apple,
balducci-apple,
Byungjoo-Lee,
bzbarsky-apple,
carol-apple,
cecille,
chrisdecenzo,
chshu,
chulspro,
Damian-Nordic,
dhrishi,
electrocucaracha,
erjiaqing,
franck-apple,
gjc13,
harimau-qirex,
hawk248,
holbrookt,
harsha-rajendran,
isiu-apple,
jelderton,
jepenven-silabs,
jmartinez-silabs,
kpschoedel,
lazarkov,
LuDuda and
mlepage-google
pullapprove
bot
requested review from
saurabhst,
selissia,
tcarmelveilleux,
tecimovic,
vijs,
vivien-apple,
wbschiller,
woody-apple,
xylophone21 and
yunhanw-google
This builds upon bzbarsky's original PR project-chip#15268 based upon discussions with him to create separate EncodeForWrite/EncodeForRead APIs that only apply to fabric-scoped structs. This is then used (for now) to omit encoding the fabric index on writes. Tests: Ran chip-tool to write an ACL entry and confirmed that that the fabric index field is omitted on writes.
mrjerryjohns
force-pushed
the
im/omit-fabric-idx-write
branch
from
6232da9 to
8585c34
Compare
bzbarsky-apple
approved these changes
Feb 16, 2022
|
PR #15284: Size comparison from 72896fc to 8585c34 Increases (16 builds for cyw30739, efr32, k32w, linux, p6, qpg, telink)
Full report (17 builds for cyw30739, efr32, k32w, linux, p6, qpg, telink)
|
|
PR #15284: Size comparison from 72896fc to d5a25c8 Increases (22 builds for cyw30739, efr32, esp32, k32w, linux, mbed, p6, qpg, telink)
Full report (24 builds for cyw30739, efr32, esp32, k32w, linux, mbed, p6, qpg, telink)
|
…ps the invalid fabric checker in AttributeValueEncoder::EncodeListItem...
|
PR #15284: Size comparison from 72896fc to 6bc1676 Increases (17 builds for efr32, esp32, mbed, nrfconnect)
Full report (20 builds for efr32, esp32, mbed, nrfconnect)
|
lazarkov
approved these changes
Feb 17, 2022
yunhanw-google
approved these changes
Feb 17, 2022
mrjerryjohns
added a commit
to mrjerryjohns/connectedhomeip
that referenced
this pull request
Feb 18, 2022
This builds upon the PR project-chip#15284 to omit fabric sensitive fields in the call to `EncodeForRead` if the fabric index present in the object doesn't match the accessing fabric. The test-cluster.xml has been updated to enhance the TestFabricScoped struct to include a number of different data types to ensure adequate coverage. Cluster definitions for OperationalCredentials and Acl clusters have been updated to reflect the spec correctly w.r.t fabric sensitive fields. Tests: Adds a YAML TestMultiFabric test that writes/reads fabric-sensitive data from multiple fabrics and validates the read back data. Adds a similar Python version of the test that is identical but is more robust towards the fabric indices and order of data read back from the server.
mrjerryjohns
added a commit
that referenced
this pull request
Mar 4, 2022
* Omit fabric sensitive data for Reads This builds upon the PR #15284 to omit fabric sensitive fields in the call to `EncodeForRead` if the fabric index present in the object doesn't match the accessing fabric. The test-cluster.xml has been updated to enhance the TestFabricScoped struct to include a number of different data types to ensure adequate coverage. Cluster definitions for OperationalCredentials and Acl clusters have been updated to reflect the spec correctly w.r.t fabric sensitive fields. Tests: Adds a YAML TestMultiFabric test that writes/reads fabric-sensitive data from multiple fabrics and validates the read back data. Adds a similar Python version of the test that is identical but is more robust towards the fabric indices and order of data read back from the server. * Re-run codegen * Forgot to add the MultiFabric YAML test * Re-run codegen and review fixes * Disabling the multi-fabric test for now since it's not quite working yet even on master * Rerun code gen * Re-gen * Minor indent fix * Disable more fabric-scoped tests in Python while we wait for #15688 to get resolved
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This builds upon @bzbarsky-apple 's original PR #15268 based upon discussions with him to create separate
DataModel::EncodeForWrite/DataModel::EncodeForReadAPIs that only apply to fabric-scoped structs. The latter requires passing an additionalaccessingFabricIndexargument as well that will be used in the future to omit fabric sensitive fields.This is then used (for now) to omit encoding the fabric index on writes. On the read side, the
AttributeReportBuilder::EncodeValuewill now correctly callEncodeForReadvsEncodefor fabric-scoped structs and non-scoped structs respectively.The
EncodeAPI is removed from fabric-scoped structs. This permits catching incorrect uses of that API without passing in an accessing fabric in the relevant contexts.This consciously created set of distinct methods for write vs. read permit an um-ambiguous way for callers to express the intention of the encoding and to correctly omit the fabric index on writes.
This will also be used later to correctly omit/include fabric-sensitive bits as well in a future PR.
Tests: Ran chip-tool to write an ACL entry and confirmed that that the fabric index field is omitted on writes.