Community
Participate
Working Groups
By fixing bug 221211, we changed the IFileService behavior to always throw an exception in case of failure executing a file operation; and, to ensure that multi-operations stop at the first error (but can return partial results). The IRemoteFileSubSystem partially inherits this behavior, so at the very least the Javadoc needs to be updated; bug IRemoteFileSubsystem also needs to be reviewed for changed behavior in case of cancellation (e.g.SystemOperationCancelledException is now used instead of returning false, is this handled correctly?) We should also consider throwing exceptions in IRemoteFileSubsystem rather than returning boolean return value, because exceptions can convey more information about the issue that actually went wrong on the remote host. I'm assigning this bug for consideration in RC3, as per DaveD's suggestion in bug 221211 comment 33. Since it's a very late, very breaking API change we'll need to discuss this before we proceed.
As per bug 221211 comment 44, we also need to take care of the return values in setLastModified() and setReadOnly(): In IRemoteFileSubsystem, these are currently documented to silently return "false" if the file to change does not exist. Is this what we really want? Particularly, what do we want to do in the following situations: (9a) operation is not implemented in the Service --> Throw an exception, or silently do nothing? Consider the case in uploadResourcesFromWorkspace() when the "PRESERVETIMESTAMPS" preference is switched on. (9b) Item to work on did not exist on remote. Throw an exception or silently do nothing? Or should we revert to returning a boolean for these two cases?
(In reply to comment #1) > As per bug 221211 comment 44, we also need to take care of the return values in > setLastModified() and setReadOnly(): In IRemoteFileSubsystem, these are > currently documented to silently return "false" if the file to change does not > exist. > > Is this what we really want? Particularly, what do we want to do in > the following situations: > > (9a) operation is not implemented in the Service --> Throw an exception, > or silently do nothing? Consider the case in > uploadResourcesFromWorkspace() > when the "PRESERVETIMESTAMPS" preference is switched on. > Throw SystemUnsupportedOperationException exception in this case. > (9b) Item to work on did not exist on remote. Throw an exception or > silently do nothing? > Throw an exception, see bug 221211 comment 53. > Or should we revert to returning a boolean for these two cases? > No, returning boolean and throwing exception is not right (IMO). We can handle all usecases we need with adding just another class of exceptions and catching it where needed instead of isolating some special case as a boolean return value. Moreover, I think it is confusing for most of the clients to check both for exceptions and return values.
I've propagated the interface changes made for IFileService up to their counterparts in IRemoteFileSubSystem. The changes were not as extensive as I had feared so this should be a relatively benign change. Dave McKnight - please review the changes, they are not extensive and I tried to keep the exact semantics. While testing I was getting an NPE on deletes in the local file system if I attempted a delete from a read-only folder. It's the result of an incorrect construction of a RemoteFileSecurityException (which really should be a SystemRemoteSecurityException). I'll write up a separate bug report for this.
I will be committing the changes I've made. DaveM -- please review when synchronizing.
Dave can you please put a "Summary of API Changes" as well as migration notes for clients here on the bug?
The API changes look good to me.
After reviewing the change, I think I found some issues. * SystemCopyRemoteFileAction#doCopy() - Used to forward exceptions from ss.copy(), now they are only logged --> I assume the users used to see error dialog, now they won't * SystemMoveRemoteFileAction#doCopy() - Same problem, used to forward exceptions from ss.copy(), now only logged --> I assume the users used to see error dialog, now they won't * SystemViewRemoteFileAdapter#doDeleteBatch() - Same, used to forward exceptions from ss.doDeleteBatch(), now only logged --> I assume the users used to see error dialog, now they won't * Target milestone was wrong (RC1, must be RC2) Reopening to check and address
Summary of API changes: The following methods in IRSERemoteFileSubSystem were changed to not return a value instead of returning a boolean. An exception is now signalled if the operation fails for any reason. In most cases this is a change in signature only as the exceptions were signalled if the method was not returning true. The one case that is semantically different is delete(...) which could have returned false if the file was not found and now throws an exception. IRSERemoteFileSubSystem copy(...) copyBatch(...) delete(...) deleteBatch(...) move(...) rename(...) setLastModified(...) setReadyOnly(...) FileServiceSubSystem -- changes made as above to reflect the new interface
(In reply to comment #7) > After reviewing the change, I think I found some issues. > > * SystemCopyRemoteFileAction#doCopy() > - Used to forward exceptions from ss.copy(), now they are only logged > --> I assume the users used to see error dialog, now they won't > > * SystemMoveRemoteFileAction#doCopy() > - Same problem, used to forward exceptions from ss.copy(), now only logged > --> I assume the users used to see error dialog, now they won't > > * SystemViewRemoteFileAdapter#doDeleteBatch() > - Same, used to forward exceptions from ss.doDeleteBatch(), now only logged > --> I assume the users used to see error dialog, now they won't > > * Target milestone was wrong (RC1, must be RC2) > > Reopening to check and address Found one more like this SystemViewRemoteFileAdapter#doRename() Fixing these simplifies the code. I will commit. DaveM please re-review. Changes made are listed above. Thanks!
Fixed.
Found bug 234034 when reviewing again. But the change here is ok.
