Reverted the AttributePersistenceProvider and added a SafeAttributePe…

…rsistenceProvider (#28302)

* Renamed AttributePersistenceProvider to SafeAttributePersistenceProvider. Reverted to the previous AttributePersistenceProvider. Updated the tests and examples that used the AttributePersistenceProvider.

* Restyled by whitespace

* Restyled by clang-format

* Restyled by gn

* Fixed the key generated for safe attributes.

Co-authored-by: Tennessee Carmel-Veilleux <tennessee.carmelveilleux@gmail.com>

* Apply documentation suggestions from code review

Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>

* Removed unused aPath and import.

* Fixed some docs. Removed size from the SafeAttributePersistenceProvider::SafeReadValue method.

* Replaced the aType input var from the SafeAttributePersistenceProvider::SafeReadValue method with documentation on the expectet types.

* Replaced the SafeAttributePersistenceProvider::GetNullValueForNullableType logic with that used in NumericAttributeTraits::GetNullValue. Mode the GetNullValueForNullableType methods private and commented out the relevant unit tests.

* Restyled by clang-format

* Replaced the SafeAttributePersistenceProvider::GetNullValueForNullableType methods with NumericAttributeTraits.

* Apply documentation suggestions from code review

Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>

* Restyled by clang-format

* Refactored SafeAttributePersistenceProvider reducing the number of templated functions.

Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>

* Fixed minor bug in suggestions.

* Restyled by whitespace

---------

Co-authored-by: Restyled.io <commits@restyled.io>
Co-authored-by: Tennessee Carmel-Veilleux <tennessee.carmelveilleux@gmail.com>
Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>

src/app/AttributePersistenceProvider.h

@@ -16,19 +16,17 @@
 #pragma once
 
 #include <app/ConcreteAttributePath.h>
-#include <app/data-model/Nullable.h>
 #include <app/util/attribute-metadata.h>
-#include <cstring>
-#include <inttypes.h>
-#include <lib/support/BufferReader.h>
-#include <lib/support/BufferWriter.h>
 #include <lib/support/Span.h>
 
 namespace chip {
 namespace app {
 
 /**
- * Interface for persisting attribute values.
+ * Interface for persisting attribute values. This will write attributes in storage with platform endianness for scalars
+ * and uses a different key space from SafeAttributePersistenceProvider.
+ * When storing cluster attributes that are managed via the AttributeAccessInterface, it is recommended to
+ * use SafeAttributePersistenceProvider.
  */
 
 class AttributePersistenceProvider
@@ -61,171 +59,17 @@ class AttributePersistenceProvider
      * Read an attribute value from non-volatile memory.
      *
      * @param [in]     aPath the attribute path for the data being persisted.
-     * @param [in]     aType the attribute type.
-     * @param [in]     aSize the attribute size.
+     * @param [in]     aMetadata the attribute metadata, as a convenience.
      * @param [in,out] aValue where to place the data.  The size of the buffer
-     *                 will be equal to `size`.
+     *                 will be equal to `size` member of aMetadata.
      *
      *                 The data is expected to be in native endianness for
      *                 integers and floats.  For strings, see the string
      *                 representation description in the WriteValue
      *                 documentation.
      */
-    virtual CHIP_ERROR ReadValue(const ConcreteAttributePath & aPath, EmberAfAttributeType aType, size_t aSize,
+    virtual CHIP_ERROR ReadValue(const ConcreteAttributePath & aPath, const EmberAfAttributeMetadata * aMetadata,
                                  MutableByteSpan & aValue) = 0;
-
-    /**
-     * Get the KVS representation of null for the given type.
-     * @tparam T The type for which the null representation should be returned.
-     * @return A value of type T that in the KVS represents null.
-     */
-    template <typename T, std::enable_if_t<std::is_same<bool, T>::value, bool> = true>
-    static uint8_t GetNullValueForNullableType()
-    {
-        return 0xff;
-    }
-
-    /**
-     * Get the KVS representation of null for the given type.
-     * @tparam T The type for which the null representation should be returned.
-     * @return A value of type T that in the KVS represents null.
-     */
-    template <typename T, std::enable_if_t<std::is_unsigned<T>::value && !std::is_same<bool, T>::value, bool> = true>
-    static T GetNullValueForNullableType()
-    {
-        T nullValue = 0;
-        nullValue   = T(~nullValue);
-        return nullValue;
-    }
-
-    /**
-     * Get the KVS representation of null for the given type.
-     * @tparam T The type for which the null representation should be returned.
-     * @return A value of type T that in the KVS represents null.
-     */
-    template <typename T, std::enable_if_t<std::is_signed<T>::value && !std::is_same<bool, T>::value, bool> = true>
-    static T GetNullValueForNullableType()
-    {
-        T shiftBit = 1;
-        return T(shiftBit << ((sizeof(T) * 8) - 1));
-    }
-
-    // The following API provides helper functions to simplify the access of commonly used types.
-    // The API may not be complete.
-    // Currently implemented write and read types are: uint8_t, uint16_t, uint32_t, unit64_t and
-    // their nullable varieties, and bool.
-
-    /**
-     * Write an attribute value of type intX, uintX or bool to non-volatile memory.
-     *
-     * @param [in] aPath the attribute path for the data being written.
-     * @param [in] aValue the data to write.
-     */
-    template <typename T, std::enable_if_t<std::is_integral<T>::value, bool> = true>
-    CHIP_ERROR WriteScalarValue(const ConcreteAttributePath & aPath, T & aValue)
-    {
-        uint8_t value[sizeof(T)];
-        auto w = Encoding::LittleEndian::BufferWriter(value, sizeof(T));
-        w.EndianPut(uint64_t(aValue), sizeof(T));
-
-        return WriteValue(aPath, ByteSpan(value));
-    }
-
-    /**
-     * Read an attribute of type intX, uintX or bool from non-volatile memory.
-     *
-     * @param [in]     aPath the attribute path for the data being persisted.
-     * @param [in,out] aValue where to place the data.
-     */
-    template <typename T, std::enable_if_t<std::is_integral<T>::value, bool> = true>
-    CHIP_ERROR ReadScalarValue(const ConcreteAttributePath & aPath, T & aValue)
-    {
-        uint8_t attrData[sizeof(T)];
-        MutableByteSpan tempVal(attrData);
-        // **Note** aType in the ReadValue function is only used to check if the value is of a string type. Since this template
-        // function is only enabled for integral values, we know that this case will not occur, so we can pass the enum of an
-        // arbitrary integral type. 0x20 is the ZCL enum type for ZCL_INT8U_ATTRIBUTE_TYPE.
-        auto err = ReadValue(aPath, 0x20, sizeof(T), tempVal);
-        if (err != CHIP_NO_ERROR)
-        {
-            return err;
-        }
-
-        chip::Encoding::LittleEndian::Reader r(tempVal.data(), tempVal.size());
-        r.RawReadLowLevelBeCareful(&aValue);
-        return r.StatusCode();
-    }
-
-    /**
-     * Write an attribute value of type nullable intX, uintX or bool to non-volatile memory.
-     *
-     * @param [in] aPath the attribute path for the data being written.
-     * @param [in] aValue the data to write.
-     */
-    template <typename T, std::enable_if_t<std::is_integral<T>::value, bool> = true>
-    CHIP_ERROR WriteScalarValue(const ConcreteAttributePath & aPath, DataModel::Nullable<T> & aValue)
-    {
-        if (aValue.IsNull())
-        {
-            auto nullVal = GetNullValueForNullableType<T>();
-            return WriteScalarValue(aPath, nullVal);
-        }
-        return WriteScalarValue(aPath, aValue.Value());
-    }
-
-    /**
-     * Read an attribute of type nullable intX, uintX from non-volatile memory.
-     *
-     * @param [in]     aPath the attribute path for the data being persisted.
-     * @param [in,out] aValue where to place the data.
-     */
-    template <typename T, std::enable_if_t<std::is_integral<T>::value && !std::is_same<bool, T>::value, bool> = true>
-    CHIP_ERROR ReadScalarValue(const ConcreteAttributePath & aPath, DataModel::Nullable<T> & aValue)
-    {
-        T tempIntegral;
-
-        CHIP_ERROR err = ReadScalarValue(aPath, tempIntegral);
-        if (err != CHIP_NO_ERROR)
-        {
-            return err;
-        }
-
-        if (tempIntegral == GetNullValueForNullableType<T>())
-        {
-            aValue.SetNull();
-            return CHIP_NO_ERROR;
-        }
-
-        aValue.SetNonNull(tempIntegral);
-        return CHIP_NO_ERROR;
-    }
-
-    /**
-     * Read an attribute of type nullable bool from non-volatile memory.
-     *
-     * @param [in]     aPath the attribute path for the data being persisted.
-     * @param [in,out] aValue where to place the data.
-     */
-    template <typename T, std::enable_if_t<std::is_same<bool, T>::value, bool> = true>
-    CHIP_ERROR ReadScalarValue(const ConcreteAttributePath & aPath, DataModel::Nullable<T> & aValue)
-    {
-        uint8_t tempIntegral;
-
-        CHIP_ERROR err = ReadScalarValue(aPath, tempIntegral);
-        if (err != CHIP_NO_ERROR)
-        {
-            return err;
-        }
-
-        if (tempIntegral == GetNullValueForNullableType<T>())
-        {
-            aValue.SetNull();
-            return CHIP_NO_ERROR;
-        }
-
-        aValue.SetNonNull(tempIntegral);
-        return CHIP_NO_ERROR;
-    }
 };
 
 /**

src/app/BUILD.gn

@@ -183,6 +183,7 @@ static_library("app") {
     "ReadHandler.cpp",
     "RequiredPrivilege.cpp",
     "RequiredPrivilege.h",
+    "SafeAttributePersistenceProvider.h",
     "StatusResponse.cpp",
     "StatusResponse.h",
     "SubscriptionResumptionStorage.h",

src/app/DefaultAttributePersistenceProvider.cpp

@@ -22,7 +22,7 @@
 namespace chip {
 namespace app {
 
-CHIP_ERROR DefaultAttributePersistenceProvider::WriteValue(const ConcreteAttributePath & aPath, const ByteSpan & aValue)
+CHIP_ERROR DefaultAttributePersistenceProvider::InternalWriteValue(const StorageKeyName & aKey, const ByteSpan & aValue)
 {
     VerifyOrReturnError(mStorage != nullptr, CHIP_ERROR_INCORRECT_STATE);
 
@@ -33,20 +33,16 @@ CHIP_ERROR DefaultAttributePersistenceProvider::WriteValue(const ConcreteAttribu
     {
         return CHIP_ERROR_BUFFER_TOO_SMALL;
     }
-    return mStorage->SyncSetKeyValue(
-        DefaultStorageKeyAllocator::AttributeValue(aPath.mEndpointId, aPath.mClusterId, aPath.mAttributeId).KeyName(),
-        aValue.data(), static_cast<uint16_t>(aValue.size()));
+    return mStorage->SyncSetKeyValue(aKey.KeyName(), aValue.data(), static_cast<uint16_t>(aValue.size()));
 }
 
-CHIP_ERROR DefaultAttributePersistenceProvider::ReadValue(const ConcreteAttributePath & aPath, EmberAfAttributeType aType,
-                                                          size_t aSize, MutableByteSpan & aValue)
+CHIP_ERROR DefaultAttributePersistenceProvider::InternalReadValue(const StorageKeyName & aKey, EmberAfAttributeType aType,
+                                                                  size_t aSize, MutableByteSpan & aValue)
 {
     VerifyOrReturnError(mStorage != nullptr, CHIP_ERROR_INCORRECT_STATE);
 
     uint16_t size = static_cast<uint16_t>(min(aValue.size(), static_cast<size_t>(UINT16_MAX)));
-    ReturnErrorOnFailure(mStorage->SyncGetKeyValue(
-        DefaultStorageKeyAllocator::AttributeValue(aPath.mEndpointId, aPath.mClusterId, aPath.mAttributeId).KeyName(),
-        aValue.data(), size));
+    ReturnErrorOnFailure(mStorage->SyncGetKeyValue(aKey.KeyName(), aValue.data(), size));
     EmberAfAttributeType type = aType;
     if (emberAfIsStringAttributeType(type))
     {
@@ -71,12 +67,45 @@ CHIP_ERROR DefaultAttributePersistenceProvider::ReadValue(const ConcreteAttribut
     return CHIP_NO_ERROR;
 }
 
+CHIP_ERROR DefaultAttributePersistenceProvider::WriteValue(const ConcreteAttributePath & aPath, const ByteSpan & aValue)
+{
+    return InternalWriteValue(DefaultStorageKeyAllocator::AttributeValue(aPath.mEndpointId, aPath.mClusterId, aPath.mAttributeId),
+                              aValue);
+}
+
+CHIP_ERROR DefaultAttributePersistenceProvider::ReadValue(const ConcreteAttributePath & aPath,
+                                                          const EmberAfAttributeMetadata * aMetadata, MutableByteSpan & aValue)
+{
+    return InternalReadValue(DefaultStorageKeyAllocator::AttributeValue(aPath.mEndpointId, aPath.mClusterId, aPath.mAttributeId),
+                             aMetadata->attributeType, aMetadata->size, aValue);
+}
+
+CHIP_ERROR DefaultAttributePersistenceProvider::SafeWriteValue(const ConcreteAttributePath & aPath, const ByteSpan & aValue)
+{
+    return InternalWriteValue(
+        DefaultStorageKeyAllocator::SafeAttributeValue(aPath.mEndpointId, aPath.mClusterId, aPath.mAttributeId), aValue);
+}
+
+CHIP_ERROR DefaultAttributePersistenceProvider::SafeReadValue(const ConcreteAttributePath & aPath, MutableByteSpan & aValue)
+{
+    return InternalReadValue(
+        DefaultStorageKeyAllocator::SafeAttributeValue(aPath.mEndpointId, aPath.mClusterId, aPath.mAttributeId), 0x20,
+        aValue.size(), aValue);
+}
+
 namespace {
 
 AttributePersistenceProvider * gAttributeSaver = nullptr;
 
 } // anonymous namespace
 
+/**
+ * Gets the global attribute saver.
+ *
+ * Note: When storing cluster attributes that are managed via AttributeAccessInterface, it is recommended to
+ * use SafeAttributePersistenceProvider. See AttributePersistenceProvider and SafeAttributePersistenceProvider
+ * class documentation for more information.
+ */
 AttributePersistenceProvider * GetAttributePersistenceProvider()
 {
     return gAttributeSaver;
@@ -90,5 +119,27 @@ void SetAttributePersistenceProvider(AttributePersistenceProvider * aProvider)
     }
 }
 
+namespace {
+
+SafeAttributePersistenceProvider * gSafeAttributeSaver = nullptr;
+
+} // anonymous namespace
+
+/**
+ * Gets the global attribute safe saver.
+ */
+SafeAttributePersistenceProvider * GetSafeAttributePersistenceProvider()
+{
+    return gSafeAttributeSaver;
+}
+
+void SetSafeAttributePersistenceProvider(SafeAttributePersistenceProvider * aProvider)
+{
+    if (aProvider != nullptr)
+    {
+        gSafeAttributeSaver = aProvider;
+    }
+}
+
 } // namespace app
 } // namespace chip

src/app/DefaultAttributePersistenceProvider.h

@@ -16,6 +16,7 @@
 #pragma once
 
 #include <app/AttributePersistenceProvider.h>
+#include <app/SafeAttributePersistenceProvider.h>
 #include <lib/core/CHIPPersistentStorageDelegate.h>
 #include <lib/support/DefaultStorageKeyAllocator.h>
 
@@ -30,7 +31,7 @@ namespace app {
  * of this class, since it can't be constructed automatically without knowing
  * what PersistentStorageDelegate is to be used.
  */
-class DefaultAttributePersistenceProvider : public AttributePersistenceProvider
+class DefaultAttributePersistenceProvider : public AttributePersistenceProvider, public SafeAttributePersistenceProvider
 {
 public:
     DefaultAttributePersistenceProvider() {}
@@ -50,11 +51,19 @@ class DefaultAttributePersistenceProvider : public AttributePersistenceProvider
 
     // AttributePersistenceProvider implementation.
     CHIP_ERROR WriteValue(const ConcreteAttributePath & aPath, const ByteSpan & aValue) override;
-    CHIP_ERROR ReadValue(const ConcreteAttributePath & aPath, EmberAfAttributeType aType, size_t aSize,
+    CHIP_ERROR ReadValue(const ConcreteAttributePath & aPath, const EmberAfAttributeMetadata * aMetadata,
                          MutableByteSpan & aValue) override;
 
+    // SafeAttributePersistenceProvider implementation.
+    CHIP_ERROR SafeWriteValue(const ConcreteAttributePath & aPath, const ByteSpan & aValue) override;
+    CHIP_ERROR SafeReadValue(const ConcreteAttributePath & aPath, MutableByteSpan & aValue) override;
+
 protected:
     PersistentStorageDelegate * mStorage;
+
+private:
+    CHIP_ERROR InternalWriteValue(const StorageKeyName & aKey, const ByteSpan & aValue);
+    CHIP_ERROR InternalReadValue(const StorageKeyName & aKey, EmberAfAttributeType aType, size_t aSize, MutableByteSpan & aValue);
 };
 
 } // namespace app

src/app/DeferredAttributePersistenceProvider.cpp

@@ -57,10 +57,10 @@ CHIP_ERROR DeferredAttributePersistenceProvider::WriteValue(const ConcreteAttrib
     return mPersister.WriteValue(aPath, aValue);
 }
 
-CHIP_ERROR DeferredAttributePersistenceProvider::ReadValue(const ConcreteAttributePath & aPath, EmberAfAttributeType aType,
-                                                           size_t aSize, MutableByteSpan & aValue)
+CHIP_ERROR DeferredAttributePersistenceProvider::ReadValue(const ConcreteAttributePath & aPath,
+                                                           const EmberAfAttributeMetadata * aMetadata, MutableByteSpan & aValue)
 {
-    return mPersister.ReadValue(aPath, aType, aSize, aValue);
+    return mPersister.ReadValue(aPath, aMetadata, aValue);
 }
 
 void DeferredAttributePersistenceProvider::FlushAndScheduleNext()

src/app/DeferredAttributePersistenceProvider.h

@@ -67,7 +67,7 @@ class DeferredAttributePersistenceProvider : public AttributePersistenceProvider
      * For other attributes, immediately pass the write operation to the decorated persister.
      */
     CHIP_ERROR WriteValue(const ConcreteAttributePath & aPath, const ByteSpan & aValue) override;
-    CHIP_ERROR ReadValue(const ConcreteAttributePath & aPath, EmberAfAttributeType aType, size_t aSize,
+    CHIP_ERROR ReadValue(const ConcreteAttributePath & aPath, const EmberAfAttributeMetadata * aMetadata,
                          MutableByteSpan & aValue) override;
 
 private: