From 303834558de5f596ba8e23b39a8a0b7c9f3b6cc1 Mon Sep 17 00:00:00 2001
From: yunhanw-google <yunhanw@google.com>
Date: Fri, 20 May 2022 09:44:23 -0700
Subject: [PATCH] Fix clusterStateCache dataVersion bug (#18593)

---
 src/app/AttributePathParams.h                |  9 +++++
 src/app/ClusterStateCache.cpp                | 28 +++++++++++++--
 src/controller/tests/data_model/TestRead.cpp | 36 ++++++++++++++++++++
 3 files changed, 70 insertions(+), 3 deletions(-)

diff --git a/src/app/AttributePathParams.h b/src/app/AttributePathParams.h
index 47ac890f8de3c3..adfc77cfe27830 100644
--- 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
index c99daf57e0c999..e7aaca424055fe 100644
--- 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
index 85216a0dfe7460..5d93aad5e7a9f0 100644
--- 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.
     {