Bug 21682 - Stock Rotation: Update DB is failing with strict_sql_modes ON
Summary: Stock Rotation: Update DB is failing with strict_sql_modes ON
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Database (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Martin Renvoize
QA Contact: Testopia
URL:
Keywords:
Depends on: 11897
Blocks: 21656
  Show dependency treegraph
 
Reported: 2018-10-26 10:50 UTC by Martin Renvoize
Modified: 2022-04-19 14:46 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 21682: Corrections for strict sql in stockrotationrotas (2.34 KB, patch)
2018-10-26 10:59 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21683: (follow-up) - DBUpdate for Dev installs (1.01 KB, patch)
2018-10-27 09:01 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21683: (follow-up) - DBUpdate for Dev installs (1.01 KB, patch)
2018-10-27 09:06 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21682: Prequel addition of default_exists (2.73 KB, patch)
2018-10-29 17:05 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 21682: Prequel addition of default_exists (2.73 KB, patch)
2018-10-29 17:14 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 21682: Corrections for strict sql in stockrotationrotas (2.39 KB, patch)
2018-10-29 17:14 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 21682: (follow-up) - DBUpdate for Dev installs (1.07 KB, patch)
2018-10-29 17:15 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 21682: Follow up to use the default_exists check (1.40 KB, patch)
2018-10-29 17:15 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 21682: Corrections for strict sql in stockrotationrotas (2.39 KB, patch)
2018-10-31 18:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21682: Add update DB entry for existing installs (1.05 KB, patch)
2018-10-31 18:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21682: Corrections for strict sql in stockrotationrotas (2.39 KB, patch)
2018-11-01 08:52 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21682: Add update DB entry for existing installs (1.12 KB, patch)
2018-11-01 08:52 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21682: Add update DB entry for existing installs (1.12 KB, patch)
2018-11-01 14:33 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 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