Community
Participate
Working Groups
If you delete a read-only directory you get no error and the directory is removed from the Remote Systems View. If you refresh the parent of the removed directory it will come back. The directory you delete must have a file inside it or else the directory will actually be deleted. -----------Enter bugs above this line----------- TM 2.0RC2 Testing installation : eclipse-SDK-3.3RC3 RSE install : RSE 2.0 RC2 java.runtime : Sun 1.5.0_11-b03 os.name: : Windows XP, Service Pack 2 ------------------------------------------------
I think this is a minor issue only
Created attachment 75878 [details] No longer return true if the classification is not a file Removed the code that did a check if the file is a file and not a folder in DStoreFileService.delete. Before if it was a folder it would return true even if deleting the directory failed. I also noticed that we did a check if the error started with "failed" and threw an exception if it did. I changed this to check for SUCCESS and return true else throw the exception. In case's deleting a directory was successful we weren't setting the status to anything. Patch sets the status to success in those cases. Legal Message: I, Kevin Doyle, declare that I developed attached code from scratch, without referencing any 3rd party materials except material licensed under the EPL. I am authorized by my employer, IBM Canada Ltd. to make this contribution under the EPL.
I've committed the patch.
There might be a backward compatibility issue: I'm wondering what the effect of this fix is, when running an RSE 2.0.1 client against an RSE 2.0.0 server. Looking at the code, it seems that in case a directory is being deleted (successfully), the "status" is not being set at all, so the client would issue an error. Which is not correct, because the error would be shown although everything is fine. If, however, there is an error deleting the remote directory, it looks like an exception would be thrown in the UniversalFileSystemMiner, so a status ERROR would be returned. This observation is just from looking at the code, and might need to be verified by testing; but for me, it looks like a more proper code at the client would throw the error only if an error status is sent from the remote, and return OK if no status is sent from the remote. Reopening to investigate.
FYI, changes were in DStoreFileService.java 1.16 UniversalFileSystemMIner.java 1.21
You are right, Martin. The fix will have a problem with an older 2.0 dStore server.
Created attachment 75984 [details] Fix for backwards compatibility I forgot that we are maintaining backwards compatibility with the dstore servers. Nice catch Martin. I don't like making this change as an empty status is an error in my opinion, but to maintain backwards compatibility this patch checks for an empty source message and returns true. While testing I noticed that if deleting a file in an archive failed it didn't display the file name in the error dialog. Patch fixes that as well. Legal Message: I, Kevin Doyle, declare that I developed attached code from scratch, without referencing any 3rd party materials except material licensed under the EPL. I am authorized by my employer, IBM Canada Ltd. to make this contribution under the EPL.
applied the patch, and tried the following scenarios: Expand an Archive file, so that a folder could be shown. And expand that virtual folder as well. In another workspace, delete that expanded folder. Go back to the first workspace, and tried to delete the virtual folder again. Got error message saying cannot delete. Dismissed the message, but the parent of the virtual folder is not refreshed, so that the non-existing folder still shown. Refresh its parent, got the following error: java.lang.NullPointerException at org.eclipse.rse.internal.services.dstore.files.DStoreHostFile.isFile(DStoreHostFile.java:214) at org.eclipse.rse.internal.services.dstore.files.DStoreHostFile.getClassification(DStoreHostFile.java:300) at org.eclipse.rse.internal.subsystems.files.dstore.DStoreFile.getClassification(DStoreFile.java:202) at org.eclipse.rse.internal.files.ui.view.SystemViewRemoteFileAdapter.testAttribute(SystemViewRemoteFileAdapter.java:3035) at org.eclipse.ui.internal.ActionExpression$ObjectStateExpression.preciselyMatches(ActionExpression.java:530) at org.eclipse.ui.internal.ActionExpression$ObjectStateExpression.isEnabledFor(ActionExpression.java:499) at org.eclipse.ui.internal.ActionExpression$AndExpression.isEnabledFor(ActionExpression.java:132) at org.eclipse.ui.internal.ActionExpression$SingleExpression.isEnabledFor(ActionExpression.java:743) at org.eclipse.ui.internal.ActionExpression.isEnabledFor(ActionExpression.java:1053) at org.eclipse.ui.internal.decorators.DecoratorDefinition.isEnabledFor(DecoratorDefinition.java:281) at org.eclipse.ui.internal.decorators.DecoratorManager.getDecoratorsFor(DecoratorManager.java:193) at org.eclipse.ui.internal.decorators.LightweightDecoratorManager.getDecoratorsFor(LightweightDecoratorManager.java:292) at org.eclipse.ui.internal.decorators.LightweightDecoratorManager.getDecorations(LightweightDecoratorManager.java:315) at org.eclipse.ui.internal.decorators.DecorationScheduler$1.ensureResultCached(DecorationScheduler.java:369) at org.eclipse.ui.internal.decorators.DecorationScheduler$1.run(DecorationScheduler.java:329) at org.eclipse.core.internal.jobs.Worker.run(Worker.java:55)
Created attachment 76123 [details] Updated to fix NPE's in DStoreHostFile Xuan try this patch and see if you get any more NPE's. Legal Message: I, Kevin Doyle, declare that I developed attached code from scratch, without referencing any 3rd party materials except material licensed under the EPL. I am authorized by my employer, IBM Canada Ltd. to make this contribution under the EPL.
Applied the patch, and tried the following scenarios: Expand an Archive file, so that a folder could be shown. And expand that virtual folder as well. In another workspace, delete that expanded folder. Go back to the first workspace, and tried to delete the virtual folder again. Got error message saying cannot delete. Dismissed the message, but the parent of the virtual folder is not refreshed, so that the non-existing folder still shown. Refresh the folder itself, I got the following error in my dstore server: java.lang.NullPointerException at org.eclipse.rse.internal.dstore.universal.miners.filesystem.ArchiveQu eryThread.createDataElement(ArchiveQueryThread.java:230) at org.eclipse.rse.internal.dstore.universal.miners.filesystem.ArchiveQu eryThread.doQueryAll(ArchiveQueryThread.java:138) at org.eclipse.rse.internal.dstore.universal.miners.filesystem.ArchiveQu eryThread.run(ArchiveQueryThread.java:57) java.lang.NullPointerException at org.eclipse.rse.internal.dstore.universal.miners.filesystem.FileClass ifier.classifyVirtualChildren(FileClassifier.java:1006) at org.eclipse.rse.internal.dstore.universal.miners.filesystem.FileClass ifier.run(FileClassifier.java:297) If I refresh the parent, it is fine. This NPE may not releate to the fix. But please investigate more to make sure it is not a regression. Thanks.
(In reply to comment #10) Xuan I'm not sure this is a valid testcase... when I understand right, you did this: * In workspace A, expand a folder inside x.zip * In workspace B, delete that folder inside x.zip --> As a result, x.zip will be re-written to x.zip.tmp, then renamed back into x.zip on the remote; in other words, the archive file itself gets a different timestamp Because of that, I find it quite natural that the parent needs to be refreshed. After all, the parent AND grandparent of the deleted virtual folder all get invalid, don't they? Also, did you try this scenario on a workspace without Kevin's patch and did it work as expected? I would assume there is the same failure so it would not be a regression. In general, I think that when refreshing a virtual folder, the refresh logic should look at the timestamp of the containing archive (i.e. check if it has changed on the remote). If it has changed, the archive itself as well as all its children need to be refreshed because anything could have changed inside the archive. I'm not sure, but I don't think we have such logic in place yet; looks like we should probably create a separate bug or enhancement request asking for such behavior.
(In reply to comment #11) > (In reply to comment #10) > Xuan I'm not sure this is a valid testcase... when I understand right, you did > this: > > * In workspace A, expand a folder inside x.zip > * In workspace B, delete that folder inside x.zip > --> As a result, x.zip will be re-written to x.zip.tmp, then renamed > back into x.zip on the remote; in other words, the archive file > itself gets a different timestamp > > Because of that, I find it quite natural that the parent needs to be refreshed. > After all, the parent AND grandparent of the deleted virtual folder all get > invalid, don't they? The deleted folders parent and grandparent are being marked stale inside the DeleteJob. > Also, did you try this scenario on a workspace without Kevin's patch and did it > work as expected? I would assume there is the same failure so it would not be a > regression. It's actually a regression because before we would always return true when deleting a directory even if it failed. Therefore the folder would be removed from the view, so you couldn't try to refresh the deleted object. > In general, I think that when refreshing a virtual folder, the refresh logic > should look at the timestamp of the containing archive (i.e. check if it has > changed on the remote). If it has changed, the archive itself as well as all > its children need to be refreshed because anything could have changed inside > the archive. I'm not sure, but I don't think we have such logic in place yet; > looks like we should probably create a separate bug or enhancement request > asking for such behavior. > In addition if delete fails for reason FILE_DOES_NOT_EXIST then we should display an error, but also update the Remote Systems View. We know the file no longer exists and shouldn't force the user to do a refresh. The NPE is happening because we aren't checking if the list of children is null when going through them. I added in the null check, but now if you refresh the deleted folder it will show no children, but the folder is still displayed. On Local Archives the folder is also removed. On Local it checks for exists and SystemZipHandler's hash map does not contain an element for it as it no longer exists. On DStore we don't query the remote's SystemZipHandler and try to determine exists differently. For DStore we check if the DataElement.isDeleted() is true, but it returns false as the value for deleted is not set to "deleted". We then try to see if the type is proper, but it is as well, so exists() returns true. Doing this on normal DStore folders the file still returns true for exists for the same reasons, but in SystemView.add we do a refresh on the parent. When getting the children's content the DataElements type is updated to "universal.FilterObject" which is not a valid type for a DStoreHostFile so exists returns false and thats why the parent is refreshed. Since I can't get remote debugging to work on my machine I can't see where exactly normal files and virtual files are changing the type differently.
(In reply to comment #11) Yeah, the ideal behaviour should be: When I tried to deleted the virtual folder in workspace A, I got a message saying I cannot. Then, the archive file needs to be refreshed, and the deleted folder should be gone.
(In reply to comment #12) Kevin, I could help you debugging once I am finish the problem I am investigating.
Kevin - I think in your patch I'd like to see a comment why you return true in DStoreFileService, when sourceMsg.equals("") Just leave a note that this can only happen when running against an old (version 2.0.0) dstore server. BTW, is there a way for retrieving the dstore server version, and perhaps doing that check only if it's 2.0.0 but not if it's newer? For the code in DStoreHostFile, I think I'd like to see fewer lines changed compared to the original. The case of path==null or name==null is the special case, and could be handled like this: if (path==null) { //file was deleted on the host return null; } This would leave more of the original code untouched, keep the normal flow of operation clearer, and make it easier to e.g. set a breakpoint at the "return null" statement because in reality, no client should ever hit this (i.e. it should check for file.exists() before getting a name, or the file should be removed by some other means). BTW, those NPE issues remind me about problems we've had with the absolute path of DStoreHostFile's changing when they were renamed. The SystemView, and its elementMap, rely on the absolutePath of an element never changing. I think this was bug 184053, reading it might be instructive. For the rest, it looks like you'll need to debug why DataElement.isDeleted() does not return true.
(In reply to comment #15) > Kevin - I think in your patch I'd like to see a comment why you return true in > DStoreFileService, when sourceMsg.equals("") Will be in next patch. > Just leave a note that this can only happen when running against an old > (version 2.0.0) dstore server. BTW, is there a way for retrieving the dstore > server version, and perhaps doing that check only if it's 2.0.0 but not if it's > newer? Are we planning to update the version from 9 (2.0.0) to 10 with (2.0.1)? DataStore has getServerVersion() which returns an int. Currently it just returns 9 for both. > For the code in DStoreHostFile, I think I'd like to see fewer lines changed > compared to the original. The case of path==null or name==null is the special > case, and could be handled like this: > > if (path==null) { > //file was deleted on the host > return null; > } > > This would leave more of the original code untouched, keep the normal flow of > operation clearer, and make it easier to e.g. set a breakpoint at the "return > null" statement because in reality, no client should ever hit this (i.e. it > should check for file.exists() before getting a name, or the file should be > removed by some other means). I'll fix it up to look like that.
Created attachment 76331 [details] Updated based on Comments I've updated DStoreHostFile, added a comment for checking source message is the empty string, and updated ArchiveQueryThread that will make DataElement's type an invalid type so exists returns false. Legal Message: I, Kevin Doyle, declare that I developed attached code from scratch, without referencing any 3rd party materials except material licensed under the EPL. I am authorized by my employer, IBM Canada Ltd. to make this contribution under the EPL.
The fix looks good. I've tested the scenario in both Linux and Windows DStore, and it is fine. I will commit the change.
Patch is committed.
Verified fixed in I20071108.
