Skip to content
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

Fixed GROUP BY error in artist.class get_albums #2265

Merged
merged 6 commits into from Feb 11, 2020

Conversation

@KingOperator
Copy link
Contributor

KingOperator commented Jan 16, 2020

Hi @lachlan-00,

when using the current develop version I experienced an empty album list on the artist page. In the debug log the following error occurs:

2020-01-16 21:36:40 [xyz] (dba.class) -> Error_query SQL: SELECT MAX(``album``.``id``) AS ``id``, ``album``.``release_type``, ``album``.``mbid`` FROM ``album`` LEFT JOIN ``song`` ON ``song``.``album``=``album``.``id`` LEFT JOIN ``catalog`` ON ``catalog``.``id`` = ``song``.``catalog`` WHERE (``song``.``artist``='1234' OR ``album``.``album_artist``='1234') GROUP BY ``album``.``prefix``, ``album``.``name``, ``album``.``album_artist``, ``album``.``release_type``, ``album``.``mbid``, ``album``.``year`` ORDER BY ``album``.``name``, ``album``.``disk``, ``album``.``year`` 2020-01-16 21:36:40 [xyz] (dba.class) -> Error_query MSG: ["42000",1055,"Expression #2 of ORDER BY clause is not in GROUP BY clause and contains nonaggregated column 'ampache.album.disk' which is not functionally dependent on columns in GROUP BY clause; this is incompatible with sql_mode=only_full_group_by"]

This error is independent of the used MySQL version (7.x or 8) and depends on the used sql_mode=only_full_group_by.

Please validate the provided fix and merge if it is ok for you.

Br

@KingOperator KingOperator force-pushed the KingOperator:fix-get_albums-sql branch from 8e6f959 to 2adda66 Jan 16, 2020
KingOperator
@KingOperator KingOperator requested a review from lachlan-00 Jan 16, 2020
@lachlan-00

This comment has been minimized.

Copy link
Member

lachlan-00 commented Jan 16, 2020

Thanks, i have a separate mysql server to test on so i'll try it out and get it in.

Thank you for finding it. the queries have really exploded over the last year.

@lachlan-00

This comment has been minimized.

Copy link
Member

lachlan-00 commented Jan 16, 2020

I think what i'll also do is set up my dev environments to use full grouping seeing as it's a mysql default.

@lachlan-00 lachlan-00 self-assigned this Jan 21, 2020
@lachlan-00

This comment has been minimized.

Copy link
Member

lachlan-00 commented Jan 22, 2020

i wonder if this line is backwards.

$sort_disk = (AmpConfig::get('album_group')) ? "" : ", `album`.`disk`";

If you set this code instead does it work?

$sort_disk = (AmpConfig::get('album_group')) ? ", `album`.`disk`" : "";
@lachlan-00

This comment has been minimized.

Copy link
Member

lachlan-00 commented Jan 22, 2020

although it seems like just putting the disk part into 319 will work instead

WHERE (`song`.`artist`='$this->id' OR `album`.`album_artist`='$this->id') $catalog_where GROUP BY `album`.`prefix`, `album`.`name`, `album`.`album_artist`, `album`.`release_type`, `album`.`mbid`, `album`.`year`, `album`.`disk` ORDER BY $sql_sort";
@KingOperator

This comment has been minimized.

Copy link
Contributor Author

KingOperator commented Jan 26, 2020

I've tested your proposals and they both work. It's up to you which one to choose. I would prefer the last one (1st solution: more code, 2nd solution: makes no sense from an function perspective).

But it looks like there is another problem with the grouping function. In the artist view I still see two entries for the same album (without disc suffix). In the album view both discs are listed which is ok. I'm going to investigate this further next days.

@lachlan-00

This comment has been minimized.

Copy link
Member

lachlan-00 commented Jan 27, 2020

I think you've found the same thing. It probably has to be something like max disk in the select instead.

Group by disk will split them up again making the query do the opposite of what we want

KingOperator added 2 commits Jan 28, 2020
@KingOperator

This comment has been minimized.

Copy link
Contributor Author

KingOperator commented Jan 28, 2020

Hi lachlan,
the duplicate albums were caused by the static album.disk in the default case. I've fixed it by setting the album.disk in the default case depending on the album_group config. This makes the previous fix unnecessary.
Additionally I've found another SQL GROUP BY depending bug in the artist sort function. I've fixed it in the query class. The solution isn't the prettiest but it works.
Sorry for the failed travis build but I don't know what's the problem. Can you please tell me? I'm not experienced with it.
Let me know if the changes are OK for you and if I can further assist.

@lachlan-00

This comment has been minimized.

Copy link
Member

lachlan-00 commented Jan 29, 2020

in your git directory you run this before a commit (don't worry i forget it a lot)

php ./php-cs-fixer -v fix

that runs all the scripts that travis checks.
https://travis-ci.org/ampache/ampache/jobs/643125027?utm_medium=notification&utm_source=github_status

the job log also shows the script output.

I'm glad you had a look and worked it out. it's great to see more people with an interest in the code.

@lachlan-00

This comment has been minimized.

Copy link
Member

lachlan-00 commented Jan 29, 2020

btw i'll do a test on the current code. (check your spacing ;)) and i'm happy with your changes so far.

@lachlan-00

This comment has been minimized.

Copy link
Member

lachlan-00 commented Jan 29, 2020

Reminder for me that mariadb can set the same grouping as MySQL with this query

SET @@SQL_MODE = 'STRICT_TRANS_TABLES,ERROR_FOR_DIVISION_BY_ZERO,NO_AUTO_CREATE_USER,NO_ENGINE_SUBSTITUTION,ONLY_FULL_GROUP_BY';
lachlan-00 and others added 2 commits Jan 30, 2020
…-get_albums-sql
@KingOperator

This comment has been minimized.

Copy link
Contributor Author

KingOperator commented Feb 4, 2020

Hi lachlan, thank's for the help with the php-cs-fixer. I saw you've already fixed the spacing. Can I further assist?

@lachlan-00

This comment has been minimized.

Copy link
Member

lachlan-00 commented Feb 4, 2020

I just need to test the change on a full group server now.

Copy link
Member

lachlan-00 left a comment

tested on fullgroup mariadb successfully!

Thanks for waiting for this. i'll be running a laptop with this db setting full time now.

@lachlan-00 lachlan-00 merged commit 7ff78bf into ampache:develop Feb 11, 2020
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.