From 89beaee736cb90cead4d7bbad65a12cd314cb81e Mon Sep 17 00:00:00 2001 From: Joe Wicentowski Date: Mon, 11 May 2026 17:24:17 -0400 Subject: [PATCH 1/3] [bugfix] Preserve attribute namespace when copied via enclosed expression MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 `{$x//@foo:k}` 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) --- .../dom/memtree/DocumentBuilderReceiver.java | 88 ++++++++++++++- .../ElementConstructorAttrNamespaceTest.java | 102 ++++++++++++++++++ 2 files changed, 189 insertions(+), 1 deletion(-) create mode 100644 exist-core/src/test/java/org/exist/xquery/ElementConstructorAttrNamespaceTest.java diff --git a/exist-core/src/main/java/org/exist/dom/memtree/DocumentBuilderReceiver.java b/exist-core/src/main/java/org/exist/dom/memtree/DocumentBuilderReceiver.java index 38884a1fa50..e51782b1178 100644 --- a/exist-core/src/main/java/org/exist/dom/memtree/DocumentBuilderReceiver.java +++ b/exist-core/src/main/java/org/exist/dom/memtree/DocumentBuilderReceiver.java @@ -206,12 +206,98 @@ public void characters(final char[] ch, final int start, final int len) throws S @Override public void attribute(final QName qname, final String value) throws SAXException { try { - builder.addAttribute(checkNS(false, qname), value); + // Attribute namespace handling must be independent of the checkNS + // flag: when an attribute is copied into a new element constructor + // (XQuery 3.1 §3.9.1.3, default copy-namespaces preserve), the + // attribute's prefix MUST be rebound if it conflicts with an + // in-scope binding on the new element, and its (prefix, URI) + // mapping MUST be reflected as a namespace node on that element. + final QName resolved = qname.hasNamespace() + ? resolveAttributeQName(qname) + : qname; + builder.addAttribute(resolved, value); } catch(final DOMException e) { throw new SAXException(e.getMessage(), e); } } + /** + * Resolves a namespaced attribute QName against the current in-scope + * namespaces, rebinding to a freshly generated prefix on prefix-to-URI + * conflict, and emitting a namespace node on the parent element to make + * the binding visible to the serializer. + */ + private QName resolveAttributeQName(final QName qname) { + final XQueryContext context = builder.getContext(); + final String uri = qname.getNamespaceURI(); + String prefix = qname.getPrefix(); + if (prefix == null || prefix.isEmpty()) { + // Attribute with namespace but no prefix: pick an existing prefix + // mapped to this URI, or generate a fresh one. + final String existing = context == null ? null : context.getInScopePrefix(uri); + if (existing != null && !existing.isEmpty()) { + prefix = existing; + } else { + prefix = generatePrefix(context, null); + if (context != null) { + context.declareInScopeNamespace(prefix, uri); + } + emitNamespaceNode(prefix, uri); + return new QName(qname.getLocalPart(), uri, prefix); + } + } else if (context != null) { + final String boundUri = context.getInScopeNamespace(prefix); + if (boundUri == null) { + // Prefix is not in scope -> declare it + context.declareInScopeNamespace(prefix, uri); + } else if (!boundUri.equals(uri)) { + // Prefix is bound to a different URI -> generate a fresh prefix + String reuse = context.getInScopePrefix(uri); + if (reuse == null || reuse.isEmpty()) { + prefix = generatePrefix(context, null); + context.declareInScopeNamespace(prefix, uri); + } else { + prefix = reuse; + } + } + } + emitNamespaceNode(prefix, uri); + return new QName(qname.getLocalPart(), uri, prefix); + } + + /** + * Adds an xmlns:prefix=uri namespace node to the current element when not + * 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) { + if (prefix == null || prefix.isEmpty() || XMLConstants.XML_NS_PREFIX.equals(prefix)) { + return; + } + final DocumentImpl doc = builder.getDocument(); + final int parent = doc.getLastNode(); + if (parent < 0 || doc.getNodeType(parent) != org.w3c.dom.Node.ELEMENT_NODE) { + return; + } + final QName parentName = doc.nodeName[parent]; + if (parentName != null && prefix.equals(parentName.getPrefix()) + && uri.equals(parentName.getNamespaceURI())) { + return; + } + final int firstNs = doc.alphaLen[parent]; + if (firstNs >= 0) { + for (int ns = firstNs; + ns < doc.nextNamespace && doc.namespaceParent[ns] == parent; + ns++) { + final QName nsName = doc.namespaceCode[ns]; + if (nsName != null && prefix.equals(nsName.getLocalPart())) { + return; + } + } + } + builder.namespaceNode(prefix, uri); + } + @Override public void ignorableWhitespace(final char[] ch, final int start, final int len) throws SAXException { if (!suppressWhitespace) { diff --git a/exist-core/src/test/java/org/exist/xquery/ElementConstructorAttrNamespaceTest.java b/exist-core/src/test/java/org/exist/xquery/ElementConstructorAttrNamespaceTest.java new file mode 100644 index 00000000000..ed928f962e1 --- /dev/null +++ b/exist-core/src/test/java/org/exist/xquery/ElementConstructorAttrNamespaceTest.java @@ -0,0 +1,102 @@ +/* + * eXist-db Open Source Native XML Database + * Copyright (C) 2001 The eXist-db Authors + * + * info@exist-db.org + * http://www.exist-db.org + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + */ +package org.exist.xquery; + +import org.exist.test.ExistXmldbEmbeddedServer; +import org.junit.ClassRule; +import org.junit.Test; +import org.xmldb.api.base.ResourceSet; +import org.xmldb.api.base.XMLDBException; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; + +/** + * Regression tests for QT4 / XQTS 3.1 prod-DirElemContent.namespace + * Constr-inscope-{1..4}: when a namespaced attribute is copied into a + * direct element constructor via an enclosed expression, and the prefix + * the source attribute uses is already bound to a different namespace URI + * on the new constructor, eXist must rebind the attribute to a freshly + * generated prefix rather than silently dropping it. + * + *

XQuery 3.1 §3.9.3.4 (Direct Element Constructors) and §4.16 + * (Copy-Namespaces Declaration). The default copy-namespaces mode is + * {@code preserve, inherit}: an attribute's own in-scope namespace MUST + * be preserved on the constructed element.

+ */ +public class ElementConstructorAttrNamespaceTest { + + @ClassRule + public static final ExistXmldbEmbeddedServer existEmbeddedServer = + new ExistXmldbEmbeddedServer(false, true, true); + + /** Constr-inscope-3: prefix on the new constructor conflicts with the copied attribute's prefix. */ + @Test + public void copiedAttributeWithConflictingPrefixIsPreserved() throws XMLDBException { + final String xquery = """ + for $x in + return {$x//@*:attr1}"""; + final ResourceSet result = existEmbeddedServer.executeQuery(xquery); + assertEquals(1, result.getSize()); + final String out = result.getResource(0).getContent().toString(); + // The attribute must survive the copy: its namespace URI is + // http://www.example.com/parent1 and its local name is attr1. + assertTrue("attribute attr1 must be present in: " + out, + out.contains(":attr1=\"attr1\"")); + assertTrue("source namespace must be declared on the new element: " + out, + out.contains("http://www.example.com/parent1")); + } + + /** Constr-inscope-4: two attributes with conflicting prefixes copied via enclosed expr. */ + @Test + public void multipleCopiedAttributesWithConflictingPrefixesArePreserved() throws XMLDBException { + final String xquery = """ + for $x in + + + + return {$x//@*:attr1, $x//@*:attr2}"""; + final ResourceSet result = existEmbeddedServer.executeQuery(xquery); + assertEquals(1, result.getSize()); + final String out = result.getResource(0).getContent().toString(); + assertTrue("attr1 must be present: " + out, out.contains(":attr1=\"attr1\"")); + assertTrue("attr2 must be present: " + out, out.contains(":attr2=\"attr2\"")); + assertTrue("parent1 namespace must be declared: " + out, + out.contains("http://www.example.com/parent1")); + assertTrue("parent2 namespace must be declared: " + out, + out.contains("http://www.example.com/parent2")); + } + + /** Simpler reproducer: rename in-scope namespace using a single copied attribute. */ + @Test + public void copiedAttributeNamespaceRebindsPrefix() throws XMLDBException { + final String xquery = """ + let $src := + return {$src/@*}"""; + final ResourceSet result = existEmbeddedServer.executeQuery(xquery); + assertEquals(1, result.getSize()); + final String out = result.getResource(0).getContent().toString(); + assertTrue("attribute k must be present: " + out, out.contains(":k=\"v\"")); + assertTrue("source URI A must be retained on the constructed element: " + out, + out.contains("http://example.com/A")); + } +} From 0b139a66144a8fa5ceb290850f6e5e3d11a47340 Mon Sep 17 00:00:00 2001 From: Joe Wicentowski Date: Tue, 12 May 2026 01:58:15 -0400 Subject: [PATCH 2/3] [bugfix] Use XMLUnit structural compare for namespace-order-dependent assertions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit XQueryTest.attributeNamespace and noNamepaceDefinedForPrefix_1959010 used assertEquals on serialized XML strings, which broke under the namespace-node ordering shift introduced by PR #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 #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) --- .../java/org/exist/xquery/XQueryTest.java | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/exist-core/src/test/java/org/exist/xquery/XQueryTest.java b/exist-core/src/test/java/org/exist/xquery/XQueryTest.java index 22846889895..e4a9a67cc57 100644 --- a/exist-core/src/test/java/org/exist/xquery/XQueryTest.java +++ b/exist-core/src/test/java/org/exist/xquery/XQueryTest.java @@ -1759,7 +1759,14 @@ public void attributeNamespace() throws XMLDBException { XPathQueryService.class); ResourceSet result = service.query(query); assertEquals(1, result.getSize()); - assertEquals("" + "ccc" + "", result.getResource(0).getContent().toString()); + // XQuery 3.1 §2: "the relative order of namespace nodes that share a parent is also implementation dependent." + final Source expected = Input.fromString("ccc").build(); + final Source actual = Input.fromString(result.getResource(0).getContent().toString()).build(); + final Diff diff = DiffBuilder.compare(expected) + .withTest(actual) + .checkForIdentical() + .build(); + assertFalse(diff.toString(), diff.hasDifferences()); } @Test @@ -2969,8 +2976,14 @@ public void noNamepaceDefinedForPrefix_1959010() throws XMLDBException { ResourceSet result = service.query(query); assertEquals(1, result.getSize()); - assertEquals(query, "ccc", - result.getResource(0).getContent().toString()); + // XQuery 3.1 §2: "the relative order of namespace nodes that share a parent is also implementation dependent." + final Source expected = Input.fromString("ccc").build(); + final Source actual = Input.fromString(result.getResource(0).getContent().toString()).build(); + final Diff diff = DiffBuilder.compare(expected) + .withTest(actual) + .checkForIdentical() + .build(); + assertFalse(query + "\n" + diff.toString(), diff.hasDifferences()); } /** From 792e62279de63bcd9081ba6cf4c5d88813bae300 Mon Sep 17 00:00:00 2001 From: Joe Wicentowski Date: Mon, 1 Jun 2026 08:25:12 -0400 Subject: [PATCH 3/3] [refactor] Address Codacy nits in DocumentBuilderReceiver (NPath + empty bodies + reassigning param) Per @reinhapa's 2026-06-01 review on PR #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 --- .../dom/memtree/DocumentBuilderReceiver.java | 85 +++++++++++++------ 1 file changed, 61 insertions(+), 24 deletions(-) diff --git a/exist-core/src/main/java/org/exist/dom/memtree/DocumentBuilderReceiver.java b/exist-core/src/main/java/org/exist/dom/memtree/DocumentBuilderReceiver.java index e51782b1178..4f03618bbeb 100644 --- a/exist-core/src/main/java/org/exist/dom/memtree/DocumentBuilderReceiver.java +++ b/exist-core/src/main/java/org/exist/dom/memtree/DocumentBuilderReceiver.java @@ -107,6 +107,7 @@ public XQueryContext getContext() { @Override public void setDocumentLocator(Locator locator) { + // no-op: in-memory builder does not surface source-location information. } @Override @@ -276,26 +277,55 @@ private void emitNamespaceNode(final String prefix, final String uri) { } final DocumentImpl doc = builder.getDocument(); final int parent = doc.getLastNode(); - if (parent < 0 || doc.getNodeType(parent) != org.w3c.dom.Node.ELEMENT_NODE) { + if (!isElementParent(doc, parent)) { return; } - final QName parentName = doc.nodeName[parent]; - if (parentName != null && prefix.equals(parentName.getPrefix()) - && uri.equals(parentName.getNamespaceURI())) { + if (isParentSelfDeclaration(doc, parent, prefix, uri)) { + return; + } + if (hasExistingPrefixDeclaration(doc, parent, prefix)) { return; } + builder.namespaceNode(prefix, uri); + } + + private static boolean isElementParent(final DocumentImpl doc, final int parent) { + return parent >= 0 && doc.getNodeType(parent) == org.w3c.dom.Node.ELEMENT_NODE; + } + + /** + * The parent element already carries the prefix-to-uri binding via its + * own name (e.g. parent is {@code } and we're being + * asked to emit {@code xmlns:c="..."} for the same URI). The declaration + * is redundant. + */ + private static boolean isParentSelfDeclaration(final DocumentImpl doc, final int parent, + final String prefix, final String uri) { + final QName parentName = doc.nodeName[parent]; + return parentName != null + && prefix.equals(parentName.getPrefix()) + && uri.equals(parentName.getNamespaceURI()); + } + + /** + * Scan the namespace declarations already attached to {@code parent} and + * return true if any of them binds the same {@code prefix}. + */ + private static boolean hasExistingPrefixDeclaration(final DocumentImpl doc, final int parent, + final String prefix) { final int firstNs = doc.alphaLen[parent]; - if (firstNs >= 0) { - for (int ns = firstNs; - ns < doc.nextNamespace && doc.namespaceParent[ns] == parent; - ns++) { - final QName nsName = doc.namespaceCode[ns]; - if (nsName != null && prefix.equals(nsName.getLocalPart())) { - return; - } + if (firstNs < 0) { + return false; + } + for (int ns = firstNs; + ns < doc.nextNamespace && doc.namespaceParent[ns] == parent; + ns++) { + final QName nsName = doc.namespaceCode[ns]; + if (nsName != null && prefix.equals(nsName.getLocalPart())) { + return true; } } - builder.namespaceNode(prefix, uri); + return false; } @Override @@ -317,18 +347,22 @@ public void cdataSection(final char[] ch, final int start, final int len) throws @Override public void skippedEntity(final String name) throws SAXException { + // no-op: entity references are not surfaced through the in-memory builder. } @Override public void endCDATA() throws SAXException { + // no-op: CDATA boundaries are not surfaced through the in-memory builder. } @Override public void endDTD() throws SAXException { + // no-op: DTD declarations are not surfaced through the in-memory builder. } @Override public void startCDATA() throws SAXException { + // no-op: CDATA boundaries are not surfaced through the in-memory builder. } @Override @@ -343,14 +377,17 @@ public void comment(final char[] ch, final int start, final int length) throws S @Override public void endEntity(final String name) throws SAXException { + // no-op: entity boundaries are not surfaced through the in-memory builder. } @Override public void startEntity(final String name) throws SAXException { + // no-op: entity boundaries are not surfaced through the in-memory builder. } @Override public void startDTD(final String name, final String publicId, final String systemId) throws SAXException { + // no-op: DTD declarations are not surfaced through the in-memory builder. } @Override @@ -394,18 +431,18 @@ private QName checkNS(boolean isElement, final QName qname) { return qname; } - private String generatePrefix(final XQueryContext context, String prefix) { + private String generatePrefix(final XQueryContext context, final String requestedPrefix) { + if (requestedPrefix != null) { + return requestedPrefix; + } + // Generate "XXX", "XXX1", "XXX2", ... until we find one not already + // bound in scope. + String candidate = "XXX"; int i = 0; - while(prefix == null) { - prefix = "XXX"; - if(i > 0) { - prefix += String.valueOf(i); - } - if(context.getInScopeNamespace(prefix) != null) { - prefix = null; - i++; - } + while (context.getInScopeNamespace(candidate) != null) { + i++; + candidate = "XXX" + i; } - return prefix; + return candidate; } } \ No newline at end of file