Skip to content
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.

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

2.8 _getCacheInternalId() is too generic #3

Closed
weierophinney opened this issue Dec 31, 2019 · 11 comments · Fixed by #6
Closed

2.8 _getCacheInternalId() is too generic #3

weierophinney opened this issue Dec 31, 2019 · 11 comments · Fixed by #6
Labels
Bug Something isn't working Unit Test Needed
Milestone

Comments

@weierophinney
Copy link
Member

In Zend\Paginator\Paginator::_getCacheInternalId() (lines 863 - 871) the md5 hash of the adaptor being generated matches all adaptor instances. The issue is on line 868:

. json_encode($this->getAdapter())

This sucker is returning '{}' for all my table adaptors (adaptor being supplied is Zend\Paginator\Adapter\DbSelect).

I don't understand the reason for replacing spl_object_hash, but I do ask how do I get my paginator instances working again?


Originally posted by @richard-parnaby-king at zendframework/zend-paginator#41

@weierophinney weierophinney added Bug Something isn't working Unit Test Needed labels Dec 31, 2019
@weierophinney
Copy link
Member Author

@richard-parnaby-king

This sucker is returning '{}' for all my table adapters…

I can not reproduce this problem. I get always a correct / usable cache ID.

Please test this short script:

$select    = new Zend\Db\Sql\Select(''); // add a table name
$dbAdapter = new Zend\Db\Adapter\Adapter(
    [
        'driver'   => '', // add your database config
        'database' => '',
        'username' => '',
        'password' => '',
    ]
);
$adapter   = new Zend\Paginator\Adapter\DbSelect($select, $dbAdapter);
$paginator = new Zend\Paginator\Paginator($adapter);

$cache = Zend\Cache\StorageFactory::adapterFactory(
    Zend\Cache\Storage\Adapter\BlackHole::class
);
$paginator::setCache($cache);

var_dump($paginator->getItemsByPage(1));

Does this work for you? (Please recheck the internal cache ID.)


Originally posted by @froschdesign at zendframework/zend-paginator#41 (comment)

@weierophinney
Copy link
Member Author

There's another good reason for not using spl_object_hash: it will vary between requests, meaning you'll never get a cache hit on subsequent requests. I think that may have been the root issue behind the original change.


Originally posted by @weierophinney at zendframework/zend-paginator#41 (comment)

@weierophinney
Copy link
Member Author

When I use the black hole adapter it appears to work fine. I am using the filesystem cache adapter and I am getting collisions:


$select = new \Zend\Db\Sql\Select('table1'); // add a table name
$dbAdapter = new \Zend\Db\Adapter\Adapter(
        [
    'driver' => '...', // add your database config
    'dsn' => '...',
    'username' => '...',
    'password' => '...',
        ]
);
$adapter = new \Zend\Paginator\Adapter\DbSelect($select, $dbAdapter);
$paginator = new \Zend\Paginator\Paginator($adapter);

$cache = \Zend\Cache\StorageFactory::factory(
                array(
                    'adapter' => array(
                        'name' => 'filesystem',
                        'options' => array(
                            'dirLevel' => 2,
                            'cacheDir' => 'data/cache',
                            'dirPermission' => 0755,
                            'filePermission' => 0666,
                        ),
                    ),
                    'plugins' => array('serializer'),
                )
);
$paginator::setCache($cache);

var_dump($paginator->getItemsByPage(1));

$select = new \Zend\Db\Sql\Select('table2'); // add a table name
$adapter = new \Zend\Paginator\Adapter\DbSelect($select, $dbAdapter);
$paginator = new \Zend\Paginator\Paginator($adapter);

$cache = \Zend\Cache\StorageFactory::factory(
                array(
                    'adapter' => array(
                        'name' => 'filesystem',
                        'options' => array(
                            'dirLevel' => 2,
                            'cacheDir' => 'data/cache',
                            'dirPermission' => 0755,
                            'filePermission' => 0666,
                        ),
                    ),
                    'plugins' => array('serializer'),
                )
);
$paginator::setCache($cache);

var_dump($paginator->getItemsByPage(1));

Originally posted by @richard-parnaby-king at zendframework/zend-paginator#41 (comment)

@weierophinney
Copy link
Member Author

same problem as reported by @richard-parnaby-king .


Originally posted by @mariojrrc at zendframework/zend-paginator#41 (comment)

@weierophinney
Copy link
Member Author

I think I have found a solution, however I cannot run the unit tests - I am getting a fatal error:

Fatal error: Class 'ZendTest\Paginator\TestAsset\TestArrayAggregate' not found in C:\zend-paginator\test\FactoryTest.php on line 67

I suspect the autoload generated by composer is incomplete, but I don't know how to fix it.

Could someone:
a) help me get the unit tests working
and/or
b) verify that my change (https://github.com/zendframework/zend-paginator/compare/master...richard-parnaby-king:f60e16f59513a5cb921567e4d8233c96883902c0?expand=1) works so I can do a pull request?


Originally posted by @richard-parnaby-king at zendframework/zend-paginator#41 (comment)

@weierophinney
Copy link
Member Author

@richard-parnaby-king your solution would have the same problem of using spl_object_hash, It has an Uptime value for mysql that could change on subsequent requests.

I think the solution for Adapter\DbSelect would be using the SQL itself

protected function _getCacheInternalId()
{
    $adapter = $this->getAdapter();

    if ($adapter instanceof \Zend\Paginator\Adapter\DbSelect) {
        $reflection = new \ReflectionObject($adapter);
        $property = $reflection->getProperty('select');
        $property->setAccessible(true);
        $select = $property->getValue($adapter);
        return md5(
            $reflection->getName()
            . hash('sha512', $select->getSqlString())
            . $this->getItemCountPerPage()
        );
    }

    // @codingStandardsIgnoreEnd
    return md5(
        get_class($adapter)
        . json_encode($adapter)
        . $this->getItemCountPerPage()
    );
}

or make the changes in the Adapter\DbSelect to avoid the Reflection

@Zend\Paginator\Paginator
protected function _getCacheInternalId()
{
    $adapter = $this->getAdapter();

    if ($adapter instanceof \Zend\Paginator\Adapter\DbSelect) {
        return md5(
            get_class($adapter)
            . $adapter->getCacheId()
            . $this->getItemCountPerPage()
        }

    // @codingStandardsIgnoreEnd
    return md5(
        get_class($adapter)
        . json_encode($adapter)
        . $this->getItemCountPerPage()
    );
}
@Zend\Paginator\Adapter\DbSelect
public function getCacheId()
{
    return hash('sha512', $this->select->getSqlString())
}

I could change the interface, but that probably would break someone

@froschdesign what do you think? making a reflection is too ugly?


Originally posted by @rcapile at zendframework/zend-paginator#41 (comment)

@weierophinney
Copy link
Member Author

@rcapile I've been deving on an mssql box so wasn't getting an uptime value. As such, I was getting the same value between requests.

I agree with using the sql string as the source of the hash.


Originally posted by @richard-parnaby-king at zendframework/zend-paginator#41 (comment)

@weierophinney
Copy link
Member Author

@richard-parnaby-king I waiting for @froschdesign input to make a pull request.

Although I think the right solution should be changing the Zend\Paginator\Adapter\AdapterInterface but that would be a major BC.

we (we = @mariojrrc ) could make two pull request: one with the code above for the current version and another changing the interface for the next


Originally posted by @rcapile at zendframework/zend-paginator#41 (comment)

@weierophinney
Copy link
Member Author

@rcapile

Although I think the right solution should be changing the Zend\Paginator\Adapter\AdapterInterface but that would be a major BC.

I see two problems here:

  1. Does the adapter need to know that the result is being cached? No.
  2. Changing the interface is a BC break.

Originally posted by @froschdesign at zendframework/zend-paginator#41 (comment)

@weierophinney
Copy link
Member Author

@froschdesign So the reflection is the best way? ou creating a public function getSelect() in DbSelect?


Originally posted by @rcapile at zendframework/zend-paginator#41 (comment)

@weierophinney
Copy link
Member Author

I'm really sorry about the late response.

Here is a simple idea:

class DbSelect implements AdapterInterface, JsonSerializable
{
    // ...

    public function jsonSerialize()
    {
        return [
            'select'       => $this->sql->buildSqlString($this->select),
            'count_select' => $this->sql->buildSqlString($this->getSelectCount()),
        ];
    }
}

(Edit: The select query for counting should also included.
Edit 2: The method should not return a hashed value, this should be done in the cache method.)


Originally posted by @froschdesign at zendframework/zend-paginator#41 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working Unit Test Needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants