laravel / framework

The Laravel Framework.
https://laravel.com
MIT License
32.55k stars 11.03k forks source link

Model::whereNot()->orWhere() incorrectly modifies query structure with SoftDeletes trait or inside a Scope. #50078

Closed GusMilc closed 8 months ago

GusMilc commented 9 months ago

Laravel Version

10.43.0

PHP Version

8.2.4

Database Driver & Version

No response

Description

I ran a command similar to this on my project and got an unexpected result:

Product::whereNot("status", "V")->orWhereNull("synced_at")->get();

I debugged using the function toRawSql()

Product::whereNot("status", "V")->orWhereNull("synced_at")->toRawSql();

and got this sql:

select * from "products" where not (not "status" = 'V' or "synced_at" is null) and "products"."deleted_at" is null

I notice that the problem happened after I added the "SoftDeletes" trait to my Model. If I remove the trait, the sql returns correctly:

select * from "products" where not "status" = 'V' or "synced_at" is null

I start debugging and checked that the issue also happens when using a scope, for example: Product::invalid()->get();

When I change the order of the conditions, the program returns the correct sql:

Product::whereNull("synced_at")->orWhereNot("status", "V")->toRawSql();

result: select * from "products" where "synced_at" is null or not "status" = 'V' and "products"."deleted_at" is null

It happens in:

It doesn't happen if you invert the order:

It doesnt happen without SoftDeletes trait and directly outside of a scope. ✅

It doesn't happen with the "and" where:

Here are some prints I took from my TinkerWell when I was debugging:

image

image

Steps To Reproduce

First, create a Product model with a scope that uses whereNot and orWhere (in my case, I used orWhereNull but you can use orWhere).

<?php

namespace App\Models;

use Illuminate\Database\Eloquent\Builder;
use Illuminate\Database\Eloquent\Model;

class Product extends Model
{

    protected $fillable = [
        'status',
        'synced_at',
    ];

    /*
    *   if not V or not synced, then it is invalid
    */
    public function scopeInvalid(Builder $query): void
    {
        $query->whereNot('status','V')->orWhereNull('synced_at');
    }

}

Now, create the query using whereNot and orWhereNot functions:

Product::whereNot("status", "V") ->orWhereNull("synced_at")->toRawSql();

The code above should result in the expected query: select * from "products" where not "status" = 'V' or "synced_at" is null

Finally create the query using scope:

Product::invalid()->toRawSql();

The second query should resoult in an unexpected query with the condition inside a not ():

select * from "products" where not (not "status" = 'V' or "synced_at" is null)


Add the SoftDeletes trait to the model:

<?php

namespace App\Models;

use Illuminate\Database\Eloquent\Builder;
use Illuminate\Database\Eloquent\Model;
use Illuminate\Database\Eloquent\SoftDeletes;

class Product extends Model
{

    use SoftDeletes;

    protected $fillable = [
        'status',
        'synced_at',
    ];

    /*
    *   if not V or not synced, then it is invalid
    */
    public function scopeInvalid(Builder $query): void
    {
        $query->whereNot('status','V')->orWhere('synced_at',null);
    }

}

Let's run again the queries that we made:

Product::whereNot("status", "V") ->orWhereNull("synced_at")->toRawSql();

The code above should result in the unexpected query: select * from "products" where not (not "status" = 'V' or "synced_at" is null) and "products"."deleted_at" is null

Again, using the scope:

Product::invalid()->toRawSql();

And we should get the incorrect query again.

select * from "products" where not (not "status" = 'V' or "synced_at" is null) and "products"."deleted_at" is null

github-actions[bot] commented 9 months ago

Thank you for reporting this issue!

As Laravel is an open source project, we rely on the community to help us diagnose and fix issues as it is not possible to research and fix every issue reported to us via GitHub.

If possible, please make a pull request fixing the issue you have described, along with corresponding tests. All pull requests are promptly reviewed by the Laravel team.

Thank you!

amjpdevp commented 9 months ago

To fix this issue, you can modify your scope to use explicit parentheses to control the logic of the conditions. Here's an adjusted version of your scopeInvalid method:

public function scopeInvalid(Builder $query): void
{
    $query->where(function ($query) {
        $query->whereNot('status', 'V')->orWhereNull('synced_at');
    })->whereNull('deleted_at');
}

With this modification, the conditions inside the where closure will be grouped together, ensuring the correct logic in the generated SQL query.

This should result in the expected SQL query:

select * from "products" where (not "status" = 'V' or "synced_at" is null) and "products"."deleted_at" is null

By explicitly grouping the conditions using the where closure, you can control the logical structure of the query and avoid unexpected behavior caused by the combination of SoftDeletes and the specific order of whereNot and orWhere.

Guilhem-DELAITRE commented 8 months ago

I tried to write a fix, let me know your thoughts : https://github.com/laravel/framework/pull/50207