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

Paginators Cache ID is too generic #669

Closed
FabianKoestring opened this issue Sep 30, 2021 · 7 comments · Fixed by #670
Closed

Paginators Cache ID is too generic #669

FabianKoestring opened this issue Sep 30, 2021 · 7 comments · Fixed by #670
Labels
Bug Something isn't working
Milestone

Comments

@FabianKoestring
Copy link
Contributor

Hey there,

it seems like the _getCacheInternalId() Method in laminas-paginator/src/Paginator.php does create too generic cache ids. I use DoctrinePaginator with multiple WHERE parts. The generated cache key is the same as the DoctrinePaginator without WHERE part.

There is an exception for DbSelect instances. See laminas/laminas-paginator#6

Is there a way to get around this problem?

@FabianKoestring
Copy link
Contributor Author

Maybe implement \JsonSerializable?

@TomHAnderson
Copy link
Member

Can you provide a unit test for this? Though your description and the other bug reference are useful, seeing this in code would be most beneficial.

@FabianKoestring
Copy link
Contributor Author

FabianKoestring commented Oct 6, 2021

@TomHAnderson I cant provide a failing unit test because the relevant parts are protected or private. Maybe following laminas example explains the problem a little bit more.

Working example (Different pages creates different cache id´s)

        /** @var EntityManager $entityManager */
        $entityManager = $container->get(EntityManager::class);

        /** @var StorageAdapterFactoryInterface $storageFactory */
        $storageFactory = $container->get(StorageAdapterFactoryInterface::class);

        $cache = $storageFactory->create(
            'filesystem',
            [
                'cache_dir' => '/var/www/stack/src/crm/data/cache/'
            ],
            [
                ['name' => 'serializer'],
            ]
        );
        
        $queryBuilder = new QueryBuilder($entityManager);
        $queryBuilder
            ->select('t0')
            ->from(Entity\City', 't0');

        $doctrinePaginator = new DoctrinePaginator($queryBuilder);
        $doctrinePaginatorAdapter = new DoctrinePaginatorAdapter($doctrinePaginator);

        $paginator = new LaminasPaginator($doctrinePaginatorAdapter);
        $paginator::setCache($cache);

        $paginator->getItemsByPage(1); //_getCacheId() => Laminas_Paginator_1_07381ecd2347c3ccacd038f34ab9c699
        $paginator->getItemsByPage(2); //_getCacheId() => Laminas_Paginator_2_07381ecd2347c3ccacd038f34ab9c699

Failing example (Different where parts creates same cache id`s)

        /** @var EntityManager $entityManager */
        $entityManager = $container->get(EntityManager::class);

        /** @var StorageAdapterFactoryInterface $storageFactory */
        $storageFactory = $container->get(StorageAdapterFactoryInterface::class);

        $cache = $storageFactory->create(
            'filesystem',
            [
                'cache_dir' => '/var/www/stack/src/crm/data/cache/'
            ],
            [
                ['name' => 'serializer'],
            ]
        );

        $queryBuilder1 = new QueryBuilder($entityManager);
        $queryBuilder1
            ->select('t0')
            ->from('Entity\City', 't0')
            ->where(
                $queryBuilder1->expr()->eq('t0.name', $queryBuilder1->expr()->literal('test'))
            );

        $doctrinePaginator1 = new DoctrinePaginator($queryBuilder1);
        $doctrinePaginatorAdapter1 = new DoctrinePaginatorAdapter($doctrinePaginator1);

        $paginator1 = new LaminasPaginator($doctrinePaginatorAdapter1);
        $paginator1::setCache($cache);

        $queryBuilder2 = new QueryBuilder($entityManager);
        $queryBuilder2
            ->select('t0')
            ->from('Entity\City', 't0')
            ->where(
                $queryBuilder2->expr()->eq('t0.name', $queryBuilder2->expr()->literal('test2'))
            );

        $doctrinePaginator2 = new DoctrinePaginator($queryBuilder2);
        $doctrinePaginatorAdapter2 = new DoctrinePaginatorAdapter($doctrinePaginator2);

        $paginator2 = new LaminasPaginator($doctrinePaginatorAdapter2);
        $paginator2::setCache($cache);

        $paginator1->getItemsByPage(1); //_getCacheId() => Laminas_Paginator_1_07381ecd2347c3ccacd038f34ab9c699
        $paginator2->getItemsByPage(1); //_getCacheId() => Laminas_Paginator_1_07381ecd2347c3ccacd038f34ab9c699

doctrine/doctrine-orm-module/src/DoctrineORMModule/Paginator/Adapter/DoctrinePaginator.php shoud implement \JsonSerializable.

public function jsonSerialize()
    {
        return [
            'select'       => $this->paginator->getQuery()->getSQL(),
            'count_select' => $this->paginator->count(),
        ];
    }

@TomHAnderson
Copy link
Member

Is this the reason you think adding JsonSerilizable will fix the issue?

https://github.com/laminas/laminas-paginator/blob/2.11.x/src/Paginator.php#L906

@FabianKoestring
Copy link
Contributor Author

Is this the reason you think adding JsonSerilizable will fix the issue?

https://github.com/laminas/laminas-paginator/blob/2.11.x/src/Paginator.php#L906

Yea, am i wrong? 🤷‍♂️

@TomHAnderson
Copy link
Member

No! Not at all. I just wanted to be sure we're on the same page. I think your solution of adding JsonSerializable support is a non-intrusive method of fixing the problem you've described. I'd like to go ahead with it. Because you brought up the issue would you like to provide a PR? If not, that's fine and I'll take care of it but I think you should give credit where credit is due.

@FabianKoestring
Copy link
Contributor Author

No! Not at all. I just wanted to be sure we're on the same page. I think your solution of adding JsonSerializable support is a non-intrusive method of fixing the problem you've described. I'd like to go ahead with it. Because you brought up the issue would you like to provide a PR? If not, that's fine and I'll take care of it but I think you should give credit where credit is due.

I will give it a try in the next few days. 👌

@driehle driehle added this to the 4.0.2 milestone Oct 18, 2021
@driehle driehle added the Bug Something isn't working label Oct 18, 2021
driehle pushed a commit to FabianKoestring/DoctrineORMModule that referenced this issue Oct 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
None yet
3 participants