adding a bulk delete files method#153
Conversation
|
@max-zilla can you help me review this one? I believe it is related to some of the batch actions you have implemented on files in a dataset. Thank you. |
| } | ||
| } | ||
|
|
||
| def bulkDeleteFiles() = PrivateServerAction (parse.json) {implicit request=> |
There was a problem hiding this comment.
This action would let anyone logged in call this action and the removeFile call doesn't seem to reinforce permissions. We should use something like def removeFile(id: UUID) = PermissionAction(Permission.DeleteFile, Some(ResourceRef(ResourceRef.file, id))) but I am not sure which permission to use since I don't believe we can pass in a list of resources.
There was a problem hiding this comment.
Would another option be to files the FileService removeFile method to take permissions into account? It looks like it's supposed to do that, but just doesn't.
There was a problem hiding this comment.
I made a commit that does that.
There was a problem hiding this comment.
I implemented the ability to check a list of resources at once a while ago: https://github.com/clowder-framework/clowder/blob/develop/app/api/Permissions.scala#L282 I also added a bulk GET for a list of files: https://github.com/clowder-framework/clowder/blob/develop/app/services/FileService.scala#L104
You can use these in combination like this:
https://github.com/clowder-framework/clowder/blob/develop/app/util/SearchUtils.scala#L302
both of these calls return little objects that tell you what does/doesn't have permission and what was/wasn't found (if you asked for multiple files). Please use this pattern instead. The changes here, you added a new permission check inside the file service but existing uses of that call in api Files have already done the check so now it is checking twice.
If you remove the permission check in the service, and use the pattern above inside bulkDeleteFiles, it will use existing code and be a bit more efficient.
There was a problem hiding this comment.
@max-zilla thanks. that will be much better. i'll get this changed and push a new commit later today.
There was a problem hiding this comment.
I removed that check. Also merged develop into the branch.
There was a problem hiding this comment.
@tcnichol @max-zilla not sure about the changes to https://github.com/clowder-framework/clowder/blob/develop/app/services/mongodb/MongoDBFileService.scala#L818. I don't see are reference to permissions and the changes in the branch just look like spacing changes? Am I looking in the wrong place? Thank you.
There was a problem hiding this comment.
That commit is just spacing changes.
The actual change in checking permissions is in this file:
https://github.com/clowder-framework/clowder/pull/153/files'
line 1627 uses the new checkPermission method that takes in a list of resourceRef. I then removed any permission checks elsewhere.
There was a problem hiding this comment.
yeah, I saw the change to the controller, I understand that part. I am not sure I saw a change related to what @max-zilla said above regarding "Can you please also remove the permission check in 818 of MongoDB File Service as well, so we don't have redundant calls there." that's the file where there is white space changes, but nothing else?
There was a problem hiding this comment.
It looks like I removed the redundant check he mentioned a few commits ago on this one.
needs to be improved, moved to separate method
this might not be the best approach
only deletes files with proper permission
| datasets.index(fileDataset.id) | ||
| } | ||
| val currentResourceRef = ResourceRef(ResourceRef.file, file.id) | ||
| val hasPermission = Permission.checkPermission(user.get,Permission.DeleteFile, currentResourceRef) |
There was a problem hiding this comment.
This call is redundant and has already been done in most cases for this function.
| if (fileIds.isEmpty){ | ||
| BadRequest("No file ids supplied") | ||
| } else { | ||
| for (fileId <- fileIds){ |
There was a problem hiding this comment.
you can use a pattern like this to check many file permissions at once https://github.com/clowder-framework/clowder/blob/develop/app/util/SearchUtils.scala#L302
|
Also please add entry to swagger.yml for your new endpoint. |
# Conflicts: # CHANGELOG.md
…d of changing code in the fileservice method
| } | ||
| } | ||
|
|
||
| def bulkDeleteFiles() = PrivateServerAction (parse.json) {implicit request=> |
There was a problem hiding this comment.
The changed calls here look good! Can you please also remove the permission check in 818 of MongoDB File Service as well, so we don't have redundant calls there. Then I should be able to approve
lmarini
left a comment
lmarini
left a comment
There was a problem hiding this comment.
Just a question about one of the proposed changes, otherwise the rest looks good. Need to add changelog entry.
| } | ||
| } | ||
|
|
||
| def bulkDeleteFiles() = PrivateServerAction (parse.json) {implicit request=> |
There was a problem hiding this comment.
@tcnichol @max-zilla not sure about the changes to https://github.com/clowder-framework/clowder/blob/develop/app/services/mongodb/MongoDBFileService.scala#L818. I don't see are reference to permissions and the changes in the branch just look like spacing changes? Am I looking in the wrong place? Thank you.
lmarini
left a comment
lmarini
left a comment
There was a problem hiding this comment.
It works well. Two followups:
- @tcnichol I have created a followup issue #209
- @max-zilla can you double check your comment and my comments below regarding "remove the permission check in 818 of MongoDB File Service" and see if we missed something?
4 participants
There was an issue (#12) about bulk delete files tagged api.
Multiple files can be deleted using the marked feature in datasets, but I noticed that there was still no bulk delete endpoint in the api that fit what was requested in the issue.