Source count caching broken: impossible to enable source count caching for SqlBase-based source plugins (plus, unneeded cache I/O)

1. Why is this issue marked Major?

2. This override needs a doc block explaining what it actually does:

    +++ b/core/modules/migrate/src/Plugin/migrate/source/SqlBase.php
    @@ -385,7 +385,7 @@ protected function fetchNextBatch() {
       /**
        * {@inheritdoc}
        */
    -  public function count($refresh = FALSE) {
    +  protected function doCount() {
         return (int) $this->query()->countQuery()->execute()->fetchField();
       }

3. Update the comment:

    +++ b/core/modules/migrate/src/Plugin/migrate/source/SourcePluginBase.php
    @@ -450,13 +450,17 @@ public function count($refresh = FALSE) {
    ...
         // 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) {
    +    if ($refresh) {
           $count = $this->doCount();
           $this->getCache()->set($this->cacheKey, $count);
         }

4. It may be out of scope for this issue, but I would like to clean this up:

        if ($refresh) {
          $count = $this->doCount();
          $this->getCache()->set($this->cacheKey, $count);
        }
        else {
          // Caching is in play, first try to retrieve a cached count.
          $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 $count;
      }

   We can avoid a couple of lines of duplicated code:

        if (!$refresh) {
          // ...
          $cache_object = ...
          if (...) {
            // ...
            return $cache_object->data;
          }
        }
   
        // No cached count, or refresh requested. Ask the derived class to count
        // 'em up, and cache the result.
        $count = $this->doCount();
        $this->getCache()->set($this->cacheKey, $count);
        return $count;

5. It is OK to expand the issue scope as in #3, but we need to update the "Proposed resolution" in the issue summary, and maybe even update the title. I am adding the tag for an IS update.

6. A bugfix like this needs an automated test, and I like to see a test-only patch.

Some of these are Novice tasks, so I am adding that tag.

8.1: Thanks for the explanation.

8.2: Currently, the API docs for SqlBase::count() say,

Gets the source count.

Return a count of available source records, from the cache if appropriate. Returns -1 if the source is not countable.

I think that is accurate. (Maybe not the part about "from the cache", but this issue will fix that.) After the current patch, the API docs for SqlBase::doCount() would read,

Gets the source count checking if the source is countable or using the iterator_count function.

and that is not accurate.

8.3: Obsolete since you decided to do 8.4.

8.4: The change looks good. I am surprised it saved only one line. We could save two more by removing the else after if (...) { return ... }. That is the usual style in Drupal, even though many good programmers hate it.

Back to NW for 8.2 and 8.6.

That issue was blocked on #3092227: SourcePluginBase should cache based on a hash of the source configuration, not just the plugin id. — which appears to be a duplicate of #2751829: Default source plugin cache key is insufficiently unique 😬. I disagree that this functionality/bug fix needs to be blocked on that, because if you're not specifying cache_counts: true in your migration definition, then none of this kicks in anyway. If this patch lands, you'll need to specify cache_key too if you do not want to run into cache collisions. But at least that's better than not being able to turn this on at all and potentially getting blocked on bad source count performance (like I am).

The reason this and/or #2598670: Source count caching does not work for SQL sources needs to be blocked on #3092227: SourcePluginBase should cache based on a hash of the source configuration, not just the plugin id. is because if you have an existing set of migrations using the same sql plugin but different configurations, and you've set `cache_counts: true` thinking it was going to help you cache counts and give you a performance boost, then due to the combination of the two bugs, your counts at least work, despite not actually being cached.

If you fix this here without fixing the default cache_key hash, then you are breaking counts on those previously working migrations, which isn't acceptable.

LGTM. I haven't tested it, but at a glance the approach gets a +1 from me, along with fixing the cache_key here.

Looks like a good chunk of my code made it into this patch, so I'll leave the RTBC to others.

@Wim Leers, @mikelutz:

My first thought would be to keep the scope narrow and fix one problem at a time, starting with the older issue. But if you both agree that we should do it all here, then I will go along and see what the core commiters think.

1. What are the guidelines for adding a word to the dictionary instead of skipping spell check on a particular line?

    +++ b/core/misc/cspell/dictionary.txt
    @@ -1470,6 +1470,7 @@ specialchars
    ...
    +sqlbase

   The spell checker complains about migrate_sqlbase_count_cache_test (the module name) in the namespace line? That seems like a problem … a problem for another day.

2. +1 for moving this to the constructor:

    +++ b/core/modules/migrate/src/Plugin/migrate/source/SourcePluginBase.php
    @@ -199,7 +200,9 @@ public function __construct(array $configuration, $plugin_id, $plugin_definition
    ...
    -    $this->cacheKey = !empty($configuration['cache_key']) ? $configuration['cache_key'] : NULL;
    +    if ($this->cacheCounts) {
    +      $this->cacheKey = !empty($configuration['cache_key']) ? $configuration['cache_key'] : $plugin_id . '-' . hash('sha256', Json::encode($configuration));
    +    }

   Can we simplify this using ??? Does it make any difference whether we concatenate the plugin ID (as here) or add it to the array before applying Json::encode()? That is, Json::encode(['plugin_id' => $plugin_id] + $configuration).

3. I still think we should update the doc block (#8.2). See #13.

    +++ b/core/modules/migrate/src/Plugin/migrate/source/SqlBase.php
    @@ -385,7 +385,8 @@ protected function fetchNextBatch() {
       /**
        * {@inheritdoc}
        */
    -  public function count($refresh = FALSE) {
    +  protected function doCount() {
    +    // Uses countQuery() because it is faster than the parent implementation.

4. Nit: it is valid YAML, but the usual practice is to indent the dependency list:

    +++ b/core/modules/migrate/tests/modules/migrate_sqlbase_count_cache_test/migrate_sqlbase_count_cache_test.info.yml
    @@ -0,0 +1,6 @@
    +type: module
    +name: Migrate SqlBase count cache test.
    +description: 'Provides a source plugin to test that counts are cached in SQL sources.'
    +package: Testing
    +dependencies:
    +- drupal:migrate

   Also, the description should have ending punctuation but not the name.

5. It seems like a cheat to use source_module = "migrate", but I guess it is good enough for the test.

    +++ b/core/modules/migrate/tests/modules/migrate_sqlbase_count_cache_test/src/Plugin/migrate/source/SqlCountCache.php
    @@ -0,0 +1,44 @@
    ...
    +/**
    + * Source plugin for SqlBase count cache test.
    + *
    + * @MigrateSource(
    + *   id = "sql_count_cache",
    + *   source_module = "migrate",
    + * )
    + */
    +class SqlCountCache extends SqlBase {

6. We do not translate strings in test classes, but I am not sure about the policy for test modules. Do we want to use t() here?

    +  public function fields() {
    +    return [
    +      'id' => t('Id'),
    +    ];
    +  }

7. Do we want to say that this test @covers the test class, not SqlBase or SqlBase::count()?

    +++ b/core/modules/migrate/tests/src/Kernel/MigrateSqlSourceCountCacheTest.php
    @@ -0,0 +1,103 @@
    ...
    +/**
    + * Tests SqlBase source count caching.
    + *
    + * @covers \Drupal\migrate_sqlbase_count_cache_test\Plugin\migrate\source\SqlCountCache

8. Funny, this came up on another issue I was reviewing today, #3189064: Migration sql source plugins can not be serialised because of reference to the database connection. Compare with Drupal\Tests\migrate\Kernel\Plugin\MigrationPluginListTest, which sets up a connection. Do we have an issue to make this more convenient?

    +    // Since we don't yet inject the database connection, we need to use a
    +    // reflection hack to set it in the plugin instance.
    +    $reflector = new \ReflectionObject($plugin);
    +    $property = $reflector->getProperty('database');
    +    $property->setAccessible(TRUE);
    +    $property->setValue($plugin, $this->getDatabase($source_data));

9. This looks like a problem:

    +      // Compute the default cache key, to ensure nothing was written to cache.
    +      // @see \Drupal\migrate\Plugin\migrate\source\SourcePluginBase::__construct()
    +      $cache_key = hash('sha256', $plugin->getPluginId());
    +      $this->assertFalse(\Drupal::cache('migrate')->get($cache_key, 'cache'));

   That is not the right cache key! If we keep the reflection hack, then we could add a protected method to compute the cache key and make it accessible in this test. Maybe it is not worth that much trouble. Is there a good way to test that we have the right key? Perhaps add an elseif clause after if ($expected_cache_key) that checks for $source_plugin_configuration['cache_counts'], and add a corresponding entry to the data provider.

10. Since we are using the same source data, expected result, and expected count, wouldn’t it be simpler to define these in the test method instead of the data provider? Come to think of it, why do have $expected_results if we never reference it?

My first thought would be to keep the scope narrow and fix one problem at a time, starting with the older issue. But if you both agree that we should do it all here, then I will go along and see what the core commiters think.

We originally decided to separate the issues around #2598670-33: Source count caching does not work for SQL sources, but I've wanted them both fixed for well over a year now, and those issues stalled. If we can fix them together here and close all the other issues, and the FMs don't object, I think we should just get it done. They are both relatively trivial fixes, they are definitely intertwined and the code has been written, just gotten stale. Lets just address #33 and get it done.

In #33, I thought it was already clear that you both approved of this approach. I was not asking for confirmation. I am sorry I was ambiguous.

Thanks for all the fixes in #35!

The test-only patch in #35 has two failures and an error. The failures are because we test that the cache key is defined in the constructor, which is one of the changes in this issue. So these failures do not prove that we have fixed anything. The error is caused by trying to access the data property after a cache miss. Since we are also changing the cache key, the failure does not prove anything either. We do not need a failing test, but if we want to provide one, it will have to be different from the test-only patch.

1. This is much better. Now that $cacheKey is set in the constructor (#33.2), we can test it here.

    +++ b/core/modules/migrate/tests/src/Kernel/MigrateSqlSourceCountCacheTest.php
    @@ -0,0 +1,110 @@
    ...
    +    // Verify the the computed cache key.
    +    $property = $reflector->getProperty('cacheKey');
    +    $property->setAccessible(TRUE);
    +    $this->assertSame($expected_cache_key, $property->getValue($plugin));

2. After asserting $expected_count == $count, we could assert $expected_count == $cached_count instead of asserting that two computed values are equal:

    +    $count = $plugin->count();
    +    $this->assertSame($expected_count, $count);
    +
    +    // Cache hit afterwards.
    +    if ($expected_cache_key) {
    +      $cached_count = \Drupal::cache('migrate')->get($expected_cache_key, 'cache')->data;
    +      $this->assertSame($count, $cached_count);
    +    }

3. Same snippet: you removed the else clause, checking for no caching after $plugin->count(). I guess the point is what I said in #33.9: checking any particular cache key does not really tell us anything. Is there a way to test that the cache is completely empty, or that it has no entries with the known prefix?

4. Follow-up to #33.10 and the reply in #35. In MigrateSourceTestBase, we declare that providerSource() is the data provider for testSource(). Now we are using it as the provider for another test method. That seems wrong.

   I do not like the pattern of defining test methods in base classes. I think I see a follow-up issue.

   If we keep the current providerSource(), then I think we should add @cover comments for everything tested by testSource(). Maybe the right way to do that would be to declare testSource(), add the comments there, and just call the parent method.

   A simpler idea is to have providerSource() return an empty array and create a separate provider for the new test.

Re #37.3: I guess we would have to replace the cache provided by \Drupal->cache('migrate') with an implementation that lets us check whether any cache keys starting with $plugin_id have been created. That seems like a lot of work just to verify that something has not been cached.

This is blocking #3190818: Allow source counts to be cached: implement ::doCount() instead of ::count()

This is not specific to Drupal 7 sources.

37.4 I agree this seems wrong. The new test is an outlier in all the source plugin tests in that it has it's own testSource method. I'd much rather keep to the existing way of doing these tests. Instead of adding a new testSource I think we should test the caching in the existing \Drupal\Tests\migrate\Kernel\MigrateSqlSourceTestBase::testSource in an if block so existing tests will not test caching. If we do that the block of 'reflection' code is not repeated and it makes it easier for contrib to test caching.

The existing \Drupal\Tests\migrate\Kernel\MigrateSourceTestBase::testSource has been changed in a similar manner (an if block) to allow testing of high_water and there is an issue to also add testing checkRequirements. With that in mind this seems like a reasonable idea.

I know benjifisher does not like test methods in test base classes but that is not an issue to resolved here (I should add that I am rather fond of \Drupal\Tests\migrate\Kernel\MigrateSourceTestBase).

I have made a patch to see what this looks like. I kept the use of strings for the array keys in MigrateSqlSourceCountCacheTest::providerSource but changed them to be consistent with the parameter names in testSource. Moved the new test to the directory path where source plugin tests normally live. Changed the name in the test module to avoid the dictionary change (I don't think that will be accepted).

@Wim Leers, glad you liked the changes in #42.

I missed this.

+++ b/core/modules/migrate/tests/src/Kernel/Plugin/source/MigrateSqlSourceCountCacheTest.php
@@ -0,0 +1,82 @@
+        'expected_cache_id' => NULL,
...
+        'expected_cache_id' => 'sql_count_cache-dbed2396c230e025663091479993a206441bf1f9ae4e60ebf3b504e4a76ad471',
...
+        'expected_cache_id' => 'sql_count_cache-83c62856dd5afc011f32574bcdc11c595557d629e1d73045e9353df2441ec269',
...
+        'expected_cache_id' => 'custom_cache_key_here',

The parameter name in the method signature is expected_cache_key.

It has been a long time since I last looked at this issue, and the patch has changed a lot since then.

I will start a fresh review now, but the issue summary needs an update. At least

• Replace the reference to #2751829: Default source plugin cache key is insufficiently unique with a reference to #3092227: SourcePluginBase should cache based on a hash of the source configuration, not just the plugin id..
• Explain what MemoryCounterBackendFactory is all about.

Perhaps I am to blame for MemoryCounterBackendFactory: see #37.3 and #38. I have learned (from @alexpott, I think) to be suspicious of classes that are used only in tests. Let's discuss this one openly.

To make a migration have a cacheable source count, all you need to do is specify cache_counts: true under the migration definition's source key. And until #2751829: Default source plugin cache key is insufficiently unique is fixed, you'll also need to specify cache_key.

Is that enough to justify a change record? I will add the issue tag for that. If you disagree, then you can remove it.

NW for the issue summary update.

1. I think one of the comments already explains why we use this for the cache key. It will help to add that to the issue summary.

    +++ b/core/modules/migrate/src/Plugin/migrate/source/SourcePluginBase.php
    @@ -215,7 +216,9 @@ public function __construct(array $configuration, $plugin_id, $plugin_definition
    ...
    -    $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));
    +    }

2. This is more complicated than it has to be:

        $count = NULL;
   
        // 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();
        }
   
        // If count caching is enabled: read from cache or update it.
        if ($this->cacheCounts) {
          if ($count === NULL) {
            $cache_object = $this->getCache()->get($this->cacheKey, 'cache');
            if (is_object($cache_object)) {
              // Early return for a cache hit.
              return $cache_object->data;
            }
            $count = $this->doCount();
          }
          // No early return, so this was not a cache hit. Either a forced refresh
          // or a cache miss would lead to this point. Either way, update the cache.
          $this->getCache()->set($this->cacheKey, $count);
        }
        return $count;

   I made a little boolean table: the possible T/F values of $this->cacheCounts, $refresh, and whether there is a cache hit. I concluded that we return the cached value in the T/F/T case. In all other cases, we call doCount() and return its value. If $this->cacheCounts is T, then we cache the computed value before returning it. Like this:

        if ($this->cacheCounts && !$refresh) {
          $cache_object = $this->getCache()->get($this->cacheKey, 'cache');
          if (is_object($cache_object)) {
            return $cache_object->data;
          }
        }
        $count = $this->doCount();
        if ($this->cacheCounts) {
          $this->getCache()->set($this->cacheKey, $count);
        }
        return $count;

   That saves a few lines and reduces the maximum level of nesting by 1. I think this version is simple enough that it does not need any code comments, but add some of them back if you disagree.

3. Can we use string|null for the @param comment?

    +++ b/core/modules/migrate/tests/src/Kernel/MigrateSqlSourceTestBase.php
    @@ -67,12 +77,14 @@ protected function getDatabase(array $source_data) {
    ...
    +   * @param mixed $expected_cache_key
    +   *   (optional) The expected cache key.

4. There are a lot of calls to \Drupal::cache('migrate') here:

    @@ -82,6 +94,33 @@ public function testSource(array $source_data, array $expected_data, $expected_c
    ...
    +    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(\Drupal::cache('migrate')
    +        ->get($expected_cache_key, 'cache'));
    +
    +      $this->assertSame([], \Drupal::cache('migrate')->getCounter('set'));
    +      $count = $plugin->count();
    +      $this->assertSame($expected_count, $count);
    +      $this->assertSame([$expected_cache_key => 1], \Drupal::cache('migrate')->getCounter('set'));
    +
    +      // Cache hit afterwards.
    +      $cache_item = \Drupal::cache('migrate')
    +        ->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([], \Drupal::cache('migrate')->getCounter('set'));
    +      $plugin->count();
    +      $this->assertSame([], \Drupal::cache('migrate')->getCounter('set'));
    +    }

   Can we call it just once, before the if block: $cache = \Drupal::cache('migrate')? Or call it $migrate_cache if you think that is clearer. If we add an @var comment, that will help future us understand where getCounter() comes from.

5. Since the last three parameters for testSource() are optional, wouldn’t it be simpler to leave them off instead of giving them the default values?

    +++ b/core/modules/migrate/tests/src/Kernel/Plugin/source/MigrateSqlSourceCountCacheTest.php
    @@ -0,0 +1,82 @@
    ...
    +    return [
    +      'uncached source count' => $data + [
    +        'configuration' => [],
    +        'high_water' => NULL,
    +        'expected_cache_key' => NULL,
    +      ],

6. Is that someone’s birthday? If so, you are 30 days older than my daughter. Maybe I can introduce you some day. ;)

    +          'some_source_plugin_configuration_key' => 19920106,

I should look at the failures in the test-only patch, but not at this hour.

Some of the previous discussion is coming back to me. In particular, #31-#33. We are rolling in the fix for #3092227: SourcePluginBase should cache based on a hash of the source configuration, not just the plugin id. into this issue, right?

I'll look at this in the next few days

#52.1 Updated the IS.

#52
2. Nice work to simplify the logic. It looks right to me, and tests are still passing. :-)
3, 4, 5. All fixed.
6. Looking back I see that I added that line but I don't know why I picked that number. Maybe I was playing with https://unix.click/.

Todo: Answer the question in the last paragraph. The cache_key generation is certainly here but the test over there is different.

Yes, the test is different. It tests that when the source configuration is different and caching is used the caches are unique and have the correct value. Therefor, I have moved that tests here.

This should finish resolving all the points in #52.

What is to be in the change record?

OK, this is ready for more review.

I am reviewing the patch in #56. For now, I will update the issue summary following #51.

@quietone:

What is to be in the change record?

I am not sure we need one. I suggested it in #51. If we do not need a change record, then we can just remove the tag.

@quietone:

Thanks for all the updates based on #52. They look good, except for one minor point:

    +++ b/core/modules/migrate/tests/src/Kernel/MigrateSqlSourceTestBase.php
    @@ -82,6 +94,32 @@ public function testSource(array $source_data, array $expected_data, $expected_c
    ...
    +    $cache = \Drupal::cache('migrate');
    +    if ($expected_cache_key) {

In #52.4, I said, "If we add an @var comment, that will help future us understand where getCounter() comes from." I will not set the issue to NW for this, but I still think that /** @var MemoryCounterBackend $cache **/ will be helpful to the next person who looks at this test.

As usual, my attention to detail still needs improvement. This patch adds the inline @var asked for in #52.4 and again in #59.

Like benjifisher I don't see a need for a change record, so removing tag. I am sure someone will add it back if they think otherwise.

@quietone, thanks for the addition! Back to RTBC.

Some great work here folks, a few observations/questions

1. Should we be looking to resolve the existing bug #3092227: SourcePluginBase should cache based on a hash of the source configuration, not just the plugin id. first and postpone this on that issue? Seems like we may have a scope creep issue here.

2. +++ b/core/lib/Drupal/Core/Cache/MemoryCounterBackendFactory.php
   @@ -0,0 +1,24 @@
   +class MemoryCounterBackendFactory implements CacheFactoryInterface {
   ...
   +      $this->bins[$bin] = new MemoryCounterBackend();
   

   At the moment MemoryCounterBackend has a todo comment to remove it and use phpunit mocks instead.

   There is only one usage of it in core (\Drupal\Tests\path_alias\Kernel\AliasTest) - by adding another usage and a factory for creating it, we're moving away from that todo.

   Are we sure we want to do that - can we achieve the same result with mocks?

   If not could we have our test implement ServiceModifierInterface and CacheFactoryInterface direct, to prevent others from inadvertently adding more usages of that class.

   Of if we're sure we want to keep that class around, should we be removing the todo here?

3. +++ b/core/modules/migrate/tests/src/Kernel/MigrateSqlSourceTestBase.php
   @@ -82,6 +94,33 @@ public function testSource(array $source_data, array $expected_data, $expected_c
   +      $this->assertNotSame(FALSE, $cache_item, 'This is not a cache hit.');
   

   Should we use $this->assertNotEmpty instead of assertNotSame?

@larowlan:

#62.1: We discussed this question in #33-#35. I am neutral: I raised the same question. Both @mikelutz and @wimleers think it is a good idea to fix both at once.

#62.2: Maybe I just need more practice with mocks, but it seems to me that the tests are clearer if we use a real class in this case. I vote for removing the todo comment, but I will defer to people who have more experience with PHPUnit.

Restoring status after HEAD was broken.

The testbot reports a spelling error. Back to NW.

Any reason this isn't using a MR?
Regardless... attached for review.

Interdiff

Thanks for the quick fix! I verified that the only change is a spelling correction in a comment, and the testbot is happy. Back to RTBC.

I notice #3212521: [backport] cspell dislikes identifer in core/modules/views/src/Plugin/views/filter/FilterPluginBase.php and will fail any patch touching that file. Are there going to be a lot of spelling corrections on open issues from now on?

No, I don't think so. The spelling fix applied in #69 is a duplicate of #3212547: cspell Dictionaries changed, checking all files, which I reckon will be committed before this issue. To catch future changes to the dictionary a follow up was made, #3212579: Spell check all files if dictionary.txt changes.

ARF - another random failure, queued a retest

The retest passed. Back to RTBC.

Committed and pushed a6124cc0b6 to 9.3.x and 70a9d088d1 to 9.2.x and a6bb85217a to 9.1.x. Thanks!