Skip to content

Commit

Permalink
Add the capability of changing database field types when upgrading th…
Browse files Browse the repository at this point in the history 
…e 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.
  • Loading branch information
@drgrice1
drgrice1 committed Dec 19, 2024
1 parent 475de2b commit 43af347
Show file tree
Hide file tree
Showing 9 changed files with 206 additions and 62 deletions.
29 changes: 16 additions & 13 deletions bin/upgrade-database-to-utf8mb4.pl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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 {
Expand All @@ -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"
Expand Down Expand Up @@ -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) {
Expand Down
113 changes: 88 additions & 25 deletions lib/WeBWorK/ContentGenerator/CourseAdmin.pm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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;

Expand Down Expand Up @@ -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:')));
Expand Down Expand Up @@ -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('')
);
}

Expand Down Expand Up @@ -1503,17 +1523,20 @@ 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")) ],
)
);
}

# 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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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(
Expand All @@ -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('')));
}
Expand All @@ -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('')
);
}

Expand Down
25 changes: 21 additions & 4 deletions lib/WeBWorK/DB/Schema/NewSQL/Std.pm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -368,16 +368,33 @@ 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
####################################################

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
Expand All @@ -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]`");
Expand Down
2 changes: 0 additions & 2 deletions lib/WeBWorK/DB/Schema/NewSQL/Versioned.pm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) = @_;

Expand Down
37 changes: 31 additions & 6 deletions lib/WeBWorK/Utils/CourseDBIntegrityCheck.pm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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];
Expand All @@ -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;
}
}
}

Expand All @@ -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.
Expand Down Expand Up @@ -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;
}

Expand Down
Loading

0 comments on commit 43af347

Please sign in to comment.