From 6b5d9c20ee3cdc537f7430721cfe4d05ce8442dc Mon Sep 17 00:00:00 2001 From: funkpower Date: Fri, 13 Nov 2026 16:23:48 -0500 Subject: [PATCH] bibfetch: an edition's facts from its own record, and a cover whole to its end A lookup by ISBN took the year, publisher, language and page count from Open Library's index, which answers for the work and gathers them from every edition: Penguin's English War and Peace came back Spanish, Norton's, and 1845. The index now gives only the title, the authors and the subjects, and the edition record the rest (#6). A cover must run to its end marker, FF D9 for a JPEG and IEND for a PNG, so a body that only starts like an image is refused (#4). A relative redirect whose query holds another address is resolved as relative (#5). Closes #4, #5, #6. --- crates/bibfetch/README.md | 12 +++-- crates/bibfetch/src/cover.rs | 81 +++++++++++++++++++++++++++++++-- crates/bibfetch/src/source.rs | 84 ++++++++++++++++++++++++++--------- 3 files changed, 148 insertions(+), 29 deletions(-) diff --git a/crates/bibfetch/README.md b/crates/bibfetch/README.md index f643e08..5dddfa5 100644 --- a/crates/bibfetch/README.md +++ b/crates/bibfetch/README.md @@ -86,15 +86,18 @@ is a failure — nobody replied, and the identifier may be perfectly good. |---|---|---| | `doi` | Crossref | It assigns most scholarly DOIs | | `arxiv` | the arXiv | Its own register of preprints | -| `isbn` | OpenLibrary's edition record, then its index, then Google Books | Books, without a key | +| `isbn` | OpenLibrary's index, then its edition record, then Google Books | Books, without a key | One register is enough for a DOI or a preprint: the body that assigns the identifier is the only one that has it. A book is different. An ISBN is assigned by a publisher and recorded by whoever gets round to it, so an answer that names the book but not who wrote it is common — OpenLibrary's edition record names its authors by key rather than by name, and its index -names them. So the registers are asked in turn and the first complete answer -wins. An incomplete one is kept in case nothing better comes, and a register +names them. The index answers about the work, though, not the edition: the +publishers, languages, years and page counts it holds belong to every edition +at once, so only the title, the authors and the subjects are taken from it, +and the rest comes from the edition record for that ISBN. So the registers are +asked in turn and the first complete answer wins. An incomplete one is kept in case nothing better comes, and a register being down is a failure only when every register is. Google Books is asked last because it answers anonymous callers only until a @@ -127,7 +130,8 @@ id is asked only when every form of the ISBN asked was answered with no cover; i asking by ISBN failed, that failure is the result. What comes back is checked before it is written: a JPEG or PNG by its bytes -as well as by what the server says, no larger than ten megabytes, reached in at +as well as by what the server says, whole to its end marker (`FF D9` for a +JPEG, `IEND` for a PNG), no larger than ten megabytes, reached in at most three redirects, each over HTTPS to `covers.openlibrary.org`, `archive.org` or a host under `archive.org`, and never to an address by number. One request is made at a time, a `429` or `503` is waited out for as long as diff --git a/crates/bibfetch/src/cover.rs b/crates/bibfetch/src/cover.rs index d5b002b..5947ced 100644 --- a/crates/bibfetch/src/cover.rs +++ b/crates/bibfetch/src/cover.rs @@ -386,6 +386,23 @@ by ISBN every five minutes from one address" } } +/// Whether an image's bytes run to its end marker: `FF D9` for a JPEG, which +/// some encoders follow with zero padding, and the `IEND` chunk for a PNG, +/// which is its last twelve bytes. +fn ends_whole(media_type: &str, bytes: &[u8]) -> bool { + match media_type { + "image/jpeg" => { + let end = bytes.iter().rposition(|&b| b != 0).map_or(0, |at| at + 1); + end >= 5 && bytes[..end].ends_with(&[0xFF, 0xD9]) + } + _ => { + bytes.len() >= 8 + 12 + && bytes[bytes.len() - 12..].starts_with(&[0, 0, 0, 0]) + && &bytes[bytes.len() - 8..bytes.len() - 4] == b"IEND" + } + } +} + /// Read and check a body that is meant to be a cover. fn read(mut response: ureq::http::Response, address: &str) -> Result { let declared = header(&response, "content-type") @@ -438,6 +455,14 @@ fn read(mut response: ureq::http::Response, address: &str) -> Result "{address}: said {media_type} but is not one" ))); } + // The start of an image is not an image. A body sent with no length, + // or a stub, can begin right and stop short; what ends it says it is + // whole. + if !ends_whole(media_type, &bytes) { + return Err(Error::Shape(format!( + "{address}: a {media_type} that stops before its end" + ))); + } // No floor on the size. Open Library's stand-in for a missing cover is a // 43-byte, one-pixel GIF, and it serves that only when `default=false` // is left off: with it, as every address here has it, a missing cover is @@ -512,7 +537,12 @@ fn origin(url: &str) -> Option<&str> { /// Where a `Location` points, read against the address that sent it. fn resolve(from: &str, location: &str) -> String { - if location.contains("://") { + // Absolute only when the scheme comes first: a relative address can + // carry another address in its query. + if location + .find("://") + .is_some_and(|at| !location[..at].contains(['/', '?', '#'])) + { return location.to_string(); } if let Some(rest) = location.strip_prefix("//") { @@ -670,15 +700,19 @@ mod tests { .agent() } + /// A JPEG's start, filler, and its end marker, `len` bytes in all. fn jpeg(len: usize) -> Vec { let mut bytes = vec![0xFF, 0xD8, 0xFF, 0xE0]; - bytes.resize(len, 0x11); + bytes.resize(len - 2, 0x11); + bytes.extend([0xFF, 0xD9]); bytes } + /// A PNG's signature, filler, and its `IEND` chunk, `len` bytes in all. fn png(len: usize) -> Vec { let mut bytes = b"\x89PNG\r\n\x1a\n".to_vec(); - bytes.resize(len, 0x22); + bytes.resize(len - 12, 0x22); + bytes.extend(b"\0\0\0\0IEND\xAE\x42\x60\x82"); bytes } @@ -1170,6 +1204,38 @@ mod tests { assert_eq!(stub.asked(), [ISBN13, ISBN10]); } + #[test] + fn a_magic_number_alone_is_not_a_cover() { + let mut cut_jpeg = jpeg(4096); + cut_jpeg.truncate(4000); + let mut cut_png = png(4096); + cut_png.truncate(4000); + for (media_type, body) in [ + ("image/jpeg", vec![0xFF, 0xD8, 0xFF]), + ("image/png", b"\x89PNG\r\n\x1a\n".to_vec()), + ("image/jpeg", cut_jpeg), + ("image/png", cut_png), + ] { + let stub = Stub::start(move |_, _| Reply::image(media_type, body.clone())); + let found = stub + .covers() + .fetch(&agent(), "9780306406157", None, CoverSize::Large) + .map(|cover| cover.map(|cover| cover.bytes.len())); + assert!( + matches!(found, Err(Error::Shape(_))), + "{media_type}: {found:?}" + ); + } + } + + #[test] + fn a_jpeg_padded_after_its_end_marker_is_whole() { + let mut padded = jpeg(2048); + padded.extend([0, 0, 0]); + assert!(ends_whole("image/jpeg", &padded)); + assert!(!ends_whole("image/jpeg", &[0xFF, 0xD8, 0xFF, 0, 0])); + } + #[test] fn a_redirect_to_an_address_by_number_is_refused() { // A listener standing in for a service on the LAN; it only counts. @@ -1217,6 +1283,15 @@ mod tests { assert_eq!(resolve(from, "//c.example/x"), "https://c.example/x"); assert_eq!(resolve(from, "/x?y=1"), "https://a.example/x?y=1"); assert_eq!(resolve(from, "x.jpg"), "https://a.example/b/isbn/x.jpg"); + // An address in a relative one's query does not make it absolute. + assert_eq!( + resolve(from, "/view_archive.php?from=https://archive.org/x"), + "https://a.example/view_archive.php?from=https://archive.org/x" + ); + assert_eq!( + resolve(from, "x?u=https://c.example/"), + "https://a.example/b/isbn/x?u=https://c.example/" + ); assert_eq!( origin("http://127.0.0.1:8080/a?b"), Some("http://127.0.0.1:8080") diff --git a/crates/bibfetch/src/source.rs b/crates/bibfetch/src/source.rs index c7b1c96..f7d35e7 100644 --- a/crates/bibfetch/src/source.rs +++ b/crates/bibfetch/src/source.rs @@ -55,14 +55,13 @@ impl Source { match kind { "doi" => &[Source::Crossref], "arxiv" => &[Source::Arxiv], - // The index first. Asked for the fields by name it answers with - // the title, the authors, the publisher, the language, a page - // count and the subjects in one request, which is the whole of - // what a catalogue wants; the edition record is then needed only - // for a subtitle or a publisher the index did not give. Asking it - // second cost three requests per book where one usually does, and - // at three books in flight that is the difference between two - // days and two hours. + // The index first, for what is true of the whole work: the title, + // the authors by name and the subjects. It answers about the + // work, not the edition, so its publishers, languages, years and + // page counts belong to every edition at once, and Penguin's + // English War and Peace came back Spanish, Norton's, and 1845. + // The edition record says those for this ISBN; it names its + // authors only by key, which is why the index is still asked. "isbn" => &[ Source::OpenLibrarySearch, Source::OpenLibrary, @@ -90,8 +89,7 @@ impl Source { // edition's, in another translation or language. Source::OpenLibrarySearch => format!( "https://openlibrary.org/search.json?q=isbn:{}\ -&fields=title,subtitle,author_name,first_publish_year,publisher,language,subject,\ -number_of_pages_median&limit=1", +&fields=title,subtitle,author_name,subject&limit=1", encode(id) ), Source::GoogleBooks => format!( @@ -304,15 +302,7 @@ struct SearchDoc { #[serde(default)] author_name: Vec, #[serde(default)] - first_publish_year: Option, - #[serde(default)] - publisher: Vec, - #[serde(default)] - language: Vec, - #[serde(default)] subject: Vec, - #[serde(rename = "number_of_pages_median", default)] - pages: Option, } /// How many of a work's subject headings are worth carrying. @@ -336,14 +326,14 @@ fn open_library_search(id: &str, kind: &str, body: &str) -> Result title, }; record.authors = doc.author_name; - record.year = doc.first_publish_year; - record.container = doc.publisher.into_iter().next(); - record.language = doc.language.into_iter().next(); + // Not the year, publisher, language or page count: the index gives them + // for the work, gathered from every edition, and the edition record is + // asked for this one's. + // // The index lists every subject any edition of the work carries, which // runs to hundreds on a well-known book. A shelf needs a handful. record.subjects = doc.subject; record.subjects.truncate(SUBJECT_LIMIT); - record.pages = doc.pages; Ok(record) } @@ -594,6 +584,56 @@ mod tests { } } + #[test] + fn what_is_true_of_one_edition_comes_from_its_record_and_not_the_index() { + // ISBN 9780140447934, Penguin's English War and Peace, as the index + // answered on 2026-11-13: the work's languages, publishers and first + // year, gathered from every edition. + let index = r#"{"docs":[{"title":"War and Peace","author_name":["Leo Tolstoy"], + "first_publish_year":1845,"publisher":["W.W. Norton"], + "language":["spa","eng"],"number_of_pages_median":1296, + "subject":["Napoleonic Wars, 1800-1815"]}]}"#; + let alone = open_library_search("9780140447934", "isbn", index).expect("a record"); + assert_eq!(alone.title.as_deref(), Some("War and Peace")); + assert_eq!(alone.authors, ["Leo Tolstoy"]); + assert_eq!(alone.subjects, ["Napoleonic Wars, 1800-1815"]); + assert_eq!( + ( + alone.year, + alone.container.as_deref(), + alone.language.as_deref(), + alone.pages + ), + (None, None, None, None) + ); + // Not complete, so `lookup` goes on to ask the edition record. + assert!(!alone.complete()); + + let edition = r#"{"title":"War and Peace","publishers":["Penguin Classics"], + "publish_date":"2007","languages":[{"key":"/languages/eng"}], + "number_of_pages":1392}"#; + let mut kept = alone; + kept.fill_from(open_library("9780140447934", "isbn", edition).expect("a record")); + assert_eq!(kept.year, Some(2007)); + assert_eq!(kept.container.as_deref(), Some("Penguin Classics")); + assert_eq!(kept.language.as_deref(), Some("eng")); + assert_eq!(kept.pages, Some(1392)); + assert_eq!(kept.authors, ["Leo Tolstoy"]); + } + + #[test] + fn the_index_is_not_asked_for_what_belongs_to_editions() { + let url = Source::OpenLibrarySearch.url("1"); + for field in [ + "first_publish_year", + "publisher", + "language", + "number_of_pages", + ] { + assert!(!url.contains(field), "{field} in {url}"); + } + } + #[test] fn the_index_is_not_asked_for_the_works_cover() { assert!(!Source::OpenLibrarySearch.url("1").contains("cover_i"));