deduplicate the concrete paths intersected with the other wildcard pa…

…ths in server (#17719)
project-chip · Aug 4, 2023 · 0773cfb · 0773cfb
1 parent af6aa95
commit 0773cfb
Show file tree

Hide file tree

Showing 7 changed files with 206 additions and 0 deletions.
diff --git a/src/app/InteractionModelEngine.cpp b/src/app/InteractionModelEngine.cpp
@@ -24,10 +24,14 @@
  */
 
 #include "InteractionModelEngine.h"
+
 #include <cinttypes>
 
 #include <lib/core/CHIPTLVUtilities.hpp>
 
+extern bool emberAfContainsAttribute(chip::EndpointId endpoint, chip::ClusterId clusterId, chip::AttributeId attributeId,
+                                     bool asServer);
+
 namespace chip {
 namespace app {
 InteractionModelEngine sInteractionModelEngine;
@@ -852,6 +856,61 @@ CHIP_ERROR InteractionModelEngine::PushFrontAttributePathList(ObjectList<Attribu
     return err;
 }
 
+void InteractionModelEngine::RemoveDuplicateConcreteAttributePath(ObjectList<AttributePathParams> *& aAttributePaths)
+{
+    ObjectList<AttributePathParams> * prev = nullptr;
+    auto * path1                           = aAttributePaths;
+
+    while (path1 != nullptr)
+    {
+        bool duplicate = false;
+        // skip all wildcard paths and invalid concrete attribute
+        if (path1->mValue.HasAttributeWildcard() ||
+            !emberAfContainsAttribute(path1->mValue.mEndpointId, path1->mValue.mClusterId, path1->mValue.mAttributeId, true))
+        {
+            prev  = path1;
+            path1 = path1->mpNext;
+            continue;
+        }
+
+        // Check whether a wildcard path expands to something that includes this concrete path.
+        for (auto * path2 = aAttributePaths; path2 != nullptr; path2 = path2->mpNext)
+        {
+            if (path2 == path1)
+            {
+                continue;
+            }
+
+            if (path2->mValue.HasAttributeWildcard() && path2->mValue.IsAttributePathSupersetOf(path1->mValue))
+            {
+                duplicate = true;
+                break;
+            }
+        }
+
+        // if path1 duplicates something from wildcard expansion, discard path1
+        if (!duplicate)
+        {
+            prev  = path1;
+            path1 = path1->mpNext;
+            continue;
+        }
+
+        if (path1 == aAttributePaths)
+        {
+            aAttributePaths = path1->mpNext;
+            mAttributePathPool.ReleaseObject(path1);
+            path1 = aAttributePaths;
+        }
+        else
+        {
+            prev->mpNext = path1->mpNext;
+            mAttributePathPool.ReleaseObject(path1);
+            path1 = prev->mpNext;
+        }
+    }
+}
+
 void InteractionModelEngine::ReleaseEventPathList(ObjectList<EventPathParams> *& aEventPathList)
 {
     ReleasePool(aEventPathList, mEventPathPool);

diff --git a/src/app/InteractionModelEngine.h b/src/app/InteractionModelEngine.h
@@ -157,6 +157,10 @@ class InteractionModelEngine : public Messaging::UnsolicitedMessageHandler,
     CHIP_ERROR PushFrontAttributePathList(ObjectList<AttributePathParams> *& aAttributePathList,
                                           AttributePathParams & aAttributePath);
 
+    // If a concrete path indicates an attribute that is also referenced by a wildcard path in the request,
+    // the path SHALL be removed from the list.
+    void RemoveDuplicateConcreteAttributePath(ObjectList<AttributePathParams> *& aAttributePaths);
+
     void ReleaseEventPathList(ObjectList<EventPathParams> *& aEventPathList);
 
     CHIP_ERROR PushFrontEventPathParamsList(ObjectList<EventPathParams> *& aEventPathList, EventPathParams & aEventPath);

diff --git a/src/app/ReadHandler.cpp b/src/app/ReadHandler.cpp
@@ -473,6 +473,7 @@ CHIP_ERROR ReadHandler::ProcessAttributePathList(AttributePathIBs::Parser & aAtt
     // if we have exhausted this container
     if (CHIP_END_OF_TLV == err)
     {
+        InteractionModelEngine::GetInstance()->RemoveDuplicateConcreteAttributePath(mpAttributePathList);
         mAttributePathExpandIterator = AttributePathExpandIterator(mpAttributePathList);
         err                          = CHIP_NO_ERROR;
     }

diff --git a/src/app/tests/TestInteractionModelEngine.cpp b/src/app/tests/TestInteractionModelEngine.cpp
@@ -24,6 +24,8 @@
 
 #include <app/InteractionModelEngine.h>
 #include <app/tests/AppTestContext.h>
+#include <app/util/mock/Constants.h>
+#include <app/util/mock/Functions.h>
 #include <lib/core/CHIPCore.h>
 #include <lib/core/CHIPTLV.h>
 #include <lib/core/CHIPTLVDebug.hpp>
@@ -44,6 +46,7 @@ class TestInteractionModelEngine
 {
 public:
     static void TestAttributePathParamsPushRelease(nlTestSuite * apSuite, void * apContext);
+    static void TestRemoveDuplicateConcreteAttribute(nlTestSuite * apSuite, void * apContext);
     static int GetAttributePathListLength(ObjectList<AttributePathParams> * apattributePathParamsList);
 };
 
@@ -95,6 +98,130 @@ void TestInteractionModelEngine::TestAttributePathParamsPushRelease(nlTestSuite
     InteractionModelEngine::GetInstance()->ReleaseAttributePathList(attributePathParamsList);
     NL_TEST_ASSERT(apSuite, GetAttributePathListLength(attributePathParamsList) == 0);
 }
+
+void TestInteractionModelEngine::TestRemoveDuplicateConcreteAttribute(nlTestSuite * apSuite, void * apContext)
+{
+    TestContext & ctx = *static_cast<TestContext *>(apContext);
+    CHIP_ERROR err    = CHIP_NO_ERROR;
+    err               = InteractionModelEngine::GetInstance()->Init(&ctx.GetExchangeManager(), &ctx.GetFabricTable());
+    NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
+    ObjectList<AttributePathParams> * attributePathParamsList = nullptr;
+    AttributePathParams attributePathParams1;
+    AttributePathParams attributePathParams2;
+    AttributePathParams attributePathParams3;
+
+    // Three concrete paths, no duplicates
+    attributePathParams1.mEndpointId  = Test::kMockEndpoint3;
+    attributePathParams1.mClusterId   = Test::MockClusterId(2);
+    attributePathParams1.mAttributeId = Test::MockAttributeId(1);
+
+    attributePathParams2.mEndpointId  = Test::kMockEndpoint3;
+    attributePathParams2.mClusterId   = Test::MockClusterId(2);
+    attributePathParams2.mAttributeId = Test::MockAttributeId(2);
+
+    attributePathParams3.mEndpointId  = Test::kMockEndpoint3;
+    attributePathParams3.mClusterId   = Test::MockClusterId(2);
+    attributePathParams3.mAttributeId = Test::MockAttributeId(3);
+
+    InteractionModelEngine::GetInstance()->PushFrontAttributePathList(attributePathParamsList, attributePathParams1);
+    InteractionModelEngine::GetInstance()->PushFrontAttributePathList(attributePathParamsList, attributePathParams2);
+    InteractionModelEngine::GetInstance()->PushFrontAttributePathList(attributePathParamsList, attributePathParams3);
+    InteractionModelEngine::GetInstance()->RemoveDuplicateConcreteAttributePath(attributePathParamsList);
+    NL_TEST_ASSERT(apSuite, GetAttributePathListLength(attributePathParamsList) == 3);
+    InteractionModelEngine::GetInstance()->ReleaseAttributePathList(attributePathParamsList);
+
+    attributePathParams1.mEndpointId  = kInvalidEndpointId;
+    attributePathParams1.mClusterId   = kInvalidClusterId;
+    attributePathParams1.mAttributeId = kInvalidAttributeId;
+
+    attributePathParams2.mEndpointId  = Test::kMockEndpoint3;
+    attributePathParams2.mClusterId   = Test::MockClusterId(2);
+    attributePathParams2.mAttributeId = Test::MockAttributeId(2);
+
+    attributePathParams3.mEndpointId  = Test::kMockEndpoint3;
+    attributePathParams3.mClusterId   = Test::MockClusterId(2);
+    attributePathParams3.mAttributeId = Test::MockAttributeId(3);
+
+    // 1st path is wildcard endpoint, 2nd, 3rd paths are concrete paths, the concrete ones would be removed.
+    InteractionModelEngine::GetInstance()->PushFrontAttributePathList(attributePathParamsList, attributePathParams1);
+    InteractionModelEngine::GetInstance()->PushFrontAttributePathList(attributePathParamsList, attributePathParams2);
+    InteractionModelEngine::GetInstance()->PushFrontAttributePathList(attributePathParamsList, attributePathParams3);
+    InteractionModelEngine::GetInstance()->RemoveDuplicateConcreteAttributePath(attributePathParamsList);
+    NL_TEST_ASSERT(apSuite, GetAttributePathListLength(attributePathParamsList) == 1);
+    InteractionModelEngine::GetInstance()->ReleaseAttributePathList(attributePathParamsList);
+
+    // 2nd path is wildcard endpoint, 1st, 3rd paths are concrete paths, the latter two would be removed.
+    InteractionModelEngine::GetInstance()->PushFrontAttributePathList(attributePathParamsList, attributePathParams2);
+    InteractionModelEngine::GetInstance()->PushFrontAttributePathList(attributePathParamsList, attributePathParams1);
+    InteractionModelEngine::GetInstance()->PushFrontAttributePathList(attributePathParamsList, attributePathParams3);
+    InteractionModelEngine::GetInstance()->RemoveDuplicateConcreteAttributePath(attributePathParamsList);
+    NL_TEST_ASSERT(apSuite, GetAttributePathListLength(attributePathParamsList) == 1);
+    InteractionModelEngine::GetInstance()->ReleaseAttributePathList(attributePathParamsList);
+
+    // 3nd path is wildcard endpoint, 1st, 2nd paths are concrete paths, the latter two would be removed.
+    InteractionModelEngine::GetInstance()->PushFrontAttributePathList(attributePathParamsList, attributePathParams2);
+    InteractionModelEngine::GetInstance()->PushFrontAttributePathList(attributePathParamsList, attributePathParams3);
+    InteractionModelEngine::GetInstance()->PushFrontAttributePathList(attributePathParamsList, attributePathParams1);
+    InteractionModelEngine::GetInstance()->RemoveDuplicateConcreteAttributePath(attributePathParamsList);
+    NL_TEST_ASSERT(apSuite, GetAttributePathListLength(attributePathParamsList) == 1);
+    InteractionModelEngine::GetInstance()->ReleaseAttributePathList(attributePathParamsList);
+
+    attributePathParams1.mEndpointId  = Test::kMockEndpoint3;
+    attributePathParams1.mClusterId   = Test::MockClusterId(2);
+    attributePathParams1.mAttributeId = kInvalidAttributeId;
+
+    attributePathParams2.mEndpointId  = Test::kMockEndpoint2;
+    attributePathParams2.mClusterId   = Test::MockClusterId(2);
+    attributePathParams2.mAttributeId = Test::MockAttributeId(2);
+
+    attributePathParams3.mEndpointId  = Test::kMockEndpoint2;
+    attributePathParams3.mClusterId   = Test::MockClusterId(2);
+    attributePathParams3.mAttributeId = Test::MockAttributeId(3);
+
+    // 1st is wildcard one, but not intersect with the latter two concrete paths, so the paths in total are 3 finally
+    InteractionModelEngine::GetInstance()->PushFrontAttributePathList(attributePathParamsList, attributePathParams1);
+    InteractionModelEngine::GetInstance()->PushFrontAttributePathList(attributePathParamsList, attributePathParams2);
+    InteractionModelEngine::GetInstance()->PushFrontAttributePathList(attributePathParamsList, attributePathParams3);
+    InteractionModelEngine::GetInstance()->RemoveDuplicateConcreteAttributePath(attributePathParamsList);
+    NL_TEST_ASSERT(apSuite, GetAttributePathListLength(attributePathParamsList) == 3);
+    InteractionModelEngine::GetInstance()->ReleaseAttributePathList(attributePathParamsList);
+
+    attributePathParams1.mEndpointId  = kInvalidEndpointId;
+    attributePathParams1.mClusterId   = kInvalidClusterId;
+    attributePathParams1.mAttributeId = kInvalidAttributeId;
+
+    attributePathParams2.mEndpointId  = Test::kMockEndpoint3;
+    attributePathParams2.mClusterId   = kInvalidClusterId;
+    attributePathParams2.mAttributeId = kInvalidAttributeId;
+
+    attributePathParams3.mEndpointId  = kInvalidEndpointId;
+    attributePathParams3.mClusterId   = kInvalidClusterId;
+    attributePathParams3.mAttributeId = Test::MockAttributeId(3);
+
+    // Wildcards cannot be deduplicated.
+    InteractionModelEngine::GetInstance()->PushFrontAttributePathList(attributePathParamsList, attributePathParams1);
+    InteractionModelEngine::GetInstance()->PushFrontAttributePathList(attributePathParamsList, attributePathParams2);
+    InteractionModelEngine::GetInstance()->PushFrontAttributePathList(attributePathParamsList, attributePathParams3);
+    InteractionModelEngine::GetInstance()->RemoveDuplicateConcreteAttributePath(attributePathParamsList);
+    NL_TEST_ASSERT(apSuite, GetAttributePathListLength(attributePathParamsList) == 3);
+    InteractionModelEngine::GetInstance()->ReleaseAttributePathList(attributePathParamsList);
+
+    attributePathParams1.mEndpointId  = kInvalidEndpointId;
+    attributePathParams1.mClusterId   = Test::MockClusterId(2);
+    attributePathParams1.mAttributeId = Test::MockAttributeId(10);
+
+    attributePathParams2.mEndpointId  = Test::kMockEndpoint3;
+    attributePathParams2.mClusterId   = Test::MockClusterId(2);
+    attributePathParams2.mAttributeId = Test::MockAttributeId(10);
+
+    // 1st path is wildcard endpoint, 2nd path is invalid attribute
+    InteractionModelEngine::GetInstance()->PushFrontAttributePathList(attributePathParamsList, attributePathParams1);
+    InteractionModelEngine::GetInstance()->PushFrontAttributePathList(attributePathParamsList, attributePathParams2);
+    InteractionModelEngine::GetInstance()->RemoveDuplicateConcreteAttributePath(attributePathParamsList);
+    NL_TEST_ASSERT(apSuite, GetAttributePathListLength(attributePathParamsList) == 2);
+    InteractionModelEngine::GetInstance()->ReleaseAttributePathList(attributePathParamsList);
+}
+
 } // namespace app
 } // namespace chip
 
@@ -104,6 +231,7 @@ namespace {
 const nlTest sTests[] =
         {
                 NL_TEST_DEF("TestAttributePathParamsPushRelease", chip::app::TestInteractionModelEngine::TestAttributePathParamsPushRelease),
+                NL_TEST_DEF("TestRemoveDuplicateConcreteAttribute", chip::app::TestInteractionModelEngine::TestRemoveDuplicateConcreteAttribute),
                 NL_TEST_SENTINEL()
         };
 // clang-format on

diff --git a/src/app/util/attribute-storage.cpp b/src/app/util/attribute-storage.cpp
@@ -650,6 +650,10 @@ EmberAfStatus emAfReadOrWriteAttribute(EmberAfAttributeSearchRecord * attRecord,
             // Dynamic endpoints are external and don't factor into storage size
             if (!isDynamicEndpoint)
             {
+                if (emAfEndpoints[ep].endpointType == nullptr)
+                {
+                    return EMBER_ZCL_STATUS_UNSUPPORTED_ATTRIBUTE;
+                }
                 attributeOffsetIndex = static_cast<uint16_t>(attributeOffsetIndex + emAfEndpoints[ep].endpointType->endpointSize);
             }
         }

diff --git a/src/app/util/mock/attribute-storage.cpp b/src/app/util/mock/attribute-storage.cpp
@@ -164,6 +164,11 @@ uint16_t emberAfGetServerAttributeIndexByAttributeId(chip::EndpointId endpoint,
     return UINT16_MAX;
 }
 
+bool emberAfContainsAttribute(chip::EndpointId endpoint, chip::ClusterId clusterId, chip::AttributeId attributeId, bool asServer)
+{
+    return !(emberAfGetServerAttributeIndexByAttributeId(endpoint, clusterId, attributeId) == UINT16_MAX);
+}
+
 chip::EndpointId emberAfEndpointFromIndex(uint16_t index)
 {
     VerifyOrDie(index < ArraySize(endpoints));

diff --git a/src/darwin/Framework/CHIP/CHIPIMDispatch.mm b/src/darwin/Framework/CHIP/CHIPIMDispatch.mm
@@ -272,6 +272,11 @@ uint16_t emberAfGetServerAttributeIndexByAttributeId(EndpointId endpoint, Cluste
     return UINT16_MAX;
 }
 
+bool emberAfContainsAttribute(chip::EndpointId endpoint, chip::ClusterId clusterId, chip::AttributeId attributeId, bool asServer)
+{
+    return false;
+}
+
 uint8_t emberAfClusterCount(EndpointId endpoint, bool server)
 {
     if (endpoint == kSupportedEndpoint && server) {