garydgregory commented on code in PR #779:
URL: https://github.com/apache/commons-io/pull/779#discussion_r2325737526
##########
src/main/java/org/apache/commons/io/input/BoundedInputStream.java:
##########
@@ -427,13 +421,19 @@ public long getMaxLength() {
}
/**
- * Gets how many bytes remain to read.
+ * Returns the bytes remaining before the configured read limit is reached.
Review Comment:
See above about documenting getter methods.
##########
src/main/java/org/apache/commons/io/input/BoundedInputStream.java:
##########
@@ -405,9 +399,9 @@ public synchronized long getCount() {
}
/**
- * Gets the max count of bytes to read.
+ * Returns the maximum number of bytes this stream is allowed to read.
*
- * @return The max count of bytes to read.
+ * @return the maximum number of bytes permitted, or {@value IOUtils#EOF}
if unbounded.
Review Comment:
Please do not change the Commons convention that a sentence starts with a
capital letter.
##########
src/test/java/org/apache/commons/io/input/BoundedInputStreamTest.java:
##########
@@ -80,21 +91,75 @@ void testAfterReadConsumer() throws Exception {
// @formatter:on
}
- @SuppressWarnings("resource")
- @Test
- void testAvailableAfterClose() throws Exception {
+ static Stream<Arguments> testAvailableAfterClose() throws IOException {
+ // Case 1: behaves like ByteArrayInputStream — close() is a no-op,
available() still returns a value (e.g., 42).
+ final InputStream noOpClose = mock(InputStream.class);
+ when(noOpClose.available()).thenReturn(42, 42);
+
+ // Case 2: returns 0 after close (Commons memory-backed streams that
ignore close but report 0 when exhausted).
+ final InputStream returnsZeroAfterClose = mock(InputStream.class);
+ when(returnsZeroAfterClose.available()).thenReturn(42, 0);
+
+ // Case 3: throws IOException after close (e.g., FileInputStream-like
behavior).
+ final InputStream throwsAfterClose = mock(InputStream.class);
+ when(throwsAfterClose.available()).thenReturn(42).thenThrow(new
IOException("Stream closed"));
+
+ return Stream.of(
+ Arguments.of("no-op close → still returns 42", noOpClose, 42),
Review Comment:
The arrow Unicode character is unnecessary IMO, elsewhere in this PR, you
use "->", are either better explained with a word? Does arrow mean "convert",
or "returns", or...?
##########
src/main/java/org/apache/commons/io/input/BoundedInputStream.java:
##########
@@ -370,16 +370,10 @@ protected synchronized void afterRead(final int n) throws
IOException {
super.afterRead(n);
}
- /**
- * {@inheritDoc}
- */
@Override
public int available() throws IOException {
- if (isMaxCount()) {
- onMaxLength(maxCount, getCount());
- return 0;
- }
- return in.available();
+ final int remaining = Math.toIntExact(Math.min(getRemaining(),
Integer.MAX_VALUE));
Review Comment:
This method now throws `ArithmeticException`; should we:
- Javadoc it, or,
- Convert it to an `IllegalStateException`, or,
- Something else?
##########
src/test/java/org/apache/commons/io/input/BoundedInputStreamTest.java:
##########
@@ -203,10 +268,10 @@ void testCounts(final long startCount) throws Exception {
@Test
void testMarkReset() throws Exception {
- final byte[] helloWorld = "Hello
World".getBytes(StandardCharsets.UTF_8);
+ final byte[] helloWorld = "Hello World".getBytes(UTF_8);
Review Comment:
In general, I would not change the access style in a PR because there is now
more to review.
##########
src/main/java/org/apache/commons/io/input/BoundedInputStream.java:
##########
@@ -405,9 +399,9 @@ public synchronized long getCount() {
}
/**
- * Gets the max count of bytes to read.
+ * Returns the maximum number of bytes this stream is allowed to read.
Review Comment:
In general, Commons uses the convention that a getter method "Gets
something", a setter method "Sets something", and an is method "Tests
something". Please do not change the style here.
##########
src/test/java/org/apache/commons/io/input/BoundedInputStreamTest.java:
##########
@@ -80,21 +91,75 @@ void testAfterReadConsumer() throws Exception {
// @formatter:on
}
- @SuppressWarnings("resource")
- @Test
- void testAvailableAfterClose() throws Exception {
+ static Stream<Arguments> testAvailableAfterClose() throws IOException {
+ // Case 1: behaves like ByteArrayInputStream — close() is a no-op,
available() still returns a value (e.g., 42).
+ final InputStream noOpClose = mock(InputStream.class);
+ when(noOpClose.available()).thenReturn(42, 42);
+
+ // Case 2: returns 0 after close (Commons memory-backed streams that
ignore close but report 0 when exhausted).
+ final InputStream returnsZeroAfterClose = mock(InputStream.class);
+ when(returnsZeroAfterClose.available()).thenReturn(42, 0);
+
+ // Case 3: throws IOException after close (e.g., FileInputStream-like
behavior).
+ final InputStream throwsAfterClose = mock(InputStream.class);
+ when(throwsAfterClose.available()).thenReturn(42).thenThrow(new
IOException("Stream closed"));
+
+ return Stream.of(
+ Arguments.of("no-op close → still returns 42", noOpClose, 42),
+ Arguments.of("after close → returns 0", returnsZeroAfterClose,
42),
+ Arguments.of("after close → throws IOException",
throwsAfterClose, 42));
+ }
+
+ @DisplayName("available() after close follows underlying stream semantics")
Review Comment:
In general, we don't use `@DisplayName`; using Javadoc is both richer in
formatting and more versatile.
--
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
