Skip to content

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

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

Merged
merged 10 commits into from
Jan 26, 2022
Merged

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

merged 10 commits into from
Jan 26, 2022

Conversation

dmason30
Copy link
Contributor

@dmason30 dmason30 commented Sep 2, 2021

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
Copy link
Contributor Author

dmason30 commented Sep 2, 2021

@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
Copy link
Owner

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

@mikebronner mikebronner merged commit efaeb5f into mikebronner:master Jan 26, 2022
@dmason30 dmason30 deleted the false-query-binding branch January 26, 2022 12:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants