Unverified Commit a6bb8521 authored by alexpott's avatar alexpott
Browse files

Issue #3190815 by Wim Leers, quietone, NickDickinsonWilde, benjifisher,...

Issue #3190815 by Wim Leers, quietone, NickDickinsonWilde, benjifisher, mikelutz, larowlan: Source count caching broken: impossible to enable source count caching for SqlBase-based source plugins (plus, unneeded cache I/O)

(cherry picked from commit a6124cc0)
parent 4bc7f218
<?php
namespace Drupal\Core\Cache;
class MemoryCounterBackendFactory implements CacheFactoryInterface {
/**
* Instantiated memory cache bins.
*
* @var \Drupal\Core\Cache\MemoryBackend[]
*/
protected $bins = [];
/**
* {@inheritdoc}
*/
public function get($bin) {
if (!isset($this->bins[$bin])) {
$this->bins[$bin] = new MemoryCounterBackend();
}
return $this->bins[$bin];
}
}
<?php
/**
* @file
* Post update functions for migrate.
*/
/**
* Clear the source count cache.
*/
function migrate_post_update_clear_migrate_source_count_cache() {
// Empty post_update hook.
}
......@@ -2,6 +2,7 @@
namespace Drupal\migrate\Plugin\migrate\source;
use Drupal\Component\Serialization\Json;
use Drupal\Core\Plugin\PluginBase;
use Drupal\migrate\Event\MigrateRollbackEvent;
use Drupal\migrate\Event\RollbackAwareInterface;
......@@ -246,7 +247,9 @@ public function __construct(array $configuration, $plugin_id, $plugin_definition
$this->$property = (bool) $configuration[$config_key];
}
}
$this->cacheKey = !empty($configuration['cache_key']) ? $configuration['cache_key'] : NULL;
if ($this->cacheCounts) {
$this->cacheKey = $configuration['cache_key'] ?? $plugin_id . '-' . hash('sha256', Json::encode($configuration));
}
$this->idMap = $this->migration->getIdMap();
$this->highWaterProperty = !empty($configuration['high_water_property']) ? $configuration['high_water_property'] : FALSE;
......@@ -265,7 +268,7 @@ public function __construct(array $configuration, $plugin_id, $plugin_definition
* Initializes the iterator with the source data.
*
* @return \Iterator
* Returns an iteratable object of data for this source.
* Returns an iterable object of data for this source.
*/
abstract protected function initializeIterator();
......@@ -486,30 +489,19 @@ public function count($refresh = FALSE) {
return -1;
}
if (!isset($this->cacheKey)) {
$this->cacheKey = hash('sha256', $this->getPluginId());
}
// If a refresh is requested, or we're not caching counts, ask the derived
// class to get the count from the source.
if ($refresh || !$this->cacheCounts) {
$count = $this->doCount();
$this->getCache()->set($this->cacheKey, $count);
}
else {
// Caching is in play, first try to retrieve a cached count.
// Return the cached count if we are caching counts and a refresh is not
// requested.
if ($this->cacheCounts && !$refresh) {
$cache_object = $this->getCache()->get($this->cacheKey, 'cache');
if (is_object($cache_object)) {
// Success.
$count = $cache_object->data;
}
else {
// No cached count, ask the derived class to count 'em up, and cache
// the result.
$count = $this->doCount();
$this->getCache()->set($this->cacheKey, $count);
return $cache_object->data;
}
}
$count = $this->doCount();
// Update the cache if we are caching counts.
if ($this->cacheCounts) {
$this->getCache()->set($this->cacheKey, $count);
}
return $count;
}
......
......@@ -383,9 +383,9 @@ protected function fetchNextBatch() {
abstract public function query();
/**
* {@inheritdoc}
* Gets the source count using countQuery().
*/
public function count($refresh = FALSE) {
protected function doCount() {
return (int) $this->query()->countQuery()->execute()->fetchField();
}
......
name: Cacheable Embedded Data Test
type: module
description: Module containing a cacheable embedded data source.
package: Testing
version: VERSION
<?php
namespace Drupal\migrate_cache_counts_test\Plugin\migrate\source;
use Drupal\migrate\Plugin\migrate\source\EmbeddedDataSource;
use Drupal\migrate\Plugin\migrate\source\SourcePluginBase;
/**
* A copy of embedded_data which allows caching the count.
*
* @MigrateSource(
* id = "cacheable_embedded_data",
* source_module = "migrate"
* )
*/
class CacheableEmbeddedDataSource extends EmbeddedDataSource {
/**
* {@inheritdoc}
*/
public function count($refresh = FALSE) {
return SourcePluginBase::count($refresh);
}
/**
* {@inheritdoc}
*/
protected function doCount() {
return parent::count(TRUE);
}
}
type: module
name: Migrate SqlBase count cache test
description: Provides a source plugin to test that counts are cached in SQL sources.
package: Testing
<?php
namespace Drupal\migrate_sql_count_cache_test\Plugin\migrate\source;
use Drupal\migrate\Plugin\migrate\source\SqlBase;
/**
* Source plugin for Sql count cache test.
*
* @MigrateSource(
* id = "sql_count_cache"
* )
*/
class SqlCountCache extends SqlBase {
/**
* {@inheritdoc}
*/
public function fields() {
return [
'id' => t('Id'),
];
}
/**
* {@inheritdoc}
*/
public function getIds() {
return [
'id' => [
'type' => 'integer',
],
];
}
/**
* {@inheritdoc}
*/
public function query() {
return $this->select('source_table', 's')->fields('s', ['id']);
}
}
......@@ -2,13 +2,23 @@
namespace Drupal\Tests\migrate\Kernel;
use Drupal\Core\Cache\MemoryCounterBackendFactory;
use Drupal\Core\Database\Driver\sqlite\Connection;
use Drupal\Core\DependencyInjection\ContainerBuilder;
/**
* Base class for tests of Migrate source plugins that use a database.
*/
abstract class MigrateSqlSourceTestBase extends MigrateSourceTestBase {
/**
* {@inheritdoc}
*/
public function register(ContainerBuilder $container) {
parent::register($container);
$container->register('cache_factory', MemoryCounterBackendFactory::class);
}
/**
* Builds an in-memory SQLite database from a set of source data.
*
......@@ -67,12 +77,14 @@ protected function getDatabase(array $source_data) {
* (optional) Configuration for the source plugin.
* @param mixed $high_water
* (optional) The value of the high water field.
* @param string|null $expected_cache_key
* (optional) The expected cache key.
*
* @dataProvider providerSource
*
* @requires extension pdo_sqlite
*/
public function testSource(array $source_data, array $expected_data, $expected_count = NULL, array $configuration = [], $high_water = NULL) {
public function testSource(array $source_data, array $expected_data, $expected_count = NULL, array $configuration = [], $high_water = NULL, $expected_cache_key = NULL) {
$plugin = $this->getPlugin($configuration);
// Since we don't yet inject the database connection, we need to use a
......@@ -82,6 +94,33 @@ public function testSource(array $source_data, array $expected_data, $expected_c
$property->setAccessible(TRUE);
$property->setValue($plugin, $this->getDatabase($source_data));
/** @var MemoryCounterBackend $cache **/
$cache = \Drupal::cache('migrate');
if ($expected_cache_key) {
// Verify the the computed cache key.
$property = $reflector->getProperty('cacheKey');
$property->setAccessible(TRUE);
$this->assertSame($expected_cache_key, $property->getValue($plugin));
// Cache miss prior to calling ::count().
$this->assertFalse($cache->get($expected_cache_key, 'cache'));
$this->assertSame([], $cache->getCounter('set'));
$count = $plugin->count();
$this->assertSame($expected_count, $count);
$this->assertSame([$expected_cache_key => 1], $cache->getCounter('set'));
// Cache hit afterwards.
$cache_item = $cache->get($expected_cache_key, 'cache');
$this->assertNotSame(FALSE, $cache_item, 'This is not a cache hit.');
$this->assertSame($expected_count, $cache_item->data);
}
else {
$this->assertSame([], $cache->getCounter('set'));
$plugin->count();
$this->assertSame([], $cache->getCounter('set'));
}
parent::testSource($source_data, $expected_data, $expected_count, $configuration, $high_water);
}
......
<?php
namespace Drupal\Tests\migrate\Kernel\Plugin\source;
use Drupal\Tests\migrate\Kernel\MigrateSqlSourceTestBase;
/**
* Tests SqlBase source count caching.
*
* @covers \Drupal\migrate_sql_count_cache_test\Plugin\migrate\source\SqlCountCache
* @covers \Drupal\migrate\Plugin\migrate\source\SqlBase::doCount
* @covers \Drupal\migrate\Plugin\migrate\source\SourcePluginBase::count
*
* @group migrate
*/
class MigrateSqlSourceCountCacheTest extends MigrateSqlSourceTestBase {
/**
* {@inheritdoc}
*/
protected static $modules = ['migrate_sql_count_cache_test'];
/**
* {@inheritdoc}
*/
public function providerSource() {
// All tests use the same source_data, expected_data, expected_count, and
// high_water. The high water is set later to maintain the order of the
// parameters.
$data = [
'source_data' => [
'source_table' => [
['id' => 1],
['id' => 2],
['id' => 3],
['id' => 4],
],
],
'expected_data' => [
['id' => 1],
['id' => 2],
['id' => 3],
['id' => 4],
],
'expected_count' => 4,
];
return [
'uncached source count' => $data,
'cached source count, auto-generated cache key' => $data + [
'configuration' => [
'cache_counts' => TRUE,
],
'high_water' => NULL,
'expected_cache_key' => 'sql_count_cache-dbed2396c230e025663091479993a206441bf1f9ae4e60ebf3b504e4a76ad471',
],
'cached source count, auto-generated cache key for alternative source configuration' => $data + [
'configuration' => [
'cache_counts' => TRUE,
'some_source_plugin_configuration_key' => 19920106,
],
'high_water' => NULL,
'expected_cache_key' => 'sql_count_cache-83c62856dd5afc011f32574bcdc11c595557d629e1d73045e9353df2441ec269',
],
'cached source count, provided cache key' => $data + [
'configuration' => [
'cache_counts' => TRUE,
'cache_key' => 'custom_cache_key_here',
],
'high_water' => NULL,
'expected_cache_key' => 'custom_cache_key_here',
],
];
}
}
<?php
namespace Drupal\Tests\migrate\Kernel\Plugin\source;
use Drupal\migrate\Plugin\migrate\source\SourcePluginBase;
use Drupal\Tests\migrate\Kernel\MigrateTestBase;
/**
* Test source counts are correctly cached.
*
* @group migrate
*/
class MigrationSourceCacheTest extends MigrateTestBase {
/**
* {@inheritdoc}
*/
protected static $modules = ['migrate_cache_counts_test'];
/**
* The migration plugin manager.
*
* @var \Drupal\migrate\Plugin\MigrationPluginManagerInterface
*/
protected $migrationPluginManager;
/**
* {@inheritdoc}
*/
protected function setUp(): void {
parent::setUp();
$this->migrationPluginManager = $this->container->get('plugin.manager.migration');
}
/**
* Tests that counts for the same plugin_id are not crossed.
*/
public function testCacheCountsNotContaminated() {
$migration_1_definition = [
'source' => [
'plugin' => 'cacheable_embedded_data',
'cache_counts' => TRUE,
'ids' => [
'id' => [
'type' => 'integer',
],
],
'data_rows' => [
[
['id' => 1],
],
],
],
];
$migration_2_definition = [
'source' => [
'plugin' => 'cacheable_embedded_data',
'cache_counts' => TRUE,
'ids' => [
'id' => [
'type' => 'integer',
],
],
'data_rows' => [
['id' => 1],
['id' => 2],
],
],
];
$migration_1 = $this->migrationPluginManager->createStubMigration($migration_1_definition);
$migration_2 = $this->migrationPluginManager->createStubMigration($migration_2_definition);
$migration_1_source = $migration_1->getSourcePlugin();
$migration_2_source = $migration_2->getSourcePlugin();
// Verify correct counts when count is refreshed.
$this->assertSame(1, $migration_1_source->count(TRUE));
$this->assertSame(2, $migration_2_source->count(TRUE));
// Verify correct counts are cached.
$this->assertSame(1, $migration_1_source->count());
$this->assertSame(2, $migration_2_source->count());
// Verify the cache keys are different.
$cache_key_property = new \ReflectionProperty(SourcePluginBase::class, 'cacheKey');
$cache_key_property->setAccessible(TRUE);
$this->assertNotEquals($cache_key_property->getValue($migration_1_source), $cache_key_property->getValue($migration_2_source));
}
/**
* Test that values are pulled from the cache when appropriate.
*/
public function testCacheCountsUsed() {
$migration_definition = [
'source' => [
'plugin' => 'cacheable_embedded_data',
'cache_counts' => TRUE,
'ids' => [
'id' => [
'type' => 'integer',
],
],
'data_rows' => [
['id' => 1],
['id' => 2],
],
],
];
$migration = $this->migrationPluginManager->createStubMigration($migration_definition);
$migration_source = $migration->getSourcePlugin();
$this->assertSame(2, $migration_source->count());
// Pollute the cache.
$cache_key_property = new \ReflectionProperty($migration_source, 'cacheKey');
$cache_key_property->setAccessible(TRUE);
$cache_key = $cache_key_property->getValue($migration_source);
\Drupal::cache('migrate')->set($cache_key, 7);
$this->assertSame(7, $migration_source->count());
$this->assertSame(2, $migration_source->count(TRUE));
}
}
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment