feat: opt-in Clark-notation prop keys for namespace disambiguation#410
Open
dschmidt wants to merge 2 commits into
Open
feat: opt-in Clark-notation prop keys for namespace disambiguation#410dschmidt wants to merge 2 commits into
dschmidt wants to merge 2 commits into
Conversation
Contributor
Author
|
I just realised OpenCloud uses xmlns Attributes on the props - figuring out what this means for this PR :) |
Adds an optional `clarkNotationProps` field to `WebDAVParsingContext`,
defaulting to `false`. When set to `true`, every property key inside each
`propstat.prop` is rewritten to Clark notation `{namespaceURI}localName`.
This lets consumers calling parseXML directly disambiguate properties that
share a local name across different XML namespaces — RFC 4918 allows this
and the default unconditional prefix stripping today collapses such
properties (into an array under one bare key, with the namespace origin
lost). With Clark notation each property carries a single canonical key
regardless of how the server serialised the namespace (prefix or inline
default xmlns).
The structural envelope of DAVResult (multistatus/response/propstat/prop/
status/href) is unaffected: a small walker renames any prefixed structural
key back to its bare local name before normaliseResult runs, then rewrites
prop content to Clark notation. The walker stops at each <prop> so prop
contents do not cascade into the structural rename pass. xmlns
declarations on the multistatus element are kept as attributes and used
to resolve prop-key namespaces; inline xmlns="..." on individual property
elements overrides scope. Unknown prefixes fall back to the null namespace
rather than throwing.
Default behaviour, normaliseResult, normaliseResponse and the downstream
helpers (prepareFileFromProps, parseStat, parseSearch) are unchanged.
Refs perry-mitchell#210
3a4a2b4 to
b04e568
Compare
Contributor
Author
|
Feature got a bit bigger than anticipated, but I'm pretty satisfied with the outcome. Appreciate any feedback! |
- Inline the single-use `localName` helper into `walk` (top-level function no longer needed). - Simplify `unwrapXmlnsWrappedValue`: the "complex element clone with xmlns attrs stripped" branch was never reached by tests and would have returned the value half-rewritten anyway (grandchild keys unchanged), so it is removed. Complex elements are now returned as-is and the limitation is documented in the JSDoc. - Drop the V8 inline-cache comment from `applyClarkNotation`; the code is straightforward enough to speak for itself. - Add a `clarkNotationProps` test for prefix-only serialisation (Nextcloud-style: `<oc:permissions>` with `xmlns:oc` declared on the multistatus root). The existing tests exercised the inline-`xmlns` path; this covers the prefix-to-URI lookup path.
Contributor
Author
|
@perry-mitchell any thoughts? |
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.
Related to #210, though not a full fix for the high-level
stat()shape it asks about.Motivation
I'm building a generic extension metadata mechanism in OpenCloud. Third-party extensions can attach typed metadata to files, and the natural mapping for cross-protocol access turned out to be one custom XML namespace per extension. Multiple extensions can then reuse the same local property name without colliding, because the namespace URI is what disambiguates them. RFC 4918 supports this cleanly, and on the server side it falls out for free.
On the client side it doesn't. Today
parseXMLstrips namespace prefixes unconditionally, so two properties with the same local name from different namespaces (e.g.<value xmlns="proj-ns">and<value xmlns="rev-ns">) collapse to a singlevaluekey (with values merged into an array) and lose their namespace origin entirely. Real-world servers add a second twist: Go'sencoding/xmland similar marshalers serialise the standard DAV/oc properties with prefixes (<d:getlastmodified>) but custom-namespace properties with inline default xmlns on the element (<value xmlns="...">), because they have no prefix registered for the custom namespace. The client has to deal with both serialisation styles in the same response.What I originally tried
My first attempt at this PR was a simple opt-in that just told
parseXMLto keep namespace prefixes on tags (essentiallyremoveNSPrefix: false). That works for servers that prefix-serialise custom namespaces, but for the inline-xmlnsstyle it just leaked fast-xml-parser's raw object shape into the result ({ "text": "...", "@xmlns": "..." }per element, arrays when collisions occur, and so on). That made the option's output awkward to reason about and inconsistent across server serialisation styles. I'd be shipping a half-feature whose consumers would commit to an unpleasant shape, and any "real" fix later would mean a second parallel opt-in.So the actual feature is Clark notation: one canonical key per property regardless of how the server expressed the namespace.
What this PR does
Adds an optional
clarkNotationPropsfield toWebDAVParsingContext, defaulting tofalse. When set totrue, every property key inside eachpropstat.propis rewritten to Clark notation{namespaceURI}localName. The structural shape ofDAVResultis unaffected:multistatus,response,propstat,prop,status,hrefall keep their bare names, and the existingnormaliseResultruns unchanged.Downstream helpers like
prepareFileFromProps,parseStatandparseSearchassume bare prop keys and are intentionally left untouched; consumers that opt in handle the Clark-notation keys on their side, which is a single property access if they know the namespace they're after.Implementation notes
The implementation isn't completely trivial and I'd really appreciate review feedback. A single-pass tree walker runs after
fast-xml-parserparses withremoveNSPrefix: falseand does two things in one traversal:normaliseResultcan do its job.<prop>, walks the children, resolves each one's namespace (from the xmlns scope of the multistatus element, or from an inlinexmlns="..."on the element itself), unwraps the{ text, "@xmlns" }object shape, and emits a single Clark-notation key per property. Same-local-name collisions in different namespaces become distinct keys.Edge cases:
I tried to keep the cost as low as I could by benchmarking alternative walker designs (V8 micro-optimisations, algorithmic restructure with direct navigation of the envelope, and parser-cooperative approaches using fast-xml-parser hooks). The full benchmark suite landed within noise of the current shape on end-to-end
parseXML, since the parser dominates the cost and the walker is a small slice on top. I left the walker as a clean generic traversal rather than commit a more complex variant for a wash result. Very open to suggestions if you see a cheaper structural pattern.Performance
Benchmarked against the current default and a discarded two-pass alternative. Each benchmark fixture is a synthetic multistatus response: one
<d:response>per file with a single<d:propstat>(status 200) containing 8 standard properties (d:getlastmodified,d:getcontentlength,d:getetag,d:getcontenttype,d:resourcetype,oc:permissions,oc:id,oc:fileid) plus additional extension properties from two distinct custom namespaces with inline default-xmlns serialisation (the realistic Sabre / Go-encoding/xmlshape), rotating, up to the configured prop count per file.Overhead settles around 25% on realistic-and-large inputs, paid only by consumers that opt in.
Tests
Three cases under
parseXML > clarkNotationProps: documents today's collapse-on-collision behaviour (lossy array of values), verifies that the Clark-notation rewrite yields distinct canonical keys covering both prefix-resolved (DAV/oc) and inline-xmlns (extension) properties, and a malformed-input case where an undeclared prefix falls back to the null namespace rather than crashing.