From 4193da7c711d79d257a73c416af62316285a76f3 Mon Sep 17 00:00:00 2001 From: Paul Mitchum Date: Wed, 26 Jun 2024 13:55:51 -0700 Subject: [PATCH] Remote cache filtered CSV downloads for one hour (#4192) --- modules/common/src/CacheableResponseTrait.php | 3 +- .../src/Kernel/CacheableResponseTraitTest.php | 68 +++++++++++++++++++ .../Controller/QueryDownloadController.php | 29 +++++++- .../src/Functional/DatastoreServiceTest.php | 1 + .../Functional/Service/ResourcePurgerTest.php | 3 +- .../AbstractQueryControllerTest.php | 4 +- .../QueryDownloadControllerTest.php | 10 ++- .../AlterTableQuery/BuilderTest.php | 5 ++ .../AlterTableQuery/MySQLQueryTest.php | 6 +- .../tests/src/Unit/DatastoreServiceTest.php | 4 ++ .../tests/src/Unit/Form/DashboardFormTest.php | 5 ++ .../Unit/Plugin/QueueWorker/ImportJobTest.php | 4 +- .../QueueWorker/ResourcePurgerWorkerTest.php | 5 ++ .../src/Unit/Service/DatastoreQueryTest.php | 2 + .../src/Unit/Service/Info/ImportInfoTest.php | 4 +- .../tests/src/Unit/Service/PostImportTest.php | 2 + .../Unit/Service/ResourceLocalizerTest.php | 1 + .../Unit/SqlEndpoint/WebServiceApiTest.php | 5 +- .../src/Unit/SqlParser/SqlParserTest.php | 1 + .../Unit/Controller/ControllerPageTest.php | 10 ++- 20 files changed, 157 insertions(+), 15 deletions(-) create mode 100644 modules/common/tests/src/Kernel/CacheableResponseTraitTest.php diff --git a/modules/common/src/CacheableResponseTrait.php b/modules/common/src/CacheableResponseTrait.php index cecbf30d24..4428ecfad3 100644 --- a/modules/common/src/CacheableResponseTrait.php +++ b/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; diff --git a/modules/common/tests/src/Kernel/CacheableResponseTraitTest.php b/modules/common/tests/src/Kernel/CacheableResponseTraitTest.php new file mode 100644 index 0000000000..fa94fa647e --- /dev/null +++ b/modules/common/tests/src/Kernel/CacheableResponseTraitTest.php @@ -0,0 +1,68 @@ +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(); + } + +} diff --git a/modules/datastore/src/Controller/QueryDownloadController.php b/modules/datastore/src/Controller/QueryDownloadController.php index 5b28cf83f2..d80fa0521a 100644 --- a/modules/datastore/src/Controller/QueryDownloadController.php +++ b/modules/datastore/src/Controller/QueryDownloadController.php @@ -2,7 +2,11 @@ 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; @@ -10,10 +14,28 @@ /** * 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,7 +111,7 @@ 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") { @@ -97,7 +119,8 @@ private function initStreamedCsvResponse($filename = "data.csv") { $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); } /** diff --git a/modules/datastore/tests/src/Functional/DatastoreServiceTest.php b/modules/datastore/tests/src/Functional/DatastoreServiceTest.php index 2b037e7094..22cf6163cb 100644 --- a/modules/datastore/tests/src/Functional/DatastoreServiceTest.php +++ b/modules/datastore/tests/src/Functional/DatastoreServiceTest.php @@ -20,6 +20,7 @@ * * @group datastore * @group btb + * @group functional */ class DatastoreServiceTest extends BrowserTestBase { diff --git a/modules/datastore/tests/src/Functional/Service/ResourcePurgerTest.php b/modules/datastore/tests/src/Functional/Service/ResourcePurgerTest.php index fe4eb1480f..a4cd8242f1 100644 --- a/modules/datastore/tests/src/Functional/Service/ResourcePurgerTest.php +++ b/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; diff --git a/modules/datastore/tests/src/Unit/Controller/AbstractQueryControllerTest.php b/modules/datastore/tests/src/Unit/Controller/AbstractQueryControllerTest.php index 8b8c0f2739..f8126034a9 100644 --- a/modules/datastore/tests/src/Unit/Controller/AbstractQueryControllerTest.php +++ b/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 { diff --git a/modules/datastore/tests/src/Unit/Controller/QueryDownloadControllerTest.php b/modules/datastore/tests/src/Unit/Controller/QueryDownloadControllerTest.php index fa77b2cf49..d95534078f 100644 --- a/modules/datastore/tests/src/Unit/Controller/QueryDownloadControllerTest.php +++ b/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,6 +234,7 @@ 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(); @@ -243,7 +242,12 @@ public function testStreamedLimit() { $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') ?? '' + ); } /** diff --git a/modules/datastore/tests/src/Unit/DataDictionary/AlterTableQuery/BuilderTest.php b/modules/datastore/tests/src/Unit/DataDictionary/AlterTableQuery/BuilderTest.php index 0f9e36e12b..0de96e628e 100644 --- a/modules/datastore/tests/src/Unit/DataDictionary/AlterTableQuery/BuilderTest.php +++ b/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; diff --git a/modules/datastore/tests/src/Unit/DataDictionary/AlterTableQuery/MySQLQueryTest.php b/modules/datastore/tests/src/Unit/DataDictionary/AlterTableQuery/MySQLQueryTest.php index 46e4dcf4a0..bc2a795fc1 100644 --- a/modules/datastore/tests/src/Unit/DataDictionary/AlterTableQuery/MySQLQueryTest.php +++ b/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 { diff --git a/modules/datastore/tests/src/Unit/DatastoreServiceTest.php b/modules/datastore/tests/src/Unit/DatastoreServiceTest.php index d68ef70daa..f2a3bbaf24 100644 --- a/modules/datastore/tests/src/Unit/DatastoreServiceTest.php +++ b/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 { diff --git a/modules/datastore/tests/src/Unit/Form/DashboardFormTest.php b/modules/datastore/tests/src/Unit/Form/DashboardFormTest.php index 2c8ae216a3..8aa724886e 100644 --- a/modules/datastore/tests/src/Unit/Form/DashboardFormTest.php +++ b/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 { /** diff --git a/modules/datastore/tests/src/Unit/Plugin/QueueWorker/ImportJobTest.php b/modules/datastore/tests/src/Unit/Plugin/QueueWorker/ImportJobTest.php index 7aadfaf48d..4e2e77e65c 100644 --- a/modules/datastore/tests/src/Unit/Plugin/QueueWorker/ImportJobTest.php +++ b/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 { diff --git a/modules/datastore/tests/src/Unit/Plugin/QueueWorker/ResourcePurgerWorkerTest.php b/modules/datastore/tests/src/Unit/Plugin/QueueWorker/ResourcePurgerWorkerTest.php index dd936cf980..41c4dafd24 100644 --- a/modules/datastore/tests/src/Unit/Plugin/QueueWorker/ResourcePurgerWorkerTest.php +++ b/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() { diff --git a/modules/datastore/tests/src/Unit/Service/DatastoreQueryTest.php b/modules/datastore/tests/src/Unit/Service/DatastoreQueryTest.php index 77cfbdc2aa..850f59419d 100644 --- a/modules/datastore/tests/src/Unit/Service/DatastoreQueryTest.php +++ b/modules/datastore/tests/src/Unit/Service/DatastoreQueryTest.php @@ -30,6 +30,8 @@ /** * @group dkan + * @group datastore + * @group unit */ class DatastoreQueryTest extends TestCase { use TestHelperTrait; diff --git a/modules/datastore/tests/src/Unit/Service/Info/ImportInfoTest.php b/modules/datastore/tests/src/Unit/Service/Info/ImportInfoTest.php index 4ddf9ecb9f..a9d10c63f6 100644 --- a/modules/datastore/tests/src/Unit/Service/Info/ImportInfoTest.php +++ b/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 { diff --git a/modules/datastore/tests/src/Unit/Service/PostImportTest.php b/modules/datastore/tests/src/Unit/Service/PostImportTest.php index c65f6fabcc..6281387e10 100644 --- a/modules/datastore/tests/src/Unit/Service/PostImportTest.php +++ b/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 { diff --git a/modules/datastore/tests/src/Unit/Service/ResourceLocalizerTest.php b/modules/datastore/tests/src/Unit/Service/ResourceLocalizerTest.php index c9c34cf1fb..46cbb0b064 100644 --- a/modules/datastore/tests/src/Unit/Service/ResourceLocalizerTest.php +++ b/modules/datastore/tests/src/Unit/Service/ResourceLocalizerTest.php @@ -26,6 +26,7 @@ /** * @group dkan * @group datastore + * @group unit */ class ResourceLocalizerTest extends TestCase { /** diff --git a/modules/datastore/tests/src/Unit/SqlEndpoint/WebServiceApiTest.php b/modules/datastore/tests/src/Unit/SqlEndpoint/WebServiceApiTest.php index 2e4b560e17..8e15f6117a 100644 --- a/modules/datastore/tests/src/Unit/SqlEndpoint/WebServiceApiTest.php +++ b/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; diff --git a/modules/datastore/tests/src/Unit/SqlParser/SqlParserTest.php b/modules/datastore/tests/src/Unit/SqlParser/SqlParserTest.php index c13e384aa0..9322d48435 100644 --- a/modules/datastore/tests/src/Unit/SqlParser/SqlParserTest.php +++ b/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 diff --git a/modules/frontend/tests/src/Unit/Controller/ControllerPageTest.php b/modules/frontend/tests/src/Unit/Controller/ControllerPageTest.php index 2e4dbb7170..c4756437c9 100644 --- a/modules/frontend/tests/src/Unit/Controller/ControllerPageTest.php +++ b/modules/frontend/tests/src/Unit/Controller/ControllerPageTest.php @@ -1,6 +1,8 @@