Bugzilla – Attachment 116209 Details for
Bug 25078
Update DB process - wrap each DBRev inside a transaction and better error handling
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 25078: [POC] Put the db entries in a structure to handle them better
Bug-25078-POC-Put-the-db-entries-in-a-structure-to.patch (text/plain), 21.06 KB, created by
Jonathan Druart
on 2021-02-02 12:05:54 UTC
(
hide
)
Description:
Bug 25078: [POC] Put the db entries in a structure to handle them better
Filename:
MIME Type:
Creator:
Jonathan Druart
Created:
2021-02-02 12:05:54 UTC
Size:
21.06 KB
patch
obsolete
>From a46f57943902659d06217382106eb6dc14ca1e74 Mon Sep 17 00:00:00 2001 >From: Jonathan Druart <jonathan.druart@bugs.koha-community.org> >Date: Fri, 29 Jan 2021 15:16:03 +0100 >Subject: [PATCH] Bug 25078: [POC] Put the db entries in a structure to handle > them better > >If we have have a hashref (ideally in a module!) we could have do >something more powerful and deal correctly with them. > >More work is needed: >- The "export" of the $db_entries hashref from updatedatabase.pl is >really dirty, we don't want to do that >- We should not need to eval { require $updatedatabase_path } from .pm, >but retrieve it from another pm > >The main problems with this approach is: >1. Someone will have to move all(?) the entries into the new >structure/file >2. Habit will change, updatedatabase.pl will become a 10 lines script >that will loop over the db entries and display the output > >In my opinion it's what we want. > >Don't be afraid by the diff size, most of the changes is a move of the >subroutine from updatedatabase.pl to C4/Installer.pm >--- > C4/Installer.pm | 182 +++++++++++- > installer/data/mysql/updatedatabase.pl | 258 +++++------------- > installer/install.pl | 47 +--- > .../prog/en/modules/installer/step3.tt | 25 +- > 4 files changed, 280 insertions(+), 232 deletions(-) > >diff --git a/C4/Installer.pm b/C4/Installer.pm >index a004cb8150..24aada3e7f 100644 >--- a/C4/Installer.pm >+++ b/C4/Installer.pm >@@ -19,6 +19,7 @@ package C4::Installer; > > use Modern::Perl; > >+use Try::Tiny; > use Encode qw( encode is_utf8 ); > use DBIx::RunSQL; > use YAML::Syck qw( LoadFile ); >@@ -30,7 +31,7 @@ use vars qw(@ISA @EXPORT); > BEGIN { > require Exporter; > @ISA = qw( Exporter ); >- push @EXPORT, qw( primary_key_exists foreign_key_exists index_exists column_exists TableExists); >+ push @EXPORT, qw( primary_key_exists foreign_key_exists index_exists column_exists TableExists TransformToNum CheckVersion update ); > }; > > =head1 NAME >@@ -683,6 +684,185 @@ sub TableExists { # Could be renamed table_exists for consistency > return 0; > } > >+sub execute_entry { >+ my ( $DBversion, $bug_number, $description, $sub ) = @_; >+ >+ my $error; >+ try { >+ Koha::Database->schema->txn_do($sub); >+ } catch { >+ $error = $_; >+ }; >+ >+ return $error; >+} >+ >+sub update { >+ my ( $db_entries ) = @_; >+ >+ my ( @done, @errors ); >+ for my $db_entry ( @$db_entries ) { >+ >+ my $version = $db_entry->{version}; >+ next unless CheckVersion( $version ); >+ >+ my $error = execute_entry( >+ $version, >+ $db_entry->{bug_number}, >+ $db_entry->{description}, >+ $db_entry->{sub} >+ ); >+ >+ $db_entry->{time} = POSIX::strftime("%H:%M:%S",localtime); >+ $db_entry->{output} = output_version( { %$db_entry, done => !$error } ); >+ if ( $error ) { >+ push @errors, { %$db_entry, error => $error }; >+ last; # We stop the update if an error occurred! >+ } >+ >+ SetVersion($version); >+ push @done, $db_entry; >+ } >+ return { success => \@done, error => \@errors }; >+} >+ >+sub output_version { >+ my ( $db_entry) = @_; >+ >+ my $descriptions = $db_entry->{description}; >+ my $DBversion = $db_entry->{version}; >+ my $bug_number = $db_entry->{bug_number}; >+ my $time = $db_entry->{time}; >+ my $done = $db_entry->{done} ? "done" : "failed"; >+ >+ unless ( ref($descriptions) ) { >+ $descriptions = [ $descriptions ]; >+ } >+ >+ my $first = 1; >+ my $output; >+ for my $description ( @$descriptions ) { >+ if ( @$descriptions > 1 ) { >+ if ( $first ) { >+ unless ( $bug_number ) { >+ $output = sprintf "Upgrade to %s %s [%s]: %s", $DBversion, $done, $time, $description; >+ } else { >+ $output = sprintf "Upgrade to %s %s [%s]: Bug %5s - %s", $DBversion, $done, $time, $bug_number, $description; >+ } >+ } else { >+ $output = sprintf "\t\t\t\t\t\t - %s", $description; >+ } >+ } else { >+ unless ( $bug_number ) { >+ $output = sprintf "Upgrade to %s %s [%s]: %s", $DBversion, $done, $time, $description; >+ } else { >+ $output = sprintf "Upgrade to %s %s [%s]: Bug %5s - %s", $DBversion, $done, $time, $bug_number, $description; >+ } >+ } >+ $first = 0; >+ } >+ return $output; >+} >+ >+=head2 DropAllForeignKeys($table) >+ >+Drop all foreign keys of the table $table >+ >+=cut >+ >+sub DropAllForeignKeys { >+ my ($table) = @_; >+ # get the table description >+ my $dbh = C4::Context->dbh; >+ my $sth = $dbh->prepare("SHOW CREATE TABLE $table"); >+ $sth->execute; >+ my $vsc_structure = $sth->fetchrow; >+ # split on CONSTRAINT keyword >+ my @fks = split /CONSTRAINT /,$vsc_structure; >+ # parse each entry >+ foreach (@fks) { >+ # isolate what is before FOREIGN KEY, if there is something, it's a foreign key to drop >+ $_ = /(.*) FOREIGN KEY.*/; >+ my $id = $1; >+ if ($id) { >+ # we have found 1 foreign, drop it >+ $dbh->do("ALTER TABLE $table DROP FOREIGN KEY $id"); >+ $id=""; >+ } >+ } >+} >+ >+ >+=head2 TransformToNum >+ >+Transform the Koha version from a 4 parts string >+to a number, with just 1 . >+ >+=cut >+ >+sub TransformToNum { >+ my $version = shift; >+ # remove the 3 last . to have a Perl number >+ $version =~ s/(.*\..*)\.(.*)\.(.*)/$1$2$3/; >+ # three X's at the end indicate that you are testing patch with dbrev >+ # change it into 999 >+ # prevents error on a < comparison between strings (should be: lt) >+ $version =~ s/XXX$/999/; >+ return $version; >+} >+ >+=head2 SetVersion >+ >+set the DBversion in the systempreferences >+ >+=cut >+ >+sub SetVersion { >+ return if $_[0]=~ /XXX$/; >+ #you are testing a patch with a db revision; do not change version >+ my $kohaversion = TransformToNum($_[0]); >+ my $dbh = C4::Context->dbh; >+ if (C4::Context->preference('Version')) { >+ my $finish=$dbh->prepare("UPDATE systempreferences SET value=? WHERE variable='Version'"); >+ $finish->execute($kohaversion); >+ } else { >+ my $finish=$dbh->prepare("INSERT into systempreferences (variable,value,explanation) values ('Version',?,'The Koha database version. WARNING: Do not change this value manually, it is maintained by the webinstaller')"); >+ $finish->execute($kohaversion); >+ } >+ C4::Context::clear_syspref_cache(); # invalidate cached preferences >+} >+ >+sub NewVersion { >+ # void FIXME replace me >+} >+ >+=head2 CheckVersion >+ >+Check whether a given update should be run when passed the proposed version >+number. The update will always be run if the proposed version is greater >+than the current database version and less than or equal to the version in >+kohaversion.pl. The update is also run if the version contains XXX, though >+this behavior will be changed following the adoption of non-linear updates >+as implemented in bug 7167. >+ >+=cut >+ >+sub CheckVersion { >+ my ($proposed_version) = @_; >+ my $version_number = TransformToNum($proposed_version); >+ >+ # The following line should be deleted when bug 7167 is pushed >+ return 1 if ( $proposed_version =~ m/XXX/ ); >+ >+ if ( C4::Context->preference("Version") < $version_number >+ && $version_number <= TransformToNum( $Koha::VERSION ) ) >+ { >+ return 1; >+ } >+ else { >+ return 0; >+ } >+} > > =head1 AUTHOR > >diff --git a/installer/data/mysql/updatedatabase.pl b/installer/data/mysql/updatedatabase.pl >index b9c7041570..137b960813 100755 >--- a/installer/data/mysql/updatedatabase.pl >+++ b/installer/data/mysql/updatedatabase.pl >@@ -47,8 +47,6 @@ use MARC::File::XML ( BinaryEncoding => 'utf8' ); > use File::Path qw[remove_tree]; # perl core module > use File::Slurp; > >-use Try::Tiny; >- > # FIXME - The user might be installing a new database, so can't rely > # on /etc/koha.conf anyway. > >@@ -23419,74 +23417,96 @@ if( CheckVersion( $DBversion ) ) { > NewVersion( $DBversion, "", "Koha 20.11.00 release" ); > } > >-$DBversion = '20.12.00.000'; >-if( CheckVersion( $DBversion ) ) { >- NewVersion( $DBversion, "", "Sorry, this is my first life, I am still learning!" ); >-} >- >-$DBversion = '20.12.00.001'; >-if( CheckVersion( $DBversion ) ) { >- $dbh->do(q{ >+our $db_entries = [ >+ { >+ version => '20.12.00.000', >+ bug_number => undef, >+ description => "Sorry, this is my first life, I am still learning!", >+ }, >+ { >+ version => '20.12.00.001', >+ bug_number => 27252, >+ description => "Add ElasticsearchCrossFields system preference", >+ sub => sub { >+ $dbh->do( >+ q{ > INSERT IGNORE INTO systempreferences ( `variable`, `value`, `options`, `explanation`, `type` ) VALUES > ('ElasticsearchCrossFields', '1', '', 'Enable "cross_fields" option for searches using Elastic search.', 'YesNo') >- }); >- NewVersion( $DBversion, 27252, "Add ElasticsearchCrossFields system preference"); >-} >- >-$DBversion = '20.12.00.002'; >-if( CheckVersion( $DBversion ) ) { >- $dbh->do(q{UPDATE systempreferences SET `type` = 'Choice' WHERE `variable` = 'UsageStatsCountry'}); >- NewVersion( $DBversion, 27351, "Set type for UsageStatsCountry to Choice"); >-} >- >-$DBversion = '20.12.00.003'; >-if( CheckVersion( $DBversion ) ) { >- $dbh->do(q{UPDATE systempreferences SET `type` = 'Choice' WHERE `variable` = 'Mana'}); >- NewVersion( $DBversion, 27349, "Update type for Mana system preference to Choice"); >-} >- >-$DBversion = '20.12.00.004'; >-if( CheckVersion( $DBversion ) ) { >- $dbh->do(q{UPDATE systempreferences set variable="TaxRates" WHERE variable="gist"}); >- NewVersion( $DBversion, 27485, "Rename system preference 'gist' to 'TaxRates'"); >-} >- >-$DBversion = '20.12.00.005'; >-if( CheckVersion( $DBversion ) ) { >- execute_entry( >- $DBversion, 27491, "Rename system preference 'opaclanguages' to 'OPACLanguages'", >- sub { >+ } >+ ); >+ } >+ }, >+ { >+ version => '20.12.00.002', >+ bug_number => 27351, >+ description => "Set type for UsageStatsCountry to Choice", >+ sub => sub { > $dbh->do( >- q{UPDATE systempreferences set variable="OPACLanguages" WHERE variable="opaclanguages"} >+q{UPDATE systempreferences SET `type` = 'Choice' WHERE `variable` = 'UsageStatsCountry'} > ); > } >- ); >-} >+ }, >+ { >+ version => '20.12.00.003', >+ bug_number => 27349, >+ description => "Update type for Mana system preference to Choice", >+ sub => sub { >+ $dbh->do( >+q{UPDATE systempreferences SET `type` = 'Choice' WHERE `variable` = 'Mana'} >+ ); >+ } >+ }, > >-$DBversion = '20.12.00.006'; >-if( CheckVersion( $DBversion ) ) { >- execute_entry( $DBversion, 12345, "Testing failure", >- sub { >+ { >+ version => '20.12.00.004', >+ bug_number => 27485, >+ description => "Rename system preference 'gist' to 'TaxRates'", >+ sub => sub { >+ $dbh->do( >+q{UPDATE systempreferences set variable="TaxRates" WHERE variable="gist"} >+ ); >+ }, >+ }, >+ >+ { >+ version => '20.12.00.005', >+ bug_number => 27491, >+ description => "Rename system preference 'opaclanguages' to 'OPACLanguages'", >+ sub => sub { > $dbh->do( >- q{ALTER TABLE foo} >+q{UPDATE systempreferences set variable="OPACLanguages" WHERE variable="opaclanguages"} > ); > } >- ); >-} > >+ }, >+ { >+ version => '20.12.00.006', >+ bug_number => 42424, >+ description => "Testing failure", >+ sub => sub { >+ $dbh->do(q{ALTER TABLE foo}); >+ } >+ >+ }, > >-sub execute_entry { >- my ( $DBversion, $bug_number, $description, $sub ) = @_; >+]; > >- try { >- Koha::Database->schema->txn_do($sub); >- } catch { >- die sprintf "Something wrong happened during %s (%s):\n%s", $DBversion, $description, $_; >- }; > >- NewVersion( $DBversion, $bug_number, $description ); >+sub get_db_entries { return $db_entries; } >+ >+unless ( $ENV{HTTP_HOST} ) { # Is that correct? >+ my $report = update( $db_entries ); >+ >+ for my $s ( @{ $report->{success} } ) { >+ say $s->{output}; >+ } >+ for my $e ( @{ $report->{error} } ) { >+ say $e->{output}; >+ say "ERROR - " . $e->{error}; >+ } > } > >+ > # SEE bug 13068 > # if there is anything in the atomicupdate, read and execute it. > my $update_dir = C4::Context->config('intranetdir') . '/installer/data/mysql/atomicupdate/'; >@@ -23505,132 +23525,4 @@ foreach my $file ( sort readdir $dirh ) { > } > } > >-=head1 FUNCTIONS >- >-=head2 DropAllForeignKeys($table) >- >-Drop all foreign keys of the table $table >- >-=cut >- >-sub DropAllForeignKeys { >- my ($table) = @_; >- # get the table description >- my $sth = $dbh->prepare("SHOW CREATE TABLE $table"); >- $sth->execute; >- my $vsc_structure = $sth->fetchrow; >- # split on CONSTRAINT keyword >- my @fks = split /CONSTRAINT /,$vsc_structure; >- # parse each entry >- foreach (@fks) { >- # isolate what is before FOREIGN KEY, if there is something, it's a foreign key to drop >- $_ = /(.*) FOREIGN KEY.*/; >- my $id = $1; >- if ($id) { >- # we have found 1 foreign, drop it >- $dbh->do("ALTER TABLE $table DROP FOREIGN KEY $id"); >- $id=""; >- } >- } >-} >- >- >-=head2 TransformToNum >- >-Transform the Koha version from a 4 parts string >-to a number, with just 1 . >- >-=cut >- >-sub TransformToNum { >- my $version = shift; >- # remove the 3 last . to have a Perl number >- $version =~ s/(.*\..*)\.(.*)\.(.*)/$1$2$3/; >- # three X's at the end indicate that you are testing patch with dbrev >- # change it into 999 >- # prevents error on a < comparison between strings (should be: lt) >- $version =~ s/XXX$/999/; >- return $version; >-} >- >-=head2 SetVersion >- >-set the DBversion in the systempreferences >- >-=cut >- >-sub SetVersion { >- return if $_[0]=~ /XXX$/; >- #you are testing a patch with a db revision; do not change version >- my $kohaversion = TransformToNum($_[0]); >- if (C4::Context->preference('Version')) { >- my $finish=$dbh->prepare("UPDATE systempreferences SET value=? WHERE variable='Version'"); >- $finish->execute($kohaversion); >- } else { >- my $finish=$dbh->prepare("INSERT into systempreferences (variable,value,explanation) values ('Version',?,'The Koha database version. WARNING: Do not change this value manually, it is maintained by the webinstaller')"); >- $finish->execute($kohaversion); >- } >- C4::Context::clear_syspref_cache(); # invalidate cached preferences >-} >- >-sub NewVersion { >- my ( $DBversion, $bug_number, $descriptions ) = @_; >- >- SetVersion($DBversion); >- >- unless ( ref($descriptions) ) { >- $descriptions = [ $descriptions ]; >- } >- my $first = 1; >- my $time = POSIX::strftime("%H:%M:%S",localtime); >- for my $description ( @$descriptions ) { >- if ( @$descriptions > 1 ) { >- if ( $first ) { >- unless ( $bug_number ) { >- say sprintf "Upgrade to %s done [%s]: %s", $DBversion, $time, $description; >- } else { >- say sprintf "Upgrade to %s done [%s]: Bug %5s - %s", $DBversion, $time, $bug_number, $description; >- } >- } else { >- say sprintf "\t\t\t\t\t\t - %s", $description; >- } >- } else { >- unless ( $bug_number ) { >- say sprintf "Upgrade to %s done [%s]: %s", $DBversion, $time, $description; >- } else { >- say sprintf "Upgrade to %s done [%s]: Bug %5s - %s", $DBversion, $time, $bug_number, $description; >- } >- } >- $first = 0; >- } >-} >- >-=head2 CheckVersion >- >-Check whether a given update should be run when passed the proposed version >-number. The update will always be run if the proposed version is greater >-than the current database version and less than or equal to the version in >-kohaversion.pl. The update is also run if the version contains XXX, though >-this behavior will be changed following the adoption of non-linear updates >-as implemented in bug 7167. >- >-=cut >- >-sub CheckVersion { >- my ($proposed_version) = @_; >- my $version_number = TransformToNum($proposed_version); >- >- # The following line should be deleted when bug 7167 is pushed >- return 1 if ( $proposed_version =~ m/XXX/ ); >- >- if ( C4::Context->preference("Version") < $version_number >- && $version_number <= TransformToNum( $Koha::VERSION ) ) >- { >- return 1; >- } >- else { >- return 0; >- } >-} >- > exit; >diff --git a/installer/install.pl b/installer/install.pl >index be6ee27f8c..2386625e27 100755 >--- a/installer/install.pl >+++ b/installer/install.pl >@@ -394,41 +394,18 @@ elsif ( $step && $step == 3 ) { > chk_log( $logdir, "updatedatabase-error_$filename_suffix" ) > ); > >- my $cmd = C4::Context->config("intranetdir") >- . "/installer/data/$info{dbms}/updatedatabase.pl >> $logfilepath 2>> $logfilepath_errors"; >- >- system($cmd ); >- >- my $fh; >- open( $fh, "<:encoding(utf-8)", $logfilepath ) >- or die "Cannot open log file $logfilepath: $!"; >- my @report = <$fh>; >- close $fh; >- if (@report) { >- $template->param( update_report => >- [ map { { line => $_ =~ s/\t/  /gr } } split( /\n/, join( '', @report ) ) ] >- ); >- $template->param( has_update_succeeds => 1 ); >- } >- else { >- eval { `rm $logfilepath` }; >- } >- open( $fh, "<:encoding(utf-8)", $logfilepath_errors ) >- or die "Cannot open log file $logfilepath_errors: $!"; >- @report = <$fh>; >- close $fh; >- if (@report) { >- $template->param( update_errors => >- [ map { { line => $_ } } split( /\n/, join( '', @report ) ) ] >- ); >- $template->param( has_update_errors => 1 ); >- warn >-"The following errors were returned while attempting to run the updatedatabase.pl script:\n"; >- foreach my $line (@report) { warn "$line\n"; } >- } >- else { >- eval { `rm $logfilepath_errors` }; >- } >+ my $updatedatabase_path = C4::Context->config("intranetdir") >+ . "/installer/data/$info{dbms}/updatedatabase.pl"; >+ >+ eval { require $updatedatabase_path }; >+ my $db_entries = get_db_entries(); >+ my $report = update( $db_entries ); >+ >+ # FIXME restore log to logfilepath and logfilepath_errors >+ >+ $template->param( success => $report->{success}, error => $report->{error} ); >+ >+ #warn "The following errors were returned while attempting to run the updatedatabase.pl script:\n"; #FIXME restore this > $template->param( $op => 1 ); > } > else { >diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/installer/step3.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/installer/step3.tt >index 370c1646b5..53b42fbb0c 100644 >--- a/koha-tmpl/intranet-tmpl/prog/en/modules/installer/step3.tt >+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/installer/step3.tt >@@ -248,28 +248,27 @@ > > [% IF ( updatestructure ) %] > <h2>Updating database structure</h2> >- [% IF ( has_update_succeeds ) %] >+ [% IF success %] > <p>Update report:</p> > <ul> >- [% FOREACH l IN update_report %] >- [% SET line = l.line %] >- [% IF line.match('^Upgrade to') %] >- <li>[% line | $raw %]</li> >- [% ELSE %] >- [% line | $raw %]<br/> >- [% END %] >+ [% FOR s IN success %] >+ <li>[% s.output | html %]</li> > [% END %] > </ul> > [% END %] >- [% IF ( has_update_errors ) %] >- <p>Update errors :</p> >+ [% IF error %] >+ <p>Update error :</p> > <ul> >- [% FOREACH update_error IN update_errors %] >- <li class="update_error">[% update_error.line | html %]</li> >+ [% FOR e IN error %] >+ <li class="update_error"> >+ [% e.output | html %] >+ <br/> >+ ERROR: [% e.error | html %] >+ </li> > [% END %] > </ul> > [% END %] >- [% UNLESS ( has_update_errors ) %] >+ [% UNLESS error %] > <p>Everything went okay. Update done.</p> > [% END %] > <p><a href="install.pl?step=3&op=finished" class="btn btn-primary">Continue to log in to Koha</a></p> >-- >2.20.1
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 25078
:
102521
|
116032
|
116033
|
116034
|
116208
|
116209
|
117084
|
121909
|
121910
|
121911
|
121982
|
121983
|
121984
|
122208
|
122322
|
122323
|
122324
|
122325
|
122326
|
122329
|
122665
|
122666
|
122667
|
122668
|
122669
|
122670
|
122671
|
122679
|
122680
|
122875
|
123356
|
123357
|
123358
|
123359
|
123360
|
123361
|
123362
|
123363
|
123364
|
123365
|
123366
|
123367
|
123850
|
123851
|
123853
|
123937
|
124269