Validate for a valid cluster path when Invoke is called #37207
Merged
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
|
Review changes with Changed Files
|
|
PR #37207: Size comparison from d21aaa5 to 325ac8a Full report (3 builds for cc32xx, stm32)
|
|
PR #37207: Size comparison from d21aaa5 to 31e5ee7 Full report (20 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, nrfconnect, qpg, stm32, tizen)
|
tcarmelveilleux
approved these changes
Jan 27, 2025
arkq
approved these changes
Jan 27, 2025
tehampson
reviewed
Jan 27, 2025
tehampson
reviewed
Jan 27, 2025
src/data-model-providers/codegen/tests/TestCodegenModelViaMocks.cpp
Outdated
Show resolved
Hide resolved
|
PR #37207: Size comparison from d21aaa5 to 63fcea0 Full report (44 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, nrfconnect, psoc6, qpg, stm32, telink, tizen)
|
Co-authored-by: Boris Zbarsky <[email protected]>
Co-authored-by: Boris Zbarsky <[email protected]>
|
PR #37207: Size comparison from d21aaa5 to e94b6a7 Full report (71 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
tehampson
approved these changes
Jan 27, 2025
Co-authored-by: Boris Zbarsky <[email protected]>
|
PR #37207: Size comparison from d21aaa5 to a605a2c Full report (71 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
PR #37207: Size comparison from b082219 to 7052ac5 Full report (71 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
PR #37207: Size comparison from d7c1270 to 9fb74d2 Full report (14 builds for cc13x4_26x4, cc32xx, nrfconnect, qpg, stm32, tizen)
|
…and having unit tests seems reasonable
|
PR #37207: Size comparison from d7c1270 to 0961de7 Full report (71 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
Co-authored-by: Boris Zbarsky <[email protected]>
Co-authored-by: Boris Zbarsky <[email protected]>
Co-authored-by: Boris Zbarsky <[email protected]>
|
PR #37207: Size comparison from d7c1270 to 5119c75 Full report (71 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
… document. Not happy and hoping we can improve in the future
|
PR #37207: Size comparison from 435583e to 5abb6eb Full report (72 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
Comment on lines
+89
to
+92
| /// - `request.path` MUST be valid: Invoke` is only guaranteed to function correctly for | ||
| /// VALID paths (i.e. use `ProviderMetadataTree::AcceptedCommands` to check). This is | ||
| /// because we assume ACL or flags (like timed invoke) have to happen before invoking | ||
| /// this command. |
There was a problem hiding this comment.
This should be more like:
/// - `request.path` MUST refer to a command that actually exists. This is because in practice
/// callers must do ACL and flag checks (e.g. for timed invoke) before calling this function.
///
/// Callers that do not care about those checks should use `ProviderMetadataTree::AcceptedCommands`
/// to check for command existence.
| /// this command. | ||
| /// - TODO: as interfaces are updated, we may want to make the above requirement more | ||
| /// relaxed, as it seems desirable for users of this interface to have guaranteed | ||
| /// behavior (like error on invalid paths) where as today this seems unclear as some |
There was a problem hiding this comment.
"whereas" is one word.
| /// - TODO: as interfaces are updated, we may want to make the above requirement more | ||
| /// relaxed, as it seems desirable for users of this interface to have guaranteed | ||
| /// behavior (like error on invalid paths) where as today this seems unclear as some | ||
| /// command intercepts do not validate if the path is valid per endpoints. |
There was a problem hiding this comment.
"do not validate that the command is in fact accepted on the endpoint provided."
| @@ -324,6 +325,12 @@ const EmberAfCluster * CodegenDataModelProvider::FindServerCluster(const Concret | |||
| CHIP_ERROR CodegenDataModelProvider::AcceptedCommands(const ConcreteClusterPath & path, | |||
| DataModel::ListBuilder<DataModel::AcceptedCommandEntry> & builder) | |||
| { | |||
| // Some CommandHandlerInterface instances are registered of ALL endpoints, so make sure first that | |||
| // the cluster actually exists on this endpoint before asking the CommandHandlerInterface whether it | |||
| // supports the command. | |||
There was a problem hiding this comment.
What "the command"? This comment should be more like:
// Some CommandHandlerInterface instances are registered of ALL endpoints, so make sure first that
// the cluster actually exists on this endpoint before asking the CommandHandlerInterface what commands
// it claims to support.
Comment on lines
+412
to
+414
| // Some CommandHandlerInterface instances are registered of ALL endpoints, so make sure first that | ||
| // the cluster actually exists on this endpoint before asking the CommandHandlerInterface whether it | ||
| // supports the command. |
There was a problem hiding this comment.
Again, this comment does not make sense: what is "the command"? See wording suggestion above.
andy31415
added a commit
to andy31415/connectedhomeip
that referenced
this pull request
Feb 3, 2025
Post-merge review highlighted a few more updates. Performed them here.
yufengwangca
pushed a commit
that referenced
this pull request
Feb 4, 2025
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 fixes #37184
When we are performing an invoke, our logic allows for CommandHandlerInterface to do the first processing and if that fails, we fall back to ember. In the
SoftwareDiagnosticscase (and probably other) we register them as wildcard endpoints so we always find the CHI in that case.Existing code would do "find CHI and if found, let it process" and because SoftwareDiagnostics was available on any endpoint, we would just accept any endpoint. Added code to pre-validate we have a good endpoint/cluster combination before even attempting CHI.
Testing
Unit test and integration test added.