Bug 34881 - Database update for bug 28854 isn't fully idempotent
Summary: Database update for bug 28854 isn't fully idempotent
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Installation and upgrade (command-line installer) (show other bugs)
Version: Main
Hardware: All All
: P5 - low major (vote)
Assignee: Emily Lamancusa
QA Contact: Testopia
URL:
Keywords:
Depends on: 28854
Blocks:
  Show dependency treegraph
 
Reported: 2023-09-22 09:29 UTC by Martin Renvoize
Modified: 2023-11-13 13: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:
23.11.00,23.05.05,22.11.12


Attachments
Bug 34881: Improve idempotency of DBRev (3.67 KB, patch)
2023-09-22 09:37 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 34881: [alternative patch] Improve idempotency of DBRev (2.97 KB, patch)
2023-10-11 13:47 UTC, Emily Lamancusa
Details | Diff | Splinter Review
Bug 34881: [alternative patch] Improve idempotency of DBRev (3.02 KB, patch)
2023-10-11 20:13 UTC, David Nind
Details | Diff | Splinter Review
Bug 34881: Improve idempotency of DBRev (3.05 KB, patch)
2023-10-12 14:55 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 34881: (QA follow-up) Remove stray paren (897 bytes, patch)
2023-10-12 14:55 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Renvoize 2023-09-22 09:29:15 UTC
The AV added as part of bug 28854 can be duplicated if the update is run more than once.
Comment 1 Martin Renvoize 2023-09-22 09:37:12 UTC
Created attachment 156047 [details] [review]
Bug 34881: Improve idempotency of DBRev
Comment 2 Marcel de Rooy 2023-09-22 09:47:50 UTC
I think this part

+            $dbh->do(qq{
+                INSERT IGNORE INTO systempreferences( `variable`, `value`, `options`, `explanation`, `type` )
+                VALUES
+                  ( 'BundleLostValue', $lost_val, '', 'Sets the LOST AV value that represents "Missing from bundle" as a lost value', 'Free' ),
+                  ( 'BundleNotLoanValue', $nfl_val, '', 'Sets the NOT_LOAN AV value that represents "Added to bundle" as a not for loan value', 'Free')
+            });
+
+            say $out "Added new system preference 'BundleLostValue'";
+            say $out "Added new system preference 'BundleNotLoanValue'";

should not be in the if you added.
Comment 3 Martin Renvoize 2023-09-27 15:27:49 UTC
It can't not be.. it depends on a variable added within the IF
Comment 4 David Nind 2023-09-27 18:30:24 UTC
Just checking on what is the best way to test this.

I tried these steps, but not sure if this is the best way:

1. Check the authorized values list for LOST and note that there is this value: 5 Missing from bundle

2. Run the database update (found this on another idempotent bug): perl -e 'use C4::Installer; warn Data::Dumper::Dumper(C4::Installer::run_db_rev("installer/data/mysql/db_revs/220600015.pl"));'

3. Note that for the LOST authorized value lists, the value is duplicated: 6 Missing from bundle

4. Reset everything: reset_all

5. Run the database update again (see step 2). This time I got this error when running the update (at least the value isn't duplicated!): 

perl -e 'use C4::Installer; warn Data::Dumper::Dumper(C4::Installer::run_db_rev("installer/data/mysql/db_revs/220600015.pl"));'
$VAR1 = {
          'error' => bless( {
                              'msg' => '{UNKNOWN}: DBI Exception: DBD::mysql::db selectrow_array failed: You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near \'\' at line 1 at /kohadevbox/koha/C4/Installer.pm line 741
'
                            }, 'DBIx::Class::Exception' ),
          'time' => '18:19:10',
          'exec_output' => 'Added new table \'item_bundles\'
',
          'bug_number' => '28854',
          'output' => [
                        'Upgrade to 22.06.00.015  [18:19:10]: Bug 28854 - Item bundles support',
                        '	Added new table \'item_bundles\''
                      ],
          'description' => 'Item bundles support',
          'version' => '22.06.00.015',
          'filepath' => 'installer/data/mysql/db_revs/220600015.pl'
Comment 5 Emily Lamancusa 2023-10-11 13:42:47 UTC
The subquery in line 24 of the patch is missing its close parenthesis at the end - that's probably what's causing the error.

I agree that it feels off to put the system preference revisions (and the not-for-loan AV revision for that matter) all inside of the IF statement that depends only on the Lost AV.

Can we safely assume that either the whole dbrev has been run, or none of it has been run? If so, why not put everything inside the if block that's already there:
10  if( !TableExists( 'item_bundles' ) ) {


If not, then shouldn't we check each thing separately? Uploading an alternative patch for how this could work...
Comment 6 Emily Lamancusa 2023-10-11 13:47:09 UTC
Created attachment 156823 [details] [review]
Bug 34881: [alternative patch] Improve idempotency of DBRev
Comment 7 David Nind 2023-10-11 20:13:43 UTC
Created attachment 156878 [details] [review]
Bug 34881: [alternative patch] Improve idempotency of DBRev

Signed-off-by: David Nind <david@davidnind.com>
Comment 8 David Nind 2023-10-11 20:27:29 UTC
Thanks for the alternative patch Emily! 

I've signed off as I don't get an error when running the database update.

If it shouldn't be signed off yet, please change the status back to needs sign-off.


Running the database update - first time
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Following the test plan in #c4 I no longer get an error message when I run the database update for the first time:

perl -e 'use C4::Installer; warn Data::Dumper::Dumper(C4::Installer::run_db_rev("installer/data/mysql/db_revs/220600015.pl"));'
$VAR1 = {
          'output' => [
                        'Upgrade to 22.06.00.015  [20:11:52]: Bug 28854 - Item bundles support',
                        '	Added new table \'item_bundles\'',
                        '	Added new system preference \'BundleLostValue\'',
                        '	Added new system preference \'BundleNotLoanValue\''
                      ],
          'filepath' => 'installer/data/mysql/db_revs/220600015.pl',
          'version' => '22.06.00.015',
          'exec_output' => 'Added new table \'item_bundles\'
Added new system preference \'BundleLostValue\'
Added new system preference \'BundleNotLoanValue\'
',
          'description' => 'Item bundles support',
          'bug_number' => '28854',
          'error' => undef,
          'time' => '20:11:52'


Running the database update - second time
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

perl -e 'use C4::Installer; warn Data::Dumper::Dumper(C4::Installer::run_db_rev("installer/data/mysql/db_revs/220600015.pl"));'
$VAR1 = {
          'output' => [
                        'Upgrade to 22.06.00.015  [20:11:52]: Bug 28854 - Item bundles support',
                        '	Added new table \'item_bundles\'',
                        '	Added new system preference \'BundleLostValue\'',
                        '	Added new system preference \'BundleNotLoanValue\''
                      ],
          'filepath' => 'installer/data/mysql/db_revs/220600015.pl',
          'version' => '22.06.00.015',
          'exec_output' => 'Added new table \'item_bundles\'
Added new system preference \'BundleLostValue\'
Added new system preference \'BundleNotLoanValue\'
',
          'description' => 'Item bundles support',
          'bug_number' => '28854',
          'error' => undef,
          'time' => '20:11:52'
        };



Running the database update - third and subsequent times
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

perl -e 'use C4::Installer; warn Data::Dumper::Dumper(C4::Installer::run_db_rev("installer/data/mysql/db_revs/220600015.pl"));'
$VAR1 = {
          'bug_number' => '28854',
          'exec_output' => 'Added new table \'item_bundles\'
Added new system preference \'BundleLostValue\'
Added new system preference \'BundleNotLoanValue\'
',
          'filepath' => 'installer/data/mysql/db_revs/220600015.pl',
          'error' => undef,
          'version' => '22.06.00.015',
          'time' => '20:15:11',
          'output' => [
                        'Upgrade to 22.06.00.015  [20:15:11]: Bug 28854 - Item bundles support',
                        '	Added new table \'item_bundles\'',
                        '	Added new system preference \'BundleLostValue\'',
                        '	Added new system preference \'BundleNotLoanValue\''
                      ],
          'description' => 'Item bundles support'
        };
Comment 9 Nick Clemens 2023-10-12 14:55:04 UTC
Created attachment 156978 [details] [review]
Bug 34881: Improve idempotency of DBRev

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 10 Nick Clemens 2023-10-12 14:55:06 UTC
Created attachment 156979 [details] [review]
Bug 34881: (QA follow-up) Remove stray paren

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 11 Tomás Cohen Arazi 2023-10-18 13:23:16 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 12 Fridolin Somers 2023-10-25 06:30:01 UTC
Pushed to 23.05.x for 23.05.05
Comment 13 Matt Blenkinsop 2023-11-13 13:46:06 UTC
Nice work everyone!

Pushed to oldstable for 22.11.x