Skip to content

[bugfix] declare copy-namespaces no-inherit breaking element constructors#6222

Open
joewiz wants to merge 2 commits intoeXist-db:developfrom
joewiz:v2/copy-namespaces-fix
Open

[bugfix] declare copy-namespaces no-inherit breaking element constructors#6222
joewiz wants to merge 2 commits intoeXist-db:developfrom
joewiz:v2/copy-namespaces-fix

Conversation

@joewiz
Copy link
Copy Markdown
Member

@joewiz joewiz commented Apr 8, 2026

Summary

declare copy-namespaces preserve, no-inherit was incorrectly applying the no-inherit flag during element constructor evaluation, causing two distinct bugs:

  1. Name resolution failure: getURIForPrefix() skips inheritedInScopeNamespaces when inheritNamespaces=false, so a child constructor <b/> inside <e xmlns="http://example.com/"> could not see the inherited default namespace, and a prefixed name like <ns:c/> whose prefix was declared on an enclosing element threw XPTY0004.

  2. in-scope-prefixes() returning incomplete results: Nested direct constructors like <e3>{<e2>{<e1/>}</e2>}</e3> — calling in-scope-prefixes(<e1>) only returned <e1>'s own prefix, missing namespace2 and namespace3 declared by the enclosing constructors.

Per XQuery 3.1 §3.9.3.4, no-inherit governs how namespaces propagate from copied source nodes (existing XDM nodes placed into constructors via {$var}) — it must not affect namespace resolution for element constructors themselves.

Also fixes the real-world regression in #2182 where declare copy-namespaces preserve, no-inherit caused XPath path steps over constructed elements to silently return empty sequences.

Companion to PR #6219 (serializer xmlns="" fix).

What Changed

ElementConstructor.java — Fix A (name resolution)

Temporarily restores inheritNamespaces=true while resolving the element's QName via QName.parse(), so that prefix-to-URI lookups consult the inherited namespace context. Restored in a finally block.

FunInScopePrefixes.java — Fix B (ancestor traversal)

Always traverse the in-memory node's ancestor chain when collecting namespace prefixes, removing the coarse inheritNamespaces() switch that previously blocked traversal for all no-inherit queries. Updated cleanup to remove all entries with an empty URI (namespace undeclarations).

EnclosedExpr.java — Fix C (no-inherit copy semantics)

When no-inherit is active and an enclosed expression {...} places a pre-existing element node (a variable reference) into the outer element, a new NoInheritCopyReceiver intercepts the copy event stream and injects namespace undeclarations (xmlns:prefix="") onto the root of the copy for every ancestor namespace binding not already declared by that root. This neutralizes ancestor traversal for those nodes so in-scope-prefixes() returns only the node's own context.

Pre-existing nodes are distinguished from direct constructors by capturing the MemTreeBuilder allocated during the enclosed expression's evaluation (via new peekDocumentBuilder()) and checking whether each result node belongs to that builder's document.

XQueryContext.java / ModuleContext.java: added peekDocumentBuilder() to inspect the current builder without disturbing the context stack.

Spec References

XQTS Before/After (prod-CopyNamespacesDecl, XQ 3.1)

Test Before After
K2-CopyNamespacesProlog-4 FAIL PASS
K2-CopyNamespacesProlog-5 FAIL PASS
K2-CopyNamespacesProlog-9 (direct constructor half) FAIL PASS
copynamespace-2 FAIL PASS
K2-CopyNamespacesProlog-9 (variable copy half) PASS PASS ✓
copynamespace-3 (no-inherit, variable copy) PASS PASS ✓
copynamespace-5 (no-inherit, variable copy + FLWOR sort) PASS PASS ✓

35/35 passing in the prod-CopyNamespacesDecl test set.

CI Changes (second commit)

This PR also cherry-picks CI improvements from PR #6186 and extends them:

  • exist-parent/pom.xml: Added forkedProcessTimeoutInSeconds=600 to surefire and failsafe plugins, preventing hung test JVMs from blocking CI indefinitely (e.g. DeadlockIT, MoveResourceTest).
  • .github/workflows/ci-test.yml: Added --offline to the license:check step (eliminates 7+ min timeouts from unreachable repos; the develop cache has all needed artifacts). Added wagon HTTP timeout flags via step-level MAVEN_OPTS to prevent Maven Build hangs caused by stalled connections to repo.exist-db.org and repo.evolvedbinary.com.

Test Plan

  • XQTS prod-CopyNamespacesDecl: 35/35 pass (4 were failing before, none regressed)
  • JUnit full suite: 6,542 tests, 0 failures, 0 errors
  • Manual: declare copy-namespaces preserve, no-inherit; <e xmlns="http://example.com/">{ <b/> }</e><b> in http://example.com/ (implied by K2-CopyNamespacesProlog-4 ✓)
  • Manual: Fragment selection skips namespace declaration for xsi:type value #2182 (@daliboris) — $items/t:item/cc:collection returns result instead of empty sequence (implied by copynamespace-2 + Fix A ✓)

🤖 Generated with Claude Code

@joewiz joewiz requested a review from a team as a code owner April 8, 2026 04:32
@joewiz joewiz marked this pull request as draft April 8, 2026 10:36
@joewiz joewiz marked this pull request as ready for review April 8, 2026 11:32
@joewiz joewiz force-pushed the v2/copy-namespaces-fix branch 4 times, most recently from 3f2b3bc to 45a3e4d Compare April 8, 2026 20:46
…tructors

With `declare copy-namespaces preserve, no-inherit`, eXist was incorrectly
applying the no-inherit flag during element constructor evaluation, causing
two distinct bugs:

1. Name resolution failure: `getURIForPrefix()` skips `inheritedInScopeNamespaces`
   when `inheritNamespaces=false`, so a child constructor `<b/>` inside
   `<e xmlns="http://example.com/">` could not see the inherited default namespace,
   and a prefixed name like `<ns:c/>` whose prefix was declared on an enclosing
   element threw XPTY0004.

2. `in-scope-prefixes()` returning incomplete results for nested direct constructors:
   `<e3>{<e2>{<e1/>}</e2>}</e3>` — `<e1>` only returned its own prefix, missing
   `namespace2` and `namespace3` from the enclosing constructor elements.

Per XQuery 3.1 §3.9.3.4, `no-inherit` governs how namespaces propagate from
*copied source nodes* (existing XDM nodes placed into constructors via `{$var}`)
into the result — it must not affect namespace resolution for element constructors
themselves, nor prevent ancestor traversal when collecting in-scope prefixes of
directly constructed elements.

Fix A (ElementConstructor — name resolution): temporarily restore
`inheritNamespaces=true` while resolving the element's QName so that
`QName.parse()` can look up prefix-to-URI mappings in the inherited context.
Restored to `false` in a `finally` block.

Fix B (FunInScopePrefixes): always traverse the in-memory node's ancestor chain
when collecting namespace prefixes. The previous coarse `inheritNamespaces()` switch
prevented ancestor traversal for all no-inherit queries, including direct constructors
where traversal is correct. Updated the cleanup pass to remove all entries with an
empty URI (namespace undeclarations), not just empty-key+empty-value pairs.

Fix C (EnclosedExpr + NoInheritCopyReceiver): when `no-inherit` is active and an
enclosed expression places a pre-existing element node (a variable reference, not a
direct constructor) into the outer element, inject namespace undeclarations
(xmlns:prefix="") onto the root of the copy for every ancestor namespace binding
not already declared by that root. This neutralizes ancestor traversal for those
nodes so that `in-scope-prefixes()` returns only the node's own namespace context.

Pre-existing nodes are distinguished from direct constructors by capturing the
MemTreeBuilder allocated during the enclosed expression's evaluation
(via new `peekDocumentBuilder()`) and checking whether each result node belongs
to that builder's document.

Fixes the following pre-existing XQTS failures in prod-CopyNamespacesDecl:
  K2-CopyNamespacesProlog-4, K2-CopyNamespacesProlog-5,
  K2-CopyNamespacesProlog-9 (second half: direct constructors),
  copynamespace-2

Also fixes the real-world regression reported in eXist-db#2182 where
`declare copy-namespaces preserve, no-inherit` caused XPath steps over
constructed elements to return empty sequences.

Companion to PR eXist-db#6219 (serializer xmlns="" fix).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@joewiz joewiz force-pushed the v2/copy-namespaces-fix branch from 45a3e4d to 7855ad7 Compare April 13, 2026 13:26
Copy link
Copy Markdown
Contributor

@duncdrum duncdrum left a comment

Choose a reason for hiding this comment

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

more namespace fun, one comment in file.

Comment on lines +298 to +302
/*
if (qn.getPrefix() == null && context.inScopeNamespaces.get("xmlns") != null) {
qn.setNamespaceURI((String)context.inScopeNamespaces.get("xmlns"));
}
*/
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

dead code?

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.

Looks like it, I would be in favour of removing this.

return inScopeNamespaces;
}

public MemTreeBuilder peekDocumentBuilder() {
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.

Why is this method named _peek_DocumentBuilder?
This looks like a standard getter to me

@line-o line-o changed the title [bugfix] fix declare copy-namespaces no-inherit breaking element constructors [bugfix] declare copy-namespaces no-inherit breaking element constructors Apr 14, 2026
Copy link
Copy Markdown
Member

@line-o line-o left a comment

Choose a reason for hiding this comment

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

Do these extra checks have a significant impact on element construction performance even if copy-namespaces no-inherit is not set?

&& next.getType() == Type.ELEMENT
&& next instanceof NodeImpl) {
final NodeImpl nodeImpl = (NodeImpl) next;
final boolean isPreExisting = (innerBuilder == null)
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.

I would prefer to populate innerBuilder lazily on first access instead of always regardless if it will ever be used.
Then it also becomes clear that only its document is needed to check for preExisting nodes.

…plementation

- Remove stale commented-out code in ElementConstructor (dead since the
  in-scope namespace API was updated)
- Rename XQueryContext.peekDocumentBuilder() to getCurrentDocumentBuilder()
  for consistency with standard Java getter naming
- Lazily capture innerBuilder in EnclosedExpr only when copy-namespaces
  no-inherit is active, avoiding the method call entirely in the common
  (inherit) case

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@joewiz
Copy link
Copy Markdown
Member Author

joewiz commented Apr 15, 2026

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

Good catches, addressed in 2d9e248:

  • Removed the stale commented-out block in ElementConstructor (dead since the in-scope namespace API was modernized)
  • Renamed peekDocumentBuilder()getCurrentDocumentBuilder() in XQueryContext, ModuleContext, and the single call site in EnclosedExpr
  • Made innerBuilder capture lazy: moved noInherit computation before the try block so getCurrentDocumentBuilder() is only called when copy-namespaces no-inherit is active — the hot path (default inherit mode) now has zero overhead from this method

On the performance question: with the lazy fix, the overhead in the default (inherit) case is exactly one context.inheritNamespaces() call per EnclosedExpr evaluation — just a field read on XQueryContext. The per-item check at line 168 (noInherit && ancestorNS != null) short-circuits immediately on the first boolean. I don't think this warrants a microbenchmark, but happy to add one if you'd like to see the numbers.

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.

3 participants