more updates to cleanup code. I am a bit concerned about O(n^2) attribute access...

andreilitvin · andreilitvin · commit 63dd9a93a0ef · 2025-01-14T13:33:49.000-05:00
diff --git a/src/app/AttributePathExpandIterator.cpp b/src/app/AttributePathExpandIterator.cpp
@@ -14,6 +14,7 @@
  *    See the License for the specific language governing permissions and
  *    limitations under the License.
  */
+#include "app/data-model-provider/MetadataSearch.h"
 #include <app/AttributePathExpandIterator.h>
 
 #include <app/GlobalAttributes.h>
@@ -119,34 +120,58 @@ bool AttributePathExpandIterator::IsValidAttributeId(AttributeId attributeId)
         break;
     }
 
+    DataModel::AttributeFinder finder(mDataModelProvider);
+
     const ConcreteAttributePath attributePath(mPosition.mOutputPath.mEndpointId, mPosition.mOutputPath.mClusterId, attributeId);
-    return mDataModelProvider->GetAttributeInfo(attributePath).has_value();
+    return finder.Find(attributePath).has_value();
 }
 
 std::optional<AttributeId> AttributePathExpandIterator::NextAttributeId()
 {
-    if (mPosition.mOutputPath.mAttributeId == kInvalidAttributeId)
+    if (mPosition.mOutputPath.mAttributeId == kInvalidClusterId)
+    {
+        mAttributeIndex = kInvalidIndex;
+    }
+
+    if (mAttributeIndex == kInvalidIndex)
     {
-        if (mPosition.mAttributePath->mValue.HasWildcardAttributeId())
+        // start a new iteration on the current endpoint
+        mAttributes = mDataModelProvider->Attributes(mPosition.mOutputPath);
+
+        if (mPosition.mOutputPath.mAttributeId != kInvalidAttributeId)
         {
-            AttributeEntry entry = mDataModelProvider->FirstAttribute(mPosition.mOutputPath);
-            return entry.IsValid()                                         //
-                ? entry.path.mAttributeId                                  //
-                : Clusters::Globals::Attributes::GeneratedCommandList::Id; //
+            // Position on the correct cluster if we have a start point
+            mAttributeIndex = 0;
+            while ((mAttributeIndex < mAttributes.size()) &&
+                   (mAttributes[mAttributeIndex].attributeId != mPosition.mOutputPath.mAttributeId))
+            {
+                mAttributeIndex++;
+            }
         }
+    }
 
-        // At this point, the attributeID is NOT a wildcard (i.e. it is fixed).
-        //
-        // For wildcard expansion, we validate that this is a valid attribute for the given
-        // cluster on the given endpoint. If not a wildcard expansion, return it as-is.
-        if (mPosition.mAttributePath->mValue.IsWildcardPath())
+    if (mPosition.mOutputPath.mAttributeId == kInvalidAttributeId)
+    {
+        if (!mPosition.mAttributePath->mValue.HasWildcardAttributeId())
         {
-            if (!IsValidAttributeId(mPosition.mAttributePath->mValue.mAttributeId))
+            // The attributeID is NOT a wildcard (i.e. it is fixed).
+            //
+            // For wildcard expansion, we validate that this is a valid attribute for the given
+            // cluster on the given endpoint. If not a wildcard expansion, return it as-is.
+            if (mPosition.mAttributePath->mValue.IsWildcardPath())
             {
-                return std::nullopt;
+                if (!IsValidAttributeId(mPosition.mAttributePath->mValue.mAttributeId))
+                {
+                    return std::nullopt;
+                }
             }
+            return mPosition.mAttributePath->mValue.mAttributeId;
         }
-        return mPosition.mAttributePath->mValue.mAttributeId;
+        mAttributeIndex = 0;
+    }
+    else
+    {
+        mAttributeIndex++;
     }
 
     // Advance the existing attribute id if it can be advanced.
@@ -171,10 +196,9 @@ std::optional<AttributeId> AttributePathExpandIterator::NextAttributeId()
         return std::nullopt;
     }
 
-    AttributeEntry entry = mDataModelProvider->NextAttribute(mPosition.mOutputPath);
-    if (entry.IsValid())
+    if (mAttributeIndex < mAttributes.size())
     {
-        return entry.path.mAttributeId;
+        return mAttributes[mAttributeIndex].attributeId;
     }
 
     // Finished the data model, start with global attributes
diff --git a/src/app/AttributePathExpandIterator.h b/src/app/AttributePathExpandIterator.h
@@ -126,6 +126,9 @@ class AttributePathExpandIterator
     DataModel::MetadataList<DataModel::ServerClusterEntry> mClusters; // all clustesr ON THE CURRENT endpoint
     size_t mClusterIndex = kInvalidIndex;
 
+    DataModel::MetadataList<DataModel::AttributeEntry2> mAttributes; // all clustesr ON THE CURRENT cluster
+    size_t mAttributeIndex = kInvalidIndex;
+
     /// Move to the next endpoint/cluster/attribute triplet that is valid given
     /// the current mOutputPath and mpAttributePath.
     ///
diff --git a/src/app/InteractionModelEngine.cpp b/src/app/InteractionModelEngine.cpp
@@ -1567,7 +1567,8 @@ CHIP_ERROR InteractionModelEngine::PushFrontAttributePathList(SingleLinkedListNo
 
 bool InteractionModelEngine::IsExistentAttributePath(const ConcreteAttributePath & path)
 {
-    return GetDataModelProvider()->GetAttributeInfo(path).has_value();
+    DataModel::AttributeFinder finder(mDataModelProvider);
+    return finder.Find(path).has_value();
 }
 
 void InteractionModelEngine::RemoveDuplicateConcreteAttributePath(SingleLinkedListNode<AttributePathParams> *& aAttributePaths)
diff --git a/src/app/WriteHandler.cpp b/src/app/WriteHandler.cpp
@@ -16,6 +16,7 @@
  *    limitations under the License.
  */
 
+#include "app/data-model-provider/MetadataSearch.h"
 #include <app/AppConfig.h>
 #include <app/AttributeAccessInterfaceRegistry.h>
 #include <app/AttributeValueDecoder.h>
@@ -110,7 +111,9 @@ std::optional<bool> WriteHandler::IsListAttributePath(const ConcreteAttributePat
         return std::nullopt;
     }
 
-    auto info = mDataModelProvider->GetAttributeInfo(path);
+    DataModel::AttributeFinder finder(mDataModelProvider);
+    std::optional<DataModel::AttributeEntry2> info = finder.Find(path);
+
     if (!info.has_value())
     {
         return std::nullopt;
diff --git a/src/app/data-model-provider/BUILD.gn b/src/app/data-model-provider/BUILD.gn
@@ -23,7 +23,6 @@ source_set("data-model-provider") {
     "EventsGenerator.h",
     "MetadataList.h",
     "MetadataSearch.h",
-    "MetadataTypes.cpp",
     "MetadataTypes.h",
     "OperationTypes.h",
     "Provider.h",
diff --git a/src/app/data-model-provider/MetadataTypes.cpp b/src/app/data-model-provider/MetadataTypes.cpp
diff --git a/src/app/data-model-provider/MetadataTypes.h b/src/app/data-model-provider/MetadataTypes.h
@@ -84,16 +84,6 @@ struct ClusterInfo
     ClusterInfo(DataVersion version) : dataVersion(version) {}
 };
 
-struct ClusterEntry
-{
-    ConcreteClusterPath path;
-    ClusterInfo info;
-
-    bool IsValid() const { return path.HasValidIds(); }
-
-    static const ClusterEntry kInvalid;
-};
-
 enum class AttributeQualityFlags : uint32_t
 {
     kListAttribute   = 0x0004, // This attribute is a list attribute
@@ -103,25 +93,6 @@ enum class AttributeQualityFlags : uint32_t
     kTimed           = 0x0040, // `T` quality on attributes (writes require timed interactions)
 };
 
-struct AttributeInfo
-{
-    BitFlags<AttributeQualityFlags> flags;
-
-    // read/write access will be missing if read/write is NOT allowed
-    std::optional<Access::Privilege> readPrivilege;  // generally defaults to View if readable
-    std::optional<Access::Privilege> writePrivilege; // generally defaults to Operate if writable
-};
-
-struct AttributeEntry
-{
-    ConcreteAttributePath path;
-    AttributeInfo info;
-
-    bool IsValid() const { return path.HasValidIds(); }
-
-    static const AttributeEntry kInvalid;
-};
-
 struct AttributeEntry2
 {
     AttributeId attributeId;
@@ -185,15 +156,6 @@ class ProviderMetadataTree
 public:
     virtual ~ProviderMetadataTree() = default;
 
-    // Attribute iteration and accessors provide cluster-level access over
-    // attributes
-    virtual AttributeEntry FirstAttribute(const ConcreteClusterPath & cluster)                = 0;
-    virtual AttributeEntry NextAttribute(const ConcreteAttributePath & before)                = 0;
-    virtual std::optional<AttributeInfo> GetAttributeInfo(const ConcreteAttributePath & path) = 0;
-
-    /// List metadata capabilties
-    ///
-    ///  TODO: convert ALL items above to the new format
     using SemanticTag = Clusters::Descriptor::Structs::SemanticTagStruct::Type;
 
     virtual MetadataList<EndpointEntry> Endpoints() = 0;
diff --git a/src/app/reporting/Engine.cpp b/src/app/reporting/Engine.cpp
@@ -82,7 +82,9 @@ std::optional<CHIP_ERROR> ValidateReadAttributeACL(DataModel::Provider * dataMod
                              .requestType = RequestType::kAttributeReadRequest,
                              .entityId    = path.mAttributeId };
 
-    std::optional<DataModel::AttributeInfo> info = dataModel->GetAttributeInfo(path);
+    DataModel::AttributeFinder finder(dataModel);
+
+    std::optional<DataModel::AttributeEntry2> info = finder.Find(path);
 
     // If the attribute exists, we know whether it is readable (readPrivilege has value)
     // and what the required access privilege is. However for attributes missing from the metatada
diff --git a/src/data-model-providers/codegen/CodegenDataModelProvider.cpp b/src/data-model-providers/codegen/CodegenDataModelProvider.cpp
diff --git a/src/data-model-providers/codegen/CodegenDataModelProvider.h b/src/data-model-providers/codegen/CodegenDataModelProvider.h

Original file line number	Diff line number	Diff line change
`@@ -1567,7 +1567,8 @@ CHIP_ERROR InteractionModelEngine::PushFrontAttributePathList(SingleLinkedListNo`
`1567`	`1567`
`1568`	`1568`	`bool InteractionModelEngine::IsExistentAttributePath(const ConcreteAttributePath & path)`
`1569`	`1569`	`{`
`1570`		`- return GetDataModelProvider()->GetAttributeInfo(path).has_value();`
	`1570`	`+ DataModel::AttributeFinder finder(mDataModelProvider);`
	`1571`	`+ return finder.Find(path).has_value();`
`1571`	`1572`	`}`
`1572`	`1573`
`1573`	`1574`	`void InteractionModelEngine::RemoveDuplicateConcreteAttributePath(SingleLinkedListNode<AttributePathParams> *& aAttributePaths)`