Add move operation to FileDescriptor and refine base typing - #1128
Conversation
| Files.move(normalizedPath, destination.normalizedPath, StandardCopyOption.ATOMIC_MOVE) | ||
| Some(destination) | ||
| } catch { | ||
| case NonFatal(_) => None |
There was a problem hiding this comment.
Should we log a warning in these cases? Just for visibility.
| def move(pathString: String): Option[SftpFileDescriptor] = { | ||
| val destination = mvLocation(pathString) | ||
| if (destination.path == path) Some(this) | ||
| else if (destination.parent().mkdirs() && Try(sftp(_.rename(path, destination.path))).isSuccess) Some(destination) |
There was a problem hiding this comment.
Should we delete the created directory if the rename fails?
There was a problem hiding this comment.
We could, but it is difficult. We don't really know how many directories mkDirs created along the way. We could use some is empty + just created heuristic; please let me know if you have another suggestion.
| if (copied && delete()) Some(destination.copy(summary = None)) else None | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Can't we take advantage of native server-side copy (we do mention "may be more efficient than a manual upload followed by a delete")? Or is it not possible?
There was a problem hiding this comment.
In GCS and S3, there is no move operation, so I don't think it is possible to make those more efficient than just a copy+delete. Locally and in sftp the implementation is specialized.
| Try(sftp(_.mkdirs(path))).isSuccess | ||
|
|
||
| def move(pathString: String): Option[SftpFileDescriptor] = { | ||
| val destination = mvLocation(pathString) |
There was a problem hiding this comment.
Should mvLocation here recover and return None upon a transient error?
There was a problem hiding this comment.
mvLocation should in principle not fail. It can, however, as it may fetch the remote to understand if the requested destination is a directory. We could do:
protected final def mvLocation(pathString: String): Option[Self] = {
val requestedDestination = parent().cd(pathString).asInstanceOf[Self]
for {
isDirectory <- Try(requestedDestination.exists && requestedDestination.isDirectory).toOption
} yield
if (isDirectory) requestedDestination.child(name).asInstanceOf[Self]
else requestedDestination
}And act on it. But then, we should probably review whether exists and isDirectory should also return Option as they are the failure points. Given the current status quo – where exceptions may occur – I'd be inclined to keep this as is. Let me know what you think.
| * @return | ||
| * the new file descriptor with the updated path if the move was successful, None otherwise. | ||
| */ | ||
| def move(pathString: String): Option[Self] |
There was a problem hiding this comment.
Are the S3 and GCS's concrete implementations consistent with the Local/SFTP ones and between these ScalaDocs and the PR's description? I think the former currently only return None when upload/delete fails, with the underlying layer (S3Bucket.stream/GCSBucket.push) throwing upon IO errors.
Should we wrap these in some NonFatal/Try handling?
There was a problem hiding this comment.
We can resort to Using.apply instead of Using.resource for that. But again, I'd prefer, if we care to make these functions actually more transparent, to start from the leaves and make upload a fallible operation.
| * @return | ||
| * the new file descriptor with the updated path if the move was successful, None otherwise. | ||
| */ | ||
| def move(pathString: String): Option[Self] |
There was a problem hiding this comment.
Additionally, would it make sense to extract the common logic between the move implementations to the base trait? We could have the concrete implementations implement a kind of moveTo with the backend-specific call and the rest could be extracted and kept in the trait, I believe.
There was a problem hiding this comment.
Tried to do that with mvLocation. Unfortunately the rest of the body is kind of specific, some do move, some do copy+delete, some create intermediate directories... The only clear "single point" candidates are S3 and GCS, but there is no common trait for them at the moment, and it's perhaps an over-abstraction to introduce one.
afonsoafonsoafonso
left a comment
afonsoafonsoafonso
left a comment
There was a problem hiding this comment.
Lgtm! It's noticeable that this is a bit legacy-ish, I agree with what we discussed offline about not going the extra mile to improve it since we've been going more towards other solutions.
2 participants
Adds a new move operation to
FileDescriptorthat behaves similarly to UNIX'smvutility.uploadalready does.Most notably, I am interested in using the
movemethod for sftp.Opportunistically, Self was moved up, requiring implementations to return their own type (which they already did), and using it for the
mvLocationmethod up in the hierarchy.Does this change relate to existing issues or pull requests?
No.
Does this change require an update to the documentation?
How has this been tested?
Tested the local version via new unit tests.
Tested the sftp version against a local sftp server.