Remote cache filtered CSV downloads for one hour (#4192)

modules/common/src/CacheableResponseTrait.php

@@ -50,7 +50,8 @@ protected function setCacheMaxAge() {
     if (!isset($this->cacheMaxAge)) {
       // A hack to bypass the controllers' tests.
       if (\Drupal::hasService('config.factory')) {
-        $this->cacheMaxAge = \Drupal::config('system.performance')->get('cache.page.max_age');
+        // Use null coalesce because it's possible this config has not been set.
+        $this->cacheMaxAge = \Drupal::config('system.performance')->get('cache.page.max_age') ?? 0;
       }
       else {
         $this->cacheMaxAge = 0;

modules/common/tests/src/Kernel/CacheableResponseTraitTest.php

@@ -0,0 +1,68 @@
+<?php
+
+namespace Drupal\Tests\common\Kernel;
+
+use Drupal\KernelTests\KernelTestBase;
+use Drupal\common\CacheableResponseTrait;
+use Symfony\Component\HttpFoundation\Response;
+
+/**
+ * @coversDefaultClass \Drupal\common\CacheableResponseTrait
+ *
+ * @group dkan
+ * @group common
+ * @group kernel
+ */
+class CacheableResponseTraitTest extends KernelTestBase {
+
+  protected static $modules = [
+    'common',
+    'system',
+  ];
+
+  public function testCacheableResponse() {
+    // Check the defaults from both Response and the trait.
+    $no_cache_controller = new ControllerUsesCacheableResponseTrait();
+    $response = $no_cache_controller->traitAddCacheHeaders(new Response());
+    $this->assertStringContainsString('no-cache', $response->headers->get('Cache-Control'));
+    $this->assertStringContainsString('private', $response->headers->get('Cache-Control'));
+
+    // Set the max age in config.
+    $config_max_age = 999;
+    $this->config('system.performance')->set('cache.page.max_age', $config_max_age)->save();
+    $config_controller = new ControllerUsesCacheableResponseTrait();
+    $response = $config_controller->traitAddCacheHeaders(new Response());
+    $this->assertEquals($config_max_age, $response->getMaxAge());
+    $this->assertStringContainsString('public', $response->headers->get('Cache-Control'));
+
+    // Set the max age in the controller object. This should override the
+    // config.
+    $max_time = 23;
+    $max_time_controller = new ControllerUsesCacheableResponseTrait();
+    $max_time_controller->traitSetMaxAgeProperty($max_time);
+    $response = $max_time_controller->traitAddCacheHeaders(new Response());
+    $this->assertEquals($max_time, $response->getMaxAge());
+    $this->assertStringContainsString('public', $response->headers->get('Cache-Control'));
+  }
+
+}
+
+/**
+ * Make a stub class because it's less cumbersome than using mocking.
+ */
+class ControllerUsesCacheableResponseTrait {
+  use CacheableResponseTrait;
+
+  public function traitSetMaxAgeProperty(int $max_age) {
+    $this->cacheMaxAge = $max_age;
+  }
+
+  public function traitAddCacheHeaders(Response $response): Response {
+    return $this->addCacheHeaders($response);
+  }
+
+  public function traitSetCacheMaxAge() {
+    $this->setCacheMaxAge();
+  }
+
+}

modules/datastore/src/Controller/QueryDownloadController.php

@@ -2,18 +2,40 @@
 
 namespace Drupal\datastore\Controller;
 
+use Drupal\Core\Config\ConfigFactoryInterface;
+use Drupal\common\DatasetInfo;
 use Drupal\datastore\Service\DatastoreQuery;
+use Drupal\datastore\Service\Query as QueryService;
+use Drupal\metastore\MetastoreApiResponse;
 use RootedData\RootedJsonData;
 use Symfony\Component\HttpFoundation\ParameterBag;
 use Symfony\Component\HttpFoundation\StreamedResponse;
 
 /**
  * Controller providing functionality used to stream datastore queries.
  *
- * @package Drupal\datastore
+ * This generally supports CSV download of filtered datasets.
  */
 class QueryDownloadController extends AbstractQueryController {
 
+  /**
+   * Max-age cache control header value in seconds.
+   */
+  private const RESPONSE_STREAM_MAX_AGE = 3600;
+
+  /**
+   * {@inheritDoc}
+   */
+  public function __construct(QueryService $queryService, DatasetInfo $datasetInfo, MetastoreApiResponse $metastoreApiResponse, ConfigFactoryInterface $configFactory) {
+    parent::__construct($queryService, $datasetInfo, $metastoreApiResponse, $configFactory);
+    // We do not want to cache streaming CSV content internally in Drupal,
+    // because datasets can be very large. However, we do want CDNs to be able
+    // to cache the CSV stream for a reasonable amount of time.
+    // @todo Replace this constant with some form of customizable caching
+    //   strategy.
+    $this->cacheMaxAge = static::RESPONSE_STREAM_MAX_AGE;
+  }
+
   /**
    * {@inheritdoc}
    */
@@ -89,15 +111,16 @@ protected function streamCsvResponse(DatastoreQuery $datastoreQuery, RootedJsonD
   /**
    * Create initial streamed response object.
    *
-   * @return Symfony\Component\HttpFoundation\StreamedResponse
+   * @return \Symfony\Component\HttpFoundation\StreamedResponse
    *   A streamed response object set up for data.csv file.
    */
   private function initStreamedCsvResponse($filename = "data.csv") {
     $response = new StreamedResponse();
     $response->headers->set('Content-Type', 'text/csv');
     $response->headers->set('Content-Disposition', "attachment; filename=\"$filename\"");
     $response->headers->set('X-Accel-Buffering', 'no');
-    return $response;
+    // Ensure one hour max-age plus public status.
+    return $this->addCacheHeaders($response);
   }
 
   /**

modules/datastore/tests/src/Functional/DatastoreServiceTest.php

@@ -20,6 +20,7 @@
  *
  * @group datastore
  * @group btb
+ * @group functional
  */
 class DatastoreServiceTest extends BrowserTestBase {
 

modules/datastore/tests/src/Functional/Service/ResourcePurgerTest.php

@@ -11,8 +11,9 @@
 /**
  * Test ResourcePurger service.
  *
- * @package Drupal\Tests\datastore\Functional
+ * @group dkan
  * @group datastore
+ * @group functional
  */
 class ResourcePurgerTest extends ExistingSiteBase {
   use GetDataTrait;

modules/datastore/tests/src/Unit/Controller/AbstractQueryControllerTest.php

@@ -7,7 +7,9 @@
 use Symfony\Component\HttpFoundation\Request;
 
 /**
- *
+ * @group dkan
+ * @group datastore
+ * @group unit
  */
 class AbstractQueryControllerTest extends TestCase {
 

modules/datastore/tests/src/Unit/Controller/QueryDownloadControllerTest.php

@@ -6,8 +6,6 @@
 use Drupal\Core\Cache\Context\CacheContextsManager;
 use Drupal\Core\Config\ConfigFactoryInterface;
 use Drupal\Core\Config\ImmutableConfig;
-use Drupal\Core\Database\Query\Select;
-use Drupal\Tests\common\Unit\Connection;
 use Drupal\common\DatasetInfo;
 use Drupal\datastore\Controller\QueryController;
 use Drupal\datastore\Controller\QueryDownloadController;
@@ -236,14 +234,20 @@ public function testStreamedLimit() {
     $downloadController = QueryDownloadController::create($container);
     $request = $this->mockRequest($data);
     ob_start([self::class, 'getBuffer']);
+    /** @var \Symfony\Component\HttpFoundation\StreamedResponse $streamResponse */
     $streamResponse = $downloadController->query($request);
     $this->assertEquals(200, $streamResponse->getStatusCode());
     $streamResponse->sendContent();
     ob_get_clean();
     $streamedCsv = $this->buffer;
     // Check that the CSV has the full queryLimit number of lines, plus header and final newline.
     $this->assertEquals(($queryLimit + 2), count(explode("\n", $streamedCsv)));
-
+    // Check that the max-age header is correct.
+    $this->assertEquals(3600, $streamResponse->getMaxAge());
+    $this->assertStringContainsString(
+      'public',
+      $streamResponse->headers->get('cache-control') ?? ''
+    );
   }
 
   /**

modules/datastore/tests/src/Unit/DataDictionary/AlterTableQuery/BuilderTest.php

@@ -14,6 +14,11 @@
 use PDLT\ConverterInterface;
 use PHPUnit\Framework\TestCase;
 
+/**
+ * @group dkan
+ * @group datastore
+ * @group unit
+ */
 class TestQuery extends AlterTableQueryBase {
   public function getTable(): string {
     return $this->table;

modules/datastore/tests/src/Unit/DataDictionary/AlterTableQuery/MySQLQueryTest.php

@@ -19,7 +19,11 @@
 /**
  * Unit tests for Drupal\datastore\DataDictionary\AlterTableQuery\MySQLQuery.
  *
- * @coversDefaultClass Drupal\datastore\DataDictionary\AlterTableQuery\MySQLQuery
+ * @coversDefaultClass \Drupal\datastore\DataDictionary\AlterTableQuery\MySQLQuery
+ *
+ * @group dkan
+ * @group datastore
+ * @group unit
  */
 class MySQLQueryTest extends TestCase {
 

modules/datastore/tests/src/Unit/DatastoreServiceTest.php

@@ -28,6 +28,10 @@
 /**
  * @covers \Drupal\datastore\DatastoreService
  * @coversDefaultClass \Drupal\datastore\DatastoreService
+ *
+ * @group dkan
+ * @group datastore
+ * @group unit
  */
 class DatastoreServiceTest extends TestCase {
 

modules/datastore/tests/src/Unit/Form/DashboardFormTest.php

@@ -24,6 +24,11 @@
 use Symfony\Component\HttpFoundation\Request;
 use Symfony\Component\HttpFoundation\RequestStack;
 
+/**
+ * @group dkan
+ * @group datastore
+ * @group unit
+ */
 class DashboardFormTest extends TestCase {
 
   /**

modules/datastore/tests/src/Unit/Plugin/QueueWorker/ImportJobTest.php

@@ -17,8 +17,10 @@
  * @covers \Drupal\datastore\Plugin\QueueWorker\ImportJob
  * @coversDefaultClass \Drupal\datastore\Plugin\QueueWorker\ImportJob
  *
- * @group datastore
+ * @group dkan
  * @group dkan-core
+ * @group datastore
+ * @group unit
  */
 class ImportJobTest extends TestCase {
 

modules/datastore/tests/src/Unit/Plugin/QueueWorker/ResourcePurgerWorkerTest.php

@@ -9,6 +9,11 @@
 use MockChain\Options;
 use PHPUnit\Framework\TestCase;
 
+/**
+ * @group dkan
+ * @group datastore
+ * @group unit
+ */
 class ResourcePurgerWorkerTest extends TestCase {
 
   public function test() {

modules/datastore/tests/src/Unit/Service/DatastoreQueryTest.php

@@ -30,6 +30,8 @@
 
 /**
  * @group dkan
+ * @group datastore
+ * @group unit
  */
 class DatastoreQueryTest extends TestCase {
   use TestHelperTrait;

modules/datastore/tests/src/Unit/Service/Info/ImportInfoTest.php

@@ -23,8 +23,10 @@
 /**
  * @coversDefaultClass \Drupal\datastore\Service\Info\ImportInfo
  *
- * @group datastore
+ * @group dkan
  * @group dkan-core
+ * @group datastore
+ * @group unit
  */
 class ImportInfoTest extends TestCase {
 

modules/datastore/tests/src/Unit/Service/PostImportTest.php

@@ -10,7 +10,9 @@
 /**
  * Tests the PostImport service.
  *
+ * @group dkan
  * @group datastore
+ * @group unit
  */
 class PostImportTest extends TestCase {
 

modules/datastore/tests/src/Unit/Service/ResourceLocalizerTest.php

@@ -26,6 +26,7 @@
 /**
  * @group dkan
  * @group datastore
+ * @group unit
  */
 class ResourceLocalizerTest extends TestCase {
   /**

modules/datastore/tests/src/Unit/SqlEndpoint/WebServiceApiTest.php

@@ -23,8 +23,11 @@
 use Symfony\Component\HttpFoundation\RequestStack;
 
 /**
- * @coversDefaultClass \Drupal\datastore\WebServiceApi
+ * @coversDefaultClass \Drupal\datastore\SqlEndpoint\WebServiceApi
+ *
  * @group dkan
+ * @group datastore
+ * @group unit
  */
 class WebServiceApiTest extends TestCase {
   use TestHelperTrait;

modules/datastore/tests/src/Unit/SqlParser/SqlParserTest.php

@@ -9,6 +9,7 @@
  * @group dkan
  * @group datastore
  * @group sqlparser
+ * @group unit
  *
  * @covers \Drupal\datastore\SqlParser\SqlParser
  * @coversDefaultClass \Drupal\datastore\SqlParser\SqlParser

modules/frontend/tests/src/Unit/Controller/ControllerPageTest.php

@@ -1,14 +1,20 @@
 <?php
 
-use \Drupal\Core\Config\ConfigFactory;
+namespace Drupal\Tests\frontend\Unit\Controller;
+
+use Drupal\Core\Config\ConfigFactory;
 use Drupal\Core\Config\ImmutableConfig;
 use Drupal\frontend\Controller\Page as PageController;
 use Drupal\frontend\Page;
 use PHPUnit\Framework\TestCase;
 use Symfony\Component\DependencyInjection\ContainerInterface;
 
 /**
- * @coversDefaultClass Drupal\frontend\Controller\Page
+ * @coversDefaultClass \Drupal\frontend\Controller\Page
+ *
+ * @group dkan
+ * @group frontend
+ * @group unit
  */
 class ControllerPageTest extends TestCase {