Merge pull request #261 from clowder-framework/cilogon-save-file

cilogon less verbose + more logs for save error

Author: robkooper

CHANGELOG.md

@@ -6,10 +6,23 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 
 ## 1.18.1 - 2021-08-16
 
+This release fixes a critical issue where invalid zip files could result in the files not being uploaded correctly. To check to see if you are affected, please use the following query:
+
+```
+ db.uploads.find({"status": "CREATED", "contentType": "multi/files-zipped"})
+```
+
+If any files are returned, you should check to see if these files affected and are missing from clowder.
+
 ### Fixed
+- When zip file is uploaded, it will parse the file to check if it is a valid zip file, this couuld result in files not stored in final storage space [#264](https://github.com/clowder-framework/clowder/issues/264)
 - Updated swagger documentation
 - Return 404 not found when calling file/dataset/space api endpoints with an invalid ID [#251](https://github.com/clowder-framework/clowder/issues/251)
 
+### Changed
+- Added more information when writing files to make sure files are written correctly
+- Made cilogon group check debug message instead of error message
+
 ## 1.18.0 - 2021-07-08
 
 ### Added

app/fileutils/FilesUtils.java

@@ -71,8 +71,8 @@ else if(!fileName.toLowerCase().endsWith(".ptm")){
             if(allPTMsFlag.equals("found"))
             	return "multi/files-ptm-zipped";
 
-		} catch (IOException e) {
-			// TODO Auto-generated catch block
+		} catch (Throwable e) {
+			Logger.error("Could not read zipfile", e);
 			return ("ERROR: " + e.getMessage());
 		}		
 		return mainFileType;

app/services/CILogonProvider.scala

@@ -57,7 +57,7 @@ class CILogonProvider(application: Application) extends OAuth2Provider(applicati
               }
             }
             case (Some(_), None) => throw new AuthenticationException()
-            case (None, _) => Logger.error("[securesocial] No check needed for groups")
+            case (None, _) => Logger.debug("[securesocial] No check needed for groups")
           }
           user.copy(
             identityId = IdentityId(userId, id),

app/util/FileUtils.scala

@@ -399,14 +399,19 @@ object FileUtils {
     val fileExecutionContext: ExecutionContext = Akka.system().dispatchers.lookup("akka.actor.contexts.file-processing")
     Future {
       try {
-        saveFile(file, f.ref.file, originalZipFile, clowderurl, apiKey, Some(user)).foreach { fixedfile =>
-          processFileBytes(fixedfile, f.ref.file, dataset)
-          files.setStatus(fixedfile.id, FileStatus.UPLOADED)
-          sinkService.logFileUploadEvent(fixedfile, dataset, Option(user))
-          processFile(fixedfile, clowderurl, index, flagsFromPrevious, showPreviews, dataset, runExtractors, apiKey)
-          processDataset(file, dataset, folder, clowderurl, user, index, runExtractors, apiKey)
-          files.setStatus(fixedfile.id, FileStatus.PROCESSED)
+        saveFile(file, f.ref.file, originalZipFile, clowderurl, apiKey, Some(user)) match {
+          case Some(fixedfile) => {
+            processFileBytes(fixedfile, f.ref.file, dataset)
+            files.setStatus(fixedfile.id, FileStatus.UPLOADED)
+            sinkService.logFileUploadEvent(fixedfile, dataset, Option(user))
+            processFile(fixedfile, clowderurl, index, flagsFromPrevious, showPreviews, dataset, runExtractors, apiKey)
+            processDataset(file, dataset, folder, clowderurl, user, index, runExtractors, apiKey)
+            files.setStatus(fixedfile.id, FileStatus.PROCESSED)
+          }
+          case None => Logger.error(s"File was not saved for ${file.id}, saveFile returned None")
         }
+      } catch {
+        case e: Throwable => Logger.error(s"File was not saved for ${file.id}", e)
       } finally {
         f.ref.clean()
       }
@@ -455,12 +460,19 @@ object FileUtils {
     // process rest of file in background
     val fileExecutionContext: ExecutionContext = Akka.system().dispatchers.lookup("akka.actor.contexts.file-processing")
     Future {
-      saveURL(file, url, clowderurl, apiKey, Some(user)).foreach { fixedfile =>
-        processFileBytes(fixedfile, new java.io.File(path), fileds)
-        files.setStatus(fixedfile.id, FileStatus.UPLOADED)
-        processFile(fixedfile, clowderurl, index, flagsFromPrevious, showPreviews, fileds, runExtractors, apiKey)
-        processDataset(file, fileds, folder, clowderurl, user, index, runExtractors, apiKey)
-        files.setStatus(fixedfile.id, FileStatus.PROCESSED)
+      try {
+        saveURL(file, url, clowderurl, apiKey, Some(user)) match{
+          case Some(fixedfile) => {
+            processFileBytes(fixedfile, new java.io.File(path), fileds)
+            files.setStatus(fixedfile.id, FileStatus.UPLOADED)
+            processFile(fixedfile, clowderurl, index, flagsFromPrevious, showPreviews, fileds, runExtractors, apiKey)
+            processDataset(file, fileds, folder, clowderurl, user, index, runExtractors, apiKey)
+            files.setStatus(fixedfile.id, FileStatus.PROCESSED)
+          }
+          case None => Logger.error(s"File was not saved for ${file.id}, saveFile returned None")
+        }
+      } catch {
+        case e: Throwable => Logger.error(s"File was not saved for ${file.id}", e)
       }
     }(fileExecutionContext)