From 58b78a71ddaab4af8c737e5b1f6b168dfce881bb Mon Sep 17 00:00:00 2001 From: Nicolas Dreno Date: Tue, 3 Nov 2026 19:07:15 +0100 Subject: [PATCH] fix(import): tolerate common file variations and say where a file breaks (#19) #19 reported import errors on files their author considered standard, with no sample files. Probing the BeerXML parser showed a single odd value failed the whole import with an unlocated serde message: a decimal comma ("5,5"), lowercase or upper-case enum values ("boil"), common synonyms ("Whirlpool", "Malt"), or a hop without . BeerJSON imports only reported Ajv's first error, often a combinator ("must match a schema in anyOf") or a property-less "must NOT have additional properties". BeerXML (werb-beerxml, so also the CLI and the WASM import): - Numbers read a decimal comma; with a point present, commas are thousands separators. Failures say `not a number: ""`. - Enums go through LenientEnum: case, spacing and punctuation ignored, plus synonyms (Whirlpool / Hop stand / Flameout -> Aroma, Malt -> Grain, Whole / Cones -> Leaf, LME -> Extract...). Unknown values read as absent and the converter applies its default. - A missing hop ALPHA / AMOUNT, fermentable or misc AMOUNT reads as 0, like an empty element already did. - serde_path_to_error locates failures: Error::Invalid { location } with a 1-based XPath such as /RECIPES/RECIPE[1]/HOPS/HOP[2]/AMOUNT. JolieBulle JSON errors carry their field path too. BeerJSON (@werb/validate, used by file import and GitHub Pull): - stripNulls: null optional properties mean absent. - describeErrors: the deepest, most telling error, naming the missing or unknown property, or the value and the allowed ones. Tests: Rust (lenient values, located errors), validator unit tests, WASM import tests, BeerJSON import tests, e2e imports through the file picker. --- CHANGELOG.md | 11 + Cargo.lock | 12 ++ apps/desktop/src/data/recipes.ts | 9 +- apps/desktop/src/screens/Library.tsx | 4 +- apps/desktop/src/storage/github-recipes.ts | 10 +- apps/desktop/test/beerjson-roundtrip.test.ts | 19 ++ apps/desktop/test/beerxml-to-beerjson.test.ts | 33 +++ apps/desktop/test/e2e/pages.ts | 7 + apps/desktop/test/e2e/smoke.spec.ts | 55 +++++ crates/werb-beerxml/Cargo.toml | 1 + crates/werb-beerxml/src/error.rs | 12 +- crates/werb-beerxml/src/joliebulle.rs | 3 +- crates/werb-beerxml/src/model.rs | 192 ++++++++++++++---- crates/werb-beerxml/src/parse.rs | 36 +++- crates/werb-beerxml/tests/parse_sample.rs | 91 ++++++++- packages/validate/src/index.ts | 70 +++++++ packages/validate/test/validate.test.ts | 55 ++++- 17 files changed, 564 insertions(+), 56 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index dadc4d2..d36add1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -32,6 +32,17 @@ follow [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ### Fixed +- **BeerXML imports that failed on one odd value now go through** + ([#19](https://github.com/werb-dev/werb/issues/19)). Decimal commas + (`5,5`), values in another case (`boil`, `DRY HOP`) and common synonyms + (`Whirlpool`, `Hop stand` → Aroma; `Malt` → Grain; `Whole` → Leaf) are + read; an unknown value falls back to the default instead of failing the + whole file; a missing hop alpha or amount reads as 0, like an empty one. + BeerJSON files that write `null` for optional properties import too. +- **Import errors say where the file breaks** (#19). BeerXML errors give + the element, e.g. `/RECIPES/RECIPE[1]/HOPS/HOP[2]/AMOUNT: not a number: + "lots"`; BeerJSON errors name the missing or unknown property, or the + value and the allowed ones. Same messages for GitHub Pull. - **Default water on the web app.** New recipes now start from your water on the web too; they used to start from zero because the saved default loaded after the recipe had already picked its water. A recipe you haven't touched diff --git a/Cargo.lock b/Cargo.lock index bd61a20..acea6ae 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3328,6 +3328,17 @@ dependencies = [ "zmij", ] +[[package]] +name = "serde_path_to_error" +version = "0.1.20" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "10a9ff822e371bb5403e391ecd83e182e0e77ba7f6fe0160b795797109d1b457" +dependencies = [ + "itoa", + "serde", + "serde_core", +] + [[package]] name = "serde_repr" version = "0.1.20" @@ -4861,6 +4872,7 @@ dependencies = [ "quick-xml 0.36.2", "serde", "serde_json", + "serde_path_to_error", "thiserror 1.0.69", "werb-beerjson", ] diff --git a/apps/desktop/src/data/recipes.ts b/apps/desktop/src/data/recipes.ts index 5c47af1..0f8773a 100644 --- a/apps/desktop/src/data/recipes.ts +++ b/apps/desktop/src/data/recipes.ts @@ -1,5 +1,5 @@ import type { BeerJsonFile, BeerJsonRecipe } from "@werb/adapters"; -import { validateBeerJson } from "@werb/validate"; +import { describeErrors, stripNulls, validateBeerJson } from "@werb/validate"; import type { StorageBackend } from "../storage/index.ts"; import { isTauri } from "./runtime.ts"; import { WerbError } from "./errors.ts"; @@ -86,7 +86,7 @@ const bundledRaw = import.meta.glob("../../../../examples/*.beerjson", { function parseAndCollect(path: string, raw: string): BeerJsonRecipe[] { let parsed: unknown; try { - parsed = JSON.parse(raw); + parsed = stripNulls(JSON.parse(raw)); } catch (err) { console.warn(`[recipes] ${path}: invalid JSON — ${(err as Error).message}`); return []; @@ -150,7 +150,7 @@ export interface ImportResult { export function parseBeerJsonText(raw: string): ImportResult { let parsed: unknown; try { - parsed = JSON.parse(raw); + parsed = stripNulls(JSON.parse(raw)); } catch (err) { return { recipes: [], @@ -159,8 +159,7 @@ export function parseBeerJsonText(raw: string): ImportResult { } const result = validateBeerJson(parsed); if (!result.valid) { - const first = result.errors[0]; - const detail = first ? `${first.path}: ${first.message}` : "unknown"; + const detail = describeErrors(parsed, result.errors); return { recipes: [], error: new WerbError("import.not_beerjson", { detail }) }; } const recipes = (parsed as BeerJsonFile).beerjson?.recipes ?? []; diff --git a/apps/desktop/src/screens/Library.tsx b/apps/desktop/src/screens/Library.tsx index 99b7e91..1277f9b 100644 --- a/apps/desktop/src/screens/Library.tsx +++ b/apps/desktop/src/screens/Library.tsx @@ -147,7 +147,9 @@ export function LibraryScreen({

{t("library.import_failed")}

-

{importError}

+

+ {importError} +

)} diff --git a/apps/desktop/src/storage/github-recipes.ts b/apps/desktop/src/storage/github-recipes.ts index 1ab9313..99a5c14 100644 --- a/apps/desktop/src/storage/github-recipes.ts +++ b/apps/desktop/src/storage/github-recipes.ts @@ -13,7 +13,7 @@ */ import type { BeerJsonRecipe, BeerJsonFile } from "@werb/adapters"; -import { validateBeerJson } from "@werb/validate"; +import { describeErrors, stripNulls, validateBeerJson } from "@werb/validate"; import type { GitHubBackendConfig } from "./github.ts"; import { slugify } from "../data/recipe-export.ts"; import { @@ -249,18 +249,14 @@ export async function pullRecipes( if (!entry) continue; let parsed: BeerJsonFile; try { - parsed = JSON.parse(entry.raw) as BeerJsonFile; + parsed = stripNulls(JSON.parse(entry.raw)) as BeerJsonFile; } catch (err) { result.failed.push({ file: entry.file, error: `JSON parse: ${(err as Error).message}` }); continue; } const validation = validateBeerJson(parsed); if (!validation.valid) { - const first = validation.errors[0]; - result.failed.push({ - file: entry.file, - error: first ? `${first.path}: ${first.message}` : "schema invalid", - }); + result.failed.push({ file: entry.file, error: describeErrors(parsed, validation.errors) }); continue; } const incoming = parsed.beerjson?.recipes ?? []; diff --git a/apps/desktop/test/beerjson-roundtrip.test.ts b/apps/desktop/test/beerjson-roundtrip.test.ts index 94f71b0..3bb3151 100644 --- a/apps/desktop/test/beerjson-roundtrip.test.ts +++ b/apps/desktop/test/beerjson-roundtrip.test.ts @@ -108,6 +108,25 @@ describe("BeerJSON round-trip", () => { expect(result.error?.code).toBe("import.not_beerjson"); }); + it("imports a file that writes null for optional properties", () => { + const withNulls = JSON.parse(FIXTURE_RAW); + withNulls.beerjson.recipes[0].notes = null; + withNulls.beerjson.recipes[0].style = null; + const result = parseBeerJsonText(JSON.stringify(withNulls)); + expect(result.error).toBeUndefined(); + expect(result.recipes[0]).not.toHaveProperty("notes"); + }); + + it("names the property that breaks validation", () => { + const broken = JSON.parse(FIXTURE_RAW); + delete broken.beerjson.recipes[0].batch_size; + const result = parseBeerJsonText(JSON.stringify(broken)); + expect(result.error?.code).toBe("import.not_beerjson"); + expect(result.error?.params?.detail).toBe( + '/beerjson/recipes/0: missing required property "batch_size"', + ); + }); + it("rejects valid BeerJSON with no recipes inside", () => { const empty = JSON.stringify({ beerjson: { version: 2.06, recipes: [] } }); const result = parseBeerJsonText(empty); diff --git a/apps/desktop/test/beerxml-to-beerjson.test.ts b/apps/desktop/test/beerxml-to-beerjson.test.ts index ca0fa2a..04416d9 100644 --- a/apps/desktop/test/beerxml-to-beerjson.test.ts +++ b/apps/desktop/test/beerxml-to-beerjson.test.ts @@ -170,3 +170,36 @@ describe("BeerXML → BeerJSON validates against the schema", () => { expect(result.valid, JSON.stringify(result.errors)).toBe(true); }); }); + +describe("BeerXML import tolerates common variations (#19)", () => { + const wrap = (body: string) => + `Variations1` + + `all grain2060${body}`; + + it("imports decimal commas, lowercase values and common synonyms", () => { + const recipe = importXml( + wrap( + "PaleMalt4,5" + + "806" + + "Cascade5,50,03" + + "whirlpool
pellets
", + ), + ); + expect(validateBeerJson(asDocument(recipe)).valid).toBe(true); + expect(recipe.ingredients.fermentable_additions[0]!.amount.value).toBeCloseTo(4.5, 6); + expect(recipe.ingredients.fermentable_additions[0]!.type).toBe("grain"); + expect(recipe.ingredients.hop_additions![0]!.alpha_acid.value).toBeCloseTo(5.5, 6); + expect(recipe.ingredients.hop_additions![0]!.form).toBe("pellet"); + }); + + it("says where a file breaks", () => { + const xml = wrap( + "A50.01" + + "B5lots", + ); + expect(() => wasm.parseBeerXmlJson(xml)).toThrow( + '/RECIPES/RECIPE[1]/HOPS/HOP[2]/AMOUNT: not a number: "lots"', + ); + }); +}); + diff --git a/apps/desktop/test/e2e/pages.ts b/apps/desktop/test/e2e/pages.ts index 33e7eb6..f542fc5 100644 --- a/apps/desktop/test/e2e/pages.ts +++ b/apps/desktop/test/e2e/pages.ts @@ -131,6 +131,13 @@ class LibraryPage { } } + /** Import a recipe file through the Import button's file picker. */ + async importFile(name: string, content: string) { + const chooser = this.page.waitForEvent("filechooser"); + await this.page.getByRole("button", { name: /^Import recipes/i }).click(); + await (await chooser).setFiles({ name, mimeType: "text/plain", buffer: Buffer.from(content) }); + } + /** Create a new empty recipe and save it. Lands on the recipe view. */ async newEmptyRecipe(name: string) { await this.page.getByRole("button", { name: /\+\s*New recipe/i }).click(); diff --git a/apps/desktop/test/e2e/smoke.spec.ts b/apps/desktop/test/e2e/smoke.spec.ts index 85e35f0..ed1d3db 100644 --- a/apps/desktop/test/e2e/smoke.spec.ts +++ b/apps/desktop/test/e2e/smoke.spec.ts @@ -342,6 +342,61 @@ test.describe("Recipe editor — hops", () => { }); }); +test.describe("Import (#19)", () => { + const beerXml = (hops: string) => + `Comma Pale1` + + `All Grain2060` + + `PaleMalt4,5` + + `806${hops}`; + + test("a BeerXML with decimal commas and loose values imports", async ({ page }) => { + const app = new App(page); + await app.go("Library"); + await app.library.importFile( + "comma.xml", + beerXml("Cascade5,50,03whirlpool"), + ); + await expect(page.locator("li h2", { hasText: /comma pale/i })).toBeVisible(); + await expect(page.getByTestId("import-error")).toHaveCount(0); + }); + + test("a broken BeerXML says where it breaks", async ({ page }) => { + const app = new App(page); + await app.go("Library"); + await app.library.importFile( + "broken.xml", + beerXml( + "A50.01" + + "B5lots", + ), + ); + await expect(page.getByTestId("import-error")).toContainText( + '/RECIPES/RECIPE[1]/HOPS/HOP[2]/AMOUNT: not a number: "lots"', + ); + }); + + test("a BeerJSON error names the property", async ({ page }) => { + const app = new App(page); + await app.go("Library"); + const doc = { + beerjson: { + version: 2.06, + recipes: [ + { + name: "No batch", + type: "all grain", + author: "x", + efficiency: { brewhouse: { value: 72, unit: "%" } }, + ingredients: { fermentable_additions: [] }, + }, + ], + }, + }; + await app.library.importFile("bad.beerjson", JSON.stringify(doc)); + await expect(page.getByTestId("import-error")).toContainText('missing required property "batch_size"'); + }); +}); + test.describe("Discoverability", () => { test("Library onboarding mentions local storage + GitHub sync", async ({ page }) => { const app = new App(page); diff --git a/crates/werb-beerxml/Cargo.toml b/crates/werb-beerxml/Cargo.toml index 4cc98dc..fd3466d 100644 --- a/crates/werb-beerxml/Cargo.toml +++ b/crates/werb-beerxml/Cargo.toml @@ -13,6 +13,7 @@ readme = "README.md" quick-xml = { version = "0.36", features = ["serde", "serialize"] } serde = { version = "1", features = ["derive"] } serde_json = "1" +serde_path_to_error = "0.1" thiserror = "1" werb-beerjson = { path = "../werb-beerjson" } diff --git a/crates/werb-beerxml/src/error.rs b/crates/werb-beerxml/src/error.rs index a1a055a..2bc73f4 100644 --- a/crates/werb-beerxml/src/error.rs +++ b/crates/werb-beerxml/src/error.rs @@ -7,10 +7,20 @@ use thiserror::Error; #[derive(Debug, Error)] pub enum Error { /// The input is not well-formed XML, or does not match the expected - /// BeerXML structure (e.g. missing `` root). + /// BeerXML structure, at the document root (e.g. missing ``). #[error("BeerXML parse error: {0}")] Xml(#[from] quick_xml::DeError), + /// An element's content does not fit BeerXML, at `location`: an XPath + /// with 1-based indices, e.g. `/RECIPES/RECIPE[1]/HOPS/HOP[3]/AMOUNT`. + #[error("{location}: {source}")] + Invalid { + /// Where in the document the problem is. + location: String, + /// What is wrong there. + source: quick_xml::DeError, + }, + /// A field that should be a number could not be parsed as one. The /// inner string is the offending raw text from the XML. #[error("invalid number in field {field}: {raw:?}")] diff --git a/crates/werb-beerxml/src/joliebulle.rs b/crates/werb-beerxml/src/joliebulle.rs index 6ce3e12..4fb1889 100644 --- a/crates/werb-beerxml/src/joliebulle.rs +++ b/crates/werb-beerxml/src/joliebulle.rs @@ -36,7 +36,8 @@ use crate::model::{ /// the same Option semantics as the BeerXML parser. pub fn parse_joliebulle(json: &str) -> Result, Error> { let doc: JoliebulleDocument = - serde_json::from_str(json).map_err(|e| Error::Json(e.to_string()))?; + serde_path_to_error::deserialize(&mut serde_json::Deserializer::from_str(json)) + .map_err(|e| Error::Json(format!("{}: {}", e.path(), e.inner())))?; Ok(doc.recipes.into_iter().map(Recipe::from).collect()) } diff --git a/crates/werb-beerxml/src/model.rs b/crates/werb-beerxml/src/model.rs index 337b489..480dea9 100644 --- a/crates/werb-beerxml/src/model.rs +++ b/crates/werb-beerxml/src/model.rs @@ -16,13 +16,31 @@ where D: Deserializer<'de>, { match Option::::deserialize(de)? { - Some(s) if !s.trim().is_empty() => { - s.trim().parse::().map(Some).map_err(de::Error::custom) - } + Some(s) if !s.trim().is_empty() => number::(&s).map(Some), _ => Ok(None), } } +/// Parses a BeerXML number. A decimal comma (`5,5`, from comma-decimal +/// locales) reads as a point; with a point present, commas are thousands +/// separators. +fn parse_number(s: &str) -> Option { + let t = s.trim(); + if let Ok(n) = t.parse::() { + return Some(n); + } + let normalized = if t.contains('.') { + t.replace(',', "") + } else { + t.replacen(',', ".", 1) + }; + normalized.parse().ok() +} + +fn number<'de, D: Deserializer<'de>>(s: &str) -> Result { + parse_number(s).ok_or_else(|| de::Error::custom(format!("not a number: {:?}", s.trim()))) +} + /// Serde helper for required `f64` fields: empty string falls back to /// `0.0` instead of failing. Used for fields the BeerXML spec calls /// required but real exports occasionally leave blank. @@ -34,34 +52,39 @@ where if s.trim().is_empty() { Ok(0.0) } else { - s.trim().parse::().map_err(de::Error::custom) + number::(&s) } } -/// Serde helper for `Option` fields where `T` is normally a -/// string-tagged enum. joliebulle (and probably others) export empty -/// self-closing elements (``) instead of omitting the tag -/// entirely — quick-xml hands those to serde as the literal token -/// `$text`, which fails enum deserialization with `unknown variant`. -/// -/// This helper intercepts that case: deserialize to `Option` -/// first, treat empty / whitespace / `$text` as `None`, and only on -/// a real value drive the inner deserializer via the captured string. -fn empty_str_to_none_enum<'de, T, D>(de: D) -> Result, D::Error> +/// BeerXML enums read leniently: real exports vary in case and spacing +/// (`boil`, `DRY HOP`) and use common synonyms (`Whirlpool` for `Aroma`). +/// `from_text` gets the value lowercased with only letters and digits kept. +trait LenientEnum: Sized { + fn from_text(normalized: &str) -> Option; +} + +/// Serde helper for `Option` enum fields. Empty elements, joliebulle's +/// self-closing `` (which quick-xml hands over as `$text`) and +/// unknown values all read as `None`, so one odd value never fails a whole +/// import; the converter applies its default instead. +fn lenient_enum<'de, T, D>(de: D) -> Result, D::Error> where D: Deserializer<'de>, - T: serde::de::DeserializeOwned, + T: LenientEnum, { let raw: Option = Option::deserialize(de)?; - let Some(s) = raw else { return Ok(None) }; - let trimmed = s.trim(); - if trimmed.is_empty() || trimmed == "$text" { - return Ok(None); - } - T::deserialize(serde::de::value::StringDeserializer::::new( - trimmed.to_string(), - )) - .map(Some) + Ok(raw.and_then(|s| { + let trimmed = s.trim(); + if trimmed.is_empty() || trimmed == "$text" { + return None; + } + let normalized: String = trimmed + .chars() + .filter(|c| c.is_alphanumeric()) + .collect::() + .to_lowercase(); + T::from_text(&normalized) + })) } /// A single brewing recipe — the top-level `` element. @@ -82,7 +105,7 @@ pub struct Recipe { /// Optional in practice: many exporters omit this even though the /// spec lists it as required. joliebulle writes `` when /// unset, which the empty-str helper folds back to `None`. - #[serde(default, rename = "TYPE", deserialize_with = "empty_str_to_none_enum")] + #[serde(default, rename = "TYPE", deserialize_with = "lenient_enum")] pub recipe_type: Option, /// Optional brewer name. #[serde(default)] @@ -259,6 +282,17 @@ pub enum RecipeType { AllGrain, } +impl LenientEnum for RecipeType { + fn from_text(t: &str) -> Option { + match t { + "extract" => Some(Self::Extract), + "partialmash" => Some(Self::PartialMash), + "allgrain" => Some(Self::AllGrain), + _ => None, + } + } +} + // ─── Style ──────────────────────────────────────────────────────────────── /// A `