Decouple InitDataModelHandler from libCHIP (#36725)

* Decouple InitDataModelHandler from libCHIP

* Use StartUp to init

* Seperate namespace cleanup out

* Mock function for linking

* Restyled by clang-format

* Add API comment

* Fix mutiple defination conflicts

* Address review comments

* Restyled by whitespace

* Seperate InitDataModel out

* Revert "Seperate InitDataModel out"

This reverts commit 5a8af59.

* Do not directly manipulate the base class's Startup method

* Address review comment

* Restyled by whitespace

* Adjust the init order

* Restyled by whitespace

* Update src/app/server/Server.cpp

Co-authored-by: Boris Zbarsky <[email protected]>

* Update src/controller/CHIPDeviceControllerFactory.cpp

Co-authored-by: Boris Zbarsky <[email protected]>

* Add TODO comment

* documented/named InitDataModel to make it clear that this is a temporary hack for a test

---------

Co-authored-by: Restyled.io <[email protected]>
Co-authored-by: Boris Zbarsky <[email protected]>

src/app/server/Server.cpp

@@ -27,7 +27,6 @@
 #include <app/data-model-provider/Provider.h>
 #include <app/server/Dnssd.h>
 #include <app/server/EchoHandler.h>
-#include <app/util/DataModelHandler.h>
 
 #if CONFIG_NETWORK_LAYER_BLE
 #include <ble/Ble.h>
@@ -170,17 +169,6 @@ CHIP_ERROR Server::Init(const ServerInitParams & initParams)
     SuccessOrExit(err = mAttributePersister.Init(mDeviceStorage));
     SetSafeAttributePersistenceProvider(&mAttributePersister);
 
-    // SetDataModelProvider() actually initializes/starts the provider.  We need
-    // to preserve the following ordering guarantees:
-    //
-    // 1) Provider initialization (under SetDataModelProvider) happens after
-    //    SetSafeAttributePersistenceProvider, since the provider can then use
-    //    the safe persistence provider to implement and initialize its own attribute persistence logic.
-    // 2) For now, provider initialization happens before InitDataModelHandler(), which depends
-    //    on atttribute persistence being already set up before it runs.  Longer-term, the logic from
-    //    InitDataModelHandler should just move into the codegen provider.
-    app::InteractionModelEngine::GetInstance()->SetDataModelProvider(initParams.dataModelProvider);
-
     {
         FabricTable::InitParams fabricTableInitParams;
         fabricTableInitParams.storage             = mDeviceStorage;
@@ -302,8 +290,22 @@ CHIP_ERROR Server::Init(const ServerInitParams & initParams)
     }
 #endif // CHIP_CONFIG_ENABLE_SERVER_IM_EVENT
 
-    // This initializes clusters, so should come after lower level initialization.
-    InitDataModelHandler();
+    // SetDataModelProvider() initializes and starts the provider, which in turn
+    // triggers the initialization of cluster implementations. This callsite is
+    // critical because it ensures that cluster-level initialization occurs only
+    // after all necessary low-level dependencies have been set up.
+    //
+    // Ordering guarantees:
+    // 1) Provider initialization (under SetDataModelProvider) must happen after
+    //    SetSafeAttributePersistenceProvider to ensure the provider can leverage
+    //    the safe persistence provider for attribute persistence logic.
+    // 2) It must occur after all low-level components that cluster implementations
+    //    might depend on have been initialized, as they rely on these components
+    //    during their own initialization.
+    //
+    // This remains the single point of entry to ensure that all cluster-level
+    // initialization is performed in the correct order.
+    app::InteractionModelEngine::GetInstance()->SetDataModelProvider(initParams.dataModelProvider);
 
 #if defined(CHIP_APP_USE_ECHO)
     err = InitEchoHandler(&mExchangeMgr);

src/app/tests/TestCommissioningWindowManager.cpp

@@ -41,9 +41,6 @@ using chip::CommissioningWindowAdvertisement;
 using chip::CommissioningWindowManager;
 using chip::Server;
 
-// Mock function for linking
-void InitDataModelHandler() {}
-
 namespace {
 bool sAdminFabricIndexDirty = false;
 bool sAdminVendorIdDirty    = false;

src/app/tests/TestInteractionModelEngine.cpp

@@ -39,8 +39,8 @@
 #if CHIP_CONFIG_PERSIST_SUBSCRIPTIONS
 #include <app/SimpleSubscriptionResumptionStorage.h>
 #include <lib/support/TestPersistentStorageDelegate.h>
-
 #endif // CHIP_CONFIG_PERSIST_SUBSCRIPTIONS
+
 namespace {
 
 class NullReadHandlerCallback : public chip::app::ReadHandler::ManagementCallback

src/app/tests/test-ember-api.cpp

@@ -34,3 +34,6 @@ uint16_t emberAfGetClusterServerEndpointIndex(chip::EndpointId endpoint, chip::C
     }
     return endpoint;
 }
+
+// Mock function for linking
+void InitDataModelHandler() {}

src/controller/CHIPDeviceControllerFactory.cpp

@@ -252,16 +252,10 @@ CHIP_ERROR DeviceControllerFactory::InitSystemState(FactoryInitParams params)
 
     chip::app::InteractionModelEngine * interactionModelEngine = chip::app::InteractionModelEngine::GetInstance();
 
-    // Note placement of this BEFORE `InitDataModelHandler` since InitDataModelHandler may
-    // rely on ember (does emberAfInit() and configure which may load data from NVM).
-    //
-    // Expected forward path is that we will move move and more things inside datamodel
-    // provider (e.g. storage settings) so we want datamodelprovider available before
-    // `InitDataModelHandler`.
+    // Initialize the data model now that everything cluster implementations might
+    // depend on is initalized.
     interactionModelEngine->SetDataModelProvider(params.dataModelProvider);
 
-    InitDataModelHandler();
-
     ReturnErrorOnFailure(Dnssd::Resolver::Instance().Init(stateParams.udpEndPointManager));
 
     if (params.enableServerInteractions)

src/controller/tests/TestServerCommandDispatch.cpp

@@ -126,6 +126,9 @@ CHIP_ERROR TestClusterCommandHandler::EnumerateAcceptedCommands(const ConcreteCl
 
 namespace {
 
+// TODO:(#36837) implementing its own provider instead of using "CodegenDataModelProvider"
+// TestServerCommandDispatch should provide its own dedicated data model provider rather than using CodegenDataModelProvider
+// provider. This class exists solely for one specific test scenario, on a temporary basis.
 class DispatchTestDataModel : public CodegenDataModelProvider
 {
 public:
@@ -134,6 +137,20 @@ class DispatchTestDataModel : public CodegenDataModelProvider
         static DispatchTestDataModel instance;
         return instance;
     }
+
+    // The Startup method initializes the data model provider with a given context.
+    // This approach ensures that the test relies on a more controlled and explicit data model provider
+    // rather than depending on the code-generated one with undefined modifications.
+    CHIP_ERROR Startup(DataModel::InteractionModelContext context) override
+    {
+        ReturnErrorOnFailure(CodegenDataModelProvider::Startup(context));
+        return CHIP_NO_ERROR;
+    }
+
+protected:
+    // Since the current unit tests do not involve any cluster implementations, we override InitDataModelForTesting
+    // to do nothing, thereby preventing calls to the Ember-specific InitDataModelHandler.
+    void InitDataModelForTesting() override {}
 };
 
 class TestServerCommandDispatch : public chip::Test::AppContext
@@ -144,6 +161,7 @@ class TestServerCommandDispatch : public chip::Test::AppContext
         AppContext::SetUp();
         mOldProvider = InteractionModelEngine::GetInstance()->SetDataModelProvider(&DispatchTestDataModel::Instance());
     }
+
     void TearDown()
     {
         InteractionModelEngine::GetInstance()->SetDataModelProvider(mOldProvider);

src/controller/tests/data_model/DataModelFixtures.cpp

@@ -40,6 +40,9 @@ using namespace chip::app::Clusters;
 using namespace chip::app::Clusters::UnitTesting;
 using namespace chip::Protocols;
 
+// Mock function for linking
+void InitDataModelHandler() {}
+
 namespace chip {
 namespace app {
 

src/data-model-providers/codegen/CodegenDataModelProvider.cpp

@@ -26,6 +26,7 @@
 #include <app/RequiredPrivilege.h>
 #include <app/data-model-provider/MetadataTypes.h>
 #include <app/data-model-provider/Provider.h>
+#include <app/util/DataModelHandler.h>
 #include <app/util/IMClusterCommandHandler.h>
 #include <app/util/af-types.h>
 #include <app/util/attribute-storage.h>
@@ -417,6 +418,8 @@ CHIP_ERROR CodegenDataModelProvider::Startup(DataModel::InteractionModelContext
         }
     }
 
+    InitDataModelForTesting();
+
     return CHIP_NO_ERROR;
 }
 
@@ -859,6 +862,12 @@ ConcreteCommandPath CodegenDataModelProvider::NextGeneratedCommand(const Concret
     return ConcreteCommandPath(before.mEndpointId, before.mClusterId, commandId);
 }
 
+void CodegenDataModelProvider::InitDataModelForTesting()
+{
+    // Call the Ember-specific InitDataModelHandler
+    InitDataModelHandler();
+}
+
 std::optional<DataModel::DeviceTypeEntry> CodegenDataModelProvider::FirstDeviceType(EndpointId endpoint)
 {
     // Use the `Index` version even though `emberAfDeviceTypeListFromEndpoint` would work because

src/data-model-providers/codegen/CodegenDataModelProvider.h

@@ -187,6 +187,12 @@ class CodegenDataModelProvider : public DataModel::Provider
 
     void Temporary_ReportAttributeChanged(const AttributePathParams & path) override;
 
+protected:
+    // Temporary hack for a test: Initializes the data model for testing purposes only.
+    // This method serves as a placeholder and should NOT be used outside of specific tests.
+    // It is expected to be removed or replaced with a proper implementation in the future.TODO:(#36837).
+    virtual void InitDataModelForTesting();
+
 private:
     // Iteration is often done in a tight loop going through all values.
     // To avoid N^2 iterations, cache a hint of where something is positioned

src/data-model-providers/codegen/tests/TestCodegenModelViaMocks.cpp

@@ -75,6 +75,9 @@ using namespace chip::app::Clusters::Globals::Attributes;
 
 using chip::Protocols::InteractionModel::Status;
 
+// Mock function for linking
+void InitDataModelHandler() {}
+
 namespace {
 
 constexpr AttributeId kAttributeIdReadOnly   = 0x3001;