Fix clusterStateCache dataVersion bug (#18593)

project-chip · May 31, 2022 · 3038345 · 3038345
1 parent e0f2d1d
commit 3038345
Show file tree

Hide file tree

Showing 3 changed files with 70 additions and 3 deletions.
diff --git a/src/app/AttributePathParams.h b/src/app/AttributePathParams.h
@@ -96,6 +96,15 @@ struct AttributePathParams
         return true;
     }
 
+    bool Intersects(const AttributePathParams & other) const
+    {
+        VerifyOrReturnError(HasWildcardEndpointId() || other.HasWildcardEndpointId() || mEndpointId == other.mEndpointId, false);
+        VerifyOrReturnError(HasWildcardClusterId() || other.HasWildcardClusterId() || mClusterId == other.mClusterId, false);
+        VerifyOrReturnError(HasWildcardAttributeId() || other.HasWildcardAttributeId() || mAttributeId == other.mAttributeId,
+                            false);
+        return true;
+    }
+
     bool IncludesAttributesInCluster(const DataVersionFilter & other) const
     {
         VerifyOrReturnError(HasWildcardEndpointId() || mEndpointId == other.mEndpointId, false);

diff --git a/src/app/ClusterStateCache.cpp b/src/app/ClusterStateCache.cpp
@@ -454,11 +454,33 @@ CHIP_ERROR ClusterStateCache::OnUpdateDataVersionFilterList(DataVersionFilterIBs
     CHIP_ERROR err = CHIP_NO_ERROR;
     TLV::TLVWriter backup;
 
-    for (auto & attribute : aAttributePaths)
+    // Only put paths into mRequestPathSet if they cover clusters in their entirety and no other path in our path list
+    // points to a specific attribute from any of those clusters.
+    // this would help for data-out-of-sync issue when handling store data version for the particular case on two paths: (E1, C1,
+    // wildcard), (wildcard, C1, A1)
+    for (auto & attribute1 : aAttributePaths)
     {
-        if (attribute.HasWildcardAttributeId())
+        if (attribute1.HasWildcardAttributeId())
         {
-            mRequestPathSet.insert(attribute);
+            bool intersected = false;
+            for (auto & attribute2 : aAttributePaths)
+            {
+                if (attribute2.HasWildcardAttributeId())
+                {
+                    continue;
+                }
+
+                if (attribute1.Intersects(attribute2))
+                {
+                    intersected = true;
+                    break;
+                }
+            }
+
+            if (!intersected)
+            {
+                mRequestPathSet.insert(attribute1);
+            }
         }
     }
 

diff --git a/src/controller/tests/data_model/TestRead.cpp b/src/controller/tests/data_model/TestRead.cpp
@@ -401,6 +401,42 @@ void TestReadInteraction::TestReadSubscribeAttributeResponseWithCache(nlTestSuit
     app::InteractionModelEngine::GetInstance()->RegisterReadHandlerAppCallback(&gTestReadInteraction);
 
     int testId = 0;
+
+    // Read of E2C3A1(dedup), E*C3A1(E1C3A1 not exit, E2C3A1 exist), E2C3A* (5 supported attributes)
+    // Expect no versions would be cached.
+    {
+        testId++;
+        ChipLogProgress(DataManagement, "\t -- Running Read with ClusterStateCache Test ID %d", testId);
+        app::ReadClient readClient(chip::app::InteractionModelEngine::GetInstance(), &ctx.GetExchangeManager(),
+                                   cache.GetBufferedCallback(), chip::app::ReadClient::InteractionType::Read);
+        chip::app::AttributePathParams attributePathParams1[3];
+        attributePathParams1[0].mEndpointId  = Test::kMockEndpoint2;
+        attributePathParams1[0].mClusterId   = Test::MockClusterId(3);
+        attributePathParams1[0].mAttributeId = Test::MockAttributeId(1);
+
+        attributePathParams1[1].mEndpointId  = kInvalidEndpointId;
+        attributePathParams1[1].mClusterId   = Test::MockClusterId(3);
+        attributePathParams1[1].mAttributeId = Test::MockAttributeId(1);
+
+        attributePathParams1[2].mEndpointId  = Test::kMockEndpoint2;
+        attributePathParams1[2].mClusterId   = Test::MockClusterId(3);
+        attributePathParams1[2].mAttributeId = kInvalidAttributeId;
+
+        readPrepareParams.mpAttributePathParamsList    = attributePathParams1;
+        readPrepareParams.mAttributePathParamsListSize = 3;
+        err                                            = readClient.SendRequest(readPrepareParams);
+        NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
+
+        ctx.DrainAndServiceIO();
+        NL_TEST_ASSERT(apSuite, delegate.mNumAttributeResponse == 6);
+        NL_TEST_ASSERT(apSuite, !delegate.mReadError);
+        Optional<DataVersion> version1;
+        app::ConcreteClusterPath clusterPath1(Test::kMockEndpoint2, Test::MockClusterId(3));
+        NL_TEST_ASSERT(apSuite, cache.GetVersion(clusterPath1, version1) == CHIP_NO_ERROR);
+        NL_TEST_ASSERT(apSuite, !version1.HasValue());
+        delegate.mNumAttributeResponse = 0;
+    }
+
     // Read of E2C3A1, E2C3A2 and E3C2A2.
     // Expect no versions would be cached.
     {