[#157] Save the XML document in linear time

[maximthomas]

Part of #157

Merge order

This is the first of two PRs for part 1 of #157; the second, #177 ("[#157] Load and save the XML file only when it has changed"), is built on top of this branch. Please merge them one after the other: this PR first, then #177. After this PR is merged, #177 will be rebased onto the new master.

Problem

XMLConnector is not poolable, so an operation that does not overlap with another one on the same file ends in XMLHandlerImpl.dispose(), which writes the whole document back. That save is quadratic in the number of entries. Measured with 40,000 entries, dispose() alone takes about 7.8 s (see the table below), and in the issue a search for one entry by __NAME__ through ConnectorFacade took 13.5 s at 40,000 entries.

The save walks the Xerces DOM twice through Saxon: the XPath //text()[normalize-space(.) = ''] that removes whitespace-only text, and DOMSender when TransformerFactoryImpl serializes a DOMSource. Both go through NodeList.item(i). On Xerces 2.6.2 the document's NodeListCache free list can become a self-cycle (freeNodeListCache pushes a cache that is already on the list), after which every item(i) on the container restarts from its first child, and a walk over N children costs O(N^2).

This PR fixes the save only. Parsing, the lookups (XQuery) and the decision when to read and write the file are not changed here, so a full operation is still slower than linear. The save still truncates the file before it writes it, as before; making it atomic is #160.

Change

• New XmlDocumentWriter (package-private).
  • normalizeText(Node) removes whitespace-only text and merges every other run of adjacent Text and CDATA siblings into one Text node. Whitespace means space, tab, CR and LF, which is what normalize-space strips; CDATA counts as text, as it did for the XPath. A lone Text node, the usual case, is checked in place. Text inside an entity reference is left alone, because the DOM makes it read-only; only a DOM parsed with entity references not expanded has such text, and the connector expands them.
  • write(Document, File) walks the DOM with sibling pointers and sends SAX events to the same Saxon serializer (TransformerHandler, same output properties as before). It returns the CRC-32 of the bytes written, which this PR does not use: [#157] Load and save the XML file only when it has changed #177 compares it with the file's content when the file's timestamp is too recent to tell whether the file has changed.
• Both walks follow sibling pointers, so neither touches a NodeList; normalizeText steps with the new XmlHandlerUtil.following(node, root) (the next sibling, or the next sibling of the nearest ancestor below root; a root that is not an ancestor acts as null).
• XMLHandlerImpl.dispose() calls normalizeText and write instead of the XPath and the DOMSource transform. It takes the document through getDocument() and no longer locks the document's monitor: every caller holds ConcurrentXMLHandler's write lock, and nothing else locks on the document. A handler without a document now fails with getDocument()'s ConnectorException instead of a NullPointerException; the connector does not reach that case, because a failed init() is never followed by the handler's dispose(). A failed save is still an ERROR log line, then ConnectorException. Two cases change: an IOException from closing the file was logged as WARN and the save counted as successful, and is now a failed save (A1 row 21), and the XPathExpressionException that the old code ignored is gone with the XPath. The old closing log.info("Entry {0}", method) now says Exit.
• createDocument() sets xmlns:icf on the root before xmlns:xsi, so a new file declares icf, ri, xsi in the same order as before.
• The file format does not change for a store the connector created: same indentation (the Saxon serializer is the same), same namespace declarations in the same order. An unprefixed element created with DOM level 1 createElement is written in no namespace, as before, unless it has an xmlns attribute of its own (the connector creates its elements with createElementNS). The tests compare write's output byte by byte with the old DOMSource output of the same document. A hand-edited store can come out with its namespace declarations rearranged: write declares an element's xmlns attributes in the order of its attribute map (Xerces sorts them by name) and keeps a redeclaration of a binding already in scope, where the old serializer put the element's own namespace first and dropped such redeclarations. The namespaces of every element and attribute stay the same. A DOM built in code can bind one prefix to two namespaces on one element: an xmlns attribute that contradicts the namespace of the element or of one of its attributes, or two xmlns attributes, one set with setAttribute and one with setAttributeNS. write declares such a prefix once, and the first binding wins: the element's xmlns attributes, then its name, then its attributes. The old serializer did the same for the default namespace; for any other prefix it let the node's namespace win over the element's xmlns:p attribute. When the namespace chosen for a prefix gives an attribute the expanded name of another attribute of the same element, the file is not well-formed. With xmlns:p="urn:x" on an element, its attributes p:c in urn:y and q:c in urn:x are both written as c in urn:x; the old serializer declared p as urn:y there and wrote a readable file. When the clash comes from the element's own prefix (p:b in urn:x with the same two attributes), the old serializer writes the same unreadable file as write, byte for byte.
• Whitespace-only text is removed more thoroughly than before. Saxon's XPath sees a run of adjacent Text and CDATA siblings as one text node but returns only the first DOM node of the run, so the old //text()[normalize-space(.) = ''] removed that node and left the rest; normalizeText removes the whole run. Such runs appear when elements between two indentation nodes are removed, so a saved file can differ from the old output in whitespace only:
  • deleting the last entry writes an empty <icf:OpenICFContainer .../>, not one that holds a line break;
  • updating a multi-valued attribute that held several values no longer leaves a line of spaces in the entry;
  • a hand-edited value such as <ri:firstname> <![CDATA[ ]]> </ri:firstname> is emptied by the first save, not the second.

Why the writer feeds Saxon and does not use XSLTC

The connector bundle embeds xml-apis-1.3.04.jar, and the connector server loads bundles with a child-first BundleClassLoader. Inside the bundle javax.xml.transform.TransformerFactory and org.w3c.dom.* therefore come from xml-apis 1.3.04. That copy has no TransformerFactory.newDefaultInstance(), and the JDK's XSLTC transformer cannot take the bundle's DOM classes. Surefire cannot see this, because there the JDK classes win. The writer therefore creates net.sf.saxon.TransformerFactoryImpl directly.

A new integration test, XMLConnectorBundleIT, loads the packaged bundle jar with the same child-first loader and runs a save, a reload and a search through it. It is wired into the module with maven-failsafe-plugin (**/*BundleIT.java, system property bundleJar). To check that the IT is not vacuous, write was switched to (SAXTransformerFactory) javax.xml.transform.TransformerFactory.newDefaultInstance(): the IT then fails with NoSuchMethodError.

Measurements

dispose() time, one XMLHandlerImpl per run: init() parses the generated file, then dispose() is timed (normalize + serialize + write). Three runs per size in one JVM, so the first run of each size includes warm-up. Entries are generated by the issue's harness (XmlBreakdown, Gen): ri:__ACCOUNT__ entries with __UID__, __NAME__ and a few more fields. Master is 3348526, "after" is this branch at dca4047. The later commits add tests and Javadoc, and the review fixes in A6 and A7, of which only the lone-Text shortcut changes the cost of a save noticeably: in a separate probe at 40,000 entries, a normalizeText pass over a freshly parsed store took 23-47 ms and 0.1 MB instead of 45-231 ms and 49 MB. JDK 26 (default JDK of the dev machine), Saxon-HE 9.4.0.7, Xerces 2.6.2. All numbers come from one developer machine (macOS) and were run once, so read them as orders of magnitude.

Entries	dispose() before, median (min-max of 3 runs)	dispose() after, median (min-max of 3 runs)
10,000	1,889 ms (1,150-2,127)	159 ms (85-271)
20,000	2,192 ms (2,192-2,270)	187 ms (171-387)
40,000	7,781 ms (7,647-8,377)	345 ms (323-403)

At 40,000 entries dispose() is about 22 times faster. The 10,000 and 20,000 rows are noisy (the first run of each size is slower); the 40,000 row is the clearest. Parse time is not changed by this PR (45-380 ms in both runs).

Tests

• OpenICF-xml-connector, mvn -o -pl OpenICF-xml-connector verify: surefire 142 run, 0 failed; failsafe 1 run, 0 failed (XMLConnectorBundleIT).
• XmlDocumentWriterTests (new) covers following, normalizeText, and write, which is compared with the old DOMSource output (the serializer half; the old XPath is not part of that comparison) and re-read for namespaces (DOM level 1 nodes included), CDATA, comments, processing instructions, entity references and a prefix bound to two namespaces on one element; text inside an unexpanded entity reference is left alone, and the text after it is normalized.
• XMLConnectorTests gains cases for dispose(): it allocates no Xerces node list caches (XercesNodeLists, test helper), whitespace-only values are saved as empty elements, deleting the last entry leaves an empty container, a failed save is logged and thrown, a successful save logs Exit serialize, a save without a document throws ConnectorException, and a new file declares its namespaces in the old order.
• XmlConnectorTestUtil gains store-file helpers (writeAccounts, namesInFile, account) for XMLConnectorTests and XMLConnectorBundleIT. It also gains setModified(File, long), which has no caller here: its callers come with [#157] Load and save the XML file only when it has changed #177.
• Four tests map to no arm row: valuesSurviveASaveAndAReload (a characterization of what Saxon wrote; green on the old dispose() too); commentsSplitTextRuns (kills "isText counts comments", a mutant no row's tree contains); normalizeAndWriteNeverUseNodeLists (the purpose of the writer at unit level: kills a NodeList.item(i) walk in sendElement; green from its first run, because every row walks by sibling pointers); normalizeAndWriteMakeALinearNumberOfDomCalls (counts the DOM calls of normalizeText + write through CountingDom, test helper: 63,035 for 1,000 entries, 126,035 for 2,000; kills a walk that counts the earlier siblings of each child, in sendElement or in normalizeText, which takes about 3.8 times the calls for twice the entries and which normalizeAndWriteNeverUseNodeLists misses; green from its first run for the same reason).

Test strength

Each row is an arm of the production code, the case that reaches it, what only that arm produces, and a mutant that only that case kills. "red at" is the commit where the case was red before its arm was written (A1: on the base 33485269; A2: row 1 on 6f83fa3e, the writer without the dispose() change, and rows 2-4 on 6f83fa3e plus row 1's dispose(), which only calls write and rethrows, because the old dispose() already does what they assert; A3: on 4fbcc475, which had no failsafe execution; A4 and A5: on the mutants named in their rows, because their arms were already written in A1 and A2; A6: on the commit before each arm; A7: on bd2fe56f, or on the mutant named in the row where the arm was already there). Rows marked unpinnable are arms that no test can tell from their absence; the reason is given as found.

A1: XmlDocumentWriter, following (6f83fa3)

#	arm (file:line — function — condition)	case	observable	mutant	red at
1	XmlHandlerUtil.java:92-94 — following — sibling != null: return the next sibling	followingIsTheNextSibling	following(a, document) is b, not a's child x	following not defined	33485269
2	XmlHandlerUtil.java:91 — following — no sibling: n = n.getParentNode(), try again	followingClimbsToTheNextSiblingOfAnAncestor	following(x, document) is b although x has no sibling	body return node.getNextSibling(); (no climb)	33485269
3	XmlHandlerUtil.java:91 — following — loop bound n != root	followingStopsAtRoot	following(x, a) is null although a has the sibling b	bound n != null (the walk leaves root)	33485269
4	XmlDocumentWriter.java:81-85 — normalizeText — keep side: a lone non-blank Text node stays where it is	normalizedDocumentIsLeftAlone	a's Text node in <r><a>x</a><b/></r> is the same object after the call	normalizeText not defined (afterwards: every lone Text node replaced by a new one; the other 138 tests stay green on it)	33485269
5	XmlDocumentWriter.java:74-100 — normalizeText — a blank lone Text node is removed (:83-84); the walk: descend, following(node, root) after a leaf, after, following(parent, root) after a run that ends its parent	whitespaceOnlyTextIsRemoved	a empty, every child of r an element (also after the empty c)	empty body (also killed here: the walk stops after an empty element, the walk stops after a last run — verified)	33485269
6	XmlDocumentWriter.java:248-256 — normalizeText — isXmlWhitespace: only space, tab, CR, LF make a run blank	paddedAndNonXmlSpaceValuesAreKept	the U+3000 value is kept	isXmlWhitespace = value.toString().isBlank() (Character.isWhitespace)	33485269, re-run after review
7	XmlDocumentWriter.java:243-246 — normalizeText — isText: CDATA_SECTION_NODE is text	whitespaceOnlyCdataIsRemoved	the whitespace-only CDATA section is removed	isText = TEXT_NODE only	33485269
8	XmlDocumentWriter.java:81,86-98 — normalizeText — a Text node with a text sibling after it starts a run: one Text node with the joined value (if (!isXmlWhitespace(value)) insert)	adjacentTextAndCdataBecomeOneTextNode	a holds one Text node " x<y "	the lone-Text condition without (after == null || !isText(after)) (A5 row 1 also fails on it — verified)	33485269
9	XmlDocumentWriter.java:81 — normalizeText — node.getNodeType() == Node.TEXT_NODE: a lone non-blank CDATA section is not a lone Text node and becomes a Text node	singleCdataBecomesATextNode	b's only child is a TEXT_NODE "z"	the lone-Text condition without node.getNodeType() == Node.TEXT_NODE (verified)	33485269
10	XmlDocumentWriter.java:111-130 — write — the road every document takes: Saxon TransformerHandler with INDENT=yes, startDocument, the document's children, endDocument; send: ELEMENT_NODE, TEXT_NODE; sendElement: xmlns:p attributes declared, element and attributes named through bind	outputIsTheSameAsBefore	file bytes equal the old DOMSource output	write not defined (also killed here: INDENT dropped, xmlns:p not declared — verified)	33485269
11	XmlDocumentWriter.java:119-129 — write — CheckedOutputStream: returns the CRC-32 of the bytes written	writeReturnsTheChecksumOfTheFile	return value = CRC-32 of the file	return 0L;	33485269
12	XmlDocumentWriter.java:162,206-217 — sendElement/bind/declare — namespace scope: pushContext, declarePrefix, getURI, !uri.equals(inScope) → declare, popContext	siblingsDeclareTheirOwnNamespaces	p:a and p:b read back in urn:1	bind returns the node's namespace without declaring it (also killed here: popContext dropped — verified)	33485269
13	XmlDocumentWriter.java:138 — send — case Node.CDATA_SECTION_NODE	cdataIsWrittenAsText	r reads back with the text x<y	the label dropped (CDATA falls to default)	33485269
14	XmlDocumentWriter.java:149-153 — send — case Node.ENTITY_REFERENCE_NODE: the children are sent	entityReferencesAndDoctypeAreWrittenAsBefore	output <r>v<x/>…</r>, equal to the old output	the case dropped (<r/>)	33485269
15	XmlDocumentWriter.java:209-210 — bind — uri == null with a prefix: the namespace in scope (prefixed DOM level 1 nodes such as createDocument()'s xsi:schemaLocation)	prefixedAttributeWithoutANamespaceTakesTheOneInScope	xsi:schemaLocation reads back in the XSI namespace	return ""; for that branch	33485269
16	XmlDocumentWriter.java:168-169 — sendElement — XMLNS.equals(name): the default namespace declaration, xmlns="" included	defaultNamespaceUndeclarationIsKept	a reads back in no namespace	the first loop declares only xmlns:p	33485269
17	XmlDocumentWriter.java:182-183 — sendElement — prefix.isEmpty() ? "": an unprefixed attribute is in no namespace and declares nothing	namespacedAttributeWithoutAPrefixDeclaresNoDefaultNamespace	r reads back in no namespace	every attribute named through bind (declares xmlns="urn:x" on r)	33485269
18	XmlDocumentWriter.java:142-144 — send — case Node.COMMENT_NODE	createdDocumentReadsBackTheSame	the comment " c " reads back	the comment dropped	33485269
19	XmlDocumentWriter.java:146-148 — send — case Node.PROCESSING_INSTRUCTION_NODE	handEditedDocumentReadsBackTheSame	<?pi data?> reads back	the PI dropped	33485269
20	XmlDocumentWriter.java:116-118 — write — METHOD xml, ENCODING UTF-8, {http://xml.apache.org/xslt}indent-amount	—	unpinnable by construction: Saxon 9.4's serializer defaults to method xml and UTF-8 and ignores the Xalan indent-amount key, so deleting any of the three lines leaves every output byte the same (verified); kept as "the output settings used before"	—	—
21	XmlDocumentWriter.java:120 — write — try-with-resources closes the stream; an IOException from close() is a failed save	—	unpinnable by construction: Saxon has flushed every byte when endDocument returns, so an unclosed FileOutputStream leaves the same file (verified); the leak shows only as an open descriptor, and a test cannot make close() fail	—	—
22	XmlDocumentWriter.java:192-194 — sendElement — endPrefixMapping for each declared prefix	—	unpinnable by construction: Saxon 9.4's TransformerHandler ignores endPrefixMapping; dropping the loop leaves the output the same (verified); kept for the SAX contract	—	—
23	XmlDocumentWriter.java:180 — sendElement — the second loop skips xmlns and xmlns:* attributes	—	unpinnable by construction: Saxon 9.4's TransformerHandler drops xmlns attributes passed in startElement's Attributes; passing them changes nothing (verified); kept for the SAX contract	—	—
24	XmlDocumentWriter.java:154-156 — send — default: break; (document types and the like are not written)	—	unpinnable by construction: a no-op; deleting the label is an equivalent mutant (a switch without default does the same). Row 14's exact comparison with the old output already shows "no DOCTYPE"	—	—

A2: dispose() uses the writer (4fbcc47, row 5's test in d37978d)

#	arm (file:line — function — condition)	case	observable	mutant	red at
1	XMLHandlerImpl.java:390 — dispose — the save goes through XmlDocumentWriter.write instead of Saxon's XPath //text() and DOMSender	disposeNeverUsesNodeLists	after init() + dispose() of a loaded file, XercesNodeLists.used(getDocument()) is false	the old dispose() body (both Saxon walks go through NodeList.item(i))	6f83fa3e
2	XMLHandlerImpl.java:389 — dispose — XmlDocumentWriter.normalizeText(document) before write	whitespaceOnlyValueIsEmptiedBySave	firstname " \t " is an empty element in the file, as before #157	write without normalizeText	6f83fa3e + row 1's write-only dispose()
3	XMLHandlerImpl.java:392-395 — dispose — catch (TransformerException | SAXException | IOException ex): ERROR line, then ConnectorException	failedSaveIsLoggedAndThrown	System.err has Failed saving changes to xml file: java.io.FileNotFoundException, and dispose() throws ConnectorException	the catch without log.error (row 1's catch only rethrows; the compiler requires it from row 1 on)	6f83fa3e + row 1's write-only dispose()
4	XMLHandlerImpl.java:475 — createDocument — xmlns:icf set on the root before xmlns:xsi	newFileDeclaresNamespacesAsBefore	the new file declares icf, ri, xsi in that order	no xmlns:icf attribute (the writer then declares icf last: ri, xsi, icf)	6f83fa3e + row 1's write-only dispose()
5	XMLHandlerImpl.java:397 — dispose — the closing log line of a successful save says Exit {0}	successfulSaveLogsItsExit	System.out has a line ending in Exit serialize	the old text log.info("Entry {0}", method)	dca40471

A3: IT on the packaged bundle (dca4047)

#	arm (file:line — function — condition)	case	observable	mutant	red at
1	OpenICF-xml-connector/pom.xml:124-144 — maven-failsafe-plugin execution (integration-test, verify; include **/*BundleIT.java; system property bundleJar)	XMLConnectorBundleIT#bundleSavesLoadsAndFindsEntries	failsafe reports Tests run: 1 for XMLConnectorBundleIT	no failsafe execution in the module (the IT compiles and never runs)	4fbcc475
2	XmlDocumentWriter.java:112 — write — Saxon's factory created directly (new net.sf.saxon.TransformerFactoryImpl(), A1's code), which only the bundle's child-first loader can tell apart	XMLConnectorBundleIT#bundleSavesLoadsAndFindsEntries	the IT passes inside the bundle and fails when write takes the JDK's transformer	(SAXTransformerFactory) javax.xml.transform.TransformerFactory.newDefaultInstance() in write: under surefire only the format tests notice, the bundle's xml-apis 1.3.04 has no such method	4fbcc475

A4: a whitespace-only run is removed whole (d5236ca)

#	arm (file:line — function — condition)	case	observable	mutant	red at
1	XmlDocumentWriter.java:93,96-98 — normalizeText — a blank run: every node of the run is removed, not only the first	whitespaceOnlyRunOfSeveralNodesIsRemovedWhole	a in <r><a> <![CDATA[ ]]> </a></r> has no children	a blank run removes only its first node, as the old XPath did (the other 131 tests stay green on it)	mutant only, arm from 6f83fa3e
2	XMLHandlerImpl.java:389 — dispose — normalizeText removes the two whitespace-only Text nodes that a deleted entry leaves side by side	deletingTheLastEntryLeavesAnEmptyContainer	after the only entry of a store file is deleted, the container in the file has no children	the old XPath and its removal loop in place of normalizeText: the file keeps a line break (disposeNeverUsesNodeLists also fails, through its node-list probe); row 1's mutant fails it too	mutant only, arm from 4fbcc475

A5: a merged run of text keeps its place (7b13f6f)

#	arm (file:line — function — condition)	case	observable	mutant	red at
1	XmlDocumentWriter.java:94 — normalizeText — the merged Text node is inserted before the first node of its run	mergedRunKeepsItsPlace	x in <r><x>a<![CDATA[b]]><!--c--></x></r> holds the Text node ab, then the comment	parent.appendChild(…) in place of insertBefore(…, node): the value moves after the comment (the other 134 tests stay green on it)	mutant only, arm from 6f83fa3e

A6: review fixes (1f97256..bd2fe56)

#	arm (file:line — function — condition)	case	observable	mutant	red at
1	XmlDocumentWriter.java:75 — normalizeText — an entity reference is not descended into: its children are read-only	textInsideAnEntityReferenceIsLeftAlone	with Xerces 2.6.2 and entity references not expanded, normalizeText returns and the reference keeps its blank Text node and its CDATA section	Node child = node.getFirstChild(); (removeChild throws DOMException NO_MODIFICATION_ALLOWED_ERR)	7a1a5eb5
2	XmlDocumentWriter.java:209,212 — bind — an unprefixed DOM level 1 element (createElement) is in no namespace, undeclaring a default namespace in scope with xmlns=""	unprefixedDomLevel1ElementIsInNoNamespace	file bytes equal the old DOMSource output (<b xmlns=""/>), with and without an xmlns attribute on the parent	if (uri == null) (the element takes the default namespace in scope)	1f97256e
3	XmlDocumentWriter.java:81-85 — normalizeText — a lone Text node is checked in place, without a list or a buffer	—	unpinnable by construction: the general branch gives the same tree for a lone Text node, so only allocation tells them apart; the conditions that choose the branch are pinned by A1 rows 4, 5, 8 and 9	—	—
4	XMLHandlerImpl.java:387 — dispose — the document comes from getDocument(), with no monitor on it	saveWithoutADocumentThrowsConnectorException	dispose() on a handler whose init() never ran throws ConnectorException Data file does not exists: …	synchronized (document) { … } around the save, as before (a NullPointerException)	f238a5a3
5	XmlHandlerUtil.java:91 — following — loop bound n != null: a root that is not an ancestor acts as null	followingWithARootOutsideTheAncestorsEndsAtTheTop	following(b, a) is null in <r><a><x/></a><b/></r>	bound n != root only (a NullPointerException above the document)	e851b4ab

A7: second review fixes (b3fba18, faf5d15)

#	arm (file:line — function — condition)	case	observable	mutant	red at
1	XmlDocumentWriter.java:226-228 — declare — a prefix the element already declares is not declared again	twoXmlnsAttributesDeclareTheDefaultNamespaceOnce	r with two xmlns attributes (setAttribute urn:y, then setAttributeNS urn:z, which Xerces puts first) reads back from the file, in urn:z	the check dropped (xmlns declared twice, and the parser rejects the file; the other 141 tests stay green on it)	bd2fe56f
2	XmlDocumentWriter.java:209 — bind — declared.contains(prefix): a prefix the element already declares, by an attribute or for its name, keeps that namespace	aPrefixIsDeclaredOncePerElementAsBefore	file bytes equal the old DOMSource output for four children that bind one prefix to two namespaces (<b xmlns="urn:y"/> three times, <p:b xmlns:p="urn:x" p:c="v"/>)	both checks dropped, as at bd2fe56f (row 1's case fails too). The bind check alone is unpinnable: Saxon 9.4's serializer writes the declarations it got from startPrefixMapping and does not compare them with the namespace passed to startElement or addAttribute, so with row 1's check in place, dropping it leaves all 142 tests green (verified); kept so that every name is sent with the namespace the file gives it	bd2fe56f
3	XmlDocumentWriter.java:75-76 — normalizeText — after an entity reference the walk goes on with following(node, root)	textAfterAnEntityReferenceIsNormalized	in <r>&e; <c> </c></r>, the blank Text node after the reference and the one inside c are removed	return at the first entity reference (the other 141 tests stay green on it; at bd2fe56f all 139 did)	mutant only, arm from 1f97256e

A1 row 6's test was corrected in review (a literal U+3000 became \u3000); its red was re-run with the corrected test on the tree without that arm.

Notes on the rows: A1 has 24 rows, 5 unpinnable (20-24); A2 has 5 rows; A3 has 2 rows; A4 has 2 rows; A5 has 1 row; A6 has 5 rows, 1 unpinnable (3); A7 has 3 rows, and row 2's bind check is unpinnable on its own. A2 row 5 was found at the last read: its arm is in 4fbcc47, its test was added afterwards, and its red ran on dca40471 with that one line reverted. A3 row 2's arm was written in A1, so its "red" is the manual mutant described above (applied, run, reverted).

[vharseko]

I found no correctness problem in the paths the connector actually uses: XMLHandlerImpl parses with entity references expanded, and dispose() runs under ConcurrentXMLHandler's write lock. The points below are two edge cases outside that path, three efficiency items, and two cleanups. I checked them against the PR description and #157 and don't argue with anything decided there: fixing only the save, sending Saxon events instead of using XSLTC, the namespace-order change, and leaving the atomic save to #160.

Edge cases (the connector's own parsing doesn't reach them)

1. normalizeText edits read-only entity-reference children — XmlDocumentWriter.java:95

Under the DOM rules, the children of an EntityReference are read-only. If a DOM is parsed with setExpandEntityReferences(false), as in <!DOCTYPE r [<!ENTITY e '<a/> <b/>'>]><r>&e;</r>, the walk reaches the blank text inside the entity and calls removeChild on it. That throws DOMException (NO_MODIFICATION_ALLOWED_ERR), which dispose()'s catch (TransformerException | SAXException | IOException) doesn't catch. The result: no "Failed saving changes" log line, no ConnectorException.wrap, and the file is never written. write() handles ENTITY_REFERENCE_NODE on purpose (entityReferencesAndDoctypeAreWrittenAsBefore), so normalizeText could skip entity-reference subtrees the same way. Unconfirmed: I haven't reproduced this on Xerces 2.6.2. The JDK parser I tried left the reference with no children.

2. bind() puts DOM level 1 nodes into a namespace the writer invented — XmlDocumentWriter.java:206

Take a parent built with createElementNS("urn:x", "a"), with no prefix and no xmlns attribute, and a child built with createElement("b"). The writer declares xmlns="urn:x" on <a>. For <b>, bind("", null) returns urn:x, so no xmlns="" is written and <b> reads back inside urn:x. The old serializer looked only at real xmlns attributes and left <b> in no namespace. The connector always uses prefixed names, so its own files aren't affected. This only matters to other callers of the writer.

Efficiency

3. A new Saxon factory, and so a new Configuration, on every save — XmlDocumentWriter.java:112

The connector isn't poolable, so nearly every operation ends in dispose() and pays this fixed cost. That matters most for small stores, where #157 measured 45–72 ms per operation. Because dispose() already runs under the write lock, one static factory (or one shared Configuration) would do. The old code had the same cost, but since this line is being rewritten anyway, it's a cheap fix.

4. normalizeText allocates for every text run — XmlDocumentWriter.java:81

Every text run, including the common case of a single TEXT_NODE with no text sibling, gets an ArrayList, a StringBuilder and a copy of its value. At 40,000 entries that is roughly 1M short-lived objects per save. A fast path would avoid most of it: when the node is a TEXT_NODE and getNextSibling() isn't text, check isXmlWhitespace(node.getNodeValue()) directly, and build the list and buffer only for real multi-node or CDATA runs.

5. The CRC-32 and the changed flag are computed but not used — XmlDocumentWriter.java:120

XMLHandlerImpl.dispose() ignores both return values, but every save still runs CRC-32 over the whole file (about 11.7 MB at 40,000 entries). If they are for #177, either add them there or say so here, as the description already does for setModified.

Cleanup

6. synchronized (document) no longer excludes anything — XMLHandlerImpl.java:388

The new comment notes that callers already hold the write lock, and nothing else in the module locks on the document. When init() failed and document is null, this block throws NullPointerException instead of the usual ConnectorException from getDocument(). Dropping the monitor, or going through getDocument(), would fix both.

7. following(node, root) throws NPE when root isn't an ancestor of node — XmlHandlerUtil.java:90

The loop climbs past the Document to null and then calls getNextSibling() on it. The precondition is only in the Javadoc, and this is a public method in a shared util class that #177 adds callers to. With n != null && n != root as the loop condition, misuse returns null instead of crashing.

[maximthomas]

I checked each point on this branch with the module's own Xerces 2.6.2 and Saxon 9.4.0.7. Fixes are in 1f97256..bd2fe56. For point 3 I left the code as it is; the numbers are below. The description has been updated (A6 rows; the CRC note under "Change").

1. Entity-reference children. Reproduced on Xerces 2.6.2, with deferred and non-deferred DOMs: your '<a/> <b/>' gives NO_MODIFICATION_ALLOWED_ERR. So does a non-blank run of several nodes, such as 'x<![CDATA[y]]>'. The JDK parser leaves the reference empty, as you found. This was not a regression: on master the old XPath strip already failed on any unexpanded entity reference, because Saxon's DOM wrapper throws IllegalArgumentException: Unsupported node type in DOM! 5. That includes 'v<x/>', which this branch saves. normalizeText no longer descends into entity references (1f97256, textInsideAnEntityReferenceIsLeftAlone).

2. bind() and DOM level 1 elements. Confirmed, and the gap was a little wider than described. The old serializer wrote an unprefixed level 1 element in no namespace even when the parent's default namespace came from a real xmlns attribute. In both cases it wrote <a xmlns="urn:x"><b xmlns=""/></a>. The writer now does the same. A prefixed level 1 node still takes the namespace in scope, which matches the old output (cbbee60). unprefixedDomLevel1ElementIsInNoNamespace compares both cases byte for byte with the old DOMSource output.

3. A Saxon factory per save. The cost is real but small. On a warm JVM, new TransformerFactoryImpl() plus newTransformerHandler() takes about 34 µs and 10 KB; with a shared factory it is 2 µs. A save of 1,000 entries takes about 10 ms (1.5 ms in normalizeText, 8-11 ms in write), so the factory is about 0.3% of the save and under 0.1% of a 45-72 ms operation. Each query also builds a new XQueryHandler, and with it a new SaxonXQDataSource and Configuration: one or two per operation. One correction to the reasoning: the write lock belongs to the per-file ConcurrentXMLHandler, so saves of different stores can run in parallel. A shared factory would still be safe, because Saxon 9.4's newTransformerHandler() builds a new Controller on every call. Even so, for 34 µs I would keep the factory local. If you prefer a shared one, it is a one-line change.

4. Allocation in normalizeText. Confirmed. A freshly parsed store of 40,000 entries in the #157 shape has 560,001 text runs, all of them single Text nodes. One pass allocated 49 MB and took 45-231 ms. With the lone-Text shortcut it allocates 0.1 MB and takes 23-47 ms. On an already normalized store, the figures went from 22 MB and 18-34 ms to 0 MB and 7-10 ms, and the resulting trees are identical (f238a5a). normalizeAndWriteMakeALinearNumberOfDomCalls now counts 63,035 and 126,035 DOM calls for 1,000 and 2,000 entries.

5. The CRC-32 and changed. changed had no caller in either PR. It is gone, and normalizeText now returns void. normalizedDocumentIsLeftAlone now checks that the Text node is still the same object (b3b4792). The CRC stays. #177 compares it with the file's content when the file's timestamp is too recent to show a change (fileHasChanged()), and the description now says so, as it does for setModified. It costs 1.2-1.6 ms for 12.3 MB, about 0.3% of the write.

6. synchronized (document). Confirmed. The block is gone, and dispose() takes the document from getDocument() (e851b4a, saveWithoutADocumentThrowsConnectorException). For the record, the NPE was not new, since master had the same block, and the connector could not reach it: when init() fails, ConcurrentXMLHandler never counts the caller, and XMLConnector.dispose() returns early.

7. following(). The NPE is real. The loop condition is now n != null && n != root (bd2fe56, followingWithARootOutsideTheAncestorsEndsAtTheTop), and the Javadoc says that any other root acts as null. One correction: #177 does not add callers of following(), or of anything else in XmlHandlerUtil. Both callers are in normalizeText, and both pass a descendant of root.

#177 is rebased onto bd2fe56. In its commit that loads and saves only on change, the new test from point 6 becomes saveWithoutADocumentSavesNothing. There, dispose() with nothing to save returns before it touches the document.

[vharseko]

I checked the fix round 1f97256..bd2fe56 against the first round and your reply, on Xerces 2.6.2 and Saxon 9.4.0.7. Points 1, 4, 5, 6 and 7 are fixed as described. I accept both corrections in your reply (the write lock is per file; #177 adds no callers of following()), and keeping the factory local for point 3. The lone-Text shortcut builds the same tree as the old general branch for every input I tried. Two things are left, and the first one was introduced in this round.

1. bind() declares a prefix twice on one element, and the file is not well-formed — XmlDocumentWriter.java:205-212

sendElement first declares the element's own xmlns and xmlns:p attributes, then calls bind for the element's name. Since cbbee60, an unprefixed DOM level 1 element (its namespace is null) is bound to "". When that element has its own xmlns attribute with another value, bind declares "" a second time on the same element, and Saxon writes both declarations:

DOM (the parent is createElementNS("urn:x", "a"))	old DOMSource output	7a1a5eb	bd2fe56
createElement("b") + setAttribute("xmlns", "urn:y")	<b xmlns="urn:y"/>	<b xmlns="urn:y"/>	<b xmlns="urn:y" xmlns=""/>
createElement("b") + setAttributeNS(XMLNS, "xmlns", "urn:y")	<b xmlns="urn:y"/>	<b xmlns="urn:y"/>	<b xmlns="urn:y" xmlns=""/>
createElement("b") + setAttributeNS(XMLNS, "xmlns", "urn:x")	<b/>	<b xmlns="urn:x"/>	<b xmlns="urn:x" xmlns=""/>
createElementNS("urn:x", "b") + setAttributeNS(XMLNS, "xmlns", "urn:y")	<b xmlns="urn:y"/>	<b xmlns="urn:y" xmlns="urn:x"/>	as 7a1a5eb

A namespace-aware parser rejects every bd2fe56 cell, and the 7a1a5eb cell of the last row: Attribute "xmlns" bound to namespace "http://www.w3.org/2000/xmlns/" was already specified for element "b". The first three rows are new in cbbee60. The last row has been there since 6f83fa3, and I missed it in the first round. The connector does not build such a DOM: it parses namespace-aware, and its own elements and attributes don't contradict their xmlns attributes. So this is in the same category as point 2 of the first round, but the outcome is worse: instead of an element in the wrong namespace, the result is a file that cannot be loaded back.

A fix that gives the old output: let a declaration from the element's own attributes win, as DOMSource did.

String inScope = namespaces.getURI(prefix);
if (declared.contains(prefix)) {
    return inScope == null ? "" : inScope;
}

With this fix, rows 1, 2 and 4 are byte-for-byte equal to the old output. Row 3 becomes <b xmlns="urn:x"/>, the kept redeclaration that the description already lists. The module's 139 tests stay green. A case with the element's own xmlns attribute in unprefixedDomLevel1ElementIsInNoNamespace, or a separate test, would pin it.

2. textInsideAnEntityReferenceIsLeftAlone doesn't check that the walk continues after the reference — XmlDocumentWriterTests.java:148

The test kills the mutant in A6 row 1 (Node child = node.getFirstChild();). It does not kill one that stops the walk at the reference: with if (node.getNodeType() == Node.ENTITY_REFERENCE_NODE) { return; } before line 75, all 139 tests stay green, so a document with an unexpanded entity reference would keep all the whitespace after its first reference. Text after the reference would pin it. For example, with <r>&e; <c> </c></r>, assert that the blank Text node after the reference and the one inside c are removed.

[maximthomas]

Both points are fixed in b3fba18 and faf5d15, checked with the module's Xerces 2.6.2 and Saxon 9.4.0.7. The description is updated: the file-format note under "Change", "Tests", and a new A7 table.

1. A prefix declared twice on one element. Reproduced. Your four DOMs give exactly the outputs in your table, and the parser rejects every bd2fe56 cell. I took your rule: a prefix that the element already declares keeps that namespace (bind, b3fba18). Rows 1, 2 and 4 now match the old output byte for byte. Row 3 is <b xmlns="urn:x"/>, the kept redeclaration that the description already lists.

The same defect has two more triggers, both there since 6f83fa3, and the fix covers both:

• A prefixed attribute whose namespace contradicts the element's prefix: createElementNS("urn:x", "p:b") + setAttributeNS("urn:y", "p:c", "v"). bd2fe56 wrote xmlns:p twice. The writer now gives <p:b xmlns:p="urn:x" p:c="v"/>, the old output.
• Two xmlns attributes on one element. setAttributeNS(XMLNS, "xmlns", …) does not find an attribute set with setAttribute("xmlns", …), so Xerces keeps both, and the first loop of sendElement declared both. Your check in bind does not see this case, so declare now skips a prefix that the element already declares, and the first declaration wins (twoXmlnsAttributesDeclareTheDefaultNamespaceOnce). The old serializer kept the other one (urn:y where the writer now writes urn:z). Both outputs are well-formed.

One difference from the old output remains. For a prefix other than the default one, the old serializer let the node's namespace win over the element's own xmlns:p attribute. createElementNS("urn:x", "p:b") + xmlns:p="urn:y" came out as <p:b xmlns:p="urn:x"/>, and a p:c attribute in urn:z next to xmlns:p="urn:y" came out with xmlns:p="urn:z". With the fix, the xmlns:p attribute wins for every prefix, so both now come out with xmlns:p="urn:y". The files are well-formed, and the node is in the declared namespace. bd2fe56 wrote two declarations in both cases, as every commit since 6f83fa3 did. Matching Saxon's split rule would mean holding the declarations back until the names are bound, and I don't think that is worth it for DOMs the connector does not build. The description now says this.

For the mutation record: the check in bind is unpinnable on its own. With the declare check in place, dropping declared.contains(prefix) from bind leaves all 142 tests green. Saxon 9.4's serializer writes the declarations it got from startPrefixMapping and does not compare them with the namespace passed to startElement or addAttribute. I kept the check so that every name is sent with the namespace the file gives it (A7 row 2). aPrefixIsDeclaredOncePerElementAsBefore compares four such children with the old DOMSource output and fails on bd2fe56's code. Dropping the declare check alone fails only the two-xmlns test.

2. The walk after an entity reference. Confirmed: with return at the first entity reference, all 139 tests at bd2fe56 stay green. textAfterAnEntityReferenceIsNormalized (faf5d15) parses <r>&e; <c> </c></r> with entity references not expanded, and asserts that the blank Text node after the reference and the one inside c are removed. It fails on that mutant, and the other 141 tests stay green on it. I made it a separate test, so that textInsideAnEntityReferenceIsLeftAlone still checks one thing.

mvn -o -pl OpenICF-xml-connector verify: 142 unit tests and 1 IT, green on JDK 26 and on JDK 11. #177 is rebased onto the new tip (946df85; 191 unit tests and 1 IT, green on both JDKs). It does not touch XmlDocumentWriter or its tests.

[vharseko]

I checked the third round, b3fba18 and faf5d15, against my second review and your reply, with the module's Xerces 2.6.2 and Saxon 9.4.0.7. Both points are fixed as described. The four DOMs from my table and your two extra triggers now give well-formed output, rows 1, 2 and 4 match the old output byte for byte, and textAfterAnEntityReferenceIsNormalized kills the mutant that returns at the first entity reference. I accept letting the xmlns:p attribute win for every prefix instead of copying Saxon's split rule. I also accept that the bind check can't be pinned on its own: with the declare check in place, I get the same bytes without it. Every CI job that has finished is green; the two Windows jobs are still pending.

One claim in the reply doesn't hold, and there's one nit.

1. An attribute moved into the declared namespace can clash with another attribute — XmlDocumentWriter.java:209

The reply says that once the xmlns:p attribute wins for every prefix, "the files are well-formed". That holds for the element, but not for attributes. An attribute moved into the declared namespace can end up with the same expanded name as another attribute of the same element.

DOM: b = createElementNS("urn:r", "b"), then setAttributeNS(XMLNS, "xmlns:p", "urn:x"), setAttributeNS("urn:y", "p:c", "1") and setAttributeNS("urn:x", "q:c", "2").

	b in the output	namespace-aware parse
old DOMSource	<b xmlns:p="urn:y" xmlns:q="urn:x" p:c="1" q:c="2"/>	ok: {urn:y}c, {urn:x}c
bd2fe56	<b xmlns:p="urn:x" xmlns:p="urn:y" xmlns:q="urn:x" p:c="1" q:c="2"/>	rejected: xmlns:p twice
faf5d15	<b xmlns:p="urn:x" xmlns:q="urn:x" p:c="1" q:c="2"/>	rejected: Attribute "c" bound to namespace "urn:x" was already specified for element "b"

This round didn't introduce it: bd2fe56 already wrote an unreadable file here. Still, of the "any other prefix" differences you list, this is the only case I found where the old output was readable and the new one isn't. It's the same category as before: the connector doesn't build such a DOM, and a parsed store can't contain one. The old serializer has the same defect when the clash comes from the element's own prefix. With createElementNS("urn:x", "p:b") + p:c in urn:y + q:c in urn:x, both outputs are <p:b xmlns:p="urn:x" xmlns:q="urn:x" p:c="1" q:c="2"/>. The same holds for two p:c attributes in two namespaces, and in both cases the writer matches the old output byte for byte.

I'm not asking you to change the rule; closing this would need a fresh prefix for the moved attribute, which isn't worth it for such DOMs. A sentence in the description's file-format note is enough. It should say that when the namespace chosen for a prefix gives an attribute the expanded name of another attribute of the same element, the file is not well-formed, as with the old serializer when the clash comes from the element's prefix.

2. Nit: a broken line wrap in bind's Javadoc — XmlDocumentWriter.java:202-203

Line 202 breaks after "A DOM level 1 node has", well before the margin, and the sentence continues on line 203.

With the note in point 1 (or without it, if this thread is record enough), I have nothing else on this PR.

[vharseko]

Repeating review 5472693782 (of faf5d158) as a change request. The branch is still at faf5d158, nothing in the thread answers that review, and both of its points are still open:

1. The description note on attribute clashes (XmlDocumentWriter.java:209). The file-format note under "Change" still has no sentence on it. It should say that when the namespace chosen for a prefix gives an attribute the expanded name of another attribute of the same element, the file is not well-formed. The old serializer does the same when the clash comes from the element's own prefix. The rule itself stays as it is; only the note is asked for. The DOM and the outputs are in 5472693782.
2. Nit: the line wrap in bind's Javadoc (XmlDocumentWriter.java:202-203). Line 202 still breaks after "A DOM level 1 node has", and the sentence goes on in line 203.

With these two, I have nothing else on this PR. #177 has been approved and waits on this one.

[maximthomas]

Both points of 5472693782, repeated in this review, are done. That review arrived after my reply to the second one (issuecomment-6084596815), and I missed it.

1. The attribute clash. I reproduced it with the module's Xerces 2.6.2 and Saxon 9.4.0.7. Your DOM gives exactly the three outputs in your table. With createElementNS("urn:x", "p:b"), the old serializer and write give the same bytes, and the parser rejects both. The same goes for two p:c attributes in two namespaces, with or without an xmlns:p attribute on the element. The rule stays as it is. The file-format note under "Change" now ends with:

When the namespace chosen for a prefix gives an attribute the expanded name of another attribute of the same element, the file is not well-formed. With xmlns:p="urn:x" on an element, its attributes p:c in urn:y and q:c in urn:x are both written as c in urn:x; the old serializer declared p as urn:y there and wrote a readable file. When the clash comes from the element's own prefix (p:b in urn:x with the same two attributes), the old serializer writes the same unreadable file as write, byte for byte.

2. The line wrap. d4fe809 rewraps bind's Javadoc. The words and the file's line count stay the same, so the file:line references in the description still point to the right lines.

mvn -o -pl OpenICF-xml-connector verify: 142 unit tests and 1 IT, green on JDK 26 and on JDK 11. #177 does not touch XmlDocumentWriter, so it stays at c7f327f, on top of faf5d15. After this PR is merged, it will be rebased onto master, as planned.