From 6f83fa3ef7da9a80a1914ded3c016adf7422f930 Mon Sep 17 00:00:00 2001 From: maximthomas Date: Wed, 7 Oct 2026 15:53:17 +0300 Subject: [PATCH 01/16] [#157] Add a linear-time writer for the XML store document --- .../connectors/xml/XmlDocumentWriter.java | 241 ++++++++++++ .../connectors/xml/util/XmlHandlerUtil.java | 20 +- .../connectors/xml/XercesNodeLists.java | 61 +++ .../xml/XmlDocumentWriterTests.java | 349 ++++++++++++++++++ 4 files changed, 670 insertions(+), 1 deletion(-) create mode 100644 OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/XmlDocumentWriter.java create mode 100644 OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XercesNodeLists.java create mode 100644 OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XmlDocumentWriterTests.java diff --git a/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/XmlDocumentWriter.java b/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/XmlDocumentWriter.java new file mode 100644 index 000000000..739550ada --- /dev/null +++ b/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/XmlDocumentWriter.java @@ -0,0 +1,241 @@ +/* + * The contents of this file are subject to the terms of the Common Development and + * Distribution License (the License). You may not use this file except in compliance with the + * License. + * + * You can obtain a copy of the License at legal/CDDLv1.0.txt. See the License for the + * specific language governing permission and limitations under the License. + * + * When distributing Covered Software, include this CDDL Header Notice in each file and include + * the License file at legal/CDDLv1.0.txt. If applicable, add the following below the CDDL + * Header, with the fields enclosed by brackets [] replaced by your own identifying + * information: "Portions copyright [year] [name of copyright owner]". + * + * Copyright 2026 3A Systems, LLC. + */ + +package org.forgerock.openicf.connectors.xml; + +import static org.forgerock.openicf.connectors.xml.util.XmlHandlerUtil.following; + +import java.io.File; +import java.io.FileOutputStream; +import java.io.IOException; +import java.io.OutputStream; +import java.util.ArrayList; +import java.util.List; +import java.util.zip.CRC32; +import java.util.zip.CheckedOutputStream; + +import javax.xml.transform.OutputKeys; +import javax.xml.transform.Transformer; +import javax.xml.transform.TransformerException; +import javax.xml.transform.sax.SAXTransformerFactory; +import javax.xml.transform.sax.TransformerHandler; +import javax.xml.transform.stream.StreamResult; + +import org.w3c.dom.Document; +import org.w3c.dom.NamedNodeMap; +import org.w3c.dom.Node; +import org.xml.sax.SAXException; +import org.xml.sax.helpers.AttributesImpl; +import org.xml.sax.helpers.NamespaceSupport; + +/** + * Writes the store document to its file in time linear in the size of the document. + *

+ * Saxon's DOM wrappers walk a DOM through {@code NodeList.item(i)}, which turns quadratic on a + * Xerces DOM once the document's node list cache gets into a bad state. Here both steps follow + * sibling pointers, and Saxon's serializer only receives SAX events, so the output is what it was. + */ +final class XmlDocumentWriter { + + private static final String XMLNS = "xmlns"; + + private XmlDocumentWriter() { + } + + /** + * Removes whitespace-only text and turns every other run of text into a single Text node; + * returns whether anything changed. + *

+ * A run is a maximal sequence of adjacent Text and CDATA section siblings, which XPath sees as + * one text node. A run that holds only XML whitespace (space, tab, CR, LF: what + * {@code normalize-space} strips) is removed, as {@code //text()[normalize-space(.) = '']} + * removed it before, CDATA included. Any other run becomes one Text node with the same value, + * which is what a reload of the written file gives. + */ + static boolean normalizeText(Node root) { + boolean changed = false; + Node node = root.getFirstChild(); + while (node != null) { + if (!isText(node)) { + Node child = node.getFirstChild(); + node = child != null ? child : following(node, root); + continue; + } + Node parent = node.getParentNode(); + List run = new ArrayList(); + StringBuilder value = new StringBuilder(); + Node after = node; + while (after != null && isText(after)) { + run.add(after); + value.append(after.getNodeValue()); + after = after.getNextSibling(); + } + boolean blank = isXmlWhitespace(value); + if (blank || run.size() > 1 || node.getNodeType() != Node.TEXT_NODE) { + if (!blank) { + parent.insertBefore(parent.getOwnerDocument().createTextNode(value.toString()), node); + } + for (Node n : run) { + parent.removeChild(n); + } + changed = true; + } + node = after != null ? after : following(parent, root); + } + return changed; + } + + /** + * Writes {@code document} to {@code file} through Saxon's serializer with the output settings + * used before, and returns the CRC-32 of the bytes written. + *

+ * Saxon's factory is created directly: the connector bundle embeds an old xml-apis, which lacks + * the factory methods of newer JDKs and whose DOM classes the JDK's own transformer cannot take. + */ + static long write(Document document, File file) throws IOException, TransformerException, SAXException { + SAXTransformerFactory factory = new net.sf.saxon.TransformerFactoryImpl(); + TransformerHandler handler = factory.newTransformerHandler(); + Transformer serializer = handler.getTransformer(); + serializer.setOutputProperty(OutputKeys.INDENT, "yes"); + serializer.setOutputProperty(OutputKeys.METHOD, "xml"); + serializer.setOutputProperty(OutputKeys.ENCODING, "UTF-8"); + serializer.setOutputProperty("{http://xml.apache.org/xslt}indent-amount", "2"); + CRC32 crc = new CRC32(); + try (OutputStream out = new CheckedOutputStream(new FileOutputStream(file), crc)) { + handler.setResult(new StreamResult(out)); + NamespaceSupport namespaces = new NamespaceSupport(); + handler.startDocument(); + for (Node child = document.getFirstChild(); child != null; child = child.getNextSibling()) { + send(child, handler, namespaces); + } + handler.endDocument(); + } + return crc.getValue(); + } + + private static void send(Node node, TransformerHandler out, NamespaceSupport namespaces) throws SAXException { + switch (node.getNodeType()) { + case Node.ELEMENT_NODE: + sendElement(node, out, namespaces); + break; + case Node.TEXT_NODE: + case Node.CDATA_SECTION_NODE: + char[] text = node.getNodeValue().toCharArray(); + out.characters(text, 0, text.length); + break; + case Node.COMMENT_NODE: + char[] comment = node.getNodeValue().toCharArray(); + out.comment(comment, 0, comment.length); + break; + case Node.PROCESSING_INSTRUCTION_NODE: + out.processingInstruction(node.getNodeName(), node.getNodeValue()); + break; + case Node.ENTITY_REFERENCE_NODE: + for (Node child = node.getFirstChild(); child != null; child = child.getNextSibling()) { + send(child, out, namespaces); + } + break; + default: + // Document types and the like were not written before either. + break; + } + } + + private static void sendElement(Node element, TransformerHandler out, NamespaceSupport namespaces) + throws SAXException { + namespaces.pushContext(); + List declared = new ArrayList(); + NamedNodeMap map = element.getAttributes(); // an attribute map, not a child list + for (int i = 0; i < map.getLength(); i++) { + Node attribute = map.item(i); + String name = attribute.getNodeName(); + if (XMLNS.equals(name)) { + declare("", attribute.getNodeValue(), out, namespaces, declared); + } else if (name.startsWith(XMLNS + ":")) { + declare(name.substring(XMLNS.length() + 1), attribute.getNodeValue(), out, namespaces, declared); + } + } + String qName = element.getNodeName(); + String uri = bind(prefixOf(qName), element.getNamespaceURI(), out, namespaces, declared); + AttributesImpl attributes = new AttributesImpl(); + for (int i = 0; i < map.getLength(); i++) { + Node attribute = map.item(i); + String name = attribute.getNodeName(); + if (!XMLNS.equals(name) && !name.startsWith(XMLNS + ":")) { + String prefix = prefixOf(name); + String attributeUri = prefix.isEmpty() ? "" + : bind(prefix, attribute.getNamespaceURI(), out, namespaces, declared); + attributes.addAttribute(attributeUri, localNameOf(name), name, "CDATA", attribute.getNodeValue()); + } + } + out.startElement(uri, localNameOf(qName), qName, attributes); + for (Node child = element.getFirstChild(); child != null; child = child.getNextSibling()) { + send(child, out, namespaces); + } + out.endElement(uri, localNameOf(qName), qName); + for (String prefix : declared) { + out.endPrefixMapping(prefix); + } + namespaces.popContext(); + } + + /** + * Returns the namespace to use for {@code prefix}: the node's own, declared here if it is not + * in scope yet, or, for a DOM level 1 node without one, the namespace in scope. + */ + private static String bind(String prefix, String uri, TransformerHandler out, NamespaceSupport namespaces, + List declared) throws SAXException { + String inScope = namespaces.getURI(prefix); + if (uri == null) { + return inScope == null ? "" : inScope; + } + if (!uri.equals(inScope == null ? "" : inScope)) { + declare(prefix, uri, out, namespaces, declared); + } + return uri; + } + + private static void declare(String prefix, String uri, TransformerHandler out, NamespaceSupport namespaces, + List declared) throws SAXException { + namespaces.declarePrefix(prefix, uri); + out.startPrefixMapping(prefix, uri); + declared.add(prefix); + } + + private static String prefixOf(String qName) { + int colon = qName.indexOf(':'); + return colon < 0 ? "" : qName.substring(0, colon); + } + + private static String localNameOf(String qName) { + return qName.substring(qName.indexOf(':') + 1); + } + + private static boolean isText(Node node) { + short type = node.getNodeType(); + return type == Node.TEXT_NODE || type == Node.CDATA_SECTION_NODE; + } + + private static boolean isXmlWhitespace(CharSequence value) { + for (int i = 0; i < value.length(); i++) { + char c = value.charAt(i); + if (c != ' ' && c != '\t' && c != '\r' && c != '\n') { + return false; + } + } + return true; + } +} diff --git a/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/util/XmlHandlerUtil.java b/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/util/XmlHandlerUtil.java index 5e0d4e5ec..ac58ac427 100644 --- a/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/util/XmlHandlerUtil.java +++ b/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/util/XmlHandlerUtil.java @@ -20,6 +20,7 @@ * with the fields enclosed by brackets [] replaced by * your own identifying information: * "Portions Copyrighted 2010 [name of copyright owner]" + * Portions Copyrighted 2026 3A Systems, LLC * * $Id$ */ @@ -29,6 +30,7 @@ import com.sun.xml.xsom.XSSchema; import org.identityconnectors.framework.common.objects.AttributeInfo; import org.identityconnectors.framework.common.objects.ObjectClass; +import org.w3c.dom.Node; public class XmlHandlerUtil { @@ -78,4 +80,20 @@ public static void checkObjectType(ObjectClass objClass, XSSchema schema) { throw new IllegalArgumentException("Object type: " + objClass.getObjectClassValue() + " is not supported."); } } -} \ No newline at end of file + + /** + * Returns the node that follows {@code node} and its descendants in document order without + * leaving {@code root}, or {@code null} at the end. {@code root} is {@code node}, one of its + * ancestors, or {@code null} for the whole tree. Uses sibling and parent pointers only, so a + * walk built on it stays linear on a Xerces DOM. + */ + public static Node following(Node node, Node root) { + for (Node n = node; n != root; n = n.getParentNode()) { + Node sibling = n.getNextSibling(); + if (sibling != null) { + return sibling; + } + } + return null; + } +} diff --git a/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XercesNodeLists.java b/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XercesNodeLists.java new file mode 100644 index 000000000..d2b01c6bb --- /dev/null +++ b/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XercesNodeLists.java @@ -0,0 +1,61 @@ +/* + * The contents of this file are subject to the terms of the Common Development and + * Distribution License (the License). You may not use this file except in compliance with the + * License. + * + * You can obtain a copy of the License at legal/CDDLv1.0.txt. See the License for the + * specific language governing permission and limitations under the License. + * + * When distributing Covered Software, include this CDDL Header Notice in each file and include + * the License file at legal/CDDLv1.0.txt. If applicable, add the following below the CDDL + * Header, with the fields enclosed by brackets [] replaced by your own identifying + * information: "Portions copyright [year] [name of copyright owner]". + * + * Copyright 2026 3A Systems, LLC. + */ + +package org.forgerock.openicf.connectors.xml; + +import static org.testng.Assert.assertTrue; + +import java.lang.reflect.Field; + +import org.apache.xerces.dom.CoreDocumentImpl; +import org.apache.xerces.dom.ParentNode; +import org.w3c.dom.Document; +import org.w3c.dom.Node; + +/** + * Tells whether anything walked a Xerces DOM through {@code NodeList.item(i)} or + * {@code getLength()}: those calls allocate the node list caches whose free list makes such walks + * quadratic (#157). Sibling-pointer walks allocate none. + */ +final class XercesNodeLists { + + private XercesNodeLists() { + } + + static boolean used(Document document) throws Exception { + assertTrue(document instanceof CoreDocumentImpl, document.getClass().getName()); + Field free = CoreDocumentImpl.class.getDeclaredField("fFreeNLCache"); + free.setAccessible(true); + if (free.get(document) != null) { + return true; + } + Field cache = ParentNode.class.getDeclaredField("fNodeListCache"); + cache.setAccessible(true); + Node node = document; + while (node != null) { + if (node instanceof ParentNode && cache.get(node) != null) { + return true; + } + Node next = node.getFirstChild(); + while (next == null && node != null) { + next = node.getNextSibling(); + node = node.getParentNode(); + } + node = next; + } + return false; + } +} diff --git a/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XmlDocumentWriterTests.java b/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XmlDocumentWriterTests.java new file mode 100644 index 000000000..a65a6a2ba --- /dev/null +++ b/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XmlDocumentWriterTests.java @@ -0,0 +1,349 @@ +/* + * The contents of this file are subject to the terms of the Common Development and + * Distribution License (the License). You may not use this file except in compliance with the + * License. + * + * You can obtain a copy of the License at legal/CDDLv1.0.txt. See the License for the + * specific language governing permission and limitations under the License. + * + * When distributing Covered Software, include this CDDL Header Notice in each file and include + * the License file at legal/CDDLv1.0.txt. If applicable, add the following below the CDDL + * Header, with the fields enclosed by brackets [] replaced by your own identifying + * information: "Portions copyright [year] [name of copyright owner]". + * + * Copyright 2026 3A Systems, LLC. + */ + +package org.forgerock.openicf.connectors.xml; + +import static org.forgerock.openicf.connectors.xml.util.XmlHandlerUtil.following; +import static org.testng.Assert.assertEquals; +import static org.testng.Assert.assertFalse; +import static org.testng.Assert.assertNotNull; +import static org.testng.Assert.assertNull; +import static org.testng.Assert.assertSame; +import static org.testng.Assert.assertTrue; + +import java.io.ByteArrayOutputStream; +import java.io.File; +import java.io.StringReader; +import java.nio.file.Files; +import java.util.zip.CRC32; + +import javax.xml.parsers.DocumentBuilderFactory; +import javax.xml.transform.OutputKeys; +import javax.xml.transform.Transformer; +import javax.xml.transform.dom.DOMSource; +import javax.xml.transform.stream.StreamResult; + +import org.testng.annotations.Test; +import org.w3c.dom.Document; +import org.w3c.dom.Element; +import org.w3c.dom.NamedNodeMap; +import org.w3c.dom.Node; +import org.w3c.dom.NodeList; +import org.xml.sax.InputSource; + +public class XmlDocumentWriterTests { + + private static final String ICF = "http://openidm.forgerock.com/xml/ns/public/resource/openicf/resource-schema-1.xsd"; + private static final String RI = "http://openidm.forgerock.com/xml/ns/public/resource/instances/ef2bc95b-76e0-48e2-86d6-4d4f44d4e4a4"; + private static final String XSI = "http://www.w3.org/2001/XMLSchema-instance"; + private static final String XMLNS = "http://www.w3.org/2000/xmlns/"; + private static final String TRICKY = " a>b&c 😀 "; + + @Test + public void followingIsTheNextSibling() throws Exception { + Document document = parse(""); + assertSame(following(only(document, "a"), document), only(document, "b")); + } + + @Test + public void followingClimbsToTheNextSiblingOfAnAncestor() throws Exception { + Document document = parse(""); + assertSame(following(only(document, "x"), document), only(document, "b")); + } + + @Test + public void followingStopsAtRoot() throws Exception { + Document document = parse(""); + assertNull(following(only(document, "x"), only(document, "a"))); + } + + @Test + public void normalizedDocumentIsLeftAlone() throws Exception { + Document document = parse("x"); + assertFalse(XmlDocumentWriter.normalizeText(document)); + } + + @Test + public void whitespaceOnlyTextIsRemoved() throws Exception { + Document document = parse("\n \t \n \n x\n"); + boolean changed = XmlDocumentWriter.normalizeText(document); + assertNull(only(document, "a").getFirstChild()); + for (Node n = document.getDocumentElement().getFirstChild(); n != null; n = n.getNextSibling()) { + assertEquals(n.getNodeType(), Node.ELEMENT_NODE); + } + assertTrue(changed); + } + + @Test + public void paddedAndNonXmlSpaceValuesAreKept() throws Exception { + // U+3000 is whitespace to Character.isWhitespace and String.isBlank, but not to XML. + Document document = parse(" x \u3000"); + XmlDocumentWriter.normalizeText(document); + assertSingleText(only(document, "a"), " x "); + assertSingleText(only(document, "b"), "\u3000"); + } + + @Test + public void whitespaceOnlyCdataIsRemoved() throws Exception { + Document document = parse(""); + XmlDocumentWriter.normalizeText(document); + assertNull(only(document, "a").getFirstChild()); + } + + @Test + public void adjacentTextAndCdataBecomeOneTextNode() throws Exception { + Document document = parse(" "); + XmlDocumentWriter.normalizeText(document); + assertSingleText(only(document, "a"), " x"); + XmlDocumentWriter.normalizeText(document); + assertSingleText(only(document, "b"), "z"); + } + + @Test + public void outputIsTheSameAsBefore() throws Exception { + Document document = parse("" + + "ab" + + ""); + XmlDocumentWriter.normalizeText(document); + File file = XmlConnectorTestUtil.getRandomXMLFile(); + XmlDocumentWriter.write(document, file); + + assertEquals(new String(Files.readAllBytes(file.toPath()), "UTF-8"), before(document)); + } + + @Test + public void writeReturnsTheChecksumOfTheFile() throws Exception { + Document document = parse("x"); + File file = XmlConnectorTestUtil.getRandomXMLFile(); + long written = XmlDocumentWriter.write(document, file); + CRC32 crc = new CRC32(); + crc.update(Files.readAllBytes(file.toPath())); + assertEquals(written, crc.getValue()); + } + + @Test + public void siblingsDeclareTheirOwnNamespaces() throws Exception { + Document document = newFactory().newDocumentBuilder().newDocument(); + Element root = document.createElementNS(null, "r"); + document.appendChild(root); + root.appendChild(document.createElementNS("urn:1", "p:a")); + root.appendChild(document.createElementNS("urn:1", "p:b")); + assertReadsBackTheSame(document); + } + + @Test + public void cdataIsWrittenAsText() throws Exception { + Document document = parse(""); + assertReadsBackTheSame(document); // not normalized: write() sends the CDATA section itself + } + + @Test + public void entityReferencesAndDoctypeAreWrittenAsBefore() throws Exception { + DocumentBuilderFactory factory = newFactory(); + factory.setExpandEntityReferences(false); + Document document = factory.newDocumentBuilder().parse(new InputSource(new StringReader( + "'>]>&e;"))); + assertEquals(document.getDocumentElement().getFirstChild().getNodeType(), Node.ENTITY_REFERENCE_NODE); + File file = XmlConnectorTestUtil.getRandomXMLFile(); + XmlDocumentWriter.write(document, file); + assertEquals(new String(Files.readAllBytes(file.toPath()), "UTF-8"), before(document)); + } + + @Test + public void prefixedAttributeWithoutANamespaceTakesTheOneInScope() throws Exception { + // XMLHandlerImpl.createDocument() sets xsi:schemaLocation with setAttribute, a DOM level 1 call. + Document document = parse(""); + document.getDocumentElement().setAttribute("xsi:schemaLocation", "urn:a a.xsd"); + File file = XmlConnectorTestUtil.getRandomXMLFile(); + XmlDocumentWriter.write(document, file); + assertEquals(parseFile(file).getDocumentElement().getAttributeNS(XSI, "schemaLocation"), "urn:a a.xsd"); + } + + @Test + public void defaultNamespaceUndeclarationIsKept() throws Exception { + assertReadsBackTheSame(parse("")); + } + + @Test + public void namespacedAttributeWithoutAPrefixDeclaresNoDefaultNamespace() throws Exception { + Document document = parse(""); + document.getDocumentElement().setAttributeNS("urn:x", "a", "v"); + File file = XmlConnectorTestUtil.getRandomXMLFile(); + XmlDocumentWriter.write(document, file); + assertNull(parseFile(file).getDocumentElement().getNamespaceURI()); + } + + @Test + public void createdDocumentReadsBackTheSame() throws Exception { + // No xmlns:icf attribute on the root and none for the entries: the writer has to declare them itself. + Document document = DocumentBuilderFactory.newInstance().newDocumentBuilder() + .getDOMImplementation().createDocument(ICF, "OpenICFContainer", null); + Element root = document.getDocumentElement(); + root.setAttributeNS(XMLNS, "xmlns:xsi", XSI); + root.setAttributeNS(XMLNS, "xmlns:ri", RI); + root.setPrefix("icf"); + root.setAttribute("xsi:schemaLocation", RI + " schema.xsd"); + Element entry = document.createElementNS(RI, "__ACCOUNT__"); + entry.setPrefix("ri"); + Element name = document.createElementNS(ICF, "__NAME__"); + name.setPrefix("icf"); + name.setTextContent(TRICKY); + Element empty = document.createElementNS(RI, "firstname"); + empty.setPrefix("ri"); + empty.setTextContent(""); + entry.appendChild(name); + entry.appendChild(empty); + entry.appendChild(document.createComment(" c ")); + root.appendChild(entry); + + assertReadsBackTheSame(document); + } + + @Test + public void handEditedDocumentReadsBackTheSame() throws Exception { + Document document = parse("\n \n joe\n" + + " ab\n \t😀]]>\n" + + " \n \n"); + XmlDocumentWriter.normalizeText(document); + assertReadsBackTheSame(document); + } + + @Test + public void commentsSplitTextRuns() throws Exception { + Document document = parse(" "); + XmlDocumentWriter.normalizeText(document); + Node child = only(document, "a").getFirstChild(); + assertEquals(child.getNodeType(), Node.COMMENT_NODE); + assertNull(child.getNextSibling()); + } + + @Test + public void normalizeAndWriteNeverUseNodeLists() throws Exception { + String xml = "\n" + + " \n a\n \n" + + " \n b\n \n"; + + // The probe sees Saxon's DOMSender, which is what dispose() used before. + Document control = parse(xml); + assertFalse(XercesNodeLists.used(control)); + new net.sf.saxon.TransformerFactoryImpl().newTransformer() + .transform(new DOMSource(control), new StreamResult(new ByteArrayOutputStream())); + assertTrue(XercesNodeLists.used(control)); + + Document document = parse(xml); + XmlDocumentWriter.normalizeText(document); + XmlDocumentWriter.write(document, XmlConnectorTestUtil.getRandomXMLFile()); + assertFalse(XercesNodeLists.used(document)); + } + + /** What XMLHandlerImpl.dispose() wrote before #157. */ + private static String before(Document document) throws Exception { + Transformer saxon = new net.sf.saxon.TransformerFactoryImpl().newTransformer(); + saxon.setOutputProperty(OutputKeys.INDENT, "yes"); + saxon.setOutputProperty(OutputKeys.METHOD, "xml"); + saxon.setOutputProperty(OutputKeys.ENCODING, "UTF-8"); + saxon.setOutputProperty("{http://xml.apache.org/xslt}indent-amount", "2"); + ByteArrayOutputStream out = new ByteArrayOutputStream(); + saxon.transform(new DOMSource(document), new StreamResult(out)); + return out.toString("UTF-8"); + } + + private static void assertReadsBackTheSame(Document document) throws Exception { + File file = XmlConnectorTestUtil.getRandomXMLFile(); + XmlDocumentWriter.write(document, file); + assertTrue(Files.size(file.toPath()) > 0); + assertEquals(describe(parseFile(file)), describe(document)); + } + + private static Document parse(String xml) throws Exception { + return newFactory().newDocumentBuilder().parse(new InputSource(new StringReader(xml))); + } + + private static Document parseFile(File file) throws Exception { + Document document = newFactory().newDocumentBuilder().parse(file); + XmlDocumentWriter.normalizeText(document); // drops the indentation the serializer added + return document; + } + + private static DocumentBuilderFactory newFactory() { + DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance(); + factory.setNamespaceAware(true); + return factory; + } + + private static Element only(Document document, String localName) { + NodeList list = document.getElementsByTagNameNS("*", localName); + assertEquals(list.getLength(), 1, localName); + return (Element) list.item(0); + } + + private static void assertSingleText(Element element, String value) { + Node child = element.getFirstChild(); + assertNotNull(child); + assertEquals(child.getNodeType(), Node.TEXT_NODE); + assertEquals(child.getNodeValue(), value); + assertNull(child.getNextSibling()); + } + + /** Elements by namespace and local name, non-xmlns attributes by namespace, local name and value, text, comments, PIs. */ + private static String describe(Node node) { + StringBuilder out = new StringBuilder(); + describe(node, out); + return out.toString(); + } + + private static void describe(Node node, StringBuilder out) { + for (Node n = node.getFirstChild(); n != null; n = n.getNextSibling()) { + switch (n.getNodeType()) { + case Node.ELEMENT_NODE: + out.append('<').append(n.getNamespaceURI()).append('|').append(n.getLocalName()); + NamedNodeMap attributes = n.getAttributes(); + for (int i = 0; i < attributes.getLength(); i++) { + Node a = attributes.item(i); + String name = a.getNodeName(); + if (!name.equals("xmlns") && !name.startsWith("xmlns:")) { + String uri = a.getNamespaceURI() != null ? a.getNamespaceURI() + : name.startsWith("xsi:") ? XSI : ""; + out.append(' ').append(uri).append('|').append(name.substring(name.indexOf(':') + 1)) + .append("=[").append(a.getNodeValue()).append(']'); + } + } + out.append('>'); + describe(n, out); + out.append(""); + break; + case Node.TEXT_NODE: + case Node.CDATA_SECTION_NODE: + out.append("[text:").append(n.getNodeValue()).append(']'); + break; + case Node.COMMENT_NODE: + out.append("[comment:").append(n.getNodeValue()).append(']'); + break; + case Node.PROCESSING_INSTRUCTION_NODE: + out.append("[pi:").append(n.getNodeName()).append(' ').append(n.getNodeValue()).append(']'); + break; + default: + break; + } + } + } +} From 4fbcc475ff1e48b6232c9dc5bfd59a44a885d0a4 Mon Sep 17 00:00:00 2001 From: maximthomas Date: Thu, 8 Oct 2026 15:02:06 +0300 Subject: [PATCH 02/16] [#157] Save the XML document in linear time --- .../connectors/xml/XMLHandlerImpl.java | 92 ++------------- .../connectors/xml/XMLConnectorTests.java | 109 +++++++++++++++++- 2 files changed, 116 insertions(+), 85 deletions(-) diff --git a/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/XMLHandlerImpl.java b/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/XMLHandlerImpl.java index 604d58d3c..8b58ceb8e 100644 --- a/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/XMLHandlerImpl.java +++ b/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/XMLHandlerImpl.java @@ -38,8 +38,6 @@ import com.sun.xml.xsom.XSSchemaSet; import java.io.File; -import java.io.FileNotFoundException; -import java.io.FileOutputStream; import java.io.IOException; import java.util.ArrayList; import java.util.Collection; @@ -52,16 +50,7 @@ import javax.xml.parsers.DocumentBuilder; import javax.xml.parsers.DocumentBuilderFactory; import javax.xml.parsers.ParserConfigurationException; -import javax.xml.transform.OutputKeys; -import javax.xml.transform.Transformer; import javax.xml.transform.TransformerException; -import javax.xml.transform.TransformerFactory; -import javax.xml.transform.dom.DOMSource; -import javax.xml.transform.stream.StreamResult; -import javax.xml.xpath.XPathConstants; -import javax.xml.xpath.XPathExpression; -import javax.xml.xpath.XPathExpressionException; -import javax.xml.xpath.XPathFactory; import javax.xml.xquery.XQException; import javax.xml.xquery.XQResultSequence; @@ -395,85 +384,18 @@ public void dispose() { } try { - // Synchronize on the document so that XPath cleanup and the - // Saxon transform happen atomically with respect to any other - // thread that might still hold a reference to the same DOM. - // Saxon's DOMSender walks children via NodeList.item(i) after - // calling getLength(); a concurrent removeChild on the DOM - // can make item(i) return null and trigger a NullPointerException - // in DOMSender.walkNode. + // Callers hold the handler's write lock; the document monitor stays as before. synchronized (document) { - try { - XPathFactory xpathFactory = new net.sf.saxon.xpath.XPathFactoryImpl(); - // XPath to find empty text nodes. - XPathExpression xpathExp = xpathFactory.newXPath().compile("//text()[normalize-space(.) = '']"); - NodeList emptyTextNodes = (NodeList) xpathExp.evaluate(document, XPathConstants.NODESET); - - // Snapshot the list before mutating the DOM, then remove - // each empty text node (guarding against nodes whose - // parent has already been detached). - int len = emptyTextNodes.getLength(); - List toRemove = new ArrayList(len); - for (int i = 0; i < len; i++) { - Node n = emptyTextNodes.item(i); - if (n != null) { - toRemove.add(n); - } - } - for (Node emptyTextNode : toRemove) { - Node parent = emptyTextNode.getParentNode(); - if (parent != null) { - parent.removeChild(emptyTextNode); - } - } - } catch (XPathExpressionException e) { - //We don't care. It's just formatting. - } - - TransformerFactory transformerFactory = new net.sf.saxon.TransformerFactoryImpl(); - Transformer transformer = transformerFactory.newTransformer(); - transformer.setOutputProperty(OutputKeys.INDENT, "yes"); - transformer.setOutputProperty(OutputKeys.METHOD, "xml"); - transformer.setOutputProperty(OutputKeys.ENCODING, "UTF-8"); - transformer.setOutputProperty("{http://xml.apache.org/xslt}indent-amount", "2"); - - DOMSource source = new DOMSource(document); - /* Running this code in java 5 we had to change - StreamResult result = new StreamResult(config.getXmlFilePath()); - into - StreamResult result = new StreamResult(config.getXmlFilePath().getPath()); - Otherwise you get the following error: - javax.xml.transform.TransformerException: java.io.FileNotFoundException: - */ - /* - * If the safePath is not escaped then it throws - * net.sf.saxon.trans.XPathException: java.net.URISyntaxException: - * Illegal character in safePath at index 9: /temp/XML Connector/test.xml - * String safePath = config.getXmlFilePath().getPath().replaceAll(" ", "%20"); - */ - FileOutputStream fos = new FileOutputStream(config.getXmlFilePath()); - try { - StreamResult result = new StreamResult(fos); - transformer.transform(source, result); - } finally { - try { - fos.close(); - } catch (IOException ioe) { - log.warn("Failed to close XML output stream: {0}", ioe); - } - } + XmlDocumentWriter.normalizeText(document); + XmlDocumentWriter.write(document, config.getXmlFilePath()); } - log.info("Saving changes to xml file"); - } catch (TransformerException ex) { - log.error("Failed saving changes to xml file: {0}", ex); - throw ConnectorException.wrap(ex); - } catch (FileNotFoundException ex) { + } catch (TransformerException | SAXException | IOException ex) { log.error("Failed saving changes to xml file: {0}", ex); throw ConnectorException.wrap(ex); } - log.info("Entry {0}", method); + log.info("Exit {0}", method); } @Override @@ -550,6 +472,8 @@ private void createDocument() { document = implementation.createDocument(icfSchema.getTargetNamespace(), ICF_CONTAINER_TAG, null); Element root = document.getDocumentElement(); + // Declared like the others so that the writer puts it first, where Saxon's DOM serializer put it. + root.setAttributeNS("http://www.w3.org/2000/xmlns/", "xmlns:" + ICF_NAMESPACE_PREFIX, icfSchema.getTargetNamespace()); root.setAttributeNS("http://www.w3.org/2000/xmlns/", "xmlns:" + XSI_NAMESPACE_PREFIX, XSI_NAMESPACE); root.setAttributeNS("http://www.w3.org/2000/xmlns/", "xmlns:" + RI_NAMESPACE_PREFIX, riSchema.getTargetNamespace()); root.setPrefix(ICF_NAMESPACE_PREFIX); @@ -736,7 +660,7 @@ private String prefixAttributeName(String attrName) { return prefix; } - private Document getDocument() { + Document getDocument() { if (null == document) { throw new ConnectorException("Data file does not exists: " + config.getXmlFilePath().toString()); } diff --git a/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLConnectorTests.java b/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLConnectorTests.java index 066fe4a26..93d27e079 100644 --- a/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLConnectorTests.java +++ b/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLConnectorTests.java @@ -20,6 +20,7 @@ * with the fields enclosed by brackets [] replaced by * your own identifying information: * "Portions Copyrighted 2010 [name of copyright owner]" + * Portions Copyrighted 2026 3A Systems, LLC * * $Id$ */ @@ -28,31 +29,47 @@ import org.testng.annotations.Test; import org.testng.AssertJUnit; import static org.forgerock.openicf.connectors.xml.XmlConnectorTestUtil.*; +import static org.testng.Assert.expectThrows; +import java.io.ByteArrayOutputStream; +import java.io.File; +import java.io.PrintStream; +import java.io.UnsupportedEncodingException; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; import java.util.HashSet; import java.util.Set; +import javax.xml.parsers.DocumentBuilderFactory; +import org.forgerock.openicf.connectors.xml.xsdparser.SchemaParser; import org.identityconnectors.common.security.GuardedString; import org.identityconnectors.framework.api.APIConfiguration; import org.identityconnectors.framework.api.ConnectorFacade; import org.identityconnectors.framework.api.ConnectorFacadeFactory; +import org.identityconnectors.framework.common.exceptions.ConnectorException; import org.identityconnectors.framework.common.objects.Attribute; import org.identityconnectors.framework.common.objects.AttributeBuilder; +import org.identityconnectors.framework.common.objects.AttributeUtil; +import org.identityconnectors.framework.common.objects.ConnectorObject; import org.identityconnectors.framework.common.objects.ObjectClass; import org.identityconnectors.framework.common.objects.Schema; import org.identityconnectors.framework.common.objects.Uid; import org.identityconnectors.framework.common.objects.filter.EqualsFilter; import org.identityconnectors.test.common.TestHelpers; import org.testng.annotations.BeforeMethod; +import org.w3c.dom.Document; +import org.w3c.dom.NodeList; public class XMLConnectorTests { //private XMLConnector connector; private ConnectorFacade facade; + private File xmlFile; //private final static String XML_FILEPATH = "test/xml_store/test.xml"; @BeforeMethod public void init() { XMLConfiguration config = new XMLConfiguration(); - config.setXmlFilePath(getRandomXMLFile()); + xmlFile = getRandomXMLFile(); + config.setXmlFilePath(xmlFile); config.setXsdFilePath(XSD_SCHEMA_FILEPATH); config.setCreateFileIfNotExists(true); APIConfiguration impl = TestHelpers.createTestConfiguration(XMLConnector.class, config); @@ -235,4 +252,94 @@ public void authenticateShouldThrowExceptionWhenUsernameIsNull() { public void authenticateShouldThrowExceptionWhenUsernameIsBlank() { facade.authenticate(ObjectClass.ACCOUNT, "", new GuardedString(ATTR_ACCOUNT_VALUE_PASSWORD.toCharArray()), null); } + + @Test + public void newFileDeclaresNamespacesAsBefore() throws Exception { + facade.create(ObjectClass.ACCOUNT, getRequiredAccountAttributes(), null); + String content = new String(Files.readAllBytes(xmlFile.toPath()), StandardCharsets.UTF_8); + int icf = content.indexOf("xmlns:icf="); + int ri = content.indexOf("xmlns:ri="); + int xsi = content.indexOf("xmlns:xsi="); + AssertJUnit.assertTrue(content, 0 < icf && icf < ri && ri < xsi); + } + + @Test + public void valuesSurviveASaveAndAReload() throws Exception { + String lastName = " a>b&c \ud83d\ude00 "; + Set attributes = getRequiredAccountAttributes(); + attributes.remove(AttributeUtil.find(ATTR_ACCOUNT_LAST_NAME, attributes)); + attributes.add(AttributeBuilder.build(ATTR_ACCOUNT_LAST_NAME, lastName)); + Uid uid = facade.create(ObjectClass.ACCOUNT, attributes, null); + + DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance(); + factory.setNamespaceAware(true); + Document saved = factory.newDocumentBuilder().parse(xmlFile); + NodeList inFile = saved.getElementsByTagNameNS("*", ATTR_ACCOUNT_LAST_NAME); + AssertJUnit.assertEquals(1, inFile.getLength()); + AssertJUnit.assertEquals(lastName, inFile.item(0).getTextContent()); + + ConnectorObject read = facade.getObject(ObjectClass.ACCOUNT, uid, null); + AssertJUnit.assertEquals(lastName, + AttributeUtil.getStringValue(read.getAttributeByName(ATTR_ACCOUNT_LAST_NAME))); + } + + @Test + public void disposeNeverUsesNodeLists() throws Exception { + String icf = "http://openidm.forgerock.com/xml/ns/public/resource/openicf/resource-schema-1.xsd"; + String ri = "http://openidm.forgerock.com/xml/ns/public/resource/instances/ef2bc95b-76e0-48e2-86d6-4d4f44d4e4a4"; + Files.write(xmlFile.toPath(), ("\n" + + " \n a\n \n" + + " \n b\n \n" + + "\n").getBytes(StandardCharsets.UTF_8)); + XMLHandlerImpl handler = newHandler(); + handler.init(); + handler.dispose(); + AssertJUnit.assertFalse(XercesNodeLists.used(handler.getDocument())); + } + + @Test + public void whitespaceOnlyValueIsEmptiedBySave() throws Exception { + Set attributes = getRequiredAccountAttributes(); + attributes.add(AttributeBuilder.build(ATTR_ACCOUNT_FIRST_NAME, " \t ")); + facade.create(ObjectClass.ACCOUNT, attributes, null); + DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance(); + factory.setNamespaceAware(true); + NodeList inFile = factory.newDocumentBuilder().parse(xmlFile).getElementsByTagNameNS("*", ATTR_ACCOUNT_FIRST_NAME); + AssertJUnit.assertEquals(1, inFile.getLength()); + AssertJUnit.assertNull(inFile.item(0).getFirstChild()); + } + + @Test + public void failedSaveIsLoggedAndThrown() throws Exception { + XMLHandlerImpl handler = newHandler(); + handler.init(); // no file yet: a new document in memory + AssertJUnit.assertTrue(xmlFile.mkdir()); // a directory cannot be written, whoever runs the test + try { + String errors = captureStdErr(() -> expectThrows(ConnectorException.class, handler::dispose)); + AssertJUnit.assertTrue(errors, errors.contains("Failed saving changes to xml file: java.io.FileNotFoundException")); + } finally { + AssertJUnit.assertTrue(xmlFile.delete()); + } + } + + private XMLHandlerImpl newHandler() { + XMLConfiguration config = new XMLConfiguration(); + config.setXmlFilePath(xmlFile); + config.setXsdFilePath(XSD_SCHEMA_FILEPATH); + config.setCreateFileIfNotExists(true); + SchemaParser parser = new SchemaParser(XMLConnector.class, XSD_SCHEMA_FILEPATH); + return new XMLHandlerImpl(config, parser.parseSchema(), parser.getXsdSchema()); + } + + private static String captureStdErr(Runnable action) throws UnsupportedEncodingException { + PrintStream original = System.err; + ByteArrayOutputStream buffer = new ByteArrayOutputStream(); + System.setErr(new PrintStream(buffer, true, StandardCharsets.UTF_8.name())); + try { + action.run(); + } finally { + System.setErr(original); + } + return buffer.toString(StandardCharsets.UTF_8.name()); + } } From dca4047158111596b29449c121b4470d769172f8 Mon Sep 17 00:00:00 2001 From: maximthomas Date: Thu, 8 Oct 2026 15:08:34 +0300 Subject: [PATCH 03/16] [#157] Run the packaged XML connector bundle in an integration test --- OpenICF-xml-connector/pom.xml | 23 ++++++ .../connectors/xml/XMLConnectorBundleIT.java | 71 +++++++++++++++++++ .../connectors/xml/XmlConnectorTestUtil.java | 59 +++++++++++++++ 3 files changed, 153 insertions(+) create mode 100644 OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLConnectorBundleIT.java diff --git a/OpenICF-xml-connector/pom.xml b/OpenICF-xml-connector/pom.xml index 95750c629..ad4bfe191 100644 --- a/OpenICF-xml-connector/pom.xml +++ b/OpenICF-xml-connector/pom.xml @@ -21,6 +21,8 @@ with the fields enclosed by brackets [] replaced by your own identifying information: "Portions Copyrighted [year] [name of copyright owner]" + + Portions Copyrighted 2026 3A Systems, LLC --> 4.0.0 @@ -119,6 +121,27 @@ 10 + + + org.apache.maven.plugins + maven-failsafe-plugin + + + **/*BundleIT.java + + + ${project.build.directory}/${project.build.finalName}.jar + + + + + + integration-test + verify + + + + org.apache.felix maven-bundle-plugin diff --git a/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLConnectorBundleIT.java b/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLConnectorBundleIT.java new file mode 100644 index 000000000..5e7e5be15 --- /dev/null +++ b/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLConnectorBundleIT.java @@ -0,0 +1,71 @@ +/* + * The contents of this file are subject to the terms of the Common Development and + * Distribution License (the License). You may not use this file except in compliance with the + * License. + * + * You can obtain a copy of the License at legal/CDDLv1.0.txt. See the License for the + * specific language governing permission and limitations under the License. + * + * When distributing Covered Software, include this CDDL Header Notice in each file and include + * the License file at legal/CDDLv1.0.txt. If applicable, add the following below the CDDL + * Header, with the fields enclosed by brackets [] replaced by your own identifying + * information: "Portions copyright [year] [name of copyright owner]". + * + * Copyright 2026 3A Systems, LLC. + */ + +package org.forgerock.openicf.connectors.xml; + +import static org.forgerock.openicf.connectors.xml.XmlConnectorTestUtil.*; +import static org.testng.Assert.assertEquals; +import static org.testng.Assert.assertTrue; + +import java.io.File; +import java.util.Collections; + +import org.identityconnectors.common.security.GuardedString; +import org.identityconnectors.framework.api.APIConfiguration; +import org.identityconnectors.framework.api.ConfigurationProperties; +import org.identityconnectors.framework.api.ConnectorFacade; +import org.identityconnectors.framework.api.ConnectorFacadeFactory; +import org.identityconnectors.framework.api.ConnectorInfoManager; +import org.identityconnectors.framework.api.ConnectorInfoManagerFactory; +import org.identityconnectors.framework.common.objects.AttributeBuilder; +import org.identityconnectors.framework.common.objects.AttributeUtil; +import org.identityconnectors.framework.common.objects.ConnectorObject; +import org.identityconnectors.framework.common.objects.ObjectClass; +import org.identityconnectors.framework.common.objects.Uid; +import org.testng.annotations.Test; + +/** + * Runs the packaged bundle the way a connector server does: through a child-first bundle class + * loader that takes xml-apis, Xerces and Saxon from the bundle's own lib directory. + */ +public class XMLConnectorBundleIT { + + @Test + public void bundleSavesLoadsAndFindsEntries() throws Exception { + File bundle = new File(System.getProperty("bundleJar")); + assertTrue(bundle.isFile(), "bundle jar: " + bundle); + ConnectorInfoManager manager = ConnectorInfoManagerFactory.getInstance().getLocalManager(bundle.toURI().toURL()); + assertEquals(manager.getConnectorInfos().size(), 1); + APIConfiguration api = manager.getConnectorInfos().get(0).createDefaultAPIConfiguration(); + File xmlFile = getRandomXMLFile(); + ConfigurationProperties properties = api.getConfigurationProperties(); + properties.setPropertyValue("xmlFilePath", xmlFile); + properties.setPropertyValue("xsdFilePath", XSD_SCHEMA_FILEPATH); + properties.setPropertyValue("createFileIfNotExists", true); + ConnectorFacade facade = ConnectorFacadeFactory.getInstance().newInstance(api); + + Uid alice = facade.create(ObjectClass.ACCOUNT, account("alice"), null); + facade.create(ObjectClass.ACCOUNT, account("bob"), null); + facade.update(ObjectClass.ACCOUNT, alice, + Collections.singleton(AttributeBuilder.build(ATTR_ACCOUNT_LAST_NAME, "Changed")), null); + Uid bob = facade.authenticate(ObjectClass.ACCOUNT, "bob", new GuardedString("secret-bob".toCharArray()), null); + facade.delete(ObjectClass.ACCOUNT, bob, null); + + assertEquals(namesInFile(xmlFile), Collections.singletonList("alice")); + ConnectorObject read = facade.getObject(ObjectClass.ACCOUNT, alice, null); + assertEquals(AttributeUtil.getStringValue(read.getAttributeByName(ATTR_ACCOUNT_LAST_NAME)), "Changed"); + } +} diff --git a/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XmlConnectorTestUtil.java b/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XmlConnectorTestUtil.java index 49ec44893..210a62e7f 100644 --- a/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XmlConnectorTestUtil.java +++ b/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XmlConnectorTestUtil.java @@ -20,20 +20,28 @@ * with the fields enclosed by brackets [] replaced by * your own identifying information: * "Portions Copyrighted 2010 [name of copyright owner]" + * Portions Copyrighted 2026 3A Systems, LLC * * $Id$ */ package org.forgerock.openicf.connectors.xml; import java.io.File; +import java.io.IOException; import java.net.URISyntaxException; import java.net.URL; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.attribute.FileTime; +import java.util.ArrayList; import java.util.Collection; import java.util.HashMap; import java.util.HashSet; +import java.util.List; import java.util.Map; import java.util.Set; import java.util.UUID; +import javax.xml.parsers.DocumentBuilderFactory; import org.identityconnectors.framework.common.objects.Attribute; import org.identityconnectors.framework.common.objects.AttributeBuilder; import org.identityconnectors.framework.common.objects.AttributeUtil; @@ -41,6 +49,7 @@ import org.identityconnectors.framework.common.objects.Name; import org.identityconnectors.framework.common.objects.ResultsHandler; import org.identityconnectors.framework.common.objects.Uid; +import org.w3c.dom.NodeList; public class XmlConnectorTestUtil { @@ -114,6 +123,56 @@ public class XmlConnectorTestUtil { ICF_SCHEMA_FILEPATH = getTestFile("resource-schema-1.xsd"); } + public static final String ICF_NAMESPACE = "http://openidm.forgerock.com/xml/ns/public/resource/openicf/resource-schema-1.xsd"; + public static final String RI_NAMESPACE = "http://openidm.forgerock.com/xml/ns/public/resource/instances/ef2bc95b-76e0-48e2-86d6-4d4f44d4e4a4"; + + /** + * Writes a store file with one account per name: __UID__ uid-NAME, __NAME__ NAME, lastname + * Last-NAME and __PASSWORD__ secret-NAME. Names of equal length give files of equal size. An + * existing file is rewritten in place, so it keeps its file key. + */ + public static void writeAccounts(File file, String... names) throws IOException { + StringBuilder xml = new StringBuilder(); + xml.append("\n") + .append("\n"); + for (String name : names) { + xml.append(" \n") + .append(" uid-").append(name).append("\n") + .append(" ").append(name).append("\n") + .append(" Last-").append(name).append("\n") + .append(" secret-").append(name).append("\n") + .append(" \n"); + } + xml.append("\n"); + Files.write(file.toPath(), xml.toString().getBytes(StandardCharsets.UTF_8)); + } + + public static void setModified(File file, long millis) throws IOException { + Files.setLastModifiedTime(file.toPath(), FileTime.fromMillis(millis)); + } + + /** The __NAME__ values in the file, in document order, read by a parser of its own. */ + public static List namesInFile(File file) throws Exception { + DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance(); + factory.setNamespaceAware(true); + NodeList names = factory.newDocumentBuilder().parse(file).getElementsByTagNameNS(ICF_NAMESPACE, Name.NAME); + List result = new ArrayList(); + for (int i = 0; i < names.getLength(); i++) { + result.add(names.item(i).getTextContent()); + } + return result; + } + + /** The attributes of a new account with the values {@link #writeAccounts} writes; create assigns its own __UID__. */ + public static Set account(String name) { + Set attributes = new HashSet(); + attributes.add(AttributeBuilder.build(Name.NAME, name)); + attributes.add(AttributeBuilder.buildPassword(("secret-" + name).toCharArray())); + attributes.add(AttributeBuilder.build(ATTR_ACCOUNT_LAST_NAME, "Last-" + name)); + return attributes; + } + public static File getRandomXMLFile() { return getTestFile(UUID.randomUUID().toString() + ".xml"); } From d37978d210dc1f66883f0a6af8a067634a9c95ee Mon Sep 17 00:00:00 2001 From: maximthomas Date: Thu, 8 Oct 2026 15:24:18 +0300 Subject: [PATCH 04/16] [#157] Pin the closing log line of the XML save --- .../connectors/xml/XMLConnectorTests.java | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLConnectorTests.java b/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLConnectorTests.java index 93d27e079..e6ff1495d 100644 --- a/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLConnectorTests.java +++ b/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLConnectorTests.java @@ -322,6 +322,14 @@ public void failedSaveIsLoggedAndThrown() throws Exception { } } + @Test + public void successfulSaveLogsItsExit() throws Exception { + XMLHandlerImpl handler = newHandler(); + handler.init(); // no file yet: a new document in memory + String out = captureStdOut(handler::dispose); + AssertJUnit.assertTrue(out, out.contains("Exit serialize" + System.lineSeparator())); + } + private XMLHandlerImpl newHandler() { XMLConfiguration config = new XMLConfiguration(); config.setXmlFilePath(xmlFile); @@ -331,6 +339,18 @@ private XMLHandlerImpl newHandler() { return new XMLHandlerImpl(config, parser.parseSchema(), parser.getXsdSchema()); } + private static String captureStdOut(Runnable action) throws UnsupportedEncodingException { + PrintStream original = System.out; + ByteArrayOutputStream buffer = new ByteArrayOutputStream(); + System.setOut(new PrintStream(buffer, true, StandardCharsets.UTF_8.name())); + try { + action.run(); + } finally { + System.setOut(original); + } + return buffer.toString(StandardCharsets.UTF_8.name()); + } + private static String captureStdErr(Runnable action) throws UnsupportedEncodingException { PrintStream original = System.err; ByteArrayOutputStream buffer = new ByteArrayOutputStream(); From d5236cab186cd8f31e06cc85a11d867e57381aa9 Mon Sep 17 00:00:00 2001 From: maximthomas Date: Fri, 9 Oct 2026 09:49:25 +0300 Subject: [PATCH 05/16] [#157] Pin the removal of a whole whitespace-only run of text --- .../openicf/connectors/xml/XmlDocumentWriter.java | 6 +++--- .../openicf/connectors/xml/XMLConnectorTests.java | 10 ++++++++++ .../connectors/xml/XmlDocumentWriterTests.java | 13 ++++++++++++- 3 files changed, 25 insertions(+), 4 deletions(-) diff --git a/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/XmlDocumentWriter.java b/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/XmlDocumentWriter.java index 739550ada..412038796 100644 --- a/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/XmlDocumentWriter.java +++ b/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/XmlDocumentWriter.java @@ -61,9 +61,9 @@ private XmlDocumentWriter() { *

* A run is a maximal sequence of adjacent Text and CDATA section siblings, which XPath sees as * one text node. A run that holds only XML whitespace (space, tab, CR, LF: what - * {@code normalize-space} strips) is removed, as {@code //text()[normalize-space(.) = '']} - * removed it before, CDATA included. Any other run becomes one Text node with the same value, - * which is what a reload of the written file gives. + * {@code normalize-space} strips), CDATA included, is removed whole; the XPath used before, + * {@code //text()[normalize-space(.) = '']}, removed only its first node. Any other run + * becomes one Text node with the same value, which is what a reload of the written file gives. */ static boolean normalizeText(Node root) { boolean changed = false; diff --git a/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLConnectorTests.java b/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLConnectorTests.java index e6ff1495d..78fa9aca6 100644 --- a/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLConnectorTests.java +++ b/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLConnectorTests.java @@ -309,6 +309,16 @@ public void whitespaceOnlyValueIsEmptiedBySave() throws Exception { AssertJUnit.assertNull(inFile.item(0).getFirstChild()); } + @Test + public void deletingTheLastEntryLeavesAnEmptyContainer() throws Exception { + // The removed entry leaves two whitespace-only Text nodes side by side; both go. + writeAccounts(xmlFile, "alice"); + facade.delete(ObjectClass.ACCOUNT, new Uid("uid-alice"), null); + DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance(); + factory.setNamespaceAware(true); + AssertJUnit.assertNull(factory.newDocumentBuilder().parse(xmlFile).getDocumentElement().getFirstChild()); + } + @Test public void failedSaveIsLoggedAndThrown() throws Exception { XMLHandlerImpl handler = newHandler(); diff --git a/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XmlDocumentWriterTests.java b/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XmlDocumentWriterTests.java index a65a6a2ba..d7394875c 100644 --- a/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XmlDocumentWriterTests.java +++ b/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XmlDocumentWriterTests.java @@ -103,6 +103,14 @@ public void whitespaceOnlyCdataIsRemoved() throws Exception { assertNull(only(document, "a").getFirstChild()); } + @Test + public void whitespaceOnlyRunOfSeveralNodesIsRemovedWhole() throws Exception { + // The XPath used before #157 removed only the first node of such a run. + Document document = parse(" "); + XmlDocumentWriter.normalizeText(document); + assertNull(only(document, "a").getFirstChild()); + } + @Test public void adjacentTextAndCdataBecomeOneTextNode() throws Exception { Document document = parse(" "); @@ -255,7 +263,10 @@ public void normalizeAndWriteNeverUseNodeLists() throws Exception { assertFalse(XercesNodeLists.used(document)); } - /** What XMLHandlerImpl.dispose() wrote before #157. */ + /** + * What XMLHandlerImpl.dispose() wrote before #157 for a document whose text is already + * normalized: the serializer half only, without the XPath that removed whitespace-only text. + */ private static String before(Document document) throws Exception { Transformer saxon = new net.sf.saxon.TransformerFactoryImpl().newTransformer(); saxon.setOutputProperty(OutputKeys.INDENT, "yes"); From 7b13f6f9c4a964f170cfa4082148f7a7c95c85d0 Mon Sep 17 00:00:00 2001 From: maximthomas Date: Fri, 9 Oct 2026 10:24:42 +0300 Subject: [PATCH 06/16] [#157] Pin where a merged text run goes and that the XML save walks stay linear --- .../openicf/connectors/xml/CountingDom.java | 109 ++++++++++++++++++ .../xml/XmlDocumentWriterTests.java | 35 ++++++ 2 files changed, 144 insertions(+) create mode 100644 OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/CountingDom.java diff --git a/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/CountingDom.java b/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/CountingDom.java new file mode 100644 index 000000000..56847242a --- /dev/null +++ b/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/CountingDom.java @@ -0,0 +1,109 @@ +/* + * The contents of this file are subject to the terms of the Common Development and + * Distribution License (the License). You may not use this file except in compliance with the + * License. + * + * You can obtain a copy of the License at legal/CDDLv1.0.txt. See the License for the + * specific language governing permission and limitations under the License. + * + * When distributing Covered Software, include this CDDL Header Notice in each file and include + * the License file at legal/CDDLv1.0.txt. If applicable, add the following below the CDDL + * Header, with the fields enclosed by brackets [] replaced by your own identifying + * information: "Portions copyright [year] [name of copyright owner]". + * + * Copyright 2026 3A Systems, LLC. + */ + +package org.forgerock.openicf.connectors.xml; + +import java.lang.reflect.InvocationHandler; +import java.lang.reflect.InvocationTargetException; +import java.lang.reflect.Method; +import java.lang.reflect.Modifier; +import java.lang.reflect.Proxy; +import java.util.IdentityHashMap; +import java.util.LinkedHashSet; +import java.util.Map; +import java.util.Set; + +import org.w3c.dom.Document; +import org.w3c.dom.NamedNodeMap; +import org.w3c.dom.Node; +import org.w3c.dom.NodeList; + +/** + * Wraps a DOM in proxies that count the DOM calls made through them. A walk that is linear in the + * size of the document makes a bounded number of calls per node, so the count tells a linear walk + * from a quadratic one without timing it (#157). Each node has one proxy, so identity comparisons + * of nodes still hold, and nodes passed back to the DOM are unwrapped first. + */ +final class CountingDom { + + private final Map proxies = new IdentityHashMap(); + private long calls; + + Document wrap(Document document) { + return (Document) wrap((Object) document); + } + + long calls() { + return calls; + } + + private Object wrap(Object target) { + if (!(target instanceof Node || target instanceof NodeList || target instanceof NamedNodeMap)) { + return target; + } + Object proxy = proxies.get(target); + if (proxy == null) { + proxy = Proxy.newProxyInstance(CountingDom.class.getClassLoader(), domInterfaces(target.getClass()), + new Counted(target)); + proxies.put(target, proxy); + } + return proxy; + } + + private static Object unwrap(Object object) { + if (object != null && Proxy.isProxyClass(object.getClass()) + && Proxy.getInvocationHandler(object) instanceof Counted) { + return ((Counted) Proxy.getInvocationHandler(object)).target; + } + return object; + } + + private static Class[] domInterfaces(Class type) { + Set> interfaces = new LinkedHashSet>(); + for (Class c = type; c != null; c = c.getSuperclass()) { + for (Class i : c.getInterfaces()) { + if (Modifier.isPublic(i.getModifiers()) && i.getName().startsWith("org.w3c.dom.")) { + interfaces.add(i); + } + } + } + return interfaces.toArray(new Class[0]); + } + + private final class Counted implements InvocationHandler { + + private final Object target; + + Counted(Object target) { + this.target = target; + } + + @Override + public Object invoke(Object proxy, Method method, Object[] args) throws Throwable { + calls++; + if (args != null) { + for (int i = 0; i < args.length; i++) { + args[i] = unwrap(args[i]); + } + } + try { + return wrap(method.invoke(target, args)); + } catch (InvocationTargetException e) { + throw e.getCause(); + } + } + } +} diff --git a/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XmlDocumentWriterTests.java b/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XmlDocumentWriterTests.java index d7394875c..e14182221 100644 --- a/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XmlDocumentWriterTests.java +++ b/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XmlDocumentWriterTests.java @@ -125,6 +125,17 @@ public void singleCdataBecomesATextNode() throws Exception { assertSingleText(only(document, "b"), "z"); } + @Test + public void mergedRunKeepsItsPlace() throws Exception { + Document document = parse("a"); + XmlDocumentWriter.normalizeText(document); + Node text = only(document, "x").getFirstChild(); + assertEquals(text.getNodeType(), Node.TEXT_NODE); + assertEquals(text.getNodeValue(), "ab"); + assertEquals(text.getNextSibling().getNodeType(), Node.COMMENT_NODE); + assertNull(text.getNextSibling().getNextSibling()); + } + @Test public void outputIsTheSameAsBefore() throws Exception { Document document = parse("" @@ -263,6 +274,30 @@ public void normalizeAndWriteNeverUseNodeLists() throws Exception { assertFalse(XercesNodeLists.used(document)); } + @Test + public void normalizeAndWriteMakeALinearNumberOfDomCalls() throws Exception { + // The probe above sees only NodeLists; a walk that, say, counts the earlier siblings of + // every child is quadratic without one. Twice the entries take about twice the calls. + long small = domCallsToSave(1000); + long large = domCallsToSave(2000); + assertTrue(large < 3 * small, "1,000 entries: " + small + " DOM calls, 2,000 entries: " + large); + } + + /** The DOM calls that normalizeText and write make on an indented container of {@code entries} entries. */ + private static long domCallsToSave(int entries) throws Exception { + StringBuilder xml = new StringBuilder(""); + for (int i = 0; i < entries; i++) { + xml.append("\n \n a\n "); + } + xml.append("\n"); + CountingDom dom = new CountingDom(); + Document document = dom.wrap(parse(xml.toString())); + XmlDocumentWriter.normalizeText(document); + XmlDocumentWriter.write(document, XmlConnectorTestUtil.getRandomXMLFile()); + return dom.calls(); + } + /** * What XMLHandlerImpl.dispose() wrote before #157 for a document whose text is already * normalized: the serializer half only, without the XPath that removed whitespace-only text. From 7a1a5eb507bdd1cd557bdbce1b0dca9091dc0331 Mon Sep 17 00:00:00 2001 From: maximthomas Date: Fri, 9 Oct 2026 10:24:42 +0300 Subject: [PATCH 07/16] [#157] Say how the XML writer orders namespace declarations written by hand --- .../forgerock/openicf/connectors/xml/XmlDocumentWriter.java | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/XmlDocumentWriter.java b/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/XmlDocumentWriter.java index 412038796..baf33886d 100644 --- a/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/XmlDocumentWriter.java +++ b/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/XmlDocumentWriter.java @@ -46,7 +46,10 @@ *

* Saxon's DOM wrappers walk a DOM through {@code NodeList.item(i)}, which turns quadratic on a * Xerces DOM once the document's node list cache gets into a bad state. Here both steps follow - * sibling pointers, and Saxon's serializer only receives SAX events, so the output is what it was. + * sibling pointers, and Saxon's serializer only receives SAX events. A store the connector created + * is written byte for byte as before. Namespace declarations written by hand come out in the order + * of the element's attribute map (by name in Xerces), and a redeclaration of a binding already in + * scope is kept; the old serializer put the element's own namespace first and dropped those. */ final class XmlDocumentWriter { From 1f97256ebd7ab13e42ccd27540fc2d856defdaba Mon Sep 17 00:00:00 2001 From: maximthomas Date: Fri, 9 Oct 2026 12:05:04 +0300 Subject: [PATCH 08/16] [#157] Leave the text inside entity references alone when normalizing --- .../openicf/connectors/xml/XmlDocumentWriter.java | 3 ++- .../connectors/xml/XmlDocumentWriterTests.java | 14 ++++++++++++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/XmlDocumentWriter.java b/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/XmlDocumentWriter.java index baf33886d..758936666 100644 --- a/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/XmlDocumentWriter.java +++ b/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/XmlDocumentWriter.java @@ -67,13 +67,14 @@ private XmlDocumentWriter() { * {@code normalize-space} strips), CDATA included, is removed whole; the XPath used before, * {@code //text()[normalize-space(.) = '']}, removed only its first node. Any other run * becomes one Text node with the same value, which is what a reload of the written file gives. + * Text inside an entity reference is left alone: the DOM makes it read-only. */ static boolean normalizeText(Node root) { boolean changed = false; Node node = root.getFirstChild(); while (node != null) { if (!isText(node)) { - Node child = node.getFirstChild(); + Node child = node.getNodeType() == Node.ENTITY_REFERENCE_NODE ? null : node.getFirstChild(); node = child != null ? child : following(node, root); continue; } diff --git a/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XmlDocumentWriterTests.java b/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XmlDocumentWriterTests.java index e14182221..c14e5d3de 100644 --- a/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XmlDocumentWriterTests.java +++ b/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XmlDocumentWriterTests.java @@ -136,6 +136,20 @@ public void mergedRunKeepsItsPlace() throws Exception { assertNull(text.getNextSibling().getNextSibling()); } + @Test + public void textInsideAnEntityReferenceIsLeftAlone() throws Exception { + // The children of an entity reference are read-only; Xerces keeps them when references are not expanded. + DocumentBuilderFactory factory = newFactory(); + factory.setExpandEntityReferences(false); + Document document = factory.newDocumentBuilder().parse(new InputSource(new StringReader( + " x'>]>&e;"))); + Node reference = document.getDocumentElement().getFirstChild(); + assertEquals(reference.getNodeType(), Node.ENTITY_REFERENCE_NODE); + XmlDocumentWriter.normalizeText(document); + assertEquals(reference.getFirstChild().getNextSibling().getNodeValue(), " "); + assertEquals(reference.getLastChild().getNodeType(), Node.CDATA_SECTION_NODE); + } + @Test public void outputIsTheSameAsBefore() throws Exception { Document document = parse("" From cbbee6058347e4599c5562953c756066c31fec4e Mon Sep 17 00:00:00 2001 From: maximthomas Date: Fri, 9 Oct 2026 12:05:54 +0300 Subject: [PATCH 09/16] [#157] Write an unprefixed DOM level 1 element in no namespace, as before --- .../connectors/xml/XmlDocumentWriter.java | 12 +++++++----- .../connectors/xml/XmlDocumentWriterTests.java | 17 +++++++++++++++++ 2 files changed, 24 insertions(+), 5 deletions(-) diff --git a/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/XmlDocumentWriter.java b/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/XmlDocumentWriter.java index 758936666..8acdf7fa1 100644 --- a/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/XmlDocumentWriter.java +++ b/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/XmlDocumentWriter.java @@ -198,18 +198,20 @@ private static void sendElement(Node element, TransformerHandler out, NamespaceS /** * Returns the namespace to use for {@code prefix}: the node's own, declared here if it is not - * in scope yet, or, for a DOM level 1 node without one, the namespace in scope. + * in scope yet. A DOM level 1 node has none: with a prefix it takes the namespace in scope, and + * an element without one is in no namespace, as the old serializer wrote it. */ private static String bind(String prefix, String uri, TransformerHandler out, NamespaceSupport namespaces, List declared) throws SAXException { String inScope = namespaces.getURI(prefix); - if (uri == null) { + if (uri == null && !prefix.isEmpty()) { return inScope == null ? "" : inScope; } - if (!uri.equals(inScope == null ? "" : inScope)) { - declare(prefix, uri, out, namespaces, declared); + String own = uri == null ? "" : uri; + if (!own.equals(inScope == null ? "" : inScope)) { + declare(prefix, own, out, namespaces, declared); } - return uri; + return own; } private static void declare(String prefix, String uri, TransformerHandler out, NamespaceSupport namespaces, diff --git a/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XmlDocumentWriterTests.java b/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XmlDocumentWriterTests.java index c14e5d3de..bd3f04acb 100644 --- a/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XmlDocumentWriterTests.java +++ b/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XmlDocumentWriterTests.java @@ -210,6 +210,23 @@ public void prefixedAttributeWithoutANamespaceTakesTheOneInScope() throws Except assertEquals(parseFile(file).getDocumentElement().getAttributeNS(XSI, "schemaLocation"), "urn:a a.xsd"); } + @Test + public void unprefixedDomLevel1ElementIsInNoNamespace() throws Exception { + // As the old serializer wrote it, whether the default namespace in scope is declared by an attribute or not. + for (boolean declared : new boolean[] {false, true}) { + Document document = newFactory().newDocumentBuilder().newDocument(); + Element root = document.createElementNS("urn:x", "a"); + if (declared) { + root.setAttributeNS(XMLNS, "xmlns", "urn:x"); + } + document.appendChild(root); + root.appendChild(document.createElement("b")); + File file = XmlConnectorTestUtil.getRandomXMLFile(); + XmlDocumentWriter.write(document, file); + assertEquals(new String(Files.readAllBytes(file.toPath()), "UTF-8"), before(document), "declared: " + declared); + } + } + @Test public void defaultNamespaceUndeclarationIsKept() throws Exception { assertReadsBackTheSame(parse("")); From b3b47926dd197f4bd772d6716f85c25445ffbe39 Mon Sep 17 00:00:00 2001 From: maximthomas Date: Fri, 9 Oct 2026 12:07:01 +0300 Subject: [PATCH 10/16] [#157] Drop the unused result of normalizeText --- .../openicf/connectors/xml/XmlDocumentWriter.java | 8 ++------ .../openicf/connectors/xml/XmlDocumentWriterTests.java | 8 +++++--- 2 files changed, 7 insertions(+), 9 deletions(-) diff --git a/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/XmlDocumentWriter.java b/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/XmlDocumentWriter.java index 8acdf7fa1..d15d9959e 100644 --- a/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/XmlDocumentWriter.java +++ b/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/XmlDocumentWriter.java @@ -59,8 +59,7 @@ private XmlDocumentWriter() { } /** - * Removes whitespace-only text and turns every other run of text into a single Text node; - * returns whether anything changed. + * Removes whitespace-only text and turns every other run of text into a single Text node. *

* A run is a maximal sequence of adjacent Text and CDATA section siblings, which XPath sees as * one text node. A run that holds only XML whitespace (space, tab, CR, LF: what @@ -69,8 +68,7 @@ private XmlDocumentWriter() { * becomes one Text node with the same value, which is what a reload of the written file gives. * Text inside an entity reference is left alone: the DOM makes it read-only. */ - static boolean normalizeText(Node root) { - boolean changed = false; + static void normalizeText(Node root) { Node node = root.getFirstChild(); while (node != null) { if (!isText(node)) { @@ -95,11 +93,9 @@ static boolean normalizeText(Node root) { for (Node n : run) { parent.removeChild(n); } - changed = true; } node = after != null ? after : following(parent, root); } - return changed; } /** diff --git a/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XmlDocumentWriterTests.java b/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XmlDocumentWriterTests.java index bd3f04acb..6897ad5bd 100644 --- a/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XmlDocumentWriterTests.java +++ b/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XmlDocumentWriterTests.java @@ -73,18 +73,20 @@ public void followingStopsAtRoot() throws Exception { @Test public void normalizedDocumentIsLeftAlone() throws Exception { Document document = parse("x"); - assertFalse(XmlDocumentWriter.normalizeText(document)); + Node text = only(document, "a").getFirstChild(); + XmlDocumentWriter.normalizeText(document); + assertSingleText(only(document, "a"), "x"); + assertSame(only(document, "a").getFirstChild(), text); } @Test public void whitespaceOnlyTextIsRemoved() throws Exception { Document document = parse("\n \t \n \n x\n"); - boolean changed = XmlDocumentWriter.normalizeText(document); + XmlDocumentWriter.normalizeText(document); assertNull(only(document, "a").getFirstChild()); for (Node n = document.getDocumentElement().getFirstChild(); n != null; n = n.getNextSibling()) { assertEquals(n.getNodeType(), Node.ELEMENT_NODE); } - assertTrue(changed); } @Test From f238a5a3591bfaf51c4fbfcf23687196304c6789 Mon Sep 17 00:00:00 2001 From: maximthomas Date: Fri, 9 Oct 2026 12:09:48 +0300 Subject: [PATCH 11/16] [#157] Check a lone Text node in place, without a list or a buffer --- .../connectors/xml/XmlDocumentWriter.java | 25 +++++++++++-------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/XmlDocumentWriter.java b/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/XmlDocumentWriter.java index d15d9959e..ab93590c6 100644 --- a/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/XmlDocumentWriter.java +++ b/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/XmlDocumentWriter.java @@ -77,17 +77,20 @@ static void normalizeText(Node root) { continue; } Node parent = node.getParentNode(); - List run = new ArrayList(); - StringBuilder value = new StringBuilder(); - Node after = node; - while (after != null && isText(after)) { - run.add(after); - value.append(after.getNodeValue()); - after = after.getNextSibling(); - } - boolean blank = isXmlWhitespace(value); - if (blank || run.size() > 1 || node.getNodeType() != Node.TEXT_NODE) { - if (!blank) { + Node after = node.getNextSibling(); + if (node.getNodeType() == Node.TEXT_NODE && (after == null || !isText(after))) { + // A lone Text node, the usual case: nothing to merge. + if (isXmlWhitespace(node.getNodeValue())) { + parent.removeChild(node); + } + } else { + List run = new ArrayList(); + StringBuilder value = new StringBuilder(); + for (after = node; after != null && isText(after); after = after.getNextSibling()) { + run.add(after); + value.append(after.getNodeValue()); + } + if (!isXmlWhitespace(value)) { parent.insertBefore(parent.getOwnerDocument().createTextNode(value.toString()), node); } for (Node n : run) { From e851b4ab7fd597e740ed33fb7a5152c8a09c415f Mon Sep 17 00:00:00 2001 From: maximthomas Date: Fri, 9 Oct 2026 12:10:40 +0300 Subject: [PATCH 12/16] [#157] Save through getDocument() without the document monitor --- .../forgerock/openicf/connectors/xml/XMLHandlerImpl.java | 9 ++++----- .../openicf/connectors/xml/XMLConnectorTests.java | 7 +++++++ 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/XMLHandlerImpl.java b/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/XMLHandlerImpl.java index 8b58ceb8e..dfd5e6083 100644 --- a/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/XMLHandlerImpl.java +++ b/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/XMLHandlerImpl.java @@ -383,12 +383,11 @@ public void dispose() { log.error("UPDATE COLLUSION: File has been modified after read into memory and the data in memory has not been synced before."); } + // Callers hold the handler's write lock. + Document saved = getDocument(); try { - // Callers hold the handler's write lock; the document monitor stays as before. - synchronized (document) { - XmlDocumentWriter.normalizeText(document); - XmlDocumentWriter.write(document, config.getXmlFilePath()); - } + XmlDocumentWriter.normalizeText(saved); + XmlDocumentWriter.write(saved, config.getXmlFilePath()); log.info("Saving changes to xml file"); } catch (TransformerException | SAXException | IOException ex) { log.error("Failed saving changes to xml file: {0}", ex); diff --git a/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLConnectorTests.java b/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLConnectorTests.java index 78fa9aca6..a134130b3 100644 --- a/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLConnectorTests.java +++ b/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLConnectorTests.java @@ -332,6 +332,13 @@ public void failedSaveIsLoggedAndThrown() throws Exception { } } + @Test + public void saveWithoutADocumentThrowsConnectorException() { + XMLHandlerImpl handler = newHandler(); // init() never ran, as when it fails on the first load + ConnectorException e = expectThrows(ConnectorException.class, handler::dispose); + AssertJUnit.assertTrue(e.getMessage(), e.getMessage().startsWith("Data file does not exists: ")); + } + @Test public void successfulSaveLogsItsExit() throws Exception { XMLHandlerImpl handler = newHandler(); From bd2fe56f2bd1da980503cbb85d0a874008fb0cb2 Mon Sep 17 00:00:00 2001 From: maximthomas Date: Fri, 9 Oct 2026 12:11:49 +0300 Subject: [PATCH 13/16] [#157] End following() at the top of the tree when root is not an ancestor --- .../openicf/connectors/xml/util/XmlHandlerUtil.java | 6 +++--- .../openicf/connectors/xml/XmlDocumentWriterTests.java | 6 ++++++ 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/util/XmlHandlerUtil.java b/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/util/XmlHandlerUtil.java index ac58ac427..3a4e81124 100644 --- a/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/util/XmlHandlerUtil.java +++ b/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/util/XmlHandlerUtil.java @@ -84,11 +84,11 @@ public static void checkObjectType(ObjectClass objClass, XSSchema schema) { /** * Returns the node that follows {@code node} and its descendants in document order without * leaving {@code root}, or {@code null} at the end. {@code root} is {@code node}, one of its - * ancestors, or {@code null} for the whole tree. Uses sibling and parent pointers only, so a - * walk built on it stays linear on a Xerces DOM. + * ancestors, or {@code null} for the whole tree; any other {@code root} acts as {@code null}. + * Uses sibling and parent pointers only, so a walk built on it stays linear on a Xerces DOM. */ public static Node following(Node node, Node root) { - for (Node n = node; n != root; n = n.getParentNode()) { + for (Node n = node; n != null && n != root; n = n.getParentNode()) { Node sibling = n.getNextSibling(); if (sibling != null) { return sibling; diff --git a/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XmlDocumentWriterTests.java b/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XmlDocumentWriterTests.java index 6897ad5bd..27bf3294c 100644 --- a/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XmlDocumentWriterTests.java +++ b/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XmlDocumentWriterTests.java @@ -70,6 +70,12 @@ public void followingStopsAtRoot() throws Exception { assertNull(following(only(document, "x"), only(document, "a"))); } + @Test + public void followingWithARootOutsideTheAncestorsEndsAtTheTop() throws Exception { + Document document = parse(""); + assertNull(following(only(document, "b"), only(document, "a"))); + } + @Test public void normalizedDocumentIsLeftAlone() throws Exception { Document document = parse("x"); From b3fba18cffad9fcfe2c5329bfc2edc8219c24ac5 Mon Sep 17 00:00:00 2001 From: maximthomas Date: Fri, 9 Oct 2026 19:02:04 +0300 Subject: [PATCH 14/16] [#157] Declare a namespace prefix once per element --- .../connectors/xml/XmlDocumentWriter.java | 17 +++++++-- .../xml/XmlDocumentWriterTests.java | 37 +++++++++++++++++++ 2 files changed, 51 insertions(+), 3 deletions(-) diff --git a/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/XmlDocumentWriter.java b/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/XmlDocumentWriter.java index ab93590c6..c16922719 100644 --- a/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/XmlDocumentWriter.java +++ b/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/XmlDocumentWriter.java @@ -197,13 +197,16 @@ private static void sendElement(Node element, TransformerHandler out, NamespaceS /** * Returns the namespace to use for {@code prefix}: the node's own, declared here if it is not - * in scope yet. A DOM level 1 node has none: with a prefix it takes the namespace in scope, and - * an element without one is in no namespace, as the old serializer wrote it. + * in scope yet. A prefix the element already declares, by an attribute or for its name, keeps + * that namespace; the old serializer did the same for the default namespace, and for any other + * prefix let the node's own namespace win. A DOM level 1 node has + * none: with a prefix it takes the namespace in scope, and an element without one is in no + * namespace, as the old serializer wrote it. */ private static String bind(String prefix, String uri, TransformerHandler out, NamespaceSupport namespaces, List declared) throws SAXException { String inScope = namespaces.getURI(prefix); - if (uri == null && !prefix.isEmpty()) { + if (declared.contains(prefix) || (uri == null && !prefix.isEmpty())) { return inScope == null ? "" : inScope; } String own = uri == null ? "" : uri; @@ -213,8 +216,16 @@ private static String bind(String prefix, String uri, TransformerHandler out, Na return own; } + /** + * Declares {@code prefix} on the current element unless it is declared there already: a + * second declaration of a prefix on one element makes the file unreadable. A DOM can hold two + * {@code xmlns} attributes, one set with {@code setAttribute} and one with {@code setAttributeNS}. + */ private static void declare(String prefix, String uri, TransformerHandler out, NamespaceSupport namespaces, List declared) throws SAXException { + if (declared.contains(prefix)) { + return; + } namespaces.declarePrefix(prefix, uri); out.startPrefixMapping(prefix, uri); declared.add(prefix); diff --git a/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XmlDocumentWriterTests.java b/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XmlDocumentWriterTests.java index 27bf3294c..3e39b4758 100644 --- a/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XmlDocumentWriterTests.java +++ b/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XmlDocumentWriterTests.java @@ -235,6 +235,43 @@ public void unprefixedDomLevel1ElementIsInNoNamespace() throws Exception { } } + @Test + public void aPrefixIsDeclaredOncePerElementAsBefore() throws Exception { + // Each child binds one prefix to two namespaces: by an xmlns attribute, by its own name or by + // a prefixed attribute. Declaring both made the file unreadable. The connector builds no such DOM. + Document document = newFactory().newDocumentBuilder().newDocument(); + Element root = document.createElementNS("urn:x", "a"); + document.appendChild(root); + Element bySetAttribute = document.createElement("b"); + bySetAttribute.setAttribute("xmlns", "urn:y"); + Element bySetAttributeNS = document.createElement("b"); + bySetAttributeNS.setAttributeNS(XMLNS, "xmlns", "urn:y"); + Element namespaced = document.createElementNS("urn:x", "b"); + namespaced.setAttributeNS(XMLNS, "xmlns", "urn:y"); + Element prefixed = document.createElementNS("urn:x", "p:b"); + prefixed.setAttributeNS("urn:y", "p:c", "v"); + for (Element child : new Element[] {bySetAttribute, bySetAttributeNS, namespaced, prefixed}) { + root.appendChild(child); + } + File file = XmlConnectorTestUtil.getRandomXMLFile(); + XmlDocumentWriter.write(document, file); + assertEquals(new String(Files.readAllBytes(file.toPath()), "UTF-8"), before(document)); + } + + @Test + public void twoXmlnsAttributesDeclareTheDefaultNamespaceOnce() throws Exception { + // setAttributeNS does not find the attribute set with setAttribute, so Xerces keeps both and + // puts the new one first. The first declaration wins. + Document document = parse(""); + Element root = document.getDocumentElement(); + root.setAttribute("xmlns", "urn:y"); + root.setAttributeNS(XMLNS, "xmlns", "urn:z"); + assertEquals(root.getAttributes().getLength(), 2); + File file = XmlConnectorTestUtil.getRandomXMLFile(); + XmlDocumentWriter.write(document, file); + assertEquals(parseFile(file).getDocumentElement().getNamespaceURI(), "urn:z"); + } + @Test public void defaultNamespaceUndeclarationIsKept() throws Exception { assertReadsBackTheSame(parse("")); From faf5d158e98a6ecdd8c4c6863c6d74216e19a6c3 Mon Sep 17 00:00:00 2001 From: maximthomas Date: Fri, 9 Oct 2026 19:02:05 +0300 Subject: [PATCH 15/16] [#157] Check that normalizeText walks on after an entity reference --- .../connectors/xml/XmlDocumentWriterTests.java | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XmlDocumentWriterTests.java b/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XmlDocumentWriterTests.java index 3e39b4758..dc43436bf 100644 --- a/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XmlDocumentWriterTests.java +++ b/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XmlDocumentWriterTests.java @@ -158,6 +158,19 @@ public void textInsideAnEntityReferenceIsLeftAlone() throws Exception { assertEquals(reference.getLastChild().getNodeType(), Node.CDATA_SECTION_NODE); } + @Test + public void textAfterAnEntityReferenceIsNormalized() throws Exception { + DocumentBuilderFactory factory = newFactory(); + factory.setExpandEntityReferences(false); + Document document = factory.newDocumentBuilder().parse(new InputSource(new StringReader( + "]>&e; "))); + XmlDocumentWriter.normalizeText(document); + Node reference = document.getDocumentElement().getFirstChild(); + assertEquals(reference.getNodeType(), Node.ENTITY_REFERENCE_NODE); + assertSame(reference.getNextSibling(), only(document, "c")); + assertNull(only(document, "c").getFirstChild()); + } + @Test public void outputIsTheSameAsBefore() throws Exception { Document document = parse("" From d4fe809cf2defe9c9d5e3c39d15b229cecd9ce8c Mon Sep 17 00:00:00 2001 From: maximthomas Date: Sun, 11 Oct 2026 12:59:32 +0300 Subject: [PATCH 16/16] [#157] Rewrap bind's Javadoc --- .../forgerock/openicf/connectors/xml/XmlDocumentWriter.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/XmlDocumentWriter.java b/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/XmlDocumentWriter.java index c16922719..61c1e3d9b 100644 --- a/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/XmlDocumentWriter.java +++ b/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/XmlDocumentWriter.java @@ -199,9 +199,9 @@ private static void sendElement(Node element, TransformerHandler out, NamespaceS * Returns the namespace to use for {@code prefix}: the node's own, declared here if it is not * in scope yet. A prefix the element already declares, by an attribute or for its name, keeps * that namespace; the old serializer did the same for the default namespace, and for any other - * prefix let the node's own namespace win. A DOM level 1 node has - * none: with a prefix it takes the namespace in scope, and an element without one is in no - * namespace, as the old serializer wrote it. + * prefix let the node's own namespace win. A DOM level 1 node has none: with a prefix it takes + * the namespace in scope, and an element without one is in no namespace, as the old serializer + * wrote it. */ private static String bind(String prefix, String uri, TransformerHandler out, NamespaceSupport namespaces, List declared) throws SAXException {