Skip to content

WIP: Enhanced Deletion #12

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
samdeane wants to merge 18 commits into
base: master
Choose a base branch
from
Open

WIP: Enhanced Deletion #12

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
7bed92f
Started stubbing tombstone support.
samdeane Jun 22, 2021
44e4d37
Added blob deletion tests
samdeane Jun 22, 2021
f5ea6c7
Improved deletion tests
samdeane Jun 23, 2021
e148d84
Added constant for tombstone extension
samdeane Jun 23, 2021
4235771
Don't return data and don't enumerate files where a tombstone is pres…
samdeane Jun 23, 2021
53a2a83
Renamed tests.
samdeane Jun 23, 2021
403b36e
comment tweak
samdeane Jun 24, 2021
8c73d69
Added blobExistsAtPath
samdeane Jun 28, 2021
091f510
Honour the usingTombstone flag.
samdeane Jun 28, 2021
98bf01e
Added comments about potential cleanup.
samdeane Jun 28, 2021
fdd8bfa
Added a note about double-deletion.
samdeane Jun 28, 2021
3b4343d
Added some comments explaining the tests
samdeane Jun 28, 2021
872b52f
Updated to reflect Drew's comments
samdeane Jun 29, 2021
6276465
Fixed blob enumeration. Improved tests. Added enumeration of tombstones.
samdeane Jun 29, 2021
e8f52c4
Pass out both the blob path and the tombstone path when enumerating t…
samdeane Jun 29, 2021
4fc6892
Fixed old variable name.
samdeane Jun 30, 2021
583b037
Fixed enumeration - should be skipping directories.
samdeane Jun 30, 2021
edf65a0
Added option to ignore missing files when deleting.
samdeane Jun 30, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions Core/PARStore.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,12 @@ extern NSString *PARStoreDidSyncNotification;
- (nullable NSString *)absolutePathForBlobPath:(NSString *)path;
- (NSArray<NSString *> *)absolutePathsForBlobsPrefixedBy:(NSString *)prefix NS_SWIFT_NAME(absolutePaths(forBlobsPrefixedBy:));
- (void)enumerateBlobs:(void(^)(NSString *path))block;
- (BOOL)blobExistsAtPath:(NSString *)path;
Copy link
Collaborator

@drewmccormack drewmccormack Jun 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we would want to be able to query if a tombstone exists too. Imagine we are doing the sync engine. It would want to check if a particular file from the cloud is now a tombstone, and should be removed. Note that that is different to the blob simply not being found locally, which would indicate it should be downloaded, not deleted.

So I'm thinking probably want a query for tombstone existence too. Perhaps blobIsRegisterdDeletedAtPath:. It think this already implies the use of tombstones, otherwise how would we know.

Copy link
Author

@samdeane samdeane Jun 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I wasn't quite sure whether we needed to treat 'file doesn't exist' as different from 'tombstone exists', or whether they'd turn out to imply the same thing in any calling code. That's what I was aiming at with just having the exists call, but we may well need the other one instead.

Copy link
Collaborator

@drewmccormack drewmccormack Jun 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think most of the time you just want the exists call, and it should handle tombstones for you. But in rare occasions, like sync, you want to query whether the formerly existed.


/// @name Registered Deletion Support
- (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;

/// @name Syncing
- (void)sync;
Expand Down
170 changes: 155 additions & 15 deletions Core/PARStore.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -1382,7 +1384,48 @@ - (BOOL)writeBlobFromPath:(NSString *)sourcePath toPath:(NSString *)targetSubpat
return YES;
}

- (BOOL)writeTombstoneAtPath:(NSString *)tombstonePath forFileAtPath:(NSString *)filePath error:(NSError **)error
Copy link
Collaborator

@drewmccormack drewmccormack Jun 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to be clear, although I suggest using "registeredDeletion" in the API, the private methods can stick with tombstone.

{
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)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)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 registeringDeletion: NO ignoreIfMissing:NO error:error];
}

- (BOOL)deleteBlobAtPath:(NSString *)path registeringDeletion: (BOOL)registeringDeletion ignoreIfMissing: (BOOL)ignoreIfMissing error:(NSError **)error
{
// nil path = error
if (path == nil)
Expand All @@ -1407,24 +1450,53 @@ - (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:TombstoneFileExtension];

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];
}];
NSError *error = nil;
BOOL success = YES;
BOOL fileExists = [[NSFileManager defaultManager] fileExistsAtPath:newURL.path];

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)
{
localError = [NSError errorWithObject:self code:__LINE__ localizedDescription:[NSString stringWithFormat:@"Could not create tombstone for data blob at path '%@'", newURL.path] underlyingError:error];
}
}

if (success && fileExists)
{
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;
Expand Down Expand Up @@ -1456,6 +1528,26 @@ - (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];
}

// 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;
}

NSError *coordinatorError = nil;
__block NSData *data = nil;
[[self newFileCoordinator] coordinateReadingItemAtURL:fileURL options:NSFileCoordinatorReadingWithoutChanges error:&coordinatorError byAccessor:^(NSURL *newURL)
Expand Down Expand Up @@ -1509,31 +1601,79 @@ - (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 isEqualToString: TombstoneFileExtension]) {
Copy link
Collaborator

@drewmccormack drewmccormack Jun 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Close one :)

[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) {
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);
}
}
}
}

- (void)enumerateDeletedBlobs:(void(^)(NSString *blobPath, NSString *markerPath))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);
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);
}
}
}


#pragma mark - Syncing

- (void)applySyncChangeWithValues:(NSDictionary *)values timestamps:(NSDictionary *)timestamps
Expand Down
6 changes: 6 additions & 0 deletions PARStore.xcodeproj/project.pbxproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 */
Expand Down Expand Up @@ -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 = "<group>"; };
/* End PBXFileReference section */

/* Begin PBXFrameworksBuildPhase section */
Expand Down Expand Up @@ -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 */,
Expand Down Expand Up @@ -456,6 +460,7 @@
isa = PBXSourcesBuildPhase;
buildActionMask = 2147483647;
files = (
8BED6AEC26824978004639DF /* PARStoreBlobTests.m in Sources */,
56C7EDF716E2687F00FFBBF2 /* PARStoreTests.m in Sources */,
56C7EDFD16E26D0700FFBBF2 /* PARTestCase.m in Sources */,
);
Expand All @@ -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 */,
Expand Down
Loading