fix(odf): fail validation when package parts are unreadable

scripts/validate-odf.mjs

@@ -39,7 +39,8 @@
 import { mkdtempSync, writeFileSync, mkdirSync } from "node:fs"
 import { execFileSync } from "node:child_process"
 import { tmpdir } from "node:os"
-import { join, dirname } from "node:path"
+import { join, dirname, resolve } from "node:path"
+import { pathToFileURL } from "node:url"
 
 const args = new Map()
 for (let i = 2; i < process.argv.length; i += 2) {
@@ -48,7 +49,13 @@ for (let i = 2; i < process.argv.length; i += 2) {
 const schema = args.get("schema")
 const manifestSchema = args.get("manifest-schema")
 const jing = args.get("jing")
-const dist = args.get("dist") ?? new URL("../dist/index.mjs", import.meta.url).href
+// A URL rather than the path as typed: `extract` resolves the ZIP reader
+// relative to it, so `--dist` points at one bundle instead of the bundle
+// for the library and whatever happens to sit in ../dist for the reader.
+const distArg = args.get("dist")
+const dist = distArg
+  ? pathToFileURL(resolve(distArg)).href
+  : new URL("../dist/index.mjs", import.meta.url).href
 
 if (!schema || !jing) {
   console.error("usage: node scripts/validate-odf.mjs --schema <odf.rng> --jing <jing.jar>")
@@ -168,9 +175,7 @@ const PARTS = ["content.xml", "styles.xml", "meta.xml", "settings.xml"]
 
 /** Pull one entry out of an ODS with hucre's own ZIP reader. */
 async function extract(bytes, path) {
-  const { ZipReader } = await import(new URL("../dist/zip/reader.mjs", import.meta.url).href).catch(
-    () => ({}),
-  )
+  const { ZipReader } = await import(new URL("zip/reader.mjs", dist).href).catch(() => ({}))
   if (ZipReader) return new TextDecoder().decode(await new ZipReader(bytes).extract(path))
   // The bundle does not export it; read the workbook back through the
   // public API is not enough, so fall back to `unzip -p`.
@@ -194,19 +199,34 @@ function validate(file, rng) {
 
 const work = mkdtempSync(join(tmpdir(), "hucre-odf-val-"))
 let failures = 0
+let skipped = 0
 
 for (const [label, bytes] of await documents()) {
   console.log(`\n${label}  (${bytes.length} bytes)`)
 
-  const checks = [...PARTS.map((p) => [p, schema])]
-  if (manifestSchema) checks.push(["META-INF/manifest.xml", manifestSchema])
+  const checks = PARTS.map((p) => [p, schema])
+  if (manifestSchema) {
+    checks.push(["META-INF/manifest.xml", manifestSchema])
+  } else {
+    // A check that did not run is not a check that passed. Saying so is
+    // the difference between "valid" and "nobody looked" (#566).
+    skipped++
+    console.log(`  ${"META-INF/manifest.xml".padEnd(24)} SKIPPED (no manifest schema supplied)`)
+  }
 
   for (const [part, rng] of checks) {
     let xml
     try {
       xml = await extract(bytes, part)
     } catch {
-      console.log(`  ${part.padEnd(24)} (absent)`)
+      // Every part in PARTS is written by all three ODS writers, so one
+      // that will not come out of the package is a defect rather than an
+      // absence — and a package that cannot be opened at all lands every
+      // part here. Counting these as checks that passed is what let the
+      // script print "All parts valid." for a document it never read
+      // (#566).
+      failures++
+      console.log(`  ${part.padEnd(24)} MISSING/UNREADABLE`)
       continue
     }
 
@@ -225,5 +245,11 @@ for (const [label, bytes] of await documents()) {
   }
 }
 
-console.log(failures === 0 ? "\nAll parts valid." : `\n${failures} invalid part(s).`)
+console.log(
+  failures > 0
+    ? `\n${failures} invalid or unreadable part(s).`
+    : skipped > 0
+      ? `\nAll checked parts valid; ${skipped} manifest check(s) skipped (no manifest schema supplied).`
+      : "\nAll parts valid.",
+)
 process.exit(failures === 0 ? 0 : 1)

test/odf-validation.test.ts

@@ -0,0 +1,128 @@
+import { afterEach, describe, expect, it } from "vitest"
+import { mkdtempSync, mkdirSync, rmSync, writeFileSync } from "node:fs"
+import { tmpdir } from "node:os"
+import { delimiter, join } from "node:path"
+import { fileURLToPath } from "node:url"
+import { spawnSync } from "node:child_process"
+import type { SpawnSyncReturns } from "node:child_process"
+
+// The validator is a developer script, not library code: it needs `jing`,
+// a 40 MB OASIS grammar and a JVM, so the suite cannot run it for real.
+// What it can run is the script's own bookkeeping — which parts counted
+// as checked — against stub `unzip` and `java` on PATH and a stub bundle
+// behind `--dist`. That is the part #566 got wrong.
+
+let work: string | undefined
+
+afterEach(() => {
+  if (work) rmSync(work, { recursive: true, force: true })
+  work = undefined
+})
+
+/** The line the script prints for one part, in its own column layout. */
+function line(part: string, status: string): string {
+  return `  ${part.padEnd(24)} ${status}`
+}
+
+function runValidator(unzipMode: "missing-settings" | "unreadable"): SpawnSyncReturns<string> {
+  work = mkdtempSync(join(tmpdir(), "hucre-odf-validation-test-"))
+  const bin = join(work, "bin")
+  mkdirSync(bin)
+
+  // A stub for every export `documents()` reaches for. Add to the script
+  // and this list grows with it — the alternative is building dist to
+  // test the counting.
+  const dist = join(work, "fake-dist.mjs")
+  writeFileSync(
+    dist,
+    `export async function writeOds() { return new Uint8Array([1]) }
+export function writeOdsStream() {
+  let read = false
+  return { getReader: () => ({ async read() {
+    if (read) return { done: true }
+    read = true
+    return { done: false, value: new Uint8Array([1]) }
+  } }) }
+}
+export class OdsStreamWriter {
+  addRow() {}
+  async finish() { return new Uint8Array([1]) }
+}
+export function createCellStore() { return {} }
+export function setCell() {}
+`,
+  )
+
+  // `unzip -p <file> <part>`: one part refuses to come out, or the whole
+  // package does.
+  writeFileSync(
+    join(bin, "unzip"),
+    `#!/bin/sh
+if [ "$FAKE_UNZIP_MODE" = "missing-settings" ] && [ "$3" != "settings.xml" ]; then
+  printf '<valid/>'
+  exit 0
+fi
+exit 1
+`,
+    { mode: 0o755 },
+  )
+  // jing, accepting everything it is handed.
+  writeFileSync(join(bin, "java"), "#!/bin/sh\nexit 0\n", { mode: 0o755 })
+
+  return spawnSync(
+    process.execPath,
+    [
+      fileURLToPath(new URL("../scripts/validate-odf.mjs", import.meta.url)),
+      "--schema",
+      "unused.rng",
+      "--jing",
+      "unused.jar",
+      "--dist",
+      dist,
+    ],
+    {
+      cwd: work,
+      encoding: "utf8",
+      env: {
+        ...process.env,
+        PATH: `${bin}${delimiter}${process.env["PATH"]}`,
+        FAKE_UNZIP_MODE: unzipMode,
+      },
+    },
+  )
+}
+
+describe("ODF validator CLI", () => {
+  it("fails when a required package part is missing (#566)", () => {
+    const result = runValidator("missing-settings")
+
+    expect(result.status).toBe(1)
+    expect(result.stdout).toContain(line("settings.xml", "MISSING/UNREADABLE"))
+    // The parts that did come out are still reported as checked, so the
+    // failure is the missing one rather than a package-wide collapse.
+    expect(result.stdout).toContain(line("content.xml", "valid"))
+    expect(result.stdout).toContain(line("styles.xml", "valid"))
+    expect(result.stdout).toContain(line("meta.xml", "valid"))
+    // One part × the three documents the script writes.
+    expect(result.stdout).toContain("3 invalid or unreadable part(s).")
+    expect(result.stdout).not.toContain("All parts valid.")
+  })
+
+  it("fails when the package cannot be read at all (#566)", () => {
+    const result = runValidator("unreadable")
+
+    expect(result.status).toBe(1)
+    expect(result.stdout).toContain(line("content.xml", "MISSING/UNREADABLE"))
+    expect(result.stdout).not.toContain(line("content.xml", "valid"))
+    // Four parts × three documents, none of them checked.
+    expect(result.stdout).toContain("12 invalid or unreadable part(s).")
+  })
+
+  it("names the manifest check as skipped when no schema is supplied (#566)", () => {
+    const result = runValidator("unreadable")
+
+    expect(result.stdout).toContain(
+      line("META-INF/manifest.xml", "SKIPPED (no manifest schema supplied)"),
+    )
+  })
+})

tsconfig.cli.json

@@ -22,6 +22,7 @@
     "test/cli.test.ts",
     "test/threaded-comments-write.test.ts",
     "test/ods-fidelity.test.ts",
+    "test/odf-validation.test.ts",
     "test/parity-statement.test.ts",
     "test/real-files.test.ts",
     "test/ods-third-party.test.ts",

tsconfig.json

@@ -42,6 +42,9 @@
     // reason the file-reading tests above are here. See #474.
     "test/_reflect.ts",
     "test/reflect.test.ts",
+    // Spawns `scripts/validate-odf.mjs` with stub tools on PATH, so it
+    // needs `node:child_process` and `node:fs`.
+    "test/odf-validation.test.ts",
     // Reads the fixture corpus off disk, so it needs `node:fs`.
     "test/third-party-fixtures.test.ts",
     "test/ods-third-party.test.ts",