executeCacheQuery + fetchOne is a footgun

[Seldaek]

Bug Report

Q	A
BC Break	maybe
Version	2.13.1

Summary

When running a cached query, the docs do mention you have to fetch all results (In order for the data to actually be cached its necessary to ensure that the entire result set is read (the easiest way to ensure this is to use one of the fetchAll*() methods), but the code does not enforce this at all, making it an easy footgun.

How to reproduce

Yesterday I noticed some deprecations warnings in this code:

        $stmt = $this->getEntityManager()->getConnection()
            ->executeCacheQuery($sql, [], [], new QueryCacheProfile(...));
        $result = $stmt->fetchAll();
        $stmt->closeCursor();

        return (int) $result[0]['count'];

So I figured "oh ok fetchAll is deprecated, and I see this only uses/selects one row actually so let me go ahead and use fetchOne and clean this up a little":

        $result = $stmt->fetchOne();
        $stmt->free();

        return (int) $result;

Then this disaster happened: https://twitter.com/seldaek/status/1389657778533322758

And ultimately I found in the docs that one has to fetch all records, which in this case I kinda did anyway with fetchOne but I guess you have to fetch until it realizes there is nothing left to fetch otherwise it's not enough. And if you don't fetch all then it somehow does not use the cached result it seems, or does not persist it, anyway it kinda defeated the whole purpose of having a cached query here..

So switching back to fetchAllAssociative resolved this issue in composer/packagist@998cb8a

Expected behaviour

I think using fetchOne on a cached statement should perhaps throw, or something along those lines, or on __destruct it could maybe check if there were more rows to be fetched and if not then do the caching as I expected it to do.

I am not entirely sure how this plays out with dbal 3.x, or if I maybe misunderstood everything, but I figured people with more of a clue about the project should be aware of the presence of this footgun :)

[greg0ire]

This does not look like a bug, so I don't think we should fix it in 2.x. I think it still exist on 3.x though, and on 3.x, executeQuery and executeCacheQuery return a Result. We might split that interface into 2 parts, and add deprecations in CachingResult saying that fetchOne and methods part of the non-cacheable interface will no longer be implemented in what is returned from executeCacheQuery in 4.0.

[Seldaek]

No it's definitely not a bug in itself, more something that's unclear if you don't read the docs carefully, and so it makes it easy to write bugs using this code. IMO it'd be great if it can be made harder to misuse, but I'll leave the how up to you because I'm definitely not familiar enough with the doctrine internals.

I think there are a few ways to use this result:

• fetchAll* => no problem, this will just work as it fetches the entire result set
• fetchOne, fetchNumeric and similar => probably could be deprecated as you suggest, as they are really unlikely to do what you want
• iterate* => still could be useful when you want to keep memory use low, but the docblocks on the CachingResult for those should make it clear that if the iterator is not fully consumed, the cache will not be working as expected.

[greg0ire]

@morozov WDYT? Maybe we could target 2.x with the suggested docblock improvements, and then do what I described above?

2.x could also receive a doc improvement, IMO we could make this a stronger warning, based on @Seldaek 's comment, right now it does not catch the eye enough I think:

In order for the data to actually be cached its necessary to ensure that the entire result set is read (the easiest way to ensure this is to use one of the fetchAll*() methods):

[morozov]

@morozov WDYT?

I'm not touching this (#4189 (review)).

[greg0ire]

Okay noted!

[greg0ire]

Gave the documentation a try at #4640

[github-actions]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.