vlsi commented on code in PR #6767:
URL: https://github.com/apache/jmeter/pull/6767#discussion_r4098269958
##########
src/core/src/main/kotlin/org/apache/jmeter/save/JMeterStaxDriver.kt:
##########
@@ -17,13 +17,27 @@
package org.apache.jmeter.save
+import com.ctc.wstx.api.InvalidCharHandler
+import com.ctc.wstx.api.WstxOutputProperties
import com.thoughtworks.xstream.io.xml.StaxDriver
import javax.xml.stream.XMLOutputFactory
public class JMeterStaxDriver(
public val xmlHeader: Boolean = true,
public val indent: Boolean = true,
) : StaxDriver() {
- override fun createOutputFactory(): XMLOutputFactory =
- XMLOutputFactoryDelegate(super.createOutputFactory(), xmlHeader =
xmlHeader, indent = indent)
+ override fun createOutputFactory(): XMLOutputFactory {
+ val factory = super.createOutputFactory()
Review Comment:
`StaxDriver.createOutputFactory()` returns `XMLOutputFactory.newInstance()`,
which is not necessarily Woodstox. A plugin in `lib/ext` that ships another
StAX implementation (Aalto, for instance), or a
`javax.xml.stream.XMLOutputFactory` system property, selects a different
factory. `setProperty` with a Woodstox-only key then throws
`IllegalArgumentException`, and every save fails. Please create
`WstxOutputFactory` explicitly, or check `isPropertySupported` before setting
the property.
##########
src/core/src/main/kotlin/org/apache/jmeter/save/JMeterStaxDriver.kt:
##########
@@ -17,13 +17,27 @@
package org.apache.jmeter.save
+import com.ctc.wstx.api.InvalidCharHandler
+import com.ctc.wstx.api.WstxOutputProperties
import com.thoughtworks.xstream.io.xml.StaxDriver
import javax.xml.stream.XMLOutputFactory
public class JMeterStaxDriver(
public val xmlHeader: Boolean = true,
public val indent: Boolean = true,
) : StaxDriver() {
- override fun createOutputFactory(): XMLOutputFactory =
- XMLOutputFactoryDelegate(super.createOutputFactory(), xmlHeader =
xmlHeader, indent = indent)
+ override fun createOutputFactory(): XMLOutputFactory {
+ val factory = super.createOutputFactory()
+ // Woodstox rejects XML 1.0-illegal characters (NUL, C0 controls) by
default.
+ // Replace them so JTL/JMX save of binary payloads does not fail
(#6761).
+ factory.setProperty(
+ WstxOutputProperties.P_OUTPUT_INVALID_CHAR_HANDLER,
+ InvalidCharHandler.ReplacingHandler(INVALID_XML_CHAR_REPLACEMENT)
+ )
Review Comment:
The handler replaces characters in attribute values too: a sample label
`"lab\u0002el"` is written to the JTL as `lb="lab el"`. Please cover that in a
test and in the changelog, whichever behavior the rework settles on.
##########
src/core/src/main/kotlin/org/apache/jmeter/save/JMeterStaxDriver.kt:
##########
@@ -17,13 +17,27 @@
package org.apache.jmeter.save
+import com.ctc.wstx.api.InvalidCharHandler
+import com.ctc.wstx.api.WstxOutputProperties
import com.thoughtworks.xstream.io.xml.StaxDriver
import javax.xml.stream.XMLOutputFactory
public class JMeterStaxDriver(
public val xmlHeader: Boolean = true,
public val indent: Boolean = true,
) : StaxDriver() {
- override fun createOutputFactory(): XMLOutputFactory =
- XMLOutputFactoryDelegate(super.createOutputFactory(), xmlHeader =
xmlHeader, indent = indent)
+ override fun createOutputFactory(): XMLOutputFactory {
+ val factory = super.createOutputFactory()
+ // Woodstox rejects XML 1.0-illegal characters (NUL, C0 controls) by
default.
+ // Replace them so JTL/JMX save of binary payloads does not fail
(#6761).
Review Comment:
NUL is itself a C0 control, and tab, line feed, and carriage return are C0
controls that XML 1.0 allows, so "NUL, C0 controls" names a larger set than the
one Woodstox rejects. The comment also leaves out the consequence a maintainer
needs: the replacement cannot be undone, and it applies to attribute values as
well as text. For the current approach, something like:
```kotlin
// Woodstox rejects C0 control characters other than tab, LF, and CR.
// Each one is written as a space, in text and in attribute values, so the
original character is lost.
```
##########
src/core/src/main/kotlin/org/apache/jmeter/save/JMeterStaxDriver.kt:
##########
@@ -17,13 +17,27 @@
package org.apache.jmeter.save
+import com.ctc.wstx.api.InvalidCharHandler
+import com.ctc.wstx.api.WstxOutputProperties
import com.thoughtworks.xstream.io.xml.StaxDriver
import javax.xml.stream.XMLOutputFactory
public class JMeterStaxDriver(
public val xmlHeader: Boolean = true,
public val indent: Boolean = true,
) : StaxDriver() {
- override fun createOutputFactory(): XMLOutputFactory =
- XMLOutputFactoryDelegate(super.createOutputFactory(), xmlHeader =
xmlHeader, indent = indent)
+ override fun createOutputFactory(): XMLOutputFactory {
+ val factory = super.createOutputFactory()
+ // Woodstox rejects XML 1.0-illegal characters (NUL, C0 controls) by
default.
+ // Replace them so JTL/JMX save of binary payloads does not fail
(#6761).
+ factory.setProperty(
+ WstxOutputProperties.P_OUTPUT_INVALID_CHAR_HANDLER,
+ InvalidCharHandler.ReplacingHandler(INVALID_XML_CHAR_REPLACEMENT)
+ )
+ return XMLOutputFactoryDelegate(factory, xmlHeader = xmlHeader, indent
= indent)
+ }
+
+ private companion object {
+ const val INVALID_XML_CHAR_REPLACEMENT = ' '
+ }
Review Comment:
A companion object for one private `Char` is more structure than the value
needs. A file-level `private const val` does the same.
##########
src/core/src/test/java/org/apache/jmeter/save/SaveServiceInvalidXmlCharTest.java:
##########
@@ -0,0 +1,72 @@
+/*
+ * 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.jmeter.save;
+
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+
+import java.io.ByteArrayOutputStream;
+import java.io.StringWriter;
+import java.nio.charset.StandardCharsets;
+
+import org.apache.jmeter.junit.JMeterTestCase;
+import org.apache.jmeter.samplers.SampleEvent;
+import org.apache.jmeter.samplers.SampleResult;
+import org.apache.jmeter.samplers.SampleSaveConfiguration;
+import org.apache.jmeter.testelement.property.StringProperty;
+import org.junit.jupiter.api.Test;
+
+/**
+ * Regression for <a
href="https://github.com/apache/jmeter/issues/6761";>#6761</a>:
+ * Woodstox rejects characters that are illegal in XML 1.0 when saving JTL/JMX.
+ */
Review Comment:
The comment opens with the defect in the present tense, and after the fix it
is no longer true. State the rule the test checks first, then the old defect in
one past-tense sentence. With the current behavior, for example:
```java
/**
* A character that XML 1.0 does not allow is saved as a space.
* Saving such a value used to fail with WstxIOException (issue #6761).
*/
```
##########
src/core/src/test/java/org/apache/jmeter/save/SaveServiceInvalidXmlCharTest.java:
##########
@@ -0,0 +1,72 @@
+/*
+ * 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.jmeter.save;
+
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+
+import java.io.ByteArrayOutputStream;
+import java.io.StringWriter;
+import java.nio.charset.StandardCharsets;
+
+import org.apache.jmeter.junit.JMeterTestCase;
+import org.apache.jmeter.samplers.SampleEvent;
+import org.apache.jmeter.samplers.SampleResult;
+import org.apache.jmeter.samplers.SampleSaveConfiguration;
+import org.apache.jmeter.testelement.property.StringProperty;
+import org.junit.jupiter.api.Test;
+
+/**
+ * Regression for <a
href="https://github.com/apache/jmeter/issues/6761";>#6761</a>:
+ * Woodstox rejects characters that are illegal in XML 1.0 when saving JTL/JMX.
+ */
+public class SaveServiceInvalidXmlCharTest extends JMeterTestCase {
+
+ private static final String ILLEGAL_XML_CHARS = "pre\u0000mid\u001fsuf";
+
+ @Test
+ void saveSampleResultAllowsIllegalXmlCharsInSamplerData() {
+ SampleSaveConfiguration saveConfig = new SampleSaveConfiguration();
+ saveConfig.setSamplerData(true);
+
+ SampleResult result = new SampleResult();
+ result.setSaveConfig(saveConfig);
+ result.setSampleLabel("binary-body");
+ result.setSamplerData(ILLEGAL_XML_CHARS);
+
+ StringWriter writer = new StringWriter();
+ assertDoesNotThrow(() -> SaveService.saveSampleResult(new
SampleEvent(result, "tg"), writer));
+
+ String xml = writer.toString();
+ assertFalse(xml.isEmpty(), "JTL output should not be empty");
+ assertFalse(xml.indexOf('\u0000') >= 0, "NUL must not appear in
well-formed XML");
+ assertFalse(xml.indexOf('\u001f') >= 0, "C1 control 0x1F must not
appear in XML 1.0");
Review Comment:
These assertions still pass if the value is dropped entirely: the output is
non-empty and contains no NUL. Please assert the literal that is expected in
the output, e.g. `<samplerData class="java.lang.String">pre mid
suf</samplerData>`, so the test fails when the written value is wrong.
`assertFalse(xml.indexOf(..) >= 0)` prints only `expected: <false> but was:
<true>` on failure. An `assertEquals` against the expected string prints both
values. Once the test asserts the output, `assertDoesNotThrow` adds nothing.
##########
xdocs/changes.xml:
##########
@@ -126,6 +126,7 @@ Summary
<li><issue>5937</issue>Remove deprecated Log4j package scanning and
configure plugin metadata processing to improve startup time and avoid
deprecation warnings. Contributed by Piotr P. Karwasz
(github.com/piotrgithub)</li>
<li><pr>6620</pr>Fix report generation paths so dashboard output files are
created in the correct location after internal refactoring.</li>
<li><bug>6456</bug>Handle malformed percent-encoded URLs gracefully when
recording HTTP traffic, logging a warning instead of failing the recording.</li>
+ <li><issue>6761</issue>Allow XML save of sample results and test plans
that contain characters illegal in XML 1.0 (NUL and C0 controls) after the
Woodstox migration.</li>
Review Comment:
The Woodstox migration has not been released, so users upgrading from 5.6.3
never saw this failure, and "after the Woodstox migration" describes internal
history. For those users, the visible change is what happens to these
characters on save: replaced with a space, or kept, depending on the rework.
Please describe that instead.
##########
src/core/src/test/java/org/apache/jmeter/save/SaveServiceInvalidXmlCharTest.java:
##########
@@ -0,0 +1,72 @@
+/*
+ * 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.jmeter.save;
+
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+
+import java.io.ByteArrayOutputStream;
+import java.io.StringWriter;
+import java.nio.charset.StandardCharsets;
+
+import org.apache.jmeter.junit.JMeterTestCase;
+import org.apache.jmeter.samplers.SampleEvent;
+import org.apache.jmeter.samplers.SampleResult;
+import org.apache.jmeter.samplers.SampleSaveConfiguration;
+import org.apache.jmeter.testelement.property.StringProperty;
+import org.junit.jupiter.api.Test;
+
+/**
+ * Regression for <a
href="https://github.com/apache/jmeter/issues/6761";>#6761</a>:
+ * Woodstox rejects characters that are illegal in XML 1.0 when saving JTL/JMX.
+ */
+public class SaveServiceInvalidXmlCharTest extends JMeterTestCase {
+
+ private static final String ILLEGAL_XML_CHARS = "pre\u0000mid\u001fsuf";
+
+ @Test
+ void saveSampleResultAllowsIllegalXmlCharsInSamplerData() {
+ SampleSaveConfiguration saveConfig = new SampleSaveConfiguration();
+ saveConfig.setSamplerData(true);
+
+ SampleResult result = new SampleResult();
+ result.setSaveConfig(saveConfig);
+ result.setSampleLabel("binary-body");
+ result.setSamplerData(ILLEGAL_XML_CHARS);
+
+ StringWriter writer = new StringWriter();
+ assertDoesNotThrow(() -> SaveService.saveSampleResult(new
SampleEvent(result, "tg"), writer));
+
+ String xml = writer.toString();
+ assertFalse(xml.isEmpty(), "JTL output should not be empty");
+ assertFalse(xml.indexOf('\u0000') >= 0, "NUL must not appear in
well-formed XML");
+ assertFalse(xml.indexOf('\u001f') >= 0, "C1 control 0x1F must not
appear in XML 1.0");
+ }
+
+ @Test
+ void saveElementAllowsNulInStringProperty() {
+ StringProperty property = new StringProperty("bin", ILLEGAL_XML_CHARS);
+ ByteArrayOutputStream out = new ByteArrayOutputStream();
+ assertDoesNotThrow(() -> SaveService.saveElement(property, out));
+
+ String xml = out.toString(StandardCharsets.UTF_8);
+ assertFalse(xml.isEmpty(), "JMX fragment should not be empty");
+ assertFalse(xml.indexOf('\u0000') >= 0, "NUL must not appear in
well-formed XML");
+ assertFalse(xml.indexOf('\u001f') >= 0, "C1 control 0x1F must not
appear in XML 1.0");
+ }
Review Comment:
What users need is that a plan saved with such a value opens again with a
known value. Please add:
- a round-trip test: `saveElement`, then `SaveService.loadElement`,
asserting the loaded value;
- a test that loads a 5.6.3-style fragment containing `�` and ``,
which fails on the current code.
The name says `Nul`, but the input also contains 0x1F, and with both
characters in one string no test checks 0x1F on its own, although it is the
character in the first stack trace of the issue. A parameterized test with one
case per character, plus a case for a character in an attribute (the sample
label), would cover each separately.
##########
src/core/src/test/java/org/apache/jmeter/save/SaveServiceInvalidXmlCharTest.java:
##########
@@ -0,0 +1,72 @@
+/*
+ * 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.jmeter.save;
+
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+
+import java.io.ByteArrayOutputStream;
+import java.io.StringWriter;
+import java.nio.charset.StandardCharsets;
+
+import org.apache.jmeter.junit.JMeterTestCase;
+import org.apache.jmeter.samplers.SampleEvent;
+import org.apache.jmeter.samplers.SampleResult;
+import org.apache.jmeter.samplers.SampleSaveConfiguration;
+import org.apache.jmeter.testelement.property.StringProperty;
+import org.junit.jupiter.api.Test;
+
+/**
+ * Regression for <a
href="https://github.com/apache/jmeter/issues/6761";>#6761</a>:
+ * Woodstox rejects characters that are illegal in XML 1.0 when saving JTL/JMX.
+ */
+public class SaveServiceInvalidXmlCharTest extends JMeterTestCase {
+
+ private static final String ILLEGAL_XML_CHARS = "pre\u0000mid\u001fsuf";
+
+ @Test
+ void saveSampleResultAllowsIllegalXmlCharsInSamplerData() {
+ SampleSaveConfiguration saveConfig = new SampleSaveConfiguration();
+ saveConfig.setSamplerData(true);
+
+ SampleResult result = new SampleResult();
+ result.setSaveConfig(saveConfig);
+ result.setSampleLabel("binary-body");
+ result.setSamplerData(ILLEGAL_XML_CHARS);
+
+ StringWriter writer = new StringWriter();
+ assertDoesNotThrow(() -> SaveService.saveSampleResult(new
SampleEvent(result, "tg"), writer));
+
+ String xml = writer.toString();
+ assertFalse(xml.isEmpty(), "JTL output should not be empty");
+ assertFalse(xml.indexOf('\u0000') >= 0, "NUL must not appear in
well-formed XML");
+ assertFalse(xml.indexOf('\u001f') >= 0, "C1 control 0x1F must not
appear in XML 1.0");
Review Comment:
0x1F is a C0 control character, not C1 (C1 is 0x80–0x9F). The same message
is on line 70.
--
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]
