Skip to content

adding a bulk delete files method #153

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
lmarini merged 14 commits into from
Apr 27, 2021
Merged

adding a bulk delete files method #153

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
dbab2c4
adding a bulk delete files method
tcnichol Dec 20, 2020
e633903
changelog entry
tcnichol Jan 4, 2021
73e712c
preparing to check permissions in FileService method
tcnichol Mar 3, 2021
6547e76
adding check to see if user can delete file
tcnichol Mar 6, 2021
116c296
checks if user can delete file
tcnichol Mar 6, 2021
869e876
checks permission inside method
tcnichol Mar 7, 2021
2923f8f
Merge branch 'develop' into bulk-delete-file-api
tcnichol Mar 25, 2021
a250cb4
using the new checkPermissions method for list of resource ref instea…
tcnichol Mar 25, 2021
d6beba4
removing redundant permissions check
tcnichol Mar 31, 2021
6198cf4
matching develop
tcnichol Mar 31, 2021
e7eed03
adding swagger entry
tcnichol Mar 31, 2021
d8abe0b
Merge branch 'develop' into bulk-delete-file-api
tcnichol Mar 31, 2021
16e387c
Merge branch 'develop' into bulk-delete-file-api
lmarini Apr 27, 2021
8ce9c3b
Updated CHANGELOG.md.
lmarini Apr 27, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
- Log an event each time that a user archives or unarchives a file.
- Endpoint `/api/datasets/createfrombag` to ingest datasets in BagIt format. Includes basic dataset metadata, files,
folders and technical metadata. Downloading datasets now includes extra Datacite and Clowder metadata.
- Endpoint /api/files/bulkRemove to delete multiple files in one call. [#12](https://github.com/clowder-framework/clowder/issues/12)

## 1.16.0 - 2021-03-31

Expand Down
50 changes: 35 additions & 15 deletions app/api/Files.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,8 @@
package api

import scala.annotation.tailrec
import java.io.FileInputStream
import java.net.{URL, URLEncoder}

import javax.inject.Inject
import javax.mail.internet.MimeUtility
import _root_.util.{FileUtils, JSONLD, Parsers, RequestUtils, SearchUtils}
import _root_.util._
import com.mongodb.casbah.Imports._
import controllers.Previewers
import controllers.{Previewers, Utils}
import jsonutils.JsonUtil
import models._
import play.api.Logger
Expand All @@ -18,16 +12,16 @@ import play.api.libs.concurrent.Execution.Implicits._
import play.api.libs.iteratee.Enumerator
import play.api.libs.json.Json._
import play.api.libs.json._
import play.api.mvc.{Action, ResponseHeader, Result, SimpleResult}
import play.api.mvc.{ResponseHeader, SimpleResult}
import services._
import services.s3.S3ByteStorageService

import scala.collection.mutable.ListBuffer
import scala.util.parsing.json.JSONArray
import java.text.SimpleDateFormat
import java.io.FileInputStream
import java.net.URL
import java.util.Date

import controllers.Utils
import services.s3.S3ByteStorageService
import javax.inject.Inject
import scala.annotation.tailrec
import scala.collection.mutable.ListBuffer

/**
* Json API for files.
Expand Down Expand Up @@ -1616,6 +1610,32 @@ class Files @Inject()(
}
}

def bulkDeleteFiles() = PrivateServerAction (parse.json) {implicit request=>
Copy link
Copy Markdown
Member

@lmarini lmarini Mar 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

@tcnichol tcnichol Mar 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

@tcnichol tcnichol Mar 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made a commit that does that.

Copy link
Copy Markdown
Contributor

@max-zilla max-zilla Mar 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

@tcnichol tcnichol Mar 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@max-zilla thanks. that will be much better. i'll get this changed and push a new commit later today.

Copy link
Copy Markdown
Contributor Author

@tcnichol tcnichol Mar 31, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed that check. Also merged develop into the branch.

Copy link
Copy Markdown
Member

@lmarini lmarini Apr 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Copy Markdown
Contributor Author

@tcnichol tcnichol Apr 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

@lmarini lmarini Apr 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor Author

@tcnichol tcnichol Apr 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like I removed the redundant check he mentioned a few commits ago on this one.

request.user match {
case Some(user) => {
val fileIds = request.body.\("fileIds").asOpt[List[String]].getOrElse(List.empty[String])
if (fileIds.isEmpty){
BadRequest("No file ids supplied")
} else {
var resourceRefList: ListBuffer[ResourceRef] = ListBuffer.empty[ResourceRef]
for (fileId <- fileIds) {
if (UUID.isValid(fileId)) {
val current_resource_ref = ResourceRef(ResourceRef.file, UUID(fileId))
resourceRefList += current_resource_ref
}
}
val filesIdsCanDelete = Permission.checkPermissions(request.user, Permission.DeleteFile, resourceRefList.toList).approved.map(_.id)
for (id <- filesIdsCanDelete) {
files.removeFile(id,Utils.baseUrl(request), request.apiKey, request.user)
}
Ok(toJson(Map("status" -> "success")))
}
}
case None => {
BadRequest("No user supplied")
}
}
}

def removeFile(id: UUID) = PermissionAction(Permission.DeleteFile, Some(ResourceRef(ResourceRef.file, id))) { implicit request =>
files.get(id) match {
Expand Down
17 changes: 6 additions & 11 deletions app/controllers/Application.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,20 +1,14 @@
package controllers

import java.net.URL

import javax.inject.{Inject, Singleton}
import api.Permission
import api.Permission._
import play.api.{Logger, Play, Routes}
import models.{Event, UUID, UserStatus}
import play.api.Play.current
import play.api.mvc.Action
import play.api.{Logger, Play, Routes}
import services._
import models.{Event, UUID, User, UserStatus}
import org.owasp.html.Sanitizers
import play.api.Logger
import play.api.libs.concurrent.Execution.Implicits._
import play.api.Play.current
import util.Formatters.sanitizeHTML

import java.net.URL
import javax.inject.{Inject, Singleton}
import scala.collection.immutable.List
import scala.collection.mutable.ListBuffer

Expand Down Expand Up @@ -368,6 +362,7 @@ class Application @Inject() (files: FileService, collections: CollectionService,
api.routes.javascript.Files.updateFileName,
api.routes.javascript.Files.updateDescription,
api.routes.javascript.Files.extract,
api.routes.javascript.Files.bulkDeleteFiles,
api.routes.javascript.Files.removeFile,
api.routes.javascript.Files.follow,
api.routes.javascript.Files.unfollow,
Expand Down
104 changes: 52 additions & 52 deletions app/services/mongodb/MongoDBFileService.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -806,67 +806,67 @@ class MongoDBFileService @Inject() (
def removeFile(id: UUID, host: String, apiKey: Option[String], user: Option[User]){
get(id) match{
case Some(file) => {
if(!file.isIntermediate){
val fileDatasets = datasets.findByFileIdDirectlyContain(file.id)
for(fileDataset <- fileDatasets){
datasets.removeFile(fileDataset.id, id)
if(!file.xmlMetadata.isEmpty){
datasets.index(fileDataset.id)
}
if(!file.isIntermediate){
val fileDatasets = datasets.findByFileIdDirectlyContain(file.id)
for(fileDataset <- fileDatasets){
datasets.removeFile(fileDataset.id, id)
if(!file.xmlMetadata.isEmpty){
datasets.index(fileDataset.id)
}

if(!file.thumbnail_id.isEmpty && !fileDataset.thumbnail_id.isEmpty){
if(file.thumbnail_id.get.equals(fileDataset.thumbnail_id.get)){
datasets.newThumbnail(fileDataset.id)
collections.get(fileDataset.collections).found.foreach(collection => {
if(!collection.thumbnail_id.isEmpty){
if(collection.thumbnail_id.get.equals(fileDataset.thumbnail_id.get)){
collections.createThumbnail(collection.id)
if(!file.thumbnail_id.isEmpty && !fileDataset.thumbnail_id.isEmpty){
if(file.thumbnail_id.get.equals(fileDataset.thumbnail_id.get)){
datasets.newThumbnail(fileDataset.id)
collections.get(fileDataset.collections).found.foreach(collection => {
if(!collection.thumbnail_id.isEmpty){
if(collection.thumbnail_id.get.equals(fileDataset.thumbnail_id.get)){
collections.createThumbnail(collection.id)
}
}
}
})
}
})
}
}

}

}

val fileFolders = folders.findByFileId(file.id)
for(fileFolder <- fileFolders) {
folders.removeFile(fileFolder.id, file.id)
}
for(section <- sections.findByFileId(file.id)){
sections.removeSection(section)
}
for(comment <- comments.findCommentsByFileId(id)){
comments.removeComment(comment)
}
for(texture <- threeD.findTexturesByFileId(file.id)){
ThreeDTextureDAO.removeById(new ObjectId(texture.id.stringify))
}
for (follower <- file.followers) {
userService.unfollowFile(follower, id)
val fileFolders = folders.findByFileId(file.id)
for(fileFolder <- fileFolders) {
folders.removeFile(fileFolder.id, file.id)
}
for(section <- sections.findByFileId(file.id)){
sections.removeSection(section)
}
for(comment <- comments.findCommentsByFileId(id)){
comments.removeComment(comment)
}
for(texture <- threeD.findTexturesByFileId(file.id)){
ThreeDTextureDAO.removeById(new ObjectId(texture.id.stringify))
}
for (follower <- file.followers) {
userService.unfollowFile(follower, id)
}
}
}

// delete the actual file
if(isLastPointingToLoader(file.loader, file.loader_id)) {
for(preview <- previews.findByFileId(file.id)){
previews.removePreview(preview)
// delete the actual file
if(isLastPointingToLoader(file.loader, file.loader_id)) {
for(preview <- previews.findByFileId(file.id)){
previews.removePreview(preview)
}
if(!file.thumbnail_id.isEmpty)
thumbnails.remove(UUID(file.thumbnail_id.get))
ByteStorageService.delete(file.loader, file.loader_id, FileDAO.COLLECTION)
}
if(!file.thumbnail_id.isEmpty)
thumbnails.remove(UUID(file.thumbnail_id.get))
ByteStorageService.delete(file.loader, file.loader_id, FileDAO.COLLECTION)
}

import UUIDConversions._
FileDAO.removeById(file.id)
appConfig.incrementCount('files, -1)
appConfig.incrementCount('bytes, -file.length)
current.plugin[ElasticsearchPlugin].foreach {
_.delete(id.stringify)
}
import UUIDConversions._
FileDAO.removeById(file.id)
appConfig.incrementCount('files, -1)
appConfig.incrementCount('bytes, -file.length)
current.plugin[ElasticsearchPlugin].foreach {
_.delete(id.stringify)
}

// finally remove metadata - if done before file is deleted, document metadataCounts won't match
metadatas.removeMetadataByAttachTo(ResourceRef(ResourceRef.file, id), host, apiKey, user)
// finally remove metadata - if done before file is deleted, document metadataCounts won't match
metadatas.removeMetadataByAttachTo(ResourceRef(ResourceRef.file, id), host, apiKey, user)
}
case None => Logger.debug("File not found")
}
Expand Down
1 change: 1 addition & 0 deletions conf/routes
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -388,6 +388,7 @@ GET /api/files/:id/blob

# deprecrated use DELETE
POST /api/files/:id/remove @api.Files.removeFile(id: UUID)
POST /api/files/bulkRemove @api.Files.bulkDeleteFiles()
Comment thread
lmarini marked this conversation as resolved.
GET /api/files/:id/metadata @api.Files.get(id: UUID)
POST /api/files/:id/metadata @api.Files.addMetadata(id: UUID)
GET /api/files/:id/metadataDefinitions @api.Files.getMetadataDefinitions(id: UUID, space: Option[String] ?= None)
Expand Down
18 changes: 18 additions & 0 deletions public/swagger.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,24 @@ paths:
'401':
$ref: '#/components/responses/Disabled'

/files/bulkRemove:
post:
tags:
- files
summary: Deletes files
descriptions: Deletes a list of files by fileIds
responses:
'200':
description: Returns Status Success
content:
application/json:
schema:
type: array
items:
$ref: '#components/parameters/fileIds'
'401':
$ref: '#components/responses/Disabled'

/files/{id}:
parameters:
- $ref: '#/components/parameters/fileId'
Expand Down