You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@camel.apache.org by "Croway (via GitHub)" <gi...@apache.org> on 2026/09/11 08:43:23 UTC
[PR] CAMEL-24684: camel-groovy - reuse the groovyXml parser configuration and write documents once [camel]
Croway opened a new pull request, #26305:
URL: https://github.com/apache/camel/pull/26305
`groovyXml` created a new `XmlParser` per unmarshal (a `SAXParserFactory` class-path lookup plus feature setup, where the JDK builds a throwaway parser for every `setFeature`): 61% of the CPU and 64% of the bytes of a 1 KB parse. Marshal wrote one `String.getBytes()` and one `write()` per fragment with the platform charset and ran the `TypeConverter` on every value.
This keeps one configured `SAXParserFactory` per data format with exactly the features Groovy's constructor sets (namespace aware, non-validating, secure processing, `disallow-doctype-decl`) and a `SAXParser` per thread wrapped by a cheap `new XmlParser(saxParser)` per call; marshal renders the document in memory and writes it once with the exchange charset, and skips the converter for String/Number/Boolean values.
**Measured.** 1 KB unmarshal 33.2 → 10.8 µs (3.1×); marshal 1.1–1.2× faster and 33× on an unbuffered stream; 1 MB unmarshal unchanged (the parse dominates). A DOCTYPE/XXE document is still rejected with the same exception; marshal output is byte-identical. Tests cover reuse, 8-thread parallel unmarshal and a UTF-16BE charset.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
_Claude Code on behalf of Croway_
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-24684: camel-groovy - reuse the groovyXml parser configuration and write documents once [camel]
Posted by "Croway (via GitHub)" <gi...@apache.org>.
Croway commented on code in PR #26305:
URL: https://github.com/apache/camel/pull/26305#discussion_r4003592921
##########
components/camel-groovy/src/main/java/org/apache/camel/groovy/xml/GroovyXmlDataFormat.java:
##########
@@ -78,70 +112,137 @@ public void marshal(Exchange exchange, Object graph, OutputStream stream) throws
@Override
public Object unmarshal(Exchange exchange, InputStream stream) throws Exception {
- XmlParser parser = new XmlParser();
- return parser.parse(stream);
+ SAXParser parser = parsers.poll();
+ if (parser == null) {
+ parser = createSaxParser();
+ }
+ try {
+ XmlParser xmlParser = new XmlParser(parser);
+ // XmlParser(SAXParser) does not set this, the no-arg constructor does: keep namespace prefixes on the QNames
+ xmlParser.setNamespaceAware(true);
+ return xmlParser.parse(stream);
+ } finally {
+ parser.getXMLReader().setContentHandler(DETACHED);
+ parsers.offer(parser);
+ }
}
@Override
public String getDataFormatName() {
return "groovyXml";
}
- private void serialize(Node node, OutputStream os) {
- PrintWriter pw = new PrintWriter(os);
+ @Override
+ protected void doStart() throws Exception {
+ getSaxParserFactory();
+ }
+
+ @Override
+ protected void doStop() throws Exception {
+ parsers.clear();
+ }
+
+ private SAXParserFactory getSaxParserFactory() throws ParserConfigurationException {
+ SAXParserFactory factory = saxParserFactory;
+ if (factory == null) {
+ synchronized (this) {
+ factory = saxParserFactory;
+ if (factory == null) {
+ // the same setup as the no-arg XmlParser constructor: secure processing and DOCTYPE disallowed
+ factory = FactorySupport.createSaxParserFactory();
+ factory.setNamespaceAware(true);
+ factory.setValidating(false);
+ XmlUtil.setFeatureQuietly(factory, XMLConstants.FEATURE_SECURE_PROCESSING, true);
+ XmlUtil.setFeatureQuietly(factory, "http://apache.org/xml/features/disallow-doctype-decl", true);
+ saxParserFactory = factory;
+ }
+ }
+ }
+ return factory;
+ }
+
+ private SAXParser createSaxParser() {
+ try {
+ SAXParserFactory factory = getSaxParserFactory();
+ // a factory is not guaranteed to be thread safe, and a thread only creates one parser
+ synchronized (factory) {
+ return factory.newSAXParser();
+ }
+ } catch (ParserConfigurationException | SAXException e) {
+ throw new RuntimeCamelException(e);
+ }
+ }
+
+ private void serialize(Exchange exchange, Node node, OutputStream os) {
+ // XmlNodePrinter writes no XML declaration, so the document must be UTF-8 to be self-describing
+ PrintWriter pw = new PrintWriter(new OutputStreamWriter(os, StandardCharsets.UTF_8));
XmlNodePrinter nodePrinter = new XmlNodePrinter(pw);
nodePrinter.setPreserveWhitespace(true);
nodePrinter.print(node);
}
- private void printLines(List<Line> lines, OutputStream os) throws Exception {
+ private void printLines(List<Line> lines, Writer w) throws IOException {
// add missing root end tag
lines.add(new Line(lines.get(0).key, null, END_TAG, null));
int level = 0;
for (Line line : lines) {
int kind = line.kind;
if (kind == START_TAG) {
- String pad = StringHelper.padString(level);
- os.write(pad.getBytes());
- os.write("<".getBytes());
- os.write(line.key.getBytes());
+ w.write(StringHelper.padString(level));
+ w.write('<');
+ w.write(line.key);
if (line.attrs != null) {
StringJoiner sj = new StringJoiner(" ");
for (var a : line.attrs.entrySet()) {
sj.add(a.getKey() + "=\"" + a.getValue() + "\"");
}
if (sj.length() > 0) {
- os.write(" ".getBytes());
- os.write(sj.toString().getBytes());
+ w.write(' ');
+ w.write(sj.toString());
}
}
- os.write(">\n".getBytes());
+ w.write(">\n");
level++;
} else if (kind == END_TAG) {
level--;
- String pad = StringHelper.padString(level);
- os.write(pad.getBytes());
- os.write("</".getBytes());
- os.write(line.key.getBytes());
- os.write(">\n".getBytes());
+ w.write(StringHelper.padString(level));
+ w.write("</");
+ w.write(line.key);
+ w.write(">\n");
} else {
- String pad = StringHelper.padString(level);
- os.write(pad.getBytes());
- os.write("<".getBytes());
- os.write(line.key.getBytes());
- os.write(">".getBytes());
- os.write(line.value.getBytes());
- os.write("</".getBytes());
- os.write(line.key.getBytes());
- os.write(">\n".getBytes());
+ w.write(StringHelper.padString(level));
+ w.write('<');
+ w.write(line.key);
+ w.write('>');
+ w.write(line.value);
+ w.write("</");
+ w.write(line.key);
+ w.write(">\n");
}
}
}
private void serialize(Exchange exchange, Map<String, Object> map, OutputStream os) throws Exception {
List<Line> lines = new ArrayList<>();
doSerialize(exchange.getContext(), map, lines);
- printLines(lines, os);
+ // render in memory and write once: a Writer over the stream costs 16 KB of buffers per call, which is more
+ // than a typical document, and the Marshal EIP already writes into a memory stream
+ StringWriter w = new StringWriter(lines.size() * 32);
+ printLines(lines, w);
+ os.write(w.toString().getBytes(StandardCharsets.UTF_8));
+ }
+
+ private static String asString(CamelContext context, Object value) {
+ if (value instanceof String s) {
+ return s;
+ }
+ // final JDK types no user converter can target: the type converter would return toString() for them
+ if (value instanceof Integer || value instanceof Long || value instanceof Double || value instanceof Boolean
+ || value instanceof Short || value instanceof Byte || value instanceof Float
+ || value instanceof BigDecimal || value instanceof BigInteger) {
Review Comment:
Applied in 8fffdc7ef8c4.
_Claude Code on behalf of Croway_
##########
components/camel-groovy/src/main/java/org/apache/camel/groovy/xml/GroovyXmlDataFormat.java:
##########
@@ -78,70 +112,137 @@ public void marshal(Exchange exchange, Object graph, OutputStream stream) throws
@Override
public Object unmarshal(Exchange exchange, InputStream stream) throws Exception {
- XmlParser parser = new XmlParser();
- return parser.parse(stream);
+ SAXParser parser = parsers.poll();
+ if (parser == null) {
+ parser = createSaxParser();
+ }
+ try {
+ XmlParser xmlParser = new XmlParser(parser);
+ // XmlParser(SAXParser) does not set this, the no-arg constructor does: keep namespace prefixes on the QNames
+ xmlParser.setNamespaceAware(true);
+ return xmlParser.parse(stream);
+ } finally {
+ parser.getXMLReader().setContentHandler(DETACHED);
+ parsers.offer(parser);
+ }
}
@Override
public String getDataFormatName() {
return "groovyXml";
}
- private void serialize(Node node, OutputStream os) {
- PrintWriter pw = new PrintWriter(os);
+ @Override
+ protected void doStart() throws Exception {
+ getSaxParserFactory();
+ }
+
+ @Override
+ protected void doStop() throws Exception {
+ parsers.clear();
+ }
+
+ private SAXParserFactory getSaxParserFactory() throws ParserConfigurationException {
+ SAXParserFactory factory = saxParserFactory;
+ if (factory == null) {
+ synchronized (this) {
+ factory = saxParserFactory;
+ if (factory == null) {
+ // the same setup as the no-arg XmlParser constructor: secure processing and DOCTYPE disallowed
+ factory = FactorySupport.createSaxParserFactory();
+ factory.setNamespaceAware(true);
+ factory.setValidating(false);
+ XmlUtil.setFeatureQuietly(factory, XMLConstants.FEATURE_SECURE_PROCESSING, true);
+ XmlUtil.setFeatureQuietly(factory, "http://apache.org/xml/features/disallow-doctype-decl", true);
+ saxParserFactory = factory;
+ }
+ }
+ }
+ return factory;
+ }
+
+ private SAXParser createSaxParser() {
+ try {
+ SAXParserFactory factory = getSaxParserFactory();
+ // a factory is not guaranteed to be thread safe, and a thread only creates one parser
+ synchronized (factory) {
+ return factory.newSAXParser();
+ }
+ } catch (ParserConfigurationException | SAXException e) {
+ throw new RuntimeCamelException(e);
+ }
+ }
+
+ private void serialize(Exchange exchange, Node node, OutputStream os) {
Review Comment:
Applied in 8fffdc7ef8c4.
_Claude Code on behalf of Croway_
##########
components/camel-groovy/src/test/java/org/apache/camel/groovy/xml/GroovyXmlDataFormatReuseTest.java:
##########
@@ -0,0 +1,218 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.camel.groovy.xml;
+
+import java.io.ByteArrayInputStream;
+import java.io.ByteArrayOutputStream;
+import java.io.OutputStreamWriter;
+import java.io.PrintWriter;
+import java.nio.charset.StandardCharsets;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+import java.util.concurrent.TimeUnit;
+
+import org.xml.sax.SAXParseException;
+
+import groovy.namespace.QName;
+import groovy.util.Node;
+import groovy.xml.XmlNodePrinter;
+import groovy.xml.XmlParser;
+import org.apache.camel.CamelContext;
+import org.apache.camel.Exchange;
+import org.apache.camel.impl.DefaultCamelContext;
+import org.apache.camel.support.DefaultExchange;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+import static org.junit.jupiter.api.Assertions.assertArrayEquals;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+/**
+ * The groovyXml data format reuses its SAX parser configuration and writes with the exchange charset.
Review Comment:
Applied in 8fffdc7ef8c4.
_Claude Code on behalf of Croway_
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-24684: camel-groovy - reuse the groovyXml parser configuration and write documents once [camel]
Posted by "Croway (via GitHub)" <gi...@apache.org>.
Croway commented on code in PR #26305:
URL: https://github.com/apache/camel/pull/26305#discussion_r3990404498
##########
components/camel-groovy/src/main/java/org/apache/camel/groovy/xml/GroovyXmlDataFormat.java:
##########
@@ -78,70 +108,122 @@ public void marshal(Exchange exchange, Object graph, OutputStream stream) throws
@Override
public Object unmarshal(Exchange exchange, InputStream stream) throws Exception {
- XmlParser parser = new XmlParser();
- return parser.parse(stream);
+ SAXParser parser = saxParser.get();
+ try {
+ return new XmlParser(parser).parse(stream);
Review Comment:
Confirmed and fixed in 5bb1deff3d44: namespaceAware is set on the XmlParser wrapper, and testNamespacePrefixesSurviveTheRoundTrip unmarshals a prefixed document, checks the QName prefix, and marshals it back to bytes identical to new XmlParser() + XmlNodePrinter.
_Claude Code on behalf of Croway_
##########
components/camel-groovy/src/main/java/org/apache/camel/groovy/xml/GroovyXmlDataFormat.java:
##########
@@ -45,8 +60,23 @@ public class GroovyXmlDataFormat extends ServiceSupport implements DataFormat, D
private static final int VALUE = 2;
private static final int END_TAG = 3;
+ // replaces the XmlParser of a thread as content handler of its SAX parser after a parse, so the parsed document
+ // is not retained by the parser
+ private static final DefaultHandler DETACHED = new DefaultHandler();
+
private boolean attributeMapping = true;
+ /**
+ * Configured like the factory of {@code new XmlParser()} (secure processing, DOCTYPE disallowed, namespace aware,
+ * not validating), created once as its lookup and feature setup dominate the cost of parsing small documents.
+ */
+ private volatile SAXParserFactory saxParserFactory;
+
+ /**
+ * A SAX parser is not thread safe but can parse sequentially, so each thread reuses its own.
+ */
+ private final ThreadLocal<SAXParser> saxParser = ThreadLocal.withInitial(this::createSaxParser);
Review Comment:
Applied in 5bb1deff3d44: parsers are borrowed from a ConcurrentLinkedQueue for one unmarshal and returned in the finally, so at most one parser exists per concurrent unmarshal, and doStop() drains the queue.
_Claude Code on behalf of Croway_
##########
components/camel-groovy/src/main/java/org/apache/camel/groovy/xml/GroovyXmlDataFormat.java:
##########
@@ -78,70 +108,122 @@ public void marshal(Exchange exchange, Object graph, OutputStream stream) throws
@Override
public Object unmarshal(Exchange exchange, InputStream stream) throws Exception {
- XmlParser parser = new XmlParser();
- return parser.parse(stream);
+ SAXParser parser = saxParser.get();
+ try {
+ return new XmlParser(parser).parse(stream);
+ } finally {
+ parser.getXMLReader().setContentHandler(DETACHED);
+ }
}
@Override
public String getDataFormatName() {
return "groovyXml";
}
- private void serialize(Node node, OutputStream os) {
- PrintWriter pw = new PrintWriter(os);
+ @Override
+ protected void doStart() throws Exception {
+ getSaxParserFactory();
+ }
+
+ private SAXParserFactory getSaxParserFactory() throws ParserConfigurationException {
+ SAXParserFactory factory = saxParserFactory;
+ if (factory == null) {
+ synchronized (this) {
+ factory = saxParserFactory;
+ if (factory == null) {
+ // the same setup as the no-arg XmlParser constructor: secure processing and DOCTYPE disallowed
+ factory = FactorySupport.createSaxParserFactory();
+ factory.setNamespaceAware(true);
+ factory.setValidating(false);
+ XmlUtil.setFeatureQuietly(factory, XMLConstants.FEATURE_SECURE_PROCESSING, true);
+ XmlUtil.setFeatureQuietly(factory, "http://apache.org/xml/features/disallow-doctype-decl", true);
+ saxParserFactory = factory;
+ }
+ }
+ }
+ return factory;
+ }
+
+ private SAXParser createSaxParser() {
+ try {
+ SAXParserFactory factory = getSaxParserFactory();
+ // a factory is not guaranteed to be thread safe, and a thread only creates one parser
+ synchronized (factory) {
+ return factory.newSAXParser();
+ }
+ } catch (ParserConfigurationException | SAXException e) {
+ throw new RuntimeCamelException(e);
+ }
+ }
+
+ private void serialize(Exchange exchange, Node node, OutputStream os) {
+ PrintWriter pw = new PrintWriter(new OutputStreamWriter(os, ExchangeHelper.getCharset(exchange, true)));
XmlNodePrinter nodePrinter = new XmlNodePrinter(pw);
nodePrinter.setPreserveWhitespace(true);
nodePrinter.print(node);
}
- private void printLines(List<Line> lines, OutputStream os) throws Exception {
+ private void printLines(List<Line> lines, Writer w) throws IOException {
// add missing root end tag
lines.add(new Line(lines.get(0).key, null, END_TAG, null));
int level = 0;
for (Line line : lines) {
int kind = line.kind;
if (kind == START_TAG) {
- String pad = StringHelper.padString(level);
- os.write(pad.getBytes());
- os.write("<".getBytes());
- os.write(line.key.getBytes());
+ w.write(StringHelper.padString(level));
+ w.write('<');
+ w.write(line.key);
if (line.attrs != null) {
StringJoiner sj = new StringJoiner(" ");
for (var a : line.attrs.entrySet()) {
sj.add(a.getKey() + "=\"" + a.getValue() + "\"");
}
if (sj.length() > 0) {
- os.write(" ".getBytes());
- os.write(sj.toString().getBytes());
+ w.write(' ');
+ w.write(sj.toString());
}
}
- os.write(">\n".getBytes());
+ w.write(">\n");
level++;
} else if (kind == END_TAG) {
level--;
- String pad = StringHelper.padString(level);
- os.write(pad.getBytes());
- os.write("</".getBytes());
- os.write(line.key.getBytes());
- os.write(">\n".getBytes());
+ w.write(StringHelper.padString(level));
+ w.write("</");
+ w.write(line.key);
+ w.write(">\n");
} else {
- String pad = StringHelper.padString(level);
- os.write(pad.getBytes());
- os.write("<".getBytes());
- os.write(line.key.getBytes());
- os.write(">".getBytes());
- os.write(line.value.getBytes());
- os.write("</".getBytes());
- os.write(line.key.getBytes());
- os.write(">\n".getBytes());
+ w.write(StringHelper.padString(level));
+ w.write('<');
+ w.write(line.key);
+ w.write('>');
+ w.write(line.value);
+ w.write("</");
+ w.write(line.key);
+ w.write(">\n");
}
}
}
private void serialize(Exchange exchange, Map<String, Object> map, OutputStream os) throws Exception {
List<Line> lines = new ArrayList<>();
doSerialize(exchange.getContext(), map, lines);
- printLines(lines, os);
+ // render in memory and write once: a Writer over the stream costs 16 KB of buffers per call, which is more
+ // than a typical document, and the Marshal EIP already writes into a memory stream
+ StringWriter w = new StringWriter(lines.size() * 32);
+ printLines(lines, w);
+ os.write(w.toString().getBytes(ExchangeHelper.getCharset(exchange, true)));
+ }
+
+ private static String asString(CamelContext context, Object value) {
Review Comment:
Applied in 5bb1deff3d44: the shortcut now covers String and the final JDK types only (Integer, Long, Double, Float, Short, Byte, Boolean, BigDecimal, BigInteger); everything else goes through the type converter as before.
_Claude Code on behalf of Croway_
##########
components/camel-groovy/src/test/java/org/apache/camel/groovy/xml/GroovyXmlDataFormatReuseTest.java:
##########
@@ -0,0 +1,197 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.camel.groovy.xml;
+
+import java.io.ByteArrayInputStream;
+import java.io.ByteArrayOutputStream;
+import java.nio.charset.StandardCharsets;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+import java.util.concurrent.TimeUnit;
+
+import org.xml.sax.SAXParseException;
+
+import groovy.util.Node;
+import groovy.xml.XmlParser;
+import org.apache.camel.CamelContext;
+import org.apache.camel.Exchange;
+import org.apache.camel.impl.DefaultCamelContext;
+import org.apache.camel.support.DefaultExchange;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+import static org.junit.jupiter.api.Assertions.assertArrayEquals;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+
+/**
+ * The groovyXml data format reuses its SAX parser configuration and writes with the exchange charset.
+ */
+public class GroovyXmlDataFormatReuseTest {
+
+ private static final String BOOKS = """
+ <library>
+ <book id="bk101"><title>No Title</title></book>
+ <book id="bk102"><title>1984</title></book>
+ </library>
+ """;
+
+ private static final String XXE = """
+ <?xml version="1.0"?>
+ <!DOCTYPE foo [<!ENTITY xxe SYSTEM "file:///etc/passwd">]>
+ <foo>&xxe;</foo>
+ """;
+
+ // the output of the data format before the marshal path was changed to write through a Writer
+ private static final String EXPECTED_XML = """
+ <library>
+ <book id="bk101">
+ <title>No Title</title>
+ <year>1925</year>
+ <available>true</available>
+ <note>Ünïcödé € 中</note>
+ <price>12.5</price>
+ </book>
+ <book id="bk102" lang="en">
+ <title>1984</title>
+ <tags>
+ </tags>
+ <tags>
+ </tags>
+ <name>Città</name>
+ <country>
+ <code>IT</code>
+ </country>
+ </book>
+ <name>Biblioteca</name>
+ </library>
+ """;
+
+ private CamelContext context;
+ private GroovyXmlDataFormat dataFormat;
+
+ @BeforeEach
+ public void setUp() throws Exception {
+ context = new DefaultCamelContext();
+ context.start();
+ dataFormat = new GroovyXmlDataFormat();
+ dataFormat.start();
+ }
+
+ @AfterEach
+ public void tearDown() {
+ dataFormat.stop();
+ context.stop();
+ }
+
+ private Node unmarshal(String xml) throws Exception {
+ return (Node) dataFormat.unmarshal(new DefaultExchange(context),
+ new ByteArrayInputStream(xml.getBytes(StandardCharsets.UTF_8)));
+ }
+
+ @Test
+ public void testDoctypeIsRejectedLikeXmlParser() throws Exception {
+ SAXParseException expected = assertThrows(SAXParseException.class, () -> new XmlParser().parseText(XXE));
+ SAXParseException actual = assertThrows(SAXParseException.class, () -> unmarshal(XXE));
+ assertEquals(expected.getMessage(), actual.getMessage());
+
+ // the parser of the thread is still usable after a failed parse
+ assertEquals(2, unmarshal(BOOKS).children().size());
+ }
+
+ @Test
+ public void testParserIsReusedSequentially() throws Exception {
+ for (int i = 0; i < 3; i++) {
+ Node library = unmarshal(BOOKS);
+ assertEquals(2, library.children().size());
+ assertEquals("bk102", ((Node) library.children().get(1)).attribute("id"));
+ }
+ }
+
+ @Test
+ public void testUnmarshalConcurrently() throws Exception {
+ ExecutorService pool = Executors.newFixedThreadPool(8);
+ try {
+ List<Future<?>> futures = new ArrayList<>();
+ for (int i = 0; i < 8; i++) {
+ futures.add(pool.submit(() -> {
+ for (int j = 0; j < 20; j++) {
+ Node library = unmarshal(BOOKS);
+ assertEquals(2, library.children().size());
+ assertEquals("No Title", ((Node) library.children().get(0)).text());
+ }
+ return null;
+ }));
+ }
+ for (Future<?> f : futures) {
+ f.get(30, TimeUnit.SECONDS);
+ }
+ } finally {
+ pool.shutdownNow();
+ }
+ }
+
+ @Test
+ public void testMarshalMapIsByteIdentical() throws Exception {
+ ByteArrayOutputStream bos = new ByteArrayOutputStream();
+ dataFormat.marshal(new DefaultExchange(context), library(), bos);
+ assertArrayEquals(EXPECTED_XML.getBytes(StandardCharsets.UTF_8), bos.toByteArray());
+ }
+
+ @Test
+ public void testMarshalUsesExchangeCharset() throws Exception {
+ Exchange exchange = new DefaultExchange(context);
+ exchange.setProperty(Exchange.CHARSET_NAME, "UTF-16BE");
+ ByteArrayOutputStream bos = new ByteArrayOutputStream();
+ dataFormat.marshal(exchange, library(), bos);
+ assertArrayEquals(EXPECTED_XML.getBytes(StandardCharsets.UTF_16BE), bos.toByteArray());
Review Comment:
Went with your option 3 in 5bb1deff3d44: both XML paths write UTF-8 explicitly (which is what main produced on any JDK 18+), the exchange charset is not consulted, and the UTF-16BE test is gone. Honouring CamelCharsetName for groovyXml would need an XML declaration to be correct, and that is a separate decision from this PR.
_Claude Code on behalf of Croway_
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-24684: camel-groovy - reuse the groovyXml parser configuration and write documents once [camel]
Posted by "davsclaus (via GitHub)" <gi...@apache.org>.
davsclaus commented on code in PR #26305:
URL: https://github.com/apache/camel/pull/26305#discussion_r3989897911
##########
components/camel-groovy/src/test/java/org/apache/camel/groovy/xml/GroovyXmlDataFormatReuseTest.java:
##########
@@ -0,0 +1,197 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.camel.groovy.xml;
+
+import java.io.ByteArrayInputStream;
+import java.io.ByteArrayOutputStream;
+import java.nio.charset.StandardCharsets;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+import java.util.concurrent.TimeUnit;
+
+import org.xml.sax.SAXParseException;
+
+import groovy.util.Node;
+import groovy.xml.XmlParser;
+import org.apache.camel.CamelContext;
+import org.apache.camel.Exchange;
+import org.apache.camel.impl.DefaultCamelContext;
+import org.apache.camel.support.DefaultExchange;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+import static org.junit.jupiter.api.Assertions.assertArrayEquals;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+
+/**
+ * The groovyXml data format reuses its SAX parser configuration and writes with the exchange charset.
+ */
+public class GroovyXmlDataFormatReuseTest {
+
+ private static final String BOOKS = """
+ <library>
+ <book id="bk101"><title>No Title</title></book>
+ <book id="bk102"><title>1984</title></book>
+ </library>
+ """;
+
+ private static final String XXE = """
+ <?xml version="1.0"?>
+ <!DOCTYPE foo [<!ENTITY xxe SYSTEM "file:///etc/passwd">]>
+ <foo>&xxe;</foo>
+ """;
+
+ // the output of the data format before the marshal path was changed to write through a Writer
+ private static final String EXPECTED_XML = """
+ <library>
+ <book id="bk101">
+ <title>No Title</title>
+ <year>1925</year>
+ <available>true</available>
+ <note>Ünïcödé € 中</note>
+ <price>12.5</price>
+ </book>
+ <book id="bk102" lang="en">
+ <title>1984</title>
+ <tags>
+ </tags>
+ <tags>
+ </tags>
+ <name>Città</name>
+ <country>
+ <code>IT</code>
+ </country>
+ </book>
+ <name>Biblioteca</name>
+ </library>
+ """;
+
+ private CamelContext context;
+ private GroovyXmlDataFormat dataFormat;
+
+ @BeforeEach
+ public void setUp() throws Exception {
+ context = new DefaultCamelContext();
+ context.start();
+ dataFormat = new GroovyXmlDataFormat();
+ dataFormat.start();
+ }
+
+ @AfterEach
+ public void tearDown() {
+ dataFormat.stop();
+ context.stop();
+ }
+
+ private Node unmarshal(String xml) throws Exception {
+ return (Node) dataFormat.unmarshal(new DefaultExchange(context),
+ new ByteArrayInputStream(xml.getBytes(StandardCharsets.UTF_8)));
+ }
+
+ @Test
+ public void testDoctypeIsRejectedLikeXmlParser() throws Exception {
+ SAXParseException expected = assertThrows(SAXParseException.class, () -> new XmlParser().parseText(XXE));
+ SAXParseException actual = assertThrows(SAXParseException.class, () -> unmarshal(XXE));
+ assertEquals(expected.getMessage(), actual.getMessage());
+
+ // the parser of the thread is still usable after a failed parse
+ assertEquals(2, unmarshal(BOOKS).children().size());
+ }
+
+ @Test
+ public void testParserIsReusedSequentially() throws Exception {
+ for (int i = 0; i < 3; i++) {
+ Node library = unmarshal(BOOKS);
+ assertEquals(2, library.children().size());
+ assertEquals("bk102", ((Node) library.children().get(1)).attribute("id"));
+ }
+ }
+
+ @Test
+ public void testUnmarshalConcurrently() throws Exception {
+ ExecutorService pool = Executors.newFixedThreadPool(8);
+ try {
+ List<Future<?>> futures = new ArrayList<>();
+ for (int i = 0; i < 8; i++) {
+ futures.add(pool.submit(() -> {
+ for (int j = 0; j < 20; j++) {
+ Node library = unmarshal(BOOKS);
+ assertEquals(2, library.children().size());
+ assertEquals("No Title", ((Node) library.children().get(0)).text());
+ }
+ return null;
+ }));
+ }
+ for (Future<?> f : futures) {
+ f.get(30, TimeUnit.SECONDS);
+ }
+ } finally {
+ pool.shutdownNow();
+ }
+ }
+
+ @Test
+ public void testMarshalMapIsByteIdentical() throws Exception {
+ ByteArrayOutputStream bos = new ByteArrayOutputStream();
+ dataFormat.marshal(new DefaultExchange(context), library(), bos);
+ assertArrayEquals(EXPECTED_XML.getBytes(StandardCharsets.UTF_8), bos.toByteArray());
+ }
+
+ @Test
+ public void testMarshalUsesExchangeCharset() throws Exception {
+ Exchange exchange = new DefaultExchange(context);
+ exchange.setProperty(Exchange.CHARSET_NAME, "UTF-16BE");
+ ByteArrayOutputStream bos = new ByteArrayOutputStream();
+ dataFormat.marshal(exchange, library(), bos);
+ assertArrayEquals(EXPECTED_XML.getBytes(StandardCharsets.UTF_16BE), bos.toByteArray());
Review Comment:
This test pins something worth a second look: the bytes it asserts are valid UTF-16BE, but nothing in them says so.
`XmlNodePrinter` writes no `<?xml version="1.0" encoding="UTF-16BE"?>` declaration (I checked the Groovy source — it never emits one), and `String.getBytes("UTF-16BE")` writes no BOM. So the marshalled document is UTF-16BE with no in-band encoding signal at all.
Per the XML spec an entity with no declaration and no BOM must be read as UTF-8, so a conforming parser on the other end will not just mis-decode this — it will fail outright on the first byte pair. The `Map` path has the same shape.
Three ways out, in rough order of how much I like them:
1. **Emit a declaration when the resolved charset is not UTF-8.** Self-describing output, and the common UTF-8 case is unchanged byte-for-byte — `testMarshalMapIsByteIdentical` keeps passing as written.
2. **Write a BOM for the UTF-16/UTF-32 families** (`"UTF-16"` rather than `"UTF-16BE"` does this for you). Standards-legal, but leaves every other non-UTF-8 charset undeclared.
3. **Keep UTF-8 for the XML paths** and treat honouring `CamelCharsetName` as out of scope for `groovyXml`. Smallest change, and arguably the honest one given point 3 below.
Whichever you pick, the test should assert the *chosen* contract rather than the current fallthrough, since right now it reads as "UTF-16BE output is supported and correct" and a downstream parser will disagree.
Two things that feed into the choice:
- **The header steers this too.** `ExchangeHelper.getCharset(exchange, true)` reads the `CamelCharsetName` *header* before the exchange property, so an upstream file or HTTP consumer can now change the encoding of a marshal several steps later in the route. Previously nothing in the exchange could influence these data formats. That is standard Camel convention so I am not objecting, but it widens what "supported" has to mean.
- **The fallback change buys less than it looks like.** `Charset.defaultCharset()` is already UTF-8 on JDK 18+, and Camel's fallback is `org.apache.camel.default.charset` defaulting to UTF-8, so the two agree on any modern JDK unless someone sets `-Dfile.encoding` explicitly. The real win is narrower than the PR implies — which is part of why I would like the scope of this change stated rather than assumed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-24684: camel-groovy - reuse the groovyXml parser configuration and write documents once [camel]
Posted by "Croway (via GitHub)" <gi...@apache.org>.
Croway merged PR #26305:
URL: https://github.com/apache/camel/pull/26305
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-24684: camel-groovy - reuse the groovyXml parser configuration and write documents once [camel]
Posted by "davsclaus (via GitHub)" <gi...@apache.org>.
davsclaus commented on code in PR #26305:
URL: https://github.com/apache/camel/pull/26305#discussion_r4002339540
##########
components/camel-groovy/src/main/java/org/apache/camel/groovy/xml/GroovyXmlDataFormat.java:
##########
@@ -78,70 +112,137 @@ public void marshal(Exchange exchange, Object graph, OutputStream stream) throws
@Override
public Object unmarshal(Exchange exchange, InputStream stream) throws Exception {
- XmlParser parser = new XmlParser();
- return parser.parse(stream);
+ SAXParser parser = parsers.poll();
+ if (parser == null) {
+ parser = createSaxParser();
+ }
+ try {
+ XmlParser xmlParser = new XmlParser(parser);
+ // XmlParser(SAXParser) does not set this, the no-arg constructor does: keep namespace prefixes on the QNames
+ xmlParser.setNamespaceAware(true);
+ return xmlParser.parse(stream);
+ } finally {
+ parser.getXMLReader().setContentHandler(DETACHED);
+ parsers.offer(parser);
+ }
}
@Override
public String getDataFormatName() {
return "groovyXml";
}
- private void serialize(Node node, OutputStream os) {
- PrintWriter pw = new PrintWriter(os);
+ @Override
+ protected void doStart() throws Exception {
+ getSaxParserFactory();
+ }
+
+ @Override
+ protected void doStop() throws Exception {
+ parsers.clear();
+ }
+
+ private SAXParserFactory getSaxParserFactory() throws ParserConfigurationException {
+ SAXParserFactory factory = saxParserFactory;
+ if (factory == null) {
+ synchronized (this) {
+ factory = saxParserFactory;
+ if (factory == null) {
+ // the same setup as the no-arg XmlParser constructor: secure processing and DOCTYPE disallowed
+ factory = FactorySupport.createSaxParserFactory();
+ factory.setNamespaceAware(true);
+ factory.setValidating(false);
+ XmlUtil.setFeatureQuietly(factory, XMLConstants.FEATURE_SECURE_PROCESSING, true);
+ XmlUtil.setFeatureQuietly(factory, "http://apache.org/xml/features/disallow-doctype-decl", true);
+ saxParserFactory = factory;
+ }
+ }
+ }
+ return factory;
+ }
+
+ private SAXParser createSaxParser() {
+ try {
+ SAXParserFactory factory = getSaxParserFactory();
+ // a factory is not guaranteed to be thread safe, and a thread only creates one parser
+ synchronized (factory) {
+ return factory.newSAXParser();
+ }
+ } catch (ParserConfigurationException | SAXException e) {
+ throw new RuntimeCamelException(e);
+ }
+ }
+
+ private void serialize(Exchange exchange, Node node, OutputStream os) {
+ // XmlNodePrinter writes no XML declaration, so the document must be UTF-8 to be self-describing
+ PrintWriter pw = new PrintWriter(new OutputStreamWriter(os, StandardCharsets.UTF_8));
XmlNodePrinter nodePrinter = new XmlNodePrinter(pw);
nodePrinter.setPreserveWhitespace(true);
nodePrinter.print(node);
}
- private void printLines(List<Line> lines, OutputStream os) throws Exception {
+ private void printLines(List<Line> lines, Writer w) throws IOException {
// add missing root end tag
lines.add(new Line(lines.get(0).key, null, END_TAG, null));
int level = 0;
for (Line line : lines) {
int kind = line.kind;
if (kind == START_TAG) {
- String pad = StringHelper.padString(level);
- os.write(pad.getBytes());
- os.write("<".getBytes());
- os.write(line.key.getBytes());
+ w.write(StringHelper.padString(level));
+ w.write('<');
+ w.write(line.key);
if (line.attrs != null) {
StringJoiner sj = new StringJoiner(" ");
for (var a : line.attrs.entrySet()) {
sj.add(a.getKey() + "=\"" + a.getValue() + "\"");
}
if (sj.length() > 0) {
- os.write(" ".getBytes());
- os.write(sj.toString().getBytes());
+ w.write(' ');
+ w.write(sj.toString());
}
}
- os.write(">\n".getBytes());
+ w.write(">\n");
level++;
} else if (kind == END_TAG) {
level--;
- String pad = StringHelper.padString(level);
- os.write(pad.getBytes());
- os.write("</".getBytes());
- os.write(line.key.getBytes());
- os.write(">\n".getBytes());
+ w.write(StringHelper.padString(level));
+ w.write("</");
+ w.write(line.key);
+ w.write(">\n");
} else {
- String pad = StringHelper.padString(level);
- os.write(pad.getBytes());
- os.write("<".getBytes());
- os.write(line.key.getBytes());
- os.write(">".getBytes());
- os.write(line.value.getBytes());
- os.write("</".getBytes());
- os.write(line.key.getBytes());
- os.write(">\n".getBytes());
+ w.write(StringHelper.padString(level));
+ w.write('<');
+ w.write(line.key);
+ w.write('>');
+ w.write(line.value);
+ w.write("</");
+ w.write(line.key);
+ w.write(">\n");
}
}
}
private void serialize(Exchange exchange, Map<String, Object> map, OutputStream os) throws Exception {
List<Line> lines = new ArrayList<>();
doSerialize(exchange.getContext(), map, lines);
- printLines(lines, os);
+ // render in memory and write once: a Writer over the stream costs 16 KB of buffers per call, which is more
+ // than a typical document, and the Marshal EIP already writes into a memory stream
+ StringWriter w = new StringWriter(lines.size() * 32);
+ printLines(lines, w);
+ os.write(w.toString().getBytes(StandardCharsets.UTF_8));
+ }
+
+ private static String asString(CamelContext context, Object value) {
+ if (value instanceof String s) {
+ return s;
+ }
+ // final JDK types no user converter can target: the type converter would return toString() for them
+ if (value instanceof Integer || value instanceof Long || value instanceof Double || value instanceof Boolean
+ || value instanceof Short || value instanceof Byte || value instanceof Float
+ || value instanceof BigDecimal || value instanceof BigInteger) {
Review Comment:
Small leftover from the previous round: `BigDecimal` and `BigInteger` are not final (`public class BigDecimal extends Number`), so a `Money extends BigDecimal` with its own `@Converter` to `String` would still be short-circuited here. An exact-class check for those two keeps the fast path for the plain types and lets subclasses reach the converter:
```suggestion
// final JDK types (or exact BigDecimal/BigInteger, which are not final) no user converter can target:
// the type converter would return toString() for them
if (value instanceof Integer || value instanceof Long || value instanceof Double || value instanceof Boolean
|| value instanceof Short || value instanceof Byte || value instanceof Float
|| value.getClass() == BigDecimal.class || value.getClass() == BigInteger.class) {
```
##########
components/camel-groovy/src/main/java/org/apache/camel/groovy/xml/GroovyXmlDataFormat.java:
##########
@@ -78,70 +112,137 @@ public void marshal(Exchange exchange, Object graph, OutputStream stream) throws
@Override
public Object unmarshal(Exchange exchange, InputStream stream) throws Exception {
- XmlParser parser = new XmlParser();
- return parser.parse(stream);
+ SAXParser parser = parsers.poll();
+ if (parser == null) {
+ parser = createSaxParser();
+ }
+ try {
+ XmlParser xmlParser = new XmlParser(parser);
+ // XmlParser(SAXParser) does not set this, the no-arg constructor does: keep namespace prefixes on the QNames
+ xmlParser.setNamespaceAware(true);
+ return xmlParser.parse(stream);
+ } finally {
+ parser.getXMLReader().setContentHandler(DETACHED);
+ parsers.offer(parser);
+ }
}
@Override
public String getDataFormatName() {
return "groovyXml";
}
- private void serialize(Node node, OutputStream os) {
- PrintWriter pw = new PrintWriter(os);
+ @Override
+ protected void doStart() throws Exception {
+ getSaxParserFactory();
+ }
+
+ @Override
+ protected void doStop() throws Exception {
+ parsers.clear();
+ }
+
+ private SAXParserFactory getSaxParserFactory() throws ParserConfigurationException {
+ SAXParserFactory factory = saxParserFactory;
+ if (factory == null) {
+ synchronized (this) {
+ factory = saxParserFactory;
+ if (factory == null) {
+ // the same setup as the no-arg XmlParser constructor: secure processing and DOCTYPE disallowed
+ factory = FactorySupport.createSaxParserFactory();
+ factory.setNamespaceAware(true);
+ factory.setValidating(false);
+ XmlUtil.setFeatureQuietly(factory, XMLConstants.FEATURE_SECURE_PROCESSING, true);
+ XmlUtil.setFeatureQuietly(factory, "http://apache.org/xml/features/disallow-doctype-decl", true);
+ saxParserFactory = factory;
+ }
+ }
+ }
+ return factory;
+ }
+
+ private SAXParser createSaxParser() {
+ try {
+ SAXParserFactory factory = getSaxParserFactory();
+ // a factory is not guaranteed to be thread safe, and a thread only creates one parser
+ synchronized (factory) {
+ return factory.newSAXParser();
+ }
+ } catch (ParserConfigurationException | SAXException e) {
+ throw new RuntimeCamelException(e);
+ }
+ }
+
+ private void serialize(Exchange exchange, Node node, OutputStream os) {
Review Comment:
`exchange` is no longer used here now that the charset lookup is gone — leftover from the previous iteration. Dropping it (and the argument at the call site on line 97) avoids an unused-parameter smell.
```suggestion
private void serialize(Node node, OutputStream os) {
```
##########
components/camel-groovy/src/test/java/org/apache/camel/groovy/xml/GroovyXmlDataFormatReuseTest.java:
##########
@@ -0,0 +1,218 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.camel.groovy.xml;
+
+import java.io.ByteArrayInputStream;
+import java.io.ByteArrayOutputStream;
+import java.io.OutputStreamWriter;
+import java.io.PrintWriter;
+import java.nio.charset.StandardCharsets;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+import java.util.concurrent.TimeUnit;
+
+import org.xml.sax.SAXParseException;
+
+import groovy.namespace.QName;
+import groovy.util.Node;
+import groovy.xml.XmlNodePrinter;
+import groovy.xml.XmlParser;
+import org.apache.camel.CamelContext;
+import org.apache.camel.Exchange;
+import org.apache.camel.impl.DefaultCamelContext;
+import org.apache.camel.support.DefaultExchange;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+import static org.junit.jupiter.api.Assertions.assertArrayEquals;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+/**
+ * The groovyXml data format reuses its SAX parser configuration and writes with the exchange charset.
Review Comment:
Stale after option 3 — the exchange charset is deliberately not consulted any more.
```suggestion
* The groovyXml data format reuses its SAX parser configuration and writes UTF-8.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-24684: camel-groovy - reuse the groovyXml parser configuration and write documents once [camel]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #26305:
URL: https://github.com/apache/camel/pull/26305#issuecomment-5631823272
:star2: Thank you for your contribution to the Apache Camel project! :star2:
:robot: CI automation will test this PR automatically.
:camel: Apache Camel Committers, please review the following items:
* First-time contributors **require MANUAL approval** for the GitHub Actions to run
* You can use the command `/component-test (camel-)component-name1 (camel-)component-name2..` to request a test from the test bot although they are normally detected and executed by CI.
* You can label PRs using `skip-tests` and `test-dependents` to fine-tune the checks executed by this PR.
* Build and test logs are available in the summary page. **Only** [Apache Camel committers](https://camel.apache.org/community/team/#committers) have access to the summary.
:warning: Be careful when sharing logs. Review their contents before sharing them publicly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-24684: camel-groovy - reuse the groovyXml parser configuration and write documents once [camel]
Posted by "Croway (via GitHub)" <gi...@apache.org>.
Croway commented on PR #26305:
URL: https://github.com/apache/camel/pull/26305#issuecomment-5636342995
Thanks, all four points addressed in 5bb1deff3d44: namespace prefixes are kept (round-trip test added), the parsers are pooled in a queue drained on stop instead of a ThreadLocal, the converter shortcut is limited to final JDK types, and the output stays UTF-8 (option 3), so there is no charset change and nothing for the upgrade guide. 65 tests in the module.
_Claude Code on behalf of Croway_
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-24684: camel-groovy - reuse the groovyXml parser configuration and write documents once [camel]
Posted by "davsclaus (via GitHub)" <gi...@apache.org>.
davsclaus commented on code in PR #26305:
URL: https://github.com/apache/camel/pull/26305#discussion_r3989762908
##########
components/camel-groovy/src/main/java/org/apache/camel/groovy/xml/GroovyXmlDataFormat.java:
##########
@@ -78,70 +108,122 @@ public void marshal(Exchange exchange, Object graph, OutputStream stream) throws
@Override
public Object unmarshal(Exchange exchange, InputStream stream) throws Exception {
- XmlParser parser = new XmlParser();
- return parser.parse(stream);
+ SAXParser parser = saxParser.get();
+ try {
+ return new XmlParser(parser).parse(stream);
+ } finally {
+ parser.getXMLReader().setContentHandler(DETACHED);
+ }
}
@Override
public String getDataFormatName() {
return "groovyXml";
}
- private void serialize(Node node, OutputStream os) {
- PrintWriter pw = new PrintWriter(os);
+ @Override
+ protected void doStart() throws Exception {
+ getSaxParserFactory();
+ }
+
+ private SAXParserFactory getSaxParserFactory() throws ParserConfigurationException {
+ SAXParserFactory factory = saxParserFactory;
+ if (factory == null) {
+ synchronized (this) {
+ factory = saxParserFactory;
+ if (factory == null) {
+ // the same setup as the no-arg XmlParser constructor: secure processing and DOCTYPE disallowed
+ factory = FactorySupport.createSaxParserFactory();
+ factory.setNamespaceAware(true);
+ factory.setValidating(false);
+ XmlUtil.setFeatureQuietly(factory, XMLConstants.FEATURE_SECURE_PROCESSING, true);
+ XmlUtil.setFeatureQuietly(factory, "http://apache.org/xml/features/disallow-doctype-decl", true);
+ saxParserFactory = factory;
+ }
+ }
+ }
+ return factory;
+ }
+
+ private SAXParser createSaxParser() {
+ try {
+ SAXParserFactory factory = getSaxParserFactory();
+ // a factory is not guaranteed to be thread safe, and a thread only creates one parser
+ synchronized (factory) {
+ return factory.newSAXParser();
+ }
+ } catch (ParserConfigurationException | SAXException e) {
+ throw new RuntimeCamelException(e);
+ }
+ }
+
+ private void serialize(Exchange exchange, Node node, OutputStream os) {
+ PrintWriter pw = new PrintWriter(new OutputStreamWriter(os, ExchangeHelper.getCharset(exchange, true)));
XmlNodePrinter nodePrinter = new XmlNodePrinter(pw);
nodePrinter.setPreserveWhitespace(true);
nodePrinter.print(node);
}
- private void printLines(List<Line> lines, OutputStream os) throws Exception {
+ private void printLines(List<Line> lines, Writer w) throws IOException {
// add missing root end tag
lines.add(new Line(lines.get(0).key, null, END_TAG, null));
int level = 0;
for (Line line : lines) {
int kind = line.kind;
if (kind == START_TAG) {
- String pad = StringHelper.padString(level);
- os.write(pad.getBytes());
- os.write("<".getBytes());
- os.write(line.key.getBytes());
+ w.write(StringHelper.padString(level));
+ w.write('<');
+ w.write(line.key);
if (line.attrs != null) {
StringJoiner sj = new StringJoiner(" ");
for (var a : line.attrs.entrySet()) {
sj.add(a.getKey() + "=\"" + a.getValue() + "\"");
}
if (sj.length() > 0) {
- os.write(" ".getBytes());
- os.write(sj.toString().getBytes());
+ w.write(' ');
+ w.write(sj.toString());
}
}
- os.write(">\n".getBytes());
+ w.write(">\n");
level++;
} else if (kind == END_TAG) {
level--;
- String pad = StringHelper.padString(level);
- os.write(pad.getBytes());
- os.write("</".getBytes());
- os.write(line.key.getBytes());
- os.write(">\n".getBytes());
+ w.write(StringHelper.padString(level));
+ w.write("</");
+ w.write(line.key);
+ w.write(">\n");
} else {
- String pad = StringHelper.padString(level);
- os.write(pad.getBytes());
- os.write("<".getBytes());
- os.write(line.key.getBytes());
- os.write(">".getBytes());
- os.write(line.value.getBytes());
- os.write("</".getBytes());
- os.write(line.key.getBytes());
- os.write(">\n".getBytes());
+ w.write(StringHelper.padString(level));
+ w.write('<');
+ w.write(line.key);
+ w.write('>');
+ w.write(line.value);
+ w.write("</");
+ w.write(line.key);
+ w.write(">\n");
}
}
}
private void serialize(Exchange exchange, Map<String, Object> map, OutputStream os) throws Exception {
List<Line> lines = new ArrayList<>();
doSerialize(exchange.getContext(), map, lines);
- printLines(lines, os);
+ // render in memory and write once: a Writer over the stream costs 16 KB of buffers per call, which is more
+ // than a typical document, and the Marshal EIP already writes into a memory stream
+ StringWriter w = new StringWriter(lines.size() * 32);
+ printLines(lines, w);
+ os.write(w.toString().getBytes(ExchangeHelper.getCharset(exchange, true)));
+ }
+
+ private static String asString(CamelContext context, Object value) {
Review Comment:
The comment says "the type converter returns `toString()` for these, so skip the lookup" — that holds for the *fallback*, but not when a user has registered their own converter. A `@Converter` from an application's `Number` subclass (a `Money`, a domain amount type) to `String` takes precedence in the registry today and is skipped entirely after this change, so such a value would marshal as its raw `toString()` instead of the configured format.
`String` is safe to shortcut. For `Number`/`Boolean` I would either drop the shortcut or restrict it to the final JDK types where no user converter can apply (`Integer`, `Long`, `Double`, `Boolean`, `BigDecimal`, ...), which keeps the win for the common case without changing behaviour for custom types.
##########
components/camel-groovy/src/main/java/org/apache/camel/groovy/xml/GroovyXmlDataFormat.java:
##########
@@ -45,8 +60,23 @@ public class GroovyXmlDataFormat extends ServiceSupport implements DataFormat, D
private static final int VALUE = 2;
private static final int END_TAG = 3;
+ // replaces the XmlParser of a thread as content handler of its SAX parser after a parse, so the parsed document
+ // is not retained by the parser
+ private static final DefaultHandler DETACHED = new DefaultHandler();
+
private boolean attributeMapping = true;
+ /**
+ * Configured like the factory of {@code new XmlParser()} (secure processing, DOCTYPE disallowed, namespace aware,
+ * not validating), created once as its lookup and feature setup dominate the cost of parsing small documents.
+ */
+ private volatile SAXParserFactory saxParserFactory;
+
+ /**
+ * A SAX parser is not thread safe but can parse sequentially, so each thread reuses its own.
+ */
+ private final ThreadLocal<SAXParser> saxParser = ThreadLocal.withInitial(this::createSaxParser);
Review Comment:
This `ThreadLocal` has no counterpart in `doStop()`. `doStart()` warms the factory, but nothing ever releases the parsers, so any thread that has unmarshalled once holds a `SAXParser` — and transitively this data format and its classloader — for the life of the thread, well past `CamelContext.stop()`. On a container thread pool across a redeploy that is a classloader leak, and after a stop/start the stale parsers are silently reused.
A `ThreadLocal` cannot be cleaned up centrally (`remove()` only works on the owning thread). A bounded `ConcurrentLinkedQueue` of parsers — borrow on `unmarshal`, return in the `finally`, drain in `doStop()` — gives the same "a parser is never used by two threads at once" guarantee and is stoppable.
##########
components/camel-groovy/src/main/java/org/apache/camel/groovy/xml/GroovyXmlDataFormat.java:
##########
@@ -78,70 +108,122 @@ public void marshal(Exchange exchange, Object graph, OutputStream stream) throws
@Override
public Object unmarshal(Exchange exchange, InputStream stream) throws Exception {
- XmlParser parser = new XmlParser();
- return parser.parse(stream);
+ SAXParser parser = saxParser.get();
+ try {
+ return new XmlParser(parser).parse(stream);
Review Comment:
**Reusing a pooled `SAXParser` here silently drops namespace prefixes.**
`XmlParser(SAXParser)` is just `reader = parser.getXMLReader();` — it never assigns `namespaceAware`, and the field defaults to `false`. Only the `XmlParser(validating, namespaceAware, allowDocTypeDeclaration)` constructor (which `new XmlParser()` funnels into) sets it to `true`. `getElementName(...)` guards the prefix lookup on that flag, so every `QName` this data format produces now has an empty prefix.
Reproduced on this branch, unmarshalling `<ns:library xmlns:ns=\"urn:x\"><ns:book>a</ns:book></ns:library>` and marshalling it straight back:
```
main: <ns:library xmlns:ns="urn:x">
<ns:book>a</ns:book>
</ns:library>
this PR: <library xmlns="urn:x">
<book>a</book>
</library>
```
`QName.toString()` prints `{urn:x}library` either way, which is why the existing tests do not catch it — the difference only shows up in `QName.getPrefix()` and on the way back out. A route that round-trips namespaced XML, or reads `node.name().getPrefix()`, changes behaviour.
Setting the flag explicitly restores parity (verified — the output above becomes byte-identical to `main` again):
```suggestion
XmlParser xmlParser = new XmlParser(parser);
xmlParser.setNamespaceAware(true);
return xmlParser.parse(stream);
```
A round-trip test on a prefixed document would lock this in.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-24684: camel-groovy - reuse the groovyXml parser configuration and write documents once [camel]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #26305:
URL: https://github.com/apache/camel/pull/26305#issuecomment-5632240431
<!-- ci-tested-modules -->
:test_tube: **CI tested the following changed modules:**
- `components/camel-groovy`
---
<details><summary>:microscope: Scalpel shadow comparison — Scalpel: 16 tested, 27 compile-only — current: 16 all tested</summary>
[Maveniverse Scalpel](https://github.com/maveniverse/scalpel) detected **43 affected modules** (current approach: 16).
<details><summary>:warning: Modules only in Scalpel (27)</summary>
- `apache-camel`
- `camel-allcomponents`
- `camel-catalog`
- `camel-catalog-console`
- `camel-catalog-maven`
- `camel-catalog-suggest`
- `camel-componentdsl`
- `camel-endpointdsl`
- `camel-endpointdsl-support`
- `camel-itest`
- `camel-jbang-core`
- `camel-jbang-it`
- `camel-jbang-main`
- `camel-jbang-plugin-edit`
- `camel-jbang-plugin-generate`
- `camel-jbang-plugin-kubernetes`
- `camel-jbang-plugin-test`
- `camel-kamelet-main`
- `camel-launcher`
- `camel-report-maven-plugin`
- `camel-route-parser`
- `camel-yaml-dsl`
- `camel-yaml-dsl-deserializers`
- `camel-yaml-dsl-maven-plugin`
- `coverage`
- `docs`
- `dummy-component`
</details>
**Skip-tests mode would test 16 modules** (1 direct + 15 downstream), **skip tests for 27** (generated code, meta-modules)
<details><summary>Modules Scalpel would test (16)</summary>
- `camel-groovy`
- `camel-iso8583`
- `camel-jbang-mcp`
- `camel-jbang-plugin-mcp`
- `camel-jbang-plugin-route-parser`
- `camel-jbang-plugin-tui`
- `camel-jbang-plugin-validate`
- `camel-kamelet`
- `camel-launcher-container`
- `camel-once`
- `camel-state-store`
- `camel-test-main-junit5`
- `camel-test-main-junit6`
- `camel-xml-io-dsl`
- `camel-yaml-dsl-validator`
- `camel-yaml-dsl-validator-maven-plugin`
</details>
<details><summary>Modules with tests skipped (27)</summary>
- `apache-camel`
- `camel-allcomponents`
- `camel-catalog`
- `camel-catalog-console`
- `camel-catalog-maven`
- `camel-catalog-suggest`
- `camel-componentdsl`
- `camel-endpointdsl`
- `camel-endpointdsl-support`
- `camel-itest`
- `camel-jbang-core`
- `camel-jbang-it`
- `camel-jbang-main`
- `camel-jbang-plugin-edit`
- `camel-jbang-plugin-generate`
- `camel-jbang-plugin-kubernetes`
- `camel-jbang-plugin-test`
- `camel-kamelet-main`
- `camel-launcher`
- `camel-report-maven-plugin`
- `camel-route-parser`
- `camel-yaml-dsl`
- `camel-yaml-dsl-deserializers`
- `camel-yaml-dsl-maven-plugin`
- `coverage`
- `docs`
- `dummy-component`
</details>
> :information_source: Shadow mode — Scalpel observes but does not affect test execution. [Learn more](https://github.com/maveniverse/scalpel)
</details>
<details><summary>All tested modules (43 modules, 5m 60s total)</summary>
**Total reactor time:** 5m 60s
| Module | Duration | Status |
| --- | --- | --- |
| Camel :: Launcher | 50.4s | SUCCESS |
| Camel :: JBang :: MCP | 42.1s | SUCCESS |
| Camel :: JBang :: Plugin :: TUI | 34.6s | SUCCESS |
| Camel :: Component DSL | 22.8s | SUCCESS |
| Camel :: Catalog :: Camel Catalog | 19.0s | SUCCESS |
| Camel :: Kamelet | 18.2s | SUCCESS |
| Camel :: JBang :: Plugin :: Kubernetes | 17.4s | SUCCESS |
| Camel :: Groovy | 16.0s | SUCCESS |
| Camel :: Docs | 15.3s | SUCCESS |
| Camel :: Test :: Main :: JUnit6 | 14.1s | SUCCESS |
| Camel :: Test :: Main :: JUnit5 | 13.2s | SUCCESS |
| Camel :: JBang :: Plugin :: Testing | 8.7s | SUCCESS |
| Camel :: Catalog :: Camel Report Maven Plugin | 8.4s | SUCCESS |
| Camel :: XML DSL with camel-xml-io | 8.4s | SUCCESS |
| Camel :: Kamelet Main | 8.2s | SUCCESS |
| Camel :: Once | 8.1s | SUCCESS |
| Camel :: Catalog :: Camel Route Parser | 7.5s | SUCCESS |
| Camel :: ISO-8583 | 7.2s | SUCCESS |
| Camel :: YAML DSL :: Deserializers | 5.0s | SUCCESS |
| Camel :: YAML DSL :: Validator | 4.7s | SUCCESS |
| Camel :: All Components Sync point | 4.3s | SUCCESS |
| Camel :: State Store | 3.8s | SUCCESS |
| Camel :: Catalog :: Maven | 2.7s | SUCCESS |
| Camel :: YAML DSL :: Validator Maven Plugin | 2.6s | SUCCESS |
| Camel :: YAML DSL :: Maven Plugins | 2.5s | SUCCESS |
| Camel :: Catalog :: Suggest (deprecated) | 2.5s | SUCCESS |
| Camel :: Assembly | 1.9s | SUCCESS |
| Camel :: JBang :: Plugin :: Edit | 1.4s | SUCCESS |
| Camel :: JBang :: Integration tests | 1.2s | SUCCESS |
| Camel :: Coverage | 1.1s | SUCCESS |
| Camel :: Endpoint DSL :: Support | 1.1s | SUCCESS |
| Camel :: JBang :: Plugin :: Generate | 1.0s | SUCCESS |
| Camel :: JBang :: Plugin :: Validate | 0.8s | SUCCESS |
| Camel :: JBang :: Main | 0.8s | SUCCESS |
| Camel :: Catalog :: Dummy Component | 0.7s | SUCCESS |
| Camel :: Launcher :: Container | 0.7s | SUCCESS |
| Camel :: Catalog :: Console | 0.6s | SUCCESS |
| Camel :: JBang :: Plugin :: MCP | 0.6s | SUCCESS |
| Camel :: JBang :: Plugin :: Route Parser | 0.5s | SUCCESS |
| Camel :: Endpoint DSL | n/a | |
| Camel :: Integration Tests | n/a | |
| Camel :: JBang :: Core | n/a | |
| Camel :: YAML DSL | n/a | |
**Top 20 slowest modules:**
- `Camel :: Launcher` (50.4s)
- `Camel :: JBang :: MCP` (42.1s)
- `Camel :: JBang :: Plugin :: TUI` (34.6s)
- `Camel :: Component DSL` (22.8s)
- `Camel :: Catalog :: Camel Catalog` (19.0s)
- `Camel :: Kamelet` (18.2s)
- `Camel :: JBang :: Plugin :: Kubernetes` (17.4s)
- `Camel :: Groovy` (16.0s)
- `Camel :: Docs` (15.3s)
- `Camel :: Test :: Main :: JUnit6` (14.1s)
- `Camel :: Test :: Main :: JUnit5` (13.2s)
- `Camel :: JBang :: Plugin :: Testing` (8.7s)
- `Camel :: Catalog :: Camel Report Maven Plugin` (8.4s)
- `Camel :: XML DSL with camel-xml-io` (8.4s)
- `Camel :: Kamelet Main` (8.2s)
- `Camel :: Once` (8.1s)
- `Camel :: Catalog :: Camel Route Parser` (7.5s)
- `Camel :: ISO-8583` (7.2s)
- `Camel :: YAML DSL :: Deserializers` (5.0s)
- `Camel :: YAML DSL :: Validator` (4.7s)
</details>
---
:gear: [View full build and test results](https://github.com/apache/camel/actions/runs/34580571716)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org