diff --git a/crates/scriptmark/src/archive.rs b/crates/scriptmark/src/archive.rs index 0470807..559c342 100644 --- a/crates/scriptmark/src/archive.rs +++ b/crates/scriptmark/src/archive.rs @@ -273,8 +273,6 @@ impl<'a> Sink<'a> { self.total_bytes += size; self.file_count += 1; self.claimed.insert(out_path.clone(), name.to_string()); - // Provenance is recorded whether or not the bytes are written this run, so a cached - // extraction still traces every file back to the entry it came from. self.extracted.push(ExtractedFile { out_path: out_path.clone(), archive: self.archive.to_path_buf(), @@ -294,17 +292,16 @@ impl<'a> Sink<'a> { self.file_count -= 1; } - /// Write an accepted entry, unless it is already there from an earlier run. + /// Write an accepted entry. fn write(&mut self, name: &str, out_path: &Path, size: u64, read: &mut dyn Read) { - if out_path.exists() { - return; - } let mut buf = Vec::new(); let failure = match read.read_to_end(&mut buf) { Err(_) => Some("unreadable entry".to_string()), Ok(_) => std::fs::write(out_path, &buf).err().map(|e| e.to_string()), }; if let Some(reason) = failure { + // A failed write can leave a truncated file, and the scan lists this directory. + std::fs::remove_file(out_path).ok(); self.rollback(name, out_path, size, reason); } } @@ -319,8 +316,10 @@ fn unreadable(archive: &Path, reason: String) -> InputDiagnostic { /// Expand `archive` into `target`, writing only the entries `wanted` accepts. /// -/// Entries are flattened onto their base names. Already-extracted files are not rewritten, -/// but their provenance is recorded again, so a cached extraction still traces back. +/// Entries are flattened onto their base names. `target` belongs to this archive alone and +/// is rebuilt on every call: whatever an earlier expansion left there is removed first, so +/// it holds exactly what the archive yields now. A student who re-uploads under the same +/// name is graded on the new bytes, and an entry they dropped does not linger to be graded. pub(crate) fn expand( archive: &Path, target: &Path, @@ -332,10 +331,19 @@ pub(crate) fn expand( }; let before = diagnostics.len(); - if let Err(e) = std::fs::create_dir_all(target) { + // Cleared before the archive is opened, so a replacement that will not open grades + // nothing rather than its predecessor's files. + let fresh = match std::fs::remove_dir_all(target) { + Err(e) if e.kind() != std::io::ErrorKind::NotFound => Err(e), + _ => std::fs::create_dir_all(target), + }; + if let Err(e) = fresh { diagnostics.push(unreadable( archive, - format!("cannot create extraction directory: {e}"), + format!( + "cannot prepare extraction directory {}: {e}", + target.display() + ), )); return Vec::new(); } @@ -798,4 +806,108 @@ mod tests { DiagnosticKind::ArchiveEmpty { .. } )); } + + fn listing(dir: &Path) -> Vec { + let mut names: Vec = std::fs::read_dir(dir) + .unwrap() + .map(|e| e.unwrap().file_name().into_string().unwrap()) + .collect(); + names.sort(); + names + } + + /// P-868: a student re-uploads under the same name. Same entry name, same size — nothing + /// short of the bytes tells the two apart, and the bytes are what gets graded. + #[test] + fn test_a_replaced_archive_is_expanded_from_its_new_bytes() { + let dir = tempfile::tempdir().unwrap(); + let target = dir.path().join("out"); + let mut diagnostics = Vec::new(); + let archive = zip_with(dir.path(), "hw.zip", &[("src/lab5.py", b"x=1")]); + expand(&archive, &target, &is_gradeable, &mut diagnostics); + + zip_with(dir.path(), "hw.zip", &[("src/lab5.py", b"x=2")]); + let files = expand(&archive, &target, &is_gradeable, &mut diagnostics); + + assert_eq!(files.len(), 1); + assert_eq!(std::fs::read(&files[0].out_path).unwrap(), b"x=2"); + assert!(diagnostics.is_empty(), "got {diagnostics:?}"); + } + + /// The extraction directory is listed, not just the entries `expand` returns, so a file + /// the new archive no longer holds must not be left there to be graded. + #[test] + fn test_an_entry_dropped_or_renamed_in_the_new_archive_leaves_nothing_behind() { + let dir = tempfile::tempdir().unwrap(); + let target = dir.path().join("out"); + let mut diagnostics = Vec::new(); + let archive = zip_with( + dir.path(), + "hw.zip", + &[("lab5.py", b"x=1"), ("util.py", b"y=1"), ("old.py", b"z=1")], + ); + expand(&archive, &target, &is_gradeable, &mut diagnostics); + assert_eq!(listing(&target), ["lab5.py", "old.py", "util.py"]); + + // util.py is gone; old.py is now new.py. + let archive = zip_with( + dir.path(), + "hw.zip", + &[("lab5.py", b"x=1"), ("new.py", b"z=1")], + ); + let files = expand(&archive, &target, &is_gradeable, &mut diagnostics); + + assert_eq!(listing(&target), ["lab5.py", "new.py"]); + let entries: Vec<&str> = files.iter().map(|f| f.entry.as_str()).collect(); + assert_eq!(entries, ["lab5.py", "new.py"]); + } + + #[test] + fn test_an_unchanged_archive_expands_to_the_same_files_every_time() { + let dir = tempfile::tempdir().unwrap(); + let target = dir.path().join("out"); + let archive = zip_with( + dir.path(), + "hw.zip", + &[("src/lab5.py", b"x=1"), ("util.py", b"y=1")], + ); + let expanded = || { + let mut diagnostics = Vec::new(); + let files: Vec<(PathBuf, String, Vec)> = + expand(&archive, &target, &is_gradeable, &mut diagnostics) + .into_iter() + .map(|f| { + let bytes = std::fs::read(&f.out_path).unwrap(); + (f.out_path, f.entry, bytes) + }) + .collect(); + assert!(diagnostics.is_empty(), "got {diagnostics:?}"); + files + }; + + let first = expanded(); + assert_eq!(first.len(), 2); + assert_eq!(expanded(), first); + } + + /// A replacement that cannot be opened is reported, and grades nothing — least of all + /// the files its predecessor left. + #[test] + fn test_an_unreadable_replacement_leaves_nothing_of_the_old_extraction() { + let dir = tempfile::tempdir().unwrap(); + let target = dir.path().join("out"); + let archive = zip_with(dir.path(), "hw.zip", &[("lab5.py", b"x=1")]); + expand(&archive, &target, &is_gradeable, &mut Vec::new()); + + std::fs::write(&archive, b"not a zip").unwrap(); + let mut diagnostics = Vec::new(); + let files = expand(&archive, &target, &is_gradeable, &mut diagnostics); + + assert!(files.is_empty()); + assert!(matches!( + &diagnostics[..], + [d] if matches!(d.kind, DiagnosticKind::ArchiveUnreadable { .. }) + )); + assert!(listing(&target).is_empty()); + } } diff --git a/crates/scriptmark/src/canvas/bundle.rs b/crates/scriptmark/src/canvas/bundle.rs index 1ed367d..4651097 100644 --- a/crates/scriptmark/src/canvas/bundle.rs +++ b/crates/scriptmark/src/canvas/bundle.rs @@ -482,4 +482,50 @@ mod tests { assert_eq!(entry.expanded[0].entry, "src/Lab5.py"); assert!(entry.expanded[0].path.is_file()); } + + /// P-868: a re-fetch replaces the attachment's bytes at the same path — a download + /// whose size did not match, or a hand-repaired bundle. The next load expands the new + /// archive, not what the old one left beside it. + #[test] + fn test_a_replaced_zip_attachment_is_expanded_from_its_new_bytes() { + let dir = tempfile::tempdir().unwrap(); + let root = dir.path(); + let zip_path = root.join("attachments/3/work.zip"); + std::fs::create_dir_all(zip_path.parent().unwrap()).unwrap(); + std::fs::write(root.join(PAYLOAD_FILE), "{}").unwrap(); + let store = |entries: &[(&str, &str)]| { + use std::io::Write as _; + let mut zip = zip::ZipWriter::new(std::fs::File::create(&zip_path).unwrap()); + for (name, body) in entries { + zip.start_file(*name, zip::write::SimpleFileOptions::default()) + .unwrap(); + zip.write_all(body.as_bytes()).unwrap(); + } + zip.finish().unwrap(); + let manifest: Manifest = Manifest::from([( + "3".to_string(), + AttachmentRecord::Stored { + path: zip_path.clone(), + size: std::fs::metadata(&zip_path).unwrap().len(), + }, + )]); + std::fs::write( + root.join(MANIFEST_FILE), + serde_json::to_string(&manifest).unwrap(), + ) + .unwrap(); + }; + store(&[("src/Lab5.py", "wrong"), ("src/util.py", "u")]); + load(root).unwrap(); + + store(&[("src/Lab5.py", "right")]); + let (_, downloads, diagnostics) = load(root).unwrap(); + + assert!(diagnostics.is_empty(), "got {diagnostics:?}"); + let entry = downloads.get(&3).unwrap().as_ref().unwrap(); + assert_eq!(entry.expanded.len(), 1); + let lab5 = &entry.expanded[0].path; + assert_eq!(std::fs::read_to_string(lab5).unwrap(), "right"); + assert!(!lab5.with_file_name("util.py").exists()); + } } diff --git a/crates/scriptmark/src/discovery.rs b/crates/scriptmark/src/discovery.rs index 3ffe805..5280d2e 100644 --- a/crates/scriptmark/src/discovery.rs +++ b/crates/scriptmark/src/discovery.rs @@ -17,7 +17,7 @@ use crate::models::{ }; use crate::roster::Roster; -/// Directory archives are expanded into, beside the directory being scanned. +/// Directory archives are expanded into, inside the directory being scanned. const EXTRACT_DIR: &str = ".scriptmark_extracted"; /// Map file extensions to language identifiers. @@ -55,7 +55,10 @@ fn extract_sid(filename: &str) -> Option { Some(sid) } -/// Expand every archive in a directory into `{EXTRACT_DIR}/{archive_stem}/`. +/// Expand every archive in a directory into `{EXTRACT_DIR}/{archive file name}/`. +/// +/// Named by the whole file name, not the stem: each expansion clears its directory, so +/// `hw.zip` and `hw.tar` must not share one, and the stem of `...zip` is `..`. fn extract_archives( dir: &Path, diagnostics: &mut Vec, @@ -90,11 +93,8 @@ fn extract_archives( archives.sort(); for archive in archives { - let stem = archive - .file_stem() - .and_then(|s| s.to_str()) - .unwrap_or("unknown"); - let target = extract_root.join(stem); + let name = archive.file_name().expect("a directory entry has a name"); + let target = extract_root.join(name); extracted.extend(crate::archive::expand( &archive, &target, @@ -178,7 +178,7 @@ pub fn load_local_input( .map(|p| p.as_ref().to_path_buf()) .chain(extra_dirs) .collect(); - // Walk nested submissions; never revisit the extraction cache or directory symlinks. + // Walk nested submissions; never revisit the extraction directory or directory symlinks. let mut index = 0; while index < all_paths.len() { let dir = &all_paths[index]; @@ -269,7 +269,7 @@ pub fn load_local_input( .and_then(|e| e.to_str()) .unwrap_or("") .to_lowercase(); - // Inside an extraction directory the archive stem carries the key; the files + // A file out of an archive takes its key from the archive's name; the files // within it are named by the student. let default_key = if let Some(FileOrigin::Archive { archive, .. }) = origins.get(&path) { @@ -543,16 +543,143 @@ mod tests { } ); - // A second run reuses the extraction and must still report where the file came from. + // A second run extracts again and must report the same file, from the same place. let again = scan(dir.path()); let bob_again = again .students .iter() .find(|s| s.key().raw() == "bob") .expect("bob"); + assert_eq!(bob_again.files()[0].path, bob.files()[0].path); assert_eq!(bob_again.files()[0].origin, bob.files()[0].origin); } + fn zip_at(path: &Path, entries: &[(&str, &str)]) { + use std::io::Write as _; + let mut zip = zip::ZipWriter::new(std::fs::File::create(path).unwrap()); + for (entry, body) in entries { + zip.start_file(*entry, zip::write::SimpleFileOptions::default()) + .unwrap(); + zip.write_all(body.as_bytes()).unwrap(); + } + zip.finish().unwrap(); + } + + fn bodies(student: &StudentSubmission) -> Vec<(String, String)> { + student + .files() + .iter() + .map(|f| (f.file_name(), std::fs::read_to_string(&f.path).unwrap())) + .collect() + } + + /// P-868: the student replaces `bob_hw.zip`, fixing `sum.py` and renaming `util.py`. + /// The scan grades the new bytes, and the old `util.py` does not linger in the + /// extraction to be picked up as a submission of its own. + #[test] + fn test_a_replaced_archive_is_scanned_from_its_new_contents() { + let dir = tempfile::tempdir().unwrap(); + let zip_path = dir.path().join("bob_hw.zip"); + zip_at(&zip_path, &[("sum.py", "wrong"), ("util.py", "u")]); + let first = scan(dir.path()); + assert_eq!( + bodies(&first.students[0]), + [ + ("sum.py".into(), "wrong".into()), + ("util.py".into(), "u".into()) + ] + ); + + zip_at(&zip_path, &[("sum.py", "right"), ("helpers.py", "u")]); + let input = scan(dir.path()); + + let keys: Vec = input.students.iter().map(|s| s.key().raw()).collect(); + assert_eq!(keys, ["bob"]); + assert_eq!( + bodies(&input.students[0]), + [ + ("helpers.py".into(), "u".into()), + ("sum.py".into(), "right".into()) + ] + ); + assert!(input.unmatched.is_empty(), "got {:?}", input.unmatched); + } + + /// `bob_hw.zip` and `bob_hw.tar` share a stem. Each archive gets its own extraction, so + /// refreshing one never deletes or relabels the other's files. + #[test] + fn test_archives_sharing_a_stem_keep_separate_extractions() { + let dir = tempfile::tempdir().unwrap(); + let zip_path = dir.path().join("bob_hw.zip"); + zip_at(&zip_path, &[("lab.py", "from zip")]); + let tar_path = dir.path().join("bob_hw.tar"); + { + let mut tar = tar::Builder::new(std::fs::File::create(&tar_path).unwrap()); + let mut header = tar::Header::new_gnu(); + header.set_size(8); + header.set_mode(0o644); + header.set_cksum(); + tar.append_data(&mut header, "lab.py", &b"from tar"[..]) + .unwrap(); + tar.finish().unwrap(); + } + + for _ in 0..2 { + let input = scan(dir.path()); + let bob = &input.students[0]; + let mut got: Vec<(PathBuf, String)> = bob + .files() + .iter() + .map(|f| match &f.origin { + FileOrigin::Archive { archive, .. } => { + (archive.clone(), std::fs::read_to_string(&f.path).unwrap()) + } + other => panic!("not from an archive: {other:?}"), + }) + .collect(); + got.sort(); + assert_eq!( + got, + [ + (tar_path.clone(), "from tar".to_string()), + (zip_path.clone(), "from zip".to_string()) + ] + ); + } + } + + /// The stem of `...zip` is `..` and of `..zip` is `.`, so an extraction directory named + /// after the stem would be the scanned directory itself, or every archive's extraction + /// at once. Refreshing that would delete the submissions. + #[test] + fn test_extraction_stays_inside_its_own_directory_whatever_the_archive_is_called() { + let dir = tempfile::tempdir().unwrap(); + std::fs::write(dir.path().join("alice_sum.py"), "a").unwrap(); + zip_at(&dir.path().join("bob_hw.zip"), &[("sum.py", "b")]); + zip_at(&dir.path().join("...zip"), &[("x.py", "x")]); + zip_at(&dir.path().join("..zip"), &[("y.py", "y")]); + + for _ in 0..2 { + let input = scan(dir.path()); + let bob = input + .students + .iter() + .find(|s| s.key().raw() == "bob") + .expect("bob"); + assert_eq!(bodies(bob), [("sum.py".into(), "b".into())]); + } + assert_eq!( + std::fs::read_to_string(dir.path().join("alice_sum.py")).unwrap(), + "a" + ); + // Each archive landed in its own directory, and nothing beside the submissions. + let extracted = dir.path().join(EXTRACT_DIR); + assert!(extracted.join("...zip/x.py").is_file()); + assert!(extracted.join("..zip/y.py").is_file()); + assert!(!dir.path().join("x.py").exists()); + assert!(!dir.path().join("y.py").exists()); + } + /// Characterisation test for the zip-bomb guards, written before P-670 lifts this loop /// into a shared `expand_archive` that Canvas attachments also feed. An oversized entry /// must be reported and skipped *without* taking its archive-mates down with it — the diff --git a/crates/scriptmark/src/models/result.rs b/crates/scriptmark/src/models/result.rs index 28fca74..8dd7b8d 100644 --- a/crates/scriptmark/src/models/result.rs +++ b/crates/scriptmark/src/models/result.rs @@ -301,7 +301,7 @@ pub struct SubmissionVersion { /// Every runnable file of the attempt, by path. pub files: Vec, /// The archives those files were extracted from: a replaced archive is a changed - /// submission even while an earlier extraction still sits on disk. + /// submission even when the files graded out of it are byte-identical. #[serde(default, skip_serializing_if = "Vec::is_empty")] pub archives: Vec, } diff --git a/crates/scriptmark/tests/record.rs b/crates/scriptmark/tests/record.rs index 793e3ef..b6cc39f 100644 --- a/crates/scriptmark/tests/record.rs +++ b/crates/scriptmark/tests/record.rs @@ -682,20 +682,27 @@ fn the_assignment_name_is_a_label_so_declaring_items_later_rescores() { assert_eq!(grades(dir, None), graded(100.0, 0.0)); } +/// bob hands in `bob_hw.zip` instead of `bob_sum.py`. +fn bob_zips(dir: &Path, entries: &[(&str, &str)]) { + std::fs::remove_file(dir.join("submissions/bob_sum.py")).ok(); + let file = std::fs::File::create(dir.join("submissions/bob_hw.zip")).unwrap(); + let mut zip = zip::ZipWriter::new(file); + for (name, content) in entries { + zip.start_file(*name, zip::write::SimpleFileOptions::default()) + .unwrap(); + std::io::Write::write_all(&mut zip, content.as_bytes()).unwrap(); + } + zip.finish().unwrap(); +} + +const WRONG: &str = "def add(a, b):\n return abs(a) + abs(b)\n"; +const RIGHT: &str = "def add(a, b):\n return a + b\n"; + #[test] -fn a_replaced_archive_is_a_changed_submission_even_behind_a_stale_extraction() { +fn a_replaced_archive_is_a_changed_submission() { let temp = bench(); let dir = temp.path(); - let zip_of = |content: &str| { - std::fs::remove_file(dir.join("submissions/bob_sum.py")).ok(); - let file = std::fs::File::create(dir.join("submissions/bob_hw.zip")).unwrap(); - let mut zip = zip::ZipWriter::new(file); - zip.start_file("sum.py", zip::write::SimpleFileOptions::default()) - .unwrap(); - std::io::Write::write_all(&mut zip, content.as_bytes()).unwrap(); - zip.finish().unwrap(); - }; - zip_of("def add(a, b):\n return abs(a) + abs(b)\n"); + bob_zips(dir, &[("sum.py", WRONG)]); succeeded(scriptmark(dir, &GRADE)); let bob = record(dir) .evidence @@ -705,12 +712,36 @@ fn a_replaced_archive_is_a_changed_submission_even_behind_a_stale_extraction() { .unwrap(); assert_eq!(bob.submission.unwrap().archives.len(), 1); - // The extraction from the first archive stays on disk; the archive itself changed. - zip_of("def add(a, b):\n return a + b\n"); + bob_zips(dir, &[("sum.py", RIGHT)]); all_or_nothing(dir); refused(rescore(dir, &[]), "local:bob's submitted files changed"); } +/// P-868, as reported: bob fixes `sum.py` and re-uploads `bob_hw.zip` under the same name, +/// dropping a helper on the way. Grading again grades the new archive — and only it. +#[test] +fn a_replaced_archive_is_graded_from_its_new_contents() { + let temp = bench(); + let dir = temp.path(); + bob_zips(dir, &[("sum.py", WRONG), ("util.py", "")]); + succeeded(scriptmark(dir, &GRADE)); + assert_eq!(grades(dir, None), graded(100.0, 50.0)); + + bob_zips(dir, &[("sum.py", RIGHT)]); + succeeded(scriptmark(dir, &GRADE)); + // Exactly two students: the dropped util.py does not come back as a student "util". + assert_eq!(grades(dir, None), graded(100.0, 100.0)); + let bob = record(dir) + .evidence + .students + .into_iter() + .find(|r| r.student_id == "local:bob") + .unwrap(); + let files = bob.submission.unwrap().files; + assert_eq!(files.len(), 1); + assert_eq!(std::fs::read_to_string(&files[0].path).unwrap(), RIGHT); +} + /// A Canvas bundle as `canvas fetch` leaves one, for course 7, assignment 8. Ada tried /// twice — wrong, then right — Ben once, wrongly, and Cy, who handed nothing in, is excused. fn canvas_bundle(dir: &Path) { diff --git a/notes b/notes index b64db70..f7bba71 160000 --- a/notes +++ b/notes @@ -1 +1 @@ -Subproject commit b64db70d71a2b08b50bc636244da07bcd80266dc +Subproject commit f7bba717190efbe26fe55b8bffe89981f588d7a8