fix(files): Fix lock release handling when the file upload does not validate (#1118)

Author: klausi

src/GraphQL/Utility/FileUpload.php

@@ -223,46 +223,51 @@ public function saveFileUpload(UploadedFile $uploaded_file, array $settings): Fi
       return $response;
     }
 
-    // Begin building file entity.
-    /** @var \Drupal\file\FileInterface $file */
-    $file = $this->fileStorage->create([]);
-    $file->setOwnerId($this->currentUser->id());
-    $file->setFilename($prepared_filename);
-    $file->setMimeType($this->mimeTypeGuesser->guess($prepared_filename));
-    $file->setFileUri($file_uri);
-    // Set the size. This is done in File::preSave() but we validate the file
-    // before it is saved.
-    $file->setSize(@filesize($temp_file_path));
-
-    // Validate the file entity against entity-level validation and field-level
-    // validators.
-    if (!$this->validate($file, $validators, $response)) {
-      return $response;
-    }
-
-    // Move the file to the correct location after validation. Use
-    // FileSystemInterface::EXISTS_ERROR as the file location has already been
-    // determined above in FileSystem::getDestinationFilename().
     try {
-      $this->fileSystem->move($temp_file_path, $file_uri, FileSystemInterface::EXISTS_ERROR);
-    }
-    catch (FileException $e) {
-      $response->addViolation($this->t('Unknown error while uploading the file "@file".', [
-        '@file' => $uploaded_file->getClientOriginalName(),
-      ]));
-      $this->logger->error('Unable to move file from "@file" to "@destination".', [
-        '@file' => $uploaded_file->getRealPath(),
-        '@destination' => $file->getFileUri(),
-      ]);
-      return $response;
-    }
+      // Begin building file entity.
+      /** @var \Drupal\file\FileInterface $file */
+      $file = $this->fileStorage->create([]);
+      $file->setOwnerId($this->currentUser->id());
+      $file->setFilename($prepared_filename);
+      $file->setMimeType($this->mimeTypeGuesser->guess($prepared_filename));
+      $file->setFileUri($file_uri);
+      // Set the size. This is done in File::preSave() but we validate the file
+      // before it is saved.
+      $file->setSize(@filesize($temp_file_path));
+
+      // Validate the file entity against entity-level validation and
+      // field-level validators.
+      if (!$this->validate($file, $validators, $response)) {
+        return $response;
+      }
 
-    $file->save();
+      // Move the file to the correct location after validation. Use
+      // FileSystemInterface::EXISTS_ERROR as the file location has already been
+      // determined above in FileSystem::getDestinationFilename().
+      try {
+        $this->fileSystem->move($temp_file_path, $file_uri, FileSystemInterface::EXISTS_ERROR);
+      }
+      catch (FileException $e) {
+        $response->addViolation($this->t('Unknown error while uploading the file "@file".', [
+          '@file' => $uploaded_file->getClientOriginalName(),
+        ]));
+        $this->logger->error('Unable to move file from "@file" to "@destination".', [
+          '@file' => $uploaded_file->getRealPath(),
+          '@destination' => $file->getFileUri(),
+        ]);
+        return $response;
+      }
 
-    $this->lock->release($lock_id);
+      $file->save();
 
-    $response->setFileEntity($file);
-    return $response;
+      $response->setFileEntity($file);
+      return $response;
+    }
+    finally {
+      // This will always be executed before any return statement or exception
+      // in the try {} block.
+      $this->lock->release($lock_id);
+    }
   }
 
   /**

tests/src/Kernel/Framework/UploadFileServiceTest.php

@@ -2,7 +2,10 @@
 
 namespace Drupal\Tests\graphql\Kernel\Framework;
 
+use Drupal\Core\Lock\LockBackendInterface;
+use Drupal\graphql\GraphQL\Utility\FileUpload;
 use Drupal\Tests\graphql\Kernel\GraphQLTestBase;
+use Prophecy\Argument;
 use Symfony\Component\HttpFoundation\File\UploadedFile;
 
 /**
@@ -175,6 +178,41 @@ public function testExtensionValidation() {
     );
   }
 
+  /**
+   * Tests that the file lock is released on validation errors.
+   */
+  public function testLockReleased() {
+    // Mock the lock system to check that the lock is released.
+    $lock = $this->prophesize(LockBackendInterface::class);
+    $lock->acquire(Argument::any())->willReturn(TRUE);
+    // This is our only assertion - that the lock release method is called.
+    $lock->release(Argument::any())->shouldBeCalled();
+
+    $upload_service = new FileUpload(
+      \Drupal::service('entity_type.manager'),
+      \Drupal::service('current_user'),
+      \Drupal::service('file.mime_type.guesser'),
+      \Drupal::service('file_system'),
+      \Drupal::service('logger.channel.graphql'),
+      \Drupal::service('token'),
+      $lock->reveal(),
+      \Drupal::service('config.factory')
+    );
+
+    // Create a file with 4 bytes.
+    file_put_contents($this->file, 'test');
+
+    // Create a Symfony dummy uploaded file in test mode.
+    $uploadFile = $this->getUploadedFile(UPLOAD_ERR_OK, 4);
+
+    $upload_service->saveFileUpload($uploadFile, [
+      'uri_scheme' => 'public',
+      'file_directory' => 'test',
+      // Only allow 1 byte.
+      'max_filesize' => 1,
+    ]);
+  }
+
   /**
    * Helper method to prepare the UploadedFile depending on core version.
    *