[GitHub] [commons-io] garydgregory commented on a diff in pull request #343: IO-764 Fix big string writing by writing with ByteBuffer instead calling getBytes directly

[GitBox]

garydgregory commented on code in PR #343:
URL: https://github.com/apache/commons-io/pull/343#discussion_r842076125


##
src/test/java/org/apache/commons/io/IOUtilsTest.java:
##
@@ -1643,5 +1644,19 @@ public void testToString_URL_CharsetName() throws 
Exception {
 public void testToString_URL_CharsetNameNull() throws Exception {
 testToString_URL(null);
 }
+
+@Test
+public void testBigString() throws IOException {
+   String foo = StringUtils.repeat("\uD83D", (Integer.MAX_VALUE)/2-1);

Review Comment:
   > No shared CI will provide you these resources.
   
   Hi @michael-o 
   I wrote a different version of the test that is less resource-intensive and 
more lenient. See git master. The main code is still broken though.



-- 
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: issues-unsubscr...@commons.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [commons-io] garydgregory commented on a diff in pull request #343: IO-764 Fix big string writing by writing with ByteBuffer instead calling getBytes directly

[GitBox]

garydgregory commented on code in PR #343:
URL: https://github.com/apache/commons-io/pull/343#discussion_r841652865


##
src/test/java/org/apache/commons/io/IOUtilsTest.java:
##
@@ -1643,5 +1644,19 @@ public void testToString_URL_CharsetName() throws 
Exception {
 public void testToString_URL_CharsetNameNull() throws Exception {
 testToString_URL(null);
 }
+
+@Test
+public void testBigString() throws IOException {
+   String foo = StringUtils.repeat("\uD83D", (Integer.MAX_VALUE)/2-1);
+   
+   OutputStream dummyStream = new OutputStream() {

Review Comment:
   - Use final where you can.
   - Use NullOutputStream instead of custom code
   - Use CountingOutputStream to at least validate that we wrote the proper 
amount of data.
   - Try to use mocking to insure the right thing happened
   - Even try to use mocking (Mockito and/or Powermock) to see if you can mock 
the string test fixture.



##
src/main/java/org/apache/commons/io/IOUtils.java:
##
@@ -3362,7 +3364,12 @@ public static void write(final String data, final 
OutputStream output)
  */
 public static void write(final String data, final OutputStream output, 
final Charset charset) throws IOException {
 if (data != null) {
-output.write(data.getBytes(Charsets.toCharset(charset)));
+Charset usableCharset = Charsets.toCharset(charset);

Review Comment:
   - Needs comments as to why the implementation changed and is what it is 
since we do not want someone else to come and "simplify" the code. In-line 
locals.



##
src/main/java/org/apache/commons/io/IOUtils.java:
##
@@ -3362,7 +3364,12 @@ public static void write(final String data, final 
OutputStream output)
  */
 public static void write(final String data, final OutputStream output, 
final Charset charset) throws IOException {
 if (data != null) {
-output.write(data.getBytes(Charsets.toCharset(charset)));
+Charset usableCharset = Charsets.toCharset(charset);
+ByteBuffer bytebuffer = usableCharset.encode(data);
+
+try (WritableByteChannel channel = Channels.newChannel(output)){
+   channel.write(bytebuffer);
+}

Review Comment:
   This incorrectly closes the output stream. Run a full build or just 
`IOUtilsWriteTest`:
   ```
   IOUtilsWriteTest
   org.apache.commons.io.IOUtilsWriteTest
   
testWrite_charSequenceToOutputStream_nullEncoding(org.apache.commons.io.IOUtilsWriteTest)
   org.opentest4j.AssertionFailedError: 
ThrowOnFlushAndCloseOutputStream.close() called.
at org.junit.jupiter.api.AssertionUtils.fail(AssertionUtils.java:39)
at org.junit.jupiter.api.Assertions.fail(Assertions.java:134)
at 
org.apache.commons.io.test.ThrowOnFlushAndCloseOutputStream.close(ThrowOnFlushAndCloseOutputStream.java:50)
at 
java.nio.channels.Channels$WritableByteChannelImpl.implCloseChannel(Channels.java:469)
at 
java.nio.channels.spi.AbstractInterruptibleChannel.close(AbstractInterruptibleChannel.java:115)
at org.apache.commons.io.IOUtils.write(IOUtils.java:3372)
at org.apache.commons.io.IOUtils.write(IOUtils.java:3285)
at org.apache.commons.io.IOUtils.write(IOUtils.java:3311)
at 
org.apache.commons.io.IOUtilsWriteTest.testWrite_charSequenceToOutputStream_nullEncoding(IOUtilsWriteTest.java:348)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at 
sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at 
sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:498)
at 
org.junit.platform.commons.util.ReflectionUtils.invokeMethod(ReflectionUtils.java:725)
at 
org.junit.jupiter.engine.execution.MethodInvocation.proceed(MethodInvocation.java:60)
at 
org.junit.jupiter.engine.execution.InvocationInterceptorChain$ValidatingInvocation.proceed(InvocationInterceptorChain.java:131)
at 
org.junit.jupiter.engine.extension.TimeoutExtension.intercept(TimeoutExtension.java:149)
at 
org.junit.jupiter.engine.extension.TimeoutExtension.interceptTestableMethod(TimeoutExtension.java:140)
at 
org.junit.jupiter.engine.extension.TimeoutExtension.interceptTestMethod(TimeoutExtension.java:84)
at 
org.junit.jupiter.engine.execution.ExecutableInvoker$ReflectiveInterceptorCall.lambda$ofVoidMethod$0(ExecutableInvoker.java:115)
at 
org.junit.jupiter.engine.execution.ExecutableInvoker.lambda$invoke$0(ExecutableInvoker.java:105)
at 
org.junit.jupiter.engine.execution.InvocationInterceptorChain$InterceptedInvocation.proceed(InvocationInterceptorChain.java:106)
at 
org.junit.jupiter.engine.execution.InvocationInterceptorChain.proceed(InvocationInterceptorChain.java:64)
at 
org.junit.jupiter.engine.execution.InvocationInterce