ZIP Import: Finished base import process & error handling

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.

app/Exports/Controllers/ImportController.php

@@ -4,7 +4,7 @@ declare(strict_types=1);
 
 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 @@ class ImportController extends Controller
         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 @@ class ImportController extends Controller
         $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'];
         }
 
+        try {
             $entity = $this->imports->runImport($import, $parent);
-        if ($entity) {
-            $this->logActivity(ActivityType::IMPORT_RUN, $import);
-            return redirect($entity->getUrl());
+        } 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 @@ class ImportController extends Controller
         $import = $this->imports->findVisible($id);
         $this->imports->deleteImport($import);
 
-        $this->logActivity(ActivityType::IMPORT_DELETE, $import);
-
         return redirect('/import');
     }
 }

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\Models\ZipExportPage;
 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 @@ class ImportRepo
         $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);
     }
 }

app/Exports/ZipExports/ZipImportReferences.php

@@ -139,4 +139,21 @@ class ZipImportReferences
             ]);
         }
     }
+
+
+    /**
+     * @return Image[]
+     */
+    public function images(): array
+    {
+        return $this->images;
+    }
+
+    /**
+     * @return Attachment[]
+     */
+    public function attachments(): array
+    {
+        return $this->attachments;
+    }
 }

app/Exports/ZipExports/ZipImportRunner.php

@@ -47,14 +47,17 @@ class ZipImportRunner
      * 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,15 +68,14 @@ class ZipImportRunner
 
         // 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);
@@ -81,32 +83,46 @@ class ZipImportRunner
             $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');
-
-        // TODO - Delete import/zip after import?
-          // Do this in parent repo?
-
         return $entity;
     }
 
-    protected function cleanup()
+    /**
+     * 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);
+        }
+
+        foreach ($this->references->attachments() as $attachment) {
+            if (!$attachment->external) {
+                $this->attachmentService->deleteFileInStorage($attachment);
+            }
+        }
+
+        $this->cleanup();
+    }
+
+    protected function cleanup(): void
     {
         foreach ($this->tempFilesToCleanup as $file) {
             unlink($file);
         }
+
+        $this->tempFilesToCleanup = [];
     }
 
     protected function importBook(ZipExportBook $exportBook, ZipExportReader $reader): Book
@@ -256,9 +272,6 @@ class ZipImportRunner
     {
         $errors = [];
 
-        // TODO - Extract messages to language files
-        // TODO - Ensure these are shown to users on failure
-
         $chapters = [];
         $pages = [];
         $images = [];
@@ -266,7 +279,7 @@ class ZipImportRunner
 
         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 @@ class ZipImportRunner
         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 @@ class ZipImportRunner
         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');
             }
         }
 

app/Uploads/AttachmentService.php

@@ -151,7 +151,7 @@ class AttachmentService
      * 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);
     }

app/Uploads/ImageService.php

@@ -153,11 +153,19 @@ class ImageService
      */
     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.

lang/en/entities.php

@@ -52,6 +52,7 @@ return [
     '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 @@ return [
     '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',

lang/en/errors.php

@@ -109,6 +109,12 @@ return [
     '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',

resources/views/exports/import-show.blade.php

@@ -7,8 +7,19 @@
             <h1 class="list-heading">{{ trans('entities.import_continue') }}</h1>
             <p class="text-muted">{{ trans('entities.import_continue_desc') }}</p>
 
+            @if(session()->has('import_errors'))
                 <div class="mb-m">
-                <label class="setting-list-label mb-m">Import Details</label>
+                    <label class="setting-list-label mb-xs text-neg">@icon('warning') {{ trans('entities.import_errors') }}</label>
+                    <p class="mb-xs small">{{ trans('entities.import_errors_desc') }}</p>
+                    @foreach(session()->get('import_errors') ?? [] as $error)
+                        <p class="mb-none text-neg">{{ $error }}</p>
+                    @endforeach
+                    <hr class="mt-m">
+                </div>
+            @endif
+
+            <div class="mb-m">
+                <label class="setting-list-label mb-m">{{ trans('entities.import_details') }}</label>
                 <div class="flex-container-row justify-space-between wrap">
                     <div>
                         @include('exports.parts.import-item', ['type' => $import->type, 'model' => $data])
@@ -34,16 +45,19 @@
                 @if($import->type === 'page' || $import->type === 'chapter')
                     <hr>
                     <label class="setting-list-label">{{ trans('entities.import_location') }}</label>
-                    <p class="small mb-m">{{ trans('entities.import_location_desc') }}</p>
+                    <p class="small mb-s">{{ trans('entities.import_location_desc') }}</p>
+                    @if($errors->has('parent'))
+                        <div class="mb-s">
+                            @include('form.errors', ['name' => 'parent'])
+                        </div>
+                    @endif
                     @include('entities.selector', [
                         'name' => 'parent',
                         'entityTypes' => $import->type === 'page' ? 'chapter,book' : 'book',
                         'entityPermission' => "{$import->type}-create",
                         'selectorSize' => 'compact small',
                     ])
-                    @include('form.errors', ['name' => 'parent'])
                 @endif
-            </form>
 
                 <div class="text-right">
                     <a href="{{ url('/import') }}" class="button outline">{{ trans('common.cancel') }}</a>
@@ -58,8 +72,9 @@
                             <button type="submit" form="import-delete-form" class="text-link small text-item">{{ trans('common.confirm') }}</button>
                         </div>
                     </div>
-                <button type="submit" form="import-run-form" class="button">{{ trans('entities.import_run') }}</button>
+                    <button type="submit" class="button">{{ trans('entities.import_run') }}</button>
                 </div>
+            </form>
         </main>
     </div>
 

resources/views/exports/parts/import.blade.php

@@ -4,7 +4,7 @@
            class="text-{{ $import->type }}">@icon($import->type) {{ $import->name }}</a>
     </div>
     <div class="px-m py-s flex-container-row gap-m items-center">
-        <div class="bold opacity-80">{{ $import->getSizeString() }}</div>
-        <div class="bold opacity-80 text-muted" title="{{ $import->created_at->toISOString() }}">@icon('time'){{ $import->created_at->diffForHumans() }}</div>
+        <div class="bold opacity-80 text-muted">{{ $import->getSizeString() }}</div>
+        <div class="bold opacity-80 text-muted min-width-xs text-right" title="{{ $import->created_at->toISOString() }}">@icon('time'){{ $import->created_at->diffForHumans() }}</div>
     </div>
 </div>