Abort a course archive when the database dump actually fails#3042
Conversation
dump_tables() and NewSQL::Std::dump_table() unconditionally returned 1, so
archiveCourse()'s `unless ($dump_db_result) { croak }` guard never fired. A
mysqldump failure (wrong mysqldump path, auth failure, disk full, a MariaDB
column-statistics incompatibility, ...) therefore produced a "successful"
archive whose database was empty or missing -- discovered only later, when the
archive was unarchived and no tables were restored.
Make the failure propagate:
- dump_table() returns 0 when mysqldump exits non-zero.
- dump_tables() aggregates the per-table results and returns false if any
table's dump failed, so archiveCourse() aborts instead of writing a DB-less
archive.
Preserve the existing tolerance for courses created with an earlier version of
WeBWorK that predate some tables: dump_tables() skips tables that do not exist
(via tableExists) rather than treating them as failures, so only a genuine
mysqldump error aborts the archive. Because absent tables now produce no dump
file, restore_tables() skips missing dump files instead of restoring from a
nonexistent path.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
6c0e5eb to
c395ca7
Compare
drgrice1
left a comment
There was a problem hiding this comment.
Looks good. It has always bothered me that the code in the WeBWorK::Utils::CourseManagement::archiveCourse and WeBWorK::Utils::CourseManagement::unarchiveCourse methods looks at the return values of the WeBWorK::DB::dump_tables and WeBWorK::DB::restore_tables methods, and yet both of those methods always return 1. So it was a pointless check. That is still the case for the WeBWorK::Utils::CourseManagement::unarchiveCourse and WeBWorK::DB::restore_tables methods, and that should probably be changed also. But you can leave that for now. A large portion of the webwork2 code needs to be reworked to deal better with things that do not go as expected.
Most likely for the next release we are going to move away from using mysqldump for course archives, and move to a different archive format. In fact, I have already implemented that in one way. This is going to make me need to fix merge conflicts in that implementation!
Description:
Problem
A failed database dump produces a "successful" archive with no database. Because
WeBWorK::DB::dump_tablesandNewSQL::Std::dump_tablebothreturn 1unconditionally,archiveCourse's existing guard —— never fires. Any mysqldump failure (wrong mysqldump path, auth failure, disk full, a MariaDB column-statistics incompatibility, …) is swallowed with only a warn to the server log, and the archive is written anyway. The empty database is discovered only later, when the archive is unarchived and no tables are restored.
Fix
Make the failure propagate:
failed, so archiveCourse aborts instead of writing a DB-less archive.
The existing tolerance for courses created with an earlier version of WeBWorK (which may predate some tables) is preserved: dump_tables skips tables that don't exist (via tableExists) rather than counting them as failures, so only a genuine mysqldump error aborts the archive. Because absent tables now produce no dump file, restore_tables skips missing dump files instead of restoring from a nonexistent path.
Scope note
This intentionally does not change the restore side to abort on failure — restore_tables/restore_table still return success and only warn, matching current behavior where a partial restore is preferred over none. Only the missing-file skip was added there, as a direct consequence of no longer dumping absent tables.
Testing
e.g. a mysqldump that rejects --column-statistics against MariaDB) and archive a course.
DATA/mysqldump, and a later unarchive restores no tables.
equivalent to before.
the rest dump, and the archive succeeds.