Code using the authorised_values table expects the combination of 'category' with 'authorised_value' to always be unique. We should constrain it as such and also reap the rewards of dbic accessors that would give us.
Created attachment 89550 [details] [review] Bug 22887: Add unique constraint to authorised_values
+1 But does the updatedb work if there are already duplicate values ? installer/data/mysql/fr-FR/1-Obligatoire/authorised_values.sql insert duplicate values for instance (COUNTRY, CC)
Created attachment 101790 [details] [review] Bug 22887: Warn if duplicates exist
Created attachment 101791 [details] [review] Bug 22887: Rely on unique constraint in av admin
Created attachment 101792 [details] [review] Bug 22887: Remove dup in fr-FR av.sql https://en.wikipedia.org/wiki/Cocos_(Keeling)_Islands
Created attachment 101793 [details] [review] Bug 22887: Rely on unique constraint in av admin Test plan: Try to create 2 AV with the same code for the same category. You should see a warning
(In reply to Jonathan Druart from comment #5) > Created attachment 101792 [details] [review] [review] > Bug 22887: Remove dup in fr-FR av.sql > > https://en.wikipedia.org/wiki/Cocos_(Keeling)_Islands Other .sql files seem ok
Oooh.. great to see this one moving.. thanks for following through on it Jonathan.. I clearly dropped the ball :(
Created attachment 101889 [details] [review] Bug 22887: Add unique constraint to authorised_values Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 101890 [details] [review] Bug 22887: Warn if duplicates exist Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 101891 [details] [review] Bug 22887: Remove dup in fr-FR av.sql https://en.wikipedia.org/wiki/Cocos_(Keeling)_Islands Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 101892 [details] [review] Bug 22887: Rely on unique constraint in av admin Test plan: Try to create 2 AV with the same code for the same category. You should see a warning Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Looks great to me, signing off :)
+1, just noticed this too.
Comment on attachment 101890 [details] [review] Bug 22887: Warn if duplicates exist Review of attachment 101890 [details] [review]: ----------------------------------------------------------------- ::: installer/data/mysql/atomicupdate/bug_22887.perl @@ +7,5 @@ > + GROUP BY category, authorised_value > + HAVING c > 1 > + |, { Slice => {} }); > + if ( @$duplicates ) { > + warn "WARNING - Cannot create unique constraint on authorised_value(category, authorised_value)\n"; Nice to give a warning. We should still make sure the constraint actually gets created for everyone. Perhaps delete the duplicates and print & log what was deleted? Or die instead of warn.
Created attachment 102409 [details] [review] Bug 22887: Add unique constraint to authorised_values Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 102410 [details] [review] Bug 22887: Warn if duplicates exist Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 102411 [details] [review] Bug 22887: Remove dup in fr-FR av.sql https://en.wikipedia.org/wiki/Cocos_(Keeling)_Islands Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Lari Taskula <lari.taskula@hypernova.fi>
Created attachment 102412 [details] [review] Bug 22887: Rely on unique constraint in av admin Test plan: Try to create 2 AV with the same code for the same category. You should see a warning Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Lari Taskula <lari.taskula@hypernova.fi>
(In reply to Lari Taskula from comment #15) > Comment on attachment 101890 [details] [review] [review] > Bug 22887: Warn if duplicates exist > > Review of attachment 101890 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: installer/data/mysql/atomicupdate/bug_22887.perl > @@ +7,5 @@ > > + GROUP BY category, authorised_value > > + HAVING c > 1 > > + |, { Slice => {} }); > > + if ( @$duplicates ) { > > + warn "WARNING - Cannot create unique constraint on authorised_value(category, authorised_value)\n"; > > Nice to give a warning. We should still make sure the constraint actually > gets created for everyone. > > Perhaps delete the duplicates and print & log what was deleted? > > Or die instead of warn. The idea is to not remove data that could be use by a fork in a different way. I usually prefer to not remove the data, and warn, instead of remove them and... sometimes people forgets to backup their DB... ;) But I am not strongly attached to the approach here.
Created attachment 102511 [details] [review] Bug 22887: Schema changes
(In reply to Jonathan Druart from comment #20) > (In reply to Lari Taskula from comment #15) > > Comment on attachment 101890 [details] [review] [review] [review] > > Bug 22887: Warn if duplicates exist > > > > Review of attachment 101890 [details] [review] [review] [review]: > > ----------------------------------------------------------------- > > > > ::: installer/data/mysql/atomicupdate/bug_22887.perl > > @@ +7,5 @@ > > > + GROUP BY category, authorised_value > > > + HAVING c > 1 > > > + |, { Slice => {} }); > > > + if ( @$duplicates ) { > > > + warn "WARNING - Cannot create unique constraint on authorised_value(category, authorised_value)\n"; > > > > Nice to give a warning. We should still make sure the constraint actually > > gets created for everyone. > > > > Perhaps delete the duplicates and print & log what was deleted? > > > > Or die instead of warn. > > The idea is to not remove data that could be use by a fork in a different > way. I usually prefer to not remove the data, and warn, instead of remove > them and... sometimes people forgets to backup their DB... ;) > But I am not strongly attached to the approach here. Indeed deleting can be a bit nasty and unfair. Then what about stopping updatedatabase.pl with die() until the sysadmin fixes the problem?
> Indeed deleting can be a bit nasty and unfair. Then what about stopping > updatedatabase.pl with die() until the sysadmin fixes the problem? I agree, the updatedatabase process should stop until the problem is fixed. Otherwise one can continue using Koha without the DB constraint and end up having more problems.
Because we never died before?
(In reply to Jonathan Druart from comment #24) > Because we never died before? Maybe that's a problem ? Because of that, two separate instances of Koha running the exact same version can have different database schema.
Feel free to open a new bug report, start a discussion and bring this to the next dev meeting.
(In reply to Jonathan Druart from comment #26) > Feel free to open a new bug report, start a discussion and bring this to the > next dev meeting. I think it is directly in the scope of this Bug. We must make sure a database change applies in a way that keeps database structure consistent across any vanilla Koha installation.
It is what we are doing for years. Dying in the updatedatabase.pl will require more work than just adding a die statement. We need to handle the error, propagate it to the UI. The way it's done so far will make the change not trivial. Please discuss this on a separate bug report, when agreed I will add a follow-up for this update DB entry. I am willing to work on it if needed. There is much more to do in this area but this bug report is not the place for such discussion.
(In reply to Jonathan Druart from comment #28) > Please discuss this on a separate bug report, when agreed I will add a > follow-up for this update DB entry. I am willing to work on it if needed. > > There is much more to do in this area but this bug report is not the place > for such discussion. Bug 25078
Created attachment 102876 [details] [review] Bug 22887: Add unique constraint to authorised_values Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 102877 [details] [review] Bug 22887: Warn if duplicates exist Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 102878 [details] [review] Bug 22887: Remove dup in fr-FR av.sql https://en.wikipedia.org/wiki/Cocos_(Keeling)_Islands Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Lari Taskula <lari.taskula@hypernova.fi> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 102879 [details] [review] Bug 22887: Rely on unique constraint in av admin Test plan: Try to create 2 AV with the same code for the same category. You should see a warning Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Lari Taskula <lari.taskula@hypernova.fi> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 102880 [details] [review] Bug 22887: Schema changes Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Only wondering about this one: Bug 22887: Remove dup in fr-FR av.sql As there is a duplicate in the sample data, could we include a fix for this in the database update avoiding manual intervention for the fr-FR installations?
Created attachment 102898 [details] [review] Bug 22887: Remove dup fr-FR for existing installs
Thx Joubu!
Nice work everyone! Pushed to master for 20.05
Created attachment 102939 [details] [review] Bug 22887: (RM follow-up) Fix Charges Test The charges test added a duplicate authorised value needlessly (it's already present in the sample data). Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Got this: kohadev-koha@156b126f55d3:/kohadevbox/koha$ perl installer/data/mysql/updatedatabase.pl Upgrade to 19.12.00.068 done [19:57:44]: Bug 21190 - Add prefs AuthFailureLog and AuthSuccessLog Upgrade to 19.12.00.069 done [19:57:44]: Bug 22784 - Add a new suggestions.archived column Upgrade to 19.12.00.070 done [19:57:44]: Bug 22774 - Limit purchase suggestion in a specified time period DBD::mysql::db selectall_arrayref failed: Non-grouping field 'c' is used in HAVING clause [for Statement " SELECT category, authorised_value, COUNT(concat(category, ':', authorised_value)) AS c FROM authorised_values GROUP BY category, authorised_value HAVING c > 1 "] at installer/data/mysql/updatedatabase.pl line 21576. Can't use an undefined value as an ARRAY reference at installer/data/mysql/updatedatabase.pl line 21582.
Hm, strange. I just applied the patch set on my 'outdated' master and the updatedatabase gave no error.
(In reply to Katrin Fischer from comment #42) > Hm, strange. I just applied the patch set on my 'outdated' master and the > updatedatabase gave no error. Also tested on my master branch - works fine. MySQL version or something like that?
(In reply to Katrin Fischer from comment #43) > (In reply to Katrin Fischer from comment #42) > > Hm, strange. I just applied the patch set on my 'outdated' master and the > > updatedatabase gave no error. > > Also tested on my master branch - works fine. MySQL version or something > like that? ok, the tests are invalid - I already have the constraint. Too late here - will have to leave that to more awake minds.
(In reply to Tomás Cohen Arazi from comment #41) > Got this: > > kohadev-koha@156b126f55d3:/kohadevbox/koha$ perl > installer/data/mysql/updatedatabase.pl > Upgrade to 19.12.00.068 done [19:57:44]: Bug 21190 - Add prefs > AuthFailureLog and AuthSuccessLog > Upgrade to 19.12.00.069 done [19:57:44]: Bug 22784 - Add a new > suggestions.archived column > Upgrade to 19.12.00.070 done [19:57:44]: Bug 22774 - Limit purchase > suggestion in a specified time period > DBD::mysql::db selectall_arrayref failed: Non-grouping field 'c' is used in > HAVING clause [for Statement " > SELECT category, authorised_value, COUNT(concat(category, ':', > authorised_value)) AS c > FROM authorised_values > GROUP BY category, authorised_value > HAVING c > 1 > "] at installer/data/mysql/updatedatabase.pl line 21576. > Can't use an undefined value as an ARRAY reference at > installer/data/mysql/updatedatabase.pl line 21582. I tried with both docker-compose.mysql8.0.yml and docker-compose.mariadb_latest.yml, but cannot make it fails. Which version do you have?
Created attachment 102999 [details] [review] Bug 22887: Fix SQL syntax for MySQL 5.5 This does not happen on MySQL 8 or latest MariaDB, but does on MySQL 5.5 "Non-grouping field 'c' is used in HAVING clause"
Created attachment 103000 [details] [review] Bug 22887: Remove superfluous warn statements We print them in the description later
enhancement not backported to 19.11.x
See following bug for identified issue in AV Report Subgroups: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26622