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

Already on GitHub? Sign in to your account

FIX: CDbCacheDependency with reuseDependentData did not invalidate cache when getting cache across different requests #2290

Merged
merged 13 commits into from Apr 12, 2013

Conversation

Projects
None yet
4 participants
Contributor

marcovtwout commented Apr 3, 2013

See #2289

Contributor

marcovtwout commented Apr 3, 2013

I see it's showing a lot of master merges too, did I do something wrong while branching?

Member

klimov-paul commented Apr 3, 2013

I see it's showing a lot of master merges too, did I do something wrong while branching?

Do not worry evething is OK. These commits are present in the yiisoft/yii master and will not affect the merging.

Member

klimov-paul commented Apr 3, 2013

@marcovtwout, could you also modify unit test "CCacheDependencyTest" to expose your issue?

Contributor

marcovtwout commented Apr 4, 2013

I created a unit test, although it will not work automatically. There is no way to reset CCacheDependency::$_reusableData, I had to make the variable public for my test. But it reveals the problem.

Writing this test and fix revealed a related 'bug' in the existing code. I added some comments to the original unit test to elaborate on this problem.

Member

klimov-paul commented Apr 5, 2013

@marcovtwout, add static method "CCacheDependency::resetReusableData()":

public static function resetReusableData()
{
    self::$_reusableData=array();
}
Member

klimov-paul commented Apr 5, 2013

Replace code at "CCacheDependencyTest.php" with this one:

<?php
Yii::import('system.caching.dependencies.CFileCacheDependency');
class CCacheDependencyTest extends CTestCase
{
    protected $_cacheDependentData=null;
    public function setCacheDependentData($cacheDependentData)
    {
        $this->_cacheDependentData = $cacheDependentData;
    }
    public function getCacheDependentData()
    {
        return $this->_cacheDependentData;
    }
    public function testReuseDependentData()
    {
        MockCacheDependency::$generateDependentDataCallback=array($this,'getCacheDependentData');
        $dependency1=new MockCacheDependency();
        $dependency1->reuseDependentData = true;
        $dependency2=new MockCacheDependency();
        $dependency2->reuseDependentData = true;
        CCacheDependency::resetReusableData();
        $this->setCacheDependentData('start');
        $dependency1->evaluateDependency();
        $dependency2->evaluateDependency();
        $this->assertFalse($dependency1->getHasChanged(),'Initial dependency1 changed!');
        $this->assertFalse($dependency2->getHasChanged(),'Initial dependency2 changed!');
        $this->assertEquals(1,MockCacheDependency::$generateDependentDataCalled,'Extra invokations of "generateDependentData()"!');
        // New request:
        CCacheDependency::resetReusableData();
        MockCacheDependency::$generateDependentDataCalled=0;
        $this->assertFalse($dependency1->getHasChanged(),'Dependency1 changed for new request!');
        $this->assertFalse($dependency2->getHasChanged(),'Dependency2 changed for new request!');
        $this->assertEquals(1,MockCacheDependency::$generateDependentDataCalled,'Extra invokations of "generateDependentData()"!');
        // New request:
        CCacheDependency::resetReusableData();
        MockCacheDependency::$generateDependentDataCalled=0;
        $this->setCacheDependentData('change1');
        $this->assertTrue($dependency1->getHasChanged(),'Dependency1 is not changed after source change!');
        $dependency1->evaluateDependency();
        // New request:
        CCacheDependency::resetReusableData();
        MockCacheDependency::$generateDependentDataCalled=0;
        $this->assertFalse($dependency1->getHasChanged(),'Dependency1 has been changed!');
        $this->assertTrue($dependency2->getHasChanged(),'Dependency2 has not been changed!');
        $this->assertEquals(1,MockCacheDependency::$generateDependentDataCalled,'Extra invokations of "generateDependentData()"!');
    }
}
class MockCacheDependency extends CCacheDependency
{
    public static $generateDependentDataCallback;
    public static $generateDependentDataCalled = 0;
    public function generateDependentData()
    {
        self::$generateDependentDataCalled++;
        return call_user_func(self::$generateDependentDataCallback);
    }
}

@ghost ghost assigned klimov-paul Apr 5, 2013

Member

klimov-paul commented Apr 5, 2013

@marcovtwout, please update your Pull Request according to the instructions I gave you above.
Also merge the upstream:master into your branch and resolve conflicts.
Make sure unit test passed successfully.
I am waiting for update.

@klimov-paul klimov-paul and 1 other commented on an outdated diff Apr 9, 2013

framework/caching/dependencies/CCacheDependency.php
@@ -39,7 +39,7 @@ class CCacheDependency extends CComponent implements ICacheDependency
* @var array cached data for reusable dependencies.
* @since 1.1.11
*/
- private static $_reusableData=array();
+ protected static $reusableData=array();
@klimov-paul

klimov-paul Apr 9, 2013

Member

Do not make field protected!
Add method "resetReusableData" to the CCacheDependency itself.

@marcovtwout

marcovtwout Apr 9, 2013

Contributor

Why not? The method is available just for the unit test, right? Making it protected only allows subclasses to deal with it, instead of public.

@klimov-paul

klimov-paul Apr 9, 2013

Member

There should NOT be a protected properties inside the framework!
This will make the core code harder to support!

The method "resetReusableData" is actually missing. Each private property should have some interface for set/get or, like in this case, reset its value.

Yes we are using it for the unit test, but that does not mean, there is none who may need it in the other place.

@marcovtwout

marcovtwout Apr 9, 2013

Contributor

Alrighty, I don't want to discuss the pros/cons of private/protected in Yii framework, I will revert it.

@klimov-paul klimov-paul and 1 other commented on an outdated diff Apr 9, 2013

framework/caching/dependencies/CCacheDependency.php
@@ -53,9 +53,9 @@ public function evaluateDependency()
if ($this->reuseDependentData)
{
$hash=$this->getHash();
- if (!isset(self::$_reusableData[$hash]['dependentData']))
- self::$_reusableData[$hash]['dependentData']=$this->generateDependentData();
- $this->_data=self::$_reusableData[$hash]['dependentData'];
+ if (!isset(self::$reusableData[$hash]['dependentData']))
@klimov-paul

klimov-paul Apr 9, 2013

Member

What happened here?
Why this is shown as a diff?

@marcovtwout

marcovtwout Apr 9, 2013

Contributor

$_ -> $ (following the coding standard)

@klimov-paul

klimov-paul Apr 9, 2013

Member

Code standard says:

Property names MUST start with an initial underscore if they are private.

@marcovtwout

marcovtwout Apr 9, 2013

Contributor

Yes, but it is a protected variable now. According to the example code, that means no underscore.

@klimov-paul klimov-paul commented on an outdated diff Apr 9, 2013

framework/caching/dependencies/CCacheDependency.php
@@ -91,6 +87,14 @@ public function getDependentData()
}
+ /**
+ * @see CCacheDependencyTest
@klimov-paul

klimov-paul Apr 9, 2013

Member

Change doc comments to the following:

/**
 * Resets cached data for reusable dependencies.
 * @since 1.1.14
 */
Member

klimov-paul commented Apr 9, 2013

Looks good.
Now I need someone fresh look on this before merging.
@yiisoft, ping.

@samdark samdark commented on an outdated diff Apr 9, 2013

framework/caching/dependencies/CCacheDependency.php
@@ -69,13 +69,9 @@ public function getHasChanged()
if ($this->reuseDependentData)
{
$hash=$this->getHash();
- if (!isset(self::$_reusableData[$hash]['hasChanged']))
- {
- if (!isset(self::$_reusableData[$hash]['dependentData']))
- self::$_reusableData[$hash]['dependentData']=$this->generateDependentData();
- self::$_reusableData[$hash]['hasChanged']=self::$_reusableData[$hash]['dependentData']!=$this->_data;
- }
- return self::$_reusableData[$hash]['hasChanged'];
+ if (!isset(self::$_reusableData[$hash]['dependentData']))
@samdark

samdark Apr 9, 2013

Owner

Extra space before (.

@samdark samdark commented on the diff Apr 9, 2013

framework/caching/dependencies/CCacheDependency.php
@@ -91,6 +87,15 @@ public function getDependentData()
}
+ /**
+ * Resets cached data for reusable dependencies.
+ * @since 1.1.14
+ */
+ public static function resetReusableData()
@samdark

samdark Apr 9, 2013

Owner

Is this one usable for non-testing i.e. real apps?

@marcovtwout

marcovtwout Apr 9, 2013

Contributor

I don't think so.

@samdark

samdark Apr 9, 2013

Owner

Maybe it's better to find a work-around for tests and not to clutter public API?

@klimov-paul

klimov-paul Apr 9, 2013

Member

At least, I can see not harm of having this method.

Roy Osherove in his book "Art of unit testing" says the unit test is also an application client and there is nothing bad your program provides interface for this client.

@samdark

samdark Apr 9, 2013

Owner

It doesn't make sense to me to expose method that will be used in unit testing only and will never be used in the real application. It adds complexity to API.

Member

klimov-paul commented Apr 9, 2013

The possible use case for using "resetReusableData":

  • setup DB dependency like "SELECT MAX(timestamp) FROM updates"
  • fetch some cached data using this dependency.
  • update table "updates"
  • fetching another cached data using same dependency will consider dependency has NOT been changed, so I may require to manually reset reusable data cache.
Member

klimov-paul commented Apr 9, 2013

Maybe it's better to find a work-around for tests and not to clutter public API?

I can not see any work around, which will do sufficient testing.

Contributor

resurtm commented Apr 9, 2013

Looks good to me.

Contributor

resurtm commented Apr 9, 2013

Even if it's not possible: what do you think about adding a note that this method is intended only for internal use as it's made in CActiveRecord::addRelatedRecord()? See this lines: https://github.com/yiisoft/yii/blob/master/framework/db/ar/CActiveRecord.php#L711-L719

Owner

samdark commented Apr 9, 2013

@klimov-paul as far as I remember, phpunit was able to inject private properties.

Member

klimov-paul commented Apr 9, 2013

PHPUnit can inject protected properties, as they caould be accessed via class Reflection, but public private properties can not be extracted this way.
Although I can not tell for sure.

Member

klimov-paul commented Apr 9, 2013

Well maybe this could help:
ReflectionClass::setStaticPropertyValue

Member

klimov-paul commented Apr 9, 2013

No, reflection does NOT allow to setup private or protected properies:

ReflectionException: Class CCacheDependency does not have a property named _reusableData

Member

klimov-paul commented Apr 9, 2013

As for PHPUnit, It allows to read private properties, but seems does not allow to write them.

klimov-paul added a commit that referenced this pull request Apr 12, 2013

Merge pull request #2290 from marcovtwout/2289-CDbCacheDependency-reu…
…seDependentData-fix

FIX: CDbCacheDependency with reuseDependentData did not invalidate cache when getting cache across different requests

@klimov-paul klimov-paul merged commit 0adc8cb into yiisoft:master Apr 12, 2013

Member

klimov-paul commented Apr 12, 2013

Merged.
@marcovtwout, thank you for your effots and your patience on this.

Contributor

marcovtwout commented Apr 12, 2013

Glad to help out! :)

@marcovtwout marcovtwout deleted the marcovtwout:2289-CDbCacheDependency-reuseDependentData-fix branch Apr 12, 2013

s-larionov added a commit to s-larionov/yii that referenced this pull request Apr 26, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment