fix(query): resolve named ES date formats for ignore_malformed validation - #89

Reviewed for inclusion in v1.0.0-rc.9. Holding it — the named-format resolution is a real improvement, but as written it silently drops documents that are currently indexed, and unknown formats fail open.

Every finding was sent to an independent verifier told to refute it; these survived.

Blocker — numeric values under numeric-shaped date formats are now silently dropped

The new Value::Number arm (dates.rs:183-188) accepts a JSON number only when some compiled format is EpochMillis or EpochSecond. The old code returned true for any Value::Number under any non-epoch format.

ES does neither. DateFieldMapper reads the value with parser().text(), which stringifies a VALUE_NUMBER token, then parses that text with the declared formatter. So under basic_date (yyyyMMdd), the JSON number 20240101 renders as "20240101" and parses cleanly.

Verified by running both implementations over the same matrix: basic_date + 20240101 is accepted before this PR, rejected after. With ignore_malformed: true that value is now dropped from _source and recorded in _ignored instead of being indexed — silently, on documents that indexed fine yesterday. The same applies to any numeric-shaped pattern (basic_date, basic_date_time_no_millis, yyyyMMddHHmmss, …).

High — unknown named formats fail OPEN and accept arbitrary garbage

compile_one_format handles ~20 named formats and falls through to other => other, compiling the name itself as a Java pattern. Many real ES format names contain letters that aren't in VALID_JAVA_PATTERN_LETTERS — b, o, t, i, r — so they return Err(UnknownPatternLetter), and is_date_value_valid_with_format swallows that into true (es_compat.rs:26741-26744).

Measured, all of these currently accept literally any value: basic_time, basic_ordinal_date, basic_week_date (b), ordinal_date, hour_minute (o), time, week_date (t).

Failing open is the wrong default here: an unsupported format name should be a mapping-time error, or at minimum should fall back to the previous permissive-but-narrow behaviour rather than disabling validation entirely for that field.

Also confirmed

• The validator now contradicts the ignore_malformed: false rejection path — the stricter setting ends up more permissive than the lenient one, which is backwards.
• The format string is recompiled per value on the ingest hot path, amplifying an unvalidated user-supplied input by roughly 4× on a hot path — worth a compiled-format cache keyed on the mapping.
• Leading/trailing whitespace is now silently trimmed, accepting values ES treats as malformed.
• epoch_second overflow checking depends on JSON spelling — the number and the string form of the same value disagree.
• null in a date field no longer records _ignored, a behaviour change on existing indices.

What would make this shippable

Stringify numbers and run them through the declared formatter the way DateFieldMapper does, rather than gating on epoch formats; make an unresolvable format name fail closed (or error at mapping time); and add fixtures for the numeric-under-basic_date case and for at least one b/o/t named format, since neither is covered today.

The direction is right — resolving named formats properly is exactly what this needed, and the dates.rs extraction is a good place for it. It's the two failure directions that need inverting.

Thanks — the blocker's mechanism (DateFieldMapper stringifying numbers via text()) checked out.

• Numeric-shaped format blocker: replaced the epoch-only Value::Number branch with n.to_string() run through the exact same per-format parser a String value uses. basic_date + 20240101 (and any other numeric-shaped pattern) is now accepted; strict_date_time + a bare epoch-millis number is still correctly rejected (its stringified form doesn't match the structured yyyy-MM-dd'T'... pattern, same as before, just via the unified path now).
• Fail-open on unresolvable formats: is_date_value_valid_with_format now returns false on a compile_formats error instead of true. Went with "fail closed at validation time" rather than a mapping-time error, per your "at minimum" — a format this engine can't resolve now makes the field's values ignored (visible in _ignored) rather than silently unvalidated.
• Whitespace trimming: removed — my fix had added a .trim() on the string path that ES doesn't do; a padded value that only matches after trimming is now correctly rejected.
• null handling: checked this deliberately rather than changing it — ES treats JSON null as "no value" uniformly across field types (not malformed), so null continuing to always match (and not appear in _ignored) is correct, not a regression.

Not addressed (agreed, tracking separately): the compiled format isn't cached per mapping (still recompiled per value), and ignore_malformed:false's hard-reject path in bulk.rs uses a different, more lenient check than this one — so the stricter setting can currently be more permissive than the lenient one for the same value. That inconsistency needs the two paths unified, which felt like a separate change from this fix.

Full suite green: cargo fmt --check, cargo clippy --workspace --all-targets -- -D warnings, cargo test -p xerj-engine -p xerj-query -p xerj-api --lib (only the 2 pre-existing, unrelated snapshot_path_security_tests failures).

🤖 Generated with Claude Code

Housekeeping, not a review comment.

We've just added a Contributor License Agreement to the project (#93). Until now contributions here were covered only by Apache-2.0 §5's inbound=outbound clause, which gives no explicit patent grant — worth tightening for a project this size.

Once #93 merges, a cla-bot check will appear on this PR and it will be red until you're covered. Signing is one small pull request, once per contributor — not per PR:

1. Read CLA.md
2. Open a PR adding your GitHub username to .contributors
3. Comment @cla-bot check back here and it turns green

That PR is the signature — it comes from your own account, so the commit history is the record.

To be explicit about something: we did not add anyone to the signed list on their behalf, including you. That file asserts a person has signed, and that's not ours to assert for someone else — hence the ask rather than a quiet edit.

Sorry for the extra step on work that's already in flight. Thanks for the contributions.

Rebased onto current main and folded the review follow-up into the branch, so this now merges as one complete unit rather than landing a known regression and fixing it afterwards.

The branch is now three commits:

1. resolve named ES date formats for ignore_malformed validation (yours)
2. stringify numbers and fail closed on unresolvable date formats (yours, addressing the first review round)
3. implement the real ES named formats instead of dropping their data

Commit 3 exists because "fail closed on unresolvable formats" from round 2 was the right call for safety but had a cost worth removing: roughly seven named formats this engine could not resolve (basic_time, basic_ordinal_date, basic_week_date, ordinal_date, hour_minute, time, week_date) went from accepting anything to accepting nothing, which silently drops values on fields that indexed fine before. Implementing the formats properly removes the choice between failing open and dropping data.

Verified after the rebase: cargo build --release -p xerj-query -p xerj-api, then cargo test --release -p xerj-query -p xerj-api → 284 passed, 0 failed.

Thanks for working through two rounds of review on this one. The named-format resolution was the right direction and the dates.rs extraction is what made commit 3 straightforward.