From bcc532e8c142be6e7162a229702895c242274930 Mon Sep 17 00:00:00 2001 From: Jeremiah VALERIE Date: Fri, 11 Nov 2016 10:59:52 +0100 Subject: [PATCH 1/4] fix(README) --- README.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index 2ec2563..1de976a 100644 --- a/README.md +++ b/README.md @@ -80,7 +80,7 @@ your application: ```php $promise1A = $userLoader->load(1); -$promise1B = userLoader->load(1); +$promise1B = $userLoader->load(1); var_dump($promise1A === $promise1B); // bool(true) ``` @@ -94,7 +94,7 @@ Here's a simple example using SQL UPDATE to illustrate. ```php $sql = 'UPDATE users WHERE id=4 SET username="zuck"'; if (true === $conn->query($sql)) { - $userLoader->clear(4); + $userLoader->clear(4); } ``` @@ -164,7 +164,7 @@ list($a, $b) = DataLoader::await($myLoader->loadMany(['a', 'b']); This is equivalent to the more verbose: ```js -list($a, $b) = await DataLoader::await(\React\Promise\all([ +list($a, $b) = DataLoader::await(\React\Promise\all([ $myLoader->load('a'), $myLoader->load('b') ]); From d0b351d5b7ff55d943656362e2eb5cf04468db54 Mon Sep 17 00:00:00 2001 From: Jeremiah VALERIE Date: Fri, 11 Nov 2016 17:27:11 +0100 Subject: [PATCH 2/4] Replace BatchLoadFn class by callable --- README.md | 9 ++++---- src/BatchLoadFn.php | 43 --------------------------------------- src/DataLoader.php | 2 +- tests/BatchLoadFnTest.php | 28 ------------------------- tests/DataLoadTest.php | 24 ++++++++++------------ 5 files changed, 16 insertions(+), 90 deletions(-) delete mode 100644 src/BatchLoadFn.php delete mode 100644 tests/BatchLoadFnTest.php diff --git a/README.md b/README.md index 1de976a..07855f4 100644 --- a/README.md +++ b/README.md @@ -33,10 +33,10 @@ use Overblog\DataLoader\DataLoader; $myBatchGetUsers = function ($keys) { /* ... */ }; -$userLoader = new DataLoader(new BatchLoadFn($myBatchGetUsers)); +$userLoader = new DataLoader($myBatchGetUsers); ``` -A batch loading instance accepts a callable callback that accepts an Array of keys, and returns a Promise which +A batch loading callable / callback accepts an Array of keys, and returns a Promise which resolves to an Array of values. Then load individual values from the loader. DataLoaderPHP will coalesce all @@ -123,12 +123,11 @@ Each `DataLoaderPHP` instance contains a unique memoized cache. Use caution when used in long-lived applications or those which serve many users with different access permissions and consider creating a new instance per web request. -##### `new DataLoader(batchLoadFn $batchLoadFn [, Option $options])` +##### `new DataLoader(callable $batchLoadFn [, Option $options])` Create a new `DataLoaderPHP` given a batch loading instance and options. -- *$batchLoadFn*: A object which accepts a callable callback that accepts an Array of keys, - and returns a Promise which resolves to an Array of values. +- *$batchLoadFn*: A callable / callback which accepts an Array of keys, and returns a Promise which resolves to an Array of values. - *$options*: An optional object of options: - *batch*: Default `true`. Set to `false` to disable batching, instead diff --git a/src/BatchLoadFn.php b/src/BatchLoadFn.php deleted file mode 100644 index e5580af..0000000 --- a/src/BatchLoadFn.php +++ /dev/null @@ -1,43 +0,0 @@ - - * - * For the full copyright and license information, please view the LICENSE - * file that was distributed with this source code. - */ - -namespace Overblog\DataLoader; - -class BatchLoadFn -{ - private $batchLoadFn; - - public function __construct(callable $batchLoadFn = null) - { - $this->batchLoadFn = $batchLoadFn; - } - - public function getBatchLoadFn() - { - return $this->batchLoadFn; - } - - public function setBatchLoadFn(callable $batchLoadFn) - { - $this->batchLoadFn = $batchLoadFn; - return $this; - } - - public function __invoke(array $keys) - { - $batchLoadFn = $this->getBatchLoadFn(); - if (null === $batchLoadFn) { - throw new \LogicException('A valid batchLoadFn should be define.'); - } - - return $batchLoadFn($keys); - } -} diff --git a/src/DataLoader.php b/src/DataLoader.php index 41b6e26..e7916e9 100644 --- a/src/DataLoader.php +++ b/src/DataLoader.php @@ -16,7 +16,7 @@ class DataLoader { /** - * @var BatchLoadFn + * @var callable */ private $batchLoadFn; diff --git a/tests/BatchLoadFnTest.php b/tests/BatchLoadFnTest.php deleted file mode 100644 index 95237e6..0000000 --- a/tests/BatchLoadFnTest.php +++ /dev/null @@ -1,28 +0,0 @@ - - * - * For the full copyright and license information, please view the LICENSE - * file that was distributed with this source code. - */ - -namespace Overblog\DataLoader\Tests; - -use Overblog\DataLoader\BatchLoadFn; - -class BatchLoadFnTest extends \PHPUnit_Framework_TestCase -{ - /** - * @expectedException \LogicException - * @expectedExceptionMessage A valid batchLoadFn should be define. - */ - public function testNoBatchLoadFunctionGiven() - { - $batchLoadFn = new BatchLoadFn(); - - $batchLoadFn([]); - } -} diff --git a/tests/DataLoadTest.php b/tests/DataLoadTest.php index 9c21bf3..9e6335f 100644 --- a/tests/DataLoadTest.php +++ b/tests/DataLoadTest.php @@ -11,7 +11,6 @@ namespace Overblog\DataLoader\Tests; -use Overblog\DataLoader\BatchLoadFn; use Overblog\DataLoader\DataLoader; use Overblog\DataLoader\Option; use React\Promise\Promise; @@ -275,7 +274,7 @@ public function testAllowsForcefullyPrimingTheCache() public function testResolvesToErrorToIndicateFailure() { /** - * @var DataLoader $identityLoader + * @var DataLoader $evenLoader * @var \ArrayObject $loadCalls */ list($evenLoader, $loadCalls) = self::eventLoader(); @@ -300,7 +299,7 @@ public function testResolvesToErrorToIndicateFailure() public function testCanRepresentFailuresAndSuccessesSimultaneously() { /** - * @var DataLoader $identityLoader + * @var DataLoader $evenLoader * @var \ArrayObject $loadCalls */ list($evenLoader, $loadCalls) = self::eventLoader(); @@ -426,12 +425,13 @@ public function testCanClearValuesFromCacheAfterErrors() */ public function testPropagatesErrorToAllLoads() { - $loadCalls = new \ArrayObject(); - - $failLoader = new DataLoader(new BatchLoadFn(function ($keys) use (&$loadCalls) { - $loadCalls[] = $keys; + /** + * @var DataLoader $failLoader + * @var \ArrayObject $loadCalls + */ + list($failLoader, $loadCalls) = self::idLoader(null, function () { return \React\Promise\reject(new \Exception('I am a terrible loader')); - })); + }); $promise1 = $failLoader->load(1); $promise2 = $failLoader->load(2); @@ -776,6 +776,7 @@ public function testOnDestructionAllPromiseInQueueShouldBeCancelled() * @var DataLoader $loader */ list($loader) = self::idLoader(); + /** @var \Exception|null $exception */ $exception = null; $loader->load('A1')->then(null, function ($reason) use (&$exception) { $exception = $reason; @@ -825,20 +826,17 @@ private static function eventLoader() private static function idLoader(Option $options = null, callable $batchLoadFnCallBack = null) { $loadCalls = new \ArrayObject(); - $batchLoadFn = new BatchLoadFn(); if (null === $batchLoadFnCallBack) { $batchLoadFnCallBack = function ($keys) { return \React\Promise\resolve($keys); }; } - $batchLoadFn->setBatchLoadFn(function ($keys) use (&$loadCalls, $batchLoadFnCallBack) { + $identityLoader = new DataLoader(function ($keys) use (&$loadCalls, $batchLoadFnCallBack) { $loadCalls[] = $keys; return $batchLoadFnCallBack($keys); - }); - - $identityLoader = new DataLoader($batchLoadFn, $options); + }, $options); return [$identityLoader, $loadCalls]; } From 1a58b6ad59964d13c618b4f1a4522e297289fd39 Mon Sep 17 00:00:00 2001 From: Jeremiah VALERIE Date: Fri, 11 Nov 2016 17:49:49 +0100 Subject: [PATCH 3/4] Removed all eventLoop references --- .travis.yml | 3 +-- README.md | 7 +++---- src/DataLoader.php | 44 +++++++------------------------------------- 3 files changed, 11 insertions(+), 43 deletions(-) diff --git a/.travis.yml b/.travis.yml index 3d9c87b..69e37d0 100644 --- a/.travis.yml +++ b/.travis.yml @@ -29,5 +29,4 @@ install: composer update --prefer-dist --no-interaction script: if [ "$TRAVIS_PHP_VERSION" == "5.6" ]; then phpunit -d xdebug.max_nesting_level=1000 --debug --coverage-clover build/logs/clover.xml; else phpunit --debug; fi after_success: - - composer require "satooshi/php-coveralls:^1.0" - - travis_retry php vendor/bin/coveralls -v + - if [[ "$TRAVIS_PHP_VERSION" != "5.6"]]; then composer require "satooshi/php-coveralls:^1.0" && travis_retry php vendor/bin/coveralls -v; fi diff --git a/README.md b/README.md index 07855f4..07268c9 100644 --- a/README.md +++ b/README.md @@ -11,8 +11,7 @@ data sources such as databases or web services via batching and caching. ## Requirements -* This library require [React/Promise](https://github.com/reactphp/promise) and PHP >= 5.5 to works. -* The [React/EventLoop](https://github.com/reactphp/event-loop) component are **totally optional** (see `await` method for more details). +This library require [React/Promise](https://github.com/reactphp/promise) and PHP >= 5.5 to works. ## Getting Started @@ -40,8 +39,8 @@ A batch loading callable / callback accepts an Array of keys, and returns a Prom resolves to an Array of values. Then load individual values from the loader. DataLoaderPHP will coalesce all -individual loads which occur within a single frame of execution (a single tick -of the event loop if install or using `await` method) and then call your batch function with all requested keys. +individual loads which occur within a single frame of execution (using `await` method) +and then call your batch function with all requested keys. ```php $userLoader->load(1) diff --git a/src/DataLoader.php b/src/DataLoader.php index e7916e9..c43e701 100644 --- a/src/DataLoader.php +++ b/src/DataLoader.php @@ -35,26 +35,15 @@ class DataLoader */ private $queue = []; - /** - * @var null|\React\EventLoop\LoopInterface - */ - private $eventLoop; - - /** - * @var Promise - */ - private $resolvedPromise; - /** * @var self[] */ private static $instances = []; - public function __construct(BatchLoadFn $batchLoadFn, Option $options = null) + public function __construct(callable $batchLoadFn, Option $options = null) { $this->batchLoadFn = $batchLoadFn; $this->options = $options ?: new Option(); - $this->eventLoop = class_exists('React\\EventLoop\\Factory') ? \React\EventLoop\Factory::create() : null; $this->promiseCache = $this->options->getCacheMap(); self::$instances[] = $this; } @@ -97,12 +86,7 @@ function ($resolve, $reject) use (&$promise, $key, $shouldBatch) { // A single dispatch should be scheduled per queue at the time when the // queue changes from "empty" to "full". if (count($this->queue) === 1) { - if ($shouldBatch) { - // If batching, schedule a task to dispatch the queue. - $this->enqueuePostPromiseJob(function () { - $this->dispatchQueue(); - }); - } else { + if (!$shouldBatch) { // Otherwise dispatch the (queue of one) immediately. $this->dispatchQueue(); } @@ -124,11 +108,11 @@ function (callable $resolve, callable $reject) { /** * Loads multiple keys, promising an array of values: * - * [$a, $b] = $myLoader->loadMany(['a', 'b']); + * list($a, $b) = $myLoader->loadMany(['a', 'b']); * * This is equivalent to the more verbose: * - * [$a, $b] = \React\Promise\all([ + * list($a, $b) = \React\Promise\all([ * $myLoader->load('a'), * $myLoader->load('b') * ]); @@ -207,7 +191,9 @@ public function __destruct() if ($this->needProcess()) { foreach ($this->queue as $data) { try { - $data['promise']->cancel(); + /** @var Promise $promise */ + $promise = $data['promise']; + $promise->cancel(); } catch (\Exception $e) { // no need to do nothing if cancel failed } @@ -310,22 +296,6 @@ private function checkKey($key, $method) } } - /** - * @param $fn - */ - private function enqueuePostPromiseJob($fn) - { - if (!$this->resolvedPromise) { - $this->resolvedPromise = \React\Promise\resolve(); - } - - if ($this->eventLoop) { - $this->resolvedPromise->then(function () use ($fn) { - $this->eventLoop->nextTick($fn); - }); - } - } - /** * Given the current state of a Loader instance, perform a batch load * from its current queue. From c4dd048fda403ef3216bb8a40cca90b81ee2648e Mon Sep 17 00:00:00 2001 From: Jeremiah VALERIE Date: Fri, 11 Nov 2016 19:17:27 +0100 Subject: [PATCH 4/4] Add API Abuse tests --- .travis.yml | 2 +- src/DataLoader.php | 56 +++++++++---------- src/Option.php | 57 +------------------- tests/AbuseTest.php | 118 +++++++++++++++++++++++++++++++++++++++++ tests/DataLoadTest.php | 19 ++++--- 5 files changed, 155 insertions(+), 97 deletions(-) create mode 100644 tests/AbuseTest.php diff --git a/.travis.yml b/.travis.yml index 69e37d0..d405d18 100644 --- a/.travis.yml +++ b/.travis.yml @@ -29,4 +29,4 @@ install: composer update --prefer-dist --no-interaction script: if [ "$TRAVIS_PHP_VERSION" == "5.6" ]; then phpunit -d xdebug.max_nesting_level=1000 --debug --coverage-clover build/logs/clover.xml; else phpunit --debug; fi after_success: - - if [[ "$TRAVIS_PHP_VERSION" != "5.6"]]; then composer require "satooshi/php-coveralls:^1.0" && travis_retry php vendor/bin/coveralls -v; fi + - if [ "$TRAVIS_PHP_VERSION" == "5.6" ]; then composer require "satooshi/php-coveralls:^1.0" && travis_retry php vendor/bin/coveralls -v; fi diff --git a/src/DataLoader.php b/src/DataLoader.php index c43e701..e8980d2 100644 --- a/src/DataLoader.php +++ b/src/DataLoader.php @@ -123,7 +123,7 @@ function (callable $resolve, callable $reject) { public function loadMany($keys) { if (!is_array($keys) && !$keys instanceof \Traversable) { - throw new \InvalidArgumentException(sprintf('The %s function must be called with Array but got: %s.', __METHOD__, gettype($keys))); + throw new \InvalidArgumentException(sprintf('The "%s" method must be called with Array but got: %s.', __METHOD__, gettype($keys))); } return \React\Promise\all(array_map( function ($key) { @@ -229,51 +229,45 @@ public static function await($promise = null, $unwrap = true) { self::awaitInstances(); - if (null !== $promise) { - $resolvedValue = null; - $exception = null; + if (null === $promise) { + return null; + } + $resolvedValue = null; + $exception = null; - if (!is_callable([$promise, 'then'])) { - throw new \InvalidArgumentException('Promise must have a "then" method.'); - } + if (!is_callable([$promise, 'then'])) { + throw new \InvalidArgumentException(sprintf('The "%s" method must be called with a Promise ("then" method).', __METHOD__)); + } - $promise->then(function ($values) use (&$resolvedValue) { - $resolvedValue = $values; - }, function ($reason) use (&$exception) { - $exception = $reason; - }); - if ($exception instanceof \Exception) { - if (!$unwrap) { - return $exception; - } - throw $exception; + $promise->then(function ($values) use (&$resolvedValue) { + $resolvedValue = $values; + }, function ($reason) use (&$exception) { + $exception = $reason; + }); + if ($exception instanceof \Exception) { + if (!$unwrap) { + return $exception; } - - return $resolvedValue; + throw $exception; } + + return $resolvedValue; } private static function awaitInstances() { $dataLoaders = self::$instances; - if (empty($dataLoaders)) { - return; - } + if (!empty($dataLoaders)) { + $wait = true; - $wait = true; - - while ($wait) { - foreach ($dataLoaders as $dataLoader) { - try { + while ($wait) { + foreach ($dataLoaders as $dataLoader) { if (!$dataLoader || !$dataLoader->needProcess()) { $wait = false; continue; } $wait = true; $dataLoader->process(); - } catch (\Exception $e) { - $wait = false; - continue; } } } @@ -291,7 +285,7 @@ private function checkKey($key, $method) { if (null === $key) { throw new \InvalidArgumentException( - sprintf('The %s function must be called with a value, but got: %s.', $method, gettype($key)) + sprintf('The "%s" method must be called with a value, but got: %s.', $method, gettype($key)) ); } } diff --git a/src/Option.php b/src/Option.php index aecc641..ceaf3c6 100644 --- a/src/Option.php +++ b/src/Option.php @@ -11,8 +11,6 @@ namespace Overblog\DataLoader; -use Symfony\Component\Cache\Adapter\AdapterInterface; - class Option { /** @@ -52,9 +50,8 @@ public function __construct(array $params = []) $options = array_merge($defaultOptions, $params); - foreach ($options as $name => $value) { - $method = 'set'.ucfirst($name); - $this->$method($value); + foreach ($options as $property => $value) { + $this->$property = $value; } } @@ -66,16 +63,6 @@ public function shouldBatch() return $this->batch; } - /** - * @param boolean $batch - * @return Option - */ - public function setBatch($batch) - { - $this->batch = $batch; - return $this; - } - /** * @return int */ @@ -84,16 +71,6 @@ public function getMaxBatchSize() return $this->maxBatchSize; } - /** - * @param int $maxBatchSize - * @return Option - */ - public function setMaxBatchSize($maxBatchSize) - { - $this->maxBatchSize = $maxBatchSize; - return $this; - } - /** * @return boolean */ @@ -102,16 +79,6 @@ public function shouldCache() return $this->cache; } - /** - * @param boolean $cache - * @return Option - */ - public function setCache($cache) - { - $this->cache = $cache; - return $this; - } - /** * @return callable */ @@ -120,16 +87,6 @@ public function getCacheKeyFn() return $this->cacheKeyFn; } - /** - * @param callable $cacheKeyFn - * @return Option - */ - public function setCacheKeyFn(callable $cacheKeyFn = null) - { - $this->cacheKeyFn = $cacheKeyFn; - return $this; - } - /** * @return CacheMap */ @@ -137,14 +94,4 @@ public function getCacheMap() { return $this->cacheMap; } - - /** - * @param CacheMap $cacheMap - * @return Option - */ - public function setCacheMap(CacheMap $cacheMap) - { - $this->cacheMap = $cacheMap; - return $this; - } } diff --git a/tests/AbuseTest.php b/tests/AbuseTest.php new file mode 100644 index 0000000..8c9928f --- /dev/null +++ b/tests/AbuseTest.php @@ -0,0 +1,118 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Overblog\DataLoader\Tests; + +use Overblog\DataLoader\DataLoader; + +class AbuseTest extends \PHPUnit_Framework_TestCase +{ + /** + * @group provides-descriptive-error-messages-for-api-abuse + * + * @expectedException \InvalidArgumentException + * @expectedExceptionMessage The "Overblog\DataLoader\DataLoader::load" method must be called with a value, but got: NULL. + */ + public function testLoadFunctionRequiresAKeyNotNull() + { + self::idLoader()->load(null); + } + + /** + * @group provides-descriptive-error-messages-for-api-abuse + */ + public function testLoadFunctionRequiresAKeyWith0() + { + self::idLoader()->load(0); + } + + /** + * @group provides-descriptive-error-messages-for-api-abuse + * + * @expectedException \InvalidArgumentException + * @expectedExceptionMessage The "Overblog\DataLoader\DataLoader::loadMany" method must be called with Array but got: integer. + */ + public function testLoadManyFunctionRequiresAListOfKey() + { + self::idLoader()->loadMany(1, 2, 3); + } + + /** + * @group provides-descriptive-error-messages-for-api-abuse + */ + public function testLoadManyFunctionRequiresAListEmptyArrayAccepted() + { + self::idLoader()->loadMany([]); + } + + /** + * @group provides-descriptive-error-messages-for-api-abuse + * + * @expectedException \RuntimeException + * @expectedExceptionMessage DataLoader must be constructed with a function which accepts Array and returns Promise>, but the function did not return a Promise: array. + */ + public function testBatchFunctionMustReturnAPromiseNotAValue() + { + DataLoader::await(self::idLoader(function ($keys) { + return $keys; + })->load(1)); + } + + /** + * @group provides-descriptive-error-messages-for-api-abuse + * + * @expectedException \RuntimeException + * @expectedExceptionMessage DataLoader must be constructed with a function which accepts Array and returns Promise>, but the function did not return a Promise of an Array: NULL. + */ + public function testBatchFunctionMustReturnAPromiseOfAnArrayNotNull() + { + DataLoader::await(self::idLoader(function () { + return \React\Promise\resolve(); + })->load(1)); + } + + /** + * @group provides-descriptive-error-messages-for-api-abuse + * @expectedException \RuntimeException + * @expectedExceptionMessage DataLoader must be constructed with a function which accepts Array and returns Promise>, but the function did not return a Promise of an Array of the same length as the Array of keys. + */ + public function testBatchFunctionMustPromiseAnArrayOfCorrectLength() + { + DataLoader::await(self::idLoader(function () { + return \React\Promise\resolve([]); + })->load(1)); + } + + /** + * @group provides-descriptive-error-messages-for-api-abuse + * @expectedException \InvalidArgumentException + * @expectedExceptionMessage The "Overblog\DataLoader\DataLoader::await" method must be called with a Promise ("then" method). + */ + public function testAwaitPromiseMustHaveAThenMethod() + { + DataLoader::await([]); + } + + /** + * @param callable $batchLoadFn + * @return DataLoader + */ + private static function idLoader(callable $batchLoadFn = null) + { + if (null === $batchLoadFn) { + $batchLoadFn = function ($keys) { + return \React\Promise\all($keys); + }; + } + + return new DataLoader($batchLoadFn); + } +} diff --git a/tests/DataLoadTest.php b/tests/DataLoadTest.php index 9e6335f..bfc16f8 100644 --- a/tests/DataLoadTest.php +++ b/tests/DataLoadTest.php @@ -279,12 +279,7 @@ public function testResolvesToErrorToIndicateFailure() */ list($evenLoader, $loadCalls) = self::eventLoader(); - $caughtError = null; - try { - DataLoader::await($evenLoader->load(1)); - } catch (\Exception $error) { - $caughtError = $error; - } + $caughtError = DataLoader::await($evenLoader->load(1), false); $this->assertInstanceOf(\Exception::class, $caughtError); $this->assertEquals($caughtError->getMessage(), 'Odd: 1'); $value2 = DataLoader::await($evenLoader->load(2)); @@ -308,11 +303,10 @@ public function testCanRepresentFailuresAndSuccessesSimultaneously() $promise2 = $evenLoader->load(2); $caughtError = null; - try { - DataLoader::await($promise1); - } catch (\Exception $error) { + $promise1->then(null, function ($error) use (&$caughtError) { $caughtError = $error; - } + }); + DataLoader::await(); $this->assertInstanceOf(\Exception::class, $caughtError); $this->assertEquals($caughtError->getMessage(), 'Odd: 1'); @@ -788,6 +782,11 @@ public function testOnDestructionAllPromiseInQueueShouldBeCancelled() $this->assertEquals($exception->getMessage(), 'DataLoader destroyed before promise complete.'); } + public function testCallingAwaitFunctionWhenNoInstanceOfDataLoaderShouldNotThrowError() + { + DataLoader::await(); + } + public function cacheKey($key) { $cacheKey = [];