From 9201620f3f0c366bb5dd66fcc7b61a079a7b326f Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Thu, 27 Jan 2022 14:59:20 -0500 Subject: [PATCH] Enable persistent storage on Darwin in Server.h. (#14342) There were two remaining issues in the Darwin KVS impl that needed to be fixed to get this to pass: 1) The errors returned did not match the API documentation, which caused some consumers to fail. The most important problem here was returning CHIP_ERROR_KEY_NOT_FOUND instead of CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND from _Get, but I went ahead and aligned the other error cases (not initialized, buffer passed to _Get too small, deleting unknown key, failures to save to persistent storage) with the API documentation. 2) In _Put, if the item already existed the change of "value" was not being picked up by the NSManagedObject machinery, so updating the value of an already-existing key did not work. The fix for this is to use @dynamic instead of @synthesize for the properties of KeyValueItem (shamelessly copy/pasting from the documentation at https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/CoreData/LifeofaManagedObject.html), which I _think_ causes the getter/setter to be provided by the NSMnagedObject bits, which then know about the set we perform and know to update the data store. Fixes https://github.com/project-chip/connectedhomeip/issues/12174 --- src/app/server/BUILD.gn | 7 ---- src/app/server/Server.h | 7 ---- .../Darwin/KeyValueStoreManagerImpl.mm | 42 ++++++++++++++----- 3 files changed, 31 insertions(+), 25 deletions(-) diff --git a/src/app/server/BUILD.gn b/src/app/server/BUILD.gn index 380b0d6413d1aa..7a9d332d888b9f 100644 --- a/src/app/server/BUILD.gn +++ b/src/app/server/BUILD.gn @@ -31,13 +31,6 @@ config("server_config") { if (chip_enable_group_messaging_tests) { defines += [ "CHIP_ENABLE_GROUP_MESSAGING_TESTS" ] } - - if (current_os == "mac" || current_os == "ios") { - # Using Non persistent storage delegate since - # persistence is also disable in Unit test for linux and MacOS - # Issue #12174 for Darwin - defines += [ "CHIP_USE_NON_PERSISTENT_STORAGE_DELEGATE" ] - } } static_library("server") { diff --git a/src/app/server/Server.h b/src/app/server/Server.h index 46258222705953..161fa579233459 100644 --- a/src/app/server/Server.h +++ b/src/app/server/Server.h @@ -28,7 +28,6 @@ #include #include #include -#include #include #include #include @@ -190,13 +189,7 @@ class Server // Both PersistentStorageDelegate, and GroupDataProvider should be injected by the applications // See: https://github.com/project-chip/connectedhomeip/issues/12276 - // Currently, the GroupDataProvider cannot use KeyValueStoreMgr() due to - // (https://github.com/project-chip/connectedhomeip/issues/12174) -#ifdef CHIP_USE_NON_PERSISTENT_STORAGE_DELEGATE - TestPersistentStorageDelegate mDeviceStorage; -#else DeviceStorageDelegate mDeviceStorage; -#endif Credentials::GroupDataProviderImpl mGroupsProvider; app::DefaultAttributePersistenceProvider mAttributePersister; GroupDataProviderListener mListener; diff --git a/src/platform/Darwin/KeyValueStoreManagerImpl.mm b/src/platform/Darwin/KeyValueStoreManagerImpl.mm index 95f6b46d5b1094..44ca337782578b 100644 --- a/src/platform/Darwin/KeyValueStoreManagerImpl.mm +++ b/src/platform/Darwin/KeyValueStoreManagerImpl.mm @@ -43,16 +43,14 @@ @interface KeyValueItem : NSManagedObject @implementation KeyValueItem -@synthesize key; -@synthesize value; +@dynamic key; +@dynamic value; -- (instancetype)initWithContext:(nonnull NSManagedObjectContext *)context - key:(nonnull NSString *)key_ - value:(nonnull NSData *)value_ +- (instancetype)initWithContext:(nonnull NSManagedObjectContext *)context key:(nonnull NSString *)key value:(nonnull NSData *)value { if (self = [super initWithContext:context]) { - key = key_; - value = value_; + self.key = key; + self.value = value; } return self; } @@ -209,10 +207,11 @@ - (instancetype)initWithContext:(nonnull NSManagedObjectContext *)context { ReturnErrorCodeIf(key == nullptr, CHIP_ERROR_INVALID_ARGUMENT); ReturnErrorCodeIf(offset != 0, CHIP_ERROR_INVALID_ARGUMENT); + ReturnErrorCodeIf(gContext == nullptr, CHIP_ERROR_WELL_UNINITIALIZED); KeyValueItem * item = FindItemForKey([[NSString alloc] initWithUTF8String:key], nil, true); if (!item) { - return CHIP_ERROR_KEY_NOT_FOUND; + return CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND; } if (read_bytes_size != nullptr) { @@ -221,6 +220,17 @@ - (instancetype)initWithContext:(nonnull NSManagedObjectContext *)context if (value != nullptr) { memcpy(value, item.value.bytes, std::min((item.value.length), value_size)); +#if CHIP_CONFIG_DARWIN_STORAGE_VERBOSE_LOGGING + fprintf(stderr, "GETTING VALUE FOR: '%s': ", key); + for (size_t i = 0; i < std::min((item.value.length), value_size); ++i) { + fprintf(stderr, "%02x ", static_cast(value)[i]); + } + fprintf(stderr, "\n"); +#endif + } + + if (item.value.length > value_size) { + return CHIP_ERROR_BUFFER_TOO_SMALL; } return CHIP_NO_ERROR; @@ -229,10 +239,11 @@ - (instancetype)initWithContext:(nonnull NSManagedObjectContext *)context CHIP_ERROR KeyValueStoreManagerImpl::_Delete(const char * key) { ReturnErrorCodeIf(key == nullptr, CHIP_ERROR_INVALID_ARGUMENT); + ReturnErrorCodeIf(gContext == nullptr, CHIP_ERROR_WELL_UNINITIALIZED); KeyValueItem * item = FindItemForKey([[NSString alloc] initWithUTF8String:key], nil); if (!item) { - return CHIP_NO_ERROR; + return CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND; } __block BOOL success = NO; @@ -244,7 +255,7 @@ - (instancetype)initWithContext:(nonnull NSManagedObjectContext *)context if (!success) { ChipLogError(DeviceLayer, "Error saving context: %s", error.localizedDescription.UTF8String); - return CHIP_ERROR_INTERNAL; + return CHIP_ERROR_PERSISTED_STORAGE_FAILED; } return CHIP_NO_ERROR; @@ -253,6 +264,7 @@ - (instancetype)initWithContext:(nonnull NSManagedObjectContext *)context CHIP_ERROR KeyValueStoreManagerImpl::_Put(const char * key, const void * value, size_t value_size) { ReturnErrorCodeIf(key == nullptr, CHIP_ERROR_INVALID_ARGUMENT); + ReturnErrorCodeIf(gContext == nullptr, CHIP_ERROR_WELL_UNINITIALIZED); NSData * data = [[NSData alloc] initWithBytes:value length:value_size]; @@ -274,9 +286,17 @@ - (instancetype)initWithContext:(nonnull NSManagedObjectContext *)context if (!success) { ChipLogError(DeviceLayer, "Error saving context: %s", error.localizedDescription.UTF8String); - return CHIP_ERROR_INTERNAL; + return CHIP_ERROR_PERSISTED_STORAGE_FAILED; } +#if CHIP_CONFIG_DARWIN_STORAGE_VERBOSE_LOGGING + fprintf(stderr, "PUT VALUE FOR: '%s': ", key); + for (size_t i = 0; i < value_size; ++i) { + fprintf(stderr, "%02x ", static_cast(value)[i]); + } + fprintf(stderr, "\n"); +#endif + return CHIP_NO_ERROR; }