Strong const correctness into the contexts

andreilitvin · andreilitvin · commit d51a00c09344 · 2025-03-07T09:26:34.000-05:00
diff --git a/src/app/server-cluster/ServerClusterContext.h b/src/app/server-cluster/ServerClusterContext.h
@@ -34,9 +34,11 @@ namespace app {
 /// for every cluster maintaining such a context.
 struct ServerClusterContext
 {
-    DataModel::Provider * provider                          = nullptr; /// underlying provider that the cluster operates in
-    PersistentStorageDelegate * storage                     = nullptr; /// read/write persistent storage
-    DataModel::InteractionModelContext * interactionContext = nullptr; /// outside-world communication
+    DataModel::Provider * const provider                          = nullptr; /// underlying provider that the cluster operates in
+    PersistentStorageDelegate * const storage                     = nullptr; /// read/write persistent storage
+    DataModel::InteractionModelContext * const interactionContext = nullptr; /// outside-world communication
+
+    ServerClusterContext(ServerClusterContext &&) = default;
 
     bool operator!=(const ServerClusterContext & other) const
     {
diff --git a/src/app/server-cluster/testing/TestServerClusterContext.h b/src/app/server-cluster/testing/TestServerClusterContext.h
@@ -42,20 +42,35 @@ class NullActionContext : public app::DataModel::ActionContext
 ///   At thist time, `interactionContext::actionContext::CurrentExchange` WILL return nullptr
 ///   in the existing implementation as the exchange is too heavy of an object
 ///   to create for testing
-class TestServerClusterContext : public app::ServerClusterContext
+class TestServerClusterContext
 {
 public:
-    TestServerClusterContext()
+    TestServerClusterContext() :
+        mContext{
+            .provider           = &mTestProvider,
+            .storage            = &mTestStorage,
+            .interactionContext = &mTestContext,
+        }
     {
         mTestContext.eventsGenerator         = &mTestEventsGenerator;
         mTestContext.dataModelChangeListener = &mTestDataModelChangeListener;
         mTestContext.actionContext           = &mNullActionContext;
-
-        provider           = &mTestProvider;
-        storage            = &mTestStorage;
-        interactionContext = &mTestContext;
     }
 
+    /// Get a stable pointer to the underlying context
+    app::ServerClusterContext * Get() { return &mContext; }
+
+    /// Create a new context bound to this test context
+    app::ServerClusterContext Create()
+    {
+        return {
+            .provider           = &mTestProvider,
+            .storage            = &mTestStorage,
+            .interactionContext = &mTestContext,
+
+        };
+    };
+
     LogOnlyEvents & EventsGenerator() { return mTestEventsGenerator; }
     TestProviderChangeListener & ChangeListener() { return mTestDataModelChangeListener; }
     TestPersistentStorageDelegate & StorageDelegate() { return mTestStorage; }
@@ -68,6 +83,8 @@ class TestServerClusterContext : public app::ServerClusterContext
     TestPersistentStorageDelegate mTestStorage;
 
     app::DataModel::InteractionModelContext mTestContext;
+
+    app::ServerClusterContext mContext;
 };
 
 } // namespace Test
diff --git a/src/app/server-cluster/tests/TestDefaultServerCluster.cpp b/src/app/server-cluster/tests/TestDefaultServerCluster.cpp
@@ -14,6 +14,7 @@
  *    See the License for the specific language governing permissions and
  *    limitations under the License.
  */
+#include "app/server-cluster/ServerClusterContext.h"
 #include <pw_unit_test/framework.h>
 
 #include <access/Privilege.h>
@@ -190,7 +191,7 @@ TEST(TestDefaultServerCluster, NotifyAttributeChanged)
 
     // Create a ServerClusterContext and verify that attribute change notifications are processed.
     TestServerClusterContext context;
-    ASSERT_EQ(cluster.Startup(&context), CHIP_NO_ERROR);
+    ASSERT_EQ(cluster.Startup(context.Get()), CHIP_NO_ERROR);
 
     oldVersion = cluster.GetDataVersion();
     cluster.TestNotifyAttributeChanged(234);
@@ -214,7 +215,7 @@ TEST(TestDefaultServerCluster, NotifyAllAttributesChanged)
 
     // Create a ServerClusterContext and verify that attribute change notifications are processed.
     TestServerClusterContext context;
-    ASSERT_EQ(cluster.Startup(&context), CHIP_NO_ERROR);
+    ASSERT_EQ(cluster.Startup(context.Get()), CHIP_NO_ERROR);
 
     oldVersion = cluster.GetDataVersion();
     cluster.TestNotifyAllAttributesChanged();
diff --git a/src/data-model-providers/codegen/CodegenDataModelProvider.cpp b/src/data-model-providers/codegen/CodegenDataModelProvider.cpp
@@ -164,16 +164,11 @@ CHIP_ERROR CodegenDataModelProvider::Startup(DataModel::InteractionModelContext
 
     InitDataModelForTesting();
 
-    {
-        ServerClusterContext clusterContext;
-        clusterContext.provider           = this;
-        clusterContext.storage            = mPersistentStorageDelegate;
-        clusterContext.interactionContext = &mContext;
-
-        ReturnErrorOnFailure(mRegistry.SetContext(clusterContext));
-    }
-
-    return CHIP_NO_ERROR;
+    return mRegistry.SetContext(ServerClusterContext{
+        .provider           = this,
+        .storage            = mPersistentStorageDelegate,
+        .interactionContext = &mContext,
+    });
 }
 
 std::optional<DataModel::ActionReturnStatus> CodegenDataModelProvider::InvokeCommand(const DataModel::InvokeRequest & request,
diff --git a/src/data-model-providers/codegen/ServerClusterInterfaceRegistry.cpp b/src/data-model-providers/codegen/ServerClusterInterfaceRegistry.cpp
@@ -22,6 +22,7 @@
 #include <lib/core/DataModelTypes.h>
 #include <lib/support/CHIPMem.h>
 #include <lib/support/CodeUtils.h>
+#include <optional>
 
 namespace chip {
 namespace app {
@@ -31,7 +32,7 @@ ServerClusterInterfaceRegistry::~ServerClusterInterfaceRegistry()
     while (mRegistrations != nullptr)
     {
         ServerClusterRegistration * next = mRegistrations->next;
-        if (mContextIsValid)
+        if (mContext.has_value())
         {
             mRegistrations->serverClusterInterface->Shutdown();
         }
@@ -55,9 +56,9 @@ CHIP_ERROR ServerClusterInterfaceRegistry::Register(ServerClusterRegistration &
     // items. We preserve this however we may want to make this optional at some point in time.
     VerifyOrReturnError(Get(path) == nullptr, CHIP_ERROR_DUPLICATE_KEY_ID);
 
-    if (mContextIsValid)
+    if (mContext.has_value())
     {
-        ReturnErrorOnFailure(entry.serverClusterInterface->Startup(&mContext));
+        ReturnErrorOnFailure(entry.serverClusterInterface->Startup(&*mContext));
     }
 
     entry.next     = mRegistrations;
@@ -93,7 +94,7 @@ ServerClusterInterface * ServerClusterInterfaceRegistry::Unregister(const Concre
             }
 
             current->next = nullptr; // Make sure current does not look like part of a list.
-            if (mContextIsValid)
+            if (mContext.has_value())
             {
                 current->serverClusterInterface->Shutdown();
             }
@@ -137,7 +138,7 @@ void ServerClusterInterfaceRegistry::UnregisterAllFromEndpoint(EndpointId endpoi
             ServerClusterRegistration * actual_next = current->next;
 
             current->next = nullptr; // Make sure current does not look like part of a list.
-            if (mContextIsValid)
+            if (mContext.has_value())
             {
                 current->serverClusterInterface->Shutdown();
             }
@@ -178,22 +179,21 @@ ServerClusterInterface * ServerClusterInterfaceRegistry::Get(const ConcreteClust
     return nullptr;
 }
 
-CHIP_ERROR ServerClusterInterfaceRegistry::SetContext(const ServerClusterContext & context)
+CHIP_ERROR ServerClusterInterfaceRegistry::SetContext(ServerClusterContext && context)
 {
-    if (mContextIsValid)
+    if (mContext.has_value())
     {
         // if there is no difference, do not re-initialize.
-        VerifyOrReturnError(mContext != context, CHIP_NO_ERROR);
+        VerifyOrReturnError(*mContext != context, CHIP_NO_ERROR);
         ClearContext();
     }
 
-    mContext         = context;
-    mContextIsValid  = true;
+    mContext.emplace(std::move(context));
     bool had_failure = false;
 
     for (ServerClusterRegistration * registration = mRegistrations; registration != nullptr; registration = registration->next)
     {
-        CHIP_ERROR err = registration->serverClusterInterface->Startup(&mContext);
+        CHIP_ERROR err = registration->serverClusterInterface->Startup(&*mContext);
         if (err != CHIP_NO_ERROR)
         {
 #if CHIP_ERROR_LOGGING
@@ -219,7 +219,7 @@ CHIP_ERROR ServerClusterInterfaceRegistry::SetContext(const ServerClusterContext
 
 void ServerClusterInterfaceRegistry::ClearContext()
 {
-    if (!mContextIsValid)
+    if (!mContext.has_value())
     {
         return;
     }
@@ -228,8 +228,7 @@ void ServerClusterInterfaceRegistry::ClearContext()
         registration->serverClusterInterface->Shutdown();
     }
 
-    mContext        = ServerClusterContext{};
-    mContextIsValid = false;
+    mContext.reset();
 }
 
 } // namespace app
diff --git a/src/data-model-providers/codegen/ServerClusterInterfaceRegistry.h b/src/data-model-providers/codegen/ServerClusterInterfaceRegistry.h
@@ -127,7 +127,7 @@ class ServerClusterInterfaceRegistry
     // Set up the underlying context for all clusters that are managed by this registry.
     //
     // The values within context will be copied and used.
-    CHIP_ERROR SetContext(const ServerClusterContext & context);
+    CHIP_ERROR SetContext(ServerClusterContext && context);
 
     // Invalidates current context.
     void ClearContext();
@@ -140,8 +140,7 @@ class ServerClusterInterfaceRegistry
     ServerClusterInterface * mCachedInterface = nullptr;
 
     // Managing context for this registry
-    bool mContextIsValid = false;
-    ServerClusterContext mContext;
+    std::optional<ServerClusterContext> mContext;
 };
 
 } // namespace app
diff --git a/src/data-model-providers/codegen/tests/BUILD.gn b/src/data-model-providers/codegen/tests/BUILD.gn
@@ -62,5 +62,6 @@ chip_test_suite("tests") {
   public_deps = [
     ":mock_model",
     "${chip_root}/src/app/data-model-provider:string-builder-adapters",
+    "${chip_root}/src/app/server-cluster/testing",
   ]
 }
diff --git a/src/data-model-providers/codegen/tests/TestServerClusterInterfaceRegistry.cpp b/src/data-model-providers/codegen/tests/TestServerClusterInterfaceRegistry.cpp
@@ -19,6 +19,8 @@
 #include <app-common/zap-generated/ids/Attributes.h>
 #include <app/ConcreteClusterPath.h>
 #include <app/server-cluster/DefaultServerCluster.h>
+#include <app/server-cluster/ServerClusterContext.h>
+#include <app/server-cluster/testing/TestServerClusterContext.h>
 #include <data-model-providers/codegen/ServerClusterInterfaceRegistry.h>
 #include <lib/core/CHIPError.h>
 #include <lib/core/DataModelTypes.h>
@@ -29,6 +31,7 @@
 #include <random>
 
 using namespace chip;
+using namespace chip::Test;
 using namespace chip::app;
 using namespace chip::app::DataModel;
 using namespace chip::app::Clusters;
@@ -348,8 +351,8 @@ TEST_F(TestServerClusterInterfaceRegistry, Context)
         EXPECT_FALSE(cluster1.HasContext());
 
         // set up the registry
-        ServerClusterContext nullContext; // not valid, however we do not care
-        EXPECT_EQ(registry.SetContext(nullContext), CHIP_NO_ERROR);
+        TestServerClusterContext context;
+        EXPECT_EQ(registry.SetContext(context.Create()), CHIP_NO_ERROR);
 
         EXPECT_TRUE(cluster1.HasContext());
         EXPECT_FALSE(cluster2.HasContext());
@@ -365,7 +368,7 @@ TEST_F(TestServerClusterInterfaceRegistry, Context)
         EXPECT_FALSE(cluster2.HasContext());
         EXPECT_FALSE(cluster3.HasContext());
 
-        EXPECT_EQ(registry.SetContext(nullContext), CHIP_NO_ERROR);
+        EXPECT_EQ(registry.SetContext(context.Create()), CHIP_NO_ERROR);
         EXPECT_TRUE(cluster1.HasContext());
         EXPECT_TRUE(cluster2.HasContext());
         EXPECT_FALSE(cluster3.HasContext());
@@ -380,17 +383,15 @@ TEST_F(TestServerClusterInterfaceRegistry, Context)
         EXPECT_TRUE(cluster3.HasContext());
 
         // re-setting context works
-        EXPECT_EQ(registry.SetContext(nullContext), CHIP_NO_ERROR);
+        EXPECT_EQ(registry.SetContext(context.Create()), CHIP_NO_ERROR);
         EXPECT_TRUE(cluster1.HasContext());
         EXPECT_FALSE(cluster2.HasContext());
         EXPECT_TRUE(cluster3.HasContext());
 
         // also not valid, but different
-        ServerClusterContext otherContext;
-        InteractionModelContext modelContext;
-        otherContext.interactionContext = &modelContext;
+        TestServerClusterContext otherContext;
 
-        EXPECT_EQ(registry.SetContext(otherContext), CHIP_NO_ERROR);
+        EXPECT_EQ(registry.SetContext(otherContext.Create()), CHIP_NO_ERROR);
         EXPECT_TRUE(cluster1.HasContext());
         EXPECT_FALSE(cluster2.HasContext());
         EXPECT_TRUE(cluster3.HasContext());
@@ -412,7 +413,6 @@ TEST_F(TestServerClusterInterfaceRegistry, StartupErrors)
 
     {
         ServerClusterInterfaceRegistry registry;
-
         EXPECT_FALSE(cluster1.HasContext());
         EXPECT_FALSE(cluster2.HasContext());
 
@@ -423,8 +423,8 @@ TEST_F(TestServerClusterInterfaceRegistry, StartupErrors)
         EXPECT_FALSE(cluster1.HasContext());
         EXPECT_FALSE(cluster2.HasContext());
 
-        ServerClusterContext nullContext; // not valid, however we do not care
-        EXPECT_EQ(registry.SetContext(nullContext), CHIP_ERROR_HAD_FAILURES);
+        TestServerClusterContext context;
+        EXPECT_EQ(registry.SetContext(context.Create()), CHIP_ERROR_HAD_FAILURES);
         EXPECT_TRUE(cluster1.HasContext());
         EXPECT_FALSE(cluster2.HasContext());
 

Original file line number	Diff line number	Diff line change
`@@ -34,9 +34,11 @@ namespace app {`
`34`	`34`	`/// for every cluster maintaining such a context.`
`35`	`35`	`struct ServerClusterContext`
`36`	`36`	`{`
`37`		`- DataModel::Provider * provider = nullptr; /// underlying provider that the cluster operates in`
`38`		`- PersistentStorageDelegate * storage = nullptr; /// read/write persistent storage`
`39`		`- DataModel::InteractionModelContext * interactionContext = nullptr; /// outside-world communication`
	`37`	`+ DataModel::Provider * const provider = nullptr; /// underlying provider that the cluster operates in`
	`38`	`+ PersistentStorageDelegate * const storage = nullptr; /// read/write persistent storage`
	`39`	`+ DataModel::InteractionModelContext * const interactionContext = nullptr; /// outside-world communication`
	`40`	`+`
	`41`	`+ ServerClusterContext(ServerClusterContext &&) = default;`
`40`	`42`
`41`	`43`	`bool operator!=(const ServerClusterContext & other) const`
`42`	`44`	`{`
Original file line number	Diff line number	Diff line change
`@@ -22,6 +22,7 @@`
`22`	`22`	`#include <lib/core/DataModelTypes.h>`
`23`	`23`	`#include <lib/support/CHIPMem.h>`
`24`	`24`	`#include <lib/support/CodeUtils.h>`
	`25`	`+#include <optional>`
`25`	`26`
`26`	`27`	`namespace chip {`
`27`	`28`	`namespace app {`
`@@ -31,7 +32,7 @@ ServerClusterInterfaceRegistry::~ServerClusterInterfaceRegistry()`
`31`	`32`	`while (mRegistrations != nullptr)`
`32`	`33`	`{`
`33`	`34`	`ServerClusterRegistration * next = mRegistrations->next;`
`34`		`- if (mContextIsValid)`
	`35`	`+ if (mContext.has_value())`
`35`	`36`	`{`
`36`	`37`	`mRegistrations->serverClusterInterface->Shutdown();`
`37`	`38`	`}`
`@@ -55,9 +56,9 @@ CHIP_ERROR ServerClusterInterfaceRegistry::Register(ServerClusterRegistration &`
`55`	`56`	`// items. We preserve this however we may want to make this optional at some point in time.`
`56`	`57`	`VerifyOrReturnError(Get(path) == nullptr, CHIP_ERROR_DUPLICATE_KEY_ID);`
`57`	`58`
`58`		`- if (mContextIsValid)`
	`59`	`+ if (mContext.has_value())`
`59`	`60`	`{`
`60`		`- ReturnErrorOnFailure(entry.serverClusterInterface->Startup(&mContext));`
	`61`	`+ ReturnErrorOnFailure(entry.serverClusterInterface->Startup(&*mContext));`
`61`	`62`	`}`
`62`	`63`
`63`	`64`	`entry.next = mRegistrations;`
`@@ -93,7 +94,7 @@ ServerClusterInterface * ServerClusterInterfaceRegistry::Unregister(const Concre`
`93`	`94`	`}`
`94`	`95`
`95`	`96`	`current->next = nullptr; // Make sure current does not look like part of a list.`
`96`		`- if (mContextIsValid)`
	`97`	`+ if (mContext.has_value())`
`97`	`98`	`{`
`98`	`99`	`current->serverClusterInterface->Shutdown();`
`99`	`100`	`}`
`@@ -137,7 +138,7 @@ void ServerClusterInterfaceRegistry::UnregisterAllFromEndpoint(EndpointId endpoi`
`137`	`138`	`ServerClusterRegistration * actual_next = current->next;`
`138`	`139`
`139`	`140`	`current->next = nullptr; // Make sure current does not look like part of a list.`
`140`		`- if (mContextIsValid)`
	`141`	`+ if (mContext.has_value())`
`141`	`142`	`{`
`142`	`143`	`current->serverClusterInterface->Shutdown();`
`143`	`144`	`}`
`@@ -178,22 +179,21 @@ ServerClusterInterface * ServerClusterInterfaceRegistry::Get(const ConcreteClust`
`178`	`179`	`return nullptr;`
`179`	`180`	`}`
`180`	`181`
`181`		`-CHIP_ERROR ServerClusterInterfaceRegistry::SetContext(const ServerClusterContext & context)`
	`182`	`+CHIP_ERROR ServerClusterInterfaceRegistry::SetContext(ServerClusterContext && context)`
`182`	`183`	`{`
`183`		`- if (mContextIsValid)`
	`184`	`+ if (mContext.has_value())`
`184`	`185`	`{`
`185`	`186`	`// if there is no difference, do not re-initialize.`
`186`		`- VerifyOrReturnError(mContext != context, CHIP_NO_ERROR);`
	`187`	`+ VerifyOrReturnError(*mContext != context, CHIP_NO_ERROR);`
`187`	`188`	`ClearContext();`
`188`	`189`	`}`
`189`	`190`
`190`		`- mContext = context;`
`191`		`- mContextIsValid = true;`
	`191`	`+ mContext.emplace(std::move(context));`
`192`	`192`	`bool had_failure = false;`
`193`	`193`
`194`	`194`	`for (ServerClusterRegistration * registration = mRegistrations; registration != nullptr; registration = registration->next)`
`195`	`195`	`{`
`196`		`- CHIP_ERROR err = registration->serverClusterInterface->Startup(&mContext);`
	`196`	`+ CHIP_ERROR err = registration->serverClusterInterface->Startup(&*mContext);`
`197`	`197`	`if (err != CHIP_NO_ERROR)`
`198`	`198`	`{`
`199`	`199`	`#if CHIP_ERROR_LOGGING`
`@@ -219,7 +219,7 @@ CHIP_ERROR ServerClusterInterfaceRegistry::SetContext(const ServerClusterContext`
`219`	`219`
`220`	`220`	`void ServerClusterInterfaceRegistry::ClearContext()`
`221`	`221`	`{`
`222`		`- if (!mContextIsValid)`
	`222`	`+ if (!mContext.has_value())`
`223`	`223`	`{`
`224`	`224`	`return;`
`225`	`225`	`}`
`@@ -228,8 +228,7 @@ void ServerClusterInterfaceRegistry::ClearContext()`
`228`	`228`	`registration->serverClusterInterface->Shutdown();`
`229`	`229`	`}`
`230`	`230`
`231`		`- mContext = ServerClusterContext{};`
`232`		`- mContextIsValid = false;`
	`231`	`+ mContext.reset();`
`233`	`232`	`}`
`234`	`233`
`235`	`234`	`} // namespace app`
Original file line number	Diff line number	Diff line change
`@@ -62,5 +62,6 @@ chip_test_suite("tests") {`
`62`	`62`	`public_deps = [`
`63`	`63`	`":mock_model",`
`64`	`64`	`"${chip_root}/src/app/data-model-provider:string-builder-adapters",`
	`65`	`+ "${chip_root}/src/app/server-cluster/testing",`
`65`	`66`	`]`
`66`	`67`	`}`