Determine the unarchive source course ID from the archive contents, not its filename

[drdrew42]

Description:

Problem

Unarchiving a course whose .tar.gz has been renamed restores the course files but
not the database. The unarchive still reports success and leaves a new, database-less
course in place; the indication that something went wrong is a WeBWorK warning shown on
the admin page (and logged):

  course 'X' has no database dump in its data directory
  (checked for .../X/DATA/mysqldump). database tables will not be restored.

That warning is itself misleading: it implies the archive contains no database dump,
when in fact the dump is present — stored under the course's original name, which the
unarchive never looks at (see Cause). So an operator sees a course that seems to import
"with a warning" but is actually missing every user, set, and answer, plus a stray
files-only course directory that then blocks a corrected re-import with
"Cannot overwrite existing course."

Cause

do_unarchive_course derives the source course ID from the archive's filename
(oldCourseID => $unarchive_courseID =~ s/\.tar\.gz$//r), but Archive::Tar->extract()
writes files to the directory name stored inside the archive (the course's name at
archive time). These agree only until someone renames the file. When they diverge:

• extract() writes course files under the archived name;
• the database-dump lookup, the conflicting-course guard (_unarchiveCourse_move_away),
  and the post-restore rename all use the filename-derived name.

So the DB dump is sought in a directory that extraction never created → skipped.

Fix

Take the source course ID from the archive's single top-level directory, in
unarchiveCourse (the one chokepoint shared by the admin UI, WebworkWebservice, and CLI
callers). die if the archive doesn't contain exactly one top-level directory. The
caller's newCourseID still controls the target name, so renaming on import continues to
work via the New course ID field rather than by renaming the file.

Testing

1. Archive a course Foo from the admin course (produces Foo.tar.gz).
2. Rename/upload the archive as Bar.tar.gz.
3. Unarchive it with new course ID Bar.
   • Before: files extract under Foo/; the unarchive reports success but shows the
     warning "…database tables will not be restored"; the new Bar course has no database,
     and a files-only Foo directory is left behind.
   • After: source ID read as Foo, DB dump found and restored, course renamed to Bar.
4. Regression: a normally-named archive (Foo.tar.gz → Foo) unarchives exactly as before.

[Alex-Jordan]

What is the use case for renaming a course archive file? And shouldn't that be discouraged? It would be confusing to unarchive alex.tar.gz and get a course named drew. I'm wondering if this is the right way to address the issue, or if there is something else, like error out early if the file name does not match the course ID inside it. Or give users a choice. Or a utility that renames course archive files including all the relevant internal bits.

[drdrew42]

There isn't a "use case" for it, so much as it's an issue that's surfaced here and there (for me).

The justification is that it shouldn't matter what the file is called, as the archive contains an "original" course name already. I get your point that you don't want to unarchive "alex" and get "drew" -- but at present, an archive named "alex" that holds a course named "drew" won't unarchive successfully in the first place.

Moreover, there always remains the option to specify the course name after un-archival -- which still applies.

I don't really see this as a major issue -- I mean, we've had this failure in the code for how long now, and no one's addressed it? I'm just putting this PR out as a result of one of my WW admins running into this issue. IMO, unarchive shouldn't fail just because the filename is "wrong"...

[drgrice1]

Although it may be a bit odd to unarchive a file like alex.tar.gz and get course named drew, I agree that fixing things so that the course name is taking from the tar ball contents rather than the file name is an improvement. Particularly since the current behavior is broken in several bad ways.

One way this is broken and that this fixes that is not noted is the case that a course archive is created from an existing course, the archive file is renamed, and then that archive restored. The check for the new course id existing is passed since the new course id passed is not what is in the tar ball, and then the tar ball is extracted and overwrites anything in the existing course. Then unarchival process fails when the database dump files by the expected course name are not found.

[drgrice1]

This is a rather silly code snippet, and another example of poor webwork2 code error handling. Since the unarchiveCourse call is not wrapped in an eval statement, any exception thrown inside that call will simply die, and the if ($@) { call will never be reached. There are several exceptions that can be thrown by the unarchiveCourse method (lines 957, 961, 967, and this pull request adds line 979). Note the first two are protected against when working from the UI, and the third generally won't happen when working from the UI. The one added by this pull request is not protected against, and certainly can happen when working from the UI. So the eval should be added here. Although, the UI output for such an exception is rather lengthy. This can be made better by using the $@->message if the $@ is an object for which the message method can be called.

So I recommend changing this to

	eval {
		unarchiveCourse(
			newCourseID => $new_courseID,
			archivePath => "$ce->{webworkDirs}{courses}/$ce->{admin_course_id}/archives/$unarchive_courseID",
			ce          => $ce,
		);
	};

	if ($@) {
		return $c->tag(
			'div',
			class => 'alert alert-danger p-1 mb-2',
			$c->c(
				$c->tag(
					'p', $c->maketext('An error occurred while unarchiving the course [_1]:', $unarchive_courseID)
				),
				$c->tag('div', class => 'font-monospace', ref $@ ? $@->message : $@)
			)->join('')
		);
	} else {

The ref check should be good enough, but if you want to be even more proper you could pull in a blessed and can check on $@.

[Alex-Jordan]

If #3047 goes in as well, my concern here is lifted. The person unarchiving the course will see a course ID that matches the one that is used to make the course.

I'm giving the second approval, but I won't merge since there is at least one suggestion from @drgrice1 that is not yet resolved.