Skip to content

refactor: collapse parser text/cdata handlers, centralize value checks (9.1.0) - #492

Open
derduher wants to merge 1 commit into
masterfrom
claude/grill-with-docs-6ddcba
Open

refactor: collapse parser text/cdata handlers, centralize value checks (9.1.0)#492
derduher wants to merge 1 commit into
masterfrom
claude/grill-with-docs-6ddcba

Conversation

@derduher

Copy link
Copy Markdown
Collaborator

What changed

XMLToSitemapItemStream maintained two near-identical SAX switch statements — the text handler (~40 tags) and a cdata handler covering only the 8 tags patched for issue #445, duplicating ~150 lines verbatim. Both events now route through a single handleCharData, and the parser's inline value checks are delegated to shared guards in lib/validation.ts.

  • CDATA is now ordinary character data in every tag (e.g. <priority><![CDATA[0.5]]></priority> now parses). Previously anything outside the 8-tag list was logged as "unhandled cdata".
  • <loc> is validated with the same validateURL the sitemap index parser uses, adding a URL-parseability check on top of length + protocol. A malformed URL that passed the old inline check but fails new URL() is now dropped with a warning.
  • New guards isValidPriority, isValidVideoDuration, isValidVideoRating, isValidVideoViewCount, isValidISODate in lib/validation.ts; validateSMIOptions runs on the same guards, and the bounds (0..1, 0..28800, 0..5) now live once in LIMITS (lib/constants.ts). None of the new guards are added to the public index.ts surface.
  • Version bumped to 9.1.0 with a changelog entry; decision recorded in docs/adr/0001-cdata-is-character-data.md; first entry in a new CONTEXT.md glossary ("Sitemap URL").

Net −44 lines (174 insertions, 218 deletions).

Why

This is the top candidate from an architecture review of the parser hot spots: the twin handlers meant every per-tag change had to be made twice (the "add a field" recipe in CLAUDE.md silently required editing both), and the inlined URL/bounds checks contradicted the repo's "all validation in lib/validation.ts" rule while enforcing a weaker loc check than the index parser.

Reviewer notes

  • Behavior deltas (both documented in CHANGELOG under 9.1.0, hence minor not patch): CDATA parsing is more permissive; loc validation is slightly stricter, and invalid-loc warnings changed to the Invalid URL in sitemap: … form already used by the index parser. Test churn is confined to 4 message assertions.
  • Field-level warn messages for everything except loc are byte-identical to before — the parser still composes its own messages and only the checks moved.
  • Per-field append/truncation semantics and the errors/errorCount surface are intentionally untouched.
  • <loc> keeps assign-last-wins for multi-chunk character data; accumulating at closetag was considered and deferred (see the ADR).
  • New tests: CDATA in tags beyond the [ BUG ] CDATA cannot be used for loc tag #445 set, and rejection of an unparseable loc. The existing alltags.cdata.xml fixture still passes, so [ BUG ] CDATA cannot be used for loc tag #445 behavior is intact.
  • npm run test:full passes (lint, ESM+CJS builds, 387 tests with coverage thresholds, xmllint schema validation).

🤖 Generated with Claude Code

XMLToSitemapItemStream maintained two near-identical SAX switch
statements (~167 duplicated lines); the cdata switch covered only the
8 tags patched for issue #445, so CDATA in any other tag was rejected
as unhandled. Both events now route through a single handleCharData,
making CDATA ordinary character data in every tag.

Value checks move behind the validation seam: <loc> is validated with
the same validateURL the index parser uses (adding URL-parseability),
and the inlined priority/duration/rating/view-count/date bounds become
shared guards in lib/validation.ts with the bounds defined once in
LIMITS. validateSMIOptions runs on the same guards.

Ships as 9.1.0; behavior deltas recorded in the changelog and
docs/adr/0001-cdata-is-character-data.md.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant