Fix race conditions when adding an image, resolves #521 (#522)

src/Database/DatabaseInterface.php

@@ -34,10 +34,11 @@ interface DatabaseInterface {
      * @param string $user The user which the image belongs to
      * @param string $imageIdentifier Image identifier
      * @param Image $image The image to insert
+     * @param boolean $updateIfDuplicate Whether we should use an update statement if the image id exists, otherwise it'll result in an exception
      * @return boolean Returns true on success or false on failure
      * @throws DatabaseException
      */
-    function insertImage($user, $imageIdentifier, Image $image);
+    function insertImage($user, $imageIdentifier, Image $image, $updateIfDuplicate = true);
 
     /**
      * Delete an image from the database

src/Database/Doctrine.php

@@ -15,10 +15,12 @@
     Imbo\Resource\Images\Query,
     Imbo\Exception\DatabaseException,
     Imbo\Exception\InvalidArgumentException,
+    Imbo\Exception\DuplicateImageIdentifierException,
     Imbo\Exception,
     Doctrine\DBAL\DriverManager,
     Doctrine\DBAL\Connection,
     Doctrine\DBAL\DBALException,
+    Doctrine\DBAL\Exception\UniqueConstraintViolationException,
     PDO,
     DateTime,
     DateTimeZone;
@@ -84,7 +86,7 @@ public function __construct(array $params) {
     /**
      * {@inheritdoc}
      */
-    public function insertImage($user, $imageIdentifier, Image $image) {
+    public function insertImage($user, $imageIdentifier, Image $image, $updateIfDuplicate = true) {
         $now = time();
 
         if ($added = $image->getAddedDate()) {
@@ -95,27 +97,39 @@ public function insertImage($user, $imageIdentifier, Image $image) {
             $updated = $updated->getTimestamp();
         }
 
-        if ($id = $this->getImageId($user, $imageIdentifier)) {
-            return (boolean) $this->getConnection()->update($this->tableNames['imageinfo'], [
+        if ($updateIfDuplicate && $id = $this->getImageId($user, $imageIdentifier)) {
+            return (boolean)$this->getConnection()->update($this->tableNames['imageinfo'], [
                 'updated' => $now,
             ], [
                 'id' => $id
             ]);
         }
 
-        return (boolean) $this->getConnection()->insert($this->tableNames['imageinfo'], [
-            'size'             => $image->getFilesize(),
-            'user'             => $user,
-            'imageIdentifier'  => $imageIdentifier,
-            'extension'        => $image->getExtension(),
-            'mime'             => $image->getMimeType(),
-            'added'            => $added ?: $now,
-            'updated'          => $updated ?: $now,
-            'width'            => $image->getWidth(),
-            'height'           => $image->getHeight(),
-            'checksum'         => $image->getChecksum(),
-            'originalChecksum' => $image->getOriginalChecksum(),
-        ]);
+        try {
+            $result = $this->getConnection()->insert($this->tableNames['imageinfo'], [
+                'size' => $image->getFilesize(),
+                'user' => $user,
+                'imageIdentifier' => $imageIdentifier,
+                'extension' => $image->getExtension(),
+                'mime' => $image->getMimeType(),
+                'added' => $added ?: $now,
+                'updated' => $updated ?: $now,
+                'width' => $image->getWidth(),
+                'height' => $image->getHeight(),
+                'checksum' => $image->getChecksum(),
+                'originalChecksum' => $image->getOriginalChecksum(),
+            ]);
+        } catch (UniqueConstraintViolationException $e) {
+            throw new DuplicateImageIdentifierException(
+                'Duplicate image identifier when attempting to insert image into DB.',
+                503,
+                $e
+            );
+        } catch (DBALException $e) {
+            throw new DatabaseException('Unable to save image data', 500, $e);
+        }
+
+        return (boolean) $result;
     }
 
     /**

src/Database/Mongo.php

@@ -14,6 +14,7 @@
     Imbo\Model\Images,
     Imbo\Resource\Images\Query,
     Imbo\Exception\DatabaseException,
+    Imbo\Exception\DuplicateImageIdentifierException,
     Imbo\Helpers\BSONToArray,
     MongoDB\Client as MongoClient,
     MongoDB\Driver\Manager,
@@ -114,7 +115,7 @@ public function __construct(array $params = null, MongoClient $client = null, Mo
     /**
      * {@inheritdoc}
      */
-    public function insertImage($user, $imageIdentifier, Image $image) {
+    public function insertImage($user, $imageIdentifier, Image $image, $updateIfDuplicate = true) {
         $now = time();
 
         if ($added = $image->getAddedDate()) {
@@ -125,7 +126,7 @@ public function insertImage($user, $imageIdentifier, Image $image) {
             $updated = $updated->getTimestamp();
         }
 
-        if ($this->imageExists($user, $imageIdentifier)) {
+        if ($updateIfDuplicate && $this->imageExists($user, $imageIdentifier)) {
             try {
                 $this->getImageCollection()->updateOne(
                     ['user' => $user, 'imageIdentifier' => $imageIdentifier],
@@ -156,6 +157,15 @@ public function insertImage($user, $imageIdentifier, Image $image) {
         try {
             $this->getImageCollection()->insertOne($data);
         } catch (MongoException $e) {
+            foreach ($e->getWriteResult()->getWriteErrors() as $error) {
+                if ($error->getCode() === 11000) {
+                    throw new DuplicateImageIdentifierException(
+                        'Duplicate image identifier when attempting to insert image into DB.',
+                        503
+                    );
+                }
+            }
+
             throw new DatabaseException('Unable to save image data', 500, $e);
         }
 

src/Database/MongoDB.php

@@ -14,9 +14,11 @@
     Imbo\Model\Images,
     Imbo\Resource\Images\Query,
     Imbo\Exception\DatabaseException,
+    Imbo\Exception\DuplicateImageIdentifierException,
     MongoClient,
     MongoCollection,
     MongoException,
+    MongoDuplicateKeyException,
     DateTime,
     DateTimeZone;
 
@@ -97,7 +99,7 @@ public function __construct(array $params = null, MongoClient $client = null, Mo
     /**
      * {@inheritdoc}
      */
-    public function insertImage($user, $imageIdentifier, Image $image) {
+    public function insertImage($user, $imageIdentifier, Image $image, $updateIfDuplicate = true) {
         $now = time();
 
         if ($added = $image->getAddedDate()) {
@@ -108,7 +110,7 @@ public function insertImage($user, $imageIdentifier, Image $image) {
             $updated = $updated->getTimestamp();
         }
 
-        if ($this->imageExists($user, $imageIdentifier)) {
+        if ($updateIfDuplicate && $this->imageExists($user, $imageIdentifier)) {
             try {
                 $this->getImageCollection()->update(
                     ['user' => $user, 'imageIdentifier' => $imageIdentifier],
@@ -123,22 +125,24 @@ public function insertImage($user, $imageIdentifier, Image $image) {
         }
 
         $data = [
-            'size'             => $image->getFilesize(),
-            'user'             => $user,
-            'imageIdentifier'  => $imageIdentifier,
-            'extension'        => $image->getExtension(),
-            'mime'             => $image->getMimeType(),
-            'metadata'         => [],
-            'added'            => $added ?: $now,
-            'updated'          => $updated ?: $now,
-            'width'            => $image->getWidth(),
-            'height'           => $image->getHeight(),
-            'checksum'         => $image->getChecksum(),
+            'size' => $image->getFilesize(),
+            'user' => $user,
+            'imageIdentifier' => $imageIdentifier,
+            'extension' => $image->getExtension(),
+            'mime' => $image->getMimeType(),
+            'metadata' => [],
+            'added' => $added ?: $now,
+            'updated' => $updated ?: $now,
+            'width' => $image->getWidth(),
+            'height' => $image->getHeight(),
+            'checksum' => $image->getChecksum(),
             'originalChecksum' => $image->getOriginalChecksum(),
         ];
 
         try {
             $this->getImageCollection()->insert($data);
+        } catch (MongoDuplicateKeyException $e) {
+            throw new DuplicateImageIdentifierException('Duplicate image identifier when attempting to insert image into DB.', 503);
         } catch (MongoException $e) {
             throw new DatabaseException('Unable to save image data', 500, $e);
         }

src/EventListener/DatabaseOperations.php

@@ -74,14 +74,19 @@ public function getImagesQuery() {
      * Insert an image
      *
      * @param EventInterface $event An event instance
+     * @param array $params Optional arguments to the insert method
+     *                      - `updateIfDuplicate` controls whether an update will happen if the imageid already exists
      */
-    public function insertImage(EventInterface $event) {
+    public function insertImage(EventInterface $event, $params = []) {
         $request = $event->getRequest();
 
+        $updateIfDuplicate = !isset($params['updateIfDuplicate']) || !empty($params['updateIfDuplicate']);
+
         $event->getDatabase()->insertImage(
             $request->getUser(),
             $request->getImage()->getImageIdentifier(),
-            $request->getImage()
+            $request->getImage(),
+            $updateIfDuplicate
         );
     }
 

src/Exception/DuplicateImageIdentifierException.php

@@ -0,0 +1,19 @@
+<?php
+/**
+ * This file is part of the Imbo package
+ *
+ * (c) Christer Edvartsen <cogo@starzinger.net>
+ *
+ * For the full copyright and license information, please view the LICENSE file that was
+ * distributed with this source code.
+ */
+
+namespace Imbo\Exception;
+
+/**
+ * Duplicate Image Identifier exception - thrown if the image identifier already exists in the underlying database.
+ *
+ * @author Mats Lindh <mats@lindh.no>
+ * @package Exceptions
+ */
+class DuplicateImageIdentifierException extends RuntimeException {}

src/Image/ImagePreparation.php

@@ -90,56 +90,6 @@ public function prepareImage(EventInterface $event) {
               ->setHeight($size['height'])
               ->setOriginalChecksum(md5($imageBlob));
 
-        $imageIdentifier = $this->generateImageIdentifier($event, $image);
-        $image->setImageIdentifier($imageIdentifier);
-
         $request->setImage($image);
     }
-
-    /**
-     * Using the configured image identifier generator, attempt to generate a unique image
-     * identifier for the given image until we either have found a unique ID or we hit the maximum
-     * allowed attempts.
-     *
-     * @param EventInterface $event The current event
-     * @param Image $image The event to generate the image identifier for
-     * @return string
-     * @throws ImageException
-     */
-    private function generateImageIdentifier(EventInterface $event, Image $image) {
-        $database = $event->getDatabase();
-        $config = $event->getConfig();
-        $user = $event->getRequest()->getUser();
-        $imageIdentifierGenerator = $config['imageIdentifierGenerator'];
-
-        if (is_callable($imageIdentifierGenerator) &&
-            !($imageIdentifierGenerator instanceof GeneratorInterface)) {
-            $imageIdentifierGenerator = $imageIdentifierGenerator();
-        }
-
-        if ($imageIdentifierGenerator->isDeterministic()) {
-            return $imageIdentifierGenerator->generate($image);
-        }
-
-        // Continue generating image identifiers until we get one that does not already exist
-        $maxAttempts = 100;
-        $attempts = 0;
-        do {
-            $imageIdentifier = $imageIdentifierGenerator->generate($image);
-            $attempts++;
-        } while ($attempts < $maxAttempts && $database->imageExists($user, $imageIdentifier));
-
-        // Did we reach our max attempts limit?
-        if ($attempts === $maxAttempts) {
-            $e = new ImageException('Failed to generate unique image identifier', 503);
-            $e->setImboErrorCode(Exception::IMAGE_IDENTIFIER_GENERATION_FAILED);
-
-            // Tell the client it's OK to retry later
-            $event->getResponse()->headers->set('Retry-After', 1);
-
-            throw $e;
-        }
-
-        return $imageIdentifier;
-    }
 }

src/Resource/Images.php

@@ -11,6 +11,8 @@
 namespace Imbo\Resource;
 
 use Imbo\EventManager\EventInterface,
+    Imbo\Exception\DuplicateImageIdentifierException,
+    Imbo\Exception\ImageException,
     Imbo\Model;
 
 /**
@@ -60,16 +62,53 @@ public function getImages(EventInterface $event) {
     /**
      * Handle POST requests
      *
-     * @param EventInterface
+     * @param EventInterface $event
      */
     public function addImage(EventInterface $event) {
-        $event->getManager()->trigger('db.image.insert');
-        $event->getManager()->trigger('storage.image.insert');
-
         $request = $event->getRequest();
         $response = $event->getResponse();
         $image = $request->getImage();
 
+        // attempt to store the image in the underlying database
+        $maxAttempts = 100;
+        $config = $event->getConfig();
+
+        // retrieve and instantiate if necessary the image identifier generator
+        $imageIdentifierGenerator = $config['imageIdentifierGenerator'];
+
+        if (is_callable($imageIdentifierGenerator) &&
+            !($imageIdentifierGenerator instanceof GeneratorInterface)) {
+            $imageIdentifierGenerator = $imageIdentifierGenerator();
+        }
+
+        for ($attempt = 0; $attempt < $maxAttempts; $attempt++) {
+            try {
+                $image->setImageIdentifier($imageIdentifierGenerator->generate($image));
+                $event->getManager()->trigger('db.image.insert', ['updateIfDuplicate' => $imageIdentifierGenerator->isDeterministic()]);
+                break;
+            } catch (DuplicateImageIdentifierException $exception) {
+                // the image identifier already exists - or was created before we inserted
+                // so we retry the event to get the last submitted image
+                if ($imageIdentifierGenerator->isDeterministic()) {
+                    $event->getManager()->trigger('db.image.insert', ['updateIfDuplicate' => $imageIdentifierGenerator->isDeterministic()]);
+                }
+
+                continue;
+            }
+        }
+
+        // Image Identifier generation failed - throw exception to get us out of this state
+        if ($attempt === $maxAttempts) {
+            $e = new ImageException('Failed to generate unique image identifier', 503);
+            $e->setImboErrorCode(ImageException::IMAGE_IDENTIFIER_GENERATION_FAILED);
+
+            // Tell the client it's OK to retry later
+            $event->getResponse()->headers->set('Retry-After', 1);
+            throw $e;
+        }
+
+        $event->getManager()->trigger('storage.image.insert');
+
         $model = new Model\ArrayModel();
         $model->setData([
             'imageIdentifier' => $image->getImageIdentifier(),
@@ -80,5 +119,4 @@ public function addImage(EventInterface $event) {
 
         $response->setModel($model);
     }
-
 }