[
https://issues.apache.org/jira/browse/GROOVY-12155?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18096028#comment-18096028
]
ASF GitHub Bot commented on GROOVY-12155:
-----------------------------------------
Copilot commented on code in PR #2700:
URL: https://github.com/apache/groovy/pull/2700#discussion_r3575096826
##########
src/main/java/org/codehaus/groovy/runtime/ProcessGroovyMethods.java:
##########
@@ -79,7 +124,21 @@ public static InputStream getIn(Process self) {
* @since 1.0
*/
public static String getText(Process self) throws IOException {
- String text = IOGroovyMethods.getText(new BufferedReader(new
InputStreamReader(self.getInputStream())));
+ return getText(self, processEncoding());
+ }
+
+ /**
+ * Read the text of the output stream of the Process using the given
charset.
+ * Closes all the streams associated with the process after retrieving the
text.
+ *
+ * @param self a Process instance
+ * @param charset the charset the process writes its output in
+ * @return the text of the output
+ * @throws java.io.IOException if an IOException occurs.
+ * @since 6.0.0
+ */
+ public static String getText(Process self, Charset charset) throws
IOException {
+ String text = IOGroovyMethods.getText(new BufferedReader(new
InputStreamReader(self.getInputStream(), charset)));
closeStreams(self);
return text;
Review Comment:
getText(Process, Charset) will throw a NullPointerException if callers pass
a null charset (easy to do from dynamically-typed Groovy), and
closeStreams(self) won’t run if IOGroovyMethods.getText(...) throws. Consider
treating null as “use default process encoding” and moving closeStreams(self)
into a finally block so streams are always closed.
##########
src/test/groovy/groovy/ProcessTest.groovy:
##########
@@ -73,6 +80,69 @@ class ProcessTest {
assert "" == myProcess.text
}
+ @Test
+ void testProcessTextUsesSuppliedCharset() {
+ // a single 0xE9 byte is 'é' in ISO-8859-1 but is not valid UTF-8
+ def latin1 = new MockProcess([0xE9] as byte[])
+ assert "é" == latin1.getText(ISO_8859_1)
+
+ def utf8 = new MockProcess("é".getBytes(UTF_8))
+ assert "é" == utf8.getText(UTF_8)
+ }
+
+ @Test
+ void testProcessTextDefaultsToTheNativeEncoding() {
+ def bytes = [0xE9] as byte[]
+ def expected = new String(bytes,
Charset.forName(System.getProperty("native.encoding")))
+ assert expected == new MockProcess(bytes).text
Review Comment:
This test assumes the "native.encoding" system property is always present;
if it’s absent, Charset.forName(null) will throw and the test will fail even
though the production code falls back to Charset.defaultCharset(). Use a
null-safe fallback here too.
##########
src/test/groovy/groovy/ProcessTest.groovy:
##########
@@ -73,6 +80,69 @@ class ProcessTest {
assert "" == myProcess.text
}
+ @Test
+ void testProcessTextUsesSuppliedCharset() {
+ // a single 0xE9 byte is 'é' in ISO-8859-1 but is not valid UTF-8
+ def latin1 = new MockProcess([0xE9] as byte[])
+ assert "é" == latin1.getText(ISO_8859_1)
+
+ def utf8 = new MockProcess("é".getBytes(UTF_8))
+ assert "é" == utf8.getText(UTF_8)
+ }
+
+ @Test
+ void testProcessTextDefaultsToTheNativeEncoding() {
+ def bytes = [0xE9] as byte[]
+ def expected = new String(bytes,
Charset.forName(System.getProperty("native.encoding")))
+ assert expected == new MockProcess(bytes).text
+ }
+
+ @Test
+ void testProcessEncodingPropertyOverridesTheNativeEncoding() {
+ System.setProperty(PROCESS_ENCODING, "ISO-8859-1")
+ assert "é" == new MockProcess([0xE9] as byte[]).text
+
+ System.setProperty(PROCESS_ENCODING, "UTF-8")
+ assert "é" == new MockProcess("é".getBytes(UTF_8)).text
+ }
+
+ @Test
+ void testProcessEncodingPropertyIgnoredWhenNotAValidCharset() {
+ System.setProperty(PROCESS_ENCODING, "no-such-charset")
+ def bytes = [0xE9] as byte[]
+ def expected = new String(bytes,
Charset.forName(System.getProperty("native.encoding")))
+ assert expected == new MockProcess(bytes).text
Review Comment:
Same issue as above: Charset.forName(System.getProperty("native.encoding"))
will throw if the property is missing, but the production code falls back to
Charset.defaultCharset(). Make the test resilient by using a null-safe fallback.
##########
src/test/groovy/groovy/ProcessTest.groovy:
##########
@@ -89,6 +159,7 @@ class ProcessTest {
@AfterEach
void tearDown() {
+ System.clearProperty(PROCESS_ENCODING)
myProcess.destroy()
Review Comment:
tearDown() always clears the groovy.process.encoding system property, which
can leak global state across the test JVM if it was set before this test class
ran. Consider capturing the previous value in setUp() and restoring it (set
back to the old value or clear only if it was originally unset).
> decode process output streams using the native encoding
> -------------------------------------------------------
>
> Key: GROOVY-12155
> URL: https://issues.apache.org/jira/browse/GROOVY-12155
> Project: Groovy
> Issue Type: Bug
> Reporter: Paul King
> Assignee: Paul King
> Priority: Major
>
> see https://github.com/apache/groovy/pull/2700
> with thanks to https://github.com/netliomax25-code for identifying the problem
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
