From 43af3479fa7e74ecdc13733824e56d9d9994cdd6 Mon Sep 17 00:00:00 2001 From: Glenn Rice Date: Thu, 19 Dec 2024 10:04:22 -0600 Subject: [PATCH] Add the capability of changing database field types when upgrading the database. This is not done by default. There are check boxes that can be selected to do this when upgrading a course. Since this process can be risky there are ample warnings recommending that an archive be made before upgrading and changing field types. Also remove the `fieldOverride` usage. There are no field overrides anymore, and the current SQL::Abstract code no longer even supports it. --- bin/upgrade-database-to-utf8mb4.pl | 29 +++-- lib/WeBWorK/ContentGenerator/CourseAdmin.pm | 113 ++++++++++++++---- lib/WeBWorK/DB/Schema/NewSQL/Std.pm | 25 +++- lib/WeBWorK/DB/Schema/NewSQL/Versioned.pm | 2 - lib/WeBWorK/Utils/CourseDBIntegrityCheck.pm | 37 +++++- lib/WeBWorK/Utils/CourseManagement.pm | 13 +- .../archive_course_confirm.html.ep | 20 +++- .../CourseAdmin/rename_course_confirm.html.ep | 20 +++- .../upgrade_course_confirm.html.ep | 9 ++ 9 files changed, 206 insertions(+), 62 deletions(-) diff --git a/bin/upgrade-database-to-utf8mb4.pl b/bin/upgrade-database-to-utf8mb4.pl index 4765ce28db..3751a2fd30 100755 --- a/bin/upgrade-database-to-utf8mb4.pl +++ b/bin/upgrade-database-to-utf8mb4.pl @@ -165,7 +165,7 @@ BEGIN my $dbuser = shell_quote($ce->{database_username}); my $dbpass = $ce->{database_password}; -$ENV{'MYSQL_PWD'} = $dbpass; +local $ENV{MYSQL_PWD} = $dbpass; if (!$no_backup) { # Backup the database @@ -212,7 +212,7 @@ BEGIN }, ); -my $db = new WeBWorK::DB($ce->{dbLayouts}{ $ce->{dbLayoutName} }); +my $db = WeBWorK::DB->new($ce->{dbLayouts}{ $ce->{dbLayoutName} }); my @table_types = sort(grep { !$db->{$_}{params}{non_native} } keys %$db); sub checkAndUpdateTableColumnTypes { @@ -222,29 +222,30 @@ sub checkAndUpdateTableColumnTypes { print "\tChecking '$table' (pass $pass)\n" if $verbose; my $schema_field_data = $db->{$table_type}{record}->FIELD_DATA; - for my $field (keys %$schema_field_data) { - my $field_name = $db->{$table_type}{params}{fieldOverride}{$field} || $field; - my @name_type = @{ + for my $field_name (keys %$schema_field_data) { + my @name_type = @{ $dbh->selectall_arrayref( "SELECT COLUMN_TYPE FROM INFORMATION_SCHEMA.COLUMNS " . "WHERE TABLE_SCHEMA='$dbname' AND TABLE_NAME='$table' AND COLUMN_NAME='$field_name';" ) }; - print("\t\tThe '$field_name' column is missing from '$table'.\n" - . "\t\tYou should upgrade the course via course administration to fix this.\n" - . "\t\tYou may need to run this script again after doing that.\n"), next - if !exists($name_type[0][0]); + if (!exists($name_type[0][0])) { + print("\t\tThe '$field_name' column is missing from '$table'.\n" + . "\t\tYou should upgrade the course via course administration to fix this.\n" + . "\t\tYou may need to run this script again after doing that.\n"); + next; + } my $data_type = $name_type[0][0]; next if !$data_type; $data_type =~ s/\(\d*\)$// if $data_type =~ /^(big|small)?int\(\d*\)$/; $data_type = lc($data_type); - my $schema_data_type = lc($schema_field_data->{$field}{type} =~ s/ .*$//r); + my $schema_data_type = lc($schema_field_data->{$field_name}{type} =~ s/ .*$//r); if ($data_type ne $schema_data_type) { print "\t\tUpdating data type for column '$field_name' in table '$table'\n" if $verbose; print "\t\t\t$data_type -> $schema_data_type\n" if $verbose; - eval { $dbh->do("ALTER TABLE `$table` MODIFY $field_name $schema_field_data->{$field}{type};"); }; + eval { $dbh->do("ALTER TABLE `$table` MODIFY $field_name $schema_field_data->{$field_name}{type};"); }; my $indent = $verbose ? "\t\t" : ""; die("${indent}Failed to modify '$field_name' in '$table' from '$data_type' to '$schema_data_type.\n" . "${indent}It is recommended that you restore a database backup. Make note of the\n" @@ -286,8 +287,10 @@ sub checkAndChangeTableCharacterSet { my $error = 0; for my $course (@courses) { - print("The course '$course' does not exist on the server\n"), next - if !grep($course eq $_, @server_courses); + if (!grep { $course eq $_ } @server_courses) { + print("The course '$course' does not exist on the server\n"); + next; + } print "Checking tables for '$course'\n" if $verbose; for my $table_type (@table_types) { diff --git a/lib/WeBWorK/ContentGenerator/CourseAdmin.pm b/lib/WeBWorK/ContentGenerator/CourseAdmin.pm index fdb4463636..63e14daf58 100644 --- a/lib/WeBWorK/ContentGenerator/CourseAdmin.pm +++ b/lib/WeBWorK/ContentGenerator/CourseAdmin.pm @@ -1339,7 +1339,7 @@ sub upgrade_course_confirm ($c) { my @upgrade_courseIDs = $c->param('upgrade_courseIDs'); - my ($extra_database_tables_exist, $extra_database_fields_exist) = (0, 0); + my ($extra_database_tables_exist, $extra_database_fields_exist, $incorrect_type_database_fields_exist) = (0, 0, 0); my $status_output = $c->c; @@ -1354,8 +1354,9 @@ sub upgrade_course_confirm ($c) { # Report on database status my ($tables_ok, $dbStatus) = $CIchecker->checkCourseTables($upgrade_courseID); - my ($all_tables_ok, $extra_database_tables, $extra_database_fields, $rebuild_table_indexes, $db_report) = - $c->formatReportOnDatabaseTables($dbStatus, $upgrade_courseID); + my ($all_tables_ok, $extra_database_tables, $extra_database_fields, $rebuild_table_indexes, + $incorrect_type_database_fields, $db_report) + = $c->formatReportOnDatabaseTables($dbStatus, $upgrade_courseID); my $course_output = $c->c; @@ -1428,6 +1429,24 @@ sub upgrade_course_confirm ($c) { ); } + if ($incorrect_type_database_fields) { + $incorrect_type_database_fields_exist = 1; + push( + @$course_output, + $c->tag( + 'p', + class => 'text-danger fw-bold', + $c->maketext( + 'There are database fields that do not have the same type as the field defined in the schema ' + . 'for at least one table. Check the checkbox by the field to change its type when ' + . 'upgrading the course. Warning: This can fail which may corrupt the table. If you have ' + . 'not archived this course, then do that now before upgrading if you want to change the ' + . 'type of any of these fields.' + ) + ) + ); + } + # Report on directory status my ($directories_ok, $directory_report) = checkCourseDirectories($ce2); push(@$course_output, $c->tag('div', class => 'mb-2', $c->maketext('Directory structure:'))); @@ -1464,10 +1483,11 @@ sub upgrade_course_confirm ($c) { return $c->include( 'ContentGenerator/CourseAdmin/upgrade_course_confirm', - upgrade_courseIDs => \@upgrade_courseIDs, - extra_database_tables_exist => $extra_database_tables_exist, - extra_database_fields_exist => $extra_database_fields_exist, - status_output => $status_output->join('') + upgrade_courseIDs => \@upgrade_courseIDs, + extra_database_tables_exist => $extra_database_tables_exist, + extra_database_fields_exist => $extra_database_fields_exist, + incorrect_type_database_fields_exist => $incorrect_type_database_fields_exist, + status_output => $status_output->join('') ); } @@ -1503,8 +1523,10 @@ sub do_upgrade_course ($c) { push( @upgrade_report, $CIchecker->updateTableFields( - $upgrade_courseID, $table_name, - [ ($c->param("$upgrade_courseID.$table_name.delete_fieldIDs")) ] + $upgrade_courseID, + $table_name, + [ ($c->param("$upgrade_courseID.$table_name.delete_fieldIDs")) ], + [ ($c->param("$upgrade_courseID.$table_name.fix_type_fieldIDs")) ], ) ); } @@ -1512,8 +1534,9 @@ sub do_upgrade_course ($c) { # Analyze database status and prepare status report ($tables_ok, $dbStatus) = $CIchecker->checkCourseTables($upgrade_courseID); - my ($all_tables_ok, $extra_database_tables, $extra_database_fields, $rebuild_table_indexes, $db_report) = - $c->formatReportOnDatabaseTables($dbStatus); + my ($all_tables_ok, $extra_database_tables, $extra_database_fields, $rebuild_table_indexes, + $incorrect_type_database_fields, $db_report) + = $c->formatReportOnDatabaseTables($dbStatus); # Prepend course name $db_report = $c->c($c->tag('div', class => 'mb-2', $c->maketext('Database:')), $db_report); @@ -1541,6 +1564,20 @@ sub do_upgrade_course ($c) { ); } + if ($incorrect_type_database_fields) { + push( + @$db_report, + $c->tag( + 'p', + class => 'text-danger fw-bold', + $c->maketext( + 'There are database fields that do not have the same type as the ' + . 'field defined in the schema for at least one table.' + ) + ) + ); + } + # Add missing directories and prepare report on directory status my $dir_update_messages = updateCourseDirectories($ce2); # Needs more error messages my ($directories_ok, $directory_report) = checkCourseDirectories($ce2); @@ -2684,10 +2721,11 @@ sub formatReportOnDatabaseTables ($c, $dbStatus, $courseID = undef) { ) ); - my $all_tables_ok = 1; - my $extra_database_tables = 0; - my $extra_database_fields = 0; - my $rebuild_table_indexes = 0; + my $all_tables_ok = 1; + my $extra_database_tables = 0; + my $extra_database_fields = 0; + my $rebuild_table_indexes = 0; + my $incorrect_type_database_fields = 0; my $db_report = $c->c; @@ -2728,9 +2766,35 @@ sub formatReportOnDatabaseTables ($c, $dbStatus, $courseID = undef) { } else { $extra_database_fields = 1; } - if ($fieldInfo{$key}[1]) { - push(@$field_report, $c->hidden_field("$courseID.$table.delete_fieldIDs" => $key)); - } else { + if (defined $courseID) { + if ($fieldInfo{$key}[1]) { + push(@$field_report, $c->hidden_field("$courseID.$table.delete_fieldIDs" => $key)); + } else { + push( + @$field_report, + $c->tag( + 'span', + class => 'form-check d-inline-block', + $c->tag( + 'label', + class => 'form-check-label', + $c->c( + $c->check_box( + "$courseID.$table.delete_fieldIDs" => $key, + class => 'form-check-input' + ), + $c->maketext('Delete field when upgrading') + )->join('') + ) + ) + ); + } + } + } elsif ($field_status == WeBWorK::Utils::CourseDBIntegrityCheck::ONLY_IN_A) { + $all_tables_ok = 0; + } elsif ($field_status == WeBWorK::Utils::CourseDBIntegrityCheck::DIFFER_IN_A_AND_B) { + $incorrect_type_database_fields = 1; + if (defined $courseID) { push( @$field_report, $c->tag( @@ -2741,17 +2805,15 @@ sub formatReportOnDatabaseTables ($c, $dbStatus, $courseID = undef) { class => 'form-check-label', $c->c( $c->check_box( - "$courseID.$table.delete_fieldIDs" => $key, - class => 'form-check-input' + "$courseID.$table.fix_type_fieldIDs" => $key, + class => 'form-check-input' ), - $c->maketext('Delete field when upgrading') + $c->maketext('Change type of field when upgrading') )->join('') ) ) ); } - } elsif ($field_status == WeBWorK::Utils::CourseDBIntegrityCheck::ONLY_IN_A) { - $all_tables_ok = 0; } push(@$fields_report, $c->tag('li', $field_report->join(''))); } @@ -2765,8 +2827,9 @@ sub formatReportOnDatabaseTables ($c, $dbStatus, $courseID = undef) { push(@$db_report, $c->tag('p', class => 'text-success', $c->maketext('Database tables are ok'))) if $all_tables_ok; return ( - $all_tables_ok, $extra_database_tables, $extra_database_fields, - $rebuild_table_indexes, $db_report->join('') + $all_tables_ok, $extra_database_tables, $extra_database_fields, $rebuild_table_indexes, + $incorrect_type_database_fields, + $db_report->join('') ); } diff --git a/lib/WeBWorK/DB/Schema/NewSQL/Std.pm b/lib/WeBWorK/DB/Schema/NewSQL/Std.pm index 497db424df..794713200c 100644 --- a/lib/WeBWorK/DB/Schema/NewSQL/Std.pm +++ b/lib/WeBWorK/DB/Schema/NewSQL/Std.pm @@ -368,6 +368,24 @@ sub _drop_column_field_stmt { return "Alter table `$sql_table_name` drop column `$sql_field_name` "; } +#################################################### +# Change the type of a column to the type defined in the schema +#################################################### + +sub change_column_field_type { + my ($self, $field_name) = @_; + return 0 unless defined $self->{record}->FIELD_DATA->{$field_name}; + eval { + $self->dbh->do('ALTER TABLE `' + . $self->sql_table_name + . '` MODIFY ' + . $self->sql_field_name($field_name) . ' ' + . $self->{record}->FIELD_DATA->{$field_name}{type} + . ';'); + }; + return $@ ? 0 : 1; +} + #################################################### # rebuild indexes for the table #################################################### @@ -375,9 +393,8 @@ sub _drop_column_field_stmt { sub rebuild_indexes { my ($self) = @_; - my $sql_table_name = $self->sql_table_name; - my $field_data = $self->field_data; - my %override_fields = reverse %{ $self->{params}{fieldOverride} }; + my $sql_table_name = $self->sql_table_name; + my $field_data = $self->field_data; # A key field column is going to be removed. The schema will not have the information for this column. So the # indexes need to be obtained from the database. Note that each element of the returned array is an array reference @@ -398,7 +415,7 @@ sub rebuild_indexes { my $column = (grep { $index->[4] eq $_->[0] } @$columns)[0]; if (defined $column && $column->[5] =~ m/AUTO_INCREMENT/i) { $self->dbh->do("ALTER TABLE `$sql_table_name` MODIFY `$column->[0]` $column->[1]"); - push @auto_increment_fields, $override_fields{ $column->[0] } // $column->[0]; + push @auto_increment_fields, $column->[0]; } $self->dbh->do("ALTER TABLE `$sql_table_name` DROP INDEX `$index->[2]`"); diff --git a/lib/WeBWorK/DB/Schema/NewSQL/Versioned.pm b/lib/WeBWorK/DB/Schema/NewSQL/Versioned.pm index 6f3e3f8a23..c91808dff0 100644 --- a/lib/WeBWorK/DB/Schema/NewSQL/Versioned.pm +++ b/lib/WeBWorK/DB/Schema/NewSQL/Versioned.pm @@ -97,8 +97,6 @@ sub where_user_id_eq_set_id_eq_problem_id_eq { # FIXME the rest of the places in this class that generate field lists (basically # anywhere that calls grok_*_from_vsetID_sql), should call this method instead. -# this method can handle if the set_id field has a fieldOverride set for it, and -# the other methods can't. sub sql_field_expression { my ($self, $field, $table) = @_; diff --git a/lib/WeBWorK/Utils/CourseDBIntegrityCheck.pm b/lib/WeBWorK/Utils/CourseDBIntegrityCheck.pm index 20f7760941..44d3e3b1e4 100644 --- a/lib/WeBWorK/Utils/CourseDBIntegrityCheck.pm +++ b/lib/WeBWorK/Utils/CourseDBIntegrityCheck.pm @@ -211,11 +211,9 @@ sub checkTableFields { exists $db->{$table}{params}{tableOverride} ? $db->{$table}{params}{tableOverride} : $table; warn "$table_name is a non native table" if $db->{$table}{params}{non_native}; my @schema_field_names = $db->{$table}{record}->FIELDS; - my %schema_override_field_names; + my %schema_field_names = map { $_ => 1 } @schema_field_names; for my $field (@schema_field_names) { - my $field_name = $db->{$table}{params}{fieldOverride}{$field} || $field; - $schema_override_field_names{$field_name} = $field; - if ($db->{$table}->tableFieldExists($field_name)) { + if ($db->{$table}->tableFieldExists($field)) { $fieldStatus{$field} = [SAME_IN_A_AND_B]; } else { $fieldStatus{$field} = [ONLY_IN_A]; @@ -229,10 +227,19 @@ sub checkTableFields { my %database_fields = map { ${$_}[0] => $_ } @$result; # Construct a hash of field names to field data. for my $field_name (keys %database_fields) { - unless (exists($schema_override_field_names{$field_name})) { + unless (exists($schema_field_names{$field_name})) { $fields_ok = 0; $fieldStatus{$field_name} = [ONLY_IN_B]; push(@{ $fieldStatus{$field_name} }, 1) if $database_fields{$field_name}[3]; + } else { + my $data_type = $database_fields{$field_name}[1]; + $data_type =~ s/\(\d*\)$// if $data_type =~ /^(big|small)?int\(\d*\)$/; + $data_type = lc($data_type); + my $schema_data_type = lc($db->{$table}{record}->FIELD_DATA->{$field_name}{type} =~ s/ .*$//r); + if ($data_type ne $schema_data_type) { + $fieldStatus{$field_name} = [DIFFER_IN_A_AND_B]; + $fields_ok = 0; + } } } @@ -249,7 +256,7 @@ the same as the ones specified by the databaseLayout =cut sub updateTableFields { - my ($self, $courseName, $table, $delete_field_names) = @_; + my ($self, $courseName, $table, $delete_field_names, $fix_type_field_names) = @_; my @messages; # Fetch schema from course environment and search database for corresponding tables. @@ -290,6 +297,24 @@ sub updateTableFields { } } + # Change types of fields list in $fix_type_field_names to the type defined in the schema. + for my $field_name (@$fix_type_field_names) { + if ($fieldStatus->{$field_name} && $fieldStatus->{$field_name}[0] == DIFFER_IN_A_AND_B) { + if ($schema_obj->can('change_column_field_type') && $schema_obj->change_column_field_type($field_name)) { + push(@messages, [ "Changed type of column '$field_name' from table '$table'", 1 ]); + } else { + push( + @messages, + [ + "Failed to changed type of column '$field_name' from table '$table'. " + . 'It is recommended that you delete this course and restore it from an archive.', + 0 + ] + ); + } + } + } + return @messages; } diff --git a/lib/WeBWorK/Utils/CourseManagement.pm b/lib/WeBWorK/Utils/CourseManagement.pm index 206fc98120..cb95b5eba6 100644 --- a/lib/WeBWorK/Utils/CourseManagement.pm +++ b/lib/WeBWorK/Utils/CourseManagement.pm @@ -1303,18 +1303,15 @@ sub initNonNativeTables { #fields in the schema. Its not a huge issue if the database table has extra columns. } else { my %fieldStatus; - my $fields_ok = 1; - my @schema_field_names = $db->{$table}->{record}->FIELDS; - my %schema_override_field_names = (); - foreach my $field (sort @schema_field_names) { - my $field_name = $db->{$table}->{params}->{fieldOverride}->{$field} || $field; - $schema_override_field_names{$field_name} = $field; + my $fields_ok = 1; + my @schema_field_names = $db->{$table}->{record}->FIELDS; + foreach my $field_name (sort @schema_field_names) { my $database_field_exists = $db->{$table}->tableFieldExists($field_name); #if the field doesn't exist then try to add it... if (!$database_field_exists) { $fields_ok = 0; - $fieldStatus{$field} = [ONLY_IN_A]; - warn "$field from $database_table_name (aka |$table|) is only in schema, " + $fieldStatus{$field_name} = [ONLY_IN_A]; + warn "$field_name from $database_table_name (aka |$table|) is only in schema, " . "not in database, so adding it ... "; if ($db->{$table}->can("add_column_field")) { if ($db->{$table}->add_column_field($field_name)) { diff --git a/templates/ContentGenerator/CourseAdmin/archive_course_confirm.html.ep b/templates/ContentGenerator/CourseAdmin/archive_course_confirm.html.ep index 9385d07be3..b70c2b4898 100644 --- a/templates/ContentGenerator/CourseAdmin/archive_course_confirm.html.ep +++ b/templates/ContentGenerator/CourseAdmin/archive_course_confirm.html.ep @@ -6,8 +6,14 @@

<%= $_->[0] %>

% } % } -% my ($all_tables_ok, $extra_database_tables, $extra_database_fields, $rebuild_table_indexes, $db_report) = - % $c->formatReportOnDatabaseTables($dbStatus); +% my ( + % $all_tables_ok, + % $extra_database_tables, + % $extra_database_fields, + % $rebuild_table_indexes, + % $incorrect_type_database_fields, + % $db_report +% ) = $c->formatReportOnDatabaseTables($dbStatus); <%= $db_report %> % if ($extra_database_tables) {

@@ -34,6 +40,16 @@ ) =%>

% } +% if ($incorrect_type_database_fields) { +

+ <%= maketext( + 'There are database fields that do not have the same type as the field defined in the schema for ' + . 'at least one table. The types of these fields can be changed when upgrading the course. ' + . 'Make sure that you archive the course here before doing that, since changing the type of a ' + . 'field can corrupt the table.' + ) =%> +

+% } % if (!$all_tables_ok) {

<%= maketext('The course database must be upgraded before archiving this course.') =%> diff --git a/templates/ContentGenerator/CourseAdmin/rename_course_confirm.html.ep b/templates/ContentGenerator/CourseAdmin/rename_course_confirm.html.ep index e32033dc85..4c9322d704 100644 --- a/templates/ContentGenerator/CourseAdmin/rename_course_confirm.html.ep +++ b/templates/ContentGenerator/CourseAdmin/rename_course_confirm.html.ep @@ -6,8 +6,14 @@

<%= $_->[0] %>

% } % } -% my ($all_tables_ok, $extra_database_tables, $extra_database_fields, $rebuild_table_indexes, $db_report) = - % $c->formatReportOnDatabaseTables($dbStatus); +% my ( + % $all_tables_ok, + % $extra_database_tables, + % $extra_database_fields, + % $rebuild_table_indexes, + % $incorrect_type_database_fields, + % $db_report +% ) = $c->formatReportOnDatabaseTables($dbStatus); <%= $db_report =%> % if ($extra_database_tables) {

@@ -34,6 +40,16 @@ ) =%>

% } +% if ($incorrect_type_database_fields) { +

+ <%= maketext( + 'There are database fields that do not have the same type as the field defined in the schema for ' + . 'at least one table. The types of these fields can be changed when upgrading the course. ' + . 'Make sure that you archive the course before doing that, since changing the type of a ' + . 'field can corrupt the table.' + ) =%> +

+% } % if (!$all_tables_ok) {

<%= maketext('The course database must be upgraded before renaming this course.') =%> diff --git a/templates/ContentGenerator/CourseAdmin/upgrade_course_confirm.html.ep b/templates/ContentGenerator/CourseAdmin/upgrade_course_confirm.html.ep index 3d5e4ca7d0..fd1ed16e61 100644 --- a/templates/ContentGenerator/CourseAdmin/upgrade_course_confirm.html.ep +++ b/templates/ContentGenerator/CourseAdmin/upgrade_course_confirm.html.ep @@ -19,6 +19,15 @@ % } + % if ($incorrect_type_database_fields_exist) { +

+ +
+ % } <% end =%> %