Remove AttributeList from Device Layer (project-chip#41546)

* Remove AttributeList from Device Layer

It was only used in a single place and probably didn't belong in the device layer
in the first place.  Use a Span instead for DeviceInforProvider::SetUserLabelList.

Also simplify DeviceInforProvider::ClearUserLabelList().

* Add missing const to Span type

* Move max size constants into the header and add more tests

* Allow the global DeviceInfoProvider to be reset to null

This is primarily useful for testing. Also remove the comment stating that
callers should assume the provider is not null; it starts out as null, and the
cluster code interacting with it already handles null gracefully.

---------

Co-authored-by: Andrei Litvin <[email protected]>

Author: ksperling-apple

src/BUILD.gn

@@ -84,7 +84,6 @@ if (chip_build_tests) {
       "${chip_root}/src/crypto/tests",
       "${chip_root}/src/data-model-providers/codedriven/endpoint/tests",
       "${chip_root}/src/inet/tests",
-      "${chip_root}/src/include/platform/tests",
       "${chip_root}/src/lib/address_resolve/tests",
       "${chip_root}/src/app/server-cluster/tests",
       "${chip_root}/src/lib/asn1/tests",

src/app/clusters/user-label-server/tests/TestUserLabelCluster.cpp

@@ -25,6 +25,7 @@
 #include <clusters/UserLabel/Enums.h>
 #include <clusters/UserLabel/Metadata.h>
 #include <clusters/UserLabel/Structs.h>
+#include <platform/DeviceInfoProvider.h>
 
 namespace {
 
@@ -35,21 +36,55 @@ using namespace chip::app::Clusters::UserLabel;
 using namespace chip::app::Clusters::UserLabel::Attributes;
 using namespace chip::Test;
 
+// Mock DeviceInfoProvider for testing
+class MockDeviceInfoProvider : public DeviceLayer::DeviceInfoProvider
+{
+public:
+    MockDeviceInfoProvider()           = default;
+    ~MockDeviceInfoProvider() override = default;
+
+    FixedLabelIterator * IterateFixedLabel(EndpointId endpoint) override { return nullptr; }
+    UserLabelIterator * IterateUserLabel(EndpointId endpoint) override { return nullptr; }
+    SupportedCalendarTypesIterator * IterateSupportedCalendarTypes() override { return nullptr; }
+    SupportedLocalesIterator * IterateSupportedLocales() override { return nullptr; }
+
+protected:
+    // Simple no-op implementations - we only need these to return success
+    // so that the cluster's validation logic can be tested
+    CHIP_ERROR SetUserLabelLength(EndpointId endpoint, size_t val) override { return CHIP_NO_ERROR; }
+    CHIP_ERROR GetUserLabelLength(EndpointId endpoint, size_t & val) override
+    {
+        val = 0;
+        return CHIP_NO_ERROR;
+    }
+    CHIP_ERROR SetUserLabelAt(EndpointId endpoint, size_t index, const UserLabelType & userLabel) override { return CHIP_NO_ERROR; }
+    CHIP_ERROR DeleteUserLabelAt(EndpointId endpoint, size_t index) override { return CHIP_NO_ERROR; }
+};
+
 struct TestUserLabelCluster : public ::testing::Test
 {
     static void SetUpTestSuite() { ASSERT_EQ(chip::Platform::MemoryInit(), CHIP_NO_ERROR); }
 
     static void TearDownTestSuite() { chip::Platform::MemoryShutdown(); }
 
-    void SetUp() override { ASSERT_EQ(userLabel.Startup(context), CHIP_NO_ERROR); }
+    void SetUp() override
+    {
+        DeviceLayer::SetDeviceInfoProvider(&mDeviceInfoProvider);
+        ASSERT_EQ(userLabel.Startup(context), CHIP_NO_ERROR);
+    }
 
-    void TearDown() override { userLabel.Shutdown(); }
+    void TearDown() override
+    {
+        userLabel.Shutdown();
+        DeviceLayer::SetDeviceInfoProvider(nullptr);
+    }
 
     TestUserLabelCluster() : context(testContext.Create()), userLabel(kRootEndpointId) {}
 
     chip::Test::TestServerClusterContext testContext;
     ServerClusterContext context;
     UserLabelCluster userLabel;
+    MockDeviceInfoProvider mDeviceInfoProvider;
 };
 
 } // namespace
@@ -83,3 +118,56 @@ TEST_F(TestUserLabelCluster, ReadAttributeTest)
         ASSERT_GT(it.GetValue().label.size(), 0u);
     }
 }
+
+TEST_F(TestUserLabelCluster, WriteValidLabelListTest)
+{
+    ClusterTester tester(userLabel);
+    Structs::LabelStruct::Type labels[] = {
+        { .label = "room"_span, .value = "bedroom 2"_span },
+        { .label = "orientation"_span, .value = "North"_span },
+    };
+    ASSERT_EQ(tester.WriteAttribute(LabelList::Id, DataModel::List(labels)), CHIP_NO_ERROR);
+}
+
+TEST_F(TestUserLabelCluster, WriteLabelWithLabelTooLongTest)
+{
+    ClusterTester tester(userLabel);
+    constexpr auto tooLongLabel = "this_label_is_way_too_long"_span;
+    static_assert(tooLongLabel.size() > UserLabelCluster::kMaxLabelSize);
+    Structs::LabelStruct::Type labels[] = {
+        { .label = tooLongLabel, .value = "value"_span },
+    };
+    ASSERT_EQ(tester.WriteAttribute(LabelList::Id, DataModel::List(labels)), CHIP_IM_GLOBAL_STATUS(ConstraintError));
+}
+
+TEST_F(TestUserLabelCluster, WriteLabelWithValueTooLongTest)
+{
+    ClusterTester tester(userLabel);
+    constexpr auto tooLongValue = "this_value_is_way_too_long"_span;
+    static_assert(tooLongValue.size() > UserLabelCluster::kMaxValueSize);
+    Structs::LabelStruct::Type labels[] = {
+        { .label = "room"_span, .value = tooLongValue },
+    };
+    ASSERT_EQ(tester.WriteAttribute(LabelList::Id, DataModel::List(labels)), CHIP_IM_GLOBAL_STATUS(ConstraintError));
+}
+
+TEST_F(TestUserLabelCluster, WriteEmptyLabelsTest)
+{
+    ClusterTester tester(userLabel);
+    Structs::LabelStruct::Type labels[] = {
+        { .label = ""_span, .value = ""_span }, // empty label and value are allowed per spec
+    };
+    ASSERT_EQ(tester.WriteAttribute(LabelList::Id, DataModel::List(labels)), CHIP_NO_ERROR);
+}
+
+TEST_F(TestUserLabelCluster, WriteMaxSizeLabelListTest)
+{
+    ClusterTester tester(userLabel);
+    std::array<Structs::LabelStruct::Type, chip::DeviceLayer::kMaxUserLabelListLength> labels;
+    for (size_t i = 0; i < labels.size(); i++)
+    {
+        labels[i].label = "label"_span;
+        labels[i].value = "value"_span;
+    }
+    ASSERT_EQ(tester.WriteAttribute(LabelList::Id, DataModel::List(labels.data(), labels.size())), CHIP_NO_ERROR);
+}

src/app/clusters/user-label-server/user-label-cluster.cpp

@@ -19,6 +19,8 @@
 #include <app/server/Server.h>
 #include <clusters/UserLabel/Metadata.h>
 
+#include <array>
+
 namespace chip::app::Clusters {
 
 using namespace UserLabel;
@@ -66,24 +68,8 @@ CHIP_ERROR ReadLabelList(EndpointId endpoint, AttributeValueEncoder & encoder)
 /// Matches constraints on a LabelStruct.
 bool IsValidLabelEntry(const Structs::LabelStruct::Type & entry)
 {
-    constexpr size_t kMaxLabelSize = 16;
-    constexpr size_t kMaxValueSize = 16;
-
     // NOTE: spec default for label and value is empty, so empty is accepted here
-    return (entry.label.size() <= kMaxLabelSize) && (entry.value.size() <= kMaxValueSize);
-}
-
-bool IsValidLabelEntryList(const LabelList::TypeInfo::DecodableType & list)
-{
-    auto iter = list.begin();
-    while (iter.Next())
-    {
-        if (!IsValidLabelEntry(iter.GetValue()))
-        {
-            return false;
-        }
-    }
-    return true;
+    return (entry.label.size() <= UserLabelCluster::kMaxLabelSize) && (entry.value.size() <= UserLabelCluster::kMaxValueSize);
 }
 
 CHIP_ERROR WriteLabelList(const ConcreteDataAttributePath & path, AttributeValueDecoder & decoder)
@@ -96,20 +82,22 @@ CHIP_ERROR WriteLabelList(const ConcreteDataAttributePath & path, AttributeValue
 
     if (!path.IsListItemOperation())
     {
-        DeviceLayer::AttributeList<Structs::LabelStruct::Type, DeviceLayer::kMaxUserLabelListLength> labelList;
+        size_t numLabels = 0;
+        std::array<Structs::LabelStruct::Type, DeviceLayer::kMaxUserLabelListLength> labels;
         LabelList::TypeInfo::DecodableType decodablelist;
 
         ReturnErrorOnFailure(decoder.Decode(decodablelist));
-        VerifyOrReturnError(IsValidLabelEntryList(decodablelist), CHIP_IM_GLOBAL_STATUS(ConstraintError));
-
         auto iter = decodablelist.begin();
         while (iter.Next())
         {
-            ReturnErrorOnFailure(labelList.add(iter.GetValue()));
+            auto & label = iter.GetValue();
+            VerifyOrReturnError(IsValidLabelEntry(label), CHIP_IM_GLOBAL_STATUS(ConstraintError));
+            VerifyOrReturnError(numLabels < labels.size(), CHIP_ERROR_NO_MEMORY);
+            labels[numLabels++] = label;
         }
         ReturnErrorOnFailure(iter.GetStatus());
 
-        return provider->SetUserLabelList(endpoint, labelList);
+        return provider->SetUserLabelList(endpoint, Span(labels.data(), numLabels));
     }
 
     if (path.mListOp == ConcreteDataAttributePath::ListOperation::AppendItem)

src/app/clusters/user-label-server/user-label-cluster.h

@@ -37,6 +37,10 @@ class UserLabelCluster : public DefaultServerCluster, public chip::FabricTable::
     DataModel::ActionReturnStatus WriteAttribute(const DataModel::WriteAttributeRequest & request,
                                                  AttributeValueDecoder & decoder) override;
     CHIP_ERROR Attributes(const ConcreteClusterPath & path, ReadOnlyBufferBuilder<DataModel::AttributeEntry> & builder) override;
+
+    // Constraints for label entries
+    static constexpr size_t kMaxLabelSize = 16;
+    static constexpr size_t kMaxValueSize = 16;
 };
 
 } // namespace chip::app::Clusters

src/include/platform/AttributeList.h

@@ -24,7 +24,7 @@
 #include <app/util/basic-types.h>
 #include <lib/core/CHIPError.h>
 #include <lib/core/CHIPPersistentStorageDelegate.h>
-#include <platform/AttributeList.h>
+#include <lib/support/Span.h>
 
 namespace chip {
 namespace DeviceLayer {
@@ -101,7 +101,7 @@ class DeviceInfoProvider
      * @return CHIP_NO_ERROR on success.
      * @return CHIP_ERROR if an error occurs.
      */
-    CHIP_ERROR SetUserLabelList(EndpointId endpoint, const AttributeList<UserLabelType, kMaxUserLabelListLength> & labelList);
+    CHIP_ERROR SetUserLabelList(EndpointId endpoint, Span<const UserLabelType> labelList);
 
     /**
      * @brief Clears the user label list for a specified endpoint.
@@ -206,7 +206,7 @@ class DeviceInfoProvider
  *
  * Callers have to externally synchronize usage of this function.
  *
- * @return The global Device Info Provider. Assume never null.
+ * @return The global Device Info Provider.
  */
 DeviceInfoProvider * GetDeviceInfoProvider();
 
@@ -215,8 +215,6 @@ DeviceInfoProvider * GetDeviceInfoProvider();
  *
  * Callers have to externally synchronize usage of this function.
  *
- * If the `provider` is nullptr, no change is done.
- *
  * @param[in] provider the Device Info Provider
  */
 void SetDeviceInfoProvider(DeviceInfoProvider * provider);

src/include/platform/DeviceInfoProvider.h

@@ -23,7 +23,6 @@
 
 #pragma once
 
-#include <platform/AttributeList.h>
 #include <platform/CHIPDeviceConfig.h>
 #include <platform/CHIPDeviceEvent.h>
 #include <system/PlatformEventSupport.h>