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.
GetDKAN · Dec 12, 2019 · d26de9c · d26de9c
1 parent c30e947
commit d26de9c
Show file tree

Hide file tree

Showing 10 changed files with 363 additions and 185 deletions.
diff --git a/.gitignore b/.gitignore
@@ -13,4 +13,7 @@ composer.lock
 .docksal
 .idea
 docs/_build
-.vscode
+.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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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" => "*"]
-      );
-  }
-
 }