ZIP Import: Finished base import process & error handling

ssddanbrown · ssddanbrown · commit b7476a9e7fc2 · 2024-11-14T15:59:15.000Z
Added file creation reverting and DB rollback on error.
Added error display on failed import.
Extracted likely shown import form/error text to translation files.
diff --git a/app/Exports/Controllers/ImportController.php b/app/Exports/Controllers/ImportController.php
@@ -4,7 +4,7 @@
 
 namespace BookStack\Exports\Controllers;
 
-use BookStack\Activity\ActivityType;
+use BookStack\Exceptions\ZipImportException;
 use BookStack\Exceptions\ZipValidationException;
 use BookStack\Exports\ImportRepo;
 use BookStack\Http\Controller;
@@ -48,12 +48,9 @@ public function upload(Request $request)
         try {
             $import = $this->imports->storeFromUpload($file);
         } catch (ZipValidationException $exception) {
-            session()->flash('validation_errors', $exception->errors);
-            return redirect('/import');
+            return redirect('/import')->with('validation_errors', $exception->errors);
         }
 
-        $this->logActivity(ActivityType::IMPORT_CREATE, $import);
-
         return redirect($import->getUrl());
     }
 
@@ -80,20 +77,20 @@ public function run(int $id, Request $request)
         $parent = null;
 
         if ($import->type === 'page' || $import->type === 'chapter') {
+            session()->setPreviousUrl($import->getUrl());
             $data = $this->validate($request, [
-                'parent' => ['required', 'string']
+                'parent' => ['required', 'string'],
             ]);
             $parent = $data['parent'];
         }
 
-        $entity = $this->imports->runImport($import, $parent);
-        if ($entity) {
-            $this->logActivity(ActivityType::IMPORT_RUN, $import);
-            return redirect($entity->getUrl());
+        try {
+            $entity = $this->imports->runImport($import, $parent);
+        } catch (ZipImportException $exception) {
+            return redirect($import->getUrl())->with('import_errors', $exception->errors);
         }
-        // TODO - Redirect to result
-        // TODO - Or redirect back with errors
-        return 'failed';
+
+        return redirect($entity->getUrl());
     }
 
     /**
@@ -104,8 +101,6 @@ public function delete(int $id)
         $import = $this->imports->findVisible($id);
         $this->imports->deleteImport($import);
 
-        $this->logActivity(ActivityType::IMPORT_DELETE, $import);
-
         return redirect('/import');
     }
 }
diff --git a/app/Exports/ImportRepo.php b/app/Exports/ImportRepo.php
@@ -2,6 +2,7 @@
 
 namespace BookStack\Exports;
 
+use BookStack\Activity\ActivityType;
 use BookStack\Entities\Models\Entity;
 use BookStack\Entities\Queries\EntityQueries;
 use BookStack\Exceptions\FileUploadException;
@@ -14,8 +15,10 @@
 use BookStack\Exports\ZipExports\ZipExportReader;
 use BookStack\Exports\ZipExports\ZipExportValidator;
 use BookStack\Exports\ZipExports\ZipImportRunner;
+use BookStack\Facades\Activity;
 use BookStack\Uploads\FileStorage;
 use Illuminate\Database\Eloquent\Collection;
+use Illuminate\Support\Facades\DB;
 use Symfony\Component\HttpFoundation\File\UploadedFile;
 
 class ImportRepo
@@ -93,25 +96,42 @@ public function storeFromUpload(UploadedFile $file): Import
         $import->path = $path;
         $import->save();
 
+        Activity::add(ActivityType::IMPORT_CREATE, $import);
+
         return $import;
     }
 
     /**
-     * @throws ZipValidationException|ZipImportException
+     * @throws ZipImportException
      */
-    public function runImport(Import $import, ?string $parent = null): ?Entity
+    public function runImport(Import $import, ?string $parent = null): Entity
     {
         $parentModel = null;
         if ($import->type === 'page' || $import->type === 'chapter') {
             $parentModel = $parent ? $this->entityQueries->findVisibleByStringIdentifier($parent) : null;
         }
 
-        return $this->importer->run($import, $parentModel);
+        DB::beginTransaction();
+        try {
+            $model = $this->importer->run($import, $parentModel);
+        } catch (ZipImportException $e) {
+            DB::rollBack();
+            $this->importer->revertStoredFiles();
+            throw $e;
+        }
+
+        DB::commit();
+        $this->deleteImport($import);
+        Activity::add(ActivityType::IMPORT_RUN, $import);
+
+        return $model;
     }
 
     public function deleteImport(Import $import): void
     {
         $this->storage->delete($import->path);
         $import->delete();
+
+        Activity::add(ActivityType::IMPORT_DELETE, $import);
     }
 }
diff --git a/app/Exports/ZipExports/ZipImportReferences.php b/app/Exports/ZipExports/ZipImportReferences.php
@@ -139,4 +139,21 @@ public function replaceReferences(): void
             ]);
         }
     }
+
+
+    /**
+     * @return Image[]
+     */
+    public function images(): array
+    {
+        return $this->images;
+    }
+
+    /**
+     * @return Attachment[]
+     */
+    public function attachments(): array
+    {
+        return $this->attachments;
+    }
 }
diff --git a/app/Exports/ZipExports/ZipImportRunner.php b/app/Exports/ZipExports/ZipImportRunner.php
@@ -47,14 +47,17 @@ public function __construct(
      * Returns the top-level entity item which was imported.
      * @throws ZipImportException
      */
-    public function run(Import $import, ?Entity $parent = null): ?Entity
+    public function run(Import $import, ?Entity $parent = null): Entity
     {
         $zipPath = $this->getZipPath($import);
         $reader = new ZipExportReader($zipPath);
 
         $errors = (new ZipExportValidator($reader))->validate();
         if ($errors) {
-            throw new ZipImportException(["ZIP failed to validate"]);
+            throw new ZipImportException([
+                trans('errors.import_validation_failed'),
+                ...$errors,
+            ]);
         }
 
         try {
@@ -65,48 +68,61 @@ public function run(Import $import, ?Entity $parent = null): ?Entity
 
         // Validate parent type
         if ($exportModel instanceof ZipExportBook && ($parent !== null)) {
-            throw new ZipImportException(["Must not have a parent set for a Book import"]);
-        } else if ($exportModel instanceof ZipExportChapter && (!$parent instanceof Book)) {
-            throw new ZipImportException(["Parent book required for chapter import"]);
+            throw new ZipImportException(["Must not have a parent set for a Book import."]);
+        } else if ($exportModel instanceof ZipExportChapter && !($parent instanceof Book)) {
+            throw new ZipImportException(["Parent book required for chapter import."]);
         } else if ($exportModel instanceof ZipExportPage && !($parent instanceof Book || $parent instanceof Chapter)) {
-            throw new ZipImportException(["Parent book or chapter required for page import"]);
+            throw new ZipImportException(["Parent book or chapter required for page import."]);
         }
 
-        $this->ensurePermissionsPermitImport($exportModel);
-        $entity = null;
+        $this->ensurePermissionsPermitImport($exportModel, $parent);
 
         if ($exportModel instanceof ZipExportBook) {
             $entity = $this->importBook($exportModel, $reader);
         } else if ($exportModel instanceof ZipExportChapter) {
             $entity = $this->importChapter($exportModel, $parent, $reader);
         } else if ($exportModel instanceof ZipExportPage) {
             $entity = $this->importPage($exportModel, $parent, $reader);
+        } else {
+            throw new ZipImportException(['No importable data found in import data.']);
         }
 
-          // TODO - In transaction?
-            // TODO - Revert uploaded files if goes wrong
-              // TODO - Attachments
-              // TODO - Images
-              // (Both listed/stored in references)
-
         $this->references->replaceReferences();
 
         $reader->close();
         $this->cleanup();
 
-        dd('stop');
+        return $entity;
+    }
 
-        // TODO - Delete import/zip after import?
-          // Do this in parent repo?
+    /**
+     * Revert any files which have been stored during this import process.
+     * Considers files only, and avoids the database under the
+     * assumption that the database may already have been
+     * reverted as part of a transaction rollback.
+     */
+    public function revertStoredFiles(): void
+    {
+        foreach ($this->references->images() as $image) {
+            $this->imageService->destroyFileAtPath($image->type, $image->path);
+        }
 
-        return $entity;
+        foreach ($this->references->attachments() as $attachment) {
+            if (!$attachment->external) {
+                $this->attachmentService->deleteFileInStorage($attachment);
+            }
+        }
+
+        $this->cleanup();
     }
 
-    protected function cleanup()
+    protected function cleanup(): void
     {
         foreach ($this->tempFilesToCleanup as $file) {
             unlink($file);
         }
+
+        $this->tempFilesToCleanup = [];
     }
 
     protected function importBook(ZipExportBook $exportBook, ZipExportReader $reader): Book
@@ -256,17 +272,14 @@ protected function ensurePermissionsPermitImport(ZipExportPage|ZipExportChapter|
     {
         $errors = [];
 
-        // TODO - Extract messages to language files
-        // TODO - Ensure these are shown to users on failure
-
         $chapters = [];
         $pages = [];
         $images = [];
         $attachments = [];
 
         if ($exportModel instanceof ZipExportBook) {
             if (!userCan('book-create-all')) {
-                $errors[] = 'You are lacking the required permission to create books.';
+                $errors[] = trans('errors.import_perms_books');
             }
             array_push($pages, ...$exportModel->pages);
             array_push($chapters, ...$exportModel->chapters);
@@ -283,7 +296,7 @@ protected function ensurePermissionsPermitImport(ZipExportPage|ZipExportChapter|
         if (count($chapters) > 0) {
             $permission = 'chapter-create' . ($parent ? '' : '-all');
             if (!userCan($permission, $parent)) {
-                $errors[] = 'You are lacking the required permission to create chapters.';
+                $errors[] = trans('errors.import_perms_chapters');
             }
         }
 
@@ -295,25 +308,25 @@ protected function ensurePermissionsPermitImport(ZipExportPage|ZipExportChapter|
         if (count($pages) > 0) {
             if ($parent) {
                 if (!userCan('page-create', $parent)) {
-                    $errors[] = 'You are lacking the required permission to create pages.';
+                    $errors[] = trans('errors.import_perms_pages');
                 }
             } else {
                 $hasPermission = userCan('page-create-all') || userCan('page-create-own');
                 if (!$hasPermission) {
-                    $errors[] = 'You are lacking the required permission to create pages.';
+                    $errors[] = trans('errors.import_perms_pages');
                 }
             }
         }
 
         if (count($images) > 0) {
             if (!userCan('image-create-all')) {
-                $errors[] = 'You are lacking the required permissions to create images.';
+                $errors[] = trans('errors.import_perms_images');
             }
         }
 
         if (count($attachments) > 0) {
             if (!userCan('attachment-create-all')) {
-                $errors[] = 'You are lacking the required permissions to create attachments.';
+                $errors[] = trans('errors.import_perms_attachments');
             }
         }
 
diff --git a/app/Uploads/AttachmentService.php b/app/Uploads/AttachmentService.php
@@ -151,7 +151,7 @@ public function deleteFile(Attachment $attachment)
      * Delete a file from the filesystem it sits on.
      * Cleans any empty leftover folders.
      */
-    protected function deleteFileInStorage(Attachment $attachment): void
+    public function deleteFileInStorage(Attachment $attachment): void
     {
         $this->storage->delete($attachment->path);
     }
diff --git a/app/Uploads/ImageService.php b/app/Uploads/ImageService.php
@@ -153,11 +153,19 @@ public function getImageStream(Image $image): mixed
      */
     public function destroy(Image $image): void
     {
-        $disk = $this->storage->getDisk($image->type);
-        $disk->destroyAllMatchingNameFromPath($image->path);
+        $this->destroyFileAtPath($image->type, $image->path);
         $image->delete();
     }
 
+    /**
+     * Destroy the underlying image file at the given path.
+     */
+    public function destroyFileAtPath(string $type, string $path): void
+    {
+        $disk = $this->storage->getDisk($type);
+        $disk->destroyAllMatchingNameFromPath($path);
+    }
+
     /**
      * Delete gallery and drawings that are not within HTML content of pages or page revisions.
      * Checks based off of only the image name.
diff --git a/lang/en/entities.php b/lang/en/entities.php
@@ -52,6 +52,7 @@
     'import_pending_none' => 'No imports have been started.',
     'import_continue' => 'Continue Import',
     'import_continue_desc' => 'Review the content due to be imported from the uploaded ZIP file. When ready, run the import to add its contents to this system. The uploaded ZIP import file will be automatically removed on successful import.',
+    'import_details' => 'Import Details',
     'import_run' => 'Run Import',
     'import_size' => ':size Import ZIP Size',
     'import_uploaded_at' => 'Uploaded :relativeTime',
@@ -60,6 +61,8 @@
     'import_location_desc' => 'Select a target location for your imported content. You\'ll need the relevant permissions to create within the location you choose.',
     'import_delete_confirm' => 'Are you sure you want to delete this import?',
     'import_delete_desc' => 'This will delete the uploaded import ZIP file, and cannot be undone.',
+    'import_errors' => 'Import Errors',
+    'import_errors_desc' => 'The follow errors occurred during the import attempt:',
 
     // Permissions and restrictions
     'permissions' => 'Permissions',
diff --git a/lang/en/errors.php b/lang/en/errors.php
@@ -109,6 +109,12 @@
     'import_zip_cant_read' => 'Could not read ZIP file.',
     'import_zip_cant_decode_data' => 'Could not find and decode ZIP data.json content.',
     'import_zip_no_data' => 'ZIP file data has no expected book, chapter or page content.',
+    'import_validation_failed' => 'Import ZIP failed to validate with errors:',
+    'import_perms_books' => 'You are lacking the required permissions to create books.',
+    'import_perms_chapters' => 'You are lacking the required permissions to create chapters.',
+    'import_perms_pages' => 'You are lacking the required permissions to create pages.',
+    'import_perms_images' => 'You are lacking the required permissions to create images.',
+    'import_perms_attachments' => 'You are lacking the required permission to create attachments.',
 
     // API errors
     'api_no_authorization_found' => 'No authorization token found on the request',
diff --git a/resources/views/exports/import-show.blade.php b/resources/views/exports/import-show.blade.php
diff --git a/resources/views/exports/parts/import.blade.php b/resources/views/exports/parts/import.blade.php

Original file line number	Diff line number	Diff line change
`@@ -151,7 +151,7 @@ public function deleteFile(Attachment $attachment)`
`151`	`151`	`* Delete a file from the filesystem it sits on.`
`152`	`152`	`* Cleans any empty leftover folders.`
`153`	`153`	`*/`
`154`		`- protected function deleteFileInStorage(Attachment $attachment): void`
	`154`	`+ public function deleteFileInStorage(Attachment $attachment): void`
`155`	`155`	`{`
`156`	`156`	`$this->storage->delete($attachment->path);`
`157`	`157`	`}`