mikebronner / laravel-model-caching

Eloquent model-caching made easy.
MIT License
2.23k stars 212 forks source link

[bug] Fix current binding count when previous binding is false #409

Closed dmason30 closed 2 years ago

dmason30 commented 2 years ago

This bug is difficult to explain clearly so I will try my best.

I discovered it when I was trying query where a relation had a false column then using a whereRaw after. See this example below from the added test in this PR:

$books = (new Book)
    ->whereHas('author', function ($query) {
        return $query->where('is_famous', false); // Passing false here is the issue
    })
    ->whereRaw("title = ?", ['Mixed Clause']) // This binding ends up not being included in the cache key
    ->get();

Issue Before this PR fix the cache key that is generated would be missing the 'Mixed Clause' binding see below where the key ends without the value.

-exists-and_books.author_id_=_authors.id-is_famous_=_-authors.deleted_at_null-_and_title_=

Workaround If you use zero instead as the value $query->where('is_famous', 0) the key is generated correctly.

Cause This is caused by false bindings being ignored in CacheKey::getValuesFromBindings and the currentBindings count was therefore not being incremented correctly.

protected function getValuesFromBindings(array $where, string $values) : string
{
    // If this array value is false then the binding is ignored
    if (($this->query->bindings["where"][$this->currentBinding] ?? false) !== false) {
        // ...
        $this-currentBinding++;
        // ...

Fix Rather than CacheKey::getValuesFromBindings comparing using false it should use some specific value to compare with so it only skips if the binding is not set. Feel free to feedback on this but its a necessary evil to have some sort of unique string provided that is unlikely to be passed as a field value.

protected function getValuesFromBindings(array $where, string $values) : string
{
    // Fallback to this when the current binding does not exist in the bindings array
    $bindingFallback = __CLASS__ . ':UNKNOWN_BINDING';

    if (($this->query->bindings["where"][$this->currentBinding] ??  $bindingFallback) !==  $bindingFallback) {

And this ensures that the generated cache key contains all the correct bindings:

-exists-and_books.author_id_=_authors.id-is_famous_=_-authors.deleted_at_null-_and_title_=_Mixed_Clause

Potential Additional Change? Not sure if you noticed but when the value is false the generated cache key inserts the false as an empty string -is_famous_=_- .

I wonder if we should detect false values and make the key insert them as 0 or 'false' string, e.g. -is_famous_=_0- or is_famous_=_false-?

dmason30 commented 2 years ago

@mikebronner Been a while, hope you are well, also I hope the above is clear enough. Took me a while to debug the cause but at least the fix was simple in the end.

mikebronner commented 2 years ago

Thank you for all the work you put into this!!