fix(archive-reader): read OPF attributes under any prefix bound to its namespace - #330
Merged
Merged
Conversation
…s namespace An XML prefix is arbitrary: what names an attribute is its namespace and local name. `opf` is the convention, but a package binding the OPF namespace to another prefix names the same attributes, and the parser matched the literal `opf:` spellings — so `pkg:scheme="GoogleBooks"` was read as no scheme at all, and the identifier fell back to whatever its value could be inferred as. The three attributes the package's own vocabulary defines are now read under every prefix the document binds to the namespace: the identifier scheme, `role` and `file-as`. `opf` stays accepted whether or not it is declared, since using it undeclared is invalid but common, and the unprefixed spellings EPUB 2 uses stay a fallback rather than an equivalent — an unprefixed attribute is in no namespace. `rendition:layout` and its siblings are deliberately untouched. Those are `<meta property>` values governed by EPUB's reserved vocabulary prefixes, declared with `<package prefix>`, which is a different mechanism from XML namespaces. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 69e250b5c5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… read Namespace bindings are scoped to the element declaring them, so reading them off `<package>` alone missed a prefix a package declares on `<metadata>` or on the identifier itself, and trusted one the root bound to the OPF namespace even where a descendant had rebound it. Bindings now accumulate while descending: `xmlNamespaceScope` layers an element's declarations over the scope it was reached under, and the prefixes naming the namespace are resolved at the element whose attribute is being read. `opfNamespacePrefixes` takes that scope rather than raw attributes, and a document that explicitly binds `opf` elsewhere is honoured — the tolerance for an undeclared `opf` only applies when nothing binds it. Caught by Codex on the first version of this fix. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug
An XML prefix is arbitrary — what names an attribute is its (namespace, local name) pair.
opfis the convention, but a package may bind the OPF namespace to any prefix, and it names the same attributes:The parser matched the literal
opf:spellings, so this identifier was read as having no scheme and fell back to whatever its value could be inferred as —Unknownhere, since a Google Books id has no recognizable syntax. An ISBN-shaped value masked the problem by inferring correctly anyway.Reported by Codex on #329.
Bindings are element-scoped
The first commit resolved prefixes from
<package>. That is wrong in both directions, as Codex pointed out on this PR: XML scopes bindings to the element declaring them, inherited by descendants. So it missed a prefix declared on<metadata>or on thedc:identifieritself, and it trusted a prefix the root bound to the OPF namespace even where a descendant had rebound it.Bindings now accumulate while descending.
xmlNamespaceScopelayers one element's declarations over the scope it was reached under, and the prefixes naming the namespace are resolved at the element whose attribute is being read.Scope
Three attributes, all defined by the package's own vocabulary: the identifier scheme (
scheme/Scheme),role, andfile-as.rendition:layoutand its siblings are deliberately untouched. Those are<meta property="rendition:layout">values, governed by EPUB 3's reserved vocabulary prefixes declared with<package prefix="…">— a different mechanism from XML namespaces, and matching the literal string is correct by default. Worth its own change if you want it.Element names already matched by local name (
localNameEq), so<opf:package>and<pkg:metadata>were never affected.Two rules kept deliberately
opfis accepted when nothing binds it. Using it undeclared is invalid but common in the wild. A document that explicitly bindsopfto another namespace is honoured, and it is then not read.schemeis read after every prefixed form, as the EPUB 2 legacy shape it is.A prefix bound to some other namespace is never read.
New exports
OPF_NAMESPACEXmlNamespaceScope/xmlNamespaceScope(attributes, inherited?)opfNamespacePrefixes(scope)opfNamespacedAttribute(attributes, prefixes, localNames)OPF_IDENTIFIER_SCHEME_LOCAL_NAMESscheme,Scheme— for a consumer resolving namespaces itself, as anything with a DOM can viagetAttributeNSopfIdentifierSchemeAttributegains an optional second argument for the prefixes, defaulting to["opf"], so the signature stays backward compatible.OPF_IDENTIFIER_SCHEME_ATTRIBUTESis unchanged and now documented as not covering an aliased prefix.Testing
348 tests pass, up from 324, with none modified — no behaviour change for documents using the conventional prefix.
Parser cases, each verified to fail before the corresponding fix:
roleandfile-as<metadata>; declared on the identifier itself; declared on a creator itselfopf:undeclared still read; the unprefixed EPUB 2 spellings still readPlus 14 unit cases for
xmlNamespaceScope/opfNamespacePrefixes/opfNamespacedAttribute, including that layering a scope does not mutate the inherited one.tsc and biome clean.
Docs
gitbook/archive-reader/identifiers.mdgains the aliased-prefix explanation with the XML above, the element-scoping model with an example, and both rules.🤖 Generated with Claude Code