Determine the unarchive source course ID from the archive contents, not its filename - #3043
Conversation
unarchiveCourse() derived the source course ID from the caller-supplied oldCourseID, which the admin controller computes from the archive's *filename*. Archive::Tar->extract(), however, always writes files to the directory name stored *inside* the archive (the course's name at archive time). These agree only until someone renames the .tar.gz. When they diverge (e.g. an archive renamed before import), the course files extract under the archived name while the database-dump lookup, the conflicting-course guard (_unarchiveCourse_move_away), and the post-restore rename all use the filename-derived name. The result is a course whose files restore correctly but whose database is not restored -- reported only as a warning while the unarchive otherwise "succeeds": course 'X' has no database dump in its data directory (checked for .../X/DATA/mysqldump). database tables will not be restored. Fix at the single chokepoint in unarchiveCourse() so every caller is covered: open the archive first and take the source course ID from its sole top-level directory, dying if the archive does not contain exactly one. 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. The oldCourseID option is now unused, so drop it from the sole caller (do_unarchive_course) and from the POD. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
b4fadf7 to
5093faf
Compare
|
What is the use case for renaming a course archive file? And shouldn't that be discouraged? It would be confusing to unarchive |
|
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
left a comment
drgrice1
left a comment
There was a problem hiding this comment.
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.
Wrap the `unarchiveCourse` call in the `do_unarchive_course` method of the `WeBWorK::ContentGenerator::CourseAdmin` package in an `eval`, so that exceptions are caught. The exceptions are already checked for on the next line, but deal with the exception message better. Instead of displaying `$@` which might include a backtrace, show `$@->message` in the case that `$@` is an object.
Wrap the `unarchiveCourse` call in the `do_unarchive_course` method of the `WeBWorK::ContentGenerator::CourseAdmin` package in an `eval`, so that exceptions are caught. The exceptions are already checked for on the next line, but deal with the exception message better. Instead of displaying `$@` which might include a backtrace, show `$@->message` in the case that `$@` is an object.
Wrap the `unarchiveCourse` call in the `do_unarchive_course` method of the `WeBWorK::ContentGenerator::CourseAdmin` package in an `eval`, so that exceptions are caught. The exceptions are already checked for on the next line, but deal with the exception message better. Instead of displaying `$@` which might include a backtrace, show `$@->message` in the case that `$@` is an object.
…m-archive-addition Suggestion from #3043.
Wrap the `unarchiveCourse` call in the `do_unarchive_course` method of the `WeBWorK::ContentGenerator::CourseAdmin` package in an `eval`, so that exceptions are caught. The exceptions are already checked for on the next line, but deal with the exception message better. Instead of displaying `$@` which might include a backtrace, show `$@->message` in the case that `$@` is an object.
|
@Alex-Jordan I think @drgrice1 caching changed this, which was just merged. Since only one course per courseID is listed due to the key used in the hash. The code will only include the most recent modified time file information, so I think it will only be listed once and include the filename of the one that has the most recent modified time. I haven't tested, but that is my quick read of the code and the cached data that is created/stored. |
|
I think I misread the code. The cache is keyed based on filename, while the return data is keyed based on courseID. So only one course will be returned, but it will be linked to the last file read in the loop. |
|
Did you mean to ask this in #3047, #3110, or #3109? What is displayed in the admin course is affected by those pull requests. This only determines the course id for the archive file, but doesn't change what is displayed in the admin course UI. In any case, with the current setup you would only see one of the courses, and which one you are shown would be the filename that is listed last by the This is not something that we though about in displaying the course id instead of the filename both on the course listings page and the unarchive course page. Perhaps the filename should be shown also on those pages, and certainly in order to have both shown the |
Yes, sorry, I was on my phone email and in my memory the conversation about the use case for this came up and that's what I searched for. Maybe we can make it show the filename, but to reduce clutter, only show file names for when the filename differs from the course ID inside. |
|
I think that making it show the file names only when the filename differs will be messy code wise, and may not appear the best. I mean, if there are two archive files that contain the same course id, and those two have the file names shown, but the rest do not, I think that will not look the best. My thought was that all would show both the course id and then the filename and file size would be in a parenthetical after the course id. In fact here is what I have at this point: 
|
4 participants
Description:
Problem
Unarchiving a course whose
.tar.gzhas been renamed restores the course files butnot 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):
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_coursederives the source course ID from the archive's filename(
oldCourseID => $unarchive_courseID =~ s/\.tar\.gz$//r), butArchive::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;_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 CLIcallers).
dieif the archive doesn't contain exactly one top-level directory. Thecaller's
newCourseIDstill controls the target name, so renaming on import continues towork via the New course ID field rather than by renaming the file.
Testing
Foofrom the admin course (producesFoo.tar.gz).Bar.tar.gz.Bar.Foo/; the unarchive reports success but shows thewarning "…database tables will not be restored"; the new
Barcourse has no database,and a files-only
Foodirectory is left behind.Foo, DB dump found and restored, course renamed toBar.Foo.tar.gz→Foo) unarchives exactly as before.