davsclaus commented on code in PR #25555:
URL: https://github.com/apache/camel/pull/25555#discussion_r3861911793
##########
core/camel-xml-jaxp/src/main/java/org/apache/camel/converter/jaxp/XmlConverter.java:
##########
@@ -718,12 +718,21 @@ public Document toDOMDocument(StreamCache cache, Exchange
exchange)
public Document toDOMDocument(InputStream in, Exchange exchange)
throws IOException, SAXException, ParserConfigurationException {
DocumentBuilder documentBuilder =
createDocumentBuilder(getDocumentBuilderFactory(exchange));
Review Comment:
This is the core of the concern above: `documentBuilder.parse()` still runs
unconditionally here for every payload, including ones that are obviously not
XML (empty body, JSON, plain text). The JIRA's whole ask is to short-circuit
*before* this call with a cheap byte-level check, precisely to avoid the
expensive DOM construction described as the production pain point (5M
occurrences, GC pressure). Catching the exception afterward doesn't prevent
that cost from being paid.
##########
core/camel-xml-jaxp/src/main/java/org/apache/camel/converter/jaxp/XmlConverter.java:
##########
@@ -718,12 +718,21 @@ public Document toDOMDocument(StreamCache cache, Exchange
exchange)
public Document toDOMDocument(InputStream in, Exchange exchange)
throws IOException, SAXException, ParserConfigurationException {
DocumentBuilder documentBuilder =
createDocumentBuilder(getDocumentBuilderFactory(exchange));
- if (in instanceof IOHelper.EncodingInputStream encIn) {
- // DocumentBuilder detects encoding from XML declaration, so we
need to
- // revert the converted encoding for the input stream
- return documentBuilder.parse(encIn.toOriginalInputStream());
- } else {
- return documentBuilder.parse(in);
+ try {
+ if (in instanceof IOHelper.EncodingInputStream encIn) {
+ // DocumentBuilder detects encoding from XML declaration, so
we need to
+ // revert the converted encoding for the input stream
+ return documentBuilder.parse(encIn.toOriginalInputStream());
+ } else {
+ return documentBuilder.parse(in);
+ }
+ } catch (SAXParseException e) {
+ throw new TypeConversionException(
Review Comment:
Two things here:
1. `CoreTypeConverterRegistry.createTypeConversionException()` already wraps
any exception thrown from a `@Converter` method into a
`TypeConversionException` when conversion goes through the standard registry
(`convertTo`/`getBody()`), which is the path in the JIRA's own stack trace (CXF
→ `MessageSupport.getBody`). So this explicit catch+rethrow doesn't change
anything for the reported scenario — it only matters for direct calls to
`toDOMDocument`, like the new tests do.
2. The JIRA asks for `return null` with `@Converter(allowNull = true)`
instead of throwing, so the framework can "fall through gracefully." Worth
confirming with the reporter/PMC whether throwing is an acceptable deviation,
since it's a different behavior contract than what was specified.
##########
core/camel-xml-jaxp/src/main/java/org/apache/camel/converter/jaxp/XmlConverter.java:
##########
@@ -718,12 +718,21 @@ public Document toDOMDocument(StreamCache cache, Exchange
exchange)
public Document toDOMDocument(InputStream in, Exchange exchange)
throws IOException, SAXException, ParserConfigurationException {
DocumentBuilder documentBuilder =
createDocumentBuilder(getDocumentBuilderFactory(exchange));
- if (in instanceof IOHelper.EncodingInputStream encIn) {
- // DocumentBuilder detects encoding from XML declaration, so we
need to
- // revert the converted encoding for the input stream
- return documentBuilder.parse(encIn.toOriginalInputStream());
- } else {
- return documentBuilder.parse(in);
+ try {
+ if (in instanceof IOHelper.EncodingInputStream encIn) {
+ // DocumentBuilder detects encoding from XML declaration, so
we need to
+ // revert the converted encoding for the input stream
+ return documentBuilder.parse(encIn.toOriginalInputStream());
+ } else {
+ return documentBuilder.parse(in);
+ }
+ } catch (SAXParseException e) {
+ throw new TypeConversionException(
+ in, Document.class,
Review Comment:
Minor: since `toDOMDocument(byte[], Exchange)` now delegates through `new
ByteArrayInputStream(data)`, `TypeConversionException.getFromType()`/the
message will always report `ByteArrayInputStream` here, regardless of whether
the original caller passed a `byte[]` or a `StreamCache`. Slightly less
diagnostic than reporting the original type.
##########
core/camel-xml-jaxp/src/test/java/org/apache/camel/converter/jaxp/XmlConverterPrologTest.java:
##########
@@ -0,0 +1,87 @@
+/*
+ * 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.converter.jaxp;
+
+import java.io.ByteArrayInputStream;
+import java.nio.charset.StandardCharsets;
+
+import org.apache.camel.TypeConversionException;
+import org.junit.jupiter.api.Test;
+
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+/**
+ * Verifies that {@link XmlConverter#toDOMDocument(byte[],
org.apache.camel.Exchange)} and
+ * {@link XmlConverter#toDOMDocument(java.io.InputStream,
org.apache.camel.Exchange)} throw
+ * {@link TypeConversionException} with a descriptive message for non-XML
payloads rather than propagating a raw
+ * {@code SAXParseException: Content is not allowed in prolog}.
+ *
+ * The StreamCache overload delegates to toDOMDocument(InputStream, Exchange),
so the InputStream tests cover that path
+ * as well.
+ */
+class XmlConverterPrologTest {
+
+ // ---- byte[] overload ----
+
+ @Test
+ void toDOMDocument_emptyByteArrayThrowsTypeConversionException() {
Review Comment:
Minor style: this and the other test methods use snake_case-style names
(`toDOMDocument_emptyByteArrayThrowsTypeConversionException`). No other test
class in this module uses underscores in method names — the convention
elsewhere is camelCase (e.g. `testFoo`). Not blocking, but worth aligning for
consistency.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
