WIP: Enhanced Deletion #12
Conversation
samdeane
force-pushed
the
sd-file-deletion
branch
from
dcd8d14 to
3b4343d
Compare
drewmccormack
left a comment
drewmccormack
left a comment
There was a problem hiding this comment.
Looking good. Nice job!
I mentioned a few changes in the comments.
There is one other I wanted to bring up: what happens if someone re-inserts a file that has a tombstone? I guess in our system, we don't want this, because if we remove the tombstone, we get the problem that a tombstone on other devices will still result in the file being deleted eventually. But perhaps what we can do is simply give an error if someone goes to insert a file that already has a tombstone.
| - (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; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| return YES; | ||
| } | ||
|
|
||
| - (BOOL)writeTombstoneAtPath:(NSString *)tombstonePath forFileAtPath:(NSString *)filePath error:(NSError **)error |
There was a problem hiding this comment.
Just to be clear, although I suggest using "registeredDeletion" in the API, the private methods can stick with tombstone.
Core/PARStore.m
Outdated
| // 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. |
There was a problem hiding this comment.
I think it is safer to return an error. It gives the little bit more information. If you expect to find a file, and you don't, that is probably something you want to know about.
Core/PARStore.h
Outdated
| - (BOOL)blobExistsAtPath:(NSString *)path; | ||
|
|
||
| /// @name Registered Deletion Support | ||
| - (BOOL)deleteBlobAtPath:(NSString *)path registeringDeletion: (BOOL)usingTombstone error:(NSError **)error; |
There was a problem hiding this comment.
Small thing, but variable name is still old one (usingTombstone)
| BOOL isDir = false; | ||
| if ([fileManager fileExistsAtPath:url.path isDirectory:&isDir] && !isDir) continue; | ||
| if (url.pathExtension == TombstoneFileExtension) { | ||
| if ([url.pathExtension isEqualToString: TombstoneFileExtension]) { |
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.
2 participants
Work in progress on some changes for use in Agenda.
Adds the facility to replace deleted blob files with a "tombstone" marker file which forces the rest of the API to treat the blob as deleted even if the actual data file returns.
This is intended to work around situations where the blob directory is being synced with an external service that might get confused and put back deleted files instead of removing them.
Note: the behaviour of the existing
deleteBlobAtPathcall should be unaffected by these changes. A new variant of the call has been added to opt-in to tombstones. Other API has been updated to be aware of the presence of tombstone files, but should be unaffected if they are never created. There will be a minor reduction in efficiency due to the need to check for tombstones, but it shouldn't be significant in comparison to whatever work is actually done with the files.