spatie / laravel-query-builder

Easily build Eloquent queries from API requests
https://spatie.be/docs/laravel-query-builder
MIT License
4.04k stars 398 forks source link

Filter by related column only applying to one query per request lifecycle #720

Closed mitchierichie closed 2 years ago

mitchierichie commented 2 years ago

Laravel version: 8.78.1 PHP version: 8.0.10 Package version: 4.0.2

I'm trying to filter by a related column on more than one query per request lifecycle, and then I'm using union on the resulting QueryBuilder objects to run the queries at the same time.

For some reason I'm getting errors because this is my resulting query:

(select `vehicles`.`id`,
        `vehicles`.`updated_at`,
        `vehicles`.`product_id`,
        `vehicles`.`title`,
        `vehicles`.`description`,
        `vehicles`.`price`,
        `vehicles`.`currency`,
        `vehicles`.`location_id`,
        `vehicles`.`product_model_id`,
        `vehicles`.`manufacturer_id`,
        `vehicles`.`stock_number`,
        `vehicles`.`created_at`,
        `vehicles`.`featured`,
        `vehicles`.`status`,
        `vehicles`.`morph_id`,
        `vehicles`.`vin`,
        `vehicles`.`usage`,
        `vehicles`.`year`,
        `vehicles`.`class`,
        `vehicles`.*,
        `locations`.`dealer_id` as `laravel_through_key`
 from `vehicles`
          inner join `locations` on `locations`.`id` = `vehicles`.`location_id`
 where `locations`.`dealer_id` = ?
   and exists(select *
              from `categories`
                       inner join `categorizables` on `categories`.`id` = `categorizables`.`category_id`
              where `vehicles`.`id` = `categorizables`.`categorizable_id`
                and `categorizables`.`categorizable_type` = ?
                and `categories`.`slug` = ?)
   and `vehicles`.`deleted_at` is null
   and `morph_id` = ?
 order by `featured` desc, `updated_at` desc)
union
(select `vehicles`.`id`,
        `vehicles`.`updated_at`,
        `vehicles`.`product_id`,
        `vehicles`.`title`,
        `vehicles`.`description`,
        `vehicles`.`price`,
        `vehicles`.`currency`,
        `vehicles`.`location_id`,
        `vehicles`.`product_model_id`,
        `vehicles`.`manufacturer_id`,
        `vehicles`.`stock_number`,
        `vehicles`.`created_at`,
        `vehicles`.`featured`,
        `vehicles`.`status`,
        `vehicles`.`morph_id`,
        `vehicles`.`vin`,
        `vehicles`.`usage`,
        `vehicles`.`year`,
        `vehicles`.`class`
 from `vehicles`
          inner join `locations` on `locations`.`id` = `vehicles`.`location_id`
 where `locations`.`dealer_id` = ?
   and `categories`.`slug` = ?
   and `vehicles`.`deleted_at` is null
   and `morph_id` = ?
 order by `featured` desc, `updated_at` desc) ​

This is a long query and a little tough to read, so I've included the problem query (2nd of the 2 filtered queries) below:

select `vehicles`.`id`,
       `vehicles`.`updated_at`,
       `vehicles`.`product_id`,
       `vehicles`.`title`,
       `vehicles`.`description`,
       `vehicles`.`price`,
       `vehicles`.`currency`,
       `vehicles`.`location_id`,
       `vehicles`.`product_model_id`,
       `vehicles`.`manufacturer_id`,
       `vehicles`.`stock_number`,
       `vehicles`.`created_at`,
       `vehicles`.`featured`,
       `vehicles`.`status`,
       `vehicles`.`morph_id`,
       `vehicles`.`vin`,
       `vehicles`.`usage`,
       `vehicles`.`year`,
       `vehicles`.`class`
from `vehicles`
         inner join `locations` on `locations`.`id` = `vehicles`.`location_id`
where `locations`.`dealer_id` = ?
  and `categories`.`slug` = ?
  and `vehicles`.`deleted_at` is null
  and `morph_id` = ?
order by `featured` desc, `updated_at` desc

You'll see the issue is that in this query, it's filtering by 'categories'.'slug' but it's not adding an exists clause, or selecting anything from categories table, so I get a column not found exception when this runs.

Why doesn't the query builder generate the same SQL every time?

mitchierichie commented 2 years ago

I was able to fix this by extending the AllowedFilter and FiltersExact classes, and overriding the isRelationProperty method and removing an if statement:

    protected function isRelationProperty(Builder $query, string $property): bool
    {
        if (!Str::contains($property, '.')) {
            return false;
        }

        // removed this if statement
        if (in_array($property, $this->relationConstraints)) {
            return false;
        }

        $firstRelationship = explode('.', $property)[0];

        if (!method_exists($query->getModel(), $firstRelationship)) {
            return false;
        }

        return is_a($query->getModel()->{$firstRelationship}(), Relation::class);
    }

https://github.com/spatie/laravel-query-builder/blob/603145faeffed88ac86cc13c8eb22c0af416f8cb/src/Filters/FiltersExact.php#L47

This doesn't seem to have caused any errors, is this if statement required?

The other reasoning for this seems to be that in FiltersExact::withRelationConstraint(), the whereHas closure calls $this->__invoke on the FiltersExact instance, and there seems to be a reference issue where the same instance of FiltersExact is getting used more than once.

This reference issue happens even if I clone the return value of AllowedFilter::exact.

https://github.com/spatie/laravel-query-builder/blob/603145faeffed88ac86cc13c8eb22c0af416f8cb/src/Filters/FiltersExact.php#L60