Community
Participate
Working Groups
Deleting a file that is shown in the RSE FTP filesystem by external ways (through filesystem or other FTP connection) is not reported as missing by RSE. The correct behaviour should be reporting that the file is missing.
I'd leave it for RSE 2.0.1 since it's not preventing FTP from working, and it seems quite easy to fix
Created attachment 73989 [details] Patch showing an "File not found" error message If the remote file has been removed, the RSE FTP client should show a "File not found" message
Dave - this patch contains an NLS change, can we do this for 2.0.1? Should we search for an existing "file not found" kind message to re-use? Do you have any other suggestions?
Now a translatable error message is displayed if the remote file has been removed and it's attempted to be open/downloaded from RSE.
For 2.0.1 I would prefer that we try to find an existing message rather than adding a new one. We can certainly add new externalized strings in 3.0.
I'll try to find an alternative message.
I haven't found any suitable string, can you think of any string that would make sense and accessible from org.eclipse.rse.services.files.ftp ? As an alternative, the raised RemoteFileIOException can be shown with an empty string as a reported message. The exception, when catched, pops up as an error message with: "Operation failed. File system input or output error", which makes sense, but the error details are empty.
Doesn't the FTP server give use a useful message like 400 FILE NOT FOUND
Reopening until Dave's concern is addressed
(In reply to comment #8) > Doesn't the FTP server give use a useful message like > > 400 FILE NOT FOUND > What the code was doing was preventing accessing a non-existing file, I can instead wait until the retrieve fails and use the FTP failure message in the 'details' of the error popup.
That sounds better because how could you know the file is missing unless you actually tried accessing it? - User could have copied it to the location in the short time between you listing the directory and RSE trying to access it.
The LIST command is performed by getFile() to obtaing the IHostFile just before downloading (see comment #6 of bug 195830 for more info) Eventually, I should remove this call and obtain the file size from somewhere else.
Well I'm not sure where you could get the file size from... Actually the LIST for getting the file size is OK, but I don't think you need to throw the exception yourself (or even check for null, see below). Just ignore the fact that !remoteFile.exists(). Move the input = ftpClient.retrieveFileStream(remoteFile); further up in the try{... block. And it should throw a proper exception for you, including the original remote message 550 aatest: No such file or directory. When reviewing this, I noticed that your getFile() code is wrong since it must not ever return null. See docs and implementation of SftpFileService#getFile().
Created attachment 74918 [details] Patch showing the remote error message The attached patch shows the FTP server error message when the remote input stream cannot be open and returns null. I have also fixed getFile() and it doesn't return null anymore.
Patch looks good for the updated "File not found" message. But your getFile() implementation is still not correct. I think it should throw new RemoteFileCancelledException(); in case the user canceled. Otherwise, user would cancel the request and the RSE cache would think the remote file is not there and mark it as not being there - but in reality it does not know.
Created attachment 74940 [details] Patch showing the remote error message and throwing RemoteFileCancelledException if cancelled Improved patch with Martin suggestion, so now it throws a RemoteFileCancelledException instead of just returning false when the user cancels the action.
Javier you added the throw new RemoteFileCanceledException() in line 679 but it should (also) have been added in line 397. Please be careful and read the API Docs of IFileService that your service must implement properly. Those methods which return a boolean already on success (e.g. download) may throw RemoteFileCanceledException, but don't have to do so necessarily - it's up to them. Those methods that return an IHostFile or other specialized object (like getFile()) need to throw the exception in case they are canceled. It wasn't correct in SftpFileService for 2.0 either, but I fixed it last week so you can reference it now where the exception needs to be thrown.
(In reply to comment #17) > Javier you added the throw new RemoteFileCanceledException() in line 679 but it > should (also) have been added in line 397. > > Please be careful and read the API Docs of IFileService that your service must > implement properly. Those methods which return a boolean already on success > (e.g. download) may throw RemoteFileCanceledException, but don't have to do so > necessarily - it's up to them. > > Those methods that return an IHostFile or other specialized object (like > getFile()) need to throw the exception in case they are canceled. > > It wasn't correct in SftpFileService for 2.0 either, but I fixed it last week > so you can reference it now where the exception needs to be thrown. > I fixed the other places where null was returned in a similar way. I haven't added all the modifications in this path as it could be misleading for this bug.
Good, then go for it... for this bug, the patch is good now.
Dave what do you think - I think this patch should be applied now?
I just noticed that this did not make it into 2.0.1 although it was - assigned (javier) - had a valid and review-approved patch just because nobody committed the fix. Do we consider this fix critical? Javier can you merge the patch so it can be applied to HEAD?
I just checked and the patch was applied a while ago, so it's part of 2.0.1 I didn't move the bug status to fixed as I was waiting for dave's review.
Comment on attachment 74940 [details] Patch showing the remote error message and throwing RemoteFileCancelledException if cancelled iplog- since Javier was committer since Aug 2006 already: http://dev.eclipse.org/mhonarc/lists/dsdp-tm-dev/msg00375.html
