mikebronner / laravel-model-caching

Eloquent model-caching made easy.
MIT License
2.26k stars 217 forks source link

whereRaw is cached regardless of the arguments #244

Closed jf-m closed 5 years ago

jf-m commented 5 years ago

Describe the bug The following bug https://github.com/GeneaLabs/laravel-model-caching/issues/111 seems to have exactly reappeared.

Eloquent Query

SomeModel::whereRaw('somecolumn = ?', ['somevalue'])->get();
SomeModel::whereRaw('somecolumn = ?', ['anothervalue'])->get();
SomeModel::whereRaw('somecolumn = ?', ['yetanothervalue'])->get();

The second and third query always returns the result of the first query.

Environment

Workaround For the record, the following workaround is not ideal but works well :

SomeModel::whereRaw("somecolumn = '" . $somevalue . "'");
SomeModel::whereRaw("somecolumn = '" . $anotherValue . "'");
SomeModel::whereRaw("somecolumn = '" . $yetanothervalue . "'");
mikebronner commented 5 years ago

Thanks @jf-m, I will try to replicate the problem and see what is going on. :)

sten commented 5 years ago

I am having a similar problem with a library that generates a query using the where type 'InRaw'. I took a screenshot of the $wheres variable of the $query object in CacheKey:

Screen Shot 2019-05-10 at 22 34 24

In this example the cache key should use 1 as value of the InRaw where clause, however the function getValuesClause() replaces 1 with the last value from the $this->query->bindings['where'] array in getValuesFromBindings(). In this array 1 is not included. See:

Screen Shot 2019-05-10 at 22 34 45

It appears the bindings array is not correctly formed. This generates double cache keys, which break the model caching library for raw where clauses.

As far as I could debug, either the bindings array should also include the raw values or you should rely on the results of getValuesFromWhere() in getValuesClause(), which correctly returns 1. I have changed the code of getValuesClause to this, which works:

protected function getValuesClause(array $where = null) : string
    {
        if (in_array($where["type"], ["NotNull", "Null"])) {
            return "";
        }

        $values = $this->getValuesFromWhere($where);
        if($where['type'] != 'InRaw') {
            $values = $this->getValuesFromBindings($where, $values);
        }

        return "_" . $values;
    }

This should probably be further extended for other raw types. I hope this helps!

mikebronner commented 5 years ago

Thanks for the additional info, @sten :)

mikebronner commented 5 years ago

@jf-m I have not been able to replicate your whereRaw issue using the latest release (0.4.16). In all tests it performs as expected.

mikebronner commented 5 years ago

@sten can you provide the eloquent query that uses whereInRaw? I understand that you are using a different tool to generate the query, but if you could write it out as an eloquent query for me, that will help me build a test case.

mikebronner commented 5 years ago

@sten I have moved your problem to a new issue, as it is a separate problem. Let's continue the discussion there. @jf-m Please provide more information and test without other plugins that might be altering models or queries. I suspect that something else in your app is conflicting and causing the problem, as all my tests create unique cache keys that include the parameters.

jf-m commented 5 years ago

Hi @mikebronner,

After some investigations, it appears that this bug only occurs when doing a whereRaw after another where clause that has arguments.

Here is how to reproduce this bug :

SomeModel::where('id', '>', 0)->whereRaw('somecolumn = ?', ['somevalue'])->get();
SomeModel::where('id', '>', 0)->whereRaw('somecolumn = ?', ['anothervalue'])->get();

When using the debugger, I found the source of the issue, it's coming from this line inside CacheKey@getRawClauses https://github.com/GeneaLabs/laravel-model-caching/blob/1655e8f8ea3ac01c09849b6c904b5985ac845f2d/src/CacheKey.php#L276

$this->query->bindings["where"][$this->currentBinding] is returning 0, which is my first where argument instead of the second one, which contains the arguments for my whereRaw.

mikebronner commented 5 years ago

@jf-m Thanks for the follow-up, I will try to reproduce :)

mikebronner commented 5 years ago

I was able to reproduce the bug, working to have a fix ready soon. :) Thanks for your patience.

mikebronner commented 5 years ago

@jf-m This should now be fixed in release 0.4.17. Let me know how it works for you.

jf-m commented 5 years ago

@mikebronner Thank you for the quick fix !

It's working like a charm.

Cheers 🍻

mikebronner commented 5 years ago

Awesome! Thanks for letting me know :)