From d26de9ca8ba30321e29e1c2cf8811ab383886939 Mon Sep 17 00:00:00 2001 From: fmizzell Date: Thu, 12 Dec 2019 10:35:34 -0800 Subject: [PATCH] Do not artificially limit count queries (#273) * Ignore more things. * Be more exact in out implemetation of the interface in database table storage. * Do not limit count queries, and create an sql_endpoint service. * Fix code style issues. * Fix complexity issue. * Fix unit tests. * Make code concise. --- .gitignore | 5 +- .../tests/src/Unit/Controller/DocsTest.php | 17 +- .../src/Storage/AbstractDatabaseTable.php | 13 +- .../dkan_sql_endpoint.info.yml | 1 + .../dkan_sql_endpoint.services.yml | 5 + .../dkan_sql_endpoint/src/Controller/Api.php | 199 +++--------------- .../GetStringsFromStateMachineExecution.php | 72 +++++++ .../custom/dkan_sql_endpoint/src/Service.php | 178 ++++++++++++++++ .../tests/src/Unit/Controller/ApiTest.php | 10 +- .../tests/src/Unit/ServiceTest.php | 48 +++++ 10 files changed, 363 insertions(+), 185 deletions(-) create mode 100644 modules/custom/dkan_sql_endpoint/dkan_sql_endpoint.services.yml create mode 100644 modules/custom/dkan_sql_endpoint/src/Helper/GetStringsFromStateMachineExecution.php create mode 100644 modules/custom/dkan_sql_endpoint/src/Service.php create mode 100644 modules/custom/dkan_sql_endpoint/tests/src/Unit/ServiceTest.php diff --git a/.gitignore b/.gitignore index a554ef9da..ee423408c 100644 --- a/.gitignore +++ b/.gitignore @@ -13,4 +13,7 @@ composer.lock .docksal .idea docs/_build -.vscode \ No newline at end of file +.vscode +coverage +cypress/videos +package-lock.json diff --git a/modules/custom/dkan_api/tests/src/Unit/Controller/DocsTest.php b/modules/custom/dkan_api/tests/src/Unit/Controller/DocsTest.php index b72170ed7..6c172b2cd 100644 --- a/modules/custom/dkan_api/tests/src/Unit/Controller/DocsTest.php +++ b/modules/custom/dkan_api/tests/src/Unit/Controller/DocsTest.php @@ -8,7 +8,6 @@ use Drupal\Core\Extension\ModuleHandlerInterface; use Drupal\dkan_common\Tests\Mock\Chain; use Drupal\dkan_common\Tests\Mock\Options; -use PHPUnit\Framework\TestCase; use Symfony\Component\DependencyInjection\ContainerInterface; use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpFoundation\RequestStack; @@ -18,6 +17,9 @@ */ class DocsTest extends DkanTestBase { + /** + * + */ public function testGetVersions() { $mockChain = $this->getCommonMockChain(); $controller = Docs::create($mockChain->getMock()); @@ -28,9 +30,12 @@ public function testGetVersions() { $this->assertEquals($spec, $response->getContent()); } + /** + * + */ public function testGetComplete() { $mock = $this->getCommonMockChain() - ->add(RequestStack::class, 'getCurrentRequest',Request::class) + ->add(RequestStack::class, 'getCurrentRequest', Request::class) ->add(Request::class, 'get', NULL); $spec = '{"openapi":"3.0.1","info":{"title":"API Documentation","version":"Alpha"},"components":{"securitySchemes":{"basicAuth":{"type":"http","scheme":"basic"}}},"paths":{"\/api\/1\/metastore\/schemas\/dataset\/items\/{identifier}":{"get":{"summary":"Get this dataset","tags":["Dataset"],"parameters":[{"name":"identifier","in":"path","description":"Dataset uuid","required":true,"schema":{"type":"string"}}],"responses":{"200":{"description":"Ok"}}},"delete":{"summary":"This operation should not be present in dataset-specific docs.","security":[{"basicAuth":[]}],"responses":{"200":{"description":"Ok"}}},"post":null},"\/api\/1\/some\/other\/path":{"patch":{"summary":"This path and operation should not be present in dataset-specific docs.","security":[{"basicAuth":[]}],"responses":{"200":{"description":"Ok"}}}}}}'; @@ -41,9 +46,12 @@ public function testGetComplete() { $this->assertEquals($spec, $response->getContent()); } + /** + * + */ public function testGetPublic() { $mock = $this->getCommonMockChain() - ->add(RequestStack::class, 'getCurrentRequest',Request::class) + ->add(RequestStack::class, 'getCurrentRequest', Request::class) ->add(Request::class, 'get', 'false'); $spec = '{"openapi":"3.0.1","info":{"title":"API Documentation","version":"Alpha"},"components":[],"paths":{"\/api\/1\/metastore\/schemas\/dataset\/items\/{identifier}":{"get":{"summary":"Get this dataset","tags":["Dataset"],"parameters":[{"name":"identifier","in":"path","description":"Dataset uuid","required":true,"schema":{"type":"string"}}],"responses":{"200":{"description":"Ok"}}},"post":null}}}'; @@ -54,6 +62,9 @@ public function testGetPublic() { $this->assertEquals($spec, $response->getContent()); } + /** + * + */ public function getCommonMockChain() { $options = (new Options()) ->add('module_handler', ModuleHandlerInterface::class) diff --git a/modules/custom/dkan_common/src/Storage/AbstractDatabaseTable.php b/modules/custom/dkan_common/src/Storage/AbstractDatabaseTable.php index 2b1bc643a..bc13e0bc6 100644 --- a/modules/custom/dkan_common/src/Storage/AbstractDatabaseTable.php +++ b/modules/custom/dkan_common/src/Storage/AbstractDatabaseTable.php @@ -61,13 +61,16 @@ public function __construct(Connection $connection) { public function retrieve(string $id) { $this->setTable(); - $result = $this->connection->select($this->getTableName(), 't') + /* @var $statement StatementInterface */ + $statement = $this->connection->select($this->getTableName(), 't') ->fields('t', array_keys($this->getSchema()['fields'])) ->condition($this->primaryKey(), $id) - ->execute() - ->fetch(); + ->execute(); - return $result; + // The docs do not mention it, but fetch can return false. + $return = (isset($statement)) ? $statement->fetch() : NULL; + + return ($return === FALSE) ? NULL : $return; } /** @@ -107,7 +110,7 @@ public function store($data, string $id = NULL): string { $returned_id = NULL; - if (!$existing) { + if ($existing === NULL) { $q = $this->connection->insert($this->getTableName()); $q->fields($this->getNonSerialFields()); $q->values($data); diff --git a/modules/custom/dkan_sql_endpoint/dkan_sql_endpoint.info.yml b/modules/custom/dkan_sql_endpoint/dkan_sql_endpoint.info.yml index 0a31922dd..c4dcf65c3 100644 --- a/modules/custom/dkan_sql_endpoint/dkan_sql_endpoint.info.yml +++ b/modules/custom/dkan_sql_endpoint/dkan_sql_endpoint.info.yml @@ -5,4 +5,5 @@ core: 8.x package: DKAN dependencies: + - dkan_common - dkan_datastore diff --git a/modules/custom/dkan_sql_endpoint/dkan_sql_endpoint.services.yml b/modules/custom/dkan_sql_endpoint/dkan_sql_endpoint.services.yml new file mode 100644 index 000000000..b856b3f50 --- /dev/null +++ b/modules/custom/dkan_sql_endpoint/dkan_sql_endpoint.services.yml @@ -0,0 +1,5 @@ +services: + dkan_sql_endpoint.service: + class: \Drupal\dkan_sql_endpoint\Service + arguments: + - '@config.factory' diff --git a/modules/custom/dkan_sql_endpoint/src/Controller/Api.php b/modules/custom/dkan_sql_endpoint/src/Controller/Api.php index 59f9bf568..f5905de00 100644 --- a/modules/custom/dkan_sql_endpoint/src/Controller/Api.php +++ b/modules/custom/dkan_sql_endpoint/src/Controller/Api.php @@ -2,25 +2,24 @@ namespace Drupal\dkan_sql_endpoint\Controller; -use Drupal\Core\Config\ConfigFactory; +use Symfony\Component\DependencyInjection\ContainerInterface; +use Symfony\Component\HttpFoundation\RequestStack; use Drupal\Core\Database\Connection; +use Drupal\Core\DependencyInjection\ContainerInjectionInterface; +use Drupal\dkan_common\JsonResponseTrait; use Drupal\dkan_datastore\Service\Factory\Resource; +use Drupal\dkan_datastore\Storage\DatabaseTable; use Drupal\dkan_datastore\Storage\DatabaseTableFactory; -use Drupal\dkan_datastore\Storage\Query; -use Drupal\Core\DependencyInjection\ContainerInjectionInterface; -use Maquina\StateMachine\MachineOfMachines; -use SqlParser\SqlParser; -use Symfony\Component\DependencyInjection\ContainerInterface; -use Symfony\Component\HttpFoundation\JsonResponse; -use Symfony\Component\HttpFoundation\RequestStack; +use Drupal\dkan_sql_endpoint\Service; /** * Api class. */ class Api implements ContainerInjectionInterface { + use JsonResponseTrait; + private $service; private $database; - private $configFactory; private $requestStack; private $resourceServiceFactory; private $databaseTableFactory; @@ -33,11 +32,10 @@ class Api implements ContainerInjectionInterface { * @codeCoverageIgnore */ public static function create(ContainerInterface $container) { - - return new Api( + return new static( + $container->get('dkan_sql_endpoint.service'), $container->get('database'), $container->get('dkan_datastore.service.factory.resource'), - $container->get('config.factory'), $container->get('request_stack'), $container->get('dkan_datastore.database_table_factory') ); @@ -47,15 +45,15 @@ public static function create(ContainerInterface $container) { * Constructor. */ public function __construct( + Service $service, Connection $database, Resource $resourceServiceFactory, - ConfigFactory $configFactory, RequestStack $requestStack, DatabaseTableFactory $databaseTableFactory ) { + $this->service = $service; $this->database = $database; $this->resourceServiceFactory = $resourceServiceFactory; - $this->configFactory = $configFactory; $this->requestStack = $requestStack; $this->databaseTableFactory = $databaseTableFactory; } @@ -69,7 +67,7 @@ public function runQueryGet() { $query = $this->requestStack->getCurrentRequest()->get('query'); if (empty($query)) { - return $this->response("Missing 'query' query parameter", 400); + return $this->getResponse("Missing 'query' query parameter", 400); } return $this->runQuery($query); @@ -88,7 +86,7 @@ public function runQueryPost() { } if (empty($query)) { - return $this->response("Missing 'query' property in the request's body.", 400); + return $this->getResponse("Missing 'query' property in the request's body.", 400); } return $this->runQuery($query); @@ -97,190 +95,41 @@ public function runQueryPost() { /** * Private. */ - private function runQuery($query) { - $parser = new SqlParser(); - - if ($parser->validate($query) === FALSE) { - return $this->response("Invalid query string.", 500); - } - - $state_machine = $parser->getValidatingMachine(); - + private function runQuery(string $query) { try { - $query_object = $this->getQueryObject($state_machine); + $queryObject = $this->service->getQueryObject($query); } catch (\Exception $e) { - return $this->response("No datastore.", 500); + return $this->getResponseFromException($e); } - $databaseTable = $this->getDatabaseTable($state_machine); + $databaseTable = $this->getDatabaseTable($this->service->getTableName($query)); try { - $result = $databaseTable->query($query_object); + $result = $databaseTable->query($queryObject); } catch (\Exception $e) { - return $this->response("Querying a datastore that does not exist.", 500); + return $this->getResponseFromException($e); } - return $this->response($result, 200); + return $this->getResponse($result, 200); } /** * Private. */ - private function getDatabaseTable($stateMachine) { - $resource = $this->getResource($stateMachine); + private function getDatabaseTable(string $uuid): DatabaseTable { + $resource = $this->getResource($uuid); return $this->databaseTableFactory->getInstance($resource->getId(), ['resource' => $resource]); } /** * Private. */ - private function getResource(MachineOfMachines $state_machine) { - $uuid = $this->getUuidFromSelect($state_machine->gsm('select')->gsm('table_var')); - + private function getResource(string $uuid) { /* @var $resourceService \Drupal\dkan_datastore\Service\Resource */ $resourceService = $this->resourceServiceFactory->getInstance($uuid); return $resourceService->get(); } - /** - * Private. - */ - private function getQueryObject($state_machine) { - $object = new Query(); - $this->setQueryObjectSelect($object, $state_machine->gsm('select')); - $this->setQueryObjectWhere($object, $state_machine->gsm('where')); - $this->setQueryObjectOrderBy($object, $state_machine->gsm('order_by')); - $this->setQueryObjectLimit($object, $state_machine->gsm('limit')); - - return $object; - } - - /** - * Private. - */ - private function setQueryObjectSelect(Query $object, $state_machine) { - $strings = $this->getStringsFromStringMachine($state_machine->gsm('select_count_all')); - if (!empty($strings)) { - $object->count(); - return; - } - - $strings = $this->getStringsFromStringMachine($state_machine->gsm('select_var_all')); - if (!empty($strings)) { - return; - } - - $strings = $this->getStringsFromStringMachine($state_machine->gsm('select_var')); - foreach ($strings as $property) { - $object->filterByProperty($property); - } - } - - /** - * Private. - */ - private function setQueryObjectWhere(Query $object, $state_machine) { - $properties = $this->getStringsFromStringMachine($state_machine->gsm('where_column')); - $values = $this->getStringsFromStringMachine($state_machine->gsm('quoted_string')->gsm('string')); - - foreach ($properties as $index => $property) { - $value = $values[$index]; - if ($value) { - $object->conditionByIsEqualTo($property, $value); - } - } - } - - /** - * Private. - */ - private function setQueryObjectOrderBy(Query $object, $state_machine) { - $properties = $this->getStringsFromStringMachine($state_machine->gsm('order_var')); - - $direction = $this->getStringsFromStringMachine($state_machine->gsm('order_asc')); - if (!empty($direction)) { - foreach ($properties as $property) { - $object->sortByAscending($property); - } - } - else { - foreach ($properties as $property) { - $object->sortByDescending($property); - } - } - } - - /** - * Private. - */ - private function setQueryObjectLimit(Query $object, $state_machine) { - $rows_limit = $this->configFactory->get('dkan_sql_endpoint.settings')->get('rows_limit'); - - $limit = $this->getStringsFromStringMachine($state_machine->gsm('numeric1')); - if (!empty($limit) && $limit[0] <= $rows_limit) { - $object->limitTo($limit[0]); - } - else { - $object->limitTo($rows_limit); - } - - $offset = $this->getStringsFromStringMachine($state_machine->gsm('numeric2')); - if (!empty($offset)) { - $object->offsetBy($offset[0]); - } - } - - /** - * Private. - */ - private function getUuidFromSelect($machine) { - $strings = $this->getStringsFromStringMachine($machine); - if (empty($strings)) { - throw new \Exception("No UUID given"); - } - return $strings[0]; - } - - /** - * Private. - */ - private function getStringsFromStringMachine($machine) { - $strings = []; - $current_string = ""; - $array = $machine->execution; - - foreach ($array as $states_or_input) { - if (is_array($states_or_input)) { - $states = $states_or_input; - if (in_array(0, $states) && !empty($current_string)) { - $strings[] = $current_string; - $current_string = ""; - } - } - else { - $input = $states_or_input; - $current_string .= $input; - } - } - - if (!empty($current_string)) { - $strings[] = $current_string; - } - - return $strings; - } - - /** - * Private. - */ - private function response($message, $code) { - return new JsonResponse( - $message, - $code, - ["Access-Control-Allow-Origin" => "*"] - ); - } - } diff --git a/modules/custom/dkan_sql_endpoint/src/Helper/GetStringsFromStateMachineExecution.php b/modules/custom/dkan_sql_endpoint/src/Helper/GetStringsFromStateMachineExecution.php new file mode 100644 index 000000000..079a80fbd --- /dev/null +++ b/modules/custom/dkan_sql_endpoint/src/Helper/GetStringsFromStateMachineExecution.php @@ -0,0 +1,72 @@ +execution = $stateMachineExecution; + } + + /** + * Get. + */ + public function get() { + foreach ($this->execution as $states_or_input) { + if ($this->isStates($states_or_input)) { + $this->processStates($states_or_input); + continue; + } + + $input = $states_or_input; + $this->currentString .= $input; + } + + $this->saveAndResetCurrentString(); + + return $this->strings; + } + + /** + * Private. + */ + private function processStates(array $states) { + if ($this->containsFirstState($states)) { + $this->saveAndResetCurrentString(); + } + } + + /** + * Private. + */ + private function saveAndResetCurrentString() { + if (!empty($this->currentString)) { + $this->strings[] = $this->currentString; + $this->currentString = ""; + } + } + + /** + * Private. + */ + private function isStates($input): bool { + return is_array($input); + } + + /** + * Private. + */ + private function containsFirstState(array $states): bool { + return in_array(0, $states); + } + +} diff --git a/modules/custom/dkan_sql_endpoint/src/Service.php b/modules/custom/dkan_sql_endpoint/src/Service.php new file mode 100644 index 000000000..eff5129d5 --- /dev/null +++ b/modules/custom/dkan_sql_endpoint/src/Service.php @@ -0,0 +1,178 @@ +get('config.factory')); + } + + /** + * Constructor. + */ + public function __construct(ConfigFactory $configFactory) { + $this->configFactory = $configFactory; + } + + /** + * Get table name. + * + * @param string $sqlString + * A string with an sql statement. + * + * @return string + * The table name from the sql statement. + * + * @throws \Exception + */ + public function getTableName(string $sqlString): string { + $stateMachine = $this->validate($sqlString); + return $this->getTableNameFromSelect($stateMachine->gsm('select')); + } + + /** + * Private. + */ + private function getTableNameFromSelect(Machine $selectMachine): string { + $machine = $selectMachine->gsm('table_var'); + $strings = $this->getStringsFromStringMachine($machine); + if (empty($strings)) { + throw new \Exception("No table name"); + } + return $strings[0]; + } + + /** + * Get a query object from a sql string. + * + * @param string $sqlString + * A string with a sql statement. + * + * @return \Drupal\dkan_datastore\Storage\Query + * A query object. + */ + public function getQueryObject(string $sqlString): Query { + return $this->getQueryObjectFromStateMachine($this->validate($sqlString)); + } + + /** + * Private. + */ + private function validate(string $sqlString): Machine { + $parser = new SqlParser(); + if ($parser->validate($sqlString) === FALSE) { + throw new \Exception("Invalid query string."); + } + + return $parser->getValidatingMachine(); + } + + /** + * Private. + */ + private function getQueryObjectFromStateMachine(Machine $state_machine): Query { + $object = new Query(); + $this->setQueryObjectSelect($object, $state_machine->gsm('select')); + $this->setQueryObjectWhere($object, $state_machine->gsm('where')); + $this->setQueryObjectOrderBy($object, $state_machine->gsm('order_by')); + $this->setQueryObjectLimit($object, $state_machine->gsm('limit')); + + return $object; + } + + /** + * Private. + */ + private function setQueryObjectSelect(Query $object, Machine $state_machine) { + $strings = $this->getStringsFromStringMachine($state_machine->gsm('select_count_all')); + if (!empty($strings)) { + $object->count(); + return; + } + + $strings = $this->getStringsFromStringMachine($state_machine->gsm('select_var_all')); + if (!empty($strings)) { + return; + } + + $strings = $this->getStringsFromStringMachine($state_machine->gsm('select_var')); + foreach ($strings as $property) { + $object->filterByProperty($property); + } + } + + /** + * Private. + */ + private function setQueryObjectWhere(Query $object, Machine $state_machine) { + $properties = $this->getStringsFromStringMachine($state_machine->gsm('where_column')); + $values = $this->getStringsFromStringMachine($state_machine->gsm('quoted_string')->gsm('string')); + + foreach ($properties as $index => $property) { + $value = $values[$index]; + if ($value) { + $object->conditionByIsEqualTo($property, $value); + } + } + } + + /** + * Private. + */ + private function setQueryObjectOrderBy(Query $object, Machine $state_machine) { + $properties = $this->getStringsFromStringMachine($state_machine->gsm('order_var')); + + $direction = $this->getStringsFromStringMachine($state_machine->gsm('order_asc')); + $sortMethod = (!empty($direction)) ? "sortByAscending" : "sortByDescending"; + + foreach ($properties as $property) { + $object->$sortMethod($property); + } + } + + /** + * Private. + */ + private function setQueryObjectLimit(Query $object, Machine $state_machine) { + $rows_limit = $this->configFactory->get('dkan_sql_endpoint.settings')->get('rows_limit'); + + $limit = $this->getStringsFromStringMachine($state_machine->gsm('numeric1')); + if (!empty($limit) && $limit[0] <= $rows_limit) { + $object->limitTo($limit[0]); + } + elseif ($object->count == FALSE) { + $object->limitTo($rows_limit); + } + + $offset = $this->getStringsFromStringMachine($state_machine->gsm('numeric2')); + if (!empty($offset)) { + $object->offsetBy($offset[0]); + } + } + + /** + * Private. + */ + private function getStringsFromStringMachine(Machine $machine): array { + return (new GetStringsFromStateMachineExecution($machine->execution))->get(); + } + +} diff --git a/modules/custom/dkan_sql_endpoint/tests/src/Unit/Controller/ApiTest.php b/modules/custom/dkan_sql_endpoint/tests/src/Unit/Controller/ApiTest.php index 1c46add78..231c7ebd7 100644 --- a/modules/custom/dkan_sql_endpoint/tests/src/Unit/Controller/ApiTest.php +++ b/modules/custom/dkan_sql_endpoint/tests/src/Unit/Controller/ApiTest.php @@ -5,6 +5,7 @@ use Dkan\Datastore\Resource; use Drupal\Core\Config\Config; use Drupal\Core\Config\ConfigFactory; +use Drupal\Core\Config\ImmutableConfig; use Drupal\Core\Database\Connection; use Drupal\Core\DependencyInjection\Container; use Drupal\dkan_common\Tests\Mock\Chain; @@ -12,6 +13,7 @@ use Drupal\dkan_datastore\Storage\DatabaseTable; use Drupal\dkan_datastore\Storage\DatabaseTableFactory; use Drupal\dkan_sql_endpoint\Controller\Api; +use Drupal\dkan_sql_endpoint\Service; use PHPUnit\Framework\TestCase; use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpFoundation\RequestStack; @@ -46,10 +48,16 @@ public function test2() { * */ private function getContainer() { + $container = (new Chain($this)) + ->add(Container::class, "get", ConfigFactory::class) + ->add(ConfigFactory::class, "get", ImmutableConfig::class) + ->add(ImmutableConfig::class, "get", "100") + ->getMock(); + $options = (new Options()) + ->add('dkan_sql_endpoint.service', Service::create($container)) ->add("database", Connection::class) ->add('dkan_datastore.service.factory.resource', ResourceServiceFactory::class) - ->add('config.factory', ConfigFactory::class) ->add('request_stack', RequestStack::class) ->add('dkan_datastore.database_table_factory', DatabaseTableFactory::class); diff --git a/modules/custom/dkan_sql_endpoint/tests/src/Unit/ServiceTest.php b/modules/custom/dkan_sql_endpoint/tests/src/Unit/ServiceTest.php new file mode 100644 index 000000000..89c66b658 --- /dev/null +++ b/modules/custom/dkan_sql_endpoint/tests/src/Unit/ServiceTest.php @@ -0,0 +1,48 @@ +add(Container::class, "get", ConfigFactory::class) + ->add(ConfigFactory::class, "get", ImmutableConfig::class) + ->add(ImmutableConfig::class, "get", "100") + ->getMock(); + + $service = Service::create($container); + $query = $service->getQueryObject("[SELECT * FROM blah];"); + $this->assertTrue(isset($query->limit)); + $this->assertEquals($query->limit, 100); + } + + /** + * + */ + public function testNoAutoLimitOnCountSqlStatements() { + $container = (new Chain($this)) + ->add(Container::class, "get", ConfigFactory::class) + ->add(ConfigFactory::class, "get", ImmutableConfig::class) + ->add(ImmutableConfig::class, "get", "100") + ->getMock(); + + $service = Service::create($container); + $query = $service->getQueryObject("[SELECT COUNT(*) FROM blah];"); + $this->assertFalse(isset($query->limit)); + } + +}