From 76ef5af95be395a70106d01ff09d1130ac6139cb Mon Sep 17 00:00:00 2001 From: Jonathan Druart 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 ) %]

Updating database structure

- [% IF ( has_update_succeeds ) %] + [% IF success %]

Update report:

[% END %] - [% IF ( has_update_errors ) %] -

Update errors :

+ [% IF error %] +

Update error :

[% END %] - [% UNLESS ( has_update_errors ) %] + [% UNLESS error %]

Everything went okay. Update done.

[% END %]

Continue to log in to Koha

-- 2.20.1