From 4683747c4f9c93133f6b5a9cb561521e39ef2a11 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Thu, 23 Feb 2023 21:23:27 -0500 Subject: [PATCH] Make the error handling in readAttributesWithEndpointID more reasonable. (#25203) * Make the error handling in readAttributesWithEndpointID more reasonable. * Apply suggestions from code review Co-authored-by: Karsten Sperling <113487422+ksperling-apple@users.noreply.github.com> * Address review comments. * Addressing more review comments. --------- Co-authored-by: Karsten Sperling <113487422+ksperling-apple@users.noreply.github.com> --- src/darwin/Framework/CHIP/MTRBaseDevice.h | 10 +++ src/darwin/Framework/CHIP/MTRBaseDevice.mm | 79 ++++++++----------- .../Framework/CHIP/MTRDeviceController+XPC.h | 9 +++ .../Framework/CHIP/MTRDeviceController+XPC.mm | 45 +++++++++++ .../CHIP/MTRDeviceControllerXPCConnection.mm | 4 +- .../Framework/CHIPTests/MTRDeviceTests.m | 51 +++++++++--- .../Framework/CHIPTests/MTRErrorTestUtils.mm | 2 +- .../CHIPTests/MTRXPCListenerSampleTests.m | 65 ++++++++------- .../Framework/CHIPTests/MTRXPCProtocolTests.m | 5 +- 9 files changed, 177 insertions(+), 93 deletions(-) diff --git a/src/darwin/Framework/CHIP/MTRBaseDevice.h b/src/darwin/Framework/CHIP/MTRBaseDevice.h index b43145c27852ba..c1bb4cf96297dd 100644 --- a/src/darwin/Framework/CHIP/MTRBaseDevice.h +++ b/src/darwin/Framework/CHIP/MTRBaseDevice.h @@ -215,6 +215,11 @@ typedef NS_ENUM(uint8_t, MTRTransportType) { * * A non-nil attributeID along with a nil clusterID will only succeed if the * attribute ID is for a global attribute that applies to all clusters. + * + * The completion will be called with an error if the entire read interaction fails. + * Otherwise it will be called with values, which may be empty (e.g. if no paths + * matched the wildcard) or may include per-path errors if particular paths + * failed. */ - (void)readAttributesWithEndpointID:(NSNumber * _Nullable)endpointID clusterID:(NSNumber * _Nullable)clusterID @@ -344,6 +349,11 @@ typedef NS_ENUM(uint8_t, MTRTransportType) { * * If all of endpointID, clusterID, eventID are nil, all events on the * device will be read. + * + * The completion will be called with an error if the entire read interaction fails. + * Otherwise it will be called with values, which may be empty (e.g. if no paths + * matched the wildcard) or may include per-path errors if particular paths + * failed. */ - (void)readEventsWithEndpointID:(NSNumber * _Nullable)endpointID diff --git a/src/darwin/Framework/CHIP/MTRBaseDevice.mm b/src/darwin/Framework/CHIP/MTRBaseDevice.mm index c55be43d5538ac..ea47734e3facf6 100644 --- a/src/darwin/Framework/CHIP/MTRBaseDevice.mm +++ b/src/darwin/Framework/CHIP/MTRBaseDevice.mm @@ -847,22 +847,22 @@ - (void)readAttributesWithEndpointID:(NSNumber * _Nullable)endpointID auto * bridge = new MTRDataValueDictionaryCallbackBridge(queue, completion, ^(ExchangeManager & exchangeManager, const SessionHandle & session, MTRDataValueDictionaryCallback successCb, MTRErrorCallback failureCb, MTRCallbackBridgeBase * bridge) { + // interactionStatus tracks whether the whole read interaction has failed. + // + // Make sure interactionStatus survives even if this block scope is destroyed. + auto interactionStatus = std::make_shared(CHIP_NO_ERROR); + auto resultArray = [[NSMutableArray alloc] init]; - auto resultSuccess = [[NSMutableArray alloc] init]; - auto resultFailure = [[NSMutableArray alloc] init]; - auto onSuccessCb = [resultArray, resultSuccess](const app::ConcreteClusterPath & clusterPath, const uint32_t aValueId, + auto onSuccessCb = [resultArray](const app::ConcreteClusterPath & clusterPath, const uint32_t aValueId, const MTRDataValueDictionaryDecodableType & aData) { app::ConcreteAttributePath attribPath(clusterPath.mEndpointId, clusterPath.mClusterId, aValueId); [resultArray addObject:@ { MTRAttributePathKey : [[MTRAttributePath alloc] initWithPath:attribPath], MTRDataKey : aData.GetDecodedObject() }]; - if ([resultSuccess count] == 0) { - [resultSuccess addObject:[NSNumber numberWithBool:YES]]; - } }; - auto onFailureCb = [resultArray, resultFailure]( + auto onFailureCb = [resultArray, interactionStatus]( const app::ConcreteClusterPath * clusterPath, const uint32_t aValueId, CHIP_ERROR aError) { if (clusterPath) { app::ConcreteAttributePath attribPath(clusterPath->mEndpointId, clusterPath->mClusterId, aValueId); @@ -870,8 +870,11 @@ - (void)readAttributesWithEndpointID:(NSNumber * _Nullable)endpointID MTRAttributePathKey : [[MTRAttributePath alloc] initWithPath:attribPath], MTRErrorKey : [MTRError errorForCHIPErrorCode:aError] }]; - } else if ([resultFailure count] == 0) { - [resultFailure addObject:[MTRError errorForCHIPErrorCode:aError]]; + } else { + // This will only happen once per read interaction, and + // after that there will be no more calls to onFailureCb or + // onSuccessCb. + *interactionStatus = aError; } }; @@ -893,24 +896,14 @@ - (void)readAttributesWithEndpointID:(NSNumber * _Nullable)endpointID readParams.mpAttributePathParamsList = &attributePath; readParams.mAttributePathParamsListSize = 1; - auto onDone = [resultArray, resultSuccess, resultFailure, bridge, successCb, failureCb]( + auto onDone = [resultArray, interactionStatus, bridge, successCb, failureCb]( BufferedReadClientCallback * callback) { - if ([resultFailure count] > 0 || [resultSuccess count] == 0) { + if (*interactionStatus != CHIP_NO_ERROR) { // Failure - if (failureCb) { - if ([resultFailure count] > 0) { - failureCb(bridge, [MTRError errorToCHIPErrorCode:resultFailure[0]]); - } else if ([resultArray count] > 0) { - failureCb(bridge, [MTRError errorToCHIPErrorCode:resultArray[0][MTRErrorKey]]); - } else { - failureCb(bridge, CHIP_ERROR_READ_FAILED); - } - } + failureCb(bridge, *interactionStatus); } else { // Success - if (successCb) { - successCb(bridge, resultArray); - } + successCb(bridge, resultArray); } chip::Platform::Delete(callback); }; @@ -1483,22 +1476,22 @@ - (void)readEventsWithEndpointID:(NSNumber * _Nullable)endpointID auto * bridge = new MTRDataValueDictionaryCallbackBridge(queue, completion, ^(ExchangeManager & exchangeManager, const SessionHandle & session, MTRDataValueDictionaryCallback successCb, MTRErrorCallback failureCb, MTRCallbackBridgeBase * bridge) { + // interactionStatus tracks whether the whole read interaction has failed. + // + // Make sure interactionStatus survives even if this block scope is destroyed. + auto interactionStatus = std::make_shared(CHIP_NO_ERROR); + auto resultArray = [[NSMutableArray alloc] init]; - auto resultSuccess = [[NSMutableArray alloc] init]; - auto resultFailure = [[NSMutableArray alloc] init]; - auto onSuccessCb = [resultArray, resultSuccess](const app::ConcreteClusterPath & clusterPath, const uint32_t aValueId, + auto onSuccessCb = [resultArray](const app::ConcreteClusterPath & clusterPath, const uint32_t aValueId, const MTRDataValueDictionaryDecodableType & aData) { app::ConcreteEventPath eventPath(clusterPath.mEndpointId, clusterPath.mClusterId, aValueId); [resultArray addObject:@ { MTREventPathKey : [[MTREventPath alloc] initWithPath:eventPath], MTRDataKey : aData.GetDecodedObject() }]; - if ([resultSuccess count] == 0) { - [resultSuccess addObject:[NSNumber numberWithBool:YES]]; - } }; - auto onFailureCb = [resultArray, resultFailure]( + auto onFailureCb = [resultArray, interactionStatus]( const app::ConcreteClusterPath * clusterPath, const uint32_t aValueId, CHIP_ERROR aError) { if (clusterPath) { app::ConcreteEventPath eventPath(clusterPath->mEndpointId, clusterPath->mClusterId, aValueId); @@ -1506,8 +1499,11 @@ - (void)readEventsWithEndpointID:(NSNumber * _Nullable)endpointID MTREventPathKey : [[MTREventPath alloc] initWithPath:eventPath], MTRErrorKey : [MTRError errorForCHIPErrorCode:aError] }]; - } else if ([resultFailure count] == 0) { - [resultFailure addObject:[MTRError errorForCHIPErrorCode:aError]]; + } else { + // This will only happen once per read interaction, and + // after that there will be no more calls to onFailureCb or + // onSuccessCb. + *interactionStatus = aError; } }; @@ -1529,24 +1525,13 @@ - (void)readEventsWithEndpointID:(NSNumber * _Nullable)endpointID readParams.mpEventPathParamsList = &eventPath; readParams.mEventPathParamsListSize = 1; - auto onDone = [resultArray, resultSuccess, resultFailure, bridge, successCb, failureCb]( + auto onDone = [resultArray, interactionStatus, bridge, successCb, failureCb]( BufferedReadClientCallback * callback) { - if ([resultFailure count] > 0 || [resultSuccess count] == 0) { + if (*interactionStatus != CHIP_NO_ERROR) { // Failure - if (failureCb) { - if ([resultFailure count] > 0) { - failureCb(bridge, [MTRError errorToCHIPErrorCode:resultFailure[0]]); - } else if ([resultArray count] > 0) { - failureCb(bridge, [MTRError errorToCHIPErrorCode:resultArray[0][MTRErrorKey]]); - } else { - failureCb(bridge, CHIP_ERROR_READ_FAILED); - } - } + failureCb(bridge, *interactionStatus); } else { - // Success - if (successCb) { - successCb(bridge, resultArray); - } + successCb(bridge, resultArray); } chip::Platform::Delete(callback); }; diff --git a/src/darwin/Framework/CHIP/MTRDeviceController+XPC.h b/src/darwin/Framework/CHIP/MTRDeviceController+XPC.h index c2af9c108bf71d..230fb59ca17a05 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceController+XPC.h +++ b/src/darwin/Framework/CHIP/MTRDeviceController+XPC.h @@ -76,6 +76,15 @@ typedef void (^MTRValuesHandler)(id _Nullable values, NSError * _Nullable error) */ + (MTRSubscribeParams * _Nullable)decodeXPCSubscribeParams:(NSDictionary * _Nullable)params; +/** + * Returns an NSXPCInterface configured for MTRDeviceControllerServerProtocol. + */ ++ (NSXPCInterface *)xpcInterfaceForServerProtocol MTR_NEWLY_AVAILABLE; + +/** + * Returns an NSXPCInterface configured for MTRDeviceControllerClientProtocol. + */ ++ (NSXPCInterface *)xpcInterfaceForClientProtocol MTR_NEWLY_AVAILABLE; @end /** diff --git a/src/darwin/Framework/CHIP/MTRDeviceController+XPC.mm b/src/darwin/Framework/CHIP/MTRDeviceController+XPC.mm index 84ca14d6e2f2ef..8d8ee93f2282e4 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceController+XPC.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceController+XPC.mm @@ -29,6 +29,16 @@ static NSString * const kMinIntervalKey = @"minInterval"; static NSString * const kMaxIntervalKey = @"maxInterval"; +// Classes that are used by MTRDevice responses. In particular, needs to +// include NSError. +static NSSet * GetXPCAllowedClasses() +{ + static NSSet * const sXPCAllowedClasses = [NSSet setWithArray:@[ + [NSString class], [NSNumber class], [NSData class], [NSArray class], [NSDictionary class], [NSError class] + ]]; + return sXPCAllowedClasses; +} + static NSArray * _Nullable encodeAttributePath(MTRAttributePath * _Nullable path) { if (!path) { @@ -190,6 +200,41 @@ + (MTRSubscribeParams * _Nullable)decodeXPCSubscribeParams:(NSDictionary 0); @@ -933,6 +955,7 @@ - (void)test011_ReadCachedAttribute XCTAssertEqual([path.endpoint unsignedShortValue], 1); XCTAssertEqual([path.cluster unsignedLongValue], 6); XCTAssertEqual([path.attribute unsignedLongValue], 0); + XCTAssertNotNil(values[0][@"data"]); XCTAssertNil(values[0][@"error"]); XCTAssertTrue([values[0][@"data"][@"type"] isEqualToString:@"Boolean"]); XCTAssertEqual([values[0][@"data"][@"value"] boolValue], NO); @@ -957,6 +980,7 @@ - (void)test011_ReadCachedAttribute XCTAssertEqual([path.cluster unsignedLongValue], 6); XCTAssertEqual([path.attribute unsignedLongValue], 0); XCTAssertNil(value[@"error"]); + XCTAssertNotNil(value[@"data"]); } [cacheExpectation fulfill]; }]; @@ -978,6 +1002,8 @@ - (void)test011_ReadCachedAttribute MTRAttributePath * path = value[@"attributePath"]; XCTAssertEqual([path.endpoint unsignedShortValue], 1); XCTAssertEqual([path.attribute unsignedLongValue], 0); + XCTAssertNil(value[@"error"]); + XCTAssertNotNil(value[@"data"]); } [cacheExpectation fulfill]; }]; @@ -1000,6 +1026,7 @@ - (void)test011_ReadCachedAttribute XCTAssertEqual([path.endpoint unsignedShortValue], 1); XCTAssertEqual([path.cluster unsignedLongValue], 6); XCTAssertNil(value[@"error"]); + XCTAssertNotNil(value[@"data"]); } [cacheExpectation fulfill]; }]; diff --git a/src/darwin/Framework/CHIPTests/MTRErrorTestUtils.mm b/src/darwin/Framework/CHIPTests/MTRErrorTestUtils.mm index 24ee02dd7d12bc..f9911e93dedd8a 100644 --- a/src/darwin/Framework/CHIPTests/MTRErrorTestUtils.mm +++ b/src/darwin/Framework/CHIPTests/MTRErrorTestUtils.mm @@ -34,7 +34,7 @@ + (uint8_t)errorToZCLErrorCode:(NSError * _Nullable)error return EMBER_ZCL_STATUS_SUCCESS; } - if (error.domain != MTRInteractionErrorDomain) { + if (![error.domain isEqualToString:MTRInteractionErrorDomain]) { return EMBER_ZCL_STATUS_FAILURE; } diff --git a/src/darwin/Framework/CHIPTests/MTRXPCListenerSampleTests.m b/src/darwin/Framework/CHIPTests/MTRXPCListenerSampleTests.m index 0f658adb5380fd..15131ffd3027ba 100644 --- a/src/darwin/Framework/CHIPTests/MTRXPCListenerSampleTests.m +++ b/src/darwin/Framework/CHIPTests/MTRXPCListenerSampleTests.m @@ -82,8 +82,8 @@ @implementation MTRXPCListenerSample - (instancetype)init { if ([super init]) { - _serviceInterface = [NSXPCInterface interfaceWithProtocol:@protocol(MTRDeviceControllerServerProtocol)]; - _clientInterface = [NSXPCInterface interfaceWithProtocol:@protocol(MTRDeviceControllerClientProtocol)]; + _serviceInterface = [MTRDeviceController xpcInterfaceForServerProtocol]; + _clientInterface = [MTRDeviceController xpcInterfaceForClientProtocol]; _servers = [NSMutableDictionary dictionary]; _clusterStateCacheDictionary = [NSMutableDictionary dictionary]; _xpcListener = [NSXPCListener anonymousListener]; @@ -789,11 +789,25 @@ - (void)test005_ReadAttributeFailure completion:^(id _Nullable values, NSError * _Nullable error) { NSLog(@"read attribute: DeviceType values: %@, error: %@", values, error); - XCTAssertNil(values); - // Error is copied over XPC and hence cannot use MTRErrorTestUtils utility which checks against - // a local domain string object. - XCTAssertTrue([error.domain isEqualToString:MTRInteractionErrorDomain]); - XCTAssertEqual(error.code, EMBER_ZCL_STATUS_UNSUPPORTED_CLUSTER); + XCTAssertNil(error); + XCTAssertNotNil(values); + + { + XCTAssertTrue([values isKindOfClass:[NSArray class]]); + NSArray * resultArray = values; + XCTAssertEqual([resultArray count], 1); + NSDictionary * result = resultArray[0]; + + MTRAttributePath * path = result[@"attributePath"]; + XCTAssertEqual(path.endpoint.unsignedIntegerValue, 0); + XCTAssertEqual(path.cluster.unsignedIntegerValue, 10000); + XCTAssertEqual(path.attribute.unsignedIntegerValue, 0); + XCTAssertNotNil(result[@"error"]); + XCTAssertNil(result[@"data"]); + XCTAssertTrue([result[@"error"] isKindOfClass:[NSError class]]); + XCTAssertEqual([MTRErrorTestUtils errorToZCLErrorCode:result[@"error"]], + EMBER_ZCL_STATUS_UNSUPPORTED_CLUSTER); + } [expectation fulfill]; }]; @@ -814,23 +828,21 @@ - (void)test006_WriteAttributeFailure NSDictionary * writeValue = [NSDictionary dictionaryWithObjectsAndKeys:@"UnsignedInteger", @"type", [NSNumber numberWithUnsignedInteger:200], @"value", nil]; - [device writeAttributeWithEndpointID:@1 - clusterID:@8 - attributeID:@10000 - value:writeValue - timedWriteTimeout:nil - queue:queue - completion:^(id _Nullable values, NSError * _Nullable error) { - NSLog(@"write attribute: Brightness values: %@, error: %@", values, error); + [device + writeAttributeWithEndpointID:@1 + clusterID:@8 + attributeID:@10000 + value:writeValue + timedWriteTimeout:nil + queue:queue + completion:^(id _Nullable values, NSError * _Nullable error) { + NSLog(@"write attribute: Brightness values: %@, error: %@", values, error); - XCTAssertNil(values); - // Error is copied over XPC and hence cannot use MTRErrorTestUtils utility which checks - // against a local domain string object. - XCTAssertTrue([error.domain isEqualToString:MTRInteractionErrorDomain]); - XCTAssertEqual(error.code, EMBER_ZCL_STATUS_UNSUPPORTED_ATTRIBUTE); + XCTAssertNil(values); + XCTAssertEqual([MTRErrorTestUtils errorToZCLErrorCode:error], EMBER_ZCL_STATUS_UNSUPPORTED_ATTRIBUTE); - [expectation fulfill]; - }]; + [expectation fulfill]; + }]; [self waitForExpectationsWithTimeout:kTimeoutInSeconds handler:nil]; } @@ -867,9 +879,7 @@ - (void)test007_InvokeCommandFailure NSLog(@"invoke command: MoveToLevelWithOnOff values: %@, error: %@", values, error); XCTAssertNil(values); - // Error is copied over XPC and hence cannot use MTRErrorTestUtils utility which checks against a local domain string object. - XCTAssertTrue([error.domain isEqualToString:MTRInteractionErrorDomain]); - XCTAssertEqual(error.code, EMBER_ZCL_STATUS_UNSUPPORTED_COMMAND); + XCTAssertEqual([MTRErrorTestUtils errorToZCLErrorCode:error], EMBER_ZCL_STATUS_UNSUPPORTED_COMMAND); [expectation fulfill]; }]; @@ -892,10 +902,7 @@ - (void)test008_SubscribeFailure // Because our subscription has no existent paths, it gets an // InvalidAction response, which is EMBER_ZCL_STATUS_MALFORMED_COMMAND. XCTAssertNil(values); - // Error is copied over XPC and hence cannot use MTRErrorTestUtils utility which checks against a local domain string - // object. - XCTAssertTrue([error.domain isEqualToString:MTRInteractionErrorDomain]); - XCTAssertEqual(error.code, EMBER_ZCL_STATUS_MALFORMED_COMMAND); + XCTAssertEqual([MTRErrorTestUtils errorToZCLErrorCode:error], EMBER_ZCL_STATUS_MALFORMED_COMMAND); [errorReportExpectation fulfill]; }; diff --git a/src/darwin/Framework/CHIPTests/MTRXPCProtocolTests.m b/src/darwin/Framework/CHIPTests/MTRXPCProtocolTests.m index aac3da2ce493db..753adc6bddfe81 100644 --- a/src/darwin/Framework/CHIPTests/MTRXPCProtocolTests.m +++ b/src/darwin/Framework/CHIPTests/MTRXPCProtocolTests.m @@ -324,8 +324,9 @@ - (void)setUp _xpcListener = [NSXPCListener anonymousListener]; [_xpcListener setDelegate:(id) self]; - _serviceInterface = [NSXPCInterface interfaceWithProtocol:@protocol(MTRDeviceControllerServerProtocol)]; - _clientInterface = [NSXPCInterface interfaceWithProtocol:@protocol(MTRDeviceControllerClientProtocol)]; + _serviceInterface = [MTRDeviceController xpcInterfaceForServerProtocol]; + _clientInterface = [MTRDeviceController xpcInterfaceForClientProtocol]; + [_xpcListener resume]; _controllerUUID = [[NSUUID UUID] UUIDString]; _remoteDeviceController =