Skip to content

Commit 0b1d0d8

Browse files
drgrice1drgrice1
committed
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.
1 parent 1653d29 commit 0b1d0d8

File tree

9 files changed

+206
-62
lines changed

9 files changed

+206
-62
lines changed

‎bin/upgrade-database-to-utf8mb4.pl

Lines changed: 16 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -165,7 +165,7 @@ BEGIN
165165
my $dbuser = shell_quote($ce->{database_username});
166166
my $dbpass = $ce->{database_password};
167167

168-
$ENV{'MYSQL_PWD'} = $dbpass;
168+
local $ENV{MYSQL_PWD} = $dbpass;
169169

170170
if (!$no_backup) {
171171
# Backup the database
@@ -212,7 +212,7 @@ BEGIN
212212
},
213213
);
214214

215-
my $db = new WeBWorK::DB($ce->{dbLayouts}{ $ce->{dbLayoutName} });
215+
my $db = WeBWorK::DB->new($ce->{dbLayouts}{ $ce->{dbLayoutName} });
216216
my @table_types = sort(grep { !$db->{$_}{params}{non_native} } keys %$db);
217217

218218
sub checkAndUpdateTableColumnTypes {
@@ -222,29 +222,30 @@ sub checkAndUpdateTableColumnTypes {
222222

223223
print "\tChecking '$table' (pass $pass)\n" if $verbose;
224224
my $schema_field_data = $db->{$table_type}{record}->FIELD_DATA;
225-
for my $field (keys %$schema_field_data) {
226-
my $field_name = $db->{$table_type}{params}{fieldOverride}{$field} || $field;
227-
my @name_type = @{
225+
for my $field_name (keys %$schema_field_data) {
226+
my @name_type = @{
228227
$dbh->selectall_arrayref(
229228
"SELECT COLUMN_TYPE FROM INFORMATION_SCHEMA.COLUMNS "
230229
. "WHERE TABLE_SCHEMA='$dbname' AND TABLE_NAME='$table' AND COLUMN_NAME='$field_name';"
231230
)
232231
};
233232

234-
print("\t\tThe '$field_name' column is missing from '$table'.\n"
235-
. "\t\tYou should upgrade the course via course administration to fix this.\n"
236-
. "\t\tYou may need to run this script again after doing that.\n"), next
237-
if !exists($name_type[0][0]);
233+
if (!exists($name_type[0][0])) {
234+
print("\t\tThe '$field_name' column is missing from '$table'.\n"
235+
. "\t\tYou should upgrade the course via course administration to fix this.\n"
236+
. "\t\tYou may need to run this script again after doing that.\n");
237+
next;
238+
}
238239

239240
my $data_type = $name_type[0][0];
240241
next if !$data_type;
241242
$data_type =~ s/\(\d*\)$// if $data_type =~ /^(big|small)?int\(\d*\)$/;
242243
$data_type = lc($data_type);
243-
my $schema_data_type = lc($schema_field_data->{$field}{type} =~ s/ .*$//r);
244+
my $schema_data_type = lc($schema_field_data->{$field_name}{type} =~ s/ .*$//r);
244245
if ($data_type ne $schema_data_type) {
245246
print "\t\tUpdating data type for column '$field_name' in table '$table'\n" if $verbose;
246247
print "\t\t\t$data_type -> $schema_data_type\n" if $verbose;
247-
eval { $dbh->do("ALTER TABLE `$table` MODIFY $field_name $schema_field_data->{$field}{type};"); };
248+
eval { $dbh->do("ALTER TABLE `$table` MODIFY $field_name $schema_field_data->{$field_name}{type};"); };
248249
my $indent = $verbose ? "\t\t" : "";
249250
die("${indent}Failed to modify '$field_name' in '$table' from '$data_type' to '$schema_data_type.\n"
250251
. "${indent}It is recommended that you restore a database backup. Make note of the\n"
@@ -286,8 +287,10 @@ sub checkAndChangeTableCharacterSet {
286287
my $error = 0;
287288

288289
for my $course (@courses) {
289-
print("The course '$course' does not exist on the server\n"), next
290-
if !grep($course eq $_, @server_courses);
290+
if (!grep { $course eq $_ } @server_courses) {
291+
print("The course '$course' does not exist on the server\n");
292+
next;
293+
}
291294

292295
print "Checking tables for '$course'\n" if $verbose;
293296
for my $table_type (@table_types) {

‎lib/WeBWorK/ContentGenerator/CourseAdmin.pm

Lines changed: 88 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -1339,7 +1339,7 @@ sub upgrade_course_confirm ($c) {
13391339

13401340
my @upgrade_courseIDs = $c->param('upgrade_courseIDs');
13411341

1342-
my ($extra_database_tables_exist, $extra_database_fields_exist) = (0, 0);
1342+
my ($extra_database_tables_exist, $extra_database_fields_exist, $incorrect_type_database_fields_exist) = (0, 0, 0);
13431343

13441344
my $status_output = $c->c;
13451345

@@ -1354,8 +1354,9 @@ sub upgrade_course_confirm ($c) {
13541354

13551355
# Report on database status
13561356
my ($tables_ok, $dbStatus) = $CIchecker->checkCourseTables($upgrade_courseID);
1357-
my ($all_tables_ok, $extra_database_tables, $extra_database_fields, $rebuild_table_indexes, $db_report) =
1358-
$c->formatReportOnDatabaseTables($dbStatus, $upgrade_courseID);
1357+
my ($all_tables_ok, $extra_database_tables, $extra_database_fields, $rebuild_table_indexes,
1358+
$incorrect_type_database_fields, $db_report)
1359+
= $c->formatReportOnDatabaseTables($dbStatus, $upgrade_courseID);
13591360

13601361
my $course_output = $c->c;
13611362

@@ -1428,6 +1429,24 @@ sub upgrade_course_confirm ($c) {
14281429
);
14291430
}
14301431

1432+
if ($incorrect_type_database_fields) {
1433+
$incorrect_type_database_fields_exist = 1;
1434+
push(
1435+
@$course_output,
1436+
$c->tag(
1437+
'p',
1438+
class => 'text-danger fw-bold',
1439+
$c->maketext(
1440+
'There are database fields that do not have the same type as the field defined in the schema '
1441+
. 'for at least one table. Check the checkbox by the field to change its type when '
1442+
. 'upgrading the course. Warning: This can fail which may corrupt the table. If you have '
1443+
. 'not archived this course, then do that now before upgrading if you want to change the '
1444+
. 'type of any of these fields.'
1445+
)
1446+
)
1447+
);
1448+
}
1449+
14311450
# Report on directory status
14321451
my ($directories_ok, $directory_report) = checkCourseDirectories($ce2);
14331452
push(@$course_output, $c->tag('div', class => 'mb-2', $c->maketext('Directory structure:')));
@@ -1464,10 +1483,11 @@ sub upgrade_course_confirm ($c) {
14641483

14651484
return $c->include(
14661485
'ContentGenerator/CourseAdmin/upgrade_course_confirm',
1467-
upgrade_courseIDs => \@upgrade_courseIDs,
1468-
extra_database_tables_exist => $extra_database_tables_exist,
1469-
extra_database_fields_exist => $extra_database_fields_exist,
1470-
status_output => $status_output->join('')
1486+
upgrade_courseIDs => \@upgrade_courseIDs,
1487+
extra_database_tables_exist => $extra_database_tables_exist,
1488+
extra_database_fields_exist => $extra_database_fields_exist,
1489+
incorrect_type_database_fields_exist => $incorrect_type_database_fields_exist,
1490+
status_output => $status_output->join('')
14711491
);
14721492
}
14731493

@@ -1503,17 +1523,20 @@ sub do_upgrade_course ($c) {
15031523
push(
15041524
@upgrade_report,
15051525
$CIchecker->updateTableFields(
1506-
$upgrade_courseID, $table_name,
1507-
[ ($c->param("$upgrade_courseID.$table_name.delete_fieldIDs")) ]
1526+
$upgrade_courseID,
1527+
$table_name,
1528+
[ ($c->param("$upgrade_courseID.$table_name.delete_fieldIDs")) ],
1529+
[ ($c->param("$upgrade_courseID.$table_name.fix_type_fieldIDs")) ],
15081530
)
15091531
);
15101532
}
15111533

15121534
# Analyze database status and prepare status report
15131535
($tables_ok, $dbStatus) = $CIchecker->checkCourseTables($upgrade_courseID);
15141536

1515-
my ($all_tables_ok, $extra_database_tables, $extra_database_fields, $rebuild_table_indexes, $db_report) =
1516-
$c->formatReportOnDatabaseTables($dbStatus);
1537+
my ($all_tables_ok, $extra_database_tables, $extra_database_fields, $rebuild_table_indexes,
1538+
$incorrect_type_database_fields, $db_report)
1539+
= $c->formatReportOnDatabaseTables($dbStatus);
15171540

15181541
# Prepend course name
15191542
$db_report = $c->c($c->tag('div', class => 'mb-2', $c->maketext('Database:')), $db_report);
@@ -1541,6 +1564,20 @@ sub do_upgrade_course ($c) {
15411564
);
15421565
}
15431566

1567+
if ($incorrect_type_database_fields) {
1568+
push(
1569+
@$db_report,
1570+
$c->tag(
1571+
'p',
1572+
class => 'text-danger fw-bold',
1573+
$c->maketext(
1574+
'There are database fields that do not have the same type as the '
1575+
. 'field defined in the schema for at least one table.'
1576+
)
1577+
)
1578+
);
1579+
}
1580+
15441581
# Add missing directories and prepare report on directory status
15451582
my $dir_update_messages = updateCourseDirectories($ce2); # Needs more error messages
15461583
my ($directories_ok, $directory_report) = checkCourseDirectories($ce2);
@@ -2684,10 +2721,11 @@ sub formatReportOnDatabaseTables ($c, $dbStatus, $courseID = undef) {
26842721
)
26852722
);
26862723

2687-
my $all_tables_ok = 1;
2688-
my $extra_database_tables = 0;
2689-
my $extra_database_fields = 0;
2690-
my $rebuild_table_indexes = 0;
2724+
my $all_tables_ok = 1;
2725+
my $extra_database_tables = 0;
2726+
my $extra_database_fields = 0;
2727+
my $rebuild_table_indexes = 0;
2728+
my $incorrect_type_database_fields = 0;
26912729

26922730
my $db_report = $c->c;
26932731

@@ -2728,9 +2766,35 @@ sub formatReportOnDatabaseTables ($c, $dbStatus, $courseID = undef) {
27282766
} else {
27292767
$extra_database_fields = 1;
27302768
}
2731-
if ($fieldInfo{$key}[1]) {
2732-
push(@$field_report, $c->hidden_field("$courseID.$table.delete_fieldIDs" => $key));
2733-
} else {
2769+
if (defined $courseID) {
2770+
if ($fieldInfo{$key}[1]) {
2771+
push(@$field_report, $c->hidden_field("$courseID.$table.delete_fieldIDs" => $key));
2772+
} else {
2773+
push(
2774+
@$field_report,
2775+
$c->tag(
2776+
'span',
2777+
class => 'form-check d-inline-block',
2778+
$c->tag(
2779+
'label',
2780+
class => 'form-check-label',
2781+
$c->c(
2782+
$c->check_box(
2783+
"$courseID.$table.delete_fieldIDs" => $key,
2784+
class => 'form-check-input'
2785+
),
2786+
$c->maketext('Delete field when upgrading')
2787+
)->join('')
2788+
)
2789+
)
2790+
);
2791+
}
2792+
}
2793+
} elsif ($field_status == WeBWorK::Utils::CourseDBIntegrityCheck::ONLY_IN_A) {
2794+
$all_tables_ok = 0;
2795+
} elsif ($field_status == WeBWorK::Utils::CourseDBIntegrityCheck::DIFFER_IN_A_AND_B) {
2796+
$incorrect_type_database_fields = 1;
2797+
if (defined $courseID) {
27342798
push(
27352799
@$field_report,
27362800
$c->tag(
@@ -2741,17 +2805,15 @@ sub formatReportOnDatabaseTables ($c, $dbStatus, $courseID = undef) {
27412805
class => 'form-check-label',
27422806
$c->c(
27432807
$c->check_box(
2744-
"$courseID.$table.delete_fieldIDs" => $key,
2745-
class => 'form-check-input'
2808+
"$courseID.$table.fix_type_fieldIDs" => $key,
2809+
class => 'form-check-input'
27462810
),
2747-
$c->maketext('Delete field when upgrading')
2811+
$c->maketext('Change type of field when upgrading')
27482812
)->join('')
27492813
)
27502814
)
27512815
);
27522816
}
2753-
} elsif ($field_status == WeBWorK::Utils::CourseDBIntegrityCheck::ONLY_IN_A) {
2754-
$all_tables_ok = 0;
27552817
}
27562818
push(@$fields_report, $c->tag('li', $field_report->join('')));
27572819
}
@@ -2765,8 +2827,9 @@ sub formatReportOnDatabaseTables ($c, $dbStatus, $courseID = undef) {
27652827
push(@$db_report, $c->tag('p', class => 'text-success', $c->maketext('Database tables are ok'))) if $all_tables_ok;
27662828

27672829
return (
2768-
$all_tables_ok, $extra_database_tables, $extra_database_fields,
2769-
$rebuild_table_indexes, $db_report->join('')
2830+
$all_tables_ok, $extra_database_tables, $extra_database_fields, $rebuild_table_indexes,
2831+
$incorrect_type_database_fields,
2832+
$db_report->join('')
27702833
);
27712834
}
27722835

‎lib/WeBWorK/DB/Schema/NewSQL/Std.pm

Lines changed: 21 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -368,16 +368,33 @@ sub _drop_column_field_stmt {
368368
return "Alter table `$sql_table_name` drop column `$sql_field_name` ";
369369
}
370370

371+
####################################################
372+
# Change the type of a column to the type defined in the schema
373+
####################################################
374+
375+
sub change_column_field_type {
376+
my ($self, $field_name) = @_;
377+
return 0 unless defined $self->{record}->FIELD_DATA->{$field_name};
378+
eval {
379+
$self->dbh->do('ALTER TABLE `'
380+
. $self->sql_table_name
381+
. '` MODIFY '
382+
. $self->sql_field_name($field_name) . ' '
383+
. $self->{record}->FIELD_DATA->{$field_name}{type}
384+
. ';');
385+
};
386+
return $@ ? 0 : 1;
387+
}
388+
371389
####################################################
372390
# rebuild indexes for the table
373391
####################################################
374392

375393
sub rebuild_indexes {
376394
my ($self) = @_;
377395

378-
my $sql_table_name = $self->sql_table_name;
379-
my $field_data = $self->field_data;
380-
my %override_fields = reverse %{ $self->{params}{fieldOverride} };
396+
my $sql_table_name = $self->sql_table_name;
397+
my $field_data = $self->field_data;
381398

382399
# A key field column is going to be removed. The schema will not have the information for this column. So the
383400
# 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 {
398415
my $column = (grep { $index->[4] eq $_->[0] } @$columns)[0];
399416
if (defined $column && $column->[5] =~ m/AUTO_INCREMENT/i) {
400417
$self->dbh->do("ALTER TABLE `$sql_table_name` MODIFY `$column->[0]` $column->[1]");
401-
push @auto_increment_fields, $override_fields{ $column->[0] } // $column->[0];
418+
push @auto_increment_fields, $column->[0];
402419
}
403420

404421
$self->dbh->do("ALTER TABLE `$sql_table_name` DROP INDEX `$index->[2]`");

‎lib/WeBWorK/DB/Schema/NewSQL/Versioned.pm

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -97,8 +97,6 @@ sub where_user_id_eq_set_id_eq_problem_id_eq {
9797

9898
# FIXME the rest of the places in this class that generate field lists (basically
9999
# anywhere that calls grok_*_from_vsetID_sql), should call this method instead.
100-
# this method can handle if the set_id field has a fieldOverride set for it, and
101-
# the other methods can't.
102100
sub sql_field_expression {
103101
my ($self, $field, $table) = @_;
104102

‎lib/WeBWorK/Utils/CourseDBIntegrityCheck.pm

Lines changed: 31 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -211,11 +211,9 @@ sub checkTableFields {
211211
exists $db->{$table}{params}{tableOverride} ? $db->{$table}{params}{tableOverride} : $table;
212212
warn "$table_name is a non native table" if $db->{$table}{params}{non_native};
213213
my @schema_field_names = $db->{$table}{record}->FIELDS;
214-
my %schema_override_field_names;
214+
my %schema_field_names = map { $_ => 1 } @schema_field_names;
215215
for my $field (@schema_field_names) {
216-
my $field_name = $db->{$table}{params}{fieldOverride}{$field} || $field;
217-
$schema_override_field_names{$field_name} = $field;
218-
if ($db->{$table}->tableFieldExists($field_name)) {
216+
if ($db->{$table}->tableFieldExists($field)) {
219217
$fieldStatus{$field} = [SAME_IN_A_AND_B];
220218
} else {
221219
$fieldStatus{$field} = [ONLY_IN_A];
@@ -229,10 +227,19 @@ sub checkTableFields {
229227
my %database_fields = map { ${$_}[0] => $_ } @$result; # Construct a hash of field names to field data.
230228

231229
for my $field_name (keys %database_fields) {
232-
unless (exists($schema_override_field_names{$field_name})) {
230+
unless (exists($schema_field_names{$field_name})) {
233231
$fields_ok = 0;
234232
$fieldStatus{$field_name} = [ONLY_IN_B];
235233
push(@{ $fieldStatus{$field_name} }, 1) if $database_fields{$field_name}[3];
234+
} else {
235+
my $data_type = $database_fields{$field_name}[1];
236+
$data_type =~ s/\(\d*\)$// if $data_type =~ /^(big|small)?int\(\d*\)$/;
237+
$data_type = lc($data_type);
238+
my $schema_data_type = lc($db->{$table}{record}->FIELD_DATA->{$field_name}{type} =~ s/ .*$//r);
239+
if ($data_type ne $schema_data_type) {
240+
$fieldStatus{$field_name} = [DIFFER_IN_A_AND_B];
241+
$fields_ok = 0;
242+
}
236243
}
237244
}
238245

@@ -249,7 +256,7 @@ the same as the ones specified by the databaseLayout
249256
=cut
250257

251258
sub updateTableFields {
252-
my ($self, $courseName, $table, $delete_field_names) = @_;
259+
my ($self, $courseName, $table, $delete_field_names, $fix_type_field_names) = @_;
253260
my @messages;
254261

255262
# Fetch schema from course environment and search database for corresponding tables.
@@ -290,6 +297,24 @@ sub updateTableFields {
290297
}
291298
}
292299

300+
# Change types of fields list in $fix_type_field_names to the type defined in the schema.
301+
for my $field_name (@$fix_type_field_names) {
302+
if ($fieldStatus->{$field_name} && $fieldStatus->{$field_name}[0] == DIFFER_IN_A_AND_B) {
303+
if ($schema_obj->can('change_column_field_type') && $schema_obj->change_column_field_type($field_name)) {
304+
push(@messages, [ "Changed type of column '$field_name' from table '$table'", 1 ]);
305+
} else {
306+
push(
307+
@messages,
308+
[
309+
"Failed to changed type of column '$field_name' from table '$table'. "
310+
. 'It is recommended that you delete this course and restore it from an archive.',
311+
0
312+
]
313+
);
314+
}
315+
}
316+
}
317+
293318
return @messages;
294319
}
295320

0 commit comments

Comments
 (0)