From 7bed92fc5bd6da4bc94b6ba283cb74228aa7ad04 Mon Sep 17 00:00:00 2001 From: Sam Deane Date: Tue, 22 Jun 2021 17:14:48 +0100 Subject: [PATCH 01/18] Started stubbing tombstone support. --- Core/PARStore.m | 33 ++++++++++++++++++++++++++------- 1 file changed, 26 insertions(+), 7 deletions(-) diff --git a/Core/PARStore.m b/Core/PARStore.m index e23b781..84f9037 100644 --- a/Core/PARStore.m +++ b/Core/PARStore.m @@ -1383,6 +1383,16 @@ - (BOOL)writeBlobFromPath:(NSString *)sourcePath toPath:(NSString *)targetSubpat } - (BOOL)deleteBlobAtPath:(NSString *)path error:(NSError **)error +{ + return [self deleteBlobAtPath:path usingTombstone: NO error:error]; +} + +- (BOOL)writeTombstoneAtPath:(NSString *)tombstonePath forFileAtPath:(NSString *)filePath error:(NSError **)error +{ + return YES; +} + +- (BOOL)deleteBlobAtPath:(NSString *)path usingTombstone: (BOOL)usingTombstone error:(NSError **)error { // nil path = error if (path == nil) @@ -1407,15 +1417,24 @@ - (BOOL)deleteBlobAtPath:(NSString *)path error:(NSError **)error // otherwise blobs are stored in a special blob directory __block NSError *localError = nil; NSURL *fileURL = [[self blobDirectoryURL] URLByAppendingPathComponent:path]; + NSURL *tombstoneURL = [fileURL URLByAppendingPathExtension:@"deleted"]; + NSError *coordinatorError = nil; - [[self newFileCoordinator] coordinateWritingItemAtURL:fileURL options:NSFileCoordinatorWritingForReplacing error:&coordinatorError byAccessor:^(NSURL *newURL) + + [[self newFileCoordinator] coordinateWritingItemAtURL:fileURL options:NSFileCoordinatorWritingForReplacing writingItemAtURL:tombstoneURL options:NSFileCoordinatorWritingForReplacing error:&coordinatorError byAccessor:^(NSURL * _Nonnull newURL, NSURL * _Nonnull newTombstoneURL) { - // write to disk (overwrite any file that was at that same path before) - NSError *error = nil; - BOOL success = [[NSFileManager defaultManager] removeItemAtURL:fileURL error:&error]; - if (!success) - localError = [NSError errorWithObject:self code:__LINE__ localizedDescription:[NSString stringWithFormat:@"Could not delete data blob at path '%@'", newURL.path] underlyingError:error]; - }]; + // write tombstone + NSError *error = nil; + BOOL success = [self writeTombstoneAtPath:newTombstoneURL.path forFileAtPath:newURL.path error:&error]; + if (!success) { + localError = [NSError errorWithObject:self code:__LINE__ localizedDescription:[NSString stringWithFormat:@"Could not create tombstone for data blob at path '%@'", newURL.path] underlyingError:error]; + } else { + // write to disk (overwrite any file that was at that same path before) + success = [[NSFileManager defaultManager] removeItemAtURL:fileURL error:&error]; + if (!success) + localError = [NSError errorWithObject:self code:__LINE__ localizedDescription:[NSString stringWithFormat:@"Could not delete data blob at path '%@'", newURL.path] underlyingError:error]; + } + }]; // error handling if (coordinatorError && !localError) From 44e4d374a32d20c804e1943c19557b659cc972e8 Mon Sep 17 00:00:00 2001 From: Sam Deane Date: Tue, 22 Jun 2021 17:55:51 +0100 Subject: [PATCH 02/18] Added blob deletion tests --- Core/PARStore.h | 1 + Core/PARStore.m | 10 +++++- PARStore.xcodeproj/project.pbxproj | 6 ++++ Tests/PARStoreBlobTests.m | 57 ++++++++++++++++++++++++++++++ 4 files changed, 73 insertions(+), 1 deletion(-) create mode 100644 Tests/PARStoreBlobTests.m diff --git a/Core/PARStore.h b/Core/PARStore.h index 1b9ed99..00b81f2 100644 --- a/Core/PARStore.h +++ b/Core/PARStore.h @@ -68,6 +68,7 @@ extern NSString *PARStoreDidSyncNotification; - (BOOL)writeBlobFromPath:(NSString *)sourcePath toPath:(NSString *)path error:(NSError **)error; - (nullable NSData *)blobDataAtPath:(NSString *)path error:(NSError **)error; - (BOOL)deleteBlobAtPath:(NSString *)path error:(NSError **)error; +- (BOOL)deleteBlobAtPath:(NSString *)path usingTombstone: (BOOL)usingTombstone error:(NSError **)error; - (nullable NSString *)absolutePathForBlobPath:(NSString *)path; - (NSArray *)absolutePathsForBlobsPrefixedBy:(NSString *)prefix NS_SWIFT_NAME(absolutePaths(forBlobsPrefixedBy:)); - (void)enumerateBlobs:(void(^)(NSString *path))block; diff --git a/Core/PARStore.m b/Core/PARStore.m index 84f9037..a235cf0 100644 --- a/Core/PARStore.m +++ b/Core/PARStore.m @@ -1389,7 +1389,15 @@ - (BOOL)deleteBlobAtPath:(NSString *)path error:(NSError **)error - (BOOL)writeTombstoneAtPath:(NSString *)tombstonePath forFileAtPath:(NSString *)filePath error:(NSError **)error { - return YES; + if ([[NSFileManager defaultManager] fileExistsAtPath:tombstonePath]) { + // if a tombstone already exists, we need not make it again + return YES; + } + + // the tombstone contains the original modification date and file size, along with the current time + NSDictionary* attributes = [[NSFileManager defaultManager] attributesOfItemAtPath:filePath error:error]; + NSDictionary* properties = @{ @"modified": attributes.fileModificationDate, @"size": @(attributes.fileSize), @"deleted": [NSDate date] }; + return [properties writeToFile:tombstonePath atomically:YES]; } - (BOOL)deleteBlobAtPath:(NSString *)path usingTombstone: (BOOL)usingTombstone error:(NSError **)error diff --git a/PARStore.xcodeproj/project.pbxproj b/PARStore.xcodeproj/project.pbxproj index b9eeb7f..fb2a055 100644 --- a/PARStore.xcodeproj/project.pbxproj +++ b/PARStore.xcodeproj/project.pbxproj @@ -46,6 +46,8 @@ 56EAE1BB16E24E7300A7F31F /* main.m in Sources */ = {isa = PBXBuildFile; fileRef = 56EAE1B816E24E7300A7F31F /* main.m */; }; 56EAE1BE16E24EAA00A7F31F /* PARStore.m in Sources */ = {isa = PBXBuildFile; fileRef = 56EAE1BD16E24EAA00A7F31F /* PARStore.m */; }; 56FA3D771970359C00BF81D3 /* libsqlite3.dylib in Frameworks */ = {isa = PBXBuildFile; fileRef = 56FA3D761970359C00BF81D3 /* libsqlite3.dylib */; }; + 8BED6AEB26824978004639DF /* PARStoreBlobTests.m in Sources */ = {isa = PBXBuildFile; fileRef = 8BED6AEA26824978004639DF /* PARStoreBlobTests.m */; }; + 8BED6AEC26824978004639DF /* PARStoreBlobTests.m in Sources */ = {isa = PBXBuildFile; fileRef = 8BED6AEA26824978004639DF /* PARStoreBlobTests.m */; }; /* End PBXBuildFile section */ /* Begin PBXContainerItemProxy section */ @@ -114,6 +116,7 @@ 56EAE1BC16E24EAA00A7F31F /* PARStore.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name = PARStore.h; path = Core/PARStore.h; sourceTree = SOURCE_ROOT; }; 56EAE1BD16E24EAA00A7F31F /* PARStore.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; lineEnding = 0; name = PARStore.m; path = Core/PARStore.m; sourceTree = SOURCE_ROOT; xcLanguageSpecificationIdentifier = xcode.lang.objc; }; 56FA3D761970359C00BF81D3 /* libsqlite3.dylib */ = {isa = PBXFileReference; lastKnownFileType = "compiled.mach-o.dylib"; name = libsqlite3.dylib; path = usr/lib/libsqlite3.dylib; sourceTree = SDKROOT; }; + 8BED6AEA26824978004639DF /* PARStoreBlobTests.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = PARStoreBlobTests.m; sourceTree = ""; }; /* End PBXFileReference section */ /* Begin PBXFrameworksBuildPhase section */ @@ -265,6 +268,7 @@ 56C7EDFA16E26D0700FFBBF2 /* PARTestCase.h */, 56C7EDFB16E26D0700FFBBF2 /* PARTestCase.m */, 56EAE19416E24C7500A7F31F /* PARStoreTests.m */, + 8BED6AEA26824978004639DF /* PARStoreBlobTests.m */, 561148E6196E952800F488F2 /* PARSQLiteTests.m */, 56E196E41B448BE600A51AB0 /* PARDispatchQueueTests.m */, 56EAE18E16E24C7500A7F31F /* Supporting Files */, @@ -456,6 +460,7 @@ isa = PBXSourcesBuildPhase; buildActionMask = 2147483647; files = ( + 8BED6AEC26824978004639DF /* PARStoreBlobTests.m in Sources */, 56C7EDF716E2687F00FFBBF2 /* PARStoreTests.m in Sources */, 56C7EDFD16E26D0700FFBBF2 /* PARTestCase.m in Sources */, ); @@ -480,6 +485,7 @@ buildActionMask = 2147483647; files = ( 56EAE19516E24C7500A7F31F /* PARStoreTests.m in Sources */, + 8BED6AEB26824978004639DF /* PARStoreBlobTests.m in Sources */, 56E196E61B448CA300A51AB0 /* PARDispatchQueueTests.m in Sources */, 56C7EDFC16E26D0700FFBBF2 /* PARTestCase.m in Sources */, 561148E7196E952800F488F2 /* PARSQLiteTests.m in Sources */, diff --git a/Tests/PARStoreBlobTests.m b/Tests/PARStoreBlobTests.m new file mode 100644 index 0000000..f1aaa44 --- /dev/null +++ b/Tests/PARStoreBlobTests.m @@ -0,0 +1,57 @@ +// PARStoreBlobTests +// Created by Sam Deane on 22/06/21. +// All code (c) 2021 - present day, Elegant Chaos Limited. + +#import "PARTestCase.h" +#import "PARStoreExample.h" +#import "PARNotificationSemaphore.h" + +@interface PARStoreBlobTests : PARTestCase + +@end + + +@implementation PARStoreBlobTests + +- (void)setUp +{ + [super setUp]; + + // Set-up code here. +} + +- (void)tearDown +{ + // Tear-down code here. + + [super tearDown]; +} + +- (NSString *)deviceIdentifierForTest +{ + return @"948E9EEE-3398-4DD7-9183-C56866EF2350"; +} + +#pragma mark - + +- (void)testDeletion +{ + NSError *error = nil; + NSURL *url = [[self urlWithUniqueTmpDirectory] URLByAppendingPathComponent:@"doc.parstore"]; + PARStore *store = [PARStore storeWithURL:url deviceIdentifier:[self deviceIdentifierForTest]]; + XCTAssertTrue([store writeBlobData:[@"test" dataUsingEncoding:NSUTF8StringEncoding] toPath:@"blob" error:&error]); + XCTAssertTrue([store deleteBlobAtPath:@"blob" error:&error]); + [store tearDownNow]; +} + +- (void)testDeletionWithTombstone +{ + NSError *error = nil; + NSURL *url = [[self urlWithUniqueTmpDirectory] URLByAppendingPathComponent:@"doc.parstore"]; + PARStore *store = [PARStore storeWithURL:url deviceIdentifier:[self deviceIdentifierForTest]]; + XCTAssertTrue([store writeBlobData:[@"test" dataUsingEncoding:NSUTF8StringEncoding] toPath:@"blob" error:&error]); + XCTAssertTrue([store deleteBlobAtPath:@"blob" usingTombstone: YES error:&error]); + [store tearDownNow]; +} + +@end From f5ea6c79ef84deaf94ef2760362122400d457665 Mon Sep 17 00:00:00 2001 From: Sam Deane Date: Wed, 23 Jun 2021 15:54:02 +0100 Subject: [PATCH 03/18] Improved deletion tests --- Tests/PARStoreBlobTests.m | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/Tests/PARStoreBlobTests.m b/Tests/PARStoreBlobTests.m index f1aaa44..5cc336a 100644 --- a/Tests/PARStoreBlobTests.m +++ b/Tests/PARStoreBlobTests.m @@ -40,7 +40,15 @@ - (void)testDeletion NSURL *url = [[self urlWithUniqueTmpDirectory] URLByAppendingPathComponent:@"doc.parstore"]; PARStore *store = [PARStore storeWithURL:url deviceIdentifier:[self deviceIdentifierForTest]]; XCTAssertTrue([store writeBlobData:[@"test" dataUsingEncoding:NSUTF8StringEncoding] toPath:@"blob" error:&error]); + + NSURL *blobURL = [[url URLByAppendingPathComponent: @"Blobs"] URLByAppendingPathComponent: @"blob"]; + XCTAssertTrue([[NSFileManager defaultManager] fileExistsAtPath: blobURL.path]); + XCTAssertTrue([store deleteBlobAtPath:@"blob" error:&error]); + + // blob file should have gone + XCTAssertFalse([[NSFileManager defaultManager] fileExistsAtPath: blobURL.path]); + [store tearDownNow]; } @@ -50,7 +58,19 @@ - (void)testDeletionWithTombstone NSURL *url = [[self urlWithUniqueTmpDirectory] URLByAppendingPathComponent:@"doc.parstore"]; PARStore *store = [PARStore storeWithURL:url deviceIdentifier:[self deviceIdentifierForTest]]; XCTAssertTrue([store writeBlobData:[@"test" dataUsingEncoding:NSUTF8StringEncoding] toPath:@"blob" error:&error]); + + NSURL *blobURL = [[url URLByAppendingPathComponent: @"Blobs"] URLByAppendingPathComponent: @"blob"]; + XCTAssertTrue([[NSFileManager defaultManager] fileExistsAtPath: blobURL.path]); + XCTAssertTrue([store deleteBlobAtPath:@"blob" usingTombstone: YES error:&error]); + + // blob file should have gone + XCTAssertFalse([[NSFileManager defaultManager] fileExistsAtPath: blobURL.path]); + + // tombstone file should have appeared in its place + NSURL *tombstoneURL = [blobURL URLByAppendingPathExtension: @"deleted"]; + XCTAssertTrue([[NSFileManager defaultManager] fileExistsAtPath: tombstoneURL.path]); + [store tearDownNow]; } From e148d847552d369d105008f2bbf9ffdb18e69caf Mon Sep 17 00:00:00 2001 From: Sam Deane Date: Wed, 23 Jun 2021 16:19:04 +0100 Subject: [PATCH 04/18] Added constant for tombstone extension --- Core/PARStore.m | 9 ++++++++- Tests/PARStoreBlobTests.m | 33 +++++++++++++++++++++++++-------- 2 files changed, 33 insertions(+), 9 deletions(-) diff --git a/Core/PARStore.m b/Core/PARStore.m index a235cf0..9c2fae3 100644 --- a/Core/PARStore.m +++ b/Core/PARStore.m @@ -30,6 +30,8 @@ NSString *const TimestampAttributeName = @"timestamp"; NSString *const ParentTimestampAttributeName = @"parentTimestamp"; +// tombstone constants +NSString *const TombstoneFileExtension = @"deleted"; // A subclass of NSFileCoordinator that doesn't use coordination. // This is used to disable coordination, for a performance boost when it is not needed. @@ -1387,6 +1389,11 @@ - (BOOL)deleteBlobAtPath:(NSString *)path error:(NSError **)error return [self deleteBlobAtPath:path usingTombstone: NO error:error]; } +- (NSString *)tombstonePathForBlobAtPath:(NSString *)path +{ + return [path stringByAppendingPathExtension: TombstoneFileExtension]; +} + - (BOOL)writeTombstoneAtPath:(NSString *)tombstonePath forFileAtPath:(NSString *)filePath error:(NSError **)error { if ([[NSFileManager defaultManager] fileExistsAtPath:tombstonePath]) { @@ -1425,7 +1432,7 @@ - (BOOL)deleteBlobAtPath:(NSString *)path usingTombstone: (BOOL)usingTombstone e // otherwise blobs are stored in a special blob directory __block NSError *localError = nil; NSURL *fileURL = [[self blobDirectoryURL] URLByAppendingPathComponent:path]; - NSURL *tombstoneURL = [fileURL URLByAppendingPathExtension:@"deleted"]; + NSURL *tombstoneURL = [fileURL URLByAppendingPathExtension:TombstoneFileExtension]; NSError *coordinatorError = nil; diff --git a/Tests/PARStoreBlobTests.m b/Tests/PARStoreBlobTests.m index 5cc336a..06a1845 100644 --- a/Tests/PARStoreBlobTests.m +++ b/Tests/PARStoreBlobTests.m @@ -6,6 +6,8 @@ #import "PARStoreExample.h" #import "PARNotificationSemaphore.h" +extern NSString *const TombstoneFileExtension; // normally private, but exposed for testing + @interface PARStoreBlobTests : PARTestCase @end @@ -41,13 +43,13 @@ - (void)testDeletion PARStore *store = [PARStore storeWithURL:url deviceIdentifier:[self deviceIdentifierForTest]]; XCTAssertTrue([store writeBlobData:[@"test" dataUsingEncoding:NSUTF8StringEncoding] toPath:@"blob" error:&error]); - NSURL *blobURL = [[url URLByAppendingPathComponent: @"Blobs"] URLByAppendingPathComponent: @"blob"]; - XCTAssertTrue([[NSFileManager defaultManager] fileExistsAtPath: blobURL.path]); + NSString *blobPath = [store absolutePathForBlobPath: @"blob"]; + XCTAssertTrue([[NSFileManager defaultManager] fileExistsAtPath: blobPath]); XCTAssertTrue([store deleteBlobAtPath:@"blob" error:&error]); // blob file should have gone - XCTAssertFalse([[NSFileManager defaultManager] fileExistsAtPath: blobURL.path]); + XCTAssertFalse([[NSFileManager defaultManager] fileExistsAtPath: blobPath]); [store tearDownNow]; } @@ -59,19 +61,34 @@ - (void)testDeletionWithTombstone PARStore *store = [PARStore storeWithURL:url deviceIdentifier:[self deviceIdentifierForTest]]; XCTAssertTrue([store writeBlobData:[@"test" dataUsingEncoding:NSUTF8StringEncoding] toPath:@"blob" error:&error]); - NSURL *blobURL = [[url URLByAppendingPathComponent: @"Blobs"] URLByAppendingPathComponent: @"blob"]; - XCTAssertTrue([[NSFileManager defaultManager] fileExistsAtPath: blobURL.path]); + NSString *blobPath = [store absolutePathForBlobPath: @"blob"]; + XCTAssertTrue([[NSFileManager defaultManager] fileExistsAtPath: blobPath]); XCTAssertTrue([store deleteBlobAtPath:@"blob" usingTombstone: YES error:&error]); // blob file should have gone - XCTAssertFalse([[NSFileManager defaultManager] fileExistsAtPath: blobURL.path]); + XCTAssertFalse([[NSFileManager defaultManager] fileExistsAtPath: blobPath]); // tombstone file should have appeared in its place - NSURL *tombstoneURL = [blobURL URLByAppendingPathExtension: @"deleted"]; - XCTAssertTrue([[NSFileManager defaultManager] fileExistsAtPath: tombstoneURL.path]); + NSString *tombstonePath = [blobPath stringByAppendingPathExtension: TombstoneFileExtension]; + XCTAssertTrue([[NSFileManager defaultManager] fileExistsAtPath: tombstonePath]); [store tearDownNow]; } +- (void)testTombstonePreventsData +{ + NSError *error = nil; + NSURL *url = [[self urlWithUniqueTmpDirectory] URLByAppendingPathComponent:@"doc.parstore"]; + PARStore *store = [PARStore storeWithURL:url deviceIdentifier:[self deviceIdentifierForTest]]; + XCTAssertTrue([store writeBlobData:[@"test" dataUsingEncoding:NSUTF8StringEncoding] toPath:@"blob" error:&error]); + + +} + +- (void)testTombstonePreventsEnumeration +{ + +} + @end From 423577173c3ff960eec3279f9d088a3fcb7f7490 Mon Sep 17 00:00:00 2001 From: Sam Deane Date: Wed, 23 Jun 2021 16:44:18 +0100 Subject: [PATCH 05/18] Don't return data and don't enumerate files where a tombstone is present. --- Core/PARStore.m | 49 ++++++++++++++++++++++++++------------- Tests/PARStoreBlobTests.m | 23 ++++++++++++++++++ 2 files changed, 56 insertions(+), 16 deletions(-) diff --git a/Core/PARStore.m b/Core/PARStore.m index 9c2fae3..42de899 100644 --- a/Core/PARStore.m +++ b/Core/PARStore.m @@ -1389,11 +1389,6 @@ - (BOOL)deleteBlobAtPath:(NSString *)path error:(NSError **)error return [self deleteBlobAtPath:path usingTombstone: NO error:error]; } -- (NSString *)tombstonePathForBlobAtPath:(NSString *)path -{ - return [path stringByAppendingPathExtension: TombstoneFileExtension]; -} - - (BOOL)writeTombstoneAtPath:(NSString *)tombstonePath forFileAtPath:(NSString *)filePath error:(NSError **)error { if ([[NSFileManager defaultManager] fileExistsAtPath:tombstonePath]) { @@ -1490,6 +1485,18 @@ - (NSData *)blobDataAtPath:(NSString *)path error:(NSError **)error; // otherwise blobs are stored in a special blob directory __block NSError *localError = nil; NSURL *fileURL = [[self blobDirectoryURL] URLByAppendingPathComponent:path]; + + // check first for a tombstone, which indicates that the + // blob had been deleted and should be ignored + NSURL *tombstoneURL = [fileURL URLByAppendingPathExtension:TombstoneFileExtension]; + if ([[NSFileManager defaultManager] fileExistsAtPath: tombstoneURL.path]) { + NSString *description = [NSString stringWithFormat:@"Blob at path '%@' has a tombstone, indicating that it has been deleted.", self.storeURL.path]; + ErrorLog(@"%@", description); + if (error != NULL) + *error = [NSError errorWithObject:self code:__LINE__ localizedDescription:description underlyingError:nil]; + return nil; + } + NSError *coordinatorError = nil; __block NSData *data = nil; [[self newFileCoordinator] coordinateReadingItemAtURL:fileURL options:NSFileCoordinatorReadingWithoutChanges error:&coordinatorError byAccessor:^(NSURL *newURL) @@ -1543,26 +1550,36 @@ - (void)enumerateBlobs:(void(^)(NSString *path))block } else { - __block NSArray *urls; + __block NSMutableArray *urls = [NSMutableArray array]; + __block NSMutableSet *tombstones = [NSMutableSet set]; [[self newFileCoordinator] coordinateReadingItemAtURL:[self blobDirectoryURL] options:NSFileCoordinatorReadingWithoutChanges error:NULL byAccessor:^(NSURL *newURL) { NSDirectoryEnumerator *enumerator = [[NSFileManager defaultManager] enumeratorAtURL:[self blobDirectoryURL] includingPropertiesForKeys:nil options:(NSDirectoryEnumerationSkipsPackageDescendants|NSDirectoryEnumerationSkipsHiddenFiles|NSDirectoryEnumerationSkipsSubdirectoryDescendants) errorHandler:nil]; NSFileManager *fileManager = [[NSFileManager alloc] init]; - urls = [enumerator.allObjects filteredArrayUsingPredicate:[NSPredicate predicateWithBlock:^BOOL(id object, NSDictionary *bindings) { - NSURL *url = object; + for(NSURL* url in enumerator) { BOOL isDir = false; - return [fileManager fileExistsAtPath:url.path isDirectory:&isDir] && !isDir; - }]]; + if ([fileManager fileExistsAtPath:url.path isDirectory:&isDir] && !isDir) continue; + if (url.pathExtension == TombstoneFileExtension) { + [tombstones addObject:[url URLByDeletingPathExtension]]; + } else { + [urls addObject:url]; + } + } }]; NSUInteger prefixLength = self.blobDirectoryURL.path.length+1; // +1 is for the last slash for (NSURL *url in urls) { - // Resolving symbolic link here, because on iOS at least, the directory enumerator - // uses a sym linked "private" folder, causing the path to be different to what comes - // out for the blobDirectoryURL. - NSString *absolutePath = [url URLByResolvingSymlinksInPath].path; - NSString *relativePath = [absolutePath substringFromIndex:prefixLength]; - block(relativePath); + // the presence of a tombstone file will suppress enumeration of the + // corresponding data file if it still exists + // (this shouldn't happen, but might do if file-syncing messes things up) + if (![tombstones containsObject:url]) { + // Resolving symbolic link here, because on iOS at least, the directory enumerator + // uses a sym linked "private" folder, causing the path to be different to what comes + // out for the blobDirectoryURL. + NSString *absolutePath = [url URLByResolvingSymlinksInPath].path; + NSString *relativePath = [absolutePath substringFromIndex:prefixLength]; + block(relativePath); + } } } } diff --git a/Tests/PARStoreBlobTests.m b/Tests/PARStoreBlobTests.m index 06a1845..c6638ef 100644 --- a/Tests/PARStoreBlobTests.m +++ b/Tests/PARStoreBlobTests.m @@ -83,12 +83,35 @@ - (void)testTombstonePreventsData PARStore *store = [PARStore storeWithURL:url deviceIdentifier:[self deviceIdentifierForTest]]; XCTAssertTrue([store writeBlobData:[@"test" dataUsingEncoding:NSUTF8StringEncoding] toPath:@"blob" error:&error]); + // fake the presence of a tombstone, to simulate a situation where a partial + // synchronisation has caused it to exist along with the blob + NSString *tombstonePath = [[store absolutePathForBlobPath: @"blob"] stringByAppendingPathExtension: TombstoneFileExtension]; + [[@"foobar" dataUsingEncoding:NSUTF8StringEncoding] writeToFile:tombstonePath atomically:YES]; + XCTAssertNil([store blobDataAtPath:@"blob" error:&error]); + + [store tearDownNow]; } - (void)testTombstonePreventsEnumeration { + NSError *error = nil; + NSURL *url = [[self urlWithUniqueTmpDirectory] URLByAppendingPathComponent:@"doc.parstore"]; + PARStore *store = [PARStore storeWithURL:url deviceIdentifier:[self deviceIdentifierForTest]]; + XCTAssertTrue([store writeBlobData:[@"test" dataUsingEncoding:NSUTF8StringEncoding] toPath:@"blob" error:&error]); + + // fake the presence of a tombstone, to simulate a situation where a partial + // synchronisation has caused it to exist along with the blob + NSString *tombstonePath = [[store absolutePathForBlobPath: @"blob"] stringByAppendingPathExtension: TombstoneFileExtension]; + [[@"foobar" dataUsingEncoding:NSUTF8StringEncoding] writeToFile:tombstonePath atomically:YES]; + __block int count = 0; + [store enumerateBlobs:^(NSString *blobPath) { + ++count; + }]; + XCTAssertEqual(count, 0); + + [store tearDownNow]; } @end From 53a2a8377007e6587fbd00894d2581af4dc41d4a Mon Sep 17 00:00:00 2001 From: Sam Deane Date: Wed, 23 Jun 2021 16:44:46 +0100 Subject: [PATCH 06/18] Renamed tests. --- Tests/PARStoreBlobTests.m | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Tests/PARStoreBlobTests.m b/Tests/PARStoreBlobTests.m index c6638ef..d3753a9 100644 --- a/Tests/PARStoreBlobTests.m +++ b/Tests/PARStoreBlobTests.m @@ -76,7 +76,7 @@ - (void)testDeletionWithTombstone [store tearDownNow]; } -- (void)testTombstonePreventsData +- (void)testTombstoneSuppressesData { NSError *error = nil; NSURL *url = [[self urlWithUniqueTmpDirectory] URLByAppendingPathComponent:@"doc.parstore"]; @@ -93,7 +93,7 @@ - (void)testTombstonePreventsData [store tearDownNow]; } -- (void)testTombstonePreventsEnumeration +- (void)testTombstoneSuppressesEnumeration { NSError *error = nil; NSURL *url = [[self urlWithUniqueTmpDirectory] URLByAppendingPathComponent:@"doc.parstore"]; From 403b36e14b85c985a481a238fba8d107c580e47d Mon Sep 17 00:00:00 2001 From: Sam Deane Date: Thu, 24 Jun 2021 11:27:47 +0100 Subject: [PATCH 07/18] comment tweak --- Core/PARStore.m | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Core/PARStore.m b/Core/PARStore.m index 42de899..e40a961 100644 --- a/Core/PARStore.m +++ b/Core/PARStore.m @@ -1571,7 +1571,7 @@ - (void)enumerateBlobs:(void(^)(NSString *path))block for (NSURL *url in urls) { // the presence of a tombstone file will suppress enumeration of the // corresponding data file if it still exists - // (this shouldn't happen, but might do if file-syncing messes things up) + // (this shouldn't happen, but might do if external file-syncing messes things up) if (![tombstones containsObject:url]) { // Resolving symbolic link here, because on iOS at least, the directory enumerator // uses a sym linked "private" folder, causing the path to be different to what comes From 8c73d6962010da4194a4e80888e927d5d9b1978b Mon Sep 17 00:00:00 2001 From: Sam Deane Date: Mon, 28 Jun 2021 12:30:46 +0100 Subject: [PATCH 08/18] Added blobExistsAtPath --- Core/PARStore.h | 1 + Core/PARStore.m | 18 +++++++++++++++++- Tests/PARStoreBlobTests.m | 35 +++++++++++++++++++++++++++++++++++ 3 files changed, 53 insertions(+), 1 deletion(-) diff --git a/Core/PARStore.h b/Core/PARStore.h index 00b81f2..3468dbc 100644 --- a/Core/PARStore.h +++ b/Core/PARStore.h @@ -72,6 +72,7 @@ extern NSString *PARStoreDidSyncNotification; - (nullable NSString *)absolutePathForBlobPath:(NSString *)path; - (NSArray *)absolutePathsForBlobsPrefixedBy:(NSString *)prefix NS_SWIFT_NAME(absolutePaths(forBlobsPrefixedBy:)); - (void)enumerateBlobs:(void(^)(NSString *path))block; +- (BOOL)blobExistsAtPath:(NSString *)path; /// @name Syncing - (void)sync; diff --git a/Core/PARStore.m b/Core/PARStore.m index e40a961..96a8f22 100644 --- a/Core/PARStore.m +++ b/Core/PARStore.m @@ -1391,7 +1391,8 @@ - (BOOL)deleteBlobAtPath:(NSString *)path error:(NSError **)error - (BOOL)writeTombstoneAtPath:(NSString *)tombstonePath forFileAtPath:(NSString *)filePath error:(NSError **)error { - if ([[NSFileManager defaultManager] fileExistsAtPath:tombstonePath]) { + if ([[NSFileManager defaultManager] fileExistsAtPath:tombstonePath]) + { // if a tombstone already exists, we need not make it again return YES; } @@ -1402,6 +1403,21 @@ - (BOOL)writeTombstoneAtPath:(NSString *)tombstonePath forFileAtPath:(NSString * return [properties writeToFile:tombstonePath atomically:YES]; } + +- (BOOL)blobExistsAtPath:(NSString *)path +{ + NSURL *blobURL = [[self blobDirectoryURL] URLByAppendingPathComponent:path]; + + // if a tombstone file exists, return NO, regardless of the presence of the actual file + NSURL* tombstoneURL = [blobURL URLByAppendingPathExtension:TombstoneFileExtension]; + if ([[NSFileManager defaultManager] fileExistsAtPath:tombstoneURL.path]) + { + return NO; + } + + return [[NSFileManager defaultManager] fileExistsAtPath:blobURL.path]; +} + - (BOOL)deleteBlobAtPath:(NSString *)path usingTombstone: (BOOL)usingTombstone error:(NSError **)error { // nil path = error diff --git a/Tests/PARStoreBlobTests.m b/Tests/PARStoreBlobTests.m index d3753a9..ca53501 100644 --- a/Tests/PARStoreBlobTests.m +++ b/Tests/PARStoreBlobTests.m @@ -47,6 +47,7 @@ - (void)testDeletion XCTAssertTrue([[NSFileManager defaultManager] fileExistsAtPath: blobPath]); XCTAssertTrue([store deleteBlobAtPath:@"blob" error:&error]); + XCTAssertFalse([store blobExistsAtPath:@"blob"]); // blob file should have gone XCTAssertFalse([[NSFileManager defaultManager] fileExistsAtPath: blobPath]); @@ -65,6 +66,7 @@ - (void)testDeletionWithTombstone XCTAssertTrue([[NSFileManager defaultManager] fileExistsAtPath: blobPath]); XCTAssertTrue([store deleteBlobAtPath:@"blob" usingTombstone: YES error:&error]); + XCTAssertFalse([store blobExistsAtPath:@"blob"]); // blob file should have gone XCTAssertFalse([[NSFileManager defaultManager] fileExistsAtPath: blobPath]); @@ -76,6 +78,39 @@ - (void)testDeletionWithTombstone [store tearDownNow]; } + +- (void)testBlobExists +{ + NSError *error = nil; + NSURL *url = [[self urlWithUniqueTmpDirectory] URLByAppendingPathComponent:@"doc.parstore"]; + PARStore *store = [PARStore storeWithURL:url deviceIdentifier:[self deviceIdentifierForTest]]; + + XCTAssertFalse([store blobExistsAtPath:@"blob"]); + + XCTAssertTrue([store writeBlobData:[@"test" dataUsingEncoding:NSUTF8StringEncoding] toPath:@"blob" error:&error]); + XCTAssertTrue([store blobExistsAtPath:@"blob"]); + + [store tearDownNow]; +} + +- (void)testTombstoneSuppressesFileExistence +{ + NSError *error = nil; + NSURL *url = [[self urlWithUniqueTmpDirectory] URLByAppendingPathComponent:@"doc.parstore"]; + PARStore *store = [PARStore storeWithURL:url deviceIdentifier:[self deviceIdentifierForTest]]; + XCTAssertTrue([store writeBlobData:[@"test" dataUsingEncoding:NSUTF8StringEncoding] toPath:@"blob" error:&error]); + XCTAssertTrue([store blobExistsAtPath:@"blob"]); + + // fake the presence of a tombstone, to simulate a situation where a partial + // synchronisation has caused it to exist along with the blob + NSString *tombstonePath = [[store absolutePathForBlobPath: @"blob"] stringByAppendingPathExtension: TombstoneFileExtension]; + [[@"foobar" dataUsingEncoding:NSUTF8StringEncoding] writeToFile:tombstonePath atomically:YES]; + + XCTAssertFalse([store blobExistsAtPath:@"blob"]); + + [store tearDownNow]; +} + - (void)testTombstoneSuppressesData { NSError *error = nil; From 091f510f0b3064a87944245a8d972cdc2d7cb2c4 Mon Sep 17 00:00:00 2001 From: Sam Deane Date: Mon, 28 Jun 2021 13:39:20 +0100 Subject: [PATCH 09/18] Honour the usingTombstone flag. --- Core/PARStore.m | 29 +++++++++++++++++++---------- Tests/PARStoreBlobTests.m | 4 ++++ 2 files changed, 23 insertions(+), 10 deletions(-) diff --git a/Core/PARStore.m b/Core/PARStore.m index 96a8f22..82abd8d 100644 --- a/Core/PARStore.m +++ b/Core/PARStore.m @@ -1384,11 +1384,6 @@ - (BOOL)writeBlobFromPath:(NSString *)sourcePath toPath:(NSString *)targetSubpat return YES; } -- (BOOL)deleteBlobAtPath:(NSString *)path error:(NSError **)error -{ - return [self deleteBlobAtPath:path usingTombstone: NO error:error]; -} - - (BOOL)writeTombstoneAtPath:(NSString *)tombstonePath forFileAtPath:(NSString *)filePath error:(NSError **)error { if ([[NSFileManager defaultManager] fileExistsAtPath:tombstonePath]) @@ -1418,6 +1413,11 @@ - (BOOL)blobExistsAtPath:(NSString *)path return [[NSFileManager defaultManager] fileExistsAtPath:blobURL.path]; } +- (BOOL)deleteBlobAtPath:(NSString *)path error:(NSError **)error +{ + return [self deleteBlobAtPath:path usingTombstone: NO error:error]; +} + - (BOOL)deleteBlobAtPath:(NSString *)path usingTombstone: (BOOL)usingTombstone error:(NSError **)error { // nil path = error @@ -1449,13 +1449,22 @@ - (BOOL)deleteBlobAtPath:(NSString *)path usingTombstone: (BOOL)usingTombstone e [[self newFileCoordinator] coordinateWritingItemAtURL:fileURL options:NSFileCoordinatorWritingForReplacing writingItemAtURL:tombstoneURL options:NSFileCoordinatorWritingForReplacing error:&coordinatorError byAccessor:^(NSURL * _Nonnull newURL, NSURL * _Nonnull newTombstoneURL) { - // write tombstone + // TODO: Decide how to interpret existing tombstone file. + // The previous behaviour produced an error if the file had already been deleted. + // We could do this with the tombstone deletion too, or we could treat the presence of the + // tombstone as a sign that the file was deleted already and just quietly return success. + NSError *error = nil; - BOOL success = [self writeTombstoneAtPath:newTombstoneURL.path forFileAtPath:newURL.path error:&error]; + + BOOL success = !usingTombstone; if (!success) { - localError = [NSError errorWithObject:self code:__LINE__ localizedDescription:[NSString stringWithFormat:@"Could not create tombstone for data blob at path '%@'", newURL.path] underlyingError:error]; - } else { - // write to disk (overwrite any file that was at that same path before) + // write tombstone + success = [self writeTombstoneAtPath:newTombstoneURL.path forFileAtPath:newURL.path error:&error]; + if (!success) + localError = [NSError errorWithObject:self code:__LINE__ localizedDescription:[NSString stringWithFormat:@"Could not create tombstone for data blob at path '%@'", newURL.path] underlyingError:error]; + } + + if (success) { success = [[NSFileManager defaultManager] removeItemAtURL:fileURL error:&error]; if (!success) localError = [NSError errorWithObject:self code:__LINE__ localizedDescription:[NSString stringWithFormat:@"Could not delete data blob at path '%@'", newURL.path] underlyingError:error]; diff --git a/Tests/PARStoreBlobTests.m b/Tests/PARStoreBlobTests.m index ca53501..5c4300c 100644 --- a/Tests/PARStoreBlobTests.m +++ b/Tests/PARStoreBlobTests.m @@ -52,6 +52,10 @@ - (void)testDeletion // blob file should have gone XCTAssertFalse([[NSFileManager defaultManager] fileExistsAtPath: blobPath]); + // tombstone file should not have appeared in its place + NSString *tombstonePath = [blobPath stringByAppendingPathExtension: TombstoneFileExtension]; + XCTAssertFalse([[NSFileManager defaultManager] fileExistsAtPath: tombstonePath]); + [store tearDownNow]; } From 98bf01eecd17c7c923a6132c22051ea175f9f0fb Mon Sep 17 00:00:00 2001 From: Sam Deane Date: Mon, 28 Jun 2021 13:43:22 +0100 Subject: [PATCH 10/18] Added comments about potential cleanup. --- Core/PARStore.m | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/Core/PARStore.m b/Core/PARStore.m index 82abd8d..d5a0e30 100644 --- a/Core/PARStore.m +++ b/Core/PARStore.m @@ -1449,7 +1449,7 @@ - (BOOL)deleteBlobAtPath:(NSString *)path usingTombstone: (BOOL)usingTombstone e [[self newFileCoordinator] coordinateWritingItemAtURL:fileURL options:NSFileCoordinatorWritingForReplacing writingItemAtURL:tombstoneURL options:NSFileCoordinatorWritingForReplacing error:&coordinatorError byAccessor:^(NSURL * _Nonnull newURL, NSURL * _Nonnull newTombstoneURL) { - // TODO: Decide how to interpret existing tombstone file. + // TODO: Decide how to interpret an existing tombstone file. // The previous behaviour produced an error if the file had already been deleted. // We could do this with the tombstone deletion too, or we could treat the presence of the // tombstone as a sign that the file was deleted already and just quietly return success. @@ -1519,6 +1519,9 @@ - (NSData *)blobDataAtPath:(NSString *)path error:(NSError **)error; ErrorLog(@"%@", description); if (error != NULL) *error = [NSError errorWithObject:self code:__LINE__ localizedDescription:description underlyingError:nil]; + + // TODO: should we attempt to clean up sync errors here, by deleting the data file if it still exists? + return nil; } @@ -1604,7 +1607,11 @@ - (void)enumerateBlobs:(void(^)(NSString *path))block NSString *absolutePath = [url URLByResolvingSymlinksInPath].path; NSString *relativePath = [absolutePath substringFromIndex:prefixLength]; block(relativePath); - } + } /* else { + // TODO: if thshould we attempt to clean up sync errors here, by deleting the data file if it still exists? + }*/ + + } } } From fdd8bfa0378e1553d20e41c35d7794593df82e0a Mon Sep 17 00:00:00 2001 From: Sam Deane Date: Mon, 28 Jun 2021 13:49:00 +0100 Subject: [PATCH 11/18] Added a note about double-deletion. --- Core/PARStore.m | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/Core/PARStore.m b/Core/PARStore.m index d5a0e30..22a85b0 100644 --- a/Core/PARStore.m +++ b/Core/PARStore.m @@ -1449,10 +1449,13 @@ - (BOOL)deleteBlobAtPath:(NSString *)path usingTombstone: (BOOL)usingTombstone e [[self newFileCoordinator] coordinateWritingItemAtURL:fileURL options:NSFileCoordinatorWritingForReplacing writingItemAtURL:tombstoneURL options:NSFileCoordinatorWritingForReplacing error:&coordinatorError byAccessor:^(NSURL * _Nonnull newURL, NSURL * _Nonnull newTombstoneURL) { - // TODO: Decide how to interpret an existing tombstone file. - // The previous behaviour produced an error if the file had already been deleted. - // We could do this with the tombstone deletion too, or we could treat the presence of the - // tombstone as a sign that the file was deleted already and just quietly return success. + // TODO: Decide how to interpret a tombstone file being present already. + // With the previous implementation, I think that calling delete on a file that didn't exist + // (or had already been deleted) returned an error. + // To be consistent with this behaviour, we should perhaps also return an error if we + // find an existing tombstone file. + // However, my normal inclination would be to treat the presence of the tombstone as a sign + // that the file has been successfully deleted and quietly return success. NSError *error = nil; From 3b4343defa32e1a2e8d7d6767f538c0cbb54b489 Mon Sep 17 00:00:00 2001 From: Sam Deane Date: Mon, 28 Jun 2021 13:53:54 +0100 Subject: [PATCH 12/18] Added some comments explaining the tests --- Tests/PARStoreBlobTests.m | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/Tests/PARStoreBlobTests.m b/Tests/PARStoreBlobTests.m index 5c4300c..002bcc4 100644 --- a/Tests/PARStoreBlobTests.m +++ b/Tests/PARStoreBlobTests.m @@ -38,6 +38,8 @@ - (NSString *)deviceIdentifierForTest - (void)testDeletion { + // deleting a blob with the old API should work as before + NSError *error = nil; NSURL *url = [[self urlWithUniqueTmpDirectory] URLByAppendingPathComponent:@"doc.parstore"]; PARStore *store = [PARStore storeWithURL:url deviceIdentifier:[self deviceIdentifierForTest]]; @@ -61,6 +63,7 @@ - (void)testDeletion - (void)testDeletionWithTombstone { + // deleting a blob with the new API should result in a tombstone file NSError *error = nil; NSURL *url = [[self urlWithUniqueTmpDirectory] URLByAppendingPathComponent:@"doc.parstore"]; PARStore *store = [PARStore storeWithURL:url deviceIdentifier:[self deviceIdentifierForTest]]; @@ -85,6 +88,8 @@ - (void)testDeletionWithTombstone - (void)testBlobExists { + // exists should product the right result before/after creation of a blob file + NSError *error = nil; NSURL *url = [[self urlWithUniqueTmpDirectory] URLByAppendingPathComponent:@"doc.parstore"]; PARStore *store = [PARStore storeWithURL:url deviceIdentifier:[self deviceIdentifierForTest]]; @@ -99,6 +104,9 @@ - (void)testBlobExists - (void)testTombstoneSuppressesFileExistence { + // if there's a tombstone file present, exists should return NO even if the actual blob + // file is still present + NSError *error = nil; NSURL *url = [[self urlWithUniqueTmpDirectory] URLByAppendingPathComponent:@"doc.parstore"]; PARStore *store = [PARStore storeWithURL:url deviceIdentifier:[self deviceIdentifierForTest]]; @@ -117,6 +125,9 @@ - (void)testTombstoneSuppressesFileExistence - (void)testTombstoneSuppressesData { + // if there is a tombstone file present, no data should be returned even if the actual blob + // file is still present + NSError *error = nil; NSURL *url = [[self urlWithUniqueTmpDirectory] URLByAppendingPathComponent:@"doc.parstore"]; PARStore *store = [PARStore storeWithURL:url deviceIdentifier:[self deviceIdentifierForTest]]; @@ -134,6 +145,9 @@ - (void)testTombstoneSuppressesData - (void)testTombstoneSuppressesEnumeration { + // tombstone files shoulnd't be included in the enumeration + // the presence of a tombstone file should also cause the corresponding data file to be skipped + // from the enumeration (if it still exists) NSError *error = nil; NSURL *url = [[self urlWithUniqueTmpDirectory] URLByAppendingPathComponent:@"doc.parstore"]; PARStore *store = [PARStore storeWithURL:url deviceIdentifier:[self deviceIdentifierForTest]]; From 872b52f39d75c85f0522e7bfaa701da2880fdce6 Mon Sep 17 00:00:00 2001 From: Sam Deane Date: Tue, 29 Jun 2021 11:22:55 +0100 Subject: [PATCH 13/18] Updated to reflect Drew's comments --- Core/PARStore.h | 3 +- Core/PARStore.m | 62 ++++++++++++++++++++++++--------------- Tests/PARStoreBlobTests.m | 2 +- 3 files changed, 41 insertions(+), 26 deletions(-) diff --git a/Core/PARStore.h b/Core/PARStore.h index 3468dbc..1aa5abd 100644 --- a/Core/PARStore.h +++ b/Core/PARStore.h @@ -68,11 +68,12 @@ extern NSString *PARStoreDidSyncNotification; - (BOOL)writeBlobFromPath:(NSString *)sourcePath toPath:(NSString *)path error:(NSError **)error; - (nullable NSData *)blobDataAtPath:(NSString *)path error:(NSError **)error; - (BOOL)deleteBlobAtPath:(NSString *)path error:(NSError **)error; -- (BOOL)deleteBlobAtPath:(NSString *)path usingTombstone: (BOOL)usingTombstone error:(NSError **)error; +- (BOOL)deleteBlobAtPath:(NSString *)path registeringDeletion: (BOOL)usingTombstone error:(NSError **)error; - (nullable NSString *)absolutePathForBlobPath:(NSString *)path; - (NSArray *)absolutePathsForBlobsPrefixedBy:(NSString *)prefix NS_SWIFT_NAME(absolutePaths(forBlobsPrefixedBy:)); - (void)enumerateBlobs:(void(^)(NSString *path))block; - (BOOL)blobExistsAtPath:(NSString *)path; +- (BOOL)blobIsRegisteredDeletedAtPath:(NSString *)path; /// @name Syncing - (void)sync; diff --git a/Core/PARStore.m b/Core/PARStore.m index 22a85b0..85a5219 100644 --- a/Core/PARStore.m +++ b/Core/PARStore.m @@ -1413,12 +1413,19 @@ - (BOOL)blobExistsAtPath:(NSString *)path return [[NSFileManager defaultManager] fileExistsAtPath:blobURL.path]; } +- (BOOL)blobIsRegisteredDeletedAtPath:(NSString *)path +{ + NSURL *blobURL = [[self blobDirectoryURL] URLByAppendingPathComponent:path]; + NSURL* tombstoneURL = [blobURL URLByAppendingPathExtension:TombstoneFileExtension]; + return [[NSFileManager defaultManager] fileExistsAtPath:tombstoneURL.path]; +} + - (BOOL)deleteBlobAtPath:(NSString *)path error:(NSError **)error { - return [self deleteBlobAtPath:path usingTombstone: NO error:error]; + return [self deleteBlobAtPath:path registeringDeletion: NO error:error]; } -- (BOOL)deleteBlobAtPath:(NSString *)path usingTombstone: (BOOL)usingTombstone error:(NSError **)error +- (BOOL)deleteBlobAtPath:(NSString *)path registeringDeletion: (BOOL)usingTombstone error:(NSError **)error { // nil path = error if (path == nil) @@ -1449,39 +1456,41 @@ - (BOOL)deleteBlobAtPath:(NSString *)path usingTombstone: (BOOL)usingTombstone e [[self newFileCoordinator] coordinateWritingItemAtURL:fileURL options:NSFileCoordinatorWritingForReplacing writingItemAtURL:tombstoneURL options:NSFileCoordinatorWritingForReplacing error:&coordinatorError byAccessor:^(NSURL * _Nonnull newURL, NSURL * _Nonnull newTombstoneURL) { - // TODO: Decide how to interpret a tombstone file being present already. - // With the previous implementation, I think that calling delete on a file that didn't exist - // (or had already been deleted) returned an error. - // To be consistent with this behaviour, we should perhaps also return an error if we - // find an existing tombstone file. - // However, my normal inclination would be to treat the presence of the tombstone as a sign - // that the file has been successfully deleted and quietly return success. - NSError *error = nil; - - BOOL success = !usingTombstone; - if (!success) { + BOOL success = YES; + + if (usingTombstone) { // write tombstone success = [self writeTombstoneAtPath:newTombstoneURL.path forFileAtPath:newURL.path error:&error]; if (!success) + { localError = [NSError errorWithObject:self code:__LINE__ localizedDescription:[NSString stringWithFormat:@"Could not create tombstone for data blob at path '%@'", newURL.path] underlyingError:error]; + } } - if (success) { + if (success) + { success = [[NSFileManager defaultManager] removeItemAtURL:fileURL error:&error]; if (!success) + { localError = [NSError errorWithObject:self code:__LINE__ localizedDescription:[NSString stringWithFormat:@"Could not delete data blob at path '%@'", newURL.path] underlyingError:error]; + } } }]; // error handling if (coordinatorError && !localError) + { localError = coordinatorError; + } + if (localError) { ErrorLog(@"Error deleting blob: %@", localError); if (error != NULL) + { *error = localError; + } return NO; } return YES; @@ -1521,9 +1530,14 @@ - (NSData *)blobDataAtPath:(NSString *)path error:(NSError **)error; NSString *description = [NSString stringWithFormat:@"Blob at path '%@' has a tombstone, indicating that it has been deleted.", self.storeURL.path]; ErrorLog(@"%@", description); if (error != NULL) + { *error = [NSError errorWithObject:self code:__LINE__ localizedDescription:description underlyingError:nil]; + } - // TODO: should we attempt to clean up sync errors here, by deleting the data file if it still exists? + // we attempt to clean up sync errors here, by deleting the data file if it still exists + // most times this call will fail because the file won't exist, and if it fails for another + // reason there's probably nothing we can do, so we ignore any errors + [[NSFileManager defaultManager] removeItemAtURL:fileURL error:NULL]; return nil; } @@ -1600,21 +1614,21 @@ - (void)enumerateBlobs:(void(^)(NSString *path))block NSUInteger prefixLength = self.blobDirectoryURL.path.length+1; // +1 is for the last slash for (NSURL *url in urls) { - // the presence of a tombstone file will suppress enumeration of the - // corresponding data file if it still exists - // (this shouldn't happen, but might do if external file-syncing messes things up) - if (![tombstones containsObject:url]) { + if ([tombstones containsObject:url]) { + // a tombstone file exists for this data file, indicating that it has been deleted + // we skip it for the enumeration, and we attempt to clean up by deleting the data file + // (we ignore deletion errors as this method doesn't return success/failure, and the + // underlying cause of the mismatch is probably something that needs fixing by a higher + // layer anyway) + [[NSFileManager defaultManager] removeItemAtURL:url error:NULL]; + } else { // Resolving symbolic link here, because on iOS at least, the directory enumerator // uses a sym linked "private" folder, causing the path to be different to what comes // out for the blobDirectoryURL. NSString *absolutePath = [url URLByResolvingSymlinksInPath].path; NSString *relativePath = [absolutePath substringFromIndex:prefixLength]; block(relativePath); - } /* else { - // TODO: if thshould we attempt to clean up sync errors here, by deleting the data file if it still exists? - }*/ - - + } } } } diff --git a/Tests/PARStoreBlobTests.m b/Tests/PARStoreBlobTests.m index 002bcc4..30cd8aa 100644 --- a/Tests/PARStoreBlobTests.m +++ b/Tests/PARStoreBlobTests.m @@ -72,7 +72,7 @@ - (void)testDeletionWithTombstone NSString *blobPath = [store absolutePathForBlobPath: @"blob"]; XCTAssertTrue([[NSFileManager defaultManager] fileExistsAtPath: blobPath]); - XCTAssertTrue([store deleteBlobAtPath:@"blob" usingTombstone: YES error:&error]); + XCTAssertTrue([store deleteBlobAtPath:@"blob" registeringDeletion: YES error:&error]); XCTAssertFalse([store blobExistsAtPath:@"blob"]); // blob file should have gone From 6276465a9c011998146ab9ab1fddb34900e8b83a Mon Sep 17 00:00:00 2001 From: Sam Deane Date: Tue, 29 Jun 2021 12:34:53 +0100 Subject: [PATCH 14/18] Fixed blob enumeration. Improved tests. Added enumeration of tombstones. --- Core/PARStore.h | 5 ++++- Core/PARStore.m | 31 ++++++++++++++++++++++++++++++- Tests/PARStoreBlobTests.m | 39 ++++++++++++++++++++++++++++++++++----- 3 files changed, 68 insertions(+), 7 deletions(-) diff --git a/Core/PARStore.h b/Core/PARStore.h index 1aa5abd..dbc03c2 100644 --- a/Core/PARStore.h +++ b/Core/PARStore.h @@ -68,12 +68,15 @@ extern NSString *PARStoreDidSyncNotification; - (BOOL)writeBlobFromPath:(NSString *)sourcePath toPath:(NSString *)path error:(NSError **)error; - (nullable NSData *)blobDataAtPath:(NSString *)path error:(NSError **)error; - (BOOL)deleteBlobAtPath:(NSString *)path error:(NSError **)error; -- (BOOL)deleteBlobAtPath:(NSString *)path registeringDeletion: (BOOL)usingTombstone error:(NSError **)error; - (nullable NSString *)absolutePathForBlobPath:(NSString *)path; - (NSArray *)absolutePathsForBlobsPrefixedBy:(NSString *)prefix NS_SWIFT_NAME(absolutePaths(forBlobsPrefixedBy:)); - (void)enumerateBlobs:(void(^)(NSString *path))block; - (BOOL)blobExistsAtPath:(NSString *)path; + +/// @name Registered Deletion Support +- (BOOL)deleteBlobAtPath:(NSString *)path registeringDeletion: (BOOL)usingTombstone error:(NSError **)error; - (BOOL)blobIsRegisteredDeletedAtPath:(NSString *)path; +- (void)enumerateDeletedBlobs:(void(^)(NSString *path))block; /// @name Syncing - (void)sync; diff --git a/Core/PARStore.m b/Core/PARStore.m index 85a5219..0ee0cd9 100644 --- a/Core/PARStore.m +++ b/Core/PARStore.m @@ -1604,7 +1604,7 @@ - (void)enumerateBlobs:(void(^)(NSString *path))block for(NSURL* url in enumerator) { BOOL isDir = false; if ([fileManager fileExistsAtPath:url.path isDirectory:&isDir] && !isDir) continue; - if (url.pathExtension == TombstoneFileExtension) { + if ([url.pathExtension isEqualToString: TombstoneFileExtension]) { [tombstones addObject:[url URLByDeletingPathExtension]]; } else { [urls addObject:url]; @@ -1633,6 +1633,35 @@ - (void)enumerateBlobs:(void(^)(NSString *path))block } } +- (void)enumerateDeletedBlobs:(void(^)(NSString *path))block +{ + if (!self._inMemory) + { + __block NSMutableSet *tombstones = [NSMutableSet set]; + [[self newFileCoordinator] coordinateReadingItemAtURL:[self blobDirectoryURL] options:NSFileCoordinatorReadingWithoutChanges error:NULL byAccessor:^(NSURL *newURL) + { + NSDirectoryEnumerator *enumerator = [[NSFileManager defaultManager] enumeratorAtURL:[self blobDirectoryURL] includingPropertiesForKeys:nil options:(NSDirectoryEnumerationSkipsPackageDescendants|NSDirectoryEnumerationSkipsHiddenFiles|NSDirectoryEnumerationSkipsSubdirectoryDescendants) errorHandler:nil]; + NSFileManager *fileManager = [[NSFileManager alloc] init]; + for(NSURL* url in enumerator) { + BOOL isDir = false; + if (![fileManager fileExistsAtPath:url.path isDirectory:&isDir] || isDir) continue; + if ([url.pathExtension isEqualToString: TombstoneFileExtension]) { + [tombstones addObject:url]; + } + } + }]; + + NSUInteger prefixLength = self.blobDirectoryURL.path.length+1; // +1 is for the last slash + for (NSURL *url in tombstones) { + // Resolving symbolic link here, because on iOS at least, the directory enumerator + // uses a sym linked "private" folder, causing the path to be different to what comes + // out for the blobDirectoryURL. + NSString *absolutePath = [url URLByResolvingSymlinksInPath].path; + NSString *relativePath = [absolutePath substringFromIndex:prefixLength]; + block(relativePath); + } + } +} #pragma mark - Syncing diff --git a/Tests/PARStoreBlobTests.m b/Tests/PARStoreBlobTests.m index 30cd8aa..9667daf 100644 --- a/Tests/PARStoreBlobTests.m +++ b/Tests/PARStoreBlobTests.m @@ -57,6 +57,7 @@ - (void)testDeletion // tombstone file should not have appeared in its place NSString *tombstonePath = [blobPath stringByAppendingPathExtension: TombstoneFileExtension]; XCTAssertFalse([[NSFileManager defaultManager] fileExistsAtPath: tombstonePath]); + XCTAssertFalse([store blobIsRegisteredDeletedAtPath:@"blob"]); [store tearDownNow]; } @@ -81,6 +82,7 @@ - (void)testDeletionWithTombstone // tombstone file should have appeared in its place NSString *tombstonePath = [blobPath stringByAppendingPathExtension: TombstoneFileExtension]; XCTAssertTrue([[NSFileManager defaultManager] fileExistsAtPath: tombstonePath]); + XCTAssertTrue([store blobIsRegisteredDeletedAtPath:@"blob"]); [store tearDownNow]; } @@ -88,7 +90,7 @@ - (void)testDeletionWithTombstone - (void)testBlobExists { - // exists should product the right result before/after creation of a blob file + // exists should produce the right result before/after creation of a blob file NSError *error = nil; NSURL *url = [[self urlWithUniqueTmpDirectory] URLByAppendingPathComponent:@"doc.parstore"]; @@ -102,6 +104,7 @@ - (void)testBlobExists [store tearDownNow]; } + - (void)testTombstoneSuppressesFileExistence { // if there's a tombstone file present, exists should return NO even if the actual blob @@ -151,20 +154,46 @@ - (void)testTombstoneSuppressesEnumeration NSError *error = nil; NSURL *url = [[self urlWithUniqueTmpDirectory] URLByAppendingPathComponent:@"doc.parstore"]; PARStore *store = [PARStore storeWithURL:url deviceIdentifier:[self deviceIdentifierForTest]]; - XCTAssertTrue([store writeBlobData:[@"test" dataUsingEncoding:NSUTF8StringEncoding] toPath:@"blob" error:&error]); + XCTAssertTrue([store writeBlobData:[@"test" dataUsingEncoding:NSUTF8StringEncoding] toPath:@"blob1" error:&error]); + XCTAssertTrue([store writeBlobData:[@"test" dataUsingEncoding:NSUTF8StringEncoding] toPath:@"blob2" error:&error]); // fake the presence of a tombstone, to simulate a situation where a partial // synchronisation has caused it to exist along with the blob - NSString *tombstonePath = [[store absolutePathForBlobPath: @"blob"] stringByAppendingPathExtension: TombstoneFileExtension]; + NSString *tombstonePath = [[store absolutePathForBlobPath: @"blob1"] stringByAppendingPathExtension: TombstoneFileExtension]; [[@"foobar" dataUsingEncoding:NSUTF8StringEncoding] writeToFile:tombstonePath atomically:YES]; - __block int count = 0; + __block int count = 1; [store enumerateBlobs:^(NSString *blobPath) { ++count; + XCTAssertEqual(blobPath, @"blob2"); + }]; + XCTAssertEqual(count, 1); + + [store tearDownNow]; +} + +- (void)testEnumeratingTombstones +{ + NSError *error = nil; + NSURL *url = [[self urlWithUniqueTmpDirectory] URLByAppendingPathComponent:@"doc.parstore"]; + PARStore *store = [PARStore storeWithURL:url deviceIdentifier:[self deviceIdentifierForTest]]; + XCTAssertTrue([store writeBlobData:[@"test" dataUsingEncoding:NSUTF8StringEncoding] toPath:@"blob1" error:&error]); + XCTAssertTrue([store writeBlobData:[@"test" dataUsingEncoding:NSUTF8StringEncoding] toPath:@"blob2" error:&error]); + XCTAssertTrue([store writeBlobData:[@"test" dataUsingEncoding:NSUTF8StringEncoding] toPath:@"blob3" error:&error]); + + XCTAssertTrue([store deleteBlobAtPath:@"blob1" registeringDeletion: YES error:&error]); + XCTAssertTrue([store deleteBlobAtPath:@"blob3" registeringDeletion: YES error:&error]); + + __block int count = 0; + NSArray* expected = @[@"blob1.deleted", @"blob3.deleted"]; + [store enumerateDeletedBlobs:^(NSString *blobPath) { + ++count; + XCTAssertTrue([expected containsObject: blobPath]); }]; - XCTAssertEqual(count, 0); + XCTAssertEqual(count, 2); [store tearDownNow]; } + @end From e8f52c487e4fc57e52c4e2d79ecf861cd391b5fb Mon Sep 17 00:00:00 2001 From: Sam Deane Date: Tue, 29 Jun 2021 12:44:56 +0100 Subject: [PATCH 15/18] Pass out both the blob path and the tombstone path when enumerating tombstones. --- Core/PARStore.h | 2 +- Core/PARStore.m | 11 ++++++++--- Tests/PARStoreBlobTests.m | 9 ++++++--- 3 files changed, 15 insertions(+), 7 deletions(-) diff --git a/Core/PARStore.h b/Core/PARStore.h index dbc03c2..322d3b7 100644 --- a/Core/PARStore.h +++ b/Core/PARStore.h @@ -76,7 +76,7 @@ extern NSString *PARStoreDidSyncNotification; /// @name Registered Deletion Support - (BOOL)deleteBlobAtPath:(NSString *)path registeringDeletion: (BOOL)usingTombstone error:(NSError **)error; - (BOOL)blobIsRegisteredDeletedAtPath:(NSString *)path; -- (void)enumerateDeletedBlobs:(void(^)(NSString *path))block; +- (void)enumerateDeletedBlobs:(void(^)(NSString *blobPath, NSString *markerPath))block; /// @name Syncing - (void)sync; diff --git a/Core/PARStore.m b/Core/PARStore.m index 0ee0cd9..c050bf1 100644 --- a/Core/PARStore.m +++ b/Core/PARStore.m @@ -1633,7 +1633,7 @@ - (void)enumerateBlobs:(void(^)(NSString *path))block } } -- (void)enumerateDeletedBlobs:(void(^)(NSString *path))block +- (void)enumerateDeletedBlobs:(void(^)(NSString *blobPath, NSString *markerPath))block { if (!self._inMemory) { @@ -1657,8 +1657,13 @@ - (void)enumerateDeletedBlobs:(void(^)(NSString *path))block // uses a sym linked "private" folder, causing the path to be different to what comes // out for the blobDirectoryURL. NSString *absolutePath = [url URLByResolvingSymlinksInPath].path; - NSString *relativePath = [absolutePath substringFromIndex:prefixLength]; - block(relativePath); + NSString *relativeTombstonePath = [absolutePath substringFromIndex:prefixLength]; + NSString *relativePath = [relativeTombstonePath stringByDeletingPathExtension]; + + // we pass back both the path to the datafile and the path to the tombstone file + // since the relationship between the two is an implementation detail + // (right now it's just an extra file extension, but that may not always be true) + block(relativePath, relativeTombstonePath); } } } diff --git a/Tests/PARStoreBlobTests.m b/Tests/PARStoreBlobTests.m index 9667daf..31f3586 100644 --- a/Tests/PARStoreBlobTests.m +++ b/Tests/PARStoreBlobTests.m @@ -148,7 +148,7 @@ - (void)testTombstoneSuppressesData - (void)testTombstoneSuppressesEnumeration { - // tombstone files shoulnd't be included in the enumeration + // tombstone files shouldn't be included in the enumeration // the presence of a tombstone file should also cause the corresponding data file to be skipped // from the enumeration (if it still exists) NSError *error = nil; @@ -174,6 +174,7 @@ - (void)testTombstoneSuppressesEnumeration - (void)testEnumeratingTombstones { + // we should be called back with both the original data file paths, and the paths to the tombstones NSError *error = nil; NSURL *url = [[self urlWithUniqueTmpDirectory] URLByAppendingPathComponent:@"doc.parstore"]; PARStore *store = [PARStore storeWithURL:url deviceIdentifier:[self deviceIdentifierForTest]]; @@ -185,10 +186,12 @@ - (void)testEnumeratingTombstones XCTAssertTrue([store deleteBlobAtPath:@"blob3" registeringDeletion: YES error:&error]); __block int count = 0; - NSArray* expected = @[@"blob1.deleted", @"blob3.deleted"]; - [store enumerateDeletedBlobs:^(NSString *blobPath) { + NSArray* expected = @[@"blob1", @"blob3"]; + NSArray* expectedTombstones = @[@"blob1.deleted", @"blob3.deleted"]; + [store enumerateDeletedBlobs:^(NSString *blobPath, NSString *markerPath) { ++count; XCTAssertTrue([expected containsObject: blobPath]); + XCTAssertTrue([expectedTombstones containsObject: markerPath]); }]; XCTAssertEqual(count, 2); From 4fc6892fd85c29ae54dabf30e86ae6f3347c050c Mon Sep 17 00:00:00 2001 From: Sam Deane Date: Wed, 30 Jun 2021 11:08:59 +0100 Subject: [PATCH 16/18] Fixed old variable name. --- Core/PARStore.h | 2 +- Core/PARStore.m | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Core/PARStore.h b/Core/PARStore.h index 322d3b7..933f566 100644 --- a/Core/PARStore.h +++ b/Core/PARStore.h @@ -74,7 +74,7 @@ extern NSString *PARStoreDidSyncNotification; - (BOOL)blobExistsAtPath:(NSString *)path; /// @name Registered Deletion Support -- (BOOL)deleteBlobAtPath:(NSString *)path registeringDeletion: (BOOL)usingTombstone error:(NSError **)error; +- (BOOL)deleteBlobAtPath:(NSString *)path registeringDeletion: (BOOL)registeringDeletion error:(NSError **)error; - (BOOL)blobIsRegisteredDeletedAtPath:(NSString *)path; - (void)enumerateDeletedBlobs:(void(^)(NSString *blobPath, NSString *markerPath))block; diff --git a/Core/PARStore.m b/Core/PARStore.m index c050bf1..a0b5093 100644 --- a/Core/PARStore.m +++ b/Core/PARStore.m @@ -1425,7 +1425,7 @@ - (BOOL)deleteBlobAtPath:(NSString *)path error:(NSError **)error return [self deleteBlobAtPath:path registeringDeletion: NO error:error]; } -- (BOOL)deleteBlobAtPath:(NSString *)path registeringDeletion: (BOOL)usingTombstone error:(NSError **)error +- (BOOL)deleteBlobAtPath:(NSString *)path registeringDeletion: (BOOL)registeringDeletion error:(NSError **)error { // nil path = error if (path == nil) @@ -1459,7 +1459,7 @@ - (BOOL)deleteBlobAtPath:(NSString *)path registeringDeletion: (BOOL)usingTombst NSError *error = nil; BOOL success = YES; - if (usingTombstone) { + if (registeringDeletion) { // write tombstone success = [self writeTombstoneAtPath:newTombstoneURL.path forFileAtPath:newURL.path error:&error]; if (!success) From 583b03746b3c0227f47f0d64be5b4a469f7f6443 Mon Sep 17 00:00:00 2001 From: Sam Deane Date: Wed, 30 Jun 2021 17:51:55 +0100 Subject: [PATCH 17/18] Fixed enumeration - should be skipping directories. --- Core/PARStore.m | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Core/PARStore.m b/Core/PARStore.m index a0b5093..62ce826 100644 --- a/Core/PARStore.m +++ b/Core/PARStore.m @@ -1603,7 +1603,7 @@ - (void)enumerateBlobs:(void(^)(NSString *path))block NSFileManager *fileManager = [[NSFileManager alloc] init]; for(NSURL* url in enumerator) { BOOL isDir = false; - if ([fileManager fileExistsAtPath:url.path isDirectory:&isDir] && !isDir) continue; + if ([fileManager fileExistsAtPath:url.path isDirectory:&isDir] && isDir) continue; if ([url.pathExtension isEqualToString: TombstoneFileExtension]) { [tombstones addObject:[url URLByDeletingPathExtension]]; } else { From edf65a0e9c90c678e769a3b79010cfde694bfd1e Mon Sep 17 00:00:00 2001 From: Sam Deane Date: Wed, 30 Jun 2021 18:13:08 +0100 Subject: [PATCH 18/18] Added option to ignore missing files when deleting. We can't fill in all the tombstone metadata if the file has already been deleted, and arguably there is no point in making one anyway, so it's helpful to be able to choose to ignore this without failing. --- Core/PARStore.h | 2 +- Core/PARStore.m | 14 ++++++++++---- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/Core/PARStore.h b/Core/PARStore.h index 933f566..3636311 100644 --- a/Core/PARStore.h +++ b/Core/PARStore.h @@ -74,7 +74,7 @@ extern NSString *PARStoreDidSyncNotification; - (BOOL)blobExistsAtPath:(NSString *)path; /// @name Registered Deletion Support -- (BOOL)deleteBlobAtPath:(NSString *)path registeringDeletion: (BOOL)registeringDeletion error:(NSError **)error; +- (BOOL)deleteBlobAtPath:(NSString *)path registeringDeletion: (BOOL)registeringDeletion ignoreIfMissing: (BOOL)ignoreIfMissing error:(NSError **)error; - (BOOL)blobIsRegisteredDeletedAtPath:(NSString *)path; - (void)enumerateDeletedBlobs:(void(^)(NSString *blobPath, NSString *markerPath))block; diff --git a/Core/PARStore.m b/Core/PARStore.m index 62ce826..5e4d62d 100644 --- a/Core/PARStore.m +++ b/Core/PARStore.m @@ -1422,10 +1422,10 @@ - (BOOL)blobIsRegisteredDeletedAtPath:(NSString *)path - (BOOL)deleteBlobAtPath:(NSString *)path error:(NSError **)error { - return [self deleteBlobAtPath:path registeringDeletion: NO error:error]; + return [self deleteBlobAtPath:path registeringDeletion: NO ignoreIfMissing:NO error:error]; } -- (BOOL)deleteBlobAtPath:(NSString *)path registeringDeletion: (BOOL)registeringDeletion error:(NSError **)error +- (BOOL)deleteBlobAtPath:(NSString *)path registeringDeletion: (BOOL)registeringDeletion ignoreIfMissing: (BOOL)ignoreIfMissing error:(NSError **)error { // nil path = error if (path == nil) @@ -1458,8 +1458,14 @@ - (BOOL)deleteBlobAtPath:(NSString *)path registeringDeletion: (BOOL)registering { NSError *error = nil; BOOL success = YES; + BOOL fileExists = [[NSFileManager defaultManager] fileExistsAtPath:newURL.path]; - if (registeringDeletion) { + if (!(ignoreIfMissing || fileExists)) { + success = NO; + localError = [NSError errorWithObject:self code:__LINE__ localizedDescription:[NSString stringWithFormat:@"Could not delete non-existent data blob at path '%@'", newURL.path] underlyingError:nil]; + } + + if (success && registeringDeletion && fileExists) { // write tombstone success = [self writeTombstoneAtPath:newTombstoneURL.path forFileAtPath:newURL.path error:&error]; if (!success) @@ -1468,7 +1474,7 @@ - (BOOL)deleteBlobAtPath:(NSString *)path registeringDeletion: (BOOL)registering } } - if (success) + if (success && fileExists) { success = [[NSFileManager defaultManager] removeItemAtURL:fileURL error:&error]; if (!success)