This is an automated email from the ASF dual-hosted git repository.
rmaucher pushed a commit to branch 9.0.x
in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/9.0.x by this push:
new b0fc9a461f Fix three issues from review
b0fc9a461f is described below
commit b0fc9a461f6bfafc769570a9c51ceaa2b6ab8c90
Author: remm <[email protected]>
AuthorDate: Thu Sep 17 14:09:27 2026 +0200
Fix three issues from review
Problem with identical elements/attributes, with text being different.
Don't discard comments before the root closing tag.
Mishandling of context files (I focused only on server.xml for testing,
using the admin webapp).
Co authored with OpenCode.
---
.../catalina/storeconfig/StandardContextSF.java | 14 ++--
.../catalina/storeconfig/XMLFormatPreserver.java | 28 +++++---
.../catalina/storeconfig/TestStoreConfig.java | 81 ++++++++++++++++++++++
.../storeconfig/TestXMLFormatPreserver.java | 46 ++++++++++++
4 files changed, 152 insertions(+), 17 deletions(-)
diff --git a/java/org/apache/catalina/storeconfig/StandardContextSF.java
b/java/org/apache/catalina/storeconfig/StandardContextSF.java
index da2c4445db..5d64479b2e 100644
--- a/java/org/apache/catalina/storeconfig/StandardContextSF.java
+++ b/java/org/apache/catalina/storeconfig/StandardContextSF.java
@@ -140,14 +140,16 @@ public class StandardContextSF extends StoreFactoryBase {
if (log.isInfoEnabled()) {
log.info(sm.getString("standardContextSF.storeContext",
aContext.getPath(), config));
}
+ // Generate the configuration in memory so that the layout of the
previous version of the file can
+ // be preserved. This has to happen before the output stream is
opened because opening the output
+ // stream truncates the file that XMLFormatPreserver reads the
previous layout from.
+ StringWriter buffer = new StringWriter();
+ storeXMLHead(new PrintWriter(buffer));
+ super.store(new PrintWriter(buffer), -2, aContext);
+ String formatted = XMLFormatPreserver.preserve(config,
buffer.toString(), getRegistry().getEncoding());
try (FileOutputStream fos = new FileOutputStream(config);
PrintWriter writer = new PrintWriter(new
OutputStreamWriter(fos, getRegistry().getEncoding()))) {
- // Generate the configuration in memory so that the layout of
the previous version of the file can
- // be preserved
- StringWriter buffer = new StringWriter();
- storeXMLHead(new PrintWriter(buffer));
- super.store(new PrintWriter(buffer), -2, aContext);
- writer.write(XMLFormatPreserver.preserve(config,
buffer.toString(), getRegistry().getEncoding()));
+ writer.write(formatted);
}
} else {
super.store(aWriter, indent, aContext);
diff --git a/java/org/apache/catalina/storeconfig/XMLFormatPreserver.java
b/java/org/apache/catalina/storeconfig/XMLFormatPreserver.java
index 041bbb4bd9..efdc7cce7f 100644
--- a/java/org/apache/catalina/storeconfig/XMLFormatPreserver.java
+++ b/java/org/apache/catalina/storeconfig/XMLFormatPreserver.java
@@ -148,13 +148,16 @@ public final class XMLFormatPreserver {
return newXml;
}
String lineSeparator = originalXml.contains("\r\n") ? "\r\n" : "\n";
+ // The roots have the same name, so they match. The preamble of the
root is emitted by emitElement()
+ // through this match, which also preserves the attribute order of the
root and the layout tokens before
+ // the closing tag of the root.
+ fresh.root.match = original.root;
matchChildren(original.root, fresh.root);
StringBuilder result = new StringBuilder(newXml.length() + 512);
result.append("<?xml version=\"1.0\"
encoding=\"").append(encoding).append("\"?>").append(lineSeparator);
if (original.doctype != null) {
result.append(original.doctype).append(lineSeparator);
}
- emitTokens(result, original.root.preamble, 0, lineSeparator);
emitElement(result, fresh.root, 0, lineSeparator);
emitTokens(result, original.trailing, 0, lineSeparator);
String formatted = result.toString();
@@ -264,17 +267,20 @@ public final class XMLFormatPreserver {
* @return The match score, 0 if nothing is shared
*/
private static int matchScore(XmlElement fresh, XmlElement candidate) {
- // Identical attribute sets (the order may differ) are the strongest
match
- if (attributesEqual(fresh, candidate)) {
- return 1000;
- }
int score = 0;
- for (int i = 0; i < fresh.attributeNames.size(); i++) {
- String name = fresh.attributeNames.get(i);
- String value = fresh.attributeValues.get(i);
- int index = candidate.attributeIndex(name);
- if (index >= 0 &&
candidate.attributeValues.get(index).equals(value)) {
- score += isKeyAttribute(name) ? 100 : 10;
+ // Identical attribute sets (the order may differ) are the strongest
match. The text is still scored on
+ // top of the attribute score so that repeated elements with an equal
(e.g. empty) attribute set, like
+ // WatchedResource, are matched by content instead of by position.
+ if (attributesEqual(fresh, candidate)) {
+ score = 1000;
+ } else {
+ for (int i = 0; i < fresh.attributeNames.size(); i++) {
+ String name = fresh.attributeNames.get(i);
+ String value = fresh.attributeValues.get(i);
+ int index = candidate.attributeIndex(name);
+ if (index >= 0 &&
candidate.attributeValues.get(index).equals(value)) {
+ score += isKeyAttribute(name) ? 100 : 10;
+ }
}
}
String freshText = fresh.getText();
diff --git a/test/org/apache/catalina/storeconfig/TestStoreConfig.java
b/test/org/apache/catalina/storeconfig/TestStoreConfig.java
index 072d99d2b3..c987a1ca84 100644
--- a/test/org/apache/catalina/storeconfig/TestStoreConfig.java
+++ b/test/org/apache/catalina/storeconfig/TestStoreConfig.java
@@ -28,7 +28,9 @@ import javax.xml.parsers.SAXParserFactory;
import org.junit.Assert;
import org.junit.Test;
+import org.apache.catalina.Context;
import org.apache.catalina.connector.Connector;
+import org.apache.catalina.core.StandardContext;
import org.apache.catalina.realm.LockOutRealm;
import org.apache.catalina.startup.Catalina;
import org.apache.catalina.startup.CatalinaBaseConfigurationSource;
@@ -232,4 +234,83 @@ public class TestStoreConfig extends TomcatBaseTest {
.parse(new InputSource(new StringReader(serverXmlDump)));
}
+ /**
+ * Verify that StoreConfig preserves the comments of the existing
context.xml when the context is stored to its
+ * configuration file without a backup. The output stream must not
truncate the file before XMLFormatPreserver
+ * has read the layout of the previous version from it.
+ *
+ * @throws Exception if the test experiences an unexpected error
+ */
+ @Test
+ public void testStoreContextSeparatePreservesComments() throws Exception {
+ Tomcat tomcat = getTomcatInstance();
+ StoreConfigLifecycleListener storeConfigListener = new
StoreConfigLifecycleListener();
+ tomcat.getServer().addLifecycleListener(storeConfigListener);
+
+ // Use a storable realm. The default embedded realm
(Tomcat.SimpleRealm) is an inner class that the store
+ // path cannot instantiate a default instance of.
+ tomcat.getEngine().setRealm(new LockOutRealm());
+
+ File appDir = new File(getTemporaryDirectory(), "webapps/test");
+ if (!appDir.mkdirs()) {
+ Assert.fail("Unable to create the webapp directory");
+ }
+ Context context = tomcat.addContext("/test", appDir.getAbsolutePath());
+ // WatchedResource values pointing at WEB-INF/web.xml are filtered out
when storing, so use a resource that
+ // survives the filtering
+ ((StandardContext) context).addWatchedResource("conf/test.txt");
+
+ // Write a context.xml with comments that StoreConfig must preserve
+ File conf = new File(getTemporaryDirectory(), "conf");
+ if (!conf.mkdirs()) {
+ Assert.fail("Unable to create conf directory");
+ }
+ addDeleteOnTearDown(conf);
+ File contextXml = new File(conf, "test.xml");
+ Files.write(contextXml.toPath(), String.join("\n",
+ "<?xml version=\"1.0\" encoding=\"UTF-8\"?>",
+ "<!-- Context comment -->",
+ "<Context reloadable=\"true\">",
+ " <!-- watched resources -->",
+ " <WatchedResource>conf/test.txt</WatchedResource>",
+ "</Context>",
+ "").getBytes(StandardCharsets.UTF_8));
+
+ tomcat.start();
+
+ context.setConfigFile(contextXml.toURI().toURL());
+
+ // Store the context to its configuration file without a backup (the
storeContextSeparate path)
+ IStoreConfig storeConfig = storeConfigListener.getStoreConfig();
+ StoreDescription desc =
storeConfig.getRegistry().findDescription(StandardContext.class);
+ Assert.assertNotNull(desc);
+ boolean oldSeparate = desc.isStoreSeparate();
+ boolean oldBackup = desc.isBackup();
+ boolean oldExternalAllowed = desc.isExternalAllowed();
+ try {
+ desc.setStoreSeparate(true);
+ desc.setBackup(false);
+ desc.setExternalAllowed(true);
+ desc.getStoreFactory().store(null, -2, context);
+ } finally {
+ desc.setStoreSeparate(oldSeparate);
+ desc.setBackup(oldBackup);
+ desc.setExternalAllowed(oldExternalAllowed);
+ }
+
+ // Read written configuration
+ String contextXmlDump;
+ try (FileReader reader = new FileReader(contextXml);
+ StringWriter writer = new StringWriter()) {
+ IOTools.flow(reader, writer);
+ contextXmlDump = writer.toString();
+ }
+ Assert.assertTrue(contextXmlDump, contextXmlDump.contains("Context
comment"));
+ Assert.assertTrue(contextXmlDump, contextXmlDump.contains("watched
resources"));
+ Assert.assertTrue(contextXmlDump,
contextXmlDump.contains("conf/test.txt"));
+ // The stored configuration must remain well-formed
+ SAXParserFactory.newInstance().newSAXParser().getXMLReader()
+ .parse(new InputSource(new StringReader(contextXmlDump)));
+ }
+
}
diff --git a/test/org/apache/catalina/storeconfig/TestXMLFormatPreserver.java
b/test/org/apache/catalina/storeconfig/TestXMLFormatPreserver.java
index 417845bc12..6f13af0ea3 100644
--- a/test/org/apache/catalina/storeconfig/TestXMLFormatPreserver.java
+++ b/test/org/apache/catalina/storeconfig/TestXMLFormatPreserver.java
@@ -223,6 +223,52 @@ public class TestXMLFormatPreserver {
Assert.assertEquals(expected, XMLFormatPreserver.preserve(originalXml,
newXml, "UTF-8"));
}
+ @Test
+ public void testRemovedFirstTextElementDoesNotMoveComment() {
+ String originalXml = String.join("\n",
+ "<Host name=\"localhost\">",
+ " <!-- web.xml -->",
+ " <WatchedResource>WEB-INF/web.xml</WatchedResource>",
+ " <WatchedResource>conf/context.xml</WatchedResource>",
+ "</Host>",
+ "");
+ String newXml = String.join("\n",
+ "<Host name=\"localhost\">",
+ " <WatchedResource>conf/context.xml</WatchedResource>",
+ "</Host>",
+ "");
+ String expected = String.join("\n",
+ "<?xml version=\"1.0\" encoding=\"UTF-8\"?>",
+ "<Host name=\"localhost\">",
+ " <WatchedResource>conf/context.xml</WatchedResource>",
+ "</Host>",
+ "");
+ Assert.assertEquals(expected, XMLFormatPreserver.preserve(originalXml,
newXml, "UTF-8"));
+ }
+
+ @Test
+ public void testRootElementLayoutPreserved() {
+ String originalXml = String.join("\n",
+ "<Context docBase=\"/foo\" reloadable=\"true\">",
+ " <WatchedResource>WEB-INF/web.xml</WatchedResource>",
+ " <!-- Trailing comment -->",
+ "</Context>",
+ "");
+ String newXml = String.join("\n",
+ "<Context reloadable=\"true\" docBase=\"/foo\">",
+ " <WatchedResource>WEB-INF/web.xml</WatchedResource>",
+ "</Context>",
+ "");
+ String expected = String.join("\n",
+ "<?xml version=\"1.0\" encoding=\"UTF-8\"?>",
+ "<Context docBase=\"/foo\" reloadable=\"true\">",
+ " <WatchedResource>WEB-INF/web.xml</WatchedResource>",
+ " <!-- Trailing comment -->",
+ "</Context>",
+ "");
+ Assert.assertEquals(expected, XMLFormatPreserver.preserve(originalXml,
newXml, "UTF-8"));
+ }
+
@Test
public void testEscaping() {
String originalXml = "<Server><Connector port=\"8080\" note=\"a &
b < c\"/></Server>";
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
