To follow up on the github discussion regarding the resources delete API. As I understand we have three methods to delete resources:
1. Resource.delete() - non recursive (until last PR), returns false if file doesn't exist
2. ResourceStore.remove (String path) - recursive, returns true if file does exist
3. Resources.delete(Resource res) - recursive, returns false if file doesn't exist
I understand that the idea of creating (3) is that it would factor out the recursive algorithm, leaving to implementers of resource stores only the need to implement the deletion of one resource / empty directory. But then (2) is still recursive, which enforces an implementation of the recursion in the resource store anyway.
I understand the reasoning, but recursive delete can be made much more efficiently for the JDBCStore than the implementation in (3) with less queries sent to the database. I think that resource stores should be allowed to have their own implementation for that reason. Perhaps one future implementation of resource store could delete a full directory in one go, without the need for recursion.
So yes, I'd agree to get rid of the (3) method all together and change the contract.
Another thing I'd like to see changed is that I'd like the rule on whether to return true or false on a non-existing file should be the same for (1) and (2). I don't care which is chosen, but since method (2) is hardly (if anywhere at all) used, it would be least risky to change it. Unless there was/is a conscious reason to do it this way, of course. It should be better documented then though, because at this moment the behaviour towards non-existing files for (1) is not documented, which may lead people to believe that it will the same as (2), which is explicitly documented.
Thanks for taking discussion to the email list - I had to break up your pull request as it included a fix (to be back ported) and an API change.
You have two API changes in the works right - a request to create a directory (something like resource.dir()?) along with making resource.delete() recursive.
So yeah if you feel that implementing resource.delete() is easier to implement - perhaps by calling to ResourceStore.remove( path ) - then I am fine to make the change. We can also remove Resources.delete( resource ) as I do not want us maintaining more than one way to do things.
We should also make the return value consistent. The return value was chosen for consistency with File.delete(). Much like making Resource method compatible with File, the return value made it easier to migrate code that made direct use of File.
A question about resource.delete() being much more efficient to implement, are you checking (and acquiring) all the resource locks before deleting?
With respect to creating a directory - I want to avoid putting developers in the situation where the API is forcing them to create a directory using boilerplate code prior creating files. Your earlier example showed directory creation as a placeholder (almost like a sentinel file) so I was not quite sure if it was a poor example or if you had an actual use case.
To follow up on the github discussion regarding the resources delete API. As I understand we have three methods to delete resources:
Resource.delete() - non recursive (until last PR), returns false if file doesn’t exist
ResourceStore.remove (String path) - recursive, returns true if file does exist
Resources.delete(Resource res) - recursive, returns false if file doesn’t exist
I understand that the idea of creating (3) is that it would factor out the recursive algorithm, leaving to implementers of resource stores only the need to implement the deletion of one resource / empty directory. But then (2) is still recursive, which enforces an implementation of the recursion in the resource store anyway.
I understand the reasoning, but recursive delete can be made much more efficiently for the JDBCStore than the implementation in (3) with less queries sent to the database. I think that resource stores should be allowed to have their own implementation for that reason. Perhaps one future implementation of resource store could delete a full directory in one go, without the need for recursion.
So yes, I’d agree to get rid of the (3) method all together and change the contract.
Another thing I’d like to see changed is that I’d like the rule on whether to return true or false on a non-existing file should be the same for (1) and (2). I don’t care which is chosen, but since method (2) is hardly (if anywhere at all) used, it would be least risky to change it. Unless there was/is a conscious reason to do it this way, of course. It should be better documented then though, because at this moment the behaviour towards non-existing files for (1) is not documented, which may lead people to believe that it will the same as (2), which is explicitly documented.
We should also make the return value consistent. The return value was chosen for consistency with File.delete(). Much like making Resource method compatible with File, the return value made it easier to migrate code that made direct use of File.
Then ResourceStore.remove will have to change. And Files.delete as well (?)
A question about resource.delete() being much more efficient to implement, are you checking (and acquiring) all the resource locks before deleting?
Hmmm. That is a good point. But FileSystemResource.delete doesn't acquire a lock. Is that normal?
Either way, we can get the locks recursively first. Best to keep sql queries as efficient as possible.
With respect to creating a directory - I want to avoid putting developers in the situation where the API is forcing them to create a directory using boilerplate code prior creating files. Your earlier example showed directory creation as a placeholder (almost like a sentinel file) so I was not quite sure if it was a poor example or if you had an actual use case.
Don't worry about this for now. My main concern is that we stop using Files wherever possible, also in many cases where getOrCreateDirectory is used. But as we concluded, we may as well just "get" the directory, and it will be created automatically when create the first child. The only thing I'd consider is return an empty list instead of a "null" for a non-existing directory to make the use of directories easier that way.
We should also make the return value consistent. The return value was chosen for consistency with File.delete(). Much like making Resource method compatible with File, the return value made it easier to migrate code that made direct use of File.
···
Then ResourceStore.remove will have to change. And Files.delete as well (?)
Sensible, would treat the inconsistency as a bug. Our intention was to make transition from File easier.
A question about resource.delete() being much more efficient to implement, are you checking (and acquiring) all the resource locks before deleting?
Hmmm. That is a good point. But FileSystemResource.delete doesn’t acquire a lock. Is that normal?
Either way, we can get the locks recursively first. Best to keep sql queries as efficient as possible.
Okay, so lets go ahead with the proposal then.
With respect to creating a directory - I want to avoid putting developers in the situation where the API is forcing them to create a directory using boilerplate code prior creating files. Your earlier example showed directory creation as a placeholder (almost like a sentinel file) so I was not quite sure if it was a poor example or if you had an actual use case.
Don’t worry about this for now.
Only reason to think about it is we could cover it in the same proposal.
My main concern is that we stop using Files wherever possible, also in many cases where getOrCreateDirectory is used. But as we concluded, we may as well just “get” the directory, and it will be created automatically when create the first child. The only thing I’d consider is return an empty list instead of a “null” for a non-existing directory to make the use of directories easier that way.
Fair enough, yeah I like it, add it to the proposal.
Hmmm. That is a good point. But FileSystemResource.delete doesn't acquire a lock. Is that normal?
Either way, we can get the locks recursively first. Best to keep sql queries as efficient as possible.
Hi Jody, you never responded to this.
Should this be reported as a bug, that FileSystemResource.delete doesn't acquire a lock?
Hmmm. That is a good point. But FileSystemResource.delete doesn’t
acquire a lock. Is that normal?
Either way, we can get the locks recursively first. Best to keep sql
queries as efficient as possible.
Hi Jody, you never responded to this.
Should this be reported as a bug, that FileSystemResource.delete doesn’t
acquire a lock?