From 262a1f175c5d0d4c1c8097d5065c27574eb53f87 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Mon, 27 Jan 2025 09:20:31 -0500 Subject: [PATCH 01/23] Fix invoke verification of data existence of cluster and endpoint --- .../codegen/CodegenDataModelProvider.cpp | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/data-model-providers/codegen/CodegenDataModelProvider.cpp b/src/data-model-providers/codegen/CodegenDataModelProvider.cpp index a3889fb1e25ebb..1f61dea03016eb 100644 --- a/src/data-model-providers/codegen/CodegenDataModelProvider.cpp +++ b/src/data-model-providers/codegen/CodegenDataModelProvider.cpp @@ -159,6 +159,17 @@ std::optional CodegenDataModelProvider::Invoke(co TLV::TLVReader & input_arguments, CommandHandler * handler) { + // Some commands are registered of ALL endpoints, so make sure first that + // the cluster actually exists on this endpoint before trying command handling + const EmberAfCluster * cluster = FindServerCluster(request.path); + if (cluster == nullptr) + { + // this is not a valid cluster. Return the right command. + std::optional endpoint_index = TryFindEndpointIndex(request.path.mEndpointId); + return endpoint_index.has_value() ? Protocols::InteractionModel::Status::UnsupportedCluster + : Protocols::InteractionModel::Status::UnsupportedEndpoint; + } + CommandHandlerInterface * handler_interface = CommandHandlerInterfaceRegistry::Instance().GetCommandHandler(request.path.mEndpointId, request.path.mClusterId); From 0f28292099ec4a58dc2dcdade08eb7e54cc3bb69 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Mon, 27 Jan 2025 10:00:48 -0500 Subject: [PATCH 02/23] Add unit test for codegen logic issue --- .../tests/TestCodegenModelViaMocks.cpp | 89 ++++++++++++++++++- 1 file changed, 88 insertions(+), 1 deletion(-) diff --git a/src/data-model-providers/codegen/tests/TestCodegenModelViaMocks.cpp b/src/data-model-providers/codegen/tests/TestCodegenModelViaMocks.cpp index 1045114b7f9b4c..dfad4455a39162 100644 --- a/src/data-model-providers/codegen/tests/TestCodegenModelViaMocks.cpp +++ b/src/data-model-providers/codegen/tests/TestCodegenModelViaMocks.cpp @@ -218,6 +218,46 @@ class MockAccessControl : public Access::AccessControl::Delegate, public Access: bool IsDeviceTypeOnEndpoint(DeviceTypeId deviceType, EndpointId endpoint) override { return true; } }; +class MockCommandHandler : public CommandHandler +{ +public: + CHIP_ERROR FallibleAddStatus(const ConcreteCommandPath & aRequestCommandPath, + const Protocols::InteractionModel::ClusterStatusCode & aStatus, + const char * context = nullptr) override + { + // MOCK: do not do anything here + return CHIP_NO_ERROR; + } + + void AddStatus(const ConcreteCommandPath & aRequestCommandPath, const Protocols::InteractionModel::ClusterStatusCode & aStatus, + const char * context = nullptr) override + { + // MOCK: do not do anything here + } + + FabricIndex GetAccessingFabricIndex() const override { return 1; } + + CHIP_ERROR AddResponseData(const ConcreteCommandPath & aRequestCommandPath, CommandId aResponseCommandId, + const DataModel::EncodableToTLV & aEncodable) override + { + return CHIP_NO_ERROR; + } + + void AddResponse(const ConcreteCommandPath & aRequestCommandPath, CommandId aResponseCommandId, + const DataModel::EncodableToTLV & aEncodable) override + {} + + bool IsTimedInvoke() const override { return false; } + + void FlushAcksRightAwayOnSlowCommand() override {} + + Access::SubjectDescriptor GetSubjectDescriptor() const override { return kAdminSubjectDescriptor; } + + Messaging::ExchangeContext * GetExchangeContext() const override { + return nullptr; + } +}; + /// Overrides Enumerate*Commands in the CommandHandlerInterface to allow /// testing of behaviors when command enumeration is done in the interace. class CustomListCommandHandler : public CommandHandlerInterface @@ -229,7 +269,20 @@ class CustomListCommandHandler : public CommandHandlerInterface } ~CustomListCommandHandler() { CommandHandlerInterfaceRegistry::Instance().UnregisterCommandHandler(this); } - void InvokeCommand(HandlerContext & handlerContext) override { handlerContext.SetCommandNotHandled(); } + void InvokeCommand(HandlerContext & handlerContext) override + { + if (mHandleCommand) + { + handlerContext.mCommandHandler.AddStatus(handlerContext.mRequestPath, Protocols::InteractionModel::Status::Success); + handlerContext.SetCommandHandled(); + } + else + { + handlerContext.SetCommandNotHandled(); + } + } + + void SetHandleCommands(bool handle) { mHandleCommand = handle; } CHIP_ERROR EnumerateAcceptedCommands(const ConcreteClusterPath & cluster, CommandIdCallback callback, void * context) override { @@ -268,6 +321,7 @@ class CustomListCommandHandler : public CommandHandlerInterface private: bool mOverrideAccepted = false; bool mOverrideGenerated = false; + bool mHandleCommand = false; std::vector mAccepted; std::vector mGenerated; @@ -1156,6 +1210,39 @@ TEST_F(TestCodegenModelViaMocks, IterateOverGeneratedCommands) ASSERT_TRUE(cmds.data_equal(Span(expectedCommands3))); } +TEST_F(TestCodegenModelViaMocks, CommandHandlerInterfaceValidity) +{ + UseMockNodeConfig config(gTestNodeConfig); + CodegenDataModelProviderWithContext model; + + // register a CHI on ALL endpoints + CustomListCommandHandler handler(chip::NullOptional, MockClusterId(1)); + handler.SetHandleCommands(true); + + MockCommandHandler commandHandler; + + // Command succeeds on a valid endpoint + { + const ConcreteCommandPath kCommandPath(kMockEndpoint1, MockClusterId(1), kMockCommandId1); + const InvokeRequest kInvokeRequest{ .path = kCommandPath }; + chip::TLV::TLVReader tlvReader; + + // std::nullopt is returned when the command is handled + ASSERT_EQ(model.Invoke(kInvokeRequest, tlvReader, /* handler = */ &commandHandler), std::nullopt); + } + + // Command fails on an invalid endpoint + { + const ConcreteCommandPath kCommandPath(kEndpointIdThatIsMissing, MockClusterId(1), kMockCommandId1); + const InvokeRequest kInvokeRequest{ .path = kCommandPath }; + chip::TLV::TLVReader tlvReader; + + // std::nullopt is returned when the command is handled + ASSERT_EQ(model.Invoke(kInvokeRequest, tlvReader, /* handler = */ &commandHandler), + Protocols::InteractionModel::Status::UnsupportedEndpoint); + } +} + TEST_F(TestCodegenModelViaMocks, CommandHandlerInterfaceCommandHandling) { From 796fd146c6ea17a25fed75c674b2686983b8cb00 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Mon, 27 Jan 2025 10:01:55 -0500 Subject: [PATCH 03/23] Add integration test for return codes --- src/python_testing/TestInvokeReturnCodes.py | 89 +++++++++++++++++++++ 1 file changed, 89 insertions(+) create mode 100644 src/python_testing/TestInvokeReturnCodes.py diff --git a/src/python_testing/TestInvokeReturnCodes.py b/src/python_testing/TestInvokeReturnCodes.py new file mode 100644 index 00000000000000..cc59035c8834c4 --- /dev/null +++ b/src/python_testing/TestInvokeReturnCodes.py @@ -0,0 +1,89 @@ +# +# Copyright (c) 2025 Project CHIP Authors +# All rights reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# + +# === BEGIN CI TEST ARGUMENTS === +# test-runner-runs: +# run1: +# app: ${ALL_CLUSTERS_APP} +# app-args: > +# --discriminator 1234 +# --KVS kvs1 +# --trace-to json:${TRACE_APP}.json +# script-args: > +# --storage-path admin_storage.json +# --commissioning-method on-network +# --discriminator 1234 +# --passcode 20202021 +# --trace-to json:${TRACE_TEST_JSON}.json +# --trace-to perfetto:${TRACE_TEST_PERFETTO}.perfetto +# factory-reset: true +# quiet: true +# === END CI TEST ARGUMENTS === + +import logging + +import chip.clusters as Clusters +from chip.testing.matter_testing import ( + MatterBaseTest, + async_test_body, + default_matter_test_main, +) +from chip.interaction_model import InteractionModelError, Status +from mobly import asserts + + +class TestInvokeReturnCodes(MatterBaseTest): + """ + Validates that the invoke action correctly refuses commands + on invalid endpoints. + """ + + @async_test_body + async def test_invalid_endpoint_command(self): + self.print_step(0, "Commissioning - already done") + + self.print_step(1, "Find an invalid endpoint id") + root_parts = await self.read_single_attribute_check_success( + cluster=Clusters.Descriptor, + attribute=Clusters.Descriptor.Attributes.PartsList, + endpoint=0, + ) + endpoints = set(root_parts) + invalid_endpoint_id = 1 + while invalid_endpoint_id in endpoints: + invalid_endpoint_id += 1 + + self.print_step( + 2, + "Attempt to invoke SoftwareDiagnostics::ResetWatermarks on an invalid endpoint", + ) + try: + await self.send_single_cmd( + cmd=Clusters.SoftwareDiagnostics.Commands.ResetWatermarks(), + endpoint=invalid_endpoint_id, + ) + asserts.assert_true( + False, "Unexpected command success on an invalid endpoint" + ) + except InteractionModelError as e: + asserts.assert_equal( + e.status, Status.UnsupportedEndpoint, "Unexpected error returned" + ) + + +if __name__ == "__main__": + default_matter_test_main() From 6979ebdf0d9512232ab117465131cb076019b3fb Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Mon, 27 Jan 2025 10:02:13 -0500 Subject: [PATCH 04/23] Restule --- .../codegen/tests/TestCodegenModelViaMocks.cpp | 4 +--- src/python_testing/TestInvokeReturnCodes.py | 6 +----- 2 files changed, 2 insertions(+), 8 deletions(-) diff --git a/src/data-model-providers/codegen/tests/TestCodegenModelViaMocks.cpp b/src/data-model-providers/codegen/tests/TestCodegenModelViaMocks.cpp index dfad4455a39162..f11f8c9358a625 100644 --- a/src/data-model-providers/codegen/tests/TestCodegenModelViaMocks.cpp +++ b/src/data-model-providers/codegen/tests/TestCodegenModelViaMocks.cpp @@ -253,9 +253,7 @@ class MockCommandHandler : public CommandHandler Access::SubjectDescriptor GetSubjectDescriptor() const override { return kAdminSubjectDescriptor; } - Messaging::ExchangeContext * GetExchangeContext() const override { - return nullptr; - } + Messaging::ExchangeContext * GetExchangeContext() const override { return nullptr; } }; /// Overrides Enumerate*Commands in the CommandHandlerInterface to allow diff --git a/src/python_testing/TestInvokeReturnCodes.py b/src/python_testing/TestInvokeReturnCodes.py index cc59035c8834c4..c75a2917760cff 100644 --- a/src/python_testing/TestInvokeReturnCodes.py +++ b/src/python_testing/TestInvokeReturnCodes.py @@ -37,12 +37,8 @@ import logging import chip.clusters as Clusters -from chip.testing.matter_testing import ( - MatterBaseTest, - async_test_body, - default_matter_test_main, -) from chip.interaction_model import InteractionModelError, Status +from chip.testing.matter_testing import MatterBaseTest, async_test_body, default_matter_test_main from mobly import asserts From 325ac8a1932148690b250e2742a02c0ccbeb9a4a Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Mon, 27 Jan 2025 10:06:31 -0500 Subject: [PATCH 05/23] Remove odd comment --- .../codegen/tests/TestCodegenModelViaMocks.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/src/data-model-providers/codegen/tests/TestCodegenModelViaMocks.cpp b/src/data-model-providers/codegen/tests/TestCodegenModelViaMocks.cpp index f11f8c9358a625..1a462a4dd9eb75 100644 --- a/src/data-model-providers/codegen/tests/TestCodegenModelViaMocks.cpp +++ b/src/data-model-providers/codegen/tests/TestCodegenModelViaMocks.cpp @@ -1235,7 +1235,6 @@ TEST_F(TestCodegenModelViaMocks, CommandHandlerInterfaceValidity) const InvokeRequest kInvokeRequest{ .path = kCommandPath }; chip::TLV::TLVReader tlvReader; - // std::nullopt is returned when the command is handled ASSERT_EQ(model.Invoke(kInvokeRequest, tlvReader, /* handler = */ &commandHandler), Protocols::InteractionModel::Status::UnsupportedEndpoint); } From 31e5ee79728569925f527b55a6f6221df6de653e Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Mon, 27 Jan 2025 10:41:11 -0500 Subject: [PATCH 06/23] Remove unused import --- src/python_testing/TestInvokeReturnCodes.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/python_testing/TestInvokeReturnCodes.py b/src/python_testing/TestInvokeReturnCodes.py index c75a2917760cff..85efc4d0236950 100644 --- a/src/python_testing/TestInvokeReturnCodes.py +++ b/src/python_testing/TestInvokeReturnCodes.py @@ -34,8 +34,6 @@ # quiet: true # === END CI TEST ARGUMENTS === -import logging - import chip.clusters as Clusters from chip.interaction_model import InteractionModelError, Status from chip.testing.matter_testing import MatterBaseTest, async_test_body, default_matter_test_main From f8cff5e1c33296e83d263223ba07fcf3f558edcd Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Mon, 27 Jan 2025 11:20:33 -0500 Subject: [PATCH 07/23] Use asserts fail --- src/python_testing/TestInvokeReturnCodes.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/python_testing/TestInvokeReturnCodes.py b/src/python_testing/TestInvokeReturnCodes.py index 85efc4d0236950..cd01f8882338c4 100644 --- a/src/python_testing/TestInvokeReturnCodes.py +++ b/src/python_testing/TestInvokeReturnCodes.py @@ -70,9 +70,7 @@ async def test_invalid_endpoint_command(self): cmd=Clusters.SoftwareDiagnostics.Commands.ResetWatermarks(), endpoint=invalid_endpoint_id, ) - asserts.assert_true( - False, "Unexpected command success on an invalid endpoint" - ) + asserts.fail("Unexpected command success on an invalid endpoint") except InteractionModelError as e: asserts.assert_equal( e.status, Status.UnsupportedEndpoint, "Unexpected error returned" From 4be2502789acff1f16f59249ed1a103ccc016c32 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Mon, 27 Jan 2025 11:22:15 -0500 Subject: [PATCH 08/23] Add unit test that checks invalid cluster return code as well --- .../codegen/tests/TestCodegenModelViaMocks.cpp | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/data-model-providers/codegen/tests/TestCodegenModelViaMocks.cpp b/src/data-model-providers/codegen/tests/TestCodegenModelViaMocks.cpp index 1a462a4dd9eb75..9694cf0241f86a 100644 --- a/src/data-model-providers/codegen/tests/TestCodegenModelViaMocks.cpp +++ b/src/data-model-providers/codegen/tests/TestCodegenModelViaMocks.cpp @@ -1238,6 +1238,16 @@ TEST_F(TestCodegenModelViaMocks, CommandHandlerInterfaceValidity) ASSERT_EQ(model.Invoke(kInvokeRequest, tlvReader, /* handler = */ &commandHandler), Protocols::InteractionModel::Status::UnsupportedEndpoint); } + + // Command fails on an invalid cluster + { + const ConcreteCommandPath kCommandPath(kMockEndpoint1, MockClusterId(0x1123), kMockCommandId1); + const InvokeRequest kInvokeRequest{ .path = kCommandPath }; + chip::TLV::TLVReader tlvReader; + + ASSERT_EQ(model.Invoke(kInvokeRequest, tlvReader, /* handler = */ &commandHandler), + Protocols::InteractionModel::Status::UnsupportedCluster); + } } TEST_F(TestCodegenModelViaMocks, CommandHandlerInterfaceCommandHandling) From 63fcea0c35ad84ff714f00d8339f83bb3b08b422 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Mon, 27 Jan 2025 11:23:13 -0500 Subject: [PATCH 09/23] Fix comment --- src/data-model-providers/codegen/CodegenDataModelProvider.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/data-model-providers/codegen/CodegenDataModelProvider.cpp b/src/data-model-providers/codegen/CodegenDataModelProvider.cpp index 1f61dea03016eb..dfa18081328865 100644 --- a/src/data-model-providers/codegen/CodegenDataModelProvider.cpp +++ b/src/data-model-providers/codegen/CodegenDataModelProvider.cpp @@ -164,7 +164,7 @@ std::optional CodegenDataModelProvider::Invoke(co const EmberAfCluster * cluster = FindServerCluster(request.path); if (cluster == nullptr) { - // this is not a valid cluster. Return the right command. + // This is not a valid cluster. Return the right error code depending on what is valid. std::optional endpoint_index = TryFindEndpointIndex(request.path.mEndpointId); return endpoint_index.has_value() ? Protocols::InteractionModel::Status::UnsupportedCluster : Protocols::InteractionModel::Status::UnsupportedEndpoint; From b7406d3cf27d226c0bb89da97bfe4b5282b6c5fc Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Mon, 27 Jan 2025 12:16:59 -0500 Subject: [PATCH 10/23] Code review feedback: listing of commands should fail if the cluster does not exist --- .../codegen/CodegenDataModelProvider.cpp | 14 ++++++--- .../tests/TestCodegenModelViaMocks.cpp | 31 +++++++++++++++++++ 2 files changed, 41 insertions(+), 4 deletions(-) diff --git a/src/data-model-providers/codegen/CodegenDataModelProvider.cpp b/src/data-model-providers/codegen/CodegenDataModelProvider.cpp index dfa18081328865..efb158fd44e6d8 100644 --- a/src/data-model-providers/codegen/CodegenDataModelProvider.cpp +++ b/src/data-model-providers/codegen/CodegenDataModelProvider.cpp @@ -335,6 +335,11 @@ const EmberAfCluster * CodegenDataModelProvider::FindServerCluster(const Concret CHIP_ERROR CodegenDataModelProvider::AcceptedCommands(const ConcreteClusterPath & path, DataModel::ListBuilder & builder) { + // Some commands are registered of ALL endpoints, so make sure first that + // the cluster actually exists on this endpoint before trying command handling + const EmberAfCluster * serverCluster = FindServerCluster(path); + VerifyOrReturnError(serverCluster != nullptr, CHIP_ERROR_NOT_FOUND); + CommandHandlerInterface * interface = CommandHandlerInterfaceRegistry::Instance().GetCommandHandler(path.mEndpointId, path.mClusterId); if (interface != nullptr) @@ -388,8 +393,6 @@ CHIP_ERROR CodegenDataModelProvider::AcceptedCommands(const ConcreteClusterPath VerifyOrReturnError(err == CHIP_ERROR_NOT_IMPLEMENTED, err); } - const EmberAfCluster * serverCluster = FindServerCluster(path); - VerifyOrReturnError(serverCluster != nullptr, CHIP_ERROR_NOT_FOUND); VerifyOrReturnError(serverCluster->acceptedCommandList != nullptr, CHIP_NO_ERROR); const chip::CommandId * endOfList = serverCluster->acceptedCommandList; @@ -415,6 +418,11 @@ CHIP_ERROR CodegenDataModelProvider::AcceptedCommands(const ConcreteClusterPath CHIP_ERROR CodegenDataModelProvider::GeneratedCommands(const ConcreteClusterPath & path, DataModel::ListBuilder & builder) { + // Some commands are registered of ALL endpoints, so make sure first that + // the cluster actually exists on this endpoint before trying command handling + const EmberAfCluster * serverCluster = FindServerCluster(path); + VerifyOrReturnError(serverCluster != nullptr, CHIP_ERROR_NOT_FOUND); + CommandHandlerInterface * interface = CommandHandlerInterfaceRegistry::Instance().GetCommandHandler(path.mEndpointId, path.mClusterId); if (interface != nullptr) @@ -465,8 +473,6 @@ CHIP_ERROR CodegenDataModelProvider::GeneratedCommands(const ConcreteClusterPath VerifyOrReturnError(err == CHIP_ERROR_NOT_IMPLEMENTED, err); } - const EmberAfCluster * serverCluster = FindServerCluster(path); - VerifyOrReturnError(serverCluster != nullptr, CHIP_ERROR_NOT_FOUND); VerifyOrReturnError(serverCluster->generatedCommandList != nullptr, CHIP_NO_ERROR); const chip::CommandId * endOfList = serverCluster->generatedCommandList; diff --git a/src/data-model-providers/codegen/tests/TestCodegenModelViaMocks.cpp b/src/data-model-providers/codegen/tests/TestCodegenModelViaMocks.cpp index 9694cf0241f86a..239926b4d93770 100644 --- a/src/data-model-providers/codegen/tests/TestCodegenModelViaMocks.cpp +++ b/src/data-model-providers/codegen/tests/TestCodegenModelViaMocks.cpp @@ -1250,6 +1250,37 @@ TEST_F(TestCodegenModelViaMocks, CommandHandlerInterfaceValidity) } } +TEST_F(TestCodegenModelViaMocks, AcceptedGeneratedCommandsOnInvalidEndpoints) +{ + UseMockNodeConfig config(gTestNodeConfig); + CodegenDataModelProviderWithContext model; + + // register a CHI on ALL endpoints + CustomListCommandHandler handler(chip::NullOptional, MockClusterId(1)); + handler.SetHandleCommands(true); + + DataModel::ListBuilder generatedBuilder; + DataModel::ListBuilder acceptedBuilder; + + // valid endpoint will result in valid data (even though list is empty) + ASSERT_EQ(model.GeneratedCommands(ConcreteClusterPath(kMockEndpoint1, MockClusterId(1)), generatedBuilder), CHIP_NO_ERROR); + ASSERT_TRUE(generatedBuilder.IsEmpty()); + ASSERT_EQ(model.AcceptedCommands(ConcreteClusterPath(kMockEndpoint1, MockClusterId(1)), acceptedBuilder), CHIP_NO_ERROR); + ASSERT_TRUE(acceptedBuilder.IsEmpty()); + + // Invalid endpoint fails - we will get no commands there (even though CHI is registered) + ASSERT_EQ(model.GeneratedCommands(ConcreteClusterPath(kEndpointIdThatIsMissing, MockClusterId(1)), generatedBuilder), + CHIP_ERROR_NOT_FOUND); + ASSERT_EQ(model.AcceptedCommands(ConcreteClusterPath(kEndpointIdThatIsMissing, MockClusterId(1)), acceptedBuilder), + CHIP_ERROR_NOT_FOUND); + + // same for invalid cluster ID + ASSERT_EQ(model.GeneratedCommands(ConcreteClusterPath(kMockEndpoint1, MockClusterId(0x1123)), generatedBuilder), + CHIP_ERROR_NOT_FOUND); + ASSERT_EQ(model.AcceptedCommands(ConcreteClusterPath(kMockEndpoint1, MockClusterId(0x1123)), acceptedBuilder), + CHIP_ERROR_NOT_FOUND); +} + TEST_F(TestCodegenModelViaMocks, CommandHandlerInterfaceCommandHandling) { From 583c6a891b0445c0ab2f63bd8f33432d495463c1 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Mon, 27 Jan 2025 12:29:26 -0500 Subject: [PATCH 11/23] Update src/data-model-providers/codegen/CodegenDataModelProvider.cpp Co-authored-by: Boris Zbarsky --- .../codegen/CodegenDataModelProvider.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/data-model-providers/codegen/CodegenDataModelProvider.cpp b/src/data-model-providers/codegen/CodegenDataModelProvider.cpp index efb158fd44e6d8..f3b17ab784f2ec 100644 --- a/src/data-model-providers/codegen/CodegenDataModelProvider.cpp +++ b/src/data-model-providers/codegen/CodegenDataModelProvider.cpp @@ -335,8 +335,9 @@ const EmberAfCluster * CodegenDataModelProvider::FindServerCluster(const Concret CHIP_ERROR CodegenDataModelProvider::AcceptedCommands(const ConcreteClusterPath & path, DataModel::ListBuilder & builder) { - // Some commands are registered of ALL endpoints, so make sure first that - // the cluster actually exists on this endpoint before trying command handling + // 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. const EmberAfCluster * serverCluster = FindServerCluster(path); VerifyOrReturnError(serverCluster != nullptr, CHIP_ERROR_NOT_FOUND); From e94b6a7e49c372426b1ebd0684888ab37ad7c8a8 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Mon, 27 Jan 2025 12:30:55 -0500 Subject: [PATCH 12/23] Update src/data-model-providers/codegen/CodegenDataModelProvider.cpp Co-authored-by: Boris Zbarsky --- .../codegen/CodegenDataModelProvider.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/data-model-providers/codegen/CodegenDataModelProvider.cpp b/src/data-model-providers/codegen/CodegenDataModelProvider.cpp index f3b17ab784f2ec..e954f60df8acf1 100644 --- a/src/data-model-providers/codegen/CodegenDataModelProvider.cpp +++ b/src/data-model-providers/codegen/CodegenDataModelProvider.cpp @@ -419,8 +419,9 @@ CHIP_ERROR CodegenDataModelProvider::AcceptedCommands(const ConcreteClusterPath CHIP_ERROR CodegenDataModelProvider::GeneratedCommands(const ConcreteClusterPath & path, DataModel::ListBuilder & builder) { - // Some commands are registered of ALL endpoints, so make sure first that - // the cluster actually exists on this endpoint before trying command handling + // 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. const EmberAfCluster * serverCluster = FindServerCluster(path); VerifyOrReturnError(serverCluster != nullptr, CHIP_ERROR_NOT_FOUND); From a605a2cb578b0de82d30071ba4ec767e2cbbf0bd Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Mon, 27 Jan 2025 15:06:13 -0500 Subject: [PATCH 13/23] Update src/data-model-providers/codegen/CodegenDataModelProvider.cpp Co-authored-by: Boris Zbarsky --- src/data-model-providers/codegen/CodegenDataModelProvider.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/data-model-providers/codegen/CodegenDataModelProvider.cpp b/src/data-model-providers/codegen/CodegenDataModelProvider.cpp index e954f60df8acf1..86888b952c4ba7 100644 --- a/src/data-model-providers/codegen/CodegenDataModelProvider.cpp +++ b/src/data-model-providers/codegen/CodegenDataModelProvider.cpp @@ -159,8 +159,7 @@ std::optional CodegenDataModelProvider::Invoke(co TLV::TLVReader & input_arguments, CommandHandler * handler) { - // Some commands are registered of ALL endpoints, so make sure first that - // the cluster actually exists on this endpoint before trying command handling + // Ensure the command actually exists on the relevant cluster instance. const EmberAfCluster * cluster = FindServerCluster(request.path); if (cluster == nullptr) { From 7052ac5cc0ce961a105339667f591bafb8f6e739 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Tue, 28 Jan 2025 08:23:41 -0500 Subject: [PATCH 14/23] Updated comment --- src/data-model-providers/codegen/CodegenDataModelProvider.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/data-model-providers/codegen/CodegenDataModelProvider.cpp b/src/data-model-providers/codegen/CodegenDataModelProvider.cpp index 86888b952c4ba7..fb6eecaff6601f 100644 --- a/src/data-model-providers/codegen/CodegenDataModelProvider.cpp +++ b/src/data-model-providers/codegen/CodegenDataModelProvider.cpp @@ -159,7 +159,9 @@ std::optional CodegenDataModelProvider::Invoke(co TLV::TLVReader & input_arguments, CommandHandler * handler) { - // Ensure the command actually exists on the relevant cluster instance. + // Double-check that the command path is valid at least up to the cluster level: + // some CommandHandlerInterface are registered on all endpoints, so they would be found by + // `GetCommandHandler` below, however we need to ensure the path makes sense. const EmberAfCluster * cluster = FindServerCluster(request.path); if (cluster == nullptr) { From d837c7f53f8e73285749e37b32f3a27b4a71037b Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Thu, 30 Jan 2025 09:17:49 -0500 Subject: [PATCH 15/23] Try to reduce amount of code overhead for extra validation check --- src/app/data-model-provider/Provider.h | 5 ++++ .../codegen/CodegenDataModelProvider.cpp | 30 ++++++++----------- 2 files changed, 18 insertions(+), 17 deletions(-) diff --git a/src/app/data-model-provider/Provider.h b/src/app/data-model-provider/Provider.h index 13d5a4a4f55e7c..aeeffe442967c7 100644 --- a/src/app/data-model-provider/Provider.h +++ b/src/app/data-model-provider/Provider.h @@ -85,6 +85,11 @@ class Provider : public ProviderMetadataTree /// This includes cases where command handling and value return will be done asynchronously. /// - returning a value other than Success implies an error reply (error and data are mutually exclusive) /// + /// Requirements: + /// - command MUST handle any requests and validate them. In particular, in case of invalid + /// paths, a return code of UnsupportedEndpoint/UnsupportedCluster/UnsupporedCommand is + /// expected. + /// /// Return value expectations: /// - if a response has been placed into `handler` then std::nullopt MUST be returned. In particular /// note that CHIP_NO_ERROR is NOT the same as std::nullopt: diff --git a/src/data-model-providers/codegen/CodegenDataModelProvider.cpp b/src/data-model-providers/codegen/CodegenDataModelProvider.cpp index fb6eecaff6601f..e0e0b7a787a40f 100644 --- a/src/data-model-providers/codegen/CodegenDataModelProvider.cpp +++ b/src/data-model-providers/codegen/CodegenDataModelProvider.cpp @@ -163,26 +163,22 @@ std::optional CodegenDataModelProvider::Invoke(co // some CommandHandlerInterface are registered on all endpoints, so they would be found by // `GetCommandHandler` below, however we need to ensure the path makes sense. const EmberAfCluster * cluster = FindServerCluster(request.path); - if (cluster == nullptr) - { - // This is not a valid cluster. Return the right error code depending on what is valid. - std::optional endpoint_index = TryFindEndpointIndex(request.path.mEndpointId); - return endpoint_index.has_value() ? Protocols::InteractionModel::Status::UnsupportedCluster - : Protocols::InteractionModel::Status::UnsupportedEndpoint; - } - - CommandHandlerInterface * handler_interface = - CommandHandlerInterfaceRegistry::Instance().GetCommandHandler(request.path.mEndpointId, request.path.mClusterId); - - if (handler_interface) + if (cluster != nullptr) { - CommandHandlerInterface::HandlerContext context(*handler, request.path, input_arguments); - handler_interface->InvokeCommand(context); + // We can allow CHI, including ones registered on wildcard endpoints. + CommandHandlerInterface * handler_interface = + CommandHandlerInterfaceRegistry::Instance().GetCommandHandler(request.path.mEndpointId, request.path.mClusterId); - // If the command was handled, don't proceed any further and return successfully. - if (context.mCommandHandled) + if (handler_interface) { - return std::nullopt; + CommandHandlerInterface::HandlerContext context(*handler, request.path, input_arguments); + handler_interface->InvokeCommand(context); + + // If the command was handled, don't proceed any further and return successfully. + if (context.mCommandHandled) + { + return std::nullopt; + } } } From 8b0ef0327a84de22a0999e0deba8ddf8c40e46ef Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Thu, 30 Jan 2025 09:47:15 -0500 Subject: [PATCH 16/23] Update logic yet again since just checking for null fails unit tests and having unit tests seems reasonable --- .../codegen/CodegenDataModelProvider.cpp | 30 ++++++++++--------- .../codegen/EmberMetadata.cpp | 27 ++++++++++++----- .../codegen/EmberMetadata.h | 9 ++++++ 3 files changed, 44 insertions(+), 22 deletions(-) diff --git a/src/data-model-providers/codegen/CodegenDataModelProvider.cpp b/src/data-model-providers/codegen/CodegenDataModelProvider.cpp index e0e0b7a787a40f..6fd146f6573ce7 100644 --- a/src/data-model-providers/codegen/CodegenDataModelProvider.cpp +++ b/src/data-model-providers/codegen/CodegenDataModelProvider.cpp @@ -14,6 +14,7 @@ * See the License for the specific language governing permissions and * limitations under the License. */ +#include "data-model-providers/codegen/EmberMetadata.h" #include #include @@ -162,23 +163,24 @@ std::optional CodegenDataModelProvider::Invoke(co // Double-check that the command path is valid at least up to the cluster level: // some CommandHandlerInterface are registered on all endpoints, so they would be found by // `GetCommandHandler` below, however we need to ensure the path makes sense. - const EmberAfCluster * cluster = FindServerCluster(request.path); - if (cluster != nullptr) + Protocols::InteractionModel::Status status = Ember::ValidateClusterPath(request.path); + if (status != Protocols::InteractionModel::Status::Success) { - // We can allow CHI, including ones registered on wildcard endpoints. - CommandHandlerInterface * handler_interface = - CommandHandlerInterfaceRegistry::Instance().GetCommandHandler(request.path.mEndpointId, request.path.mClusterId); + return status; + } - if (handler_interface) - { - CommandHandlerInterface::HandlerContext context(*handler, request.path, input_arguments); - handler_interface->InvokeCommand(context); + CommandHandlerInterface * handler_interface = + CommandHandlerInterfaceRegistry::Instance().GetCommandHandler(request.path.mEndpointId, request.path.mClusterId); - // If the command was handled, don't proceed any further and return successfully. - if (context.mCommandHandled) - { - return std::nullopt; - } + if (handler_interface) + { + CommandHandlerInterface::HandlerContext context(*handler, request.path, input_arguments); + handler_interface->InvokeCommand(context); + + // If the command was handled, don't proceed any further and return successfully. + if (context.mCommandHandled) + { + return std::nullopt; } } diff --git a/src/data-model-providers/codegen/EmberMetadata.cpp b/src/data-model-providers/codegen/EmberMetadata.cpp index 22e31a569fe702..63808b2b126ffb 100644 --- a/src/data-model-providers/codegen/EmberMetadata.cpp +++ b/src/data-model-providers/codegen/EmberMetadata.cpp @@ -56,24 +56,35 @@ FindAttributeMetadata(const ConcreteAttributePath & aPath) if (metadata == nullptr) { - const EmberAfEndpointType * type = emberAfFindEndpointType(aPath.mEndpointId); + Status status = ValidateClusterPath(aPath); + + if (status != Status::Success) { + return status; + } + + // Since we know the attribute is unsupported and the endpoint/cluster are + // OK, this is the only option left. + return Status::UnsupportedAttribute; + } + + return metadata; +} + +Status ValidateClusterPath(const ConcreteClusterPath &path) { + + const EmberAfEndpointType * type = emberAfFindEndpointType(path.mEndpointId); if (type == nullptr) { return Status::UnsupportedEndpoint; } - const EmberAfCluster * cluster = emberAfFindClusterInType(type, aPath.mClusterId, CLUSTER_MASK_SERVER); + const EmberAfCluster * cluster = emberAfFindClusterInType(type, path.mClusterId, CLUSTER_MASK_SERVER); if (cluster == nullptr) { return Status::UnsupportedCluster; } - // Since we know the attribute is unsupported and the endpoint/cluster are - // OK, this is the only option left. - return Status::UnsupportedAttribute; - } - - return metadata; + return Status::Success; } } // namespace Ember diff --git a/src/data-model-providers/codegen/EmberMetadata.h b/src/data-model-providers/codegen/EmberMetadata.h index 64c0bfe736a25f..0f37a9e4b65b2e 100644 --- a/src/data-model-providers/codegen/EmberMetadata.h +++ b/src/data-model-providers/codegen/EmberMetadata.h @@ -16,6 +16,7 @@ */ #pragma once +#include "app/ConcreteClusterPath.h" #include #include #include @@ -42,6 +43,14 @@ std::variant FindAttributeMetadata(const ConcreteAttributePath & aPath); +/// Returns the status for a given path being valud within ember. +/// +/// Return code will be one of: +/// - Status::UnsupportedEndpoint if the path endpoint does not exist +/// - Status::UnsupportedCluster if the cluster does not exist on the given endpoint +/// - Status::Success if the path is valid within Ember +Protocols::InteractionModel::Status ValidateClusterPath(const ConcreteClusterPath &path); + } // namespace Ember } // namespace app } // namespace chip From 0961de784d6c5b4664b6fb009812cd8c566b0cd6 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Thu, 30 Jan 2025 09:52:41 -0500 Subject: [PATCH 17/23] Restyle --- .../codegen/EmberMetadata.cpp | 28 ++++++++++--------- .../codegen/EmberMetadata.h | 2 +- 2 files changed, 16 insertions(+), 14 deletions(-) diff --git a/src/data-model-providers/codegen/EmberMetadata.cpp b/src/data-model-providers/codegen/EmberMetadata.cpp index 63808b2b126ffb..54842ee6f25882 100644 --- a/src/data-model-providers/codegen/EmberMetadata.cpp +++ b/src/data-model-providers/codegen/EmberMetadata.cpp @@ -58,7 +58,8 @@ FindAttributeMetadata(const ConcreteAttributePath & aPath) { Status status = ValidateClusterPath(aPath); - if (status != Status::Success) { + if (status != Status::Success) + { return status; } @@ -70,21 +71,22 @@ FindAttributeMetadata(const ConcreteAttributePath & aPath) return metadata; } -Status ValidateClusterPath(const ConcreteClusterPath &path) { +Status ValidateClusterPath(const ConcreteClusterPath & path) +{ - const EmberAfEndpointType * type = emberAfFindEndpointType(path.mEndpointId); - if (type == nullptr) - { - return Status::UnsupportedEndpoint; - } + const EmberAfEndpointType * type = emberAfFindEndpointType(path.mEndpointId); + if (type == nullptr) + { + return Status::UnsupportedEndpoint; + } - const EmberAfCluster * cluster = emberAfFindClusterInType(type, path.mClusterId, CLUSTER_MASK_SERVER); - if (cluster == nullptr) - { - return Status::UnsupportedCluster; - } + const EmberAfCluster * cluster = emberAfFindClusterInType(type, path.mClusterId, CLUSTER_MASK_SERVER); + if (cluster == nullptr) + { + return Status::UnsupportedCluster; + } - return Status::Success; + return Status::Success; } } // namespace Ember diff --git a/src/data-model-providers/codegen/EmberMetadata.h b/src/data-model-providers/codegen/EmberMetadata.h index 0f37a9e4b65b2e..b868d3246cb041 100644 --- a/src/data-model-providers/codegen/EmberMetadata.h +++ b/src/data-model-providers/codegen/EmberMetadata.h @@ -49,7 +49,7 @@ FindAttributeMetadata(const ConcreteAttributePath & aPath); /// - Status::UnsupportedEndpoint if the path endpoint does not exist /// - Status::UnsupportedCluster if the cluster does not exist on the given endpoint /// - Status::Success if the path is valid within Ember -Protocols::InteractionModel::Status ValidateClusterPath(const ConcreteClusterPath &path); +Protocols::InteractionModel::Status ValidateClusterPath(const ConcreteClusterPath & path); } // namespace Ember } // namespace app From 47d9f5f2a02b51a4d80fecd80593df4380f6b75a Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Thu, 30 Jan 2025 12:28:22 -0500 Subject: [PATCH 18/23] Update src/data-model-providers/codegen/EmberMetadata.h Co-authored-by: Boris Zbarsky --- src/data-model-providers/codegen/EmberMetadata.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/data-model-providers/codegen/EmberMetadata.h b/src/data-model-providers/codegen/EmberMetadata.h index b868d3246cb041..d52a440fe7095d 100644 --- a/src/data-model-providers/codegen/EmberMetadata.h +++ b/src/data-model-providers/codegen/EmberMetadata.h @@ -43,7 +43,7 @@ std::variant FindAttributeMetadata(const ConcreteAttributePath & aPath); -/// Returns the status for a given path being valud within ember. +/// Returns the status for a given cluster existing in the ember metadata. /// /// Return code will be one of: /// - Status::UnsupportedEndpoint if the path endpoint does not exist From 7ddeb656f3095396633d84f1da3c3ef81b2428fc Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Thu, 30 Jan 2025 12:28:30 -0500 Subject: [PATCH 19/23] Update src/data-model-providers/codegen/EmberMetadata.h Co-authored-by: Boris Zbarsky --- src/data-model-providers/codegen/EmberMetadata.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/data-model-providers/codegen/EmberMetadata.h b/src/data-model-providers/codegen/EmberMetadata.h index d52a440fe7095d..06e1568be950c7 100644 --- a/src/data-model-providers/codegen/EmberMetadata.h +++ b/src/data-model-providers/codegen/EmberMetadata.h @@ -48,7 +48,7 @@ FindAttributeMetadata(const ConcreteAttributePath & aPath); /// Return code will be one of: /// - Status::UnsupportedEndpoint if the path endpoint does not exist /// - Status::UnsupportedCluster if the cluster does not exist on the given endpoint -/// - Status::Success if the path is valid within Ember +/// - Status::Success if the cluster exists in the ember metadata. Protocols::InteractionModel::Status ValidateClusterPath(const ConcreteClusterPath & path); } // namespace Ember From acede663a6a712176e7c6104673d9d5fd62cdef1 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Thu, 30 Jan 2025 12:28:40 -0500 Subject: [PATCH 20/23] Update src/data-model-providers/codegen/CodegenDataModelProvider.cpp Co-authored-by: Boris Zbarsky --- src/data-model-providers/codegen/CodegenDataModelProvider.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/data-model-providers/codegen/CodegenDataModelProvider.cpp b/src/data-model-providers/codegen/CodegenDataModelProvider.cpp index 6fd146f6573ce7..fcf05c91fd098e 100644 --- a/src/data-model-providers/codegen/CodegenDataModelProvider.cpp +++ b/src/data-model-providers/codegen/CodegenDataModelProvider.cpp @@ -161,7 +161,7 @@ std::optional CodegenDataModelProvider::Invoke(co CommandHandler * handler) { // Double-check that the command path is valid at least up to the cluster level: - // some CommandHandlerInterface are registered on all endpoints, so they would be found by + // some CommandHandlerInterface instances are registered on all endpoints, so they would be found by // `GetCommandHandler` below, however we need to ensure the path makes sense. Protocols::InteractionModel::Status status = Ember::ValidateClusterPath(request.path); if (status != Protocols::InteractionModel::Status::Success) From ceb46859479323e0b6fb52754ab37794e3a2b8bc Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Thu, 30 Jan 2025 12:32:57 -0500 Subject: [PATCH 21/23] Update comment --- src/app/data-model-provider/Provider.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/data-model-provider/Provider.h b/src/app/data-model-provider/Provider.h index aeeffe442967c7..16b205aab0216e 100644 --- a/src/app/data-model-provider/Provider.h +++ b/src/app/data-model-provider/Provider.h @@ -86,7 +86,7 @@ class Provider : public ProviderMetadataTree /// - returning a value other than Success implies an error reply (error and data are mutually exclusive) /// /// Requirements: - /// - command MUST handle any requests and validate them. In particular, in case of invalid + /// - The method MUST handle any requests and validate them. In particular, in case of invalid /// paths, a return code of UnsupportedEndpoint/UnsupportedCluster/UnsupporedCommand is /// expected. /// From 5119c75d481db56a61e4c8ea7fc4a33e929d69cc Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Thu, 30 Jan 2025 12:34:00 -0500 Subject: [PATCH 22/23] Update comment --- src/app/data-model-provider/Provider.h | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/app/data-model-provider/Provider.h b/src/app/data-model-provider/Provider.h index 16b205aab0216e..a5bc301817881d 100644 --- a/src/app/data-model-provider/Provider.h +++ b/src/app/data-model-provider/Provider.h @@ -87,8 +87,9 @@ class Provider : public ProviderMetadataTree /// /// Requirements: /// - The method MUST handle any requests and validate them. In particular, in case of invalid - /// paths, a return code of UnsupportedEndpoint/UnsupportedCluster/UnsupporedCommand is - /// expected. + /// paths we expect one of: + /// a) a return code of UnsupportedEndpoint/UnsupportedCluster/UnsupporedCommand OR + /// b) the status above to be placed inside `handler` /// /// Return value expectations: /// - if a response has been placed into `handler` then std::nullopt MUST be returned. In particular From 5abb6eb201c952859a8d7225500be8d1e28db951 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Fri, 31 Jan 2025 14:55:59 -0500 Subject: [PATCH 23/23] Update based on code review ... if we cannot enforce an interface, we document. Not happy and hoping we can improve in the future --- src/app/data-model-provider/Provider.h | 14 ++++--- .../codegen/CodegenDataModelProvider.cpp | 9 ---- .../tests/TestCodegenModelViaMocks.cpp | 42 ------------------- 3 files changed, 9 insertions(+), 56 deletions(-) diff --git a/src/app/data-model-provider/Provider.h b/src/app/data-model-provider/Provider.h index a5bc301817881d..530911b484fcc0 100644 --- a/src/app/data-model-provider/Provider.h +++ b/src/app/data-model-provider/Provider.h @@ -85,11 +85,15 @@ class Provider : public ProviderMetadataTree /// This includes cases where command handling and value return will be done asynchronously. /// - returning a value other than Success implies an error reply (error and data are mutually exclusive) /// - /// Requirements: - /// - The method MUST handle any requests and validate them. In particular, in case of invalid - /// paths we expect one of: - /// a) a return code of UnsupportedEndpoint/UnsupportedCluster/UnsupporedCommand OR - /// b) the status above to be placed inside `handler` + /// Preconditions: + /// - `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. + /// - 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. /// /// Return value expectations: /// - if a response has been placed into `handler` then std::nullopt MUST be returned. In particular diff --git a/src/data-model-providers/codegen/CodegenDataModelProvider.cpp b/src/data-model-providers/codegen/CodegenDataModelProvider.cpp index fcf05c91fd098e..74fc5b769189a5 100644 --- a/src/data-model-providers/codegen/CodegenDataModelProvider.cpp +++ b/src/data-model-providers/codegen/CodegenDataModelProvider.cpp @@ -160,15 +160,6 @@ std::optional CodegenDataModelProvider::Invoke(co TLV::TLVReader & input_arguments, CommandHandler * handler) { - // Double-check that the command path is valid at least up to the cluster level: - // some CommandHandlerInterface instances are registered on all endpoints, so they would be found by - // `GetCommandHandler` below, however we need to ensure the path makes sense. - Protocols::InteractionModel::Status status = Ember::ValidateClusterPath(request.path); - if (status != Protocols::InteractionModel::Status::Success) - { - return status; - } - CommandHandlerInterface * handler_interface = CommandHandlerInterfaceRegistry::Instance().GetCommandHandler(request.path.mEndpointId, request.path.mClusterId); diff --git a/src/data-model-providers/codegen/tests/TestCodegenModelViaMocks.cpp b/src/data-model-providers/codegen/tests/TestCodegenModelViaMocks.cpp index 239926b4d93770..cf156a786d0d53 100644 --- a/src/data-model-providers/codegen/tests/TestCodegenModelViaMocks.cpp +++ b/src/data-model-providers/codegen/tests/TestCodegenModelViaMocks.cpp @@ -1208,48 +1208,6 @@ TEST_F(TestCodegenModelViaMocks, IterateOverGeneratedCommands) ASSERT_TRUE(cmds.data_equal(Span(expectedCommands3))); } -TEST_F(TestCodegenModelViaMocks, CommandHandlerInterfaceValidity) -{ - UseMockNodeConfig config(gTestNodeConfig); - CodegenDataModelProviderWithContext model; - - // register a CHI on ALL endpoints - CustomListCommandHandler handler(chip::NullOptional, MockClusterId(1)); - handler.SetHandleCommands(true); - - MockCommandHandler commandHandler; - - // Command succeeds on a valid endpoint - { - const ConcreteCommandPath kCommandPath(kMockEndpoint1, MockClusterId(1), kMockCommandId1); - const InvokeRequest kInvokeRequest{ .path = kCommandPath }; - chip::TLV::TLVReader tlvReader; - - // std::nullopt is returned when the command is handled - ASSERT_EQ(model.Invoke(kInvokeRequest, tlvReader, /* handler = */ &commandHandler), std::nullopt); - } - - // Command fails on an invalid endpoint - { - const ConcreteCommandPath kCommandPath(kEndpointIdThatIsMissing, MockClusterId(1), kMockCommandId1); - const InvokeRequest kInvokeRequest{ .path = kCommandPath }; - chip::TLV::TLVReader tlvReader; - - ASSERT_EQ(model.Invoke(kInvokeRequest, tlvReader, /* handler = */ &commandHandler), - Protocols::InteractionModel::Status::UnsupportedEndpoint); - } - - // Command fails on an invalid cluster - { - const ConcreteCommandPath kCommandPath(kMockEndpoint1, MockClusterId(0x1123), kMockCommandId1); - const InvokeRequest kInvokeRequest{ .path = kCommandPath }; - chip::TLV::TLVReader tlvReader; - - ASSERT_EQ(model.Invoke(kInvokeRequest, tlvReader, /* handler = */ &commandHandler), - Protocols::InteractionModel::Status::UnsupportedCluster); - } -} - TEST_F(TestCodegenModelViaMocks, AcceptedGeneratedCommandsOnInvalidEndpoints) { UseMockNodeConfig config(gTestNodeConfig);