Framework: union and pagination creates wrong sql query making it unusable

[ChangePlaces]

I have a category table, and products table. The products can belong to up to two different categories, this is represented by having a category1_id and category2_id in the products table. I have a relationship in the category where I fetch all the products using eloquent. When using it, it results in an error that the select statements have a different number of columns because the sql creates one select count(*) as aggregate.. where..., and unions a select * where...

[johnpaulmedina]

Why not create a middle table product_categories. In this table have the category_id, and product_id.

Then define the relationship as hasMany. This should solve all your issues. You won't need to have columns with two category_id and you can scale in the future much easier.

I would recommend readying the relationships of eloquent.

https://laravel.com/docs/5.2/eloquent-relationships

[ChangePlaces]

Using a middle table would require a join. Having a cat1 / cat2 id specified in the product makes sense - this is for ebay which only allows two categories.

[srmklive]

@ChangePlaces The relationship structure you have defined is incorrect which is resulting in issues you are facing. Its not a framework issue, its simply an issue with your table relationship structure. I agree with @johnpaulmedina as It simply is a many-to-many relationship between categories & products. You should have a pivot table called 'product_category' containing product_id & category_id. You should simply define relations in your eloquent models like this:

• AppCategory.php

public function products()
{
    return $this->belongsToMany(App\Product::class, 'product_category');
}

• App\Product.php

public function categories()
{
    return $this->belongsToMany(App\Category::class, 'product_category');
}

[ChangePlaces]

The real root of the issue is I have a table that has two relationships with the same model which Eloquent can't deal with I assume. A perfectly reasonable and correct table set up. There's no need for a middle table despite what normalization chapters and fresh-out-of-college students will tell you. My schema is for efficiency, not an 'A' grade at college and is correct.

[willrowe]

What are you calling that is results in the SQL error?

[srmklive]

@ChangePlaces So if you have two relations with the same model. In case of product & category, here is what i think you are assuming:

• A product has one primary category.
• A product has one secondary category.
• A primary category has many products.
• A secondary category has many products.

To me, this structure simply doesn't makes sense. But if i have to set it up using eloquent, i may do something like this:

• AppCategory.php

public function primary_products()
{
    return $this->hasMany(App\Product::class, 'category1_id');
}

public function secondary_products()
{
    return $this->hasMany(App\Product::class, 'category2_id');
}

• App\Product.php

public function primary_category()
{
    return $this->belongsTo(App\Category::class, 'category1_id');
}

public function secondary_category()
{
    return $this->hasMany(App\Category::class, 'category2_id');
}

[fernandobandeira]

If this is indeed a bug with having two relationships with the same model as the op suggested, then despite the table structure i guess we should at least consider this as an issue because you could set up a task list, following the linked list oo pattern with two pointers:

//task table
id
name
next_id
previous_id

Both next_id and previous_id would relate to another task record and AFAIK this would also throw the error that @ChangePlaces is facing.

[srmklive]

@fernandobandeira If we follow the link list oop pattern, then the relationship among tasks can be like:

• App\Task.php

public function nextTask()
{
    return $this->hasOne(App\Task::class, 'next_id');  
}

public function previousTask()
{
    return $this->hasOne(App\Task::class, 'previous_id');  
}

I don't think the above would create a problem. I still think @ChangePlaces has not defined the relations properly in his models, thats why he is facing this issues. I don't think its a framework related error.

[gtapps]

+1
In my case, I have 2 sale types inside products: BUY NOW or AUCTION.

When trying to do some filtering, I need to merge auction products and buy now products all together, and for this I require the union so I can do some where clauses to the auction products table.

I need to use union all since left join will bring me all products even with the where clauses inside the left join.

What happens is that paginate creates the wrong sql query and doesn't work with union all, so this needs to be addressed.

Here is my solution which works flawlessly
File Illuminate\Database\Query\Builder

public function getCountForPagination($columns = ['*'])
    {
        //Change begins here
        if(count($this->unions) < 1){
            $this->backupFieldsForCount();
            $this->aggregate = ['function' => 'count', 'columns' => $this->clearSelectAliases($columns)];
            $results = $this->get()->all();
            $this->aggregate = null;
            $this->restoreFieldsForCount();
        }else{
            $results = $this->newQuery()
                    ->selectRaw("COUNT(*) as aggregate FROM (".$this->toSql().") as aggregate_query")
                    ->mergeBindings($this)
                    ->get()->all();
        }
        //Change ends here
        if (isset($this->groups)) {
            return count($results);
        }

        if (! isset($results[0])) {
            return 0;
        }

        $item = $results[0];

        if (is_object($item)) {
            return (int) $item->aggregate;
        }

        return (int) array_change_key_case((array) $item)['aggregate'];
    }

[gtapps]

Updated with a solution.

[themsaid]

@d0minationPT if you have a solution can you open a Pull Request?

[gtapps]

Hello yes, I will create one.

[gtapps]

Why was this closed?

[4levels]

This issue still exists, just ran into it when trying union'd pagination with GraphQL / Relay.

@gtapps this is how I added your (IMHO perfectly valid) solution in the latest 5.6 branch:
I don't mind creating a new PR for this, but since this is a verbatim copy of your solution, I think you should do this ;-) Credit where credit is due..
@taylorotwell @GrahamCampbell can you elaborate why this never made it in? Are we overseeing something here? The various proposed solutions (make a custom Paginator or create a new model linking both models ao.) are just wrong IMHO since unions should work as documented and there's no reference in the docs that pagination with unions doesn't work out of the box

--- illuminate/database/Query/Builder.php   2018-05-24 12:08:01.241627889 +0200
+++ illuminate/database/Query/Builder.php   2018-05-24 12:12:33.305623092 +0200
@@ -1988,10 +1988,20 @@
      */
     protected function runPaginationCountQuery($columns = ['*'])
     {
-        return $this->cloneWithout(['columns', 'orders', 'limit', 'offset'])
-                    ->cloneWithoutBindings(['select', 'order'])
-                    ->setAggregate('count', $this->withoutSelectAliases($columns))
-                    ->get()->all();
+
+        if(count($this->unions) < 1) {
+            return $this->cloneWithout(['columns', 'orders', 'limit', 'offset'])
+                        ->cloneWithoutBindings(['select', 'order'])
+                        ->setAggregate('count', $this->withoutSelectAliases($columns))
+                        ->get()->all();
+        } else {
+            return $this->newQuery()
+                        ->selectRaw("COUNT(*) as aggregate FROM (".$this->toSql().") as aggregate_query")
+                        ->mergeBindings($this)
+                        ->get()->all();
+        }
+
+
     }

     /**

[nonsocode]

Did you do a PR?