Bug 21682

Summary: Stock Rotation: Update DB is failing with strict_sql_modes ON
Product: Koha Reporter: Martin Renvoize <martin.renvoize>
Component: DatabaseAssignee: Martin Renvoize <martin.renvoize>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: major    
Priority: P5 - low CC: jonathan.druart, katrin.fischer, m.de.rooy, nick, tomascohen
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 11897    
Bug Blocks: 21656    
Attachments: Bug 21682: Corrections for strict sql in stockrotationrotas
Bug 21683: (follow-up) - DBUpdate for Dev installs
Bug 21683: (follow-up) - DBUpdate for Dev installs
Bug 21682: Prequel addition of default_exists
Bug 21682: Prequel addition of default_exists
Bug 21682: Corrections for strict sql in stockrotationrotas
Bug 21682: (follow-up) - DBUpdate for Dev installs
Bug 21682: Follow up to use the default_exists check
Bug 21682: Corrections for strict sql in stockrotationrotas
Bug 21682: Add update DB entry for existing installs
Bug 21682: Corrections for strict sql in stockrotationrotas
Bug 21682: Add update DB entry for existing installs
Bug 21682: Add update DB entry for existing installs

Description Martin Renvoize 2018-10-26 10:50:19 UTC
As pointed out in comment 369 in bug 11897 the db update isn't strict modes correct.
Comment 1 Martin Renvoize 2018-10-26 10:59:48 UTC
Created attachment 81280 [details] [review]
Bug 21682: Corrections for strict sql in stockrotationrotas

Test Plan:
1) Ensure you're database is prior to version 18.06.00.040
2) Ensure 'strict_sql_modes' is enabled
3) Run updatedatabase
4) See no errors during upgrade
5) Run a fresh install
6) Note no errors during database installation
Comment 2 Martin Renvoize 2018-10-26 11:00:32 UTC
As astutely pointed out by Jonathan... the fix appears to be simple, patch submitted.
Comment 3 Jonathan Druart 2018-10-26 11:23:50 UTC
You should update existing [dev] installs as well.
Comment 4 Katrin Fischer 2018-10-26 14:56:41 UTC
As the devs refresh the dbs quite frequently, shoudl this be blocker?
Comment 5 Jonathan Druart 2018-10-26 15:01:52 UTC
(In reply to Katrin Fischer from comment #4)
> As the devs refresh the dbs quite frequently, shoudl this be blocker?

Either we need it or we write that somewhere in the guidelines :)
Comment 6 Martin Renvoize 2018-10-27 09:01:44 UTC
Created attachment 81347 [details] [review]
Bug 21683: (follow-up) - DBUpdate for Dev installs
Comment 7 Martin Renvoize 2018-10-27 09:03:15 UTC
I'm not 100% sure on the idempotency of the atomicupdate.. I believe 'DROP DEFAULT' is idempotent in itself, but couldn't find any documentation either way and it's not trivial to check that a DEFAULT constraint exists before attempting to drop it... open to advise here.
Comment 8 Martin Renvoize 2018-10-27 09:06:20 UTC
Created attachment 81348 [details] [review]
Bug 21683: (follow-up) - DBUpdate for Dev installs
Comment 9 Martin Renvoize 2018-10-27 09:08:48 UTC
Actually.. I do need help here..

When running in SQL strict mode I get the following for the atomic update.

DBD::mysql::db do failed: BLOB/TEXT column 'description' can't have a default value [for Statement "ALTER TABLE `stockrotationrotas` ALTER COLUMN `description` DROP DEFAULT"] at (eval 1403) line 3.

I think we need to check for sql strict mode before attempting to apply the update or not.. :(

This was meant to be a simple fix :(
Comment 10 Martin Renvoize 2018-10-29 11:33:07 UTC
Actually.. I get that error whether I enable strict modes or not.. I have no idea.
Comment 11 Tomás Cohen Arazi 2018-10-29 14:18:36 UTC
This should be fixed on the DBRev, no point spending this much time for a simple fix. Specially if the engine rejects 'fixing' the DB structure.
Comment 12 Mark Tompsett 2018-10-29 17:05:24 UTC
Created attachment 81525 [details] [review]
Bug 21682: Prequel addition of default_exists

prove t/db_dependent/Installer.t
perldoc C4::Installer
-- There should be something for default_exists
Comment 13 Mark Tompsett 2018-10-29 17:14:53 UTC
Created attachment 81526 [details] [review]
Bug 21682: Prequel addition of default_exists

prove t/db_dependent/Installer.t
perldoc C4::Installer
-- There should be something for default_exists
Comment 14 Mark Tompsett 2018-10-29 17:14:57 UTC
Created attachment 81527 [details] [review]
Bug 21682: Corrections for strict sql in stockrotationrotas

Test Plan:
1) Ensure you're database is prior to version 18.06.00.040
2) Ensure 'strict_sql_modes' is enabled
3) Run updatedatabase
4) See no errors during upgrade
5) Run a fresh install
6) Note no errors during database installation

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 15 Mark Tompsett 2018-10-29 17:15:00 UTC
Created attachment 81528 [details] [review]
Bug 21682: (follow-up) - DBUpdate for Dev installs

https://bugs.koha-community.org/show_bug.cgi?id=21682

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 16 Mark Tompsett 2018-10-29 17:15:04 UTC
Created attachment 81529 [details] [review]
Bug 21682: Follow up to use the default_exists check

This will make sure it is idempotent, and avoid issues if the DB has
already been corrected either through upgrade or manually before.
Comment 17 Martin Renvoize 2018-10-29 17:51:23 UTC
Wow.. thanks M.Tompsett
Comment 18 Jonathan Druart 2018-10-29 18:06:23 UTC
Why do not you use the same statement we always use? :)

  ALTER TABLE stockrotationrotas CHANGE COLUMN description description text;
Comment 19 Martin Renvoize 2018-10-31 09:56:07 UTC
I wasn't aware that was a statement 'we always use'
Comment 20 Jonathan Druart 2018-10-31 18:48:54 UTC
Created attachment 81769 [details] [review]
Bug 21682: Corrections for strict sql in stockrotationrotas

Test Plan:
1) Ensure you're database is prior to version 18.06.00.040
2) Ensure 'strict_sql_modes' is enabled
3) Run updatedatabase
4) See no errors during upgrade
5) Run a fresh install
6) Note no errors during database installation

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 21 Jonathan Druart 2018-10-31 18:48:59 UTC
Created attachment 81770 [details] [review]
Bug 21682: Add update DB entry for existing installs
Comment 22 Jonathan Druart 2018-10-31 18:49:29 UTC
I removed the need of the new subroutine, could you please test this version?
Comment 23 Martin Renvoize 2018-11-01 08:52:22 UTC
Created attachment 81787 [details] [review]
Bug 21682: Corrections for strict sql in stockrotationrotas

Test Plan:
1) Ensure you're database is prior to version 18.06.00.040
2) Ensure 'strict_sql_modes' is enabled
3) Run updatedatabase
4) See no errors during upgrade
5) Run a fresh install
6) Note no errors during database installation

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 24 Martin Renvoize 2018-11-01 08:52:25 UTC
Created attachment 81788 [details] [review]
Bug 21682: Add update DB entry for existing installs

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 25 Martin Renvoize 2018-11-01 08:52:41 UTC
Works for me
Comment 26 Jonathan Druart 2018-11-01 14:33:57 UTC
Created attachment 81815 [details] [review]
Bug 21682: Add update DB entry for existing installs

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 27 Jonathan Druart 2018-11-01 14:34:43 UTC
I amended the last patch to move atomicupdate/bug_21620.perl to atomicupdate/bug_21682.perl
Comment 28 Nick Clemens 2018-11-01 19:09:58 UTC
Awesome work all!

Pushed to master for 18.11
Comment 29 Martin Renvoize 2018-11-09 16:15:00 UTC
Bug 11897 not in 18.05.x series.
Comment 30 Marcel de Rooy 2022-04-19 14:45:15 UTC
(In reply to Jonathan Druart from comment #26)
> Created attachment 81815 [details] [review] [review]
> Bug 21682: Add update DB entry for existing installs
> 
> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

This patch removed the NOT NULL from the title field at upgrade time.
WIll open a report for it.
Comment 31 Marcel de Rooy 2022-04-19 14:46:57 UTC
(In reply to Marcel de Rooy from comment #30)
> (In reply to Jonathan Druart from comment #26)
> > Created attachment 81815 [details] [review] [review] [review]
> > Bug 21682: Add update DB entry for existing installs
> > 
> > Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
> 
> This patch removed the NOT NULL from the title field at upgrade time.
> WIll open a report for it.

Description field