From 1a8cf55d8dbb9d32cd0923ca0a74221b83ba71b3 Mon Sep 17 00:00:00 2001
From: Boris Zbarsky <bzbarsky@apple.com>
Date: Wed, 9 Nov 2022 09:11:21 -0500
Subject: [PATCH] Fix auto resubcribe condition for in Darwin
 subscribeToAttributesWithEndpointID. (#23554)

A copy-paste error led to the wrong boolean being checked, so we would set up
auto-resubscription for any subscription which is not replacing existing
subscriptions, regardless of the value of resubscribeIfLost.
---
 src/darwin/Framework/CHIP/MTRBaseDevice.mm    |  2 +-
 .../Framework/CHIPTests/MTRDeviceTests.m      | 65 +++++++++++++++++++
 2 files changed, 66 insertions(+), 1 deletion(-)

diff --git a/src/darwin/Framework/CHIP/MTRBaseDevice.mm b/src/darwin/Framework/CHIP/MTRBaseDevice.mm
index 71e1cde79e86f8..fdc5cdbb6b2753 100644
--- a/src/darwin/Framework/CHIP/MTRBaseDevice.mm
+++ b/src/darwin/Framework/CHIP/MTRBaseDevice.mm
@@ -1222,7 +1222,7 @@ - (void)subscribeToAttributesWithEndpointID:(NSNumber * _Nullable)endpointID
                    auto readClient = Platform::New<app::ReadClient>(
                        engine, exchangeManager, callback->GetBufferedCallback(), chip::app::ReadClient::InteractionType::Subscribe);
 
-                   if (params.replaceExistingSubscriptions) {
+                   if (!params.resubscribeIfLost) {
                        err = readClient->SendRequest(readParams);
                    } else {
                        err = readClient->SendAutoResubscribeRequest(std::move(readParams));
diff --git a/src/darwin/Framework/CHIPTests/MTRDeviceTests.m b/src/darwin/Framework/CHIPTests/MTRDeviceTests.m
index a3a55d060ae9f7..78659c7b27c106 100644
--- a/src/darwin/Framework/CHIPTests/MTRDeviceTests.m
+++ b/src/darwin/Framework/CHIPTests/MTRDeviceTests.m
@@ -1234,6 +1234,8 @@ - (void)test015_FailedSubscribeWithQueueAcrossShutdown
     __auto_type clusterStateCacheContainer = [[MTRAttributeCacheContainer alloc] init];
     __auto_type * params = [[MTRSubscribeParams alloc] init];
     params.resubscribeIfLost = NO;
+    params.replaceExistingSubscriptions = NO; // Not strictly needed, but checking that doing this does not
+                                              // affect this subscription erroring out correctly.
     [device subscribeWithQueue:queue
         minInterval:1
         maxInterval:2
@@ -1272,6 +1274,7 @@ - (void)test015_FailedSubscribeWithQueueAcrossShutdown
 
     // Create second subscription which will cancel the first subscription.  We
     // can use a non-existent path here to cut down on the work that gets done.
+    params.replaceExistingSubscriptions = YES;
     [device subscribeAttributeWithEndpointId:@10000
                                    clusterId:@6
                                  attributeId:@0
@@ -1330,6 +1333,7 @@ - (void)test016_FailedSubscribeWithCacheReadDuringFailure
 
     // Create second subscription which will cancel the first subscription.  We
     // can use a non-existent path here to cut down on the work that gets done.
+    params.replaceExistingSubscriptions = YES;
     [device subscribeAttributeWithEndpointId:@10000
                                    clusterId:@6
                                  attributeId:@0
@@ -1366,6 +1370,67 @@ - (void)test017_TestMTRDeviceBasics
     [self waitForExpectations:@[ subscriptionExpectation ] timeout:60];
 }
 
+- (void)test018_SubscriptionErrorWhenNotResubscribing
+{
+#if MANUAL_INDIVIDUAL_TEST
+    [self initStack];
+    [self waitForCommissionee];
+#endif
+    MTRBaseDevice * device = GetConnectedDevice();
+    dispatch_queue_t queue = dispatch_get_main_queue();
+
+    XCTestExpectation * firstSubscribeExpectation = [self expectationWithDescription:@"First subscription complete"];
+    XCTestExpectation * errorExpectation = [self expectationWithDescription:@"First subscription errored out"];
+
+    // Subscribe
+    MTRSubscribeParams * params = [[MTRSubscribeParams alloc] initWithMinInterval:@(1) maxInterval:@(10)];
+    params.resubscribeIfLost = NO;
+    params.replaceExistingSubscriptions = NO; // Not strictly needed, but checking that doing this does not
+                                              // affect this subscription erroring out correctly.
+    __block BOOL subscriptionEstablished = NO;
+    [device subscribeToAttributesWithEndpointID:@1
+        clusterID:@6
+        attributeID:@0
+        params:params
+        queue:queue
+        reportHandler:^(id _Nullable values, NSError * _Nullable error) {
+            if (subscriptionEstablished) {
+                // We should only get an error here.
+                XCTAssertNil(values);
+                XCTAssertNotNil(error);
+                [errorExpectation fulfill];
+            } else {
+                XCTAssertNotNil(values);
+                XCTAssertNil(error);
+            }
+        }
+        subscriptionEstablished:^{
+            NSLog(@"subscribe attribute: OnOff established");
+            XCTAssertFalse(subscriptionEstablished);
+            subscriptionEstablished = YES;
+            [firstSubscribeExpectation fulfill];
+        }];
+
+    // Wait till establishment
+    [self waitForExpectations:@[ firstSubscribeExpectation ] timeout:kTimeoutInSeconds];
+
+    // Create second subscription which will cancel the first subscription.  We
+    // can use a non-existent path here to cut down on the work that gets done.
+    params.replaceExistingSubscriptions = YES;
+    [device subscribeAttributeWithEndpointId:@10000
+                                   clusterId:@6
+                                 attributeId:@0
+                                 minInterval:@(1)
+                                 maxInterval:@(2)
+                                      params:params
+                                 clientQueue:queue
+                               reportHandler:^(id _Nullable values, NSError * _Nullable error) {
+                               }
+                     subscriptionEstablished:^() {
+                     }];
+    [self waitForExpectations:@[ errorExpectation ] timeout:60];
+}
+
 - (void)test900_SubscribeAllAttributes
 {
 #if MANUAL_INDIVIDUAL_TEST