diff --git a/src/commands/upload.rs b/src/commands/upload.rs index 5602c65..058318a 100644 --- a/src/commands/upload.rs +++ b/src/commands/upload.rs @@ -180,7 +180,9 @@ pub async fn upload(ctx: &Context, args: &UploadArgs) -> Result<()> { } } + let files_refused = failures.len(); let mut added = 0u64; + let mut fills_done = 0usize; for (album_id, asset_ids) in &by_album { // Album membership goes on in chunks: an imported album can hold thousands. for chunk in asset_ids.chunks(500) { @@ -190,8 +192,16 @@ pub async fn upload(ctx: &Context, args: &UploadArgs) -> Result<()> { .add_assets(album_id, &AssetSelection::ids(chunk)) .await { - Ok(result) => added += result.added, - Err(error) => ctx.out.warn(format!("Could not fill an album: {error}")), + Ok(result) => { + added += result.added; + fills_done += 1; + } + // Filing what was uploaded is part of the run, so a refused fill is one + // of the run's failures: it carries the fields the server named, it is in + // the one document `--json` writes, and it counts towards `$?`. Named by + // the album's id where a file's failure is named by its path, which is + // also what tells the two apart in the list. + Err(error) => failures.push(Failure::for_id(album_id, &error)), } } } @@ -230,8 +240,28 @@ pub async fn upload(ctx: &Context, args: &UploadArgs) -> Result<()> { } } + // A run that files into albums has two kinds of item in it, and an album is not a + // file: counting a refused fill among the files would close a run that sent one file + // with "1 of 2 files failed". "Item" is what the README already calls a batch's + // members, and it is only reached for by a run that had one of the other kind. + // + // The noun and what the total counts move together, because the noun names what is + // being counted: once fills are on the failed side they are on the succeeded side + // too, the way `download` counts a file it skipped alongside one it wrote. Counting + // them under "file" would be the same miscount the other way round. + let (noun, fills_through) = if failures.len() > files_refused { + ("item", fills_done) + } else { + ("file", 0) + }; // A file the server already had is not a file that failed, so it counts as got through. - crate::commands::finish_batch(ctx, summary, failures.len(), uploaded.len(), "file") + crate::commands::finish_batch( + ctx, + summary, + failures.len(), + uploaded.len() + fills_through, + noun, + ) } /// One line of the `--report` JSONL: what became of one file, in the API's own field diff --git a/src/errors.rs b/src/errors.rs index 954ff48..a9c89aa 100644 --- a/src/errors.rs +++ b/src/errors.rs @@ -46,6 +46,11 @@ pub struct Failure { pub error: String, #[serde(skip_serializing_if = "Option::is_none")] pub details: Option
, + /// Whether the failure happened here rather than at the server, which decides only + /// which of the two names the sentence leads with. Not serialized: the record is the + /// API's own shape, and both names are in it either way. + #[serde(skip)] + local: bool, } impl Failure { @@ -57,6 +62,10 @@ impl Failure { path: None, error: error.to_string(), details: error.details().cloned(), + // `io::Error` carries no path of its own — "No space left on device" is the + // whole of what it knows — so the path this program was writing to is the + // only thing that says where the trouble is. + local: matches!(error, imogen_sdk::Error::Io(_)), } } @@ -69,12 +78,14 @@ impl Failure { } } - /// `assets edit` works from ids and never touches a file. + /// A failure with no file in it at all: `assets edit` works from ids, and so does the + /// album an upload could not fill. pub fn for_id(id: impl Into, error: &imogen_sdk::Error) -> Self { Self::of(error).with_id(id) } - /// A download knows which asset it was fetching; an upload only has the file. + /// The second name, for the one command that has both: a download knows the asset it + /// was fetching as well as the file it was writing. pub fn with_id(mut self, id: impl Into) -> Self { self.id = Some(id.into()); self @@ -83,8 +94,18 @@ impl Failure { /// What a person is told: whichever of the two names the command was working from, /// the server's sentence, and under it the fields it refused. Told only that three /// files failed, nobody can tell which field of which file was wrong. + /// + /// A download holds both names, and which one to lead with is the question the + /// failure answers: the server refused *this asset*, but the filesystem refused + /// *this file*, and being told the asset when the disk filled up leaves the person + /// without the directory to go and look at. pub fn message(&self) -> String { - let head = match self.id.as_deref().or(self.path.as_deref()) { + let (first, second) = if self.local { + (self.path.as_deref(), self.id.as_deref()) + } else { + (self.id.as_deref(), self.path.as_deref()) + }; + let head = match first.or(second) { Some(subject) => format!("{subject}: {}", self.error), None => self.error.clone(), }; @@ -321,6 +342,27 @@ mod tests { assert_eq!(serde_json::json!(failure)["path"], "/out/harbour.jpg"); } + /// The other half of the same record: a download that could not be written failed on + /// this machine, where the file is the subject. `io::Error` carries no path of its + /// own, so naming the asset leaves `warning: asset-7: io: No space left on device` — + /// the one thing the person needs, and the one thing it does not say. + #[test] + fn a_download_that_could_not_be_written_names_the_file() { + let error = imogen_sdk::Error::Io(std::io::Error::other("No space left on device")); + let failure = + Failure::new(Path::new("/out/2024/06/harbour.jpg"), &error).with_id("asset-7"); + let message = failure.message(); + assert!( + message.starts_with("/out/2024/06/harbour.jpg: "), + "{message} names the asset and never the file" + ); + // Both are still recorded: which asset is as much a part of the answer as which + // file, and only the sentence has to choose. + let record = serde_json::json!(&failure); + assert_eq!(record["id"], "asset-7"); + assert_eq!(record["path"], "/out/2024/06/harbour.jpg"); + } + /// An edit has no file to name, so the record carries the id alone and the person is /// told the field the server refused: ergofobe/imogen-cli#23. #[test] diff --git a/tests/batch_exit_status.rs b/tests/batch_exit_status.rs index de92239..4f95541 100644 --- a/tests/batch_exit_status.rs +++ b/tests/batch_exit_status.rs @@ -1,5 +1,6 @@ //! What a script sees when a batch partly fails: the exit status, and the one document -//! `--json` promises. ergofobe/imogen-cli#27. +//! `--json` promises. ergofobe/imogen-cli#27, and the two places that did not reach: +//! ergofobe/imogen-cli#34. //! //! These run the real program against a stub that answers the few requests a batch makes, //! because the two things under test — `$?` and the bytes on stdout — only exist once @@ -144,6 +145,36 @@ fn asset(id: &str) -> String { ) } +/// A library holding one album, which is what `upload --album` looks it up in. +fn album_called(name: &str) -> String { + format!( + r#"{{"items":[{{"id":"album-1","ownerId":"owner-1","name":"{name}","description":null, + "coverAssetId":null,"assetCount":0,"createdAt":"2024-01-01T00:00:00.000Z", + "updatedAt":"2024-01-01T00:00:00.000Z","shareSlug":null}}]}}"# + ) +} + +/// Two albums, which is what a manifest naming one per file files into. +fn two_albums() -> String { + r#"{"items":[ + {"id":"album-1","ownerId":"owner-1","name":"Trip","description":null, + "coverAssetId":null,"assetCount":0,"createdAt":"2024-01-01T00:00:00.000Z", + "updatedAt":"2024-01-01T00:00:00.000Z","shareSlug":null}, + {"id":"album-2","ownerId":"owner-1","name":"Harbour","description":null, + "coverAssetId":null,"assetCount":0,"createdAt":"2024-01-01T00:00:00.000Z", + "updatedAt":"2024-01-01T00:00:00.000Z","shareSlug":null}]}"# + .to_string() +} + +/// The album refusing what was uploaded, with the fields it faulted. +fn full_album() -> (u16, String) { + ( + 400, + r#"{"error":{"code":"validation_failed","message":"The request did not match what this endpoint expects","details":{"assetIds":["That album will not take any more"]}}}"# + .to_string(), + ) +} + fn uploaded(id: &str) -> (u16, String) { ( 201, @@ -296,6 +327,168 @@ fn a_download_reports_the_same_failure_to_both_audiences() { assert_eq!(documents[0]["failed"], 1); } +/// Filing what was uploaded is the other half of `upload --album`, and a server that +/// refused it used to be a sentence on stderr: absent from the `--json` document, +/// `addedToAlbums` silently short, exit 0. ergofobe/imogen-cli#34. +#[test] +fn an_album_that_could_not_be_filled_is_a_failure_like_any_other() { + let home = tempfile::tempdir().expect("a directory"); + let server = Server::start(|method, path| match (method, path) { + ("GET", "/api/v1/albums") => (200, album_called("Trip")), + ("POST", "/api/v1/albums/album-1/assets") => full_album(), + _ => uploaded("asset-1"), + }); + + let only = photograph(home.path(), "one.jpg"); + let prose = imogen( + &server.base, + home.path(), + &["upload", &only, "--album", "Trip"], + ); + // The file did get through, so this is the partial status rather than the one that + // says nothing worked. + assert_eq!(prose.status.code(), Some(EXIT_PARTIAL)); + let commentary = String::from_utf8_lossy(&prose.stderr); + assert!(commentary.contains("album-1"), "{commentary}"); + assert!( + commentary.contains("assetIds: That album will not take any more"), + "the fields the server named are the point: {commentary}" + ); + + let json = imogen( + &server.base, + home.path(), + &["--json", "upload", &only, "--album", "Trip"], + ); + assert_eq!(json.status.code(), Some(EXIT_PARTIAL)); + let documents = documents(&json); + assert_eq!(documents.len(), 1, "{documents:?}"); + assert_eq!(documents[0]["uploaded"], 1); + assert_eq!(documents[0]["addedToAlbums"], 0); + assert_eq!( + documents[0]["failed"], 1, + "the count and the list have to agree: {}", + documents[0] + ); + assert_eq!(documents[0]["failures"][0]["id"], "album-1"); + assert_eq!( + documents[0]["failures"][0]["details"]["assetIds"][0], + "That album will not take any more" + ); + // One file was sent, and it arrived. Closing with "1 of 2 files failed" would say two + // were, so a run holding both kinds of item says so. + assert_eq!(documents[0]["error"], "1 of 2 items failed"); + // A file's failure carries the path it was sent from; an album is not a file, which + // is what tells the two apart in one list. + assert!( + documents[0]["failures"][0].get("path").is_none(), + "{}", + documents[0] + ); +} + +/// The total has to count every item of the kind the noun names, on both sides of it. +/// A run filing into two albums where one fill is refused did four things, three of them +/// successfully — counting only the files would call it three, and drop the fill that +/// worked. `download` has summed its two kinds of success under one noun since #27. +#[test] +fn a_fill_that_worked_is_counted_beside_the_one_that_did_not() { + let home = tempfile::tempdir().expect("a directory"); + let server = Server::start(|method, path| match (method, path) { + ("GET", "/api/v1/albums") => (200, two_albums()), + ("POST", "/api/v1/albums/album-1/assets") => { + (200, r#"{"added":1,"skipped":0,"assetCount":1}"#.to_string()) + } + ("POST", "/api/v1/albums/album-2/assets") => full_album(), + _ => uploaded("asset-1"), + }); + + // One file for each album, which is what a manifest is for. + let first = photograph(home.path(), "one.jpg"); + let second = photograph(home.path(), "two.jpg"); + let manifest = home.path().join("manifest.jsonl"); + std::fs::write( + &manifest, + format!( + "{{\"path\":\"{first}\",\"album\":\"Trip\"}}\n{{\"path\":\"{second}\",\"album\":\"Harbour\"}}\n" + ), + ) + .expect("a manifest"); + + let output = imogen( + &server.base, + home.path(), + &[ + "--json", + "upload", + "--manifest", + &manifest.display().to_string(), + ], + ); + assert_eq!(output.status.code(), Some(EXIT_PARTIAL)); + let documents = documents(&output); + assert_eq!(documents.len(), 1, "{documents:?}"); + assert_eq!(documents[0]["uploaded"], 2); + assert_eq!(documents[0]["addedToAlbums"], 1); + assert_eq!(documents[0]["failed"], 1); + assert_eq!( + documents[0]["error"], "1 of 4 items failed", + "two files and two fills were attempted: {}", + documents[0] + ); +} + +/// A download that could not be written failed here, not at the server, and +/// `io::Error` carries no path of its own. Named by the asset, the person is told which +/// photograph and never which file or directory to go and look at. +/// ergofobe/imogen-cli#34. +#[test] +fn a_download_that_could_not_be_written_names_the_file() { + let home = tempfile::tempdir().expect("a directory"); + let server = Server::start(|_method, _path| (200, asset("asset-1"))); + + // A file where the output directory should be: every destination under it fails to + // be created, which is this machine's version of the disk being full. + let blocked = home.path().join("out"); + std::fs::write(&blocked, b"not a directory").expect("a file"); + + let prose = imogen( + &server.base, + home.path(), + &["download", "asset-1", "-o", &blocked.display().to_string()], + ); + assert_eq!(prose.status.code(), Some(EXIT_FAILED)); + let commentary = String::from_utf8_lossy(&prose.stderr); + assert!( + commentary.contains(&blocked.display().to_string()), + "the asset is named and the file is not: {commentary}" + ); + + // The record keeps both: which asset is as much a part of the answer as which file. + let json = imogen( + &server.base, + home.path(), + &[ + "--json", + "download", + "asset-1", + "-o", + &blocked.display().to_string(), + ], + ); + let documents = documents(&json); + assert_eq!(documents.len(), 1, "{documents:?}"); + assert_eq!(documents[0]["failures"][0]["id"], "asset-1"); + assert!( + documents[0]["failures"][0]["path"] + .as_str() + .expect("the record names the file") + .starts_with(&blocked.display().to_string()), + "{}", + documents[0] + ); +} + /// The rule only earns its keep if a run that worked still says so. #[test] fn a_batch_that_worked_still_exits_zero() {