Prevent result cache key collisions when sharing caches across different connections #713
Conversation
|
Hello, thank you for creating this pull request. I have automatically opened an issue http://www.doctrine-project.org/jira/browse/DBAL-1030 We use Jira to track the state of pull requests and the versions they got |
|
I'm not sure if this needs fixing at all: there seems to be the base mistake to use the same cache for different data-sources here. Obviously, results from one database or the other are different if they are not replicas, so the cache should be namespaced accordingly. Did you consider that first? |
|
Hi! You're absolutely right about setting the namespace appropriately. That solves this problem. But why not making the most of the automatic cache id generation and prevent a potential issue (with no extra cost nor complexity) which can save pain to some users? I would expect a safe key generation mechanism to be smart enough to take that into account and generate a different key for a different connection. What are your thoughts? |
|
I agree with that, but we'd need to check all our codebases instead of fixing the issue only locally. |
|
Sorry, don't get what you mean. |
|
I mean that we need to find every location where a cache key is generated and fix it. |
|
Do you mean within the DBAL codebase? The only location calling |
|
So... do you think it doesn't add value to add this modification? |
|
@vilartoni yes, seems valuable to me, though it still doesn't solve cases where different machines access the same cache with same |
|
@Ocramius Sure, I didn't consider the current behaviour was a bug at all. The case you mention wouldn't be solved with this PR changes, as you correctly said, but many times we should aim for the 'better' and not for the 'best' ;) WRT the documentation, I considered updating the phpdoc was enough. |
Can you check if any chapter mentions caching at all? Those need to be changed indeed. |
|
@Ocramius Haven't found any mention in the docs to the cache performed by
It keeps being automatically generated, which is the only thing I'd say the end user needs to know. |
|
Fair enuff. Last thing before merging: we need tests for the changed units. |
vilartoni
force-pushed
the
prevent-result-cache-key-collisions
branch
from
7b7539e to
90bc19d
Compare
|
@Ocramius Done. The failed tests have nothing to do with the new test I've added. |
| /** | ||
| * @test | ||
| */ | ||
| public function it_should_use_the_given_cache_key_if_present() |
There was a problem hiding this comment.
Our convention is still the good 'ol testShouldUseTheGivenCacheKeyIfPresent()
vilartoni
force-pushed
the
prevent-result-cache-key-collisions
branch
from
9d343e3 to
dfbe772
Compare
|
@Ocramius Done! ;-) |
| * | ||
| * @return array | ||
| */ | ||
| public function generateCacheKeys($query, $params, $types) | ||
| public function generateCacheKeys($query, $params, $types, $connectionParams = array()) |
There was a problem hiding this comment.
May want to type-hint this as array
vilartoni
force-pushed
the
prevent-result-cache-key-collisions
branch
from
6c93950 to
bb143d6
Compare
vilartoni
force-pushed
the
prevent-result-cache-key-collisions
branch
from
bb143d6 to
73c8278
Compare
|
@Ocramius Are you happy with the changes? |
|
Looks good, though I'll have to investigate why you needed to change the |
|
@Ocramius the change is because the PHPUnit config files used by Travis were outdated (they were missing the bootstrap configuration added in the main config file 2 or 3 years ago) |
|
Urgh, this took aaaages, sorry @vilartoni :-( I merged this into |
|
It's okay ;-) |
That argument was added to don't have key colisions for different connections. More info: doctrine/dbal#713
That argument was added to don't have key colision for different connections. More info: doctrine/dbal#713
That argument was added to not have key collisions for different connections. More info: doctrine/dbal#713
Ocramius
changed the title
There's an issue when using the default cache key generation for the result cache and using the same cache system across different connections as the generated key will be the same regardless of the connection used.
We can solve this just by using the connection params in the key generation. This issue is quite similar to the one fixed in #1075.