Skip to content

Do not fetch result cache twice in case of cache miss #4189

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Do not fetch result cache twice in case of cache miss #4189

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 16 additions & 17 deletions src/Cache/CachingResult.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,18 +43,23 @@ class CachingResult implements Result
/** @var array<int,array<string,mixed>>|null */
private $data;

/** @var array<string, mixed> */
private $fetchedData;

/**
* @param string $cacheKey
* @param string $realKey
* @param int $lifetime
* @param string $cacheKey
* @param string $realKey
* @param int $lifetime
* @param array<string, mixed> $fetchedData
*/
public function __construct(Result $result, Cache $cache, $cacheKey, $realKey, $lifetime)
public function __construct(Result $result, Cache $cache, $cacheKey, $realKey, $lifetime, array $fetchedData)
{
$this->result = $result;
$this->cache = $cache;
$this->cacheKey = $cacheKey;
$this->realKey = $realKey;
$this->lifetime = $lifetime;
$this->result = $result;
$this->cache = $cache;
$this->cacheKey = $cacheKey;
$this->realKey = $realKey;
$this->lifetime = $lifetime;
$this->fetchedData = $fetchedData;
}

/**
Expand Down Expand Up @@ -170,14 +175,8 @@ private function saveToCache(): void
return;
}

$data = $this->cache->fetch($this->cacheKey);

if ($data === false) {
$data = [];
}

$data[$this->realKey] = $this->data;
$this->fetchedData[$this->realKey] = $this->data;

$this->cache->save($this->cacheKey, $data, $this->lifetime);
$this->cache->save($this->cacheKey, $this->fetchedData, $this->lifetime);
}
}
5 changes: 4 additions & 1 deletion src/Connection.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1013,6 +1013,8 @@ public function executeCacheQuery($sql, $params, $types, QueryCacheProfile $qcp)
} elseif (array_key_exists($realKey, $data)) {
$result = new ArrayResult([]);
}
} else {
$data = [];
}

if (! isset($result)) {
Expand All @@ -1021,7 +1023,8 @@ public function executeCacheQuery($sql, $params, $types, QueryCacheProfile $qcp)
$resultCache,
$cacheKey,
$realKey,
$qcp->getLifetime()
$qcp->getLifetime(),
$data
);
}

Expand Down
77 changes: 77 additions & 0 deletions tests/Cache/CachingResultTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
<?php

namespace Doctrine\DBAL\Tests\Cache;

use Doctrine\Common\Cache\Cache;
use Doctrine\DBAL\Cache\CachingResult;
use Doctrine\DBAL\Driver\Result;
use PHPUnit\Framework\TestCase;

class CachingResultTest extends TestCase
{
/** @var string */
private $cacheKey = 'cacheKey';

/** @var string */
private $realKey = 'realKey';

/** @var int */
private $lifetime = 3600;

/** @var array<string, mixed> */
private $resultData = ['id' => 123, 'field' => 'value'];

/** @var array<string, mixed> */
private $cachedData;

/** @var Cache */
private $cache;

/** @var Result */
private $result;

protected function setUp(): void
{
$this->result = $this->createMock(Result::class);
$this->result->expects(self::exactly(2))
->method('fetchAssociative')
->will($this->onConsecutiveCalls($this->resultData, false));

$this->cache = $this->createMock(Cache::class);
$this->cache->expects(self::exactly(1))
->method('save')
->willReturnCallback(function (string $id, $data, int $ttl): void {
$this->assertEquals($this->cacheKey, $id, 'The cache key should match the given one');
$this->assertEquals($this->lifetime, $ttl, 'The cache key ttl should match the given one');
$this->cachedData = $data;
});
}

public function testShouldSaveResultToCache(): void
{
$cachingResult = new CachingResult(
$this->result,
$this->cache,
$this->cacheKey,
$this->realKey,
$this->lifetime,
['otherRealKey' => 'resultValue']
);

do {
$row = $cachingResult->fetchAssociative();
} while ($row !== false);
Copy link
Member

@morozov morozov Sep 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does a test need flow control logic? Do we not know how many rows are expected to be fetched here?

Copy link
Author

@k0ka k0ka Sep 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this particular test, we know that the 2nd fetchAssociative should be false. But that is not what is tested here. This block is a standard way to fetch all rows.

I may use Doctrine\DBAL\Driver\FetchUtils::fetchAllAssociative instead of this block if you like.


$this->assertContains(
$this->resultData,
$this->cachedData[$this->realKey],
'CachingResult should cache data from the given result'
);

$this->assertEquals(
'resultValue',
$this->cachedData['otherRealKey'],
'CachingResult should not change other keys from cache'
);
}
}
58 changes: 58 additions & 0 deletions tests/Functional/ResultCacheTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,64 @@ public function testFetchAllSavesCache(): void
self::assertCount(1, $layerCache->fetch('testcachekey'));
}

public function testCacheQueriedOnlyOnceForCacheMiss(): void
{
$layerCache = $this->createMock(ArrayCache::class);
$layerCache->expects(self::once())
Copy link
Member

@morozov morozov Jul 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd put this under the unit category since it meant to cover the logic of the unit that calls the fetch and it doesn't depend on the actual implementation of the cache.

Copy link
Author

@k0ka k0ka Jul 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can't be a unit test as it covers the full use case. Right now, there are two invocations of fetch - in different places of the library. This test checks, that fetch from real cache is executed only once.

In my PR it is executed in the Connection::executeCacheQuery method. However, it might be executed anywhere and moved somewhere else in the future.

Copy link
Member

@morozov morozov Aug 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO it should be either a functional test the tests certain functionality (hence not mocking any dependencies) or a unit test that covers a given unit and mocks the dependencies. The purpose of such a hybrid test is unclear. If it fails, it's unclear whether the case is broken or a specific unit is because the test doesn't cover either of the two.

Copy link
Author

@k0ka k0ka Aug 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I may change it to ArrayCache, but:

  1. it will depend on the ArrayCache implementation, which is a part of another project - Doctrine/Common.
  2. essentially ArrayCache is a mock by itself. I can hardly imagine why this class might be used in the production environment.
  3. dbal must accept any object, that implements Doctrine\Common\Cache\Cache interface and work with it. This class is external relative to dbal.

Functional testing performs tests of the whole library by sending input to its end-user endpoint and examining output (without checking internal structure). In our case, cache-object is just another type of the input. And we're checking whether dbal uses it correctly.

Copy link
Member

@greg0ire greg0ire Sep 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's your take on "Don't mock what you don't own?"
To me, as you pointed out in 2. ArrayCache is already a mock, but with a few differences:
a) it's easier to use, you don't have to write any expectations, only new ArrayCache()
b) it's closer to reality although it probably won't be used in production (or won't it? you don't know that, could be useful in a long-running process)
c) it is not controlled by you, and a change in it may break your tests; and if that's the case you want to know you don't want to wait until production to notice it breaks.

Copy link
Author

@k0ka k0ka Sep 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't get this question and how it is related to this topic.

Let me explain one more time:

dbal has the "contract": it will accept any object that implements Doctrine\Common\Cache\Cache interface and will use it independenty of it's internal structure to store and retrieve cache entries. In most unit and functional tests Doctrine\Common\Cache\ArrayCache is used. But it's done just for simplicity. dbal must be able to work with any cache provider.

I'm mocking my own object, that implements Doctrine\Common\Cache\Cache interface, because in this particular case it's easier.

Why Doctrine\Common\Cache\ArrayCache is harder to use? The only way to get the number of fetches from it is to retrieve statistics by the getStats() method. But consider running this code:

$cache = new ArrayCache();
$cache->fetch('test_key');
self::assertEquals(1, $cache->getStats()[Cache::STATS_MISSES]);

It will fail. The right value for Cache::STATS_MISSES after one fetch() is 2. It's because of their internal structure (the first cache miss is due to fetching namespace version - but it doesn't matter here). So instead of my current test code

$layerCache->expects(self::once())->method('fetch')

I will have to write the following code

self::assertEquals(2, $layerCache->getStats()[Cache::STATS_MISSES]);

It's completely unclear why I'm using 2 here, while the whole idea of this patch is to remove the double fetch. And if they remove this "namespace version" feature in the ArrayCache (as it useless in this case), this test will fail.

TL;DR:

  1. This test is functional as it treats dbal as a whole and doesn't depend on its internal structure
  2. Mock is used because it's easier to calculate number of real fetches. While ArrayCache is quite simple, its statistics is distorted.

Copy link
Member

@greg0ire greg0ire Sep 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see, you really need a mock since you are adding expectations to it.

essentially ArrayCache is a mock by itself.

It's more like a stub in fact, that's what got me confused. It's easier to use as a stub, but can't be used as a mock since you can't know which methods were called, and instead just get some distorted stats.

Copy link
Member

@morozov morozov Sep 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does interaction between the units of code (the cache and the underlying statement) need to be tested in a functional way? It's obviously a unit-level requirement, not a functional requirement.

The contract you're talking about is a unit-level contract. A cache is a non-functional unit by definition since it's not meant to change the behavior of the system. It only changes the internal implementation details.

Copy link
Author

@k0ka k0ka Sep 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does interaction between the units of code

Any interaction is an "interaction between units of code". The question is where these units of code reside. Cache is not part of DBAL.

A cache is a non-functional unit by definition since it's not meant to change the behavior of the system.

Cache DOES change the behavior of the system. The system is working differently depending on what the cache object returns.

I see you don't understand (or don't want to understand) why this test is functional. I can delete it if you want. Or I can copy it to the "tests/Cache" directory.

Copy link
Member

@greg0ire greg0ire Sep 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see you don't understand (or don't want to understand)

Please treat others more kindly, especially when they take that much time to answer to you. Assuming either ignorance or malice like this will not help.

As for your definition of behavior, it's debatable. To me, it would be more about direct inputs and outputs / the public interface of the system. Whether the system calls systems external systems to create the output should not be relevant in a functional test IMO.

->method('fetch')
->willReturn(false);

$layerCache->expects(self::once())
->method('save');

$result = $this->connection->executeQuery(
'SELECT * FROM caching WHERE test_int > 500',
[],
[],
new QueryCacheProfile(0, 'testcachekey', $layerCache)
);
$result->fetchAllAssociative();
}

public function testDifferentQueriesMayBeSavedToOneCacheKey(): void
Copy link
Member

@morozov morozov Jul 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test passes even without the fix. Is it really needed?

Copy link
Author

@k0ka k0ka Jul 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It shows how many queries may be stored in one cache key. This logic wasn't explicitly shown in other tests and you weren't aware of it, so I think it's better to leave this test. However, it might be taken out to another PR, as it doesn't directly relates to this one.

{
$layerCache = new ArrayCache();

$this->executeTwoCachedQueries($layerCache);
self::assertCount(2, $this->sqlLogger->queries, 'Two queries executed');
self::assertCount(2, $layerCache->fetch('testcachekey'), 'Both queries are saved to cache');

$this->executeTwoCachedQueries($layerCache);
self::assertCount(2, $this->sqlLogger->queries, 'Consecutive queries are fetched from cache');

$layerCache->delete('testcachekey');
$this->executeTwoCachedQueries($layerCache);
self::assertCount(
4,
$this->sqlLogger->queries,
'Deleting one cache key leads to deleting cache for both queris'
Copy link
Member

@greg0ire greg0ire Sep 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'Deleting one cache key leads to deleting cache for both queris'
'Deleting one cache key leads to deleting cache for both queries'

);
}

private function executeTwoCachedQueries(ArrayCache $cache): void
{
$result = $this->connection->executeQuery(
'SELECT * FROM caching WHERE test_int > 500',
[],
[],
new QueryCacheProfile(0, 'testcachekey', $cache)
);
$result->fetchAllAssociative();

$result = $this->connection->executeQuery(
'SELECT * FROM caching WHERE test_int > 400',
[],
[],
new QueryCacheProfile(0, 'testcachekey', $cache)
);
$result->fetchAllAssociative();
}

public function testFetchColumn(): void
{
$query = $this->connection->getDatabasePlatform()
Expand Down