From be7325550930ca5f30b1bd878b3a229bd72416c8 Mon Sep 17 00:00:00 2001 From: Brent Shaffer Date: Wed, 2 Oct 2019 12:47:40 -0700 Subject: [PATCH] Updates to FirestoreSessionHandler (#2360) * calls open after running any Firestore close operation * runs gc in separate transaction, sets max gcLimit to 500 * Fixes unit tests for FirestoreSessionHandler * removes unnecessary try/catch blocks * adds skipped test for session_gc bug * small improvement --- Firestore/src/FirestoreClient.php | 2 +- Firestore/src/FirestoreSessionHandler.php | 126 ++++++++++-------- .../System/FirestoreSessionHandlerTest.php | 83 ++++++++++-- .../Unit/FirestoreSessionHandlerTest.php | 23 +++- 4 files changed, 158 insertions(+), 76 deletions(-) diff --git a/Firestore/src/FirestoreClient.php b/Firestore/src/FirestoreClient.php index 7788f137b75f..6088ffa8080c 100644 --- a/Firestore/src/FirestoreClient.php +++ b/Firestore/src/FirestoreClient.php @@ -595,7 +595,7 @@ public function fieldPath(array $fieldNames) * Configuration Options. * * @type int $gcLimit The number of entities to delete in the garbage - * collection. Values larger than 1000 will be limited to 1000. + * collection. Values larger than 500 will be limited to 500. * **Defaults to** `0`, indicating garbage collection is disabled by * default. * @type string $collectionNameTemplate A sprintf compatible template diff --git a/Firestore/src/FirestoreSessionHandler.php b/Firestore/src/FirestoreSessionHandler.php index 609e53bd51b4..5d07f40920e2 100644 --- a/Firestore/src/FirestoreSessionHandler.php +++ b/Firestore/src/FirestoreSessionHandler.php @@ -51,7 +51,7 @@ * $collectionNameTemplate option. This isolates the session data from your * application data. It creates documents in the specified collection where the * ID is the session ID. By default, it does nothing on gc for reducing the - * cost. Pass a positive value up to 1000 for $gcLimit option to delete entities + * cost. Pass a positive value up to 500 for $gcLimit option to delete entities * in gc. * * The first example automatically writes the session data. It's handy, but @@ -147,6 +147,10 @@ class FirestoreSessionHandler implements SessionHandlerInterface * @var Transaction */ private $transaction; + /** + * @var string + */ + private $id; /** * Create a custom session handler backed by Cloud Firestore. @@ -159,7 +163,7 @@ class FirestoreSessionHandler implements SessionHandlerInterface * Configuration Options. * * @type int $gcLimit The number of entities to delete in the garbage - * collection. Values larger than 1000 will be limited to 1000. + * collection. Values larger than 500 will be limited to 500. * **Defaults to** `0`, indicating garbage collection is disabled by * default. * @type string $collectionNameTemplate A sprintf compatible template @@ -194,8 +198,8 @@ public function __construct( 'collectionNameTemplate' => '%1$s:%2$s', ]; - // Cut down gcLimit to 1000 - $this->options['gcLimit'] = min($this->options['gcLimit'], 1000); + // Cut down gcLimit to 500, as this is the Firestore batch limit. + $this->options['gcLimit'] = min($this->options['gcLimit'], 500); } /** @@ -235,10 +239,22 @@ public function open($savePath, $sessionName) } /** - * Just return true for this implementation. + * Close the transaction and commit any changes. */ public function close() { + if (is_null($this->transaction)) { + throw new \LogicException('open() must be called before close()'); + } + try { + $this->commitTransaction($this->transaction); + } catch (ServiceException $e) { + trigger_error( + sprintf('Session close failed: %s', $e->getMessage()), + E_USER_WARNING + ); + return false; + } return true; } @@ -250,6 +266,7 @@ public function close() */ public function read($id) { + $this->id = $id; try { $docRef = $this->getDocumentReference( $this->connection, @@ -283,26 +300,17 @@ public function read($id) */ public function write($id, $data) { - try { - $docRef = $this->getDocumentReference( - $this->connection, - $this->valueMapper, - $this->projectId, - $this->database, - $this->docId($id) - ); - $this->transaction->set($docRef, [ - 'data' => $data, - 't' => time() - ]); - $this->commitTransaction(); - } catch (ServiceException $e) { - trigger_error( - sprintf('Firestore upsert failed: %s', $e->getMessage()), - E_USER_WARNING - ); - return false; - } + $docRef = $this->getDocumentReference( + $this->connection, + $this->valueMapper, + $this->projectId, + $this->database, + $this->docId($id) + ); + $this->transaction->set($docRef, [ + 'data' => $data, + 't' => time() + ]); return true; } @@ -314,23 +322,14 @@ public function write($id, $data) */ public function destroy($id) { - try { - $docRef = $this->getDocumentReference( - $this->connection, - $this->valueMapper, - $this->projectId, - $this->database, - $this->docId($id) - ); - $this->transaction->delete($docRef); - $this->commitTransaction(); - } catch (ServiceException $e) { - trigger_error( - sprintf('Firestore delete failed: %s', $e->getMessage()), - E_USER_WARNING - ); - return false; - } + $docRef = $this->getDocumentReference( + $this->connection, + $this->valueMapper, + $this->projectId, + $this->database, + $this->docId($id) + ); + $this->transaction->delete($docRef); return true; } @@ -339,14 +338,27 @@ public function destroy($id) * * @param int $maxlifetime Remove all session data older than this number * in seconds. - * @return bool + * @return int|bool */ public function gc($maxlifetime) { if (0 === $this->options['gcLimit']) { return true; } + $deleteCount = 0; try { + $database = $this->databaseName($this->projectId, $this->database); + $beginTransaction = $this->connection->beginTransaction([ + 'database' => $database + ] + $this->options['begin']); + + $transaction = new Transaction( + $this->connection, + $this->valueMapper, + $database, + $beginTransaction['transaction'] + ); + $collectionRef = $this->getCollectionReference( $this->connection, $this->valueMapper, @@ -358,14 +370,17 @@ public function gc($maxlifetime) ->limit($this->options['gcLimit']) ->where('t', '<', time() - $maxlifetime) ->orderBy('t'); - $querySnapshot = $this->transaction->runQuery( + $querySnapshot = $transaction->runQuery( $query, $this->options['query'] ); foreach ($querySnapshot as $snapshot) { - $this->transaction->delete($snapshot->reference()); + if ($snapshot->id() != $this->id) { + $transaction->delete($snapshot->reference()); + $deleteCount++; + } } - $this->commitTransaction(); + $this->commitTransaction($transaction); } catch (ServiceException $e) { trigger_error( sprintf('Session gc failed: %s', $e->getMessage()), @@ -373,26 +388,27 @@ public function gc($maxlifetime) ); return false; } - return true; + return $deleteCount; } /** * Commit a transaction if changes exist, otherwise rollback the * transaction. Also rollback if an exception is thrown. * - * @throws \Exception + * @param Transaction $transaction The transaction to commit. + * @throws ServiceException */ - private function commitTransaction() + private function commitTransaction(Transaction $transaction) { try { - if (!$this->transaction->writer()->isEmpty()) { - $this->transaction->writer()->commit($this->options['commit']); + if (!$transaction->writer()->isEmpty()) { + $transaction->writer()->commit($this->options['commit']); } else { // trigger rollback if no writes exist. - $this->transaction->writer()->rollback($this->options['rollback']); + $transaction->writer()->rollback($this->options['rollback']); } } catch (ServiceException $e) { - $this->transaction->writer()->rollback($this->options['rollback']); + $transaction->writer()->rollback($this->options['rollback']); throw $e; } @@ -403,7 +419,7 @@ private function commitTransaction() * name according to the $collectionNameTemplate option. * ex: sessions:PHPSESSID * - * @param string $id Identifier used for the session + * @param string $id Identifier used for the session. * @return string */ private function collectionId() @@ -419,7 +435,7 @@ private function collectionId() * Format the Firebase document ID from the collection ID. * ex: sessions:PHPSESSID/abcdef * - * @param string $id Identifier used for the session + * @param string $id Identifier used for the session. * @return string */ private function docId($id) diff --git a/Firestore/tests/System/FirestoreSessionHandlerTest.php b/Firestore/tests/System/FirestoreSessionHandlerTest.php index 07b19f3e5aea..a255f6e25b44 100644 --- a/Firestore/tests/System/FirestoreSessionHandlerTest.php +++ b/Firestore/tests/System/FirestoreSessionHandlerTest.php @@ -27,7 +27,7 @@ */ class FirestoreSessionHandlerTest extends FirestoreTestCase { - public function testSessionHandler() + public function testSessionWrite() { $client = self::$client; @@ -48,9 +48,9 @@ public function testSessionHandler() sleep(1); $hasDocument = false; - $query = $client->collection($namespace . ':' . session_name()); - foreach ($query->documents() as $snapshot) { - self::$localDeletionQueue->add($snapshot->reference()); + $collection = $client->collection($namespace . ':' . session_name()); + self::$localDeletionQueue->add($collection); + foreach ($collection->documents() as $snapshot) { if (!$hasDocument) { $hasDocument = $snapshot['data'] === $storedValue; } @@ -59,25 +59,80 @@ public function testSessionHandler() $this->assertTrue($hasDocument); } - public function testSessionHandlerGarbageCollection() + public function testGarbageCollection() { $client = self::$client; + // Set session max lifetime to 0 to ensure deletion + ini_set('session.gc_maxlifetime', 0); + + // Disable probability-based GC + ini_set('session.gc_probability', 0); + $namespace = uniqid('sess-' . self::COLLECTION_NAME); - $sessionName = 'PHPSESSID'; - $collection = $client->collection($namespace . ':' . $sessionName); + $collection = $client->collection($namespace . ':' . session_name()); + self::$localDeletionQueue->add($collection); $collection->document('foo1')->set(['data' => 'foo1', 't' => time() - 1]); $collection->document('foo2')->set(['data' => 'foo2', 't' => time() - 1]); - - $this->assertCount(2, $collection->documents()); + $collection->document('foo3')->set(['data' => 'foo3', 't' => time() + 1]); + $this->assertCount(3, $collection->documents()); $handler = $client->sessionHandler([ - 'gcLimit' => 1000, - 'query' => ['maxRetries' => 0] + 'gcLimit' => 500, ]); - $handler->open($namespace, $sessionName); - $handler->gc(0); - $this->assertCount(0, $collection->documents()); + session_set_save_handler($handler, true); + session_save_path($namespace); + session_start(); + + session_gc(); + + $this->assertCount(1, $collection->documents()); + } + + public function testGarbageCollectionBeforeWrite() + { + $client = self::$client; + + // Set session max lifetime to 0 to ensure deletion + ini_set('session.gc_maxlifetime', 0); + + // Set GC divisor and probability to 1 so GC execution happens 100% + ini_set('session.gc_divisor', 1); + ini_set('session.gc_probability', 1); + + $namespace = uniqid('sess-' . self::COLLECTION_NAME); + $content = 'foo'; + $storedValue = 'name|' . serialize($content); + $collection = $client->collection($namespace . ':' . session_name()); + self::$localDeletionQueue->add($collection); + $collection->document('foo1')->set(['data' => 'foo1', 't' => time() - 1]); + $collection->document('foo2')->set(['data' => 'foo2', 't' => time() - 1]); + $this->assertCount(2, $collection->documents()); + + $handler = $client->sessionHandler(['gcLimit' => 500]); + session_set_save_handler($handler, true); + session_save_path($namespace); + session_start(); + + $sessionId = session_id(); + $_SESSION['name'] = $content; + + session_write_close(); + sleep(1); + + // assert old records have been removed and the new record has been added. + $this->assertCount(1, $collection->documents()); + } + + public function testSessionGcReturnValue() + { + // "session_gc" returns false for user-defined session handlers. + // The following test will always fail: + // ``` + // $this->assertGreaterThan(0, session_gc()); + // ``` + // This test is to remind us to implement a test the issue is fixed. + $this->markTestSkipped('session_gc returns false due to a core PHP bug'); } } diff --git a/Firestore/tests/Unit/FirestoreSessionHandlerTest.php b/Firestore/tests/Unit/FirestoreSessionHandlerTest.php index f39761631776..0b5b7c4ae33f 100644 --- a/Firestore/tests/Unit/FirestoreSessionHandlerTest.php +++ b/Firestore/tests/Unit/FirestoreSessionHandlerTest.php @@ -102,12 +102,18 @@ public function testReadNotAllowed() public function testClose() { + $this->connection->beginTransaction(['database' => $this->dbName()]) + ->shouldBeCalledTimes(1) + ->willReturn(['transaction' => 123]); + $this->connection->rollback(Argument::any()) + ->shouldBeCalledTimes(1); $firestoreSessionHandler = new FirestoreSessionHandler( $this->connection->reveal(), $this->valueMapper->reveal(), self::PROJECT, self::DATABASE ); + $firestoreSessionHandler->open(self::SESSION_SAVE_PATH, self::SESSION_NAME); $ret = $firestoreSessionHandler->close(); $this->assertTrue($ret); } @@ -234,6 +240,7 @@ public function testWrite() ); $firestoreSessionHandler->open(self::SESSION_SAVE_PATH, self::SESSION_NAME); $ret = $firestoreSessionHandler->write('sessionid', 'sessiondata'); + $firestoreSessionHandler->close(); $this->assertTrue($ret); } @@ -267,8 +274,10 @@ public function testWriteWithException() self::PROJECT, self::DATABASE ); + $firestoreSessionHandler->open(self::SESSION_SAVE_PATH, self::SESSION_NAME); $ret = $firestoreSessionHandler->write('sessionid', 'sessiondata'); + $firestoreSessionHandler->close(); $this->assertFalse($ret); } @@ -296,6 +305,7 @@ public function testDestroy() ); $firestoreSessionHandler->open(self::SESSION_SAVE_PATH, self::SESSION_NAME); $ret = $firestoreSessionHandler->destroy('sessionid'); + $firestoreSessionHandler->close(); $this->assertTrue($ret); } @@ -324,6 +334,7 @@ public function testDestroyWithException() ); $firestoreSessionHandler->open(self::SESSION_SAVE_PATH, self::SESSION_NAME); $ret = $firestoreSessionHandler->destroy('sessionid'); + $firestoreSessionHandler->close(); $this->assertFalse($ret); } @@ -366,7 +377,7 @@ public function testGc() $this->documents->next() ->shouldBeCalledTimes(1); $this->connection->beginTransaction(['database' => $this->dbName()]) - ->shouldBeCalledTimes(1) + ->shouldBeCalledTimes(2) ->willReturn(['transaction' => 123]); $this->connection->runQuery(Argument::any()) ->shouldBeCalledTimes(1) @@ -376,7 +387,7 @@ public function testGc() $phpunit->dbName() . '/documents', $options['parent'] ); - $phpunit->assertEquals(999, $options['structuredQuery']['limit']); + $phpunit->assertEquals(499, $options['structuredQuery']['limit']); $phpunit->assertEquals( self::SESSION_SAVE_PATH . ':' . self::SESSION_NAME, $options['structuredQuery']['from'][0]['collectionId'] @@ -404,13 +415,13 @@ public function testGc() $this->valueMapper->reveal(), self::PROJECT, self::DATABASE, - ['gcLimit' => 999, 'query' => ['maxRetries' => 0]] + ['gcLimit' => 499, 'query' => ['maxRetries' => 0]] ); $firestoreSessionHandler->open(self::SESSION_SAVE_PATH, self::SESSION_NAME); $ret = $firestoreSessionHandler->gc(100); - $this->assertTrue($ret); + $this->assertEquals(1, $ret); } /** @@ -419,7 +430,7 @@ public function testGc() public function testGcWithException() { $this->connection->beginTransaction(['database' => $this->dbName()]) - ->shouldBeCalledTimes(1) + ->shouldBeCalledTimes(2) ->willReturn(['transaction' => 123]); $this->connection->runQuery(Argument::any()) ->shouldBeCalledTimes(1) @@ -429,7 +440,7 @@ public function testGcWithException() $this->valueMapper->reveal(), self::PROJECT, self::DATABASE, - ['gcLimit' => 1000, 'query' => ['maxRetries' => 0]] + ['gcLimit' => 500, 'query' => ['maxRetries' => 0]] ); $firestoreSessionHandler->open(self::SESSION_SAVE_PATH, self::SESSION_NAME);