Add support to intercept attribute report and prune any removed… (project-chip#33523)

nivi-apple · restyled-commits · bzbarsky-apple · web-flow · commit c1e1e6ebfbaf · 2024-06-01T04:58:00.000Z
* Add support to intercept attribute report and prune any removed endpoints and clusters and attributes from both persisted clusters data and data storage

- When an attribute report is received, check if there are any changes to parts list or server list for the descriptor cluster or attribute list for any cluster.

- If any endpoints were removed, make sure to delete the cluster index for the endpoint and all cluster data for that endpoint should be deleted.

- If any clusters were removed from an endpoint, make sure to delete the cluster from the cluster index for that endpoint and also clear the cluster data for that cluster.

- If any attributes were removed from a cluster, make sure to update the cluster data and delete the entry for the attribute that was removed.

* Restyled by whitespace

* Restyled by clang-format

* fixes

* Fix the check for the cluster paths

* Apply suggestions from code review

Co-authored-by: Boris Zbarsky &lt;bzbarsky@apple.com&gt;

* Address review comments

* Restyled by whitespace

* Restyled by clang-format

* Added helpers for removing endpoints, clusters and attributes

* Restyled by clang-format

* Clean up the tests

* Remove addition of extra line

* Restyled by clang-format

* Apply suggestions from code review

Co-authored-by: Boris Zbarsky &lt;bzbarsky@apple.com&gt;

* Addressed review comments

* Restyled by whitespace

* Uncomment subscription pool tests

* More clean up

* Restyled by clang-format

* Add few more strong types for data structures

* Apply suggestions from code review

Co-authored-by: Boris Zbarsky &lt;bzbarsky@apple.com&gt;

* Restyled by whitespace

* Restyled by clang-format

* Update src/darwin/Framework/CHIP/MTRDevice.mm

---------

Co-authored-by: Restyled.io &lt;commits@restyled.io&gt;
Co-authored-by: Boris Zbarsky &lt;bzbarsky@apple.com&gt;
Co-authored-by: Justin Wood &lt;woody@apple.com&gt;
diff --git a/src/darwin/Framework/CHIP/MTRDevice.mm b/src/darwin/Framework/CHIP/MTRDevice.mm
@@ -209,6 +209,11 @@ - (void)storeValue:(MTRDeviceDataValueDictionary _Nullable)value forAttribute:(N
     _attributes[attribute] = value;
 }
 
+- (void)removeValueForAttribute:(NSNumber *)attribute
+{
+    [_attributes removeObjectForKey:attribute];
+}
+
 - (NSDictionary<NSNumber *, MTRDeviceDataValueDictionary> *)attributes
 {
     return _attributes;
@@ -670,7 +675,7 @@ - (void)_setDSTOffsets:(NSArray<MTRTimeSynchronizationClusterDSTOffsetStruct *>
                                  completion:setDSTOffsetResponseHandler];
 }
 
-- (NSMutableArray<NSNumber *> *)arrayOfNumbersFromAttributeValue:(NSDictionary *)dataDictionary
+- (NSMutableArray<NSNumber *> *)arrayOfNumbersFromAttributeValue:(MTRDeviceDataValueDictionary)dataDictionary
 {
     if (![MTRArrayValueType isEqual:dataDictionary[MTRTypeKey]]) {
         return nil;
@@ -1892,6 +1897,17 @@ - (void)_setCachedAttributeValue:(MTRDeviceDataValueDictionary _Nullable)value f
     _clusterDataToPersist[clusterPath] = clusterData;
 }
 
+- (void)_removeCachedAttribute:(NSNumber *)attributeID fromCluster:(MTRClusterPath *)clusterPath
+{
+    os_unfair_lock_assert_owner(&self->_lock);
+
+    if (_clusterDataToPersist == nil) {
+        return;
+    }
+    auto * clusterData = _clusterDataToPersist[clusterPath];
+    [clusterData removeValueForAttribute:attributeID];
+}
+
 - (void)_createDataVersionFilterListFromDictionary:(NSDictionary<MTRClusterPath *, NSNumber *> *)dataVersions dataVersionFilterList:(DataVersionFilter **)dataVersionFilterList count:(size_t *)count sizeReduction:(size_t)sizeReduction
 {
     size_t maxDataVersionFilterSize = dataVersions.count;
@@ -2956,7 +2972,7 @@ - (BOOL)_attributeDataValue:(NSDictionary *)one isEqualToDataValue:(NSDictionary
 }
 
 // Utility to return data value dictionary without data version
-- (NSDictionary *)_dataValueWithoutDataVersion:(NSDictionary *)attributeValue;
+- (NSDictionary *)_dataValueWithoutDataVersion:(NSDictionary *)attributeValue
 {
     // Sanity check for nil - return the same input to fail gracefully
     if (!attributeValue || !attributeValue[MTRTypeKey]) {
@@ -3018,6 +3034,116 @@ - (BOOL)_attributeAffectsDeviceConfiguration:(MTRAttributePath *)attributePath
     return NO;
 }
 
+- (void)_removeClusters:(NSSet<MTRClusterPath *> *)clusterPathsToRemove
+    doRemoveFromDataStore:(BOOL)doRemoveFromDataStore
+{
+    os_unfair_lock_assert_owner(&self->_lock);
+
+    [_persistedClusters minusSet:clusterPathsToRemove];
+
+    for (MTRClusterPath * path in clusterPathsToRemove) {
+        [_persistedClusterData removeObjectForKey:path];
+        [_clusterDataToPersist removeObjectForKey:path];
+        if (doRemoveFromDataStore) {
+            [self.deviceController.controllerDataStore clearStoredClusterDataForNodeID:self.nodeID endpointID:path.endpoint clusterID:path.cluster];
+        }
+    }
+}
+
+- (void)_removeAttributes:(NSSet<NSNumber *> *)attributes fromCluster:(MTRClusterPath *)clusterPath
+{
+    os_unfair_lock_assert_owner(&self->_lock);
+
+    for (NSNumber * attribute in attributes) {
+        [self _removeCachedAttribute:attribute fromCluster:clusterPath];
+    }
+    // Just clear out the NSCache entry for this cluster, so we'll load it from storage as needed.
+    [_persistedClusterData removeObjectForKey:clusterPath];
+    [self.deviceController.controllerDataStore removeAttributes:attributes fromCluster:clusterPath forNodeID:self.nodeID];
+}
+
+- (void)_pruneEndpointsIn:(MTRDeviceDataValueDictionary)previousPartsListValue
+              missingFrom:(MTRDeviceDataValueDictionary)newPartsListValue
+{
+    // If the parts list changed and one or more endpoints were removed, remove all the
+    // clusters for all those endpoints from our data structures.
+    // Also remove those endpoints from the data store.
+    NSMutableSet<NSNumber *> * toBeRemovedEndpoints = [NSMutableSet setWithArray:[self arrayOfNumbersFromAttributeValue:previousPartsListValue]];
+    NSSet<NSNumber *> * endpointsOnDevice = [NSSet setWithArray:[self arrayOfNumbersFromAttributeValue:newPartsListValue]];
+    [toBeRemovedEndpoints minusSet:endpointsOnDevice];
+
+    for (NSNumber * endpoint in toBeRemovedEndpoints) {
+        NSMutableSet<MTRClusterPath *> * clusterPathsToRemove = [[NSMutableSet alloc] init];
+        for (MTRClusterPath * path in _persistedClusters) {
+            if ([path.endpoint isEqualToNumber:endpoint]) {
+                [clusterPathsToRemove addObject:path];
+            }
+        }
+        [self _removeClusters:clusterPathsToRemove doRemoveFromDataStore:NO];
+        [self.deviceController.controllerDataStore clearStoredClusterDataForNodeID:self.nodeID endpointID:endpoint];
+    }
+}
+
+- (void)_pruneClustersIn:(MTRDeviceDataValueDictionary)previousServerListValue
+             missingFrom:(MTRDeviceDataValueDictionary)newServerListValue
+             forEndpoint:(NSNumber *)endpointID
+{
+    // If the server list changed and clusters were removed, remove those clusters from our data structures.
+    // Also remove them from the data store.
+    NSMutableSet<NSNumber *> * toBeRemovedClusters = [NSMutableSet setWithArray:[self arrayOfNumbersFromAttributeValue:previousServerListValue]];
+    NSSet<NSNumber *> * clustersStillOnEndpoint = [NSSet setWithArray:[self arrayOfNumbersFromAttributeValue:newServerListValue]];
+    [toBeRemovedClusters minusSet:clustersStillOnEndpoint];
+
+    NSMutableSet<MTRClusterPath *> * clusterPathsToRemove = [[NSMutableSet alloc] init];
+    for (MTRClusterPath * path in _persistedClusters) {
+        if ([path.endpoint isEqualToNumber:endpointID] && [toBeRemovedClusters containsObject:path.cluster])
+            [clusterPathsToRemove addObject:path];
+    }
+    [self _removeClusters:clusterPathsToRemove doRemoveFromDataStore:YES];
+}
+
+- (void)_pruneAttributesIn:(MTRDeviceDataValueDictionary)previousAttributeListValue
+               missingFrom:(MTRDeviceDataValueDictionary)newAttributeListValue
+                forCluster:(MTRClusterPath *)clusterPath
+{
+    // If the attribute list changed and attributes were removed, remove the attributes from our
+    // data structures.
+    NSMutableSet<NSNumber *> * toBeRemovedAttributes = [NSMutableSet setWithArray:[self arrayOfNumbersFromAttributeValue:previousAttributeListValue]];
+    NSSet<NSNumber *> * attributesStillInCluster = [NSSet setWithArray:[self arrayOfNumbersFromAttributeValue:newAttributeListValue]];
+
+    [toBeRemovedAttributes minusSet:attributesStillInCluster];
+    [self _removeAttributes:toBeRemovedAttributes fromCluster:clusterPath];
+}
+
+- (void)_pruneStoredDataForPath:(MTRAttributePath *)attributePath
+                    missingFrom:(MTRDeviceDataValueDictionary)newAttributeDataValue
+{
+    os_unfair_lock_assert_owner(&self->_lock);
+
+    if (![self _dataStoreExists] && !_clusterDataToPersist.count) {
+        MTR_LOG_DEBUG("%@ No data store to prune from", self);
+        return;
+    }
+
+    // Check if parts list changed or server list changed for the descriptor cluster or the attribute list changed for a cluster.
+    // If yes, we might need to prune any deleted endpoints, clusters or attributes from the storage and persisted cluster data.
+    if (attributePath.cluster.unsignedLongValue == MTRClusterIDTypeDescriptorID) {
+        if (attributePath.attribute.unsignedLongValue == MTRAttributeIDTypeClusterDescriptorAttributePartsListID && [attributePath.endpoint isEqualToNumber:@(kRootEndpointId)]) {
+            [self _pruneEndpointsIn:[self _cachedAttributeValueForPath:attributePath] missingFrom:newAttributeDataValue];
+            return;
+        }
+
+        if (attributePath.attribute.unsignedLongValue == MTRAttributeIDTypeClusterDescriptorAttributeServerListID) {
+            [self _pruneClustersIn:[self _cachedAttributeValueForPath:attributePath] missingFrom:newAttributeDataValue forEndpoint:attributePath.endpoint];
+            return;
+        }
+    }
+
+    if (attributePath.attribute.unsignedLongValue == MTRAttributeIDTypeGlobalAttributeAttributeListID) {
+        [self _pruneAttributesIn:[self _cachedAttributeValueForPath:attributePath] missingFrom:newAttributeDataValue forCluster:[MTRClusterPath clusterPathWithEndpointID:attributePath.endpoint clusterID:attributePath.cluster]];
+    }
+}
+
 // assume lock is held
 - (NSArray *)_getAttributesToReportWithReportedValues:(NSArray<NSDictionary<NSString *, id> *> *)reportedAttributeValues fromSubscription:(BOOL)isFromSubscription
 {
@@ -3071,13 +3197,16 @@ - (NSArray *)_getAttributesToReportWithReportedValues:(NSArray<NSDictionary<NSSt
                     [self _noteDataVersion:dataVersion forClusterPath:clusterPath];
                 }
 
-                [self _setCachedAttributeValue:attributeDataValue forPath:attributePath fromSubscription:isFromSubscription];
+                [self _pruneStoredDataForPath:attributePath missingFrom:attributeDataValue];
+
                 if (!_deviceConfigurationChanged) {
                     _deviceConfigurationChanged = [self _attributeAffectsDeviceConfiguration:attributePath];
                     if (_deviceConfigurationChanged) {
                         MTR_LOG("Device configuration changed due to changes in attribute %@", attributePath);
                     }
                 }
+
+                [self _setCachedAttributeValue:attributeDataValue forPath:attributePath fromSubscription:isFromSubscription];
             }
 
 #ifdef DEBUG
@@ -3230,6 +3359,22 @@ - (void)_storePersistedDeviceData
     [datastore storeDeviceData:[data copy] forNodeID:self.nodeID];
 }
 
+#ifdef DEBUG
+- (MTRDeviceClusterData *)_getClusterDataForPath:(MTRClusterPath *)path
+{
+    std::lock_guard lock(_lock);
+
+    return [[self _clusterDataForPath:path] copy];
+}
+
+- (BOOL)_clusterHasBeenPersisted:(MTRClusterPath *)path
+{
+    std::lock_guard lock(_lock);
+
+    return [_persistedClusters containsObject:path];
+}
+#endif
+
 - (BOOL)deviceCachePrimed
 {
     std::lock_guard lock(_lock);
diff --git a/src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.h b/src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.h
@@ -76,6 +76,9 @@ typedef void (^MTRDeviceControllerDataStoreClusterDataHandler)(NSDictionary<NSNu
 - (nullable MTRDeviceClusterData *)getStoredClusterDataForNodeID:(NSNumber *)nodeID endpointID:(NSNumber *)endpointID clusterID:(NSNumber *)clusterID;
 - (void)storeClusterData:(NSDictionary<MTRClusterPath *, MTRDeviceClusterData *> *)clusterData forNodeID:(NSNumber *)nodeID;
 - (void)clearStoredClusterDataForNodeID:(NSNumber *)nodeID;
+- (void)clearStoredClusterDataForNodeID:(NSNumber *)nodeID endpointID:(NSNumber *)endpointID;
+- (void)clearStoredClusterDataForNodeID:(NSNumber *)nodeID endpointID:(NSNumber *)endpointID clusterID:(NSNumber *)clusterID;
+- (void)removeAttributes:(NSSet<NSNumber *> *)attributes fromCluster:(MTRClusterPath *)path forNodeID:(NSNumber *)nodeID;
 - (void)clearAllStoredClusterData;
 
 /**
diff --git a/src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.mm b/src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.mm
@@ -446,6 +446,23 @@ - (BOOL)_storeEndpointIndex:(NSArray<NSNumber *> *)endpointIndex forNodeID:(NSNu
     return [self _storeAttributeCacheValue:endpointIndex forKey:[self _endpointIndexKeyForNodeID:nodeID]];
 }
 
+- (BOOL)_removeEndpointFromEndpointIndex:(NSNumber *)endpointID forNodeID:(NSNumber *)nodeID
+{
+    dispatch_assert_queue(_storageDelegateQueue);
+    if (!endpointID || !nodeID) {
+        MTR_LOG_ERROR("%s: unexpected nil input", __func__);
+        return NO;
+    }
+
+    NSMutableArray<NSNumber *> * endpointIndex = [[self _fetchEndpointIndexForNodeID:nodeID] mutableCopy];
+    if (endpointIndex == nil) {
+        return NO;
+    }
+
+    [endpointIndex removeObject:endpointID];
+    return [self _storeEndpointIndex:endpointIndex forNodeID:nodeID];
+}
+
 - (BOOL)_deleteEndpointIndexForNodeID:(NSNumber *)nodeID
 {
     dispatch_assert_queue(_storageDelegateQueue);
@@ -699,6 +716,76 @@ - (void)clearStoredClusterDataForNodeID:(NSNumber *)nodeID
     });
 }
 
+- (void)clearStoredClusterDataForNodeID:(NSNumber *)nodeID endpointID:(NSNumber *)endpointID clusterID:(NSNumber *)clusterID
+{
+    dispatch_async(_storageDelegateQueue, ^{
+        NSArray<NSNumber *> * clusterIndex = [self _fetchClusterIndexForNodeID:nodeID endpointID:endpointID];
+        NSMutableArray<NSNumber *> * clusterIndexCopy = [clusterIndex mutableCopy];
+        [clusterIndexCopy removeObject:clusterID];
+
+        BOOL success;
+        if (clusterIndexCopy.count != clusterIndex.count) {
+            success = [self _storeClusterIndex:clusterIndexCopy forNodeID:nodeID endpointID:endpointID];
+            if (!success) {
+                MTR_LOG_ERROR("clearStoredClusterDataForNodeID: _storeClusterIndex failed for node 0x%016llX endpoint %u", nodeID.unsignedLongLongValue, endpointID.unsignedShortValue);
+            }
+        }
+
+        success = [self _deleteClusterDataForNodeID:nodeID endpointID:endpointID clusterID:clusterID];
+        if (!success) {
+            MTR_LOG_ERROR("clearStoredClusterDataForNodeID: _deleteClusterDataForNodeID failed for node 0x%016llX endpoint %u cluster 0x%08lX", nodeID.unsignedLongLongValue, endpointID.unsignedShortValue, clusterID.unsignedLongValue);
+            return;
+        }
+
+        MTR_LOG("clearStoredClusterDataForNodeID: Deleted endpoint %u cluster 0x%08lX for node 0x%016llX successfully", endpointID.unsignedShortValue, clusterID.unsignedLongValue, nodeID.unsignedLongLongValue);
+    });
+}
+
+- (void)clearStoredClusterDataForNodeID:(NSNumber *)nodeID endpointID:(NSNumber *)endpointID
+{
+    dispatch_async(_storageDelegateQueue, ^{
+        BOOL success = [self _removeEndpointFromEndpointIndex:endpointID forNodeID:nodeID];
+        if (!success) {
+            MTR_LOG_ERROR("removeEndpointFromEndpointIndex for endpointID %u failed for node 0x%016llX", endpointID.unsignedShortValue, nodeID.unsignedLongLongValue);
+        }
+
+        NSArray<NSNumber *> * clusterIndex = [self _fetchClusterIndexForNodeID:nodeID endpointID:endpointID];
+
+        for (NSNumber * cluster in clusterIndex) {
+            success = [self _deleteClusterDataForNodeID:nodeID endpointID:endpointID clusterID:cluster];
+            if (!success) {
+                MTR_LOG_ERROR("Delete failed for clusterData for node 0x%016llX endpoint %u cluster 0x%08lX", nodeID.unsignedLongLongValue, endpointID.unsignedShortValue, cluster.unsignedLongValue);
+            }
+        }
+
+        success = [self _deleteClusterIndexForNodeID:nodeID endpointID:endpointID];
+        if (!success) {
+            MTR_LOG_ERROR("Delete failed for clusterIndex for node 0x%016llX endpoint %u", nodeID.unsignedLongLongValue, endpointID.unsignedShortValue);
+        }
+
+        MTR_LOG("clearStoredClusterDataForNodeID: Deleted endpoint %u for node 0x%016llX successfully", endpointID.unsignedShortValue, nodeID.unsignedLongLongValue);
+    });
+}
+
+- (void)removeAttributes:(NSSet<NSNumber *> *)attributes fromCluster:(MTRClusterPath *)path forNodeID:(NSNumber *)nodeID
+{
+    MTRDeviceClusterData * clusterData = [self getStoredClusterDataForNodeID:nodeID endpointID:path.endpoint clusterID:path.cluster];
+    if (clusterData == nil) {
+        return;
+    }
+    for (NSNumber * attribute in attributes) {
+        [clusterData removeValueForAttribute:attribute];
+    }
+
+    dispatch_async(_storageDelegateQueue, ^{
+        BOOL success = [self _storeClusterData:clusterData forNodeID:nodeID endpointID:path.endpoint clusterID:path.cluster];
+        if (!success) {
+            MTR_LOG_ERROR("removeAttributes: _storeClusterData failed for node 0x%016llX endpoint %u", nodeID.unsignedLongLongValue, path.endpoint.unsignedShortValue);
+        }
+        MTR_LOG("removeAttributes: Deleted attributes %@ from endpoint %u cluster 0x%08lX for node 0x%016llX successfully", attributes, path.endpoint.unsignedShortValue, path.cluster.unsignedLongValue, nodeID.unsignedLongLongValue);
+    });
+}
+
 - (void)clearAllStoredClusterData
 {
     dispatch_async(_storageDelegateQueue, ^{
diff --git a/src/darwin/Framework/CHIP/MTRDevice_Internal.h b/src/darwin/Framework/CHIP/MTRDevice_Internal.h
@@ -60,6 +60,7 @@ MTR_TESTABLE
 @property (nonatomic, readonly) NSDictionary<NSNumber *, MTRDeviceDataValueDictionary> * attributes; // attributeID => data-value dictionary
 
 - (void)storeValue:(MTRDeviceDataValueDictionary _Nullable)value forAttribute:(NSNumber *)attribute;
+- (void)removeValueForAttribute:(NSNumber *)attribute;
 
 - (nullable instancetype)initWithDataVersion:(NSNumber * _Nullable)dataVersion attributes:(NSDictionary<NSNumber *, MTRDeviceDataValueDictionary> * _Nullable)attributes;
 @end
diff --git a/src/darwin/Framework/CHIPTests/MTRPerControllerStorageTests.m b/src/darwin/Framework/CHIPTests/MTRPerControllerStorageTests.m
diff --git a/src/darwin/Framework/CHIPTests/TestHelpers/MTRTestDeclarations.h b/src/darwin/Framework/CHIPTests/TestHelpers/MTRTestDeclarations.h
