Bug 22887 - authorised_values is missing a unique constraint on category + authorised_value
Summary: authorised_values is missing a unique constraint on category + authorised_value
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Database (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords: additional_work_needed
Depends on:
Blocks: 5334 17809
  Show dependency treegraph
 
Reported: 2019-05-10 14:28 UTC by Martin Renvoize
Modified: 2020-06-30 08:23 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
20.05.00


Attachments
Bug 22887: Add unique constraint to authorised_values (1.81 KB, patch)
2019-05-10 14:32 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22887: Warn if duplicates exist (2.66 KB, patch)
2020-03-26 09:29 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22887: Rely on unique constraint in av admin (3.12 KB, patch)
2020-03-26 09:29 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22887: Remove dup in fr-FR av.sql (1.61 KB, patch)
2020-03-26 09:37 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22887: Rely on unique constraint in av admin (3.20 KB, patch)
2020-03-26 09:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22887: Add unique constraint to authorised_values (1.88 KB, patch)
2020-03-26 16:13 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22887: Warn if duplicates exist (2.72 KB, patch)
2020-03-26 16:13 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22887: Remove dup in fr-FR av.sql (1.67 KB, patch)
2020-03-26 16:13 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22887: Rely on unique constraint in av admin (3.26 KB, patch)
2020-03-26 16:13 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22887: Add unique constraint to authorised_values (1.88 KB, patch)
2020-04-05 18:39 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 22887: Warn if duplicates exist (2.72 KB, patch)
2020-04-05 18:40 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 22887: Remove dup in fr-FR av.sql (1.73 KB, patch)
2020-04-05 18:40 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 22887: Rely on unique constraint in av admin (3.32 KB, patch)
2020-04-05 18:40 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 22887: Schema changes (1.31 KB, patch)
2020-04-07 11:48 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 22887: Add unique constraint to authorised_values (1.93 KB, patch)
2020-04-13 17:32 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 22887: Warn if duplicates exist (2.78 KB, patch)
2020-04-13 17:32 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 22887: Remove dup in fr-FR av.sql (1.78 KB, patch)
2020-04-13 17:32 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 22887: Rely on unique constraint in av admin (3.39 KB, patch)
2020-04-13 17:32 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 22887: Schema changes (1.37 KB, patch)
2020-04-13 17:32 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 22887: Remove dup fr-FR for existing installs (1.10 KB, patch)
2020-04-14 08:13 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22887: (RM follow-up) Fix Charges Test (1.14 KB, patch)
2020-04-14 18:52 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22887: Fix SQL syntax for MySQL 5.5 (1.18 KB, patch)
2020-04-15 13:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22887: Remove superfluous warn statements (999 bytes, patch)
2020-04-15 13:05 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Renvoize 2019-05-10 14:28:50 UTC
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.
Comment 1 Martin Renvoize 2019-05-10 14:32:42 UTC
Created attachment 89550 [details] [review]
Bug 22887: Add unique constraint to authorised_values
Comment 2 Julian Maurice 2020-03-26 09:12:41 UTC
+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)
Comment 3 Jonathan Druart 2020-03-26 09:29:35 UTC
Created attachment 101790 [details] [review]
Bug 22887: Warn if duplicates exist
Comment 4 Jonathan Druart 2020-03-26 09:29:38 UTC
Created attachment 101791 [details] [review]
Bug 22887: Rely on unique constraint in av admin
Comment 5 Jonathan Druart 2020-03-26 09:37:06 UTC
Created attachment 101792 [details] [review]
Bug 22887: Remove dup in fr-FR av.sql

https://en.wikipedia.org/wiki/Cocos_(Keeling)_Islands
Comment 6 Jonathan Druart 2020-03-26 09:47:36 UTC
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
Comment 7 Jonathan Druart 2020-03-26 09:48:08 UTC
(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
Comment 8 Martin Renvoize 2020-03-26 16:11:54 UTC
Oooh.. great to see this one moving.. thanks for following through on it Jonathan.. I clearly dropped the ball :(
Comment 9 Martin Renvoize 2020-03-26 16:13:28 UTC
Created attachment 101889 [details] [review]
Bug 22887: Add unique constraint to authorised_values

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 10 Martin Renvoize 2020-03-26 16:13:31 UTC
Created attachment 101890 [details] [review]
Bug 22887: Warn if duplicates exist

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 11 Martin Renvoize 2020-03-26 16:13:34 UTC
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>
Comment 12 Martin Renvoize 2020-03-26 16:13:36 UTC
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>
Comment 13 Martin Renvoize 2020-03-26 16:14:16 UTC
Looks great to me, signing off :)
Comment 14 Lari Taskula 2020-04-05 18:19:31 UTC
+1, just noticed this too.
Comment 15 Lari Taskula 2020-04-05 18:35:30 UTC
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.
Comment 16 Lari Taskula 2020-04-05 18:39:58 UTC
Created attachment 102409 [details] [review]
Bug 22887: Add unique constraint to authorised_values

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 17 Lari Taskula 2020-04-05 18:40:01 UTC
Created attachment 102410 [details] [review]
Bug 22887: Warn if duplicates exist

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 18 Lari Taskula 2020-04-05 18:40:04 UTC
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>
Comment 19 Lari Taskula 2020-04-05 18:40:07 UTC
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>
Comment 20 Jonathan Druart 2020-04-06 10:09:58 UTC
(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.
Comment 21 Lari Taskula 2020-04-07 11:48:54 UTC
Created attachment 102511 [details] [review]
Bug 22887: Schema changes
Comment 22 Lari Taskula 2020-04-07 11:50:28 UTC
(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?
Comment 23 Julian Maurice 2020-04-07 12:02:08 UTC
> 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.
Comment 24 Jonathan Druart 2020-04-07 12:10:31 UTC
Because we never died before?
Comment 25 Julian Maurice 2020-04-07 12:19:38 UTC
(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.
Comment 26 Jonathan Druart 2020-04-07 12:26:03 UTC
Feel free to open a new bug report, start a discussion and bring this to the next dev meeting.
Comment 27 Lari Taskula 2020-04-07 12:31:05 UTC
(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.
Comment 28 Jonathan Druart 2020-04-07 12:41:51 UTC
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.
Comment 29 Lari Taskula 2020-04-07 14:34:40 UTC
(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
Comment 30 Katrin Fischer 2020-04-13 17:32:04 UTC
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>
Comment 31 Katrin Fischer 2020-04-13 17:32:07 UTC
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>
Comment 32 Katrin Fischer 2020-04-13 17:32:11 UTC
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>
Comment 33 Katrin Fischer 2020-04-13 17:32:14 UTC
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>
Comment 34 Katrin Fischer 2020-04-13 17:32:19 UTC
Created attachment 102880 [details] [review]
Bug 22887: Schema changes

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 35 Katrin Fischer 2020-04-13 17:33:07 UTC
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?
Comment 36 Jonathan Druart 2020-04-14 08:13:10 UTC
Created attachment 102898 [details] [review]
Bug 22887: Remove dup fr-FR for existing installs
Comment 37 Katrin Fischer 2020-04-14 12:28:33 UTC
Thx Joubu!
Comment 38 Martin Renvoize 2020-04-14 16:23:22 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 39 Martin Renvoize 2020-04-14 18:52:59 UTC
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>
Comment 40 Martin Renvoize 2020-04-14 18:53:15 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 41 Tomás Cohen Arazi 2020-04-14 20:12:20 UTC
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.
Comment 42 Katrin Fischer 2020-04-14 22:54:46 UTC
Hm, strange. I just applied the patch set on my 'outdated' master and the updatedatabase gave no error.
Comment 43 Katrin Fischer 2020-04-14 22:57:09 UTC
(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?
Comment 44 Katrin Fischer 2020-04-14 22:58:20 UTC
(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.
Comment 45 Jonathan Druart 2020-04-15 08:46:52 UTC
(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?
Comment 46 Jonathan Druart 2020-04-15 13:05:04 UTC
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"
Comment 47 Jonathan Druart 2020-04-15 13:05:08 UTC
Created attachment 103000 [details] [review]
Bug 22887: Remove superfluous warn statements

We print them in the description later
Comment 48 Martin Renvoize 2020-04-15 13:13:14 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 49 Martin Renvoize 2020-04-17 09:22:10 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 50 Joy Nelson 2020-05-05 22:25:01 UTC
enhancement not backported to 19.11.x