From 828c6b0fc39dd6289d148833301eff463bda49f8 Mon Sep 17 00:00:00 2001 From: Vladimir Razuvaev Date: Tue, 15 Aug 2017 18:05:09 +0700 Subject: [PATCH] Server: disable query batching by default; allow array as server config --- src/Server/Helper.php | 27 ++++++++---- src/Server/ServerConfig.php | 39 ++++++++++++++++-- src/Server/StandardServer.php | 9 +++- tests/Server/QueryExecutionTest.php | 64 ++++++++++++++++++++++++++++- tests/Server/ServerConfigTest.php | 64 ++++++++++++++++++++++++++++- 5 files changed, 188 insertions(+), 15 deletions(-) diff --git a/src/Server/Helper.php b/src/Server/Helper.php index 610b26e..eb271ce 100644 --- a/src/Server/Helper.php +++ b/src/Server/Helper.php @@ -28,7 +28,7 @@ class Helper { /** * Executes GraphQL operation with given server configuration and returns execution result - * (or promise when promise adapter is different from SyncPromiseAdapter) + * (or promise when promise adapter is different than SyncPromiseAdapter) * * @param ServerConfig $config * @param OperationParams $op @@ -61,7 +61,7 @@ class Helper $result = []; foreach ($operations as $operation) { - $result[] = $this->promiseToExecuteOperation($promiseAdapter, $config, $operation); + $result[] = $this->promiseToExecuteOperation($promiseAdapter, $config, $operation, true); } $result = $promiseAdapter->all($result); @@ -77,20 +77,28 @@ class Helper * @param PromiseAdapter $promiseAdapter * @param ServerConfig $config * @param OperationParams $op + * @param bool $isBatch * @return Promise */ - private function promiseToExecuteOperation(PromiseAdapter $promiseAdapter, ServerConfig $config, OperationParams $op) + private function promiseToExecuteOperation(PromiseAdapter $promiseAdapter, ServerConfig $config, OperationParams $op, $isBatch = false) { try { + if ($isBatch && !$config->getQueryBatching()) { + throw new RequestError("Batched queries are not supported by this server"); + } + $errors = $this->validateOperationParams($op); if (!empty($errors)) { + $errors = Utils::map($errors, function(RequestError $err) { + return Error::createLocatedError($err, null, null); + }); return $promiseAdapter->createFulfilled( new ExecutionResult(null, $errors) ); } - $doc = $op->queryId ? static::loadPersistedQuery($config, $op) : $op->query; + $doc = $op->queryId ? $this->loadPersistedQuery($config, $op) : $op->query; if (!$doc instanceof DocumentNode) { $doc = Parser::parse($doc); @@ -154,8 +162,7 @@ class Helper * @param ServerConfig $config * @param OperationParams $op * @return mixed - * @throws Error - * @throws InvariantViolation + * @throws RequestError */ private function loadPersistedQuery(ServerConfig $config, OperationParams $op) { @@ -251,7 +258,7 @@ class Helper * * @param callable|null $readRawBodyFn * @return OperationParams|OperationParams[] - * @throws Error + * @throws RequestError */ public function parseHttpRequest(callable $readRawBodyFn = null) { @@ -359,6 +366,10 @@ class Helper } } + /** + * @param $result + * @param $exitWhenDone + */ private function doSendResponse($result, $exitWhenDone) { $httpStatus = $this->resolveHttpStatus($result); @@ -382,7 +393,7 @@ class Helper * @param array $bodyParams * @param array $queryParams * @return OperationParams|OperationParams[] - * @throws Error + * @throws RequestError */ public function parseRequestParams($method, array $bodyParams, array $queryParams) { diff --git a/src/Server/ServerConfig.php b/src/Server/ServerConfig.php index 4f65d52..f8334f2 100644 --- a/src/Server/ServerConfig.php +++ b/src/Server/ServerConfig.php @@ -11,9 +11,17 @@ class ServerConfig /** * @return static */ - public static function create() + public static function create(array $config = []) { - return new static(); + $instance = new static(); + foreach ($config as $key => $value) { + $method = 'set' . ucfirst($key); + if (!method_exists($instance, $method)) { + throw new InvariantViolation("Unknown server config option \"$key\""); + } + $instance->$method($value); + } + return $instance; } /** @@ -41,6 +49,11 @@ class ServerConfig */ private $debug = false; + /** + * @var bool + */ + private $queryBatching = false; + /** * @var array|callable */ @@ -173,7 +186,7 @@ class ServerConfig { if (!is_callable($validationRules) && !is_array($validationRules) && $validationRules !== null) { throw new InvariantViolation( - __METHOD__ . ' expects array of validation rules or callable returning such array, but got: %s' . + 'Server config expects array of validation rules or callable returning such array, but got ' . Utils::printSafe($validationRules) ); } @@ -253,4 +266,24 @@ class ServerConfig $this->debug = (bool) $set; return $this; } + + /** + * @return bool + */ + public function getQueryBatching() + { + return $this->queryBatching; + } + + /** + * Allow batching queries + * + * @param bool $enableBatching + * @return ServerConfig + */ + public function setQueryBatching($enableBatching) + { + $this->queryBatching = (bool) $enableBatching; + return $this; + } } diff --git a/src/Server/StandardServer.php b/src/Server/StandardServer.php index f73a2a5..e5733f6 100644 --- a/src/Server/StandardServer.php +++ b/src/Server/StandardServer.php @@ -43,8 +43,15 @@ class StandardServer * StandardServer constructor. * @param ServerConfig $config */ - protected function __construct(ServerConfig $config) + protected function __construct($config) { + if (is_array($config)) { + $config = ServerConfig::create($config); + } + if (!$config instanceof ServerConfig) { + throw new InvariantViolation(""); + } + $this->config = $config; $this->helper = new Helper(); } diff --git a/tests/Server/QueryExecutionTest.php b/tests/Server/QueryExecutionTest.php index cee3bae..ca913b5 100644 --- a/tests/Server/QueryExecutionTest.php +++ b/tests/Server/QueryExecutionTest.php @@ -10,6 +10,7 @@ use GraphQL\Language\Parser; use GraphQL\Schema; use GraphQL\Server\Helper; use GraphQL\Server\OperationParams; +use GraphQL\Server\RequestError; use GraphQL\Server\ServerConfig; use GraphQL\Type\Definition\ObjectType; use GraphQL\Type\Definition\Type; @@ -102,7 +103,8 @@ class QueryExecutionTest extends \PHPUnit_Framework_TestCase ]) ]); - $this->config = ServerConfig::create()->setSchema($schema); + $this->config = ServerConfig::create() + ->setSchema($schema); } public function testSimpleQueryExecution() @@ -285,6 +287,42 @@ class QueryExecutionTest extends \PHPUnit_Framework_TestCase $this->assertEquals($expected, $result->toArray()); } + public function testBatchedQueriesAreDisabledByDefault() + { + $batch = [ + [ + 'query' => '{invalid}' + ], + [ + 'query' => '{f1,fieldWithException}' + ] + ]; + + $result = $this->executeBatchedQuery($batch); + + $expected = [ + [ + 'errors' => [ + [ + 'message' => 'Batched queries are not supported by this server', + 'category' => 'request' + ] + ] + ], + [ + 'errors' => [ + [ + 'message' => 'Batched queries are not supported by this server', + 'category' => 'request' + ] + ] + ], + ]; + + $this->assertEquals($expected[0], $result[0]->toArray()); + $this->assertEquals($expected[1], $result[1]->toArray()); + } + public function testMutationsAreNotAllowedInReadonlyMode() { $mutation = 'mutation { a }'; @@ -416,6 +454,8 @@ class QueryExecutionTest extends \PHPUnit_Framework_TestCase public function testExecutesBatchedQueries() { + $this->config->setQueryBatching(true); + $batch = [ [ 'query' => '{invalid}' @@ -479,6 +519,7 @@ class QueryExecutionTest extends \PHPUnit_Framework_TestCase $calls = []; $this->config + ->setQueryBatching(true) ->setRootValue('1') ->setContext([ 'buffer' => function($num) use (&$calls) { @@ -525,6 +566,27 @@ class QueryExecutionTest extends \PHPUnit_Framework_TestCase $this->assertEquals($expected[2], $result[2]->toArray()); } + public function testValidatesParamsBeforeExecution() + { + $op = OperationParams::create(['queryBad' => '{f1}']); + $helper = new Helper(); + $result = $helper->executeOperation($this->config, $op); + $this->assertInstanceOf(ExecutionResult::class, $result); + + $this->assertEquals(null, $result->data); + $this->assertCount(1, $result->errors); + + $this->assertEquals( + 'GraphQL Request must include at least one of those two parameters: "query" or "queryId"', + $result->errors[0]->getMessage() + ); + + $this->assertInstanceOf( + RequestError::class, + $result->errors[0]->getPrevious() + ); + } + private function executePersistedQuery($queryId, $variables = null) { $op = OperationParams::create(['queryId' => $queryId, 'variables' => $variables]); diff --git a/tests/Server/ServerConfigTest.php b/tests/Server/ServerConfigTest.php index a39caaa..9acc43f 100644 --- a/tests/Server/ServerConfigTest.php +++ b/tests/Server/ServerConfigTest.php @@ -1,11 +1,12 @@ assertEquals(null, $config->getDefaultFieldResolver()); $this->assertEquals(null, $config->getPersistentQueryLoader()); $this->assertEquals(false, $config->getDebug()); + $this->assertEquals(false, $config->getQueryBatching()); } public function testAllowsSettingSchema() @@ -141,4 +143,62 @@ class ServerConfigTest extends \PHPUnit_Framework_TestCase $config->setDebug(false); $this->assertSame(false, $config->getDebug()); } + + public function testAcceptsArray() + { + $arr = [ + 'schema' => new \GraphQL\Type\Schema([ + 'query' => new ObjectType(['name' => 't', 'fields' => ['a' => Type::string()]]) + ]), + 'context' => new \stdClass(), + 'rootValue' => new \stdClass(), + 'errorFormatter' => function() {}, + 'promiseAdapter' => new SyncPromiseAdapter(), + 'validationRules' => [function() {}], + 'defaultFieldResolver' => function() {}, + 'persistentQueryLoader' => function() {}, + 'debug' => true, + 'queryBatching' => true, + ]; + + $config = ServerConfig::create($arr); + + $this->assertSame($arr['schema'], $config->getSchema()); + $this->assertSame($arr['context'], $config->getContext()); + $this->assertSame($arr['rootValue'], $config->getRootValue()); + $this->assertSame($arr['errorFormatter'], $config->getErrorFormatter()); + $this->assertSame($arr['promiseAdapter'], $config->getPromiseAdapter()); + $this->assertSame($arr['validationRules'], $config->getValidationRules()); + $this->assertSame($arr['defaultFieldResolver'], $config->getDefaultFieldResolver()); + $this->assertSame($arr['persistentQueryLoader'], $config->getPersistentQueryLoader()); + $this->assertSame(true, $config->getDebug()); + $this->assertSame(true, $config->getQueryBatching()); + } + + public function testThrowsOnInvalidArrayKey() + { + $arr = [ + 'missingKey' => 'value' + ]; + + $this->setExpectedException( + InvariantViolation::class, + 'Unknown server config option "missingKey"' + ); + + ServerConfig::create($arr); + } + + public function testInvalidValidationRules() + { + $rules = new \stdClass(); + $config = ServerConfig::create(); + + $this->setExpectedException( + InvariantViolation::class, + 'Server config expects array of validation rules or callable returning such array, but got instance of stdClass' + ); + + $config->setValidationRules($rules); + } }