cviebrock / eloquent-taggable

Easily add the ability to tag your Eloquent models in Laravel.
MIT License
537 stars 72 forks source link

Popular Tags Not Working For Me #60

Closed BenQoder closed 5 years ago

BenQoder commented 7 years ago

When I Do

return $tags = \App\Job::popularTags(10); I Get Error (2/2) QueryException SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near '? GROUP BY t.tag_id ORDER BY taggable_count DESC LIMIT ?' at line 1 (SQL: SELECT t.*, COUNT(t.tag_id) AS taggable_count FROM taggable_tags t LEFT JOIN taggable_taggables tt ON tt.tag_id=t.tag_id WHERE tt.taggable_type IS App\Job GROUP BY t.tag_id ORDER BY taggable_count DESC LIMIT 10)

cviebrock commented 7 years ago

Fix coming shortly: version 3.2.1 (for Laravel 5.5) and 3.1.1 (for L5.4).

BenQoder commented 7 years ago

I'm So Sorry, I Still Get Error After Update

(2/2) QueryExceptionSQLSTATE[42000]: Syntax error or access violation: 1055 'haayaa.t.name' isn't in GROUP BY (SQL: SELECT t.*, COUNT(t.tag_id) AS taggable_count FROM taggable_tags t LEFT JOIN taggable_taggables tt ON tt.tag_id=t.tag_id WHERE tt.taggable_type = App\Job GROUP BY t.tag_id ORDER BY taggable_count DESC) in Connection.php (line 664)

BenQoder commented 7 years ago

I Modified The TagService And Got It Working

public function getPopularTags(int $limit = null, $class = null, int $minCount = null): Collection
    {
        $sql = 'SELECT t.*, COUNT(t.tag_id) AS taggable_count FROM taggable_tags t LEFT JOIN taggable_taggables tt ON tt.tag_id=t.tag_id';
        $bindings = [];

        if ($class) {
            $sql .= ' WHERE tt.taggable_type = ?';
            $bindings[] = ($class instanceof Model) ? get_class($class) : $class;
        }

        $sql .= ' GROUP BY t.tag_id, t.name, t.normalized, t.created_at, t.updated_at';

        if ($minCount) {
            $sql .= ' HAVING taggable_count >= ?';
            $bindings[] = $minCount;
        }

        $sql .= ' ORDER BY taggable_count DESC';

        if ($limit) {
            $sql .= ' LIMIT ?';
            $bindings[] = $limit;
        }

        return Tag::fromQuery($sql, $bindings);
    }
cviebrock commented 7 years ago

You changed the SQL to add those additional GROUP BY columns? That actually isn't doing anything useful in the query, and may, in fact, mess up the counts.

Looking at your original error, I don't see where it's looking for haayaa.t.name in the query, so I suspect there is something else going on here.

What version of Laravel, the package, and database are you using?

BenQoder commented 7 years ago

Im On Laravel 5.5. Using My MySql

cviebrock commented 7 years ago

Ok. So, where is the "haayaa" coming from? That isn't in the query run by the package.

BenQoder commented 7 years ago

haayaa is the name of of the mysql database

cviebrock commented 7 years ago

But you can run this query directly in the DB, right?

SELECT t.*, COUNT(t.tag_id) AS taggable_count FROM taggable_tags t 
LEFT JOIN taggable_taggables tt ON tt.tag_id=t.tag_id 
WHERE tt.taggable_type = 'App\Job' 
GROUP BY t.tag_id 
ORDER BY taggable_count DESC
BenQoder commented 7 years ago

That Returned Empty?. When I Run Directly In The DB

BenQoder commented 7 years ago

But I Don't Think The Count Is Messed Up For Now. Because I Keep Increasing The Tags And Everything Works Perfectly

cviebrock commented 7 years ago

If that SQL is returning an empty result set, but your SQL works:

SELECT t.*, COUNT(t.tag_id) AS taggable_count FROM taggable_tags t 
LEFT JOIN taggable_taggables tt ON tt.tag_id=t.tag_id 
WHERE tt.taggable_type = 'App\Job' 
GROUP BY t.tag_id, t.name, t.normalized, t.created_at, t.updated_at
ORDER BY taggable_count DESC

... then it's definitely not an issue with the package.

BenQoder commented 7 years ago

I'm working on my windows XAMPP Environment, Maybe When I Switch Over To The Server (When I'm Done). Situation Might Change... Thanks A Lot For Your Time

cviebrock commented 7 years ago

I just fixed another issue that might be related. Can you try the latest version of the package (3.2.2 for Laravel 5.5, or 3.1.2 for Laravel 5.4), and let me know if that solves things?

BenQoder commented 7 years ago

Not A Fix

ghost commented 7 years ago

I have the same problem. but it's so weird.

If I run the sql directly in mysql, it works.

but via php, it just breaks.

ghost commented 7 years ago

problem found. config/database.php change mysql -> strict to false

ghost commented 7 years ago

@cviebrock I think maybe you should group by the other columns. the strict mode will turn mysql ONLY_FULL_GROUP_BY on

cviebrock commented 6 years ago

@wppd @benidixt I apologize! I didn't think of strict mode implications with my groupBy() call.

Can you take a second and try the dev-master branch again? I added all the columns to my group, so that should fix it for both strict and non-strict modes. If it works, I'll tag a new release.

Thanks!

ghost commented 6 years ago

will run a test and let you know then

cviebrock commented 6 years ago

Gonna assume all is good now, so closing this and bumping the package

cviebrock commented 6 years ago

Version 3.2.3 should resolve your issues.

crishellco commented 6 years ago

This is still an issue with v3.2.3.

image

cviebrock commented 6 years ago

@crishellco Can you confirm that the above comes from a call to Product::withAllTags($array) and not the popular tags method? Based on the SQL, I'm assuming that's the case.

Unfortunately, I don't see an easy solution here. The GROUP BY part of that clause would require the package (i.e. the Eloquent model) to know all the fields in products.* so that it could group by them when MySQL is in strict mode.

I'm not sure if there is a way to turn strict mode off on a per-query basis, which is really the only thing I can think of right now.

hairmenu commented 6 years ago

i think there is still an issue here SELECT t.*, COUNT(t.tag_id) AS taggable_count FROM taggable_tags t as it is, taggable_count will always be equal to 1. I think it shoudl be SELECT t.*, COUNT(tt.tag_id) AS taggable_count FROM taggable_tags t

OlexandrPopov commented 6 years ago

Unfortunately, I don't see an easy solution here. The GROUP BY part of that clause would require the package (i.e. the Eloquent model) to know all the fields in products.* so that it could group by them when MySQL is in strict mode.

What do you think about selecting columns from the information_schema.columns table? There is already a method implemented in Laravel's query builder that can do it. However, it will add additional query:

select column_name as `column_name` from information_schema.columns where table_schema = ? and table_name = ?

I'm not sure if there is a way to turn strict mode off on a per-query basis, which is really the only thing I can think of right now.

It's possible to change the sql_mode variable before the query execution and restore its value after. By default Laravel sets the following value:

SET sql_mode='ONLY_FULL_GROUP_BY,STRICT_TRANS_TABLES,NO_ZERO_IN_DATE,NO_ZERO_DATE,ERROR_FOR_DIVISION_BY_ZERO,NO_AUTO_CREATE_USER,NO_ENGINE_SUBSTITUTION';

and it can be overridden like this:

SELECT @@sql_mode INTO @saved_sql_mode;
SET sql_mode = (SELECT REPLACE(@@sql_mode, 'ONLY_FULL_GROUP_BY,', ''));
-- your select query goes here
SET sql_mode = @saved_sql_mode;

but I'm not sure there is a way to to execute multiple queries at once

cviebrock commented 5 years ago

This is an old ticket, and I think several issues are being conflated. If there are still issues with strict mode and MySQL, please open another ticket.

judgej commented 5 years ago

This SQL strict mode thing is a big issue for us. We are rolling out sites across different database types, and we can't be shoving MySQL options and assumptions about the dialects of SQL they run into Azure SQL or Postgres. Is there no way to generate strict SQL grouping in the first place that works to, say, SQL92 standard?

Maybe instead of ->select($this->getTable() . '.*') in the trait, could the columns defined in the application query builder be used? i.e. the application knows what columns it needs, so let the application decide what is selected here, and then they could also all be added to the group by (also by the application).

Edit: actually a possible workaround for this is to do exactly as I suggested above - in the application, after adding the tag selection ($query->withAllTags(...)) just override the select() and groupBy() on the query to make it right - include all the named model fields you want, and also add them to the groupby. No changes to the taggables package is needed.

MyThings::withAllTags('foobar')
->select(['my_things.id', 'my_things.name', ...])
->groupBy(['my_things.id', 'my_things.name', ...])
->get();

This seems to be working for me so far.

Update: it mostly works. Doing a $query->count() is not working properly though. It does not always give the same count as is actually fetched in $query->get(), which is a bit strange. I tend to use $query->count() to get the total number of matching records when paging, just before adding the limit and offset to get a page of actual results. The problem looks like this: https://github.com/laravel/framework/issues/22883

I'm going to add a little more observation after hitting snag. The more columns you add to the select part and consequently the group by part (with strict group by settings) the bigger the chunk of sort space memory that is needed. With a dozen columns on the main model being retrieved, I'm finding sort space can be blown nearly every time, on even tiny datasets.

One solution to this is to fetch just the IDs of the main model first.

$query = MyThings::withAllTags('Foobar')
...your query conditions...
->select('my_things.id')
->groupBy('my_things.id');

If there are results, then you can fetch the rest of the records:

$ids = $query->get()->pluck('id');
if ($ids) {
   $result = $query->with('tags')->find($ids);
} else {
   $result = collect();
}

This assumes you are doing paging, so won't be selecting tens of thousands of IDs to squeeze into a find() query.

In theory all the columns that are used in a where clause also need to be added to the select clause, and so consequently to the group by clause. I'm not finding this is needed even in strict mode in MySQL, but it may be something else that needs to be tackled at some point with some database versions.