Skip to content

[bugfix] Preserve attribute namespace when copied via enclosed expression (+4 XQTS HEAD)#6356

Open
joewiz wants to merge 3 commits into
eXist-db:developfrom
joewiz:bugfix/xq31-attr-copy-namespace-preserve
Open

[bugfix] Preserve attribute namespace when copied via enclosed expression (+4 XQTS HEAD)#6356
joewiz wants to merge 3 commits into
eXist-db:developfrom
joewiz:bugfix/xq31-attr-copy-namespace-preserve

Conversation

@joewiz
Copy link
Copy Markdown
Member

@joewiz joewiz commented May 11, 2026

Summary

When a namespaced attribute is copied into a direct element constructor via an enclosed expression and the source attribute's prefix conflicts with a binding already declared on the new constructor, eXist silently changed the attribute's namespace URI to match the constructor's binding (or, for two-attribute cases, collapsed both attributes into the same prefix). For example:

let $src := <s xmlns:foo=\"http://example.com/A\" foo:k=\"v\"/>
return <out xmlns:foo=\"http://example.com/B\">{$src/@*}</out>

returned <out xmlns:foo=\"http://example.com/B\" foo:k=\"v\"/> -- attribute k ended up in URI B, not its source URI A.

Spec rule

XQuery 3.1 §3.9.4 (In-scope Namespaces of a Constructed Element):

For each prefix used in the name of the constructed element or in the names of its attributes, a namespace binding must exist. If a namespace binding does not already exist for one of these prefixes, a new namespace binding is created for it. If this would result in a conflict, because it would require two different bindings of the same prefix, then the prefix used in the node name is changed to an arbitrary implementation-dependent prefix that does not cause such a conflict, and a namespace binding is created for this new prefix.

What changed

exist-core/src/main/java/org/exist/dom/memtree/DocumentBuilderReceiver.java

DocumentImpl.copyStartNode dispatches a copied attribute node to receiver.attribute(qname, value). Within an enclosed expression EnclosedExpr disables the receiver's checkNS flag for the duration of the copy, so the attribute was previously added to the MemTree with its original (prefix, URI) QName but no namespace node was emitted on the parent constructor element for that binding. The serializer then resolved the attribute's prefix against the constructor's already-declared xmlns:* and wrote it in the wrong URI.

attribute() now resolves the QName for any namespaced attribute regardless of checkNS:

  • If the prefix is already bound to the same URI on the parent element: keep it as-is.
  • If the prefix is unbound: declare it.
  • If the prefix is bound to a different URI: reuse an existing prefix already mapped to the source URI, or generate a fresh one (XXX, XXX1, ...) and declare it.
  • In all cases emit a namespace node onto the parent element. Emission is idempotent against the parent's own element prefix and any xmlns:* nodes already declared on it.

exist-core/src/test/java/org/exist/xquery/ElementConstructorAttrNamespaceTest.java

Three JUnit regression tests covering the inline reproducers for Constr-inscope-3, Constr-inscope-4, and the minimal single-attribute case.

XQTS HEAD before / after

prod-DirElemContent.namespace (133 tests):

Before After
Failures 35 31
Constr-inscope-1 FAIL PASS
Constr-inscope-2 FAIL PASS
Constr-inscope-3 FAIL PASS
Constr-inscope-4 FAIL PASS

No regressions in the rest of the test set.

Test plan

  • 3 new JUnit regression tests pass (ElementConstructorAttrNamespaceTest)
  • ConstructedNodesTest, ConstructedNodesRecoveryTest, NamespaceUpdateTest, CompAttrConstructorErrorCodeTest, DocumentBuilderReceiverIntegrationTest, XPathQueryTest (150), XQuery3Tests (1011), XIncludeSerializerTest: 1200 tests pass, 0 failures
  • W3CXIncludeTestSuite baseline parity (37/183 failures both before and after on develop)
  • XQTS HEAD prod-DirElemContent.namespace: -4 failures, 0 regressions
  • Codacy PMD clean on the changed file (only pre-existing warnings)

🤖 Generated with Claude Code

@joewiz joewiz requested a review from a team as a code owner May 11, 2026 21:24
joewiz added a commit to joewiz/exist that referenced this pull request May 12, 2026
… assertions

XQueryTest.attributeNamespace and noNamepaceDefinedForPrefix_1959010 used
assertEquals on serialized XML strings, which broke under the namespace-node
ordering shift introduced by PR eXist-db#6356's fix. Per XQuery 3.1 §2 ("the relative
order of namespace nodes that share a parent is also implementation
dependent"), the new ordering is conformant — the assertions over-asserted on
implementation-dependent serialization detail. Converted both to XMLUnit's
DiffBuilder.checkForIdentical() pattern (matching existing usage at L1327-1333)
which compares structurally, not byte-for-byte.

No production code changes. The +4 XQTS HEAD lift (Constr-inscope-1..4) from
PR eXist-db#6356's earlier commit is preserved.

Full-module gate: Tests run: 6736, Failures: 0, Errors: 0, Skipped: 97, BUILD SUCCESS

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@joewiz
Copy link
Copy Markdown
Member Author

joewiz commented May 12, 2026

[This response was co-authored with Claude Code. -Joe]

Pushed a follow-up commit (84762a9) addressing the two XQueryTest regressions:

  • attributeNamespace
  • noNamepaceDefinedForPrefix_1959010

Both were assertEquals on serialized XML strings, asserting on the textual order of xmlns:c and xmlns:d declarations. XQuery 3.1 §2 makes the relative order of namespace nodes that share a parent implementation-dependent, so the new ordering this PR produces is conformant — the assertions were over-specifying. Converted both to DiffBuilder.checkForIdentical() (matching existing XMLUnit 2.x usage in the same file).

No production code changes; the +4 XQTS HEAD lift is preserved.

Full-module mvn test -pl exist-core gate: Tests run: 6736, Failures: 0, Errors: 0, Skipped: 97, BUILD SUCCESS.

@line-o line-o added the xquery issue is related to xquery implementation label May 12, 2026
@line-o
Copy link
Copy Markdown
Member

line-o commented Jun 1, 2026

@joewiz could you rebase this PR

* already declared there. No-op for the {@code xml} prefix or when the
* parent node is not an element.
*/
private void emitNamespaceNode(final String prefix, final String uri) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Address Codacy issue: The method 'emitNamespaceNode(String, String)' has an NPath complexity of 240, current threshold is 200

joewiz and others added 3 commits June 1, 2026 08:18
…sion

XQuery 3.1 §3.9.4 (In-scope Namespaces of a Constructed Element)
requires that, for each prefix used in the name of an attribute of a
constructed element, a namespace binding must exist on the new element
-- and if adopting the source prefix would conflict with an existing
in-scope binding, the prefix MUST be changed to an
implementation-dependent prefix that does not cause a conflict, with a
new namespace binding created for it.

When an attribute node arrives in the content sequence of a direct
element constructor via an enclosed expression (for example
`<new xmlns:foo="B">{$x//@foo:k}</new>` where the source attribute is in
URI A), EnclosedExpr disables the receiver's namespace checking for the
duration of the copy and DocumentImpl.copyStartNode invokes
receiver.attribute(qname, value). The attribute was added to the
MemTree with its original (foo, A) QName but no namespace node was
emitted on the parent element for the (foo, A) binding -- and since
xmlns:foo on the new element was already bound to B, the serializer
emitted the attribute as foo:k in URI B, silently changing its
namespace. For inline two-attribute cases the second attribute was
collapsed into the same prefix as the first.

Fix: in DocumentBuilderReceiver.attribute, resolve a namespaced
attribute's prefix against the current in-scope namespaces independent
of the checkNS flag. When the prefix is unbound, declare it; when it is
bound to a different URI, generate a fresh prefix (or reuse an existing
one mapped to the source URI). In all cases emit a namespace node onto
the parent element so the binding is visible to the serializer. The
emission is idempotent against the parent element's own prefix and any
xmlns:* nodes already declared on it.

Closes 4 XQTS HEAD failures in prod-DirElemContent.namespace:
Constr-inscope-1, Constr-inscope-2, Constr-inscope-3, Constr-inscope-4.

XQTS prod-DirElemContent.namespace: 35 -> 31 failures (-4, no
regressions across the 133-test set).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… assertions

XQueryTest.attributeNamespace and noNamepaceDefinedForPrefix_1959010 used
assertEquals on serialized XML strings, which broke under the namespace-node
ordering shift introduced by PR eXist-db#6356's fix. Per XQuery 3.1 §2 ("the relative
order of namespace nodes that share a parent is also implementation
dependent"), the new ordering is conformant — the assertions over-asserted on
implementation-dependent serialization detail. Converted both to XMLUnit's
DiffBuilder.checkForIdentical() pattern (matching existing usage at L1327-1333)
which compares structurally, not byte-for-byte.

No production code changes. The +4 XQTS HEAD lift (Constr-inscope-1..4) from
PR eXist-db#6356's earlier commit is preserved.

Full-module gate: Tests run: 6736, Failures: 0, Errors: 0, Skipped: 97, BUILD SUCCESS

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…pty bodies + reassigning param)

Per @reinhapa's 2026-06-01 review on PR eXist-db#6356:

> Address Codacy issue: The method 'emitNamespaceNode(String, String)'
> has an NPath complexity of 240, current threshold is 200

Decompose emitNamespaceNode by extracting three named helpers that
read like the F&O "must be true to skip" conditions the method is
actually checking:

  - isElementParent(doc, parent) -- parent is a real element node
  - isParentSelfDeclaration(doc, parent, prefix, uri) -- the binding
    is redundant because the parent element name already carries it
  - hasExistingPrefixDeclaration(doc, parent, prefix) -- scan namespace
    decls already attached to parent

Top-level method becomes four short guard clauses + the emit call.

While in the file, a local codacy-cli run surfaced additional pre-
existing nits that any future review-round would also catch:

  - 7 empty SAX-callback method bodies (setDocumentLocator, startCDATA,
    endCDATA, startDTD, endDTD, startEntity, endEntity, skippedEntity)
    -- add a one-line "no-op" comment to each explaining why the
    in-memory builder ignores the event.

  - generatePrefix(XQueryContext, String prefix) reassigned its
    parameter inside a while loop. Rewrite as an early-return for the
    "already supplied" path + a clean candidate-loop using an
    immutable parameter. Functionally equivalent.

Codacy clean on the file post-refactor. 210 tests pass (XPathQueryTest
150, memtree tests 60).

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@joewiz joewiz force-pushed the bugfix/xq31-attr-copy-namespace-preserve branch from 84762a9 to 792e622 Compare June 1, 2026 12:25
@joewiz
Copy link
Copy Markdown
Member Author

joewiz commented Jun 1, 2026

[This response was co-authored with Claude Code. -Joe]

Rebased + addressed in 792e62279d. Two changes:

1. NPath complexity on emitNamespaceNode — decomposed into three named guards that read like the conditions the method is actually checking:

  • isElementParent(doc, parent) — parent is a real element node
  • isParentSelfDeclaration(doc, parent, prefix, uri) — the binding is redundant because the parent element name already carries it
  • hasExistingPrefixDeclaration(doc, parent, prefix) — scan namespace decls already attached to parent

Top-level method becomes four short guard clauses + the emit call.

2. While I had the file open, a local Codacy run surfaced eight more pre-existing nits that any next review-round would also have caught. Folded them in here to save a round:

  • Seven empty SAX-callback method bodies (setDocumentLocator, startCDATA, endCDATA, startDTD, endDTD, startEntity, endEntity, skippedEntity) — one-line "no-op" comment on each explaining why the in-memory builder ignores the event.
  • generatePrefix(XQueryContext, String prefix) reassigned its parameter inside a while loop — rewrote as an early-return for the "already supplied" path + a clean candidate-loop using an immutable parameter. Functionally equivalent.

Codacy clean on the file post-refactor. 210 tests pass (XPathQueryTest 150, memtree tests 60), no behaviour changes.

On the pre-push gap: I should have caught the NPath-240 finding before pushing the original commit. The repeated failure mode was running Codacy on net-new files but not on edits to existing ones. Tightened the rule on my side — mandatory pre-push Codacy on every changed Java file — and you should see fewer of these review-cycle-burn nits coming back.

@line-o line-o requested review from a team and reinhapa June 1, 2026 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

xquery issue is related to xquery implementation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants