Issue #2008806 by damiankloip, fubhy: Add getIfOwner(), setIfOwner() and...

Issue #2008806 by damiankloip, fubhy: Add getIfOwner(), setIfOwner() and deleteIfOwner() methods to TempStore.

core/modules/user/lib/Drupal/user/TempStore.php

@@ -7,6 +7,7 @@
 
 namespace Drupal\user;
 
+use Drupal\Component\Utility\String;
 use Drupal\Core\KeyValueStore\KeyValueStoreExpirableInterface;
 use Drupal\Core\Lock\LockBackendInterface;
 
@@ -34,9 +35,6 @@
  * editing certain data, or even to restrict other users from editing it at
  * the same time. It is the responsibility of the implementation to decide
  * when and whether one owner can use or update another owner's data.
- *
- * @todo We could add getIfOwner() or setIfOwner() methods to make this more
- *   explicit.
  */
 class TempStore {
 
@@ -85,7 +83,7 @@ class TempStore {
    * @param mixed $owner
    *   The owner key to store along with the data (e.g. a user or session ID).
    */
-  function __construct(KeyValueStoreExpirableInterface $storage, LockBackendInterface $lockBackend, $owner) {
+  public function __construct(KeyValueStoreExpirableInterface $storage, LockBackendInterface $lockBackend, $owner) {
     $this->storage = $storage;
     $this->lockBackend = $lockBackend;
     $this->owner = $owner;
@@ -100,12 +98,29 @@ function __construct(KeyValueStoreExpirableInterface $storage, LockBackendInterf
    * @return mixed
    *   The data associated with the key, or NULL if the key does not exist.
    */
-  function get($key) {
+  public function get($key) {
     if ($object = $this->storage->get($key)) {
       return $object->data;
     }
   }
 
+  /**
+   * Retrieves a value from this TempStore for a given key.
+   *
+   * Only returns the value if the value is owned by $this->owner.
+   *
+   * @param string $key
+   *   The key of the data to retrieve.
+   *
+   * @return mixed
+   *   The data associated with the key, or NULL if the key does not exist.
+   */
+  public function getIfOwner($key) {
+    if (($object = $this->storage->get($key)) && ($object->owner == $this->owner)) {
+      return $object->data;
+    }
+  }
+
   /**
    * Stores a particular key/value pair only if the key doesn't already exist.
    *
@@ -117,7 +132,7 @@ function get($key) {
    * @return bool
    *   TRUE if the data was set, or FALSE if it already existed.
    */
-  function setIfNotExists($key, $value) {
+  public function setIfNotExists($key, $value) {
     $value = (object) array(
       'owner' => $this->owner,
       'data' => $value,
@@ -126,6 +141,34 @@ function setIfNotExists($key, $value) {
     return $this->storage->setWithExpireIfNotExists($key, $value, $this->expire);
   }
 
+  /**
+   * Stores a particular key/value pair in this TempStore.
+   *
+   * Only stores the given key/value pair if it does not exist yet or is owned
+   * by $this->owner.
+   *
+   * @param string $key
+   *   The key of the data to store.
+   * @param mixed $value
+   *   The data to store.
+   *
+   * @return bool
+   *   TRUE if the data was set, or FALSE if it already exists and is not owned
+   * by $this->user.
+   */
+  public function setIfOwner($key, $value) {
+    if ($this->setIfNotExists($key, $value)) {
+      return TRUE;
+    }
+
+    if (($object = $this->storage->get($key)) && ($object->owner == $this->owner)) {
+      $this->set($key, $value);
+      return TRUE;
+    }
+
+    return FALSE;
+  }
+
   /**
    * Stores a particular key/value pair in this TempStore.
    *
@@ -134,13 +177,13 @@ function setIfNotExists($key, $value) {
    * @param mixed $value
    *   The data to store.
    */
-  function set($key, $value) {
+  public function set($key, $value) {
     if (!$this->lockBackend->acquire($key)) {
       $this->lockBackend->wait($key);
       if (!$this->lockBackend->acquire($key)) {
-        throw new TempStoreException(format_string("Couldn't acquire lock to update item %key in %collection temporary storage.", array(
+        throw new TempStoreException(String::format("Couldn't acquire lock to update item %key in %collection temporary storage.", array(
           '%key' => $key,
-          '%collection' => $this->storage->collection,
+          '%collection' => $this->storage->getCollectionName(),
         )));
       }
     }
@@ -164,7 +207,7 @@ function set($key, $value) {
    *   An object with the owner and updated time if the key has a value, or
    *   NULL otherwise.
    */
-  function getMetadata($key) {
+  public function getMetadata($key) {
     // Fetch the key/value pair and its metadata.
     $object = $this->storage->get($key);
     if ($object) {
@@ -180,13 +223,13 @@ function getMetadata($key) {
    * @param string $key
    *   The key of the data to delete.
    */
-  function delete($key) {
+  public function delete($key) {
     if (!$this->lockBackend->acquire($key)) {
       $this->lockBackend->wait($key);
       if (!$this->lockBackend->acquire($key)) {
-        throw new TempStoreException(format_string("Couldn't acquire lock to delete item %key from %collection temporary storage.", array(
+        throw new TempStoreException(String::format("Couldn't acquire lock to delete item %key from %collection temporary storage.", array(
           '%key' => $key,
-          '%collection' => $this->storage->collection,
+          '%collection' => $this->storage->getCollectionName(),
         )));
       }
     }
@@ -194,4 +237,28 @@ function delete($key) {
     $this->lockBackend->release($key);
   }
 
+  /**
+   * Deletes data from the store for a given key and releases the lock on it.
+   *
+   * Only delete the given key if it is owned by $this->owner.
+   *
+   * @param string $key
+   *   The key of the data to delete.
+   *
+   * @return bool
+   *   TRUE if the object was deleted or does not exist, FALSE if it exists but
+   *   is not owned by $this->owner.
+   */
+  public function deleteIfOwner($key) {
+    if (!$object = $this->storage->get($key)) {
+      return TRUE;
+    }
+    elseif ($object->owner == $this->owner) {
+      $this->delete($key);
+      return TRUE;
+    }
+
+    return FALSE;
+  }
+
 }

core/modules/user/lib/Drupal/user/Tests/TempStoreDatabaseTest.php

@@ -123,6 +123,12 @@ public function testUserTempStore() {
     $this->assertIdenticalObject($this->objects[2], $stores[0]->get($key));
     // The object is the same when another user loads it.
     $this->assertIdenticalObject($this->objects[2], $stores[1]->get($key));
+
+    // This user should be allowed to get, update, delete.
+    $this->assertTrue($stores[0]->getIfOwner($key) instanceof \stdClass);
+    $this->assertTrue($stores[0]->setIfOwner($key, $this->objects[1]));
+    $this->assertTrue($stores[0]->deleteIfOwner($key));
+
     // Another user can update the object and become the owner.
     $stores[1]->set($key, $this->objects[3]);
     $this->assertIdenticalObject($this->objects[3], $stores[0]->get($key));
@@ -134,6 +140,11 @@ public function testUserTempStore() {
     $metadata = $stores[0]->getMetadata($key);
     $this->assertEqual($users[1], $metadata->owner);
 
+    // The first user should no longer be allowed to get, update, delete.
+    $this->assertNull($stores[0]->getIfOwner($key));
+    $this->assertFalse($stores[0]->setIfOwner($key, $this->objects[1]));
+    $this->assertFalse($stores[0]->deleteIfOwner($key));
+
     // Now manually expire the item (this is not exposed by the API) and then
     // assert it is no longer accessible.
     db_update('key_value_expire')

core/modules/user/tests/Drupal/user/Tests/TempStoreTest.php

+<?php
+
+/**
+ * @file
+ * Contains \Drupal\user\Tests\TempStoreTest.
+ */
+
+namespace Drupal\user\Tests;
+
+use Drupal\Tests\UnitTestCase;
+use Drupal\user\TempStore;
+
+/**
+ * Tests the TempStore namespace.
+ *
+ * @group Drupal
+ * @group User
+ *
+ * @coversDefaultClass \Drupal\user\TempStore
+ */
+class TempStoreTest extends UnitTestCase {
+
+  /**
+   * The mock key value expirable backend.
+   *
+   * @var \Drupal\Core\KeyValueStore\KeyValueStoreExpirableInterface|\PHPUnit_Framework_MockObject_MockObject
+   */
+  protected $keyValue;
+
+  /**
+   * The mock lock backend.
+   *
+   * @var \Drupal\Core\Lock\LockBackendInterface|\PHPUnit_Framework_MockObject_MockObject
+   */
+  protected $lock;
+
+  /**
+   * The user temp store.
+   *
+   * @var \Drupal\user\TempStore
+   */
+  protected $tempStore;
+
+  /**
+   * The owner used in this test.
+   *
+   * @var int
+   */
+  protected $owner = 1;
+
+  /**
+   * A tempstore object belonging to the owner.
+   *
+   * @var \stdClass
+   */
+  protected $ownObject;
+
+  /**
+   * A tempstore object not belonging to the owner.
+   *
+   * @var \stdClass
+   */
+  protected $otherObject;
+
+  /**
+   * {@inheritdoc}
+   */
+  public static function getInfo() {
+    return array(
+      'name' => 'TempStore',
+      'description' => 'Unit tests the Drupal\user\TempStore class.',
+      'group' => 'User',
+    );
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  protected function setUp() {
+    parent::setUp();
+
+    $this->keyValue = $this->getMock('Drupal\Core\KeyValueStore\KeyValueStoreExpirableInterface');
+    $this->lock = $this->getMock('Drupal\Core\Lock\LockBackendInterface');
+
+    $this->tempStore = new TempStore($this->keyValue, $this->lock, $this->owner);
+
+    $this->ownObject = (object) array(
+      'data' => 'test_data',
+      'owner' => $this->owner,
+      'updated' => REQUEST_TIME,
+    );
+
+    // Clone the object but change the owner.
+    $this->otherObject = clone $this->ownObject;
+    $this->otherObject->owner = 2;
+  }
+
+  /**
+   * @covers ::get()
+   */
+  public function testGet() {
+    $this->keyValue->expects($this->at(0))
+      ->method('get')
+      ->with('test_2')
+      ->will($this->returnValue(FALSE));
+    $this->keyValue->expects($this->at(1))
+      ->method('get')
+      ->with('test')
+      ->will($this->returnValue($this->ownObject));
+
+    $this->assertNull($this->tempStore->get('test_2'));
+    $this->assertSame($this->ownObject->data, $this->tempStore->get('test'));
+  }
+
+  /**
+   * Tests the getIfOwner() method.
+   *
+   * @covers ::getIfOwner()
+   */
+  public function testGetIfOwner() {
+    $this->keyValue->expects($this->at(0))
+      ->method('get')
+      ->with('test_2')
+      ->will($this->returnValue(FALSE));
+    $this->keyValue->expects($this->at(1))
+      ->method('get')
+      ->with('test')
+      ->will($this->returnValue($this->ownObject));
+    $this->keyValue->expects($this->at(2))
+      ->method('get')
+      ->with('test')
+      ->will($this->returnValue($this->otherObject));
+
+    $this->assertNull($this->tempStore->getIfOwner('test_2'));
+    $this->assertSame($this->ownObject->data, $this->tempStore->getIfOwner('test'));
+    $this->assertNull($this->tempStore->getIfOwner('test'));
+  }
+
+  /**
+   * Tests the set() method with no lock available.
+   *
+   * @covers ::set()
+   * @expectedException \Drupal\user\TempStoreException
+   */
+  public function testSetWithNoLockAvailable() {
+    $this->lock->expects($this->at(0))
+      ->method('acquire')
+      ->with('test')
+      ->will($this->returnValue(FALSE));
+    $this->lock->expects($this->at(1))
+      ->method('wait')
+      ->with('test');
+    $this->lock->expects($this->at(2))
+      ->method('acquire')
+      ->with('test')
+      ->will($this->returnValue(FALSE));
+
+    $this->keyValue->expects($this->once())
+      ->method('getCollectionName');
+
+    $this->tempStore->set('test', 'value');
+  }
+
+  /**
+   * Tests a successful set() call.
+   *
+   * @covers ::set()
+   */
+  public function testSet() {
+    $this->lock->expects($this->once())
+      ->method('acquire')
+      ->with('test')
+      ->will($this->returnValue(TRUE));
+    $this->lock->expects($this->never())
+      ->method('wait');
+    $this->lock->expects($this->once())
+      ->method('release')
+      ->with('test');
+
+    $this->keyValue->expects($this->once())
+      ->method('setWithExpire')
+      ->with('test', $this->ownObject, 604800);
+
+    $this->tempStore->set('test', 'test_data');
+  }
+
+  /**
+   * Tests the setIfNotExists() methods.
+   *
+   * @covers ::setIfNotExists()
+   */
+  public function testSetIfNotExists() {
+    $this->keyValue->expects($this->once())
+      ->method('setWithExpireIfNotExists')
+      ->with('test', $this->ownObject, 604800)
+      ->will($this->returnValue(TRUE));
+
+    $this->assertTrue($this->tempStore->setIfNotExists('test', 'test_data'));
+  }
+
+  /**
+   * Tests the setIfOwner() method when no key exists.
+   *
+   * @covers ::setIfOwner()
+   */
+  public function testSetIfOwnerWhenNotExists() {
+    $this->keyValue->expects($this->once())
+      ->method('setWithExpireIfNotExists')
+      ->will($this->returnValue(TRUE));
+
+    $this->assertTrue($this->tempStore->setIfOwner('test', 'test_data'));
+  }
+
+  /**
+   * Tests the setIfOwner() method when a key already exists but no object.
+   *
+   * @covers ::setIfOwner()
+   */
+  public function testSetIfOwnerNoObject() {
+    $this->keyValue->expects($this->once())
+      ->method('setWithExpireIfNotExists')
+      ->will($this->returnValue(FALSE));
+
+    $this->keyValue->expects($this->once())
+      ->method('get')
+      ->with('test')
+      ->will($this->returnValue(FALSE));
+
+    $this->assertFalse($this->tempStore->setIfOwner('test', 'test_data'));
+  }
+
+  /**
+   * Tests the setIfOwner() method with matching and non matching owners.
+   *
+   * @covers ::setIfOwner()
+   */
+  public function testSetIfOwner() {
+    $this->lock->expects($this->once())
+      ->method('acquire')
+      ->with('test')
+      ->will($this->returnValue(TRUE));
+
+    $this->keyValue->expects($this->exactly(2))
+      ->method('setWithExpireIfNotExists')
+      ->will($this->returnValue(FALSE));
+
+    $this->keyValue->expects($this->exactly(2))
+      ->method('get')
+      ->with('test')
+      ->will($this->onConsecutiveCalls($this->ownObject, $this->otherObject));
+
+    $this->assertTrue($this->tempStore->setIfOwner('test', 'test_data'));
+    $this->assertFalse($this->tempStore->setIfOwner('test', 'test_data'));
+  }
+
+  /**
+   * Tests the getMetadata() method.
+   *
+   * @covers ::getMetadata()
+   */
+  public function testGetMetadata() {
+    $this->keyValue->expects($this->at(0))
+      ->method('get')
+      ->with('test')
+      ->will($this->returnValue($this->ownObject));
+
+    $this->keyValue->expects($this->at(1))
+      ->method('get')
+      ->with('test')
+      ->will($this->returnValue(FALSE));
+
+    $metadata = $this->tempStore->getMetadata('test');
+    $this->assertObjectHasAttribute('owner', $metadata);
+    // Data should get removed.
+    $this->assertObjectNotHasAttribute('data', $metadata);
+
+    $this->assertNull($this->tempStore->getMetadata('test'));
+  }
+
+  /**
+   * Tests the delete() method.
+   *
+   * @covers ::delete()
+   */
+  public function testDelete() {
+    $this->lock->expects($this->once())
+      ->method('acquire')
+      ->with('test')
+      ->will($this->returnValue(TRUE));
+    $this->lock->expects($this->never())
+      ->method('wait');
+    $this->lock->expects($this->once())
+      ->method('release')
+      ->with('test');
+
+    $this->keyValue->expects($this->once())
+      ->method('delete')
+      ->with('test');
+
+    $this->tempStore->delete('test');
+  }
+
+  /**
+   * Tests the delete() method with no lock available.
+   *
+   * @covers ::delete()
+   * @expectedException \Drupal\user\TempStoreException
+   */
+  public function testDeleteWithNoLockAvailable() {
+    $this->lock->expects($this->at(0))
+      ->method('acquire')
+      ->with('test')
+      ->will($this->returnValue(FALSE));
+    $this->lock->expects($this->at(1))
+      ->method('wait')
+      ->with('test');
+    $this->lock->expects($this->at(2))
+      ->method('acquire')
+      ->with('test')
+      ->will($this->returnValue(FALSE));
+
+    $this->keyValue->expects($this->once())
+      ->method('getCollectionName');
+
+    $this->tempStore->delete('test');
+  }
+
+  /**
+   * Tests the deleteIfOwner() method.
+   *
+   * @covers ::deleteIfOwner()
+   */
+  public function testDeleteIfOwner() {
+    $this->lock->expects($this->once())
+      ->method('acquire')
+      ->with('test_2')
+      ->will($this->returnValue(TRUE));
+
+    $this->keyValue->expects($this->at(0))
+      ->method('get')
+      ->with('test_1')
+      ->will($this->returnValue(FALSE));
+    $this->keyValue->expects($this->at(1))
+      ->method('get')
+      ->with('test_2')
+      ->will($this->returnValue($this->ownObject));
+    $this->keyValue->expects($this->at(2))
+      ->method('delete')
+      ->with('test_2');
+    $this->keyValue->expects($this->at(3))
+      ->method('get')
+      ->with('test_3')
+      ->will($this->returnValue($this->otherObject));
+
+    $this->assertTrue($this->tempStore->deleteIfOwner('test_1'));
+    $this->assertTrue($this->tempStore->deleteIfOwner('test_2'));
+    $this->assertFalse($this->tempStore->deleteIfOwner('test_3'));
+  }
+
+}
+