Update code flow to hopefully make it somewhat easier to read

Author: andreilitvin

src/app/codegen-data-model-provider/CodegenDataModelProvider.cpp

@@ -14,11 +14,13 @@
  *    See the License for the specific language governing permissions and
  *    limitations under the License.
  */
+#include "lib/support/logging/TextOnlyLogging.h"
 #include <app/codegen-data-model-provider/CodegenDataModelProvider.h>
 
 #include <app-common/zap-generated/attribute-type.h>
 #include <app/CommandHandlerInterface.h>
 #include <app/CommandHandlerInterfaceRegistry.h>
+#include <app/ConcreteClusterPath.h>
 #include <app/RequiredPrivilege.h>
 #include <app/data-model-provider/MetadataTypes.h>
 #include <app/util/IMClusterCommandHandler.h>
@@ -45,22 +47,33 @@ namespace {
 class EnumeratorCommandFinder
 {
 public:
+    using HandlerCallbackFunction = CHIP_ERROR (CommandHandlerInterface::*)(const ConcreteClusterPath &,
+                                                                            CommandHandlerInterface::CommandIdCallback, void *);
+
     enum class Operation
     {
         FindFirst, // Find the first value in the list
         FindExact, // Find the given value
         FindNext   // Find the value AFTER this value
     };
 
-    EnumeratorCommandFinder(Operation operation, CommandId target) : mOperation(operation), mTarget(target) {}
+    EnumeratorCommandFinder(HandlerCallbackFunction callback, Operation operation, CommandId target = kInvalidCommandId) :
+        mCallback(callback), mOperation(operation), mTarget(target)
+    {}
 
-    /// Callback to pass in to `Enumerate*` calls in CommandHandlerInterface
-    CommandHandlerInterface::CommandIdCallback Callback() { return HandlerCallbackFn; }
+    /// Find the given command id fo
+    ///
+    /// Returns:
+    ///    - std::nullopt if no command found using the command handler interface
+    ///    - kInvalidCommandId if the find failed (but command handler interface does provide a list)
+    ///    - valid id if command handler interface usage succeeds
+    std::optional<CommandId> FindCommandId(ConcreteClusterPath cluster);
 
-    /// Get the element found (if any) after the `Callback` is used
-    std::optional<CommandId> GetFound() const { return mFound; }
+    /// Uses FindCommandId to find the given command and loads the command entry data
+    std::optional<DataModel::CommandEntry> FindCommandEntry(ConcreteClusterPath cluster);
 
 private:
+    HandlerCallbackFunction mCallback;
     Operation mOperation;
     CommandId mTarget;
     std::optional<CommandId> mFound = std::nullopt;
@@ -97,6 +110,33 @@ class EnumeratorCommandFinder
     }
 };
 
+std::optional<CommandId> EnumeratorCommandFinder::FindCommandId(ConcreteClusterPath cluster)
+{
+    CommandHandlerInterface * interface =
+        CommandHandlerInterfaceRegistry::Instance().GetCommandHandler(cluster.mEndpointId, cluster.mClusterId);
+
+    if (interface == nullptr)
+    {
+        return std::nullopt; // no data: no interface
+    }
+
+    CHIP_ERROR err = (interface->*mCallback)(cluster, HandlerCallbackFn, this);
+    if (err == CHIP_ERROR_NOT_IMPLEMENTED)
+    {
+        return std::nullopt; // no data provided by the interface
+    }
+
+    if (err != CHIP_NO_ERROR)
+    {
+        // Report the error here since we lose actual error. This generally should NOT be possible as CHI usually returns
+        // unimplemented or should just work for our use case (our callback never fails)
+        ChipLogError(DataManagement, "Enumerate error: %" CHIP_ERROR_FORMAT, err.Format());
+        return kInvalidCommandId;
+    }
+
+    return mFound.value_or(kInvalidCommandId);
+}
+
 /// Load the cluster information into the specified destination
 std::variant<CHIP_ERROR, DataModel::ClusterInfo> LoadClusterInfo(const ConcreteClusterPath & path, const EmberAfCluster & cluster)
 {
@@ -226,6 +266,19 @@ DataModel::CommandEntry CommandEntryFrom(const ConcreteClusterPath & clusterPath
     return entry;
 }
 
+std::optional<DataModel::CommandEntry> EnumeratorCommandFinder::FindCommandEntry(ConcreteClusterPath cluster)
+{
+
+    std::optional<CommandId> id = FindCommandId(cluster);
+
+    if (!id.has_value())
+    {
+        return std::nullopt;
+    }
+
+    return (*id == kInvalidCommandId) ? DataModel::CommandEntry::kInvalid : CommandEntryFrom(cluster, *id);
+}
+
 const ConcreteCommandPath kInvalidCommandPath(kInvalidEndpointId, kInvalidClusterId, kInvalidCommandId);
 
 } // namespace
@@ -558,25 +611,13 @@ std::optional<DataModel::AttributeInfo> CodegenDataModelProvider::GetAttributeIn
 
 DataModel::CommandEntry CodegenDataModelProvider::FirstAcceptedCommand(const ConcreteClusterPath & path)
 {
-    CommandHandlerInterface * interface =
-        CommandHandlerInterfaceRegistry::Instance().GetCommandHandler(path.mEndpointId, path.mClusterId);
+    auto handlerInterfaceValue =
+        EnumeratorCommandFinder(&CommandHandlerInterface::EnumerateAcceptedCommands, EnumeratorCommandFinder::Operation::FindFirst)
+            .FindCommandEntry(path);
 
-    if (interface != nullptr)
+    if (handlerInterfaceValue.has_value())
     {
-        EnumeratorCommandFinder finder(EnumeratorCommandFinder::Operation::FindFirst, kInvalidCommandId);
-        CHIP_ERROR err = interface->EnumerateAcceptedCommands(path, finder.Callback(), &finder);
-
-        if (err != CHIP_ERROR_NOT_IMPLEMENTED)
-        {
-            auto firstId = finder.GetFound();
-
-            if ((err == CHIP_NO_ERROR) && firstId.has_value())
-            {
-                return CommandEntryFrom(path, *firstId);
-            }
-
-            return DataModel::CommandEntry::kInvalid;
-        }
+        return *handlerInterfaceValue;
     }
 
     const EmberAfCluster * cluster = FindServerCluster(path);
@@ -591,27 +632,14 @@ DataModel::CommandEntry CodegenDataModelProvider::FirstAcceptedCommand(const Con
 
 DataModel::CommandEntry CodegenDataModelProvider::NextAcceptedCommand(const ConcreteCommandPath & before)
 {
-    CommandHandlerInterface * interface =
-        CommandHandlerInterfaceRegistry::Instance().GetCommandHandler(before.mEndpointId, before.mClusterId);
-
     // TODO: `Next` redirecting to a callback is slow O(n^2).
     //       see https://github.com/project-chip/connectedhomeip/issues/35790
-    if (interface != nullptr)
+    auto handlerInterfaceValue = EnumeratorCommandFinder(&CommandHandlerInterface::EnumerateAcceptedCommands,
+                                                         EnumeratorCommandFinder::Operation::FindNext, before.mCommandId)
+                                     .FindCommandEntry(before);
+    if (handlerInterfaceValue.has_value())
     {
-        EnumeratorCommandFinder finder(EnumeratorCommandFinder::Operation::FindNext, before.mCommandId);
-        CHIP_ERROR err = interface->EnumerateAcceptedCommands(before, finder.Callback(), &finder);
-
-        if (err != CHIP_ERROR_NOT_IMPLEMENTED)
-        {
-            auto nextId = finder.GetFound();
-
-            if ((err == CHIP_NO_ERROR) && nextId.has_value())
-            {
-                return CommandEntryFrom(before, *nextId);
-            }
-
-            return DataModel::CommandEntry::kInvalid;
-        }
+        return *handlerInterfaceValue;
     }
 
     const EmberAfCluster * cluster = FindServerCluster(before);
@@ -626,27 +654,13 @@ DataModel::CommandEntry CodegenDataModelProvider::NextAcceptedCommand(const Conc
 
 std::optional<DataModel::CommandInfo> CodegenDataModelProvider::GetAcceptedCommandInfo(const ConcreteCommandPath & path)
 {
-    // Command handler interface MAY override lists of commands
-    CommandHandlerInterface * interface =
-        CommandHandlerInterfaceRegistry::Instance().GetCommandHandler(path.mEndpointId, path.mClusterId);
+    auto handlerInterfaceValue = EnumeratorCommandFinder(&CommandHandlerInterface::EnumerateAcceptedCommands,
+                                                         EnumeratorCommandFinder::Operation::FindExact, path.mCommandId)
+                                     .FindCommandEntry(path);
 
-    if (interface != nullptr)
+    if (handlerInterfaceValue.has_value())
     {
-        EnumeratorCommandFinder finder(EnumeratorCommandFinder::Operation::FindExact, path.mCommandId);
-        CHIP_ERROR err = interface->EnumerateGeneratedCommands(path, finder.Callback(), &finder);
-
-        if (err != CHIP_ERROR_NOT_IMPLEMENTED)
-        {
-            auto commandId = finder.GetFound();
-
-            if ((err == CHIP_NO_ERROR) && commandId.has_value())
-            {
-                //  definitive answer: command exists
-                return CommandEntryFrom(path, *commandId).info;
-            }
-
-            return std::nullopt;
-        }
+        return handlerInterfaceValue->IsValid() ? std::make_optional(handlerInterfaceValue->info) : std::nullopt;
     }
 
     const EmberAfCluster * cluster = FindServerCluster(path);
@@ -659,59 +673,35 @@ std::optional<DataModel::CommandInfo> CodegenDataModelProvider::GetAcceptedComma
 
 ConcreteCommandPath CodegenDataModelProvider::FirstGeneratedCommand(const ConcreteClusterPath & path)
 {
-    CommandHandlerInterface * interface =
-        CommandHandlerInterfaceRegistry::Instance().GetCommandHandler(path.mEndpointId, path.mClusterId);
-
-    if (interface != nullptr)
+    std::optional<CommandId> commandId =
+        EnumeratorCommandFinder(&CommandHandlerInterface::EnumerateGeneratedCommands, EnumeratorCommandFinder::Operation::FindFirst)
+            .FindCommandId(path);
+    if (commandId.has_value())
     {
-        EnumeratorCommandFinder finder(EnumeratorCommandFinder::Operation::FindFirst, kInvalidCommandId);
-        CHIP_ERROR err = interface->EnumerateGeneratedCommands(path, finder.Callback(), &finder);
-
-        if (err != CHIP_ERROR_NOT_IMPLEMENTED)
-        {
-            auto firstId = finder.GetFound();
-
-            if ((err == CHIP_NO_ERROR) && firstId.has_value())
-            {
-                return ConcreteCommandPath(path.mEndpointId, path.mClusterId, *firstId);
-            }
-
-            return kInvalidCommandPath;
-        }
+        return *commandId == kInvalidCommandId ? kInvalidCommandPath
+                                               : ConcreteCommandPath(path.mEndpointId, path.mClusterId, *commandId);
     }
 
     const EmberAfCluster * cluster = FindServerCluster(path);
-
     VerifyOrReturnValue(cluster != nullptr, kInvalidCommandPath);
 
-    std::optional<CommandId> commandId = mGeneratedCommandsIterator.First(cluster->generatedCommandList);
+    commandId = mGeneratedCommandsIterator.First(cluster->generatedCommandList);
     VerifyOrReturnValue(commandId.has_value(), kInvalidCommandPath);
     return ConcreteCommandPath(path.mEndpointId, path.mClusterId, *commandId);
 }
 
 ConcreteCommandPath CodegenDataModelProvider::NextGeneratedCommand(const ConcreteCommandPath & before)
 {
-    CommandHandlerInterface * interface =
-        CommandHandlerInterfaceRegistry::Instance().GetCommandHandler(before.mEndpointId, before.mClusterId);
-
     // TODO: `Next` redirecting to a callback is slow O(n^2).
     //       see https://github.com/project-chip/connectedhomeip/issues/35790
-    if (interface != nullptr)
-    {
-        EnumeratorCommandFinder finder(EnumeratorCommandFinder::Operation::FindNext, before.mCommandId);
-        CHIP_ERROR err = interface->EnumerateGeneratedCommands(before, finder.Callback(), &finder);
-
-        if (err != CHIP_ERROR_NOT_IMPLEMENTED)
-        {
-            auto nextId = finder.GetFound();
-
-            if ((err == CHIP_NO_ERROR) && nextId.has_value())
-            {
-                return ConcreteCommandPath(before.mEndpointId, before.mClusterId, *nextId);
-            }
+    auto nextId = EnumeratorCommandFinder(&CommandHandlerInterface::EnumerateGeneratedCommands,
+                                          EnumeratorCommandFinder::Operation::FindNext, before.mCommandId)
+                      .FindCommandId(before);
 
-            return kInvalidCommandPath;
-        }
+    if (nextId.has_value())
+    {
+        return (*nextId == kInvalidCommandId) ? kInvalidCommandPath
+                                              : ConcreteCommandPath(before.mEndpointId, before.mClusterId, *nextId);
     }
 
     const EmberAfCluster * cluster = FindServerCluster(before);

src/app/codegen-data-model-provider/tests/TestCodegenModelViaMocks.cpp

@@ -1382,17 +1382,17 @@ TEST(TestCodegenModelViaMocks, CommandHandlerInterfaceAcceptedCommands)
     CustomListCommandHandler handler(MakeOptional(kMockEndpoint1), MockClusterId(1));
 
     // At this point, without overrides, there should be no accepted/generated commands
-    ASSERT_FALSE(model.FirstAcceptedCommand(ConcreteClusterPath(kMockEndpoint1, MockClusterId(1))).IsValid());
-    ASSERT_FALSE(model.FirstGeneratedCommand(ConcreteClusterPath(kMockEndpoint1, MockClusterId(1))).HasValidIds());
-    ASSERT_FALSE(model.GetAcceptedCommandInfo(ConcreteCommandPath(kMockEndpoint1, MockClusterId(1), 1234)).has_value());
+    EXPECT_FALSE(model.FirstAcceptedCommand(ConcreteClusterPath(kMockEndpoint1, MockClusterId(1))).IsValid());
+    EXPECT_FALSE(model.FirstGeneratedCommand(ConcreteClusterPath(kMockEndpoint1, MockClusterId(1))).HasValidIds());
+    EXPECT_FALSE(model.GetAcceptedCommandInfo(ConcreteCommandPath(kMockEndpoint1, MockClusterId(1), 1234)).has_value());
 
     handler.SetOverrideAccepted(true);
     handler.SetOverrideGenerated(true);
 
     // with overrides, the list is still empty ...
-    ASSERT_FALSE(model.FirstAcceptedCommand(ConcreteClusterPath(kMockEndpoint1, MockClusterId(1))).IsValid());
-    ASSERT_FALSE(model.FirstGeneratedCommand(ConcreteClusterPath(kMockEndpoint1, MockClusterId(1))).HasValidIds());
-    ASSERT_FALSE(model.GetAcceptedCommandInfo(ConcreteCommandPath(kMockEndpoint1, MockClusterId(1), 1234)).has_value());
+    EXPECT_FALSE(model.FirstAcceptedCommand(ConcreteClusterPath(kMockEndpoint1, MockClusterId(1))).IsValid());
+    EXPECT_FALSE(model.FirstGeneratedCommand(ConcreteClusterPath(kMockEndpoint1, MockClusterId(1))).HasValidIds());
+    EXPECT_FALSE(model.GetAcceptedCommandInfo(ConcreteCommandPath(kMockEndpoint1, MockClusterId(1), 1234)).has_value());
 
     // set some overrides
     handler.AcceptedVec().push_back(1234);
@@ -1403,26 +1403,26 @@ TEST(TestCodegenModelViaMocks, CommandHandlerInterfaceAcceptedCommands)
     DataModel::CommandEntry entry;
 
     entry = model.FirstAcceptedCommand(ConcreteClusterPath(kMockEndpoint1, MockClusterId(1)));
-    ASSERT_TRUE(entry.IsValid());
-    ASSERT_EQ(entry.path, ConcreteCommandPath(kMockEndpoint1, MockClusterId(1), 1234));
+    EXPECT_TRUE(entry.IsValid());
+    EXPECT_EQ(entry.path, ConcreteCommandPath(kMockEndpoint1, MockClusterId(1), 1234));
 
     entry = model.NextAcceptedCommand(entry.path);
-    ASSERT_TRUE(entry.IsValid());
-    ASSERT_EQ(entry.path, ConcreteCommandPath(kMockEndpoint1, MockClusterId(1), 999));
+    EXPECT_TRUE(entry.IsValid());
+    EXPECT_EQ(entry.path, ConcreteCommandPath(kMockEndpoint1, MockClusterId(1), 999));
 
     entry = model.NextAcceptedCommand(entry.path);
-    ASSERT_FALSE(entry.IsValid());
+    EXPECT_FALSE(entry.IsValid());
 
     ConcreteCommandPath path = model.FirstGeneratedCommand(ConcreteClusterPath(kMockEndpoint1, MockClusterId(1)));
-    ASSERT_TRUE(path.HasValidIds());
-    ASSERT_EQ(path, ConcreteCommandPath(kMockEndpoint1, MockClusterId(1), 33));
+    EXPECT_TRUE(path.HasValidIds());
+    EXPECT_EQ(path, ConcreteCommandPath(kMockEndpoint1, MockClusterId(1), 33));
     path = model.NextGeneratedCommand(path);
-    ASSERT_FALSE(path.HasValidIds());
+    EXPECT_FALSE(path.HasValidIds());
 
     // Command finding should work
-    ASSERT_TRUE(model.GetAcceptedCommandInfo(ConcreteCommandPath(kMockEndpoint1, MockClusterId(1), 1234)).has_value());
-    ASSERT_FALSE(model.GetAcceptedCommandInfo(ConcreteCommandPath(kMockEndpoint1, MockClusterId(1), 88)).has_value());
-    ASSERT_FALSE(model.GetAcceptedCommandInfo(ConcreteCommandPath(kMockEndpoint1, MockClusterId(1), 33)).has_value());
+    EXPECT_TRUE(model.GetAcceptedCommandInfo(ConcreteCommandPath(kMockEndpoint1, MockClusterId(1), 1234)).has_value());
+    EXPECT_FALSE(model.GetAcceptedCommandInfo(ConcreteCommandPath(kMockEndpoint1, MockClusterId(1), 88)).has_value());
+    EXPECT_FALSE(model.GetAcceptedCommandInfo(ConcreteCommandPath(kMockEndpoint1, MockClusterId(1), 33)).has_value());
 }
 
 TEST(TestCodegenModelViaMocks, EmberAttributeReadAclDeny)