When fixing GEOS-7651, I made some changes to how resource move/rename works so that it actually functions properly on windows.
This has caused a test failure over in gs-restconfig. I have tracked this failure down to here:
https://github.com/geoserver/geoserver/blob/master/src/restconfig/src/test/java/org/geoserver/rest/resources/ResourceTest.java#L298
As far as I can determine, the test expects this line to fail, as it is moving a directory resource inside itself, which fails when using a standard java rename.
The new implementation of rename uses the Apache commons file utils, which is fully capable of moving a directory inside itself.
Am I save to delete this one line, so that the test just does the one, ‘proper’ rename, or is there some other reason why moving a directory resource inside itself should fail (In which case, I can modify rename to explicitly check for this)?
Thanks,
Torben
Why would you want to move a directory inside itself?
If you do this with a jdbcresourcestore, you will get resources in the database that are unreachable from the root, i.e. do not have an actual path any longer, and furthermore, a cyclic relationship that can cause an endless recursive loop if you scan through the moved directory recursively.
I do not see how it could be different with a file system. Would it not only cause inconsistencies?Or what is the expected end result? So why would you want to permit it? Why is it not a good thing to prevent this, what goal does it accomplish?
Regards
Niels
···
On 09/09/2016 07:07 PM, Torben Barsballe wrote:
When fixing GEOS-7651, I made some changes to how resource move/rename works so that it actually functions properly on windows.
This has caused a test failure over in gs-restconfig. I have tracked this failure down to here:
https://github.com/geoserver/geoserver/blob/master/src/restconfig/src/test/java/org/geoserver/rest/resources/ResourceTest.java#L298
As far as I can determine, the test expects this line to fail, as it is moving a directory resource inside itself, which fails when using a standard java rename.
The new implementation of rename uses the Apache commons file utils, which is fully capable of moving a directory inside itself.
Am I save to delete this one line, so that the test just does the one, ‘proper’ rename, or is there some other reason why moving a directory resource inside itself should fail (In which case, I can modify rename to explicitly check for this)?
Thanks,
Torben
This is basically what I was asking - why is it bad to allow this. You have given a reasonable answer - it would cause problems for jdbcresourcestore, so for consistency we should disallow it on the file system.
To answer your question about the expected result, renaming “mydir” to “mydir/mynewdir” would cause the directory “mydir” and all its contents to be relocated to the path “mydir/mynewdir”.
The new rename implementation allows this behavior incidentally. I can add checks to explicitly disallow it.
Torben
···
On Fri, Sep 9, 2016 at 4:59 PM, Niels Charlier <niels@anonymised.com> wrote:
Why would you want to move a directory inside itself?
If you do this with a jdbcresourcestore, you will get resources in the database that are unreachable from the root, i.e. do not have an actual path any longer, and furthermore, a cyclic relationship that can cause an endless recursive loop if you scan through the moved directory recursively.
I do not see how it could be different with a file system. Would it not only cause inconsistencies?Or what is the expected end result? So why would you want to permit it? Why is it not a good thing to prevent this, what goal does it accomplish?
Regards
Niels
On 09/09/2016 07:07 PM, Torben Barsballe wrote:
When fixing GEOS-7651, I made some changes to how resource move/rename works so that it actually functions properly on windows.
This has caused a test failure over in gs-restconfig. I have tracked this failure down to here:
https://github.com/geoserver/geoserver/blob/master/src/restconfig/src/test/java/org/geoserver/rest/resources/ResourceTest.java#L298
As far as I can determine, the test expects this line to fail, as it is moving a directory resource inside itself, which fails when using a standard java rename.
The new implementation of rename uses the Apache commons file utils, which is fully capable of moving a directory inside itself.
Am I save to delete this one line, so that the test just does the one, ‘proper’ rename, or is there some other reason why moving a directory resource inside itself should fail (In which case, I can modify rename to explicitly check for this)?
Thanks,
Torben
Ah - so what it actually does is only move the directory’s contents, but allow the source directory to remain in existence. Normally with a move, I expect the original directory to disappear (or become its destination), creating some kind of paradox. For this reason linux systems generally don’t allow it, afaik. I was a bit confused there.
The alternative is that we implement it this way in jdbcresourcestore as well. That is fine by me. But then there should be a new test for that behaviour instead! Rather than just removing the old test.
Kind Regards
Niels
···
On 09/12/2016 05:49 PM, Torben Barsballe wrote:
This is basically what I was asking - why is it bad to allow this. You have given a reasonable answer - it would cause problems for jdbcresourcestore, so for consistency we should disallow it on the file system.
To answer your question about the expected result, renaming “mydir” to “mydir/mynewdir” would cause the directory “mydir” and all its contents to be relocated to the path “mydir/mynewdir”.
The new rename implementation allows this behavior incidentally. I can add checks to explicitly disallow it.
Torben
On Fri, Sep 9, 2016 at 4:59 PM, Niels Charlier <niels@anonymised.com> wrote:
Why would you want to move a directory inside itself?
If you do this with a jdbcresourcestore, you will get resources in the database that are unreachable from the root, i.e. do not have an actual path any longer, and furthermore, a cyclic relationship that can cause an endless recursive loop if you scan through the moved directory recursively.
I do not see how it could be different with a file system. Would it not only cause inconsistencies?Or what is the expected end result? So why would you want to permit it? Why is it not a good thing to prevent this, what goal does it accomplish?
Regards
Niels
On 09/09/2016 07:07 PM, Torben Barsballe wrote:
When fixing GEOS-7651, I made some changes to how resource move/rename works so that it actually functions properly on windows.
This has caused a test failure over in gs-restconfig. I have tracked this failure down to here:
https://github.com/geoserver/geoserver/blob/master/src/restconfig/src/test/java/org/geoserver/rest/resources/ResourceTest.java#L298
As far as I can determine, the test expects this line to fail, as it is moving a directory resource inside itself, which fails when using a standard java rename.
The new implementation of rename uses the Apache commons file utils, which is fully capable of moving a directory inside itself.
Am I save to delete this one line, so that the test just does the one, ‘proper’ rename, or is there some other reason why moving a directory resource inside itself should fail (In which case, I can modify rename to explicitly check for this)?
Thanks,
Torben