From 93329684398c2c0e5b527eea120131765e678a78 Mon Sep 17 00:00:00 2001 From: maximthomas Date: Thu, 8 Oct 2026 15:32:14 +0300 Subject: [PATCH 01/24] [#157] Add a stamp that tells whether the XML file has changed --- .../openicf/connectors/xml/FileStamp.java | 101 +++++++++++++ .../connectors/xml/FileStampTests.java | 140 ++++++++++++++++++ 2 files changed, 241 insertions(+) create mode 100644 OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/FileStamp.java create mode 100644 OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/FileStampTests.java diff --git a/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/FileStamp.java b/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/FileStamp.java new file mode 100644 index 00000000..3ff6c492 --- /dev/null +++ b/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/FileStamp.java @@ -0,0 +1,101 @@ +/* + * 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.io.File; +import java.io.IOException; +import java.io.InputStream; +import java.nio.file.Files; +import java.nio.file.NoSuchFileException; +import java.nio.file.attribute.BasicFileAttributes; +import java.nio.file.attribute.FileTime; +import java.util.Objects; +import java.util.zip.CRC32; +import java.util.zip.CheckedInputStream; + +/** + * What the store file looked like at one moment: its modification time, size and file key, or that + * it did not exist. Stamps that differ in any of these mean the file has changed; an older + * modification time counts too, because {@code cp -p} sets one. + *

+ * A file rewritten in place with the same size within one tick of the modification time clock + * keeps all of these. That can only go unnoticed while the stamp is racy: when it was taken, the + * modification time was less than {@link #RACY_WINDOW_MILLIS} before the clock, or after it. + * Callers compare the content ({@link #checksum}) behind a racy stamp. + */ +final class FileStamp { + + /** The coarsest common tick: 2 s on FAT; 1 s on HFS+ and ext3. */ + static final long RACY_WINDOW_MILLIS = 2000L; + + /** No file: no modification time, which every existing file has. */ + static final FileStamp MISSING = new FileStamp(null, -1L, null, false); + + /** The file could not be read: matches nothing, not even itself. */ + private static final FileStamp UNKNOWN = new FileStamp(null, -1L, null, true); + + private final FileTime lastModified; + private final long size; + private final Object fileKey; + private final boolean racy; + + private FileStamp(FileTime lastModified, long size, Object fileKey, boolean racy) { + this.lastModified = lastModified; + this.size = size; + this.fileKey = fileKey; + this.racy = racy; + } + + static FileStamp read(File file) { + long now = System.currentTimeMillis(); + try { + BasicFileAttributes attributes = Files.readAttributes(file.toPath(), BasicFileAttributes.class); + FileTime modified = attributes.lastModifiedTime(); + return new FileStamp(modified, attributes.size(), attributes.fileKey(), + now - modified.toMillis() < RACY_WINDOW_MILLIS); + } catch (NoSuchFileException e) { + return MISSING; + } catch (IOException e) { + return UNKNOWN; + } + } + + /** The CRC-32 of the file's content. */ + static long checksum(File file) throws IOException { + CRC32 crc = new CRC32(); + try (InputStream in = new CheckedInputStream(Files.newInputStream(file.toPath()), crc)) { + byte[] buffer = new byte[64 * 1024]; + while (in.read(buffer) != -1) { + // the stream updates the checksum + } + } + return crc.getValue(); + } + + boolean isRacy() { + return racy; + } + + boolean sameState(FileStamp other) { + if (this == UNKNOWN || other == UNKNOWN) { + return false; + } + return Objects.equals(lastModified, other.lastModified) + && size == other.size + && Objects.equals(fileKey, other.fileKey); + } +} diff --git a/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/FileStampTests.java b/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/FileStampTests.java new file mode 100644 index 00000000..fa388d19 --- /dev/null +++ b/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/FileStampTests.java @@ -0,0 +1,140 @@ +/* + * 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.getRandomXMLFile; +import static org.forgerock.openicf.connectors.xml.XmlConnectorTestUtil.setModified; +import static org.testng.Assert.assertEquals; +import static org.testng.Assert.assertFalse; +import static org.testng.Assert.assertNotEquals; +import static org.testng.Assert.assertTrue; + +import java.io.File; +import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.NoSuchFileException; +import java.nio.file.StandardCopyOption; +import java.nio.file.attribute.BasicFileAttributes; +import java.util.zip.CRC32; + +import org.testng.SkipException; +import org.testng.annotations.Test; + +public class FileStampTests { + + private static final long HOUR = 3_600_000L; + + @Test + public void unchangedFileMatches() throws IOException { + File file = write(getRandomXMLFile(), "abc", now() - HOUR); + assertTrue(FileStamp.read(file).sameState(FileStamp.read(file))); + } + + @Test + public void olderMtimeIsAChange() throws IOException { + File file = write(getRandomXMLFile(), "abc", now() - HOUR); + FileStamp before = FileStamp.read(file); + setModified(file, now() - 2 * HOUR); + assertFalse(before.sameState(FileStamp.read(file))); + } + + @Test + public void sizeChangeWithTheSameMtimeIsAChange() throws IOException { + long past = now() - HOUR; + File file = write(getRandomXMLFile(), "abc", past); + FileStamp before = FileStamp.read(file); + write(file, "abcd", past); + assertFalse(before.sameState(FileStamp.read(file))); + } + + @Test + public void replacedFileIsAChange() throws IOException { + long past = now() - HOUR; + File file = write(getRandomXMLFile(), "abc", past); + if (Files.readAttributes(file.toPath(), BasicFileAttributes.class).fileKey() == null) { + throw new SkipException("The file system reports no file key"); + } + FileStamp before = FileStamp.read(file); + File copy = write(getRandomXMLFile(), "abc", past); + Files.move(copy.toPath(), file.toPath(), StandardCopyOption.REPLACE_EXISTING); + setModified(file, past); + assertFalse(before.sameState(FileStamp.read(file))); + } + + @Test + public void unreadableStampMatchesNothing() throws IOException { + File loop = getRandomXMLFile(); + try { + Files.createSymbolicLink(loop.toPath(), loop.toPath().getFileName()); // a link to itself + } catch (IOException | UnsupportedOperationException e) { + throw new SkipException("Cannot create a symbolic link here: " + e); + } + try { + Files.readAttributes(loop.toPath(), BasicFileAttributes.class); + throw new SkipException("The file system resolves a link to itself"); + } catch (NoSuchFileException e) { + throw new SkipException("The file system reports a link to itself as missing"); + } catch (IOException expected) { + // ELOOP: the path is neither readable nor known to be missing + } + assertFalse(FileStamp.read(loop).sameState(FileStamp.read(loop))); + assertFalse(FileStamp.read(loop).sameState(FileStamp.MISSING)); + assertFalse(FileStamp.MISSING.sameState(FileStamp.read(loop))); + } + + @Test + public void missingFileMatchesMissing() { + assertTrue(FileStamp.read(getRandomXMLFile()).sameState(FileStamp.MISSING)); + } + + @Test + public void stampIsRacyOnlyNearTheClock() throws IOException { + File file = write(getRandomXMLFile(), "abc", now() + HOUR); + assertTrue(FileStamp.read(file).isRacy()); + setModified(file, now() - HOUR); + assertFalse(FileStamp.read(file).isRacy()); + } + + @Test + public void checksumFollowsTheContent() throws IOException { + File file = write(getRandomXMLFile(), "abc", now() - HOUR); + CRC32 crc = new CRC32(); + crc.update("abc".getBytes(StandardCharsets.UTF_8)); + assertEquals(FileStamp.checksum(file), crc.getValue()); + write(file, "abd", now() - HOUR); + assertNotEquals(FileStamp.checksum(file), crc.getValue()); + } + + @Test + public void deletedFileIsAChange() throws IOException { + File file = write(getRandomXMLFile(), "abc", now() - HOUR); + FileStamp before = FileStamp.read(file); + Files.delete(file.toPath()); + assertFalse(before.sameState(FileStamp.read(file))); + } + + private static File write(File file, String content, long modified) throws IOException { + Files.write(file.toPath(), content.getBytes(StandardCharsets.UTF_8)); + setModified(file, modified); + return file; + } + + private static long now() { + return System.currentTimeMillis(); + } +} From 2e6c4c7ac84a89ab6dc8a903c4ccd8ae4a79696b Mon Sep 17 00:00:00 2001 From: maximthomas Date: Thu, 8 Oct 2026 15:56:04 +0300 Subject: [PATCH 02/24] [#157] Load and save the XML file only when it has changed --- .../connectors/xml/XMLHandlerImpl.java | 92 +++- .../connectors/xml/XMLConnectorTests.java | 40 +- .../connectors/xml/XMLHandlerReloadTests.java | 452 ++++++++++++++++++ 3 files changed, 554 insertions(+), 30 deletions(-) create mode 100644 OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerReloadTests.java 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 dfd5e608..89218f5a 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 @@ -37,8 +37,10 @@ import com.sun.xml.xsom.XSSchema; import com.sun.xml.xsom.XSSchemaSet; +import java.io.ByteArrayInputStream; import java.io.File; import java.io.IOException; +import java.nio.file.Files; import java.util.ArrayList; import java.util.Collection; import java.util.HashMap; @@ -46,6 +48,7 @@ import java.util.Map; import java.util.Set; import java.util.UUID; +import java.util.zip.CRC32; import javax.xml.parsers.DocumentBuilder; import javax.xml.parsers.DocumentBuilderFactory; @@ -74,6 +77,7 @@ import org.identityconnectors.framework.common.objects.Uid; import org.identityconnectors.framework.common.objects.filter.EqualsFilter; import org.w3c.dom.*; +import org.xml.sax.InputSource; import org.xml.sax.SAXException; public class XMLHandlerImpl implements XMLHandler { @@ -87,8 +91,14 @@ public class XMLHandlerImpl implements XMLHandler { private Schema connSchema; private XSSchema icfSchema; private XSSchema riSchema; - private long lastModified = 0l; - private volatile long version = 0l; + /** Whether the document holds user changes that the file does not have yet. */ + private boolean dirty; + /** Whether the document was created in memory because the file did not exist, and is not saved yet. */ + private boolean unsavedNewFile; + /** The file as of the last load or save attempt. */ + private FileStamp stamp = FileStamp.MISSING; + /** The CRC-32 of the bytes last parsed or written. */ + private long checksum; public static final String XSI_NAMESPACE = "http://www.w3.org/2001/XMLSchema-instance"; public static final String ICF_NAMESPACE_PREFIX = "icf"; public static final String RI_NAMESPACE_PREFIX = "ri"; @@ -108,10 +118,37 @@ public XMLHandlerImpl(XMLConfiguration config, Schema connSchema, XSSchemaSet xs @Override public XMLHandler init() { - buildDocument(); + if (document == null || (!dirty && fileHasChanged())) { + buildDocument(); + } return this; } + /** A racy stamp cannot rule out a same-size write in the same tick, so the content decides. */ + private boolean fileHasChanged() { + File xmlFile = config.getXmlFilePath(); + FileStamp current = FileStamp.read(xmlFile); + if (!stamp.sameState(current)) { + return true; + } + if (!stamp.isRacy()) { + return false; + } + try { + if (FileStamp.checksum(xmlFile) != checksum) { + return true; + } + } catch (IOException e) { + return true; + } + stamp = current; // the content is what we hold; the stamp stops being racy once it is old enough + return false; + } + + private void markDirty() { + dirty = true; + } + @Override public Uid create(final ObjectClass objClass, final Set attributes) { final String method = "create"; @@ -211,6 +248,7 @@ public Uid create(final ObjectClass objClass, final Set attributes) { log.info("Creating new entry: {0}", attributes.toString()); } + markDirty(); getDocument().getDocumentElement().appendChild(objElement); log.info("Exit {0}", method); @@ -266,6 +304,7 @@ public Uid update(ObjectClass objClass, Uid uid, Set replaceAttribute } // Remove existing nodes from entry + markDirty(); removeChildrenFromElement(entry, prefixAttributeName(attributeName)); // Add updated nodes to entry @@ -305,6 +344,7 @@ public void delete(final ObjectClass objClass, final Uid uid) { if (entryExists(objClass, uid, ElementIdentifierFieldType.AUTO)) { Element elementToRemove = getEntry(objClass, uid, ElementIdentifierFieldType.AUTO); getDocument().getDocumentElement().removeChild(elementToRemove); + markDirty(); log.info("Deleting entry: " + elementToRemove.toString()); } else { throw new UnknownUidException("Deleting entry failed. Could not find an entry of type " + objClass.getObjectClassValue() + " with the uid " + uid.getUidValue()); @@ -367,31 +407,31 @@ public Collection search(String query, ObjectClass objClass) { return results; } - private boolean isExternallyModified() { - boolean modified = false; - if (config.getXmlFilePath().exists()) { - modified = lastModified != config.getXmlFilePath().lastModified(); - } - return modified; - } - @Override public void dispose() { final String method = "serialize"; log.info("Entry {0}", method); - if (version != lastModified && isExternallyModified()) { - log.error("UPDATE COLLUSION: File has been modified after read into memory and the data in memory has not been synced before."); + if (!dirty && !unsavedNewFile) { + log.info("Exit {0}: nothing to save", method); + return; + } + File xmlFile = config.getXmlFilePath(); + if (!stamp.sameState(FileStamp.read(xmlFile))) { + log.error("UPDATE COLLISION: {0} has changed since it was loaded or saved; overwriting it with the data in memory.", xmlFile); } - // Callers hold the handler's write lock. Document saved = getDocument(); try { XmlDocumentWriter.normalizeText(saved); - XmlDocumentWriter.write(saved, config.getXmlFilePath()); + checksum = XmlDocumentWriter.write(saved, xmlFile); + dirty = false; + unsavedNewFile = false; log.info("Saving changes to xml file"); } catch (TransformerException | SAXException | IOException ex) { log.error("Failed saving changes to xml file: {0}", ex); throw ConnectorException.wrap(ex); + } finally { + stamp = FileStamp.read(xmlFile); } log.info("Exit {0}", method); @@ -486,6 +526,9 @@ private void createDocument() { + icfSchema.getTargetNamespace() + " " + config.getXsdIcfFilePath()); } + // Not a user change: if the first save fails, a file that appears afterwards is loaded instead. + stamp = FileStamp.read(config.getXmlFilePath()); + unsavedNewFile = true; log.info("Exit {0}", method); } @@ -495,13 +538,22 @@ private void loadDocument(File xmlFile) { DocumentBuilderFactory docBuilderFactory = DocumentBuilderFactory.newInstance(); docBuilderFactory.setNamespaceAware(true); - DocumentBuilder docBuilder; try { - docBuilder = docBuilderFactory.newDocumentBuilder(); - document = docBuilder.parse(xmlFile); - lastModified = xmlFile.lastModified(); - version = lastModified; + DocumentBuilder docBuilder = docBuilderFactory.newDocumentBuilder(); + // Taken before reading: a write during the read shows as a change at the next init(). + FileStamp loadedStamp = FileStamp.read(xmlFile); + byte[] content = Files.readAllBytes(xmlFile.toPath()); + CRC32 crc = new CRC32(); + crc.update(content); + InputSource source = new InputSource(new ByteArrayInputStream(content)); + source.setSystemId(xmlFile.toURI().toString()); + Document loaded = docBuilder.parse(source); + // Nothing changes unless the parse succeeded: a malformed file keeps failing until it is fixed. + document = loaded; + stamp = loadedStamp; + checksum = crc.getValue(); + unsavedNewFile = false; log.info("Loading XML document from: {0}", xmlFile.getPath()); } catch (ParserConfigurationException ex) { throw ConnectorException.wrap(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 a134130b..b10e1211 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 @@ -36,6 +36,8 @@ import java.io.UnsupportedEncodingException; import java.nio.charset.StandardCharsets; import java.nio.file.Files; +import java.nio.file.attribute.FileTime; +import java.util.Arrays; import java.util.HashSet; import java.util.Set; import javax.xml.parsers.DocumentBuilderFactory; @@ -285,15 +287,14 @@ public void valuesSurviveASaveAndAReload() throws Exception { @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.init(); // no file yet: a new document, which dispose() saves + Document document = handler.getDocument(); + // Two children: Xerces allocates no node list cache for a parent with fewer. + document.getDocumentElement().appendChild(document.createElementNS(RI_NAMESPACE, "ri:" + ACCOUNT_TYPE)); + document.getDocumentElement().appendChild(document.createElementNS(RI_NAMESPACE, "ri:" + ACCOUNT_TYPE)); handler.dispose(); + AssertJUnit.assertTrue(xmlFile.exists()); AssertJUnit.assertFalse(XercesNodeLists.used(handler.getDocument())); } @@ -333,10 +334,11 @@ public void failedSaveIsLoggedAndThrown() throws Exception { } @Test - public void saveWithoutADocumentThrowsConnectorException() { + public void saveWithoutADocumentSavesNothing() throws Exception { 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: ")); + String out = captureStdOut(handler::dispose); + AssertJUnit.assertTrue(out, out.contains("Exit serialize: nothing to save")); + AssertJUnit.assertFalse(xmlFile.exists()); } @Test @@ -347,6 +349,24 @@ public void successfulSaveLogsItsExit() throws Exception { AssertJUnit.assertTrue(out, out.contains("Exit serialize" + System.lineSeparator())); } + @Test + public void readOnlyOperationsDoNotRewriteTheFile() throws Exception { + Uid uid = facade.create(ObjectClass.ACCOUNT, getRequiredAccountAttributes(), null); + FileTime past = FileTime.fromMillis(System.currentTimeMillis() - 3_600_000L); + Files.setLastModifiedTime(xmlFile.toPath(), past); + byte[] before = Files.readAllBytes(xmlFile.toPath()); + + facade.search(ObjectClass.ACCOUNT, null, new TestResultsHandler(), null); + facade.getObject(ObjectClass.ACCOUNT, uid, null); + facade.authenticate(ObjectClass.ACCOUNT, ATTR_ACCOUNT_VALUE_NAME, + new GuardedString(ATTR_ACCOUNT_VALUE_PASSWORD.toCharArray()), null); + facade.test(); + facade.schema(); + + AssertJUnit.assertEquals(past, Files.getLastModifiedTime(xmlFile.toPath())); + AssertJUnit.assertTrue(Arrays.equals(before, Files.readAllBytes(xmlFile.toPath()))); + } + private XMLHandlerImpl newHandler() { XMLConfiguration config = new XMLConfiguration(); config.setXmlFilePath(xmlFile); diff --git a/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerReloadTests.java b/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerReloadTests.java new file mode 100644 index 00000000..909f862b --- /dev/null +++ b/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerReloadTests.java @@ -0,0 +1,452 @@ +/* + * 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.assertFalse; +import static org.testng.Assert.assertTrue; +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.nio.file.attribute.FileTime; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collection; +import java.util.Collections; +import java.util.List; +import java.util.Set; +import java.util.function.Function; + +import org.forgerock.openicf.connectors.xml.query.QueryBuilder; +import org.forgerock.openicf.connectors.xml.xsdparser.SchemaParser; +import org.identityconnectors.common.security.GuardedString; +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.Uid; +import org.testng.SkipException; +import org.testng.annotations.BeforeMethod; +import org.testng.annotations.Test; +import org.w3c.dom.DOMException; + +public class XMLHandlerReloadTests { + + private static final long HOUR = 3_600_000L; + + private File file; + private ConcurrentXMLHandler handler; + + @BeforeMethod + public void setUp() { + file = getRandomXMLFile(); + handler = new ConcurrentXMLHandler(config(file), schemaParser().parseSchema(), schemaParser().getXsdSchema()); + } + + @Test + public void readOnlyCallsDoNotWriteTheFile() throws Exception { + writeAccounts(file, "alice"); + FileTime past = FileTime.fromMillis(now() - HOUR); + Files.setLastModifiedTime(file.toPath(), past); + byte[] before = Files.readAllBytes(file.toPath()); + + names(); + call(h -> h.authenticate("alice", new GuardedString("secret-alice".toCharArray()))); + + assertEquals(Files.getLastModifiedTime(file.toPath()), past); + assertEquals(Files.readAllBytes(file.toPath()), before); + } + + @Test + public void newFileIsCreatedByTheFirstCall() { + // Guards existing behaviour: with createFileIfNotExists even test() creates the file. + assertFalse(file.exists()); + call(h -> null); + assertTrue(file.exists()); + } + + @Test + public void changeIsInTheFileWhenTheLastUserLeaves() throws Exception { + writeAccounts(file, "alice"); + setModified(file, now() - HOUR); + call(h -> h.create(ObjectClass.ACCOUNT, account("bob"))); + assertEquals(namesInFile(file), List.of("alice", "bob")); + } + + @Test + public void failedUpdateLeavesTheFileInAgreementWithMemory() throws Exception { + writeAccounts(file, "alice"); + setModified(file, now() - HOUR); + Set twoLastNames = Collections.singleton(AttributeBuilder.build(ATTR_ACCOUNT_LAST_NAME, "A", "B")); + List inMemory; + handler.init(); // holds the document: no reload and no save until the last dispose() + try { + expectThrows(IllegalArgumentException.class, + () -> call(h -> h.update(ObjectClass.ACCOUNT, new Uid("uid-alice"), twoLastNames))); + inMemory = lastNames(handler.search(allAccounts(), ObjectClass.ACCOUNT)); + } finally { + handler.dispose(); + } + assertEquals(lastNamesIn(file), inMemory); + } + + @Test + public void deleteIsInTheFileWhenTheLastUserLeaves() throws Exception { + writeAccounts(file, "alice", "bobby"); + setModified(file, now() - HOUR); + call(h -> { + h.delete(ObjectClass.ACCOUNT, new Uid("uid-bobby")); + return null; + }); + assertEquals(namesInFile(file), List.of("alice")); + } + + @Test + public void unchangedFileIsServedFromMemory() throws Exception { + long past = now() - HOUR; + writeAccounts(file, "alice"); + setModified(file, past); + assertEquals(names(), List.of("alice")); + + // Same size, same mtime, same file, stamp not racy: indistinguishable from no change. + writeAccounts(file, "bobby"); + setModified(file, past); + assertEquals(names(), List.of("alice")); + } + + @Test + public void externalCopyWithAnOlderMtimeIsLoaded() throws Exception { + writeAccounts(file, "alice"); + setModified(file, now() - HOUR); + assertEquals(names(), List.of("alice")); + + writeAccounts(file, "carol"); + setModified(file, now() - 2 * HOUR); + assertEquals(names(), List.of("carol")); + } + + @Test + public void malformedEditFailsEveryCallUntilFixed() throws Exception { + writeAccounts(file, "alice"); + setModified(file, now() - 2 * HOUR); + assertEquals(names(), List.of("alice")); + + Files.write(file.toPath(), " expectThrows(ConnectorException.class, () -> handler.init())); + captureStdErr(() -> expectThrows(ConnectorException.class, () -> handler.init())); + + writeAccounts(file, "bob"); + setModified(file, now() - HOUR / 2); + assertEquals(names(), List.of("bob")); + } + + @Test + public void sameSizeEditInsideTheRacyWindowIsLoaded() throws Exception { + long future = now() + HOUR; // a stamp at or after the clock is racy + writeAccounts(file, "alice"); + setModified(file, future); + assertEquals(names(), List.of("alice")); + + writeAccounts(file, "bobby"); + setModified(file, future); + assertEquals(names(), List.of("bobby")); + } + + @Test + public void racyLoadIsNotParsedAgain() throws Exception { + writeAccounts(file, "alice"); + setModified(file, now() + HOUR); // racy for as long as the test runs + String first = captureStdOut(() -> names()); + assertTrue(first.contains("Loading XML document from: " + file.getPath()), first); + String second = captureStdOut(() -> names()); + assertFalse(second.contains("Loading XML document from"), second); + } + + @Test + public void unreadableFileBehindARacyStampFails() throws Exception { + writeAccounts(file, "alice"); + setModified(file, now() + HOUR); // racy for as long as the test runs + assertEquals(names(), List.of("alice")); + makeUnreadable(file); + try { + expectThrows(ConnectorException.class, () -> handler.init()); + } finally { + file.setReadable(true, false); + } + } + + @Test + public void stampConfirmedByContentStopsBeingRacy() throws Exception { + writeAccounts(file, "alice"); + setModified(file, now() - FileStamp.RACY_WINDOW_MILLIS + 1_000L); // racy for one more second + assertEquals(names(), List.of("alice")); + if (now() - Files.getLastModifiedTime(file.toPath()).toMillis() >= FileStamp.RACY_WINDOW_MILLIS) { + // A slow load, or a file system that truncates mtimes to seconds: the stamp taken was not racy. + throw new SkipException("The load of " + file + " fell outside the racy window"); + } + Thread.sleep(1_500L); // the same mtime is now outside the window + names(); // racy stamp, same content: the stamp is taken again + makeUnreadable(file); + try { + assertEquals(names(), List.of("alice")); // nothing reads the file any more + } finally { + file.setReadable(true, false); + } + } + + @Test + public void relativeDtdIsResolvedAgainstTheFile() throws Exception { + File dtd = new File(file.getParentFile(), file.getName() + ".dtd"); + Files.write(dtd.toPath(), "".getBytes(StandardCharsets.UTF_8)); + writeAccounts(file, "alice"); + String xml = new String(Files.readAllBytes(file.toPath()), StandardCharsets.UTF_8) + .replace("\nalice<", "&who;<"); + Files.write(file.toPath(), xml.getBytes(StandardCharsets.UTF_8)); + assertEquals(names(), List.of("alice")); + } + + @Test + public void outsideEditDuringAChangeIsReportedAndOverwritten() throws Exception { + writeAccounts(file, "alice"); + setModified(file, now() - 2 * HOUR); + handler.init(); + handler.create(ObjectClass.ACCOUNT, account("bob")); + writeAccounts(file, "carol"); + setModified(file, now() - HOUR); + String errors = captureStdErr(handler::dispose); + assertTrue(errors.contains("UPDATE COLLISION: " + file + " has changed since it was loaded or saved"), errors); + assertEquals(namesInFile(file), List.of("alice", "bob")); + } + + @Test + public void ownChangeIsSavedWithoutACollision() throws Exception { + writeAccounts(file, "alice"); + setModified(file, now() - HOUR); + String errors = captureStdErr(() -> call(h -> h.create(ObjectClass.ACCOUNT, account("bob")))); + assertFalse(errors.contains("UPDATE COLLISION"), errors); + } + + @Test + public void deletedFileIsRecreatedWithoutACollision() throws Exception { + writeAccounts(file, "alice"); + setModified(file, now() - HOUR); + assertEquals(names(), List.of("alice")); + Files.delete(file.toPath()); + String errors = captureStdErr(() -> assertEquals(names(), List.of())); + assertFalse(errors.contains("UPDATE COLLISION"), errors); + } + + @Test + public void retryAfterAFailedSaveDoesNotReportItsOwnPartialWrite() throws Exception { + writeAccounts(file, "alice"); + setModified(file, now() - HOUR); + byte[] before = Files.readAllBytes(file.toPath()); + XMLHandlerImpl impl = new XMLHandlerImpl(config(file), schemaParser().parseSchema(), schemaParser().getXsdSchema()); + impl.init(); + Set bob = account("bob"); + bob.add(AttributeBuilder.build(ATTR_ACCOUNT_FIRST_NAME, "x\uD800y")); // an unpaired surrogate: no encoder can write it + impl.create(ObjectClass.ACCOUNT, bob); + captureStdErr(() -> expectThrows(ConnectorException.class, impl::dispose)); + assertFalse(Arrays.equals(Files.readAllBytes(file.toPath()), before)); // the failed save truncated the file + String errors = captureStdErr(() -> expectThrows(ConnectorException.class, impl::dispose)); + assertFalse(errors.contains("UPDATE COLLISION"), errors); + } + + @Test + public void ownSaveIsNotParsedAgain() throws Exception { + writeAccounts(file, "alice"); + setModified(file, now() - HOUR); + call(h -> h.create(ObjectClass.ACCOUNT, account("bob"))); // the stamp after this save is racy + String log = captureStdOut(() -> assertEquals(names(), List.of("alice", "bob"))); + assertFalse(log.contains("Loading XML document from"), log); + } + + @Test + public void failedSaveKeepsTheChangeInMemoryAndRetriesIt() throws Exception { + writeAccounts(file, "alice"); + setModified(file, now() - 2 * HOUR); + handler.init(); + handler.create(ObjectClass.ACCOUNT, account("bob")); + Files.delete(file.toPath()); + assertTrue(file.mkdir()); // a directory cannot be written, whoever runs the test + try { + captureStdErr(() -> expectThrows(ConnectorException.class, handler::dispose)); + } finally { + assertTrue(file.delete()); + } + + // An outside file appears: it must not replace the unsaved change, which overwrites it and says so. + writeAccounts(file, "carol"); + setModified(file, now() - HOUR); + String errors = captureStdErr(() -> assertEquals(names(), List.of("alice", "bob"))); + assertTrue(errors.contains("UPDATE COLLISION"), errors); + assertEquals(namesInFile(file), List.of("alice", "bob")); + } + + @Test + public void unsavedNewDocumentGivesWayToAFileThatAppears() throws Exception { + assertFalse(file.exists()); + handler.init(); // no file: an empty document in memory + assertTrue(file.mkdir()); + try { + captureStdErr(() -> expectThrows(ConnectorException.class, handler::dispose)); + } finally { + assertTrue(file.delete()); + } + + writeAccounts(file, "alice"); + setModified(file, now() - HOUR); + byte[] appeared = Files.readAllBytes(file.toPath()); + assertEquals(names(), List.of("alice")); + assertEquals(Files.readAllBytes(file.toPath()), appeared); // loaded, not written back + } + + @Test + public void savedNewFileIsNotWrittenAgain() throws Exception { + call(h -> null); // creates the file + String log = captureStdOut(() -> call(h -> null)); + assertTrue(log.contains("Exit serialize: nothing to save"), log); + } + + @Test + public void failedDeleteOfANestedEntryLeavesTheFileAlone() throws Exception { + writeAccounts(file, "alice"); + // An XSD-invalid hand edit: getEntry's descendant query finds the entry, removeChild on the container rejects it. + String xml = new String(Files.readAllBytes(file.toPath()), StandardCharsets.UTF_8) + .replace(" \n", " \n \n") + .replace(" \n", " \n \n"); + Files.write(file.toPath(), xml.getBytes(StandardCharsets.UTF_8)); + FileTime past = FileTime.fromMillis(now() - HOUR); + Files.setLastModifiedTime(file.toPath(), past); + byte[] before = Files.readAllBytes(file.toPath()); + + expectThrows(DOMException.class, () -> call(h -> { + h.delete(ObjectClass.ACCOUNT, new Uid("uid-alice")); + return null; + })); + + assertEquals(Files.getLastModifiedTime(file.toPath()), past); + assertEquals(Files.readAllBytes(file.toPath()), before); + } + + @Test + public void externalEditWithANewerMtimeIsLoaded() throws Exception { + writeAccounts(file, "alice"); + setModified(file, now() - 2 * HOUR); + assertEquals(names(), List.of("alice")); + + writeAccounts(file, "bob"); + setModified(file, now() - HOUR); + assertEquals(names(), List.of("bob")); + } + + /** Makes {@code file} unreadable, or skips the test where its owner can read it anyway (root, Windows). */ + private static void makeUnreadable(File file) { + if (!file.setReadable(false, false) || Files.isReadable(file.toPath())) { + file.setReadable(true, false); + throw new SkipException("Cannot make " + file + " unreadable here"); + } + } + + /** One framework call: init, the operation, dispose. */ + private T call(Function operation) { + handler.init(); + try { + return operation.apply(handler); + } finally { + handler.dispose(); + } + } + + private List names() { + return call(h -> { + List names = new ArrayList(); + for (ConnectorObject object : h.search(allAccounts(), ObjectClass.ACCOUNT)) { + names.add(object.getName().getNameValue()); + } + return names; + }); + } + + private static String allAccounts() { + return new QueryBuilder(null, ObjectClass.ACCOUNT).toString(); + } + + private static List lastNames(Collection objects) { + List values = new ArrayList(); + for (ConnectorObject object : objects) { + Attribute lastName = object.getAttributeByName(ATTR_ACCOUNT_LAST_NAME); + values.add(lastName == null ? null : AttributeUtil.getStringValue(lastName)); + } + return values; + } + + private static List lastNamesIn(File file) { + XMLHandler fresh = new XMLHandlerImpl(config(file), schemaParser().parseSchema(), schemaParser().getXsdSchema()).init(); + return lastNames(fresh.search(allAccounts(), ObjectClass.ACCOUNT)); + } + + private static XMLConfiguration config(File file) { + XMLConfiguration config = new XMLConfiguration(); + config.setXmlFilePath(file); + config.setXsdFilePath(XSD_SCHEMA_FILEPATH); + config.setCreateFileIfNotExists(true); + return config; + } + + private static SchemaParser schemaParser() { + return new SchemaParser(XMLConnector.class, XSD_SCHEMA_FILEPATH); + } + + 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(); + System.setErr(new PrintStream(buffer, true, StandardCharsets.UTF_8.name())); + try { + action.run(); + } finally { + System.setErr(original); + } + return buffer.toString(StandardCharsets.UTF_8.name()); + } + + private static long now() { + return System.currentTimeMillis(); + } +} From 6eaed53f25b5799f7c7d8f4ab3b59e2794bba746 Mon Sep 17 00:00:00 2001 From: maximthomas Date: Thu, 8 Oct 2026 16:21:49 +0300 Subject: [PATCH 03/24] [#157] Keep reads of the XML document from writing to it --- .../xml/ConnectorObjectCreator.java | 13 +-- .../connectors/xml/XMLHandlerImpl.java | 41 ++++----- .../connectors/xml/XMLHandlerReloadTests.java | 92 +++++++++++++++++++ 3 files changed, 117 insertions(+), 29 deletions(-) diff --git a/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/ConnectorObjectCreator.java b/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/ConnectorObjectCreator.java index a2066552..4072c3ea 100644 --- a/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/ConnectorObjectCreator.java +++ b/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/ConnectorObjectCreator.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$ */ @@ -40,7 +41,6 @@ import org.identityconnectors.framework.common.objects.ObjectClass; import org.identityconnectors.framework.common.objects.Uid; import org.w3c.dom.Node; -import org.w3c.dom.NodeList; class ConnectorObjectCreator { @@ -49,7 +49,6 @@ class ConnectorObjectCreator { private Map attributeClassMap; private Map attributeInfoMap; private ObjectClass objectClass; - private NodeList nodeList; private ConnectorObjectBuilder conObjBuilder; protected ConnectorObjectCreator(HashMap attrClasses, HashMap attrInfos, ObjectClass objClass) { @@ -58,18 +57,17 @@ protected ConnectorObjectCreator(HashMap attrClasses, HashMap search(String query, ObjectClass objClass) { if (query != null && !query.isEmpty() && objClass != null) { - ObjectClassInfo objInfo = connSchema.findObjectClassInfo(objClass.getObjectClassValue()); - Set objAttributes = objInfo.getAttributeInfo(); - - // Map with the attribute-names and what class they are - HashMap attributeClassMap = new HashMap(); - for (AttributeInfo info : objAttributes) { - attributeClassMap.put(info.getName(), info.getType().getSimpleName()); - } - - // Map with the AttributeInfo for each attribute - HashMap attributeInfoMap = - new HashMap(AttributeInfoUtil.toMap(objInfo.getAttributeInfo())); + ConnectorObjectCreator conObjCreator = newConnectorObjectCreator(objClass); XQueryHandler xqHandler = null; try { @@ -381,17 +371,9 @@ public Collection search(String query, ObjectClass objClass) { XQResultSequence queryResult = xqHandler.getResultSequence(); - ConnectorObjectCreator conObjCreator = - new ConnectorObjectCreator(attributeClassMap, attributeInfoMap, objClass); - while (queryResult.next()) { - Node resultNode = queryResult.getItem().getNode(); - - NodeList nodes = resultNode.getChildNodes(); - - ConnectorObject conObj = conObjCreator.createConnectorObject(nodes); - results.add(conObj); + results.add(conObjCreator.createConnectorObject(queryResult.getItem().getNode())); } } catch (XQException ex) { log.error("Error while searching: {0}", ex); @@ -532,12 +514,29 @@ private void createDocument() { log.info("Exit {0}", method); } + ConnectorObjectCreator newConnectorObjectCreator(ObjectClass objClass) { + ObjectClassInfo objInfo = connSchema.findObjectClassInfo(objClass.getObjectClassValue()); + HashMap attributeClassMap = new HashMap(); + for (AttributeInfo info : objInfo.getAttributeInfo()) { + attributeClassMap.put(info.getName(), info.getType().getSimpleName()); + } + HashMap attributeInfoMap = + new HashMap(AttributeInfoUtil.toMap(objInfo.getAttributeInfo())); + return new ConnectorObjectCreator(attributeClassMap, attributeInfoMap, objClass); + } + private void loadDocument(File xmlFile) { final String method = "loadDocument"; log.info("Entry {0}", method); DocumentBuilderFactory docBuilderFactory = DocumentBuilderFactory.newInstance(); docBuilderFactory.setNamespaceAware(true); + try { + // Build every node now: a deferred node is built on its first read, and readers run in parallel. + docBuilderFactory.setAttribute(DEFER_NODE_EXPANSION, Boolean.FALSE); + } catch (IllegalArgumentException ex) { + log.warn("The XML parser {0} does not support {1}", docBuilderFactory.getClass().getName(), DEFER_NODE_EXPANSION); + } try { DocumentBuilder docBuilder = docBuilderFactory.newDocumentBuilder(); diff --git a/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerReloadTests.java b/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerReloadTests.java index 909f862b..0a44c793 100644 --- a/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerReloadTests.java +++ b/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerReloadTests.java @@ -37,6 +37,10 @@ import java.util.Set; import java.util.function.Function; +import javax.xml.parsers.DocumentBuilder; +import javax.xml.parsers.DocumentBuilderFactory; +import javax.xml.parsers.ParserConfigurationException; + import org.forgerock.openicf.connectors.xml.query.QueryBuilder; import org.forgerock.openicf.connectors.xml.xsdparser.SchemaParser; import org.identityconnectors.common.security.GuardedString; @@ -372,6 +376,94 @@ private static void makeUnreadable(File file) { } } + @Test + public void loadedDocumentIsFullyExpanded() throws Exception { + writeAccounts(file, "alice"); + XMLHandlerImpl impl = new XMLHandlerImpl(config(file), schemaParser().parseSchema(), schemaParser().getXsdSchema()); + impl.init(); + assertFalse(impl.getDocument() instanceof org.apache.xerces.dom.DeferredDocumentImpl, + impl.getDocument().getClass().getName()); + } + + @Test + public void connectorObjectIsReadWithoutNodeLists() throws Exception { + writeAccounts(file, "alice"); + XMLHandlerImpl impl = new XMLHandlerImpl(config(file), schemaParser().parseSchema(), schemaParser().getXsdSchema()); + impl.init(); + org.w3c.dom.Element entry = (org.w3c.dom.Element) impl.getDocument() + .getElementsByTagNameNS(RI_NAMESPACE, ObjectClass.ACCOUNT_NAME).item(0); + assertFalse(XercesNodeLists.used(impl.getDocument())); + + ConnectorObject object = impl.newConnectorObjectCreator(ObjectClass.ACCOUNT).createConnectorObject(entry); + + assertEquals(object.getUid().getUidValue(), "uid-alice"); + assertEquals(object.getName().getNameValue(), "alice"); + assertEquals(AttributeUtil.getStringValue(object.getAttributeByName(ATTR_ACCOUNT_LAST_NAME)), "Last-alice"); + assertFalse(XercesNodeLists.used(impl.getDocument())); + } + + @Test + public void parserWithoutTheDeferAttributeStillLoads() throws Exception { + writeAccounts(file, "alice"); + withDocumentBuilderFactory(NoDeferAttributeFactory.class, () -> assertEquals(names(), List.of("alice"))); + } + + @Test + public void parserWithoutTheDeferAttributeIsReported() throws Exception { + writeAccounts(file, "alice"); + String log = captureStdOut(() -> withDocumentBuilderFactory(NoDeferAttributeFactory.class, () -> names())); + assertTrue(log.contains("The XML parser " + NoDeferAttributeFactory.class.getName() + + " does not support http://apache.org/xml/features/dom/defer-node-expansion"), log); + } + + /** Runs {@code action} with {@code factory} as the JAXP DocumentBuilderFactory, the way a JVM-wide setting selects it. */ + private static void withDocumentBuilderFactory(Class factory, Runnable action) { + String property = "javax.xml.parsers.DocumentBuilderFactory"; + String previous = System.getProperty(property); + System.setProperty(property, factory.getName()); + try { + action.run(); + } finally { + if (previous == null) { + System.clearProperty(property); + } else { + System.setProperty(property, previous); + } + } + } + + /** A JAXP parser that supports none of the Xerces attributes: setAttribute throws, as the JAXP contract allows. */ + public static final class NoDeferAttributeFactory extends DocumentBuilderFactory { + + private final DocumentBuilderFactory xerces = new org.apache.xerces.jaxp.DocumentBuilderFactoryImpl(); + + @Override + public DocumentBuilder newDocumentBuilder() throws ParserConfigurationException { + xerces.setNamespaceAware(isNamespaceAware()); + return xerces.newDocumentBuilder(); + } + + @Override + public void setAttribute(String name, Object value) { + throw new IllegalArgumentException("Not supported: " + name); + } + + @Override + public Object getAttribute(String name) { + throw new IllegalArgumentException("Not supported: " + name); + } + + @Override + public void setFeature(String name, boolean value) throws ParserConfigurationException { + throw new ParserConfigurationException("Not supported: " + name); + } + + @Override + public boolean getFeature(String name) throws ParserConfigurationException { + throw new ParserConfigurationException("Not supported: " + name); + } + } + /** One framework call: init, the operation, dispose. */ private T call(Function operation) { handler.init(); From 0111d43793b196a610a794904108b4fdb647dc60 Mon Sep 17 00:00:00 2001 From: maximthomas Date: Thu, 8 Oct 2026 16:26:53 +0300 Subject: [PATCH 04/24] [#157] Do not hold the XMLConnector class lock while loading a file --- .../connectors/xml/ConcurrentXMLHandler.java | 6 +- .../openicf/connectors/xml/XMLConnector.java | 23 ++--- .../connectors/xml/XMLConnectorTests.java | 92 +++++++++++++++++++ 3 files changed, 109 insertions(+), 12 deletions(-) diff --git a/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/ConcurrentXMLHandler.java b/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/ConcurrentXMLHandler.java index bf6befb0..281a29e8 100644 --- a/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/ConcurrentXMLHandler.java +++ b/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/ConcurrentXMLHandler.java @@ -50,7 +50,11 @@ public class ConcurrentXMLHandler implements XMLHandler { private volatile int invokers = 0; public ConcurrentXMLHandler(XMLConfiguration config, Schema connSchema, XSSchemaSet xsdSchemas) { - proxy = new XMLHandlerImpl(config, connSchema, xsdSchemas); + this(new XMLHandlerImpl(config, connSchema, xsdSchemas)); + } + + ConcurrentXMLHandler(XMLHandler proxy) { + this.proxy = proxy; } @Override diff --git a/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/XMLConnector.java b/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/XMLConnector.java index 1fbe90da..22b5117b 100644 --- a/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/XMLConnector.java +++ b/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/XMLConnector.java @@ -55,7 +55,8 @@ public class XMLConnector implements Connector, AuthenticateOp, CreateOp, DeleteOp, SearchOp, SchemaOp, TestOp, UpdateOp { private static final Log log = Log.getLog(XMLConnector.class); - private static /*volatile*/ final Map XMLHandlerCache = new HashMap(1); + /** Guarded by {@code XMLConnector.class}. */ + static final Map XMLHandlerCache = new HashMap(1); private XMLConfiguration config; private XMLHandler xmlInstanceHandler = null; @@ -77,23 +78,23 @@ public Configuration getConfiguration() { @Override public void init(Configuration configuration) { this.config = Assertions.nullChecked((XMLConfiguration) configuration, "config"); - synchronized (XMLConnector.class) { - try { - String canonicalPath = config.getXmlFilePath().getCanonicalPath(); - ConcurrentXMLHandler handler = XMLHandlerCache.get(canonicalPath); - + ConcurrentXMLHandler handler; + try { + String canonicalPath = config.getXmlFilePath().getCanonicalPath(); + synchronized (XMLConnector.class) { + handler = XMLHandlerCache.get(canonicalPath); if (null == handler) { SchemaParser schemaParser = new SchemaParser(XMLConnector.class, config.getXsdFilePath()); handler = new ConcurrentXMLHandler(config, schema(), schemaParser.getXsdSchema()); XMLHandlerCache.put(canonicalPath, handler); } - xmlInstanceHandler = handler.init(); - } - catch (IOException ex) { - log.error(ex, "Failed to get the CanonicalPath of {0}", config.getXmlFilePath()); - throw new ConnectorIOException(ex); } + } catch (IOException ex) { + log.error(ex, "Failed to get the CanonicalPath of {0}", config.getXmlFilePath()); + throw new ConnectorIOException(ex); } + // Outside the class monitor: loading or saving one file must not hold up connectors on other files. + xmlInstanceHandler = handler.init(); log.info("XMLConnector initialized"); } 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 b10e1211..992afa4e 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 @@ -38,8 +38,10 @@ import java.nio.file.Files; import java.nio.file.attribute.FileTime; import java.util.Arrays; +import java.util.Collection; import java.util.HashSet; import java.util.Set; +import java.util.concurrent.*; import javax.xml.parsers.DocumentBuilderFactory; import org.forgerock.openicf.connectors.xml.xsdparser.SchemaParser; import org.identityconnectors.common.security.GuardedString; @@ -367,6 +369,96 @@ public void readOnlyOperationsDoNotRewriteTheFile() throws Exception { AssertJUnit.assertTrue(Arrays.equals(before, Files.readAllBytes(xmlFile.toPath()))); } + @Test(timeOut = 60_000L) + public void initOnOneFileDoesNotWaitForAnotherFile() throws Exception { + File slowFile = getRandomXMLFile(); + String key = slowFile.getCanonicalPath(); + BlockingHandler blocking = new BlockingHandler(); + synchronized (XMLConnector.class) { + XMLConnector.XMLHandlerCache.put(key, new ConcurrentXMLHandler(blocking)); + } + ExecutorService executor = Executors.newFixedThreadPool(2); + try { + Future slow = executor.submit(() -> new XMLConnector().init(configFor(slowFile))); + AssertJUnit.assertTrue(blocking.entered.await(10, TimeUnit.SECONDS)); + + Future other = executor.submit(() -> { + XMLConnector connector = new XMLConnector(); + connector.init(configFor(getRandomXMLFile())); + connector.dispose(); + }); + other.get(10, TimeUnit.SECONDS); + + blocking.release.countDown(); + slow.get(10, TimeUnit.SECONDS); + } finally { + blocking.release.countDown(); + executor.shutdownNow(); + synchronized (XMLConnector.class) { + XMLConnector.XMLHandlerCache.remove(key); + } + } + } + + private static XMLConfiguration configFor(File xmlFile) { + XMLConfiguration config = new XMLConfiguration(); + config.setXmlFilePath(xmlFile); + config.setXsdFilePath(XSD_SCHEMA_FILEPATH); + config.setCreateFileIfNotExists(true); + return config; + } + + /** A handler whose init() blocks until released. */ + private static final class BlockingHandler implements XMLHandler { + final CountDownLatch entered = new CountDownLatch(1); + final CountDownLatch release = new CountDownLatch(1); + + @Override + public XMLHandler init() { + entered.countDown(); + try { + release.await(); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + } + return this; + } + + @Override + public void dispose() { + } + + @Override + public Uid create(ObjectClass objClass, Set attributes) { + throw new UnsupportedOperationException(); + } + + @Override + public Uid update(ObjectClass objClass, Uid uid, Set replaceAttributes) { + throw new UnsupportedOperationException(); + } + + @Override + public void delete(ObjectClass objClass, Uid uid) { + throw new UnsupportedOperationException(); + } + + @Override + public Collection search(String query, ObjectClass objectClass) { + throw new UnsupportedOperationException(); + } + + @Override + public Uid authenticate(String username, GuardedString password) { + throw new UnsupportedOperationException(); + } + + @Override + public boolean isSupportUid(ObjectClass objectClass) { + throw new UnsupportedOperationException(); + } + } + private XMLHandlerImpl newHandler() { XMLConfiguration config = new XMLConfiguration(); config.setXmlFilePath(xmlFile); From 84df879927cb1354b00bde3806f3b7e3a07e86d2 Mon Sep 17 00:00:00 2001 From: maximthomas Date: Thu, 8 Oct 2026 16:50:34 +0300 Subject: [PATCH 05/24] [#157] Normalize the text of a loaded XML document --- .../openicf/connectors/xml/XMLHandlerImpl.java | 4 +++- .../openicf/connectors/xml/XMLHandlerReloadTests.java | 11 +++++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) 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 b7399b80..947a7b57 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 @@ -96,7 +96,7 @@ public class XMLHandlerImpl implements XMLHandler { private boolean dirty; /** Whether the document was created in memory because the file did not exist, and is not saved yet. */ private boolean unsavedNewFile; - /** The file as of the last load or save attempt. */ + /** The file as last seen: at the last load, save attempt, new document, or content-confirmed check. */ private FileStamp stamp = FileStamp.MISSING; /** The CRC-32 of the bytes last parsed or written. */ private long checksum; @@ -548,6 +548,8 @@ private void loadDocument(File xmlFile) { InputSource source = new InputSource(new ByteArrayInputStream(content)); source.setSystemId(xmlFile.toURI().toString()); Document loaded = docBuilder.parse(source); + // Same text shape as a saved document: CDATA and split runs become one text node, blank text goes. + XmlDocumentWriter.normalizeText(loaded); // Nothing changes unless the parse succeeded: a malformed file keeps failing until it is fixed. document = loaded; stamp = loadedStamp; diff --git a/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerReloadTests.java b/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerReloadTests.java index 0a44c793..8bacee27 100644 --- a/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerReloadTests.java +++ b/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerReloadTests.java @@ -233,6 +233,17 @@ public void relativeDtdIsResolvedAgainstTheFile() throws Exception { assertEquals(names(), List.of("alice")); } + @Test + public void cdataValueOfALoadedFileIsRead() throws Exception { + writeAccounts(file, "alice"); + String xml = new String(Files.readAllBytes(file.toPath()), StandardCharsets.UTF_8) + .replace("Last-alice", ""); + Files.write(file.toPath(), xml.getBytes(StandardCharsets.UTF_8)); + setModified(file, now() - HOUR); + assertEquals(call(h -> lastNames(h.search(allAccounts(), ObjectClass.ACCOUNT))), List.of("Last-alice")); + assertEquals(call(h -> lastNames(h.search(allAccounts(), ObjectClass.ACCOUNT))), List.of("Last-alice")); + } + @Test public void outsideEditDuringAChangeIsReportedAndOverwritten() throws Exception { writeAccounts(file, "alice"); From d0efc9c0003bbe7ee3e65a0e8f0d7a322f367be7 Mon Sep 17 00:00:00 2001 From: maximthomas Date: Thu, 8 Oct 2026 16:51:15 +0300 Subject: [PATCH 06/24] [#157] Resolve a relative DTD of a store under a non-ASCII path --- .../connectors/xml/XMLHandlerImpl.java | 2 +- .../connectors/xml/XMLHandlerReloadTests.java | 37 +++++++++++++++++++ 2 files changed, 38 insertions(+), 1 deletion(-) 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 947a7b57..79cedd6c 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 @@ -546,7 +546,7 @@ private void loadDocument(File xmlFile) { CRC32 crc = new CRC32(); crc.update(content); InputSource source = new InputSource(new ByteArrayInputStream(content)); - source.setSystemId(xmlFile.toURI().toString()); + source.setSystemId(xmlFile.toURI().toASCIIString()); Document loaded = docBuilder.parse(source); // Same text shape as a saved document: CDATA and split runs become one text node, blank text goes. XmlDocumentWriter.normalizeText(loaded); diff --git a/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerReloadTests.java b/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerReloadTests.java index 8bacee27..6aa358f6 100644 --- a/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerReloadTests.java +++ b/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerReloadTests.java @@ -244,6 +244,43 @@ public void cdataValueOfALoadedFileIsRead() throws Exception { assertEquals(call(h -> lastNames(h.search(allAccounts(), ObjectClass.ACCOUNT))), List.of("Last-alice")); } + @Test + public void relativeDtdIsResolvedUnderANonAsciiDirectory() throws Exception { + File dir = new File(file.getParentFile(), "\u0434\u0438\u0440-\u00e9"); + try { + if (!dir.mkdir()) { + throw new SkipException("Cannot create a directory with a non-ASCII name: " + dir); + } + File store = new File(dir, "store.xml"); + File dtd = new File(dir, "store.dtd"); + Files.write(dtd.toPath(), "".getBytes(StandardCharsets.UTF_8)); + writeAccounts(store, "alice"); + String xml = new String(Files.readAllBytes(store.toPath()), StandardCharsets.UTF_8) + .replace("\nalice<", "&who;<"); + Files.write(store.toPath(), xml.getBytes(StandardCharsets.UTF_8)); + XMLHandler other = new ConcurrentXMLHandler(config(store), schemaParser().parseSchema(), schemaParser().getXsdSchema()); + other.init(); + try { + List found = new ArrayList(); + for (ConnectorObject object : other.search(allAccounts(), ObjectClass.ACCOUNT)) { + found.add(object.getName().getNameValue()); + } + assertEquals(found, List.of("alice")); + } finally { + other.dispose(); + } + } finally { + File[] children = dir.listFiles(); + if (children != null) { + for (File child : children) { + child.delete(); + } + } + dir.delete(); + } + } + @Test public void outsideEditDuringAChangeIsReportedAndOverwritten() throws Exception { writeAccounts(file, "alice"); From bd73b5ebcf3d138d3b3506124d0543aa727967d6 Mon Sep 17 00:00:00 2001 From: maximthomas Date: Thu, 8 Oct 2026 16:53:14 +0300 Subject: [PATCH 07/24] [#157] Load an existing store in the bundle IT --- .../openicf/connectors/xml/XMLConnectorBundleIT.java | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) 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 index 5e7e5be1..35ad9fac 100644 --- 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 @@ -21,6 +21,7 @@ import static org.testng.Assert.assertTrue; import java.io.File; +import java.util.Arrays; import java.util.Collections; import org.identityconnectors.common.security.GuardedString; @@ -51,12 +52,18 @@ public void bundleSavesLoadsAndFindsEntries() throws Exception { assertEquals(manager.getConnectorInfos().size(), 1); APIConfiguration api = manager.getConnectorInfos().get(0).createDefaultAPIConfiguration(); File xmlFile = getRandomXMLFile(); + // An existing store, so that the first call parses a file under the bundle's loader. + writeAccounts(xmlFile, "carol"); + setModified(xmlFile, System.currentTimeMillis() - 3_600_000L); ConfigurationProperties properties = api.getConfigurationProperties(); properties.setPropertyValue("xmlFilePath", xmlFile); properties.setPropertyValue("xsdFilePath", XSD_SCHEMA_FILEPATH); properties.setPropertyValue("createFileIfNotExists", true); ConnectorFacade facade = ConnectorFacadeFactory.getInstance().newInstance(api); + ConnectorObject carol = facade.getObject(ObjectClass.ACCOUNT, new Uid("uid-carol"), null); + assertEquals(carol.getName().getNameValue(), "carol"); + Uid alice = facade.create(ObjectClass.ACCOUNT, account("alice"), null); facade.create(ObjectClass.ACCOUNT, account("bob"), null); facade.update(ObjectClass.ACCOUNT, alice, @@ -64,7 +71,7 @@ public void bundleSavesLoadsAndFindsEntries() throws Exception { 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")); + assertEquals(namesInFile(xmlFile), Arrays.asList("carol", "alice")); ConnectorObject read = facade.getObject(ObjectClass.ACCOUNT, alice, null); assertEquals(AttributeUtil.getStringValue(read.getAttributeByName(ATTR_ACCOUNT_LAST_NAME)), "Changed"); } From 529d3c6ea9c9bd6353171e5572fc0abfd33ac499 Mon Sep 17 00:00:00 2001 From: maximthomas Date: Fri, 9 Oct 2026 10:47:19 +0300 Subject: [PATCH 08/24] [#157] Keep a store file that appears before a new document is saved --- .../connectors/xml/XMLHandlerImpl.java | 24 +++++- .../connectors/xml/XMLHandlerReloadTests.java | 76 +++++++++++++++++++ 2 files changed, 99 insertions(+), 1 deletion(-) 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 79cedd6c..d3d16a17 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 @@ -119,12 +119,30 @@ public XMLHandlerImpl(XMLConfiguration config, Schema connSchema, XSSchemaSet xs @Override public XMLHandler init() { - if (document == null || (!dirty && fileHasChanged())) { + if (document == null || fileAppearedOverNewDocument() || (!dirty && fileHasChanged())) { buildDocument(); } return this; } + /** + * A file that appears where a new document is not saved yet wins over the document: the + * connector never loaded it, so a save would replace a whole store. Changes made to the new + * document are dropped, and the next load reads the file. A partial write of a failed save is + * not such a file: the save takes the stamp again. + */ + private boolean fileAppearedOverNewDocument() { + File xmlFile = config.getXmlFilePath(); + if (!unsavedNewFile || !xmlFile.isFile() || stamp.sameState(FileStamp.read(xmlFile))) { + return false; + } + if (dirty) { + log.error("{0} appeared before the new document was saved; keeping the file and dropping the changes made to the new document", xmlFile); + dirty = false; + } + return true; + } + /** A racy stamp cannot rule out a same-size write in the same tick, so the content decides. */ private boolean fileHasChanged() { File xmlFile = config.getXmlFilePath(); @@ -398,6 +416,10 @@ public void dispose() { return; } File xmlFile = config.getXmlFilePath(); + if (fileAppearedOverNewDocument()) { + log.info("Exit {0}: {1} appeared before the new document was saved; not overwriting it", method, xmlFile); + return; + } if (!stamp.sameState(FileStamp.read(xmlFile))) { log.error("UPDATE COLLISION: {0} has changed since it was loaded or saved; overwriting it with the data in memory.", xmlFile); } diff --git a/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerReloadTests.java b/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerReloadTests.java index 6aa358f6..17bd477f 100644 --- a/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerReloadTests.java +++ b/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerReloadTests.java @@ -377,6 +377,82 @@ public void unsavedNewDocumentGivesWayToAFileThatAppears() throws Exception { assertEquals(Files.readAllBytes(file.toPath()), appeared); // loaded, not written back } + @Test + public void changedNewDocumentGivesWayToAFileThatAppears() throws Exception { + assertFalse(file.exists()); + handler.init(); // no file: a new document in memory + handler.create(ObjectClass.ACCOUNT, account("bob")); + assertTrue(file.mkdir()); + try { + captureStdErr(() -> expectThrows(ConnectorException.class, handler::dispose)); + } finally { + assertTrue(file.delete()); + } + + // A store appears (a restore, a late mount): the connector never loaded it, so a save would replace all of it. + writeAccounts(file, "alice", "carol"); + setModified(file, now() - HOUR); + byte[] appeared = Files.readAllBytes(file.toPath()); + String errors = captureStdErr(() -> assertEquals(names(), List.of("alice", "carol"))); + assertEquals(Files.readAllBytes(file.toPath()), appeared); + assertTrue(errors.contains(file + " appeared before the new document was saved"), errors); + } + + @Test + public void fileThatAppearsDuringAChangeToANewDocumentIsKept() throws Exception { + assertFalse(file.exists()); + handler.init(); // no file: a new document in memory + handler.create(ObjectClass.ACCOUNT, account("bob")); + writeAccounts(file, "alice", "carol"); + setModified(file, now() - HOUR); + byte[] appeared = Files.readAllBytes(file.toPath()); + String errors = captureStdErr(handler::dispose); + assertEquals(Files.readAllBytes(file.toPath()), appeared); + assertTrue(errors.contains(file + " appeared before the new document was saved"), errors); + assertEquals(names(), List.of("alice", "carol")); + } + + @Test + public void fileThatAppearsDuringTheFirstCallIsNotOverwritten() throws Exception { + assertFalse(file.exists()); + handler.init(); // no file: an empty new document + writeAccounts(file, "alice"); + setModified(file, now() - HOUR); + byte[] appeared = Files.readAllBytes(file.toPath()); + captureStdErr(handler::dispose); + assertEquals(Files.readAllBytes(file.toPath()), appeared); + assertEquals(names(), List.of("alice")); + } + + @Test + public void changeToANewDocumentIsSavedOnceThePathIsFree() throws Exception { + assertFalse(file.exists()); + handler.init(); // no file: a new document in memory + handler.create(ObjectClass.ACCOUNT, account("bob")); + assertTrue(file.mkdir()); + try { + captureStdErr(() -> expectThrows(ConnectorException.class, handler::dispose)); + } finally { + assertTrue(file.delete()); + } + + // The stamp changed, but no file appeared: the change is saved. + captureStdErr(() -> assertEquals(names(), List.of("bob"))); + assertEquals(namesInFile(file), List.of("bob")); + } + + @Test + public void ownPartialWriteOfANewDocumentIsSavedAgain() throws Exception { + XMLHandlerImpl impl = new XMLHandlerImpl(config(file), schemaParser().parseSchema(), schemaParser().getXsdSchema()); + impl.init(); // no file: a new document in memory + Set bob = account("bob"); + bob.add(AttributeBuilder.build(ATTR_ACCOUNT_FIRST_NAME, "x\uD800y")); // an unpaired surrogate: no encoder can write it + impl.create(ObjectClass.ACCOUNT, bob); + captureStdErr(() -> expectThrows(ConnectorException.class, impl::dispose)); + assertTrue(file.exists()); // the failed save left a partial file + captureStdErr(() -> expectThrows(ConnectorException.class, impl::dispose)); // not a file that appeared: saved again + } + @Test public void savedNewFileIsNotWrittenAgain() throws Exception { call(h -> null); // creates the file From 9661a0cb3c7d7ec397adfb6d2a7d615f5c3aa0cf Mon Sep 17 00:00:00 2001 From: maximthomas Date: Fri, 9 Oct 2026 10:49:19 +0300 Subject: [PATCH 09/24] [#157] Create a new store and load an outside edit in the bundle IT --- .../connectors/xml/XMLConnectorBundleIT.java | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) 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 index 35ad9fac..511101e1 100644 --- 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 @@ -51,19 +51,13 @@ public void bundleSavesLoadsAndFindsEntries() throws Exception { ConnectorInfoManager manager = ConnectorInfoManagerFactory.getInstance().getLocalManager(bundle.toURI().toURL()); assertEquals(manager.getConnectorInfos().size(), 1); APIConfiguration api = manager.getConnectorInfos().get(0).createDefaultAPIConfiguration(); - File xmlFile = getRandomXMLFile(); - // An existing store, so that the first call parses a file under the bundle's loader. - writeAccounts(xmlFile, "carol"); - setModified(xmlFile, System.currentTimeMillis() - 3_600_000L); + File xmlFile = getRandomXMLFile(); // no file yet: the first call creates a new document under the bundle's loader ConfigurationProperties properties = api.getConfigurationProperties(); properties.setPropertyValue("xmlFilePath", xmlFile); properties.setPropertyValue("xsdFilePath", XSD_SCHEMA_FILEPATH); properties.setPropertyValue("createFileIfNotExists", true); ConnectorFacade facade = ConnectorFacadeFactory.getInstance().newInstance(api); - ConnectorObject carol = facade.getObject(ObjectClass.ACCOUNT, new Uid("uid-carol"), null); - assertEquals(carol.getName().getNameValue(), "carol"); - Uid alice = facade.create(ObjectClass.ACCOUNT, account("alice"), null); facade.create(ObjectClass.ACCOUNT, account("bob"), null); facade.update(ObjectClass.ACCOUNT, alice, @@ -71,8 +65,14 @@ public void bundleSavesLoadsAndFindsEntries() throws Exception { Uid bob = facade.authenticate(ObjectClass.ACCOUNT, "bob", new GuardedString("secret-bob".toCharArray()), null); facade.delete(ObjectClass.ACCOUNT, bob, null); - assertEquals(namesInFile(xmlFile), Arrays.asList("carol", "alice")); + assertEquals(namesInFile(xmlFile), Arrays.asList("alice")); ConnectorObject read = facade.getObject(ObjectClass.ACCOUNT, alice, null); assertEquals(AttributeUtil.getStringValue(read.getAttributeByName(ATTR_ACCOUNT_LAST_NAME)), "Changed"); + + // An outside edit, so that a call parses a file under the bundle's loader. + writeAccounts(xmlFile, "carol"); + setModified(xmlFile, System.currentTimeMillis() - 3_600_000L); + ConnectorObject carol = facade.getObject(ObjectClass.ACCOUNT, new Uid("uid-carol"), null); + assertEquals(carol.getName().getNameValue(), "carol"); } } From e30c0aec3325e1dbce4b7040541064dbb87cf799 Mon Sep 17 00:00:00 2001 From: maximthomas Date: Fri, 9 Oct 2026 10:50:56 +0300 Subject: [PATCH 10/24] [#157] Pin that a whitespace-only value of a loaded file is absent --- .../openicf/connectors/xml/XMLHandlerReloadTests.java | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerReloadTests.java b/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerReloadTests.java index 17bd477f..ad44fafd 100644 --- a/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerReloadTests.java +++ b/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerReloadTests.java @@ -244,6 +244,17 @@ public void cdataValueOfALoadedFileIsRead() throws Exception { assertEquals(call(h -> lastNames(h.search(allAccounts(), ObjectClass.ACCOUNT))), List.of("Last-alice")); } + @Test + public void whitespaceOnlyValueOfALoadedFileIsAbsent() throws Exception { + writeAccounts(file, "alice"); + String xml = new String(Files.readAllBytes(file.toPath()), StandardCharsets.UTF_8) + .replace("Last-alice", " "); + Files.write(file.toPath(), xml.getBytes(StandardCharsets.UTF_8)); + setModified(file, now() - HOUR); + // As after a save and a reload: the blank text is gone, and so is the attribute. + assertEquals(call(h -> lastNames(h.search(allAccounts(), ObjectClass.ACCOUNT))), Collections.singletonList(null)); + } + @Test public void relativeDtdIsResolvedUnderANonAsciiDirectory() throws Exception { File dir = new File(file.getParentFile(), "\u0434\u0438\u0440-\u00e9"); From 4625e870638f44e183e14fc074be01c8addf8489 Mon Sep 17 00:00:00 2001 From: maximthomas Date: Fri, 9 Oct 2026 10:52:15 +0300 Subject: [PATCH 11/24] [#157] Skip the non-ASCII directory test where the path cannot be encoded --- .../openicf/connectors/xml/XMLHandlerReloadTests.java | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerReloadTests.java b/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerReloadTests.java index ad44fafd..b96813bb 100644 --- a/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerReloadTests.java +++ b/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerReloadTests.java @@ -24,10 +24,12 @@ import java.io.ByteArrayOutputStream; import java.io.File; +import java.io.IOException; import java.io.PrintStream; import java.io.UnsupportedEncodingException; import java.nio.charset.StandardCharsets; import java.nio.file.Files; +import java.nio.file.InvalidPathException; import java.nio.file.attribute.FileTime; import java.util.ArrayList; import java.util.Arrays; @@ -259,9 +261,12 @@ public void whitespaceOnlyValueOfALoadedFileIsAbsent() throws Exception { public void relativeDtdIsResolvedUnderANonAsciiDirectory() throws Exception { File dir = new File(file.getParentFile(), "\u0434\u0438\u0440-\u00e9"); try { - if (!dir.mkdir()) { - throw new SkipException("Cannot create a directory with a non-ASCII name: " + dir); - } + // A path the platform encoding cannot hold (Linux under LANG=C) throws here, before anything is created. + Files.createDirectory(dir.toPath()); + } catch (InvalidPathException | IOException e) { + throw new SkipException("Cannot create a directory with a non-ASCII name: " + dir, e); + } + try { File store = new File(dir, "store.xml"); File dtd = new File(dir, "store.dtd"); Files.write(dtd.toPath(), "".getBytes(StandardCharsets.UTF_8)); From 6533361bc4be0f2e70795c23e39bd074fbb9e13c Mon Sep 17 00:00:00 2001 From: maximthomas Date: Fri, 9 Oct 2026 10:52:25 +0300 Subject: [PATCH 12/24] [#157] Say what the XMLConnector class monitor no longer covers --- .../java/org/forgerock/openicf/connectors/xml/XMLConnector.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/XMLConnector.java b/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/XMLConnector.java index 22b5117b..6381c755 100644 --- a/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/XMLConnector.java +++ b/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/XMLConnector.java @@ -93,7 +93,7 @@ public void init(Configuration configuration) { log.error(ex, "Failed to get the CanonicalPath of {0}", config.getXmlFilePath()); throw new ConnectorIOException(ex); } - // Outside the class monitor: loading or saving one file must not hold up connectors on other files. + // Outside the class monitor: loading one file, or waiting while another call saves it, must not hold up connectors on other files. xmlInstanceHandler = handler.init(); log.info("XMLConnector initialized"); } From 37b4a3e8b670761dc516553370aeb52f0efe7bdd Mon Sep 17 00:00:00 2001 From: maximthomas Date: Fri, 9 Oct 2026 11:31:56 +0300 Subject: [PATCH 13/24] [#157] Pin the recent-past half of the racy window stampIsRacyOnlyNearTheClock used fixtures an hour away from the clock, so a racy check that covered only future modification times passed every test. The new test puts one fixture inside the window (500 ms ago) and one just past it (3 s ago). --- .../forgerock/openicf/connectors/xml/FileStampTests.java | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/FileStampTests.java b/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/FileStampTests.java index fa388d19..27a91d81 100644 --- a/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/FileStampTests.java +++ b/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/FileStampTests.java @@ -110,6 +110,15 @@ public void stampIsRacyOnlyNearTheClock() throws IOException { assertFalse(FileStamp.read(file).isRacy()); } + @Test + public void stampIsRacyJustAfterAWrite() throws IOException { + // An outside write of the same size in the same tick right after a load or save looks like no change. + File file = write(getRandomXMLFile(), "abc", now() - 500); + assertTrue(FileStamp.read(file).isRacy()); + setModified(file, now() - 3_000); + assertFalse(FileStamp.read(file).isRacy()); + } + @Test public void checksumFollowsTheContent() throws IOException { File file = write(getRandomXMLFile(), "abc", now() - HOUR); From 1b76414d65dddb5ec3c146d9d561b1e6709caaa3 Mon Sep 17 00:00:00 2001 From: maximthomas Date: Fri, 9 Oct 2026 11:32:41 +0300 Subject: [PATCH 14/24] [#157] Name the non-ASCII test directory after the test's store file A run killed before the cleanup left the fixed directory behind, and every later local run reported the only pin of the ASCII system id as SKIP until mvn clean. --- .../openicf/connectors/xml/XMLHandlerReloadTests.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerReloadTests.java b/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerReloadTests.java index b96813bb..0f2e66b4 100644 --- a/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerReloadTests.java +++ b/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerReloadTests.java @@ -259,7 +259,8 @@ public void whitespaceOnlyValueOfALoadedFileIsAbsent() throws Exception { @Test public void relativeDtdIsResolvedUnderANonAsciiDirectory() throws Exception { - File dir = new File(file.getParentFile(), "\u0434\u0438\u0440-\u00e9"); + // Unique per run: a directory left by a killed run would otherwise turn this test into a skip. + File dir = new File(file.getParentFile(), "\u0434\u0438\u0440-\u00e9-" + file.getName()); try { // A path the platform encoding cannot hold (Linux under LANG=C) throws here, before anything is created. Files.createDirectory(dir.toPath()); From 40104db2ca5c6300c6ed262b0da9690081ab01ab Mon Sep 17 00:00:00 2001 From: maximthomas Date: Fri, 9 Oct 2026 11:33:00 +0300 Subject: [PATCH 15/24] [#157] Do not load the partial write of an unchanged new document A save of a new document that failed after writing part of the file took that file's stamp but kept the old checksum. With nothing changed in memory, the next init() compared the content under the racy stamp, parsed the partial file and failed, on every later call. For a new document only a file that appears is a reason to reload, so the content check now applies to loaded documents only, and dispose() saves again. --- .../connectors/xml/XMLHandlerImpl.java | 7 ++++--- .../connectors/xml/XMLHandlerReloadTests.java | 20 +++++++++++++++++++ 2 files changed, 24 insertions(+), 3 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 d3d16a17..e0b806c4 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 @@ -119,7 +119,7 @@ public XMLHandlerImpl(XMLConfiguration config, Schema connSchema, XSSchemaSet xs @Override public XMLHandler init() { - if (document == null || fileAppearedOverNewDocument() || (!dirty && fileHasChanged())) { + if (document == null || fileAppearedOverNewDocument() || (!dirty && !unsavedNewFile && fileHasChanged())) { buildDocument(); } return this; @@ -128,8 +128,9 @@ public XMLHandler init() { /** * A file that appears where a new document is not saved yet wins over the document: the * connector never loaded it, so a save would replace a whole store. Changes made to the new - * document are dropped, and the next load reads the file. A partial write of a failed save is - * not such a file: the save takes the stamp again. + * document are dropped, and the next load reads the file. This is the only reload of a new + * document: a partial write of its failed save is not such a file, because the save takes the + * stamp again. */ private boolean fileAppearedOverNewDocument() { File xmlFile = config.getXmlFilePath(); diff --git a/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerReloadTests.java b/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerReloadTests.java index 0f2e66b4..486a5fbc 100644 --- a/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerReloadTests.java +++ b/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerReloadTests.java @@ -470,6 +470,26 @@ public void ownPartialWriteOfANewDocumentIsSavedAgain() throws Exception { captureStdErr(() -> expectThrows(ConnectorException.class, impl::dispose)); // not a file that appeared: saved again } + @Test + public void ownPartialWriteOfAnUnchangedNewDocumentIsNotLoaded() throws Exception { + writeAccounts(file, "alice"); + setModified(file, now() - HOUR); + XMLHandlerImpl impl = new XMLHandlerImpl(config(file), schemaParser().parseSchema(), schemaParser().getXsdSchema()); + impl.init(); // the checksum is now the loaded file's, which no partial write matches + Files.delete(file.toPath()); + impl.init(); // no file: a new document in memory, with no change to save but the file itself + org.w3c.dom.Element root = impl.getDocument().getDocumentElement(); + // An unpaired surrogate: no encoder can write it + org.w3c.dom.Node unwritable = root.appendChild(impl.getDocument().createTextNode("x\uD800y")); + captureStdErr(() -> expectThrows(ConnectorException.class, impl::dispose)); + assertTrue(file.exists()); // the failed save left a partial file + root.removeChild(unwritable); + + impl.init(); // not a file that appeared: not parsed + impl.dispose(); + assertEquals(namesInFile(file), List.of()); + } + @Test public void savedNewFileIsNotWrittenAgain() throws Exception { call(h -> null); // creates the file From bf79edd244eef927fb09242f1948e464a82bcb5f Mon Sep 17 00:00:00 2001 From: maximthomas Date: Fri, 9 Oct 2026 11:33:17 +0300 Subject: [PATCH 16/24] [#157] Drop the new document when a file appears over it The ERROR line said the changes made to the new document were dropped, but only the dirty flag was cleared: the document kept them until a load succeeded. If the file that appeared was then removed, or failed to parse and was removed, the next init() saw no change, served the dropped entries and saved them. The document is now dropped with the flag, so the next init() loads the file or starts a new document. --- .../openicf/connectors/xml/XMLHandlerImpl.java | 9 +++++---- .../connectors/xml/XMLHandlerReloadTests.java | 13 +++++++++++++ 2 files changed, 18 insertions(+), 4 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 e0b806c4..c6e4e0ab 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 @@ -127,10 +127,10 @@ public XMLHandler init() { /** * A file that appears where a new document is not saved yet wins over the document: the - * connector never loaded it, so a save would replace a whole store. Changes made to the new - * document are dropped, and the next load reads the file. This is the only reload of a new - * document: a partial write of its failed save is not such a file, because the save takes the - * stamp again. + * connector never loaded it, so a save would replace a whole store. The new document is + * dropped with its changes; the next init() loads the file, or starts a new document if the + * file is gone. This is the only reload of a new document: a partial write of its failed save + * is not such a file, because the save takes the stamp again. */ private boolean fileAppearedOverNewDocument() { File xmlFile = config.getXmlFilePath(); @@ -141,6 +141,7 @@ private boolean fileAppearedOverNewDocument() { log.error("{0} appeared before the new document was saved; keeping the file and dropping the changes made to the new document", xmlFile); dirty = false; } + document = null; return true; } diff --git a/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerReloadTests.java b/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerReloadTests.java index 486a5fbc..5a6c05c7 100644 --- a/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerReloadTests.java +++ b/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerReloadTests.java @@ -458,6 +458,19 @@ public void changeToANewDocumentIsSavedOnceThePathIsFree() throws Exception { assertEquals(namesInFile(file), List.of("bob")); } + @Test + public void changesDroppedForAFileThatAppearsStayDroppedWhenItGoes() throws Exception { + assertFalse(file.exists()); + handler.init(); // no file: a new document in memory + handler.create(ObjectClass.ACCOUNT, account("bob")); + writeAccounts(file, "alice"); + setModified(file, now() - HOUR); + captureStdErr(handler::dispose); // keeps the file and drops bob + Files.delete(file.toPath()); + assertEquals(names(), List.of()); + assertEquals(namesInFile(file), List.of()); + } + @Test public void ownPartialWriteOfANewDocumentIsSavedAgain() throws Exception { XMLHandlerImpl impl = new XMLHandlerImpl(config(file), schemaParser().parseSchema(), schemaParser().getXsdSchema()); From 3c4cfc17eefdf318f48a52deb62b4613c7934aad Mon Sep 17 00:00:00 2001 From: maximthomas Date: Fri, 9 Oct 2026 11:33:33 +0300 Subject: [PATCH 17/24] [#157] Report a collision only when a file is there to overwrite A save that failed on a directory at the store path took the directory's stamp. Once the directory was gone, the retry compared that stamp with the missing file and logged UPDATE COLLISION, though it only created the file. Recreating a missing file is not a collision here, as after a reload; changeToANewDocumentIsSavedOnceThePathIsFree now checks the retry's log. --- .../org/forgerock/openicf/connectors/xml/XMLHandlerImpl.java | 3 ++- .../openicf/connectors/xml/XMLHandlerReloadTests.java | 5 +++-- 2 files changed, 5 insertions(+), 3 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 c6e4e0ab..ce2cd34d 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 @@ -422,7 +422,8 @@ public void dispose() { log.info("Exit {0}: {1} appeared before the new document was saved; not overwriting it", method, xmlFile); return; } - if (!stamp.sameState(FileStamp.read(xmlFile))) { + // A path that holds no file has nothing to overwrite: the save creates the file. + if (xmlFile.isFile() && !stamp.sameState(FileStamp.read(xmlFile))) { log.error("UPDATE COLLISION: {0} has changed since it was loaded or saved; overwriting it with the data in memory.", xmlFile); } // Callers hold the handler's write lock. diff --git a/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerReloadTests.java b/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerReloadTests.java index 5a6c05c7..ea061804 100644 --- a/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerReloadTests.java +++ b/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerReloadTests.java @@ -453,8 +453,9 @@ public void changeToANewDocumentIsSavedOnceThePathIsFree() throws Exception { assertTrue(file.delete()); } - // The stamp changed, but no file appeared: the change is saved. - captureStdErr(() -> assertEquals(names(), List.of("bob"))); + // The stamp changed, but no file appeared: the change is saved, and no file is overwritten. + String errors = captureStdErr(() -> assertEquals(names(), List.of("bob"))); + assertFalse(errors.contains("UPDATE COLLISION"), errors); assertEquals(namesInFile(file), List.of("bob")); } From 5934cdd322add5a4eb849d6d86af7c5743465f04 Mon Sep 17 00:00:00 2001 From: maximthomas Date: Fri, 9 Oct 2026 19:22:55 +0300 Subject: [PATCH 18/24] [#157] Drop the new document at the call sites, not inside the check fileAppearedOverNewDocument() returned a boolean but also dropped the new document, cleared the dirty flag and logged the ERROR, and it ran inside init()'s || chain and dispose()'s guard: reordering init()'s arms, or one more call for logging, would have changed the state. It is now a plain check, and init() and dispose() drop the document through dropNewDocument(). Behaviour is unchanged. --- .../connectors/xml/XMLHandlerImpl.java | 26 +++++++++++-------- 1 file changed, 15 insertions(+), 11 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 ce2cd34d..c6fdefad 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 @@ -119,30 +119,33 @@ public XMLHandlerImpl(XMLConfiguration config, Schema connSchema, XSSchemaSet xs @Override public XMLHandler init() { - if (document == null || fileAppearedOverNewDocument() || (!dirty && !unsavedNewFile && fileHasChanged())) { + if (document != null && fileAppearedOverNewDocument()) { + dropNewDocument(); + } + if (document == null || (!dirty && !unsavedNewFile && fileHasChanged())) { buildDocument(); } return this; } /** - * A file that appears where a new document is not saved yet wins over the document: the - * connector never loaded it, so a save would replace a whole store. The new document is - * dropped with its changes; the next init() loads the file, or starts a new document if the - * file is gone. This is the only reload of a new document: a partial write of its failed save - * is not such a file, because the save takes the stamp again. + * Whether a file appeared where a new document is not saved yet. Such a file wins over the + * document: the connector never loaded it, so a save would replace a whole store. This is the + * only reload of a new document: a partial write of its failed save is not such a file, + * because the save takes the stamp again. */ private boolean fileAppearedOverNewDocument() { File xmlFile = config.getXmlFilePath(); - if (!unsavedNewFile || !xmlFile.isFile() || stamp.sameState(FileStamp.read(xmlFile))) { - return false; - } + return unsavedNewFile && xmlFile.isFile() && !stamp.sameState(FileStamp.read(xmlFile)); + } + + /** Drops the new document with its changes: the next init() loads the file, or starts a new document if the file is gone. */ + private void dropNewDocument() { if (dirty) { - log.error("{0} appeared before the new document was saved; keeping the file and dropping the changes made to the new document", xmlFile); + log.error("{0} appeared before the new document was saved; keeping the file and dropping the changes made to the new document", config.getXmlFilePath()); dirty = false; } document = null; - return true; } /** A racy stamp cannot rule out a same-size write in the same tick, so the content decides. */ @@ -419,6 +422,7 @@ public void dispose() { } File xmlFile = config.getXmlFilePath(); if (fileAppearedOverNewDocument()) { + dropNewDocument(); log.info("Exit {0}: {1} appeared before the new document was saved; not overwriting it", method, xmlFile); return; } From 03753ae3622b98c0e57b6ce5342beebc959374f7 Mon Sep 17 00:00:00 2001 From: maximthomas Date: Fri, 9 Oct 2026 19:24:14 +0300 Subject: [PATCH 19/24] [#157] Take a new document's stamp as missing, not from a second look buildDocument() starts a new document when exists() finds no file, and createDocument() then read the stamp again. A file that appeared between the two, while the parser factory loaded, became the stamp of the new document, so the first dispose() overwrote it with the empty document and logged nothing. The stamp is now MISSING, what the check found, and such a file wins like any file that appears before the first save. A new document can now hold an unreadable stamp only after a save of its own. --- .../connectors/xml/XMLHandlerImpl.java | 3 +- .../connectors/xml/XMLHandlerReloadTests.java | 32 +++++++++++++++++++ 2 files changed, 34 insertions(+), 1 deletion(-) 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 c6fdefad..09f9ed62 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 @@ -537,8 +537,9 @@ private void createDocument() { + icfSchema.getTargetNamespace() + " " + config.getXsdIcfFilePath()); } + // Not read again: a file that appeared since the check found none is not the connector's to overwrite. + stamp = FileStamp.MISSING; // Not a user change: if the first save fails, a file that appears afterwards is loaded instead. - stamp = FileStamp.read(config.getXmlFilePath()); unsavedNewFile = true; log.info("Exit {0}", method); } diff --git a/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerReloadTests.java b/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerReloadTests.java index ea061804..a4bafc7d 100644 --- a/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerReloadTests.java +++ b/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerReloadTests.java @@ -26,6 +26,7 @@ import java.io.File; import java.io.IOException; import java.io.PrintStream; +import java.io.UncheckedIOException; import java.io.UnsupportedEncodingException; import java.nio.charset.StandardCharsets; import java.nio.file.Files; @@ -441,6 +442,16 @@ public void fileThatAppearsDuringTheFirstCallIsNotOverwritten() throws Exception assertEquals(names(), List.of("alice")); } + @Test + public void fileThatAppearsAsTheNewDocumentIsCreatedIsNotOverwritten() throws Exception { + XMLHandlerImpl impl = new XMLHandlerImpl(config(new AppearingFile(file)), schemaParser().parseSchema(), schemaParser().getXsdSchema()); + impl.init(); // the check finds no file, and one appears right after it + byte[] appeared = Files.readAllBytes(file.toPath()); + impl.dispose(); + assertEquals(Files.readAllBytes(file.toPath()), appeared); + assertEquals(names(), List.of("alice")); + } + @Test public void changeToANewDocumentIsSavedOnceThePathIsFree() throws Exception { assertFalse(file.exists()); @@ -543,6 +554,27 @@ public void externalEditWithANewerMtimeIsLoaded() throws Exception { assertEquals(names(), List.of("bob")); } + /** A store path where a file appears as soon as a check finds none there. */ + private static final class AppearingFile extends File { + + AppearingFile(File file) { + super(file.getPath()); + } + + @Override + public boolean exists() { + if (super.exists()) { + return true; + } + try { + writeAccounts(this, "alice"); + } catch (IOException e) { + throw new UncheckedIOException(e); + } + return false; + } + } + /** Makes {@code file} unreadable, or skips the test where its owner can read it anyway (root, Windows). */ private static void makeUnreadable(File file) { if (!file.setReadable(false, false) || Files.isReadable(file.toPath())) { From d10cd2a11f6b29b28e1810867be8dd9460eae38b Mon Sep 17 00:00:00 2001 From: maximthomas Date: Fri, 9 Oct 2026 19:26:36 +0300 Subject: [PATCH 20/24] [#157] Keep a new document whose failed save left a file behind an unreadable stamp A failed save takes the stamp again, so its partial write does not count as a file that appeared. If that stamp could not be read (an IOException other than NoSuchFileException), it matched nothing: the next init() dropped the new document with the ERROR for an appeared file and parsed the partial write, which failed until someone fixed the file. Only a save leaves a new document with an unreadable stamp, so the file is now taken for its partial write and saved over; that retry logs UPDATE COLLISION, as the stamp cannot tell whose write the file holds. The test reads the stamp through a link to itself. --- .../openicf/connectors/xml/FileStamp.java | 5 ++ .../connectors/xml/XMLHandlerImpl.java | 5 +- .../connectors/xml/FileStampTests.java | 7 +++ .../connectors/xml/XMLHandlerReloadTests.java | 52 +++++++++++++++++++ 4 files changed, 67 insertions(+), 2 deletions(-) diff --git a/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/FileStamp.java b/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/FileStamp.java index 3ff6c492..7c24b7b0 100644 --- a/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/FileStamp.java +++ b/OpenICF-xml-connector/src/main/java/org/forgerock/openicf/connectors/xml/FileStamp.java @@ -90,6 +90,11 @@ boolean isRacy() { return racy; } + /** Whether the file could be read, or was known to be missing, when the stamp was taken. */ + boolean isKnown() { + return this != UNKNOWN; + } + boolean sameState(FileStamp other) { if (this == UNKNOWN || other == UNKNOWN) { return false; 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 09f9ed62..2e9133a3 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 @@ -132,11 +132,12 @@ public XMLHandler init() { * Whether a file appeared where a new document is not saved yet. Such a file wins over the * document: the connector never loaded it, so a save would replace a whole store. This is the * only reload of a new document: a partial write of its failed save is not such a file, - * because the save takes the stamp again. + * because the save takes the stamp again. If that stamp could not be read, the file is still + * taken for the partial write: only a save leaves a new document with such a stamp. */ private boolean fileAppearedOverNewDocument() { File xmlFile = config.getXmlFilePath(); - return unsavedNewFile && xmlFile.isFile() && !stamp.sameState(FileStamp.read(xmlFile)); + return unsavedNewFile && stamp.isKnown() && xmlFile.isFile() && !stamp.sameState(FileStamp.read(xmlFile)); } /** Drops the new document with its changes: the next init() loads the file, or starts a new document if the file is gone. */ diff --git a/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/FileStampTests.java b/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/FileStampTests.java index 27a91d81..85e3e951 100644 --- a/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/FileStampTests.java +++ b/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/FileStampTests.java @@ -95,6 +95,13 @@ public void unreadableStampMatchesNothing() throws IOException { assertFalse(FileStamp.read(loop).sameState(FileStamp.read(loop))); assertFalse(FileStamp.read(loop).sameState(FileStamp.MISSING)); assertFalse(FileStamp.MISSING.sameState(FileStamp.read(loop))); + assertFalse(FileStamp.read(loop).isKnown()); + } + + @Test + public void readableAndMissingFilesAreKnown() throws IOException { + assertTrue(FileStamp.read(write(getRandomXMLFile(), "abc", now() - HOUR)).isKnown()); + assertTrue(FileStamp.read(getRandomXMLFile()).isKnown()); } @Test diff --git a/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerReloadTests.java b/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerReloadTests.java index a4bafc7d..59ee7be1 100644 --- a/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerReloadTests.java +++ b/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerReloadTests.java @@ -31,6 +31,7 @@ import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.nio.file.InvalidPathException; +import java.nio.file.Path; import java.nio.file.attribute.FileTime; import java.util.ArrayList; import java.util.Arrays; @@ -515,6 +516,32 @@ public void ownPartialWriteOfAnUnchangedNewDocumentIsNotLoaded() throws Exceptio assertEquals(namesInFile(file), List.of()); } + @Test + public void ownPartialWriteBehindAnUnreadableStampIsSavedAgain() throws Exception { + StatFailingFile store = new StatFailingFile(file); + XMLHandlerImpl impl = new XMLHandlerImpl(config(store), schemaParser().parseSchema(), schemaParser().getXsdSchema()); + impl.init(); // no file: a new document in memory + impl.create(ObjectClass.ACCOUNT, account("bob")); + org.w3c.dom.Element root = impl.getDocument().getDocumentElement(); + // An unpaired surrogate: no encoder can write it + org.w3c.dom.Node unwritable = root.appendChild(impl.getDocument().createTextNode("x\uD800y")); + store.statFails = true; // the stamp the failed save takes cannot be read + try { + captureStdErr(() -> expectThrows(ConnectorException.class, impl::dispose)); + } finally { + store.statFails = false; + } + assertTrue(file.exists()); // the failed save left a partial file + root.removeChild(unwritable); + + String errors = captureStdErr(() -> { + impl.init(); // not a file that appeared: neither dropped nor parsed + impl.dispose(); + }); + assertFalse(errors.contains("appeared before the new document was saved"), errors); + assertEquals(namesInFile(file), List.of("bob")); + } + @Test public void savedNewFileIsNotWrittenAgain() throws Exception { call(h -> null); // creates the file @@ -575,6 +602,31 @@ public boolean exists() { } } + /** A store path whose stamp cannot be read while {@code statFails} is set: the stamp is then read through a link to itself. */ + private static final class StatFailingFile extends File { + + boolean statFails; + private final File loop; + + StatFailingFile(File file) { + super(file.getPath()); + loop = new File(file.getPath() + ".loop"); + try { + Files.createSymbolicLink(loop.toPath(), loop.toPath().getFileName()); + } catch (IOException | UnsupportedOperationException e) { + throw new SkipException("Cannot create a symbolic link here: " + e); + } + if (FileStamp.read(loop).isKnown()) { + throw new SkipException("The file system reads a link to itself as a file or as missing"); + } + } + + @Override + public Path toPath() { + return statFails ? loop.toPath() : super.toPath(); + } + } + /** Makes {@code file} unreadable, or skips the test where its owner can read it anyway (root, Windows). */ private static void makeUnreadable(File file) { if (!file.setReadable(false, false) || Files.isReadable(file.toPath())) { From 19962a699c79bb08525056268366ab0c9fb1b0ac Mon Sep 17 00:00:00 2001 From: maximthomas Date: Fri, 9 Oct 2026 19:28:24 +0300 Subject: [PATCH 21/24] [#157] Checksum the XML file as it is parsed, without a copy in memory loadDocument() read the whole file into a byte array, checksummed the array and parsed it, so a file-sized buffer stayed alive through the parse, next to the new DOM and the old one. The parser now reads the file through a CheckedInputStream. The checksum still covers exactly the bytes parsed, because Xerces reads on to the end of the file for what may follow the root; a parser that stopped earlier would cost reloads inside the racy window, not a missed change. The new test puts 100 KB of comment after the root. --- .../openicf/connectors/xml/XMLHandlerImpl.java | 15 +++++++++------ .../connectors/xml/XMLHandlerReloadTests.java | 14 ++++++++++++++ 2 files changed, 23 insertions(+), 6 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 2e9133a3..9bf1b9c9 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 @@ -37,9 +37,9 @@ import com.sun.xml.xsom.XSSchema; import com.sun.xml.xsom.XSSchemaSet; -import java.io.ByteArrayInputStream; import java.io.File; import java.io.IOException; +import java.io.InputStream; import java.nio.file.Files; import java.util.ArrayList; import java.util.Collection; @@ -49,6 +49,7 @@ import java.util.Set; import java.util.UUID; import java.util.zip.CRC32; +import java.util.zip.CheckedInputStream; import javax.xml.parsers.DocumentBuilder; import javax.xml.parsers.DocumentBuilderFactory; @@ -573,12 +574,14 @@ private void loadDocument(File xmlFile) { DocumentBuilder docBuilder = docBuilderFactory.newDocumentBuilder(); // Taken before reading: a write during the read shows as a change at the next init(). FileStamp loadedStamp = FileStamp.read(xmlFile); - byte[] content = Files.readAllBytes(xmlFile.toPath()); CRC32 crc = new CRC32(); - crc.update(content); - InputSource source = new InputSource(new ByteArrayInputStream(content)); - source.setSystemId(xmlFile.toURI().toASCIIString()); - Document loaded = docBuilder.parse(source); + Document loaded; + // The checksum is of the bytes parsed: the parser reads on to the end, past what follows the root. + try (InputStream in = new CheckedInputStream(Files.newInputStream(xmlFile.toPath()), crc)) { + InputSource source = new InputSource(in); + source.setSystemId(xmlFile.toURI().toASCIIString()); + loaded = docBuilder.parse(source); + } // Same text shape as a saved document: CDATA and split runs become one text node, blank text goes. XmlDocumentWriter.normalizeText(loaded); // Nothing changes unless the parse succeeded: a malformed file keeps failing until it is fixed. diff --git a/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerReloadTests.java b/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerReloadTests.java index 59ee7be1..c21f9b90 100644 --- a/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerReloadTests.java +++ b/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerReloadTests.java @@ -32,6 +32,7 @@ import java.nio.file.Files; import java.nio.file.InvalidPathException; import java.nio.file.Path; +import java.nio.file.StandardOpenOption; import java.nio.file.attribute.FileTime; import java.util.ArrayList; import java.util.Arrays; @@ -193,6 +194,19 @@ public void racyLoadIsNotParsedAgain() throws Exception { assertFalse(second.contains("Loading XML document from"), second); } + @Test + public void racyLoadWithMarkupAfterTheRootIsNotParsedAgain() throws Exception { + writeAccounts(file, "alice"); + // Far past any parser buffer: the checksum taken while parsing covers it only if the parser reads to the end. + String after = "\n\n"; + Files.write(file.toPath(), after.getBytes(StandardCharsets.UTF_8), StandardOpenOption.APPEND); + setModified(file, now() + HOUR); // racy for as long as the test runs + String first = captureStdOut(() -> names()); + assertTrue(first.contains("Loading XML document from: " + file.getPath()), first); + String second = captureStdOut(() -> names()); + assertFalse(second.contains("Loading XML document from"), second); + } + @Test public void unreadableFileBehindARacyStampFails() throws Exception { writeAccounts(file, "alice"); From 0736d4531373accb48cb212bcdd05752f26585ea Mon Sep 17 00:00:00 2001 From: maximthomas Date: Fri, 9 Oct 2026 19:34:39 +0300 Subject: [PATCH 22/24] [#157] Tell an unreadable link from the file system, not from isKnown() ownPartialWriteBehindAnUnreadableStampIsSavedAgain skipped when FileStamp.read() of its link to itself was known, so an isKnown() that always returned true made the test a SKIP instead of a failure. It now asks Files.readAttributes(), as unreadableStampMatchesNothing does. --- .../openicf/connectors/xml/XMLHandlerReloadTests.java | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerReloadTests.java b/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerReloadTests.java index c21f9b90..70263c4e 100644 --- a/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerReloadTests.java +++ b/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerReloadTests.java @@ -31,8 +31,10 @@ import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.nio.file.InvalidPathException; +import java.nio.file.NoSuchFileException; import java.nio.file.Path; import java.nio.file.StandardOpenOption; +import java.nio.file.attribute.BasicFileAttributes; import java.nio.file.attribute.FileTime; import java.util.ArrayList; import java.util.Arrays; @@ -630,8 +632,13 @@ private static final class StatFailingFile extends File { } catch (IOException | UnsupportedOperationException e) { throw new SkipException("Cannot create a symbolic link here: " + e); } - if (FileStamp.read(loop).isKnown()) { - throw new SkipException("The file system reads a link to itself as a file or as missing"); + try { + Files.readAttributes(loop.toPath(), BasicFileAttributes.class); + throw new SkipException("The file system resolves a link to itself"); + } catch (NoSuchFileException e) { + throw new SkipException("The file system reports a link to itself as missing"); + } catch (IOException expected) { + // ELOOP: the path is neither readable nor known to be missing } } From a3d352d9137127895d68bb2e2e4b6dd7a334cca6 Mon Sep 17 00:00:00 2001 From: maximthomas Date: Fri, 9 Oct 2026 19:37:26 +0300 Subject: [PATCH 23/24] [#157] Pin the stamp a new document starts from Since 946df85c the collision check skips a path that holds no file, so deletedFileIsRecreatedWithoutACollision no longer failed when createDocument() left the stamp of the deleted file in place: with the assignment removed, the whole module stayed green. A store moved away and back during the first call of the new document returns with that old stamp, and the new case keeps it only if the new document starts from MISSING. --- .../connectors/xml/XMLHandlerReloadTests.java | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerReloadTests.java b/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerReloadTests.java index 70263c4e..8b69fada 100644 --- a/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerReloadTests.java +++ b/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerReloadTests.java @@ -459,6 +459,19 @@ public void fileThatAppearsDuringTheFirstCallIsNotOverwritten() throws Exception assertEquals(names(), List.of("alice")); } + @Test + public void loadedStoreMovedBackBeforeTheNewDocumentIsSavedIsKept() throws Exception { + writeAccounts(file, "alice"); + setModified(file, now() - HOUR); + assertEquals(names(), List.of("alice")); + File moved = new File(file.getPath() + ".moved"); + assertTrue(file.renameTo(moved)); + handler.init(); // no file: a new document replaces the loaded one + assertTrue(moved.renameTo(file)); // the loaded file is back, with its file key, size and mtime + handler.dispose(); + assertEquals(namesInFile(file), List.of("alice")); + } + @Test public void fileThatAppearsAsTheNewDocumentIsCreatedIsNotOverwritten() throws Exception { XMLHandlerImpl impl = new XMLHandlerImpl(config(new AppearingFile(file)), schemaParser().parseSchema(), schemaParser().getXsdSchema()); From 0db055fe2f21bb1496b44a407c3edb0caec6571b Mon Sep 17 00:00:00 2001 From: maximthomas Date: Sun, 11 Oct 2026 11:11:01 +0300 Subject: [PATCH 24/24] [#157] Say which callers StatFailingFile diverts The fixture redirects every toPath(), not only the stamp's. ownPartialWriteBehindAnUnreadableStampIsSavedAgain works because XmlDocumentWriter.write opens a FileOutputStream on the path string and isFile() uses it too. A writer that opened toPath() would fail on the link before writing a byte, and the case would fail at assertTrue(file.exists()) with no hint why. --- .../openicf/connectors/xml/XMLHandlerReloadTests.java | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerReloadTests.java b/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerReloadTests.java index 8b69fada..ad216d4c 100644 --- a/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerReloadTests.java +++ b/OpenICF-xml-connector/src/test/java/org/forgerock/openicf/connectors/xml/XMLHandlerReloadTests.java @@ -631,7 +631,13 @@ public boolean exists() { } } - /** A store path whose stamp cannot be read while {@code statFails} is set: the stamp is then read through a link to itself. */ + /** + * A store path whose stamp cannot be read while {@code statFails} is set: the stamp is then read through a link to itself. + * Only callers of {@code toPath()} are diverted, such as {@code FileStamp}. {@code isFile()} and + * {@code XmlDocumentWriter.write}, which opens a {@code FileOutputStream} on the path string, still reach the store. + * A writer that opened {@code toPath()} would fail on the link before writing a byte, and the test would find no + * partial file. + */ private static final class StatFailingFile extends File { boolean statFails;