garydgregory commented on code in PR #784:
URL: https://github.com/apache/commons-io/pull/784#discussion_r2370318884
##########
src/main/java/org/apache/commons/io/build/AbstractOrigin.java:
##########
@@ -755,6 +1120,28 @@ public Writer getWriter(final Charset charset, final
OpenOption... options) thro
return Files.newBufferedWriter(getPath(), Charsets.toCharset(charset),
options);
}
+ /**
+ * Gets this origin as a Channel, if possible.
+ *
+ * @param channelType The type of channel to return.
+ * @param options Options specifying how a file-based origin is opened,
ignored otherwise.
+ * @return A new Channel on the origin of the given type.
+ * @param <C> The type of channel to return.
+ * @throws IOException If an I/O error occurs.
+ * @throws UnsupportedOperationException If this origin cannot be
converted to a channel of the given type.
+ * @since 2.21.0
+ */
+ public <C extends Channel> C getChannel(Class<C> channelType,
OpenOption... options) throws IOException {
+ Objects.requireNonNull(channelType, "channelType");
+ final Channel channel = doGetChannel(options);
+ if (channelType.isInstance(channel)) {
+ return channelType.cast(channel);
+ }
+ throw unsupportedChannelType(channelType);
+ }
+
+ abstract Channel doGetChannel(OpenOption... options) throws IOException;
Review Comment:
What is this supposed to do? A Javadoc would help 😉
I'm _really_ not a fan of the `doFoo` naming convention. To me it screams:
"I couldn't think of a better name!". It looks like all of these
implementations create a _new_ channel (except `ChannelOrigin` obviously),
so... how about calling it`newChannel()`? At least that name is pretty clear,
even without a Javadoc 😉
##########
src/main/java/org/apache/commons/io/channels/ByteArraySeekableByteChannel.java:
##########
@@ -37,41 +37,74 @@
* accessed via {@link ByteArraySeekableByteChannel#array()}.
* </p>
*
- * @since 2.19.0
+ * @since 2.21.0
*/
public class ByteArraySeekableByteChannel implements SeekableByteChannel {
private static final int RESIZE_LIMIT = Integer.MAX_VALUE >> 1;
+
+ /**
+ * Constructs a new channel backed directly by the given byte array.
+ *
+ * <p>The channel initially contains the full contents of the array, with
its
+ * size set to {@code bytes.length} and its position set to {@code 0}.</p>
+ *
+ * <p>Reads and writes operate on the shared array.
+ * If a write operation extends beyond the current capacity, the channel
will
+ * automatically allocate a larger backing array and copy the existing
contents.</p>
+ *
+ * @param bytes The byte array to wrap, must not be {@code null}
+ * @return A new channel that uses the given array as its initial backing
store
+ * @throws NullPointerException If {@code bytes} is {@code null}
+ * @see #array()
+ * @see java.io.ByteArrayInputStream#ByteArrayInputStream(byte[])
+ */
+ public static ByteArraySeekableByteChannel wrap(byte[] bytes) {
+ Objects.requireNonNull(bytes, "bytes");
+ return new ByteArraySeekableByteChannel(bytes);
+ }
+
private byte[] data;
- private final AtomicBoolean closed = new AtomicBoolean();
+ private volatile boolean closed;
private int position;
private int size;
private final ReentrantLock lock = new ReentrantLock();
/**
- * Constructs a new instance using a default empty buffer.
+ * Constructs a new instance, with a default internal buffer capacity.
+ * <p>
+ * The initial size and position of the channel are 0.
+ * </p>
+ * <p>
+ * The default initial capacity is 32 bytes, although the capacity will
grow as needed when writing data.
+ * </p>
+ * @see java.io.ByteArrayOutputStream#ByteArrayOutputStream()
Review Comment:
There should be a blank Javadoc line before the first `@` tag.
Don't use FQCN when you don't need to in `@see java.io...`
##########
src/main/java/org/apache/commons/io/build/AbstractOrigin.java:
##########
@@ -53,15 +58,246 @@
import org.apache.commons.io.output.WriterOutputStream;
/**
- * Abstracts the origin of data for builders like a {@link File}, {@link
Path}, {@link Reader}, {@link Writer}, {@link InputStream}, {@link
OutputStream}, and
- * {@link URI}.
+ * Abstract base class that encapsulates the <em>origin</em> of data used by
Commons IO builders.
* <p>
- * Some methods may throw {@link UnsupportedOperationException} if that method
is not implemented in a concrete subclass, see {@link #getFile()} and
- * {@link #getPath()}.
+ * An origin represents where bytes/characters come from or go to, such as a
{@link File}, {@link Path},
+ * {@link Reader}, {@link Writer}, {@link InputStream}, {@link OutputStream},
or {@link URI}. Concrete subclasses
+ * expose only the operations that make sense for the underlying source or
sink; invoking an unsupported operation
+ * results in {@link UnsupportedOperationException} (see, for example, {@link
#getFile()} and {@link #getPath()}).
* </p>
*
- * @param <T> the type of instances to build.
- * @param <B> the type of builder subclass.
+ * <p>
+ * The table below summarizes which views and conversions are supported for
each origin type.
+ * Column headers show the target view; cells indicate whether that view is
available from the origin in that row.
+ * </p>
+ *
+ * <table>
+ * <caption>Origin support matrix</caption>
Review Comment:
Nice! I see entries for `WritableByteChannel`, but shouldn't there be
entries for `ReadableByteChannel`, and `SeekableByteChannel`?
Or should there be a single entry for `Channel`?
##########
src/main/java/org/apache/commons/io/channels/ByteArraySeekableByteChannel.java:
##########
@@ -37,41 +37,74 @@
* accessed via {@link ByteArraySeekableByteChannel#array()}.
* </p>
*
- * @since 2.19.0
+ * @since 2.21.0
Review Comment:
Good catch! 👍
##########
src/main/java/org/apache/commons/io/channels/ByteArraySeekableByteChannel.java:
##########
@@ -37,41 +37,74 @@
* accessed via {@link ByteArraySeekableByteChannel#array()}.
* </p>
*
- * @since 2.19.0
+ * @since 2.21.0
*/
public class ByteArraySeekableByteChannel implements SeekableByteChannel {
private static final int RESIZE_LIMIT = Integer.MAX_VALUE >> 1;
+
+ /**
+ * Constructs a new channel backed directly by the given byte array.
+ *
+ * <p>The channel initially contains the full contents of the array, with
its
+ * size set to {@code bytes.length} and its position set to {@code 0}.</p>
+ *
+ * <p>Reads and writes operate on the shared array.
+ * If a write operation extends beyond the current capacity, the channel
will
+ * automatically allocate a larger backing array and copy the existing
contents.</p>
+ *
+ * @param bytes The byte array to wrap, must not be {@code null}
+ * @return A new channel that uses the given array as its initial backing
store
+ * @throws NullPointerException If {@code bytes} is {@code null}
+ * @see #array()
+ * @see java.io.ByteArrayInputStream#ByteArrayInputStream(byte[])
+ */
+ public static ByteArraySeekableByteChannel wrap(byte[] bytes) {
+ Objects.requireNonNull(bytes, "bytes");
+ return new ByteArraySeekableByteChannel(bytes);
+ }
+
private byte[] data;
- private final AtomicBoolean closed = new AtomicBoolean();
+ private volatile boolean closed;
private int position;
private int size;
private final ReentrantLock lock = new ReentrantLock();
/**
- * Constructs a new instance using a default empty buffer.
+ * Constructs a new instance, with a default internal buffer capacity.
+ * <p>
+ * The initial size and position of the channel are 0.
+ * </p>
+ * <p>
+ * The default initial capacity is 32 bytes, although the capacity will
grow as needed when writing data.
+ * </p>
+ * @see java.io.ByteArrayOutputStream#ByteArrayOutputStream()
*/
public ByteArraySeekableByteChannel() {
- this(0);
+ this(32);
}
/**
- * Constructs a new instance from a byte array.
- *
- * @param data input data or pre-allocated array.
+ * Constructs a new instance, with an internal buffer of the given
capacity, in bytes.
+ * <p>
+ * The initial size and position of the channel are 0.
+ * </p>
+ * @param size Capacity of the internal buffer to allocate, in bytes.
+ * @see java.io.ByteArrayOutputStream#ByteArrayOutputStream(int)
Review Comment:
Don't use FQCN when you don't need to in `@see java.io...`
##########
src/main/java/org/apache/commons/io/build/AbstractStreamBuilder.java:
##########
@@ -258,6 +260,23 @@ public Writer getWriter() throws IOException {
return checkOrigin().getWriter(getCharset(), getOpenOptions());
}
+ /**
+ * Gets a Channel from the origin with OpenOption[].
+ *
+ * @param channelType The channel type, not null.
+ * @return A channel of the specified type.
+ * @param <C> The channel type.
+ * @throws IllegalStateException if the {@code origin} is {@code
null}.
+ * @throws UnsupportedOperationException if the origin cannot be converted
to a {@link ReadableByteChannel}.
+ * @throws IOException if an I/O error occurs.
+ * @see AbstractOrigin#getChannel
+ * @see #getOpenOptions()
+ * @since 2.21.0
+ */
+ public <C extends Channel> C getChannel(Class<C> channelType) throws
IOException {
Review Comment:
Yeah, that's likely a good general solution 👍
##########
src/main/java/org/apache/commons/io/channels/ByteArraySeekableByteChannel.java:
##########
@@ -37,41 +37,74 @@
* accessed via {@link ByteArraySeekableByteChannel#array()}.
* </p>
*
- * @since 2.19.0
+ * @since 2.21.0
*/
public class ByteArraySeekableByteChannel implements SeekableByteChannel {
private static final int RESIZE_LIMIT = Integer.MAX_VALUE >> 1;
+
+ /**
+ * Constructs a new channel backed directly by the given byte array.
+ *
+ * <p>The channel initially contains the full contents of the array, with
its
+ * size set to {@code bytes.length} and its position set to {@code 0}.</p>
+ *
+ * <p>Reads and writes operate on the shared array.
+ * If a write operation extends beyond the current capacity, the channel
will
+ * automatically allocate a larger backing array and copy the existing
contents.</p>
+ *
+ * @param bytes The byte array to wrap, must not be {@code null}
+ * @return A new channel that uses the given array as its initial backing
store
+ * @throws NullPointerException If {@code bytes} is {@code null}
+ * @see #array()
+ * @see java.io.ByteArrayInputStream#ByteArrayInputStream(byte[])
+ */
+ public static ByteArraySeekableByteChannel wrap(byte[] bytes) {
+ Objects.requireNonNull(bytes, "bytes");
+ return new ByteArraySeekableByteChannel(bytes);
+ }
+
private byte[] data;
- private final AtomicBoolean closed = new AtomicBoolean();
+ private volatile boolean closed;
private int position;
private int size;
private final ReentrantLock lock = new ReentrantLock();
/**
- * Constructs a new instance using a default empty buffer.
+ * Constructs a new instance, with a default internal buffer capacity.
+ * <p>
+ * The initial size and position of the channel are 0.
+ * </p>
+ * <p>
+ * The default initial capacity is 32 bytes, although the capacity will
grow as needed when writing data.
+ * </p>
+ * @see java.io.ByteArrayOutputStream#ByteArrayOutputStream()
*/
public ByteArraySeekableByteChannel() {
- this(0);
+ this(32);
Review Comment:
Hm, why not `IOUtils.DEFAULT_BUFFER_SIZE` (8 KiB)? Confusingly we use `1024`
in our own `AbstractByteArrayOutputStream` and `ByteArrayOutputStream`.
I'm surprised this change doesn't break anything, because if you only make
that change, `ByteArraySeekableByteChannelTest` breaks.
When I tried this kinf of change here and in Compress'
`SeekableInMemoryByteChannel`, the tests broke when I changed the default size
to `IOUtils.DEFAULT_BUFFER_SIZE`. But now it works in
`SeekableInMemoryByteChannel`...
##########
src/main/java/org/apache/commons/io/channels/ByteArraySeekableByteChannel.java:
##########
@@ -37,41 +37,74 @@
* accessed via {@link ByteArraySeekableByteChannel#array()}.
* </p>
*
- * @since 2.19.0
+ * @since 2.21.0
*/
public class ByteArraySeekableByteChannel implements SeekableByteChannel {
private static final int RESIZE_LIMIT = Integer.MAX_VALUE >> 1;
+
+ /**
+ * Constructs a new channel backed directly by the given byte array.
+ *
+ * <p>The channel initially contains the full contents of the array, with
its
+ * size set to {@code bytes.length} and its position set to {@code 0}.</p>
+ *
+ * <p>Reads and writes operate on the shared array.
+ * If a write operation extends beyond the current capacity, the channel
will
+ * automatically allocate a larger backing array and copy the existing
contents.</p>
+ *
+ * @param bytes The byte array to wrap, must not be {@code null}
+ * @return A new channel that uses the given array as its initial backing
store
+ * @throws NullPointerException If {@code bytes} is {@code null}
+ * @see #array()
+ * @see java.io.ByteArrayInputStream#ByteArrayInputStream(byte[])
+ */
+ public static ByteArraySeekableByteChannel wrap(byte[] bytes) {
+ Objects.requireNonNull(bytes, "bytes");
+ return new ByteArraySeekableByteChannel(bytes);
+ }
+
private byte[] data;
- private final AtomicBoolean closed = new AtomicBoolean();
+ private volatile boolean closed;
Review Comment:
This seems unrelated. Why is this better? Just because it's simpler? Just
curious.
##########
src/test/java/org/apache/commons/io/channels/ByteArraySeekableByteChannelCompressTest.java:
##########
@@ -62,7 +66,7 @@ void testCloseIsIdempotent() throws Exception {
@ParameterizedTest
@ValueSource(ints = { 0, 1, 2, 3, 4, 5, 6 })
void testReadingFromAPositionAfterEndReturnsEOF(final int size) throws
Exception {
- try (SeekableByteChannel c = new ByteArraySeekableByteChannel(size)) {
+ try (SeekableByteChannel c = ByteArraySeekableByteChannel.wrap(new
byte[size])) {
Review Comment:
I don't think this is a good idea for reading tests. You want to test
reading from the same array over and over when you are reading because you want
to make sure there are no side-effects that incorrectly _write_ to the
underlying byte array.
##########
src/test/java/org/apache/commons/io/channels/ByteArraySeekableByteChannelCompressTest.java:
##########
@@ -42,6 +42,10 @@ class ByteArraySeekableByteChannelCompressTest {
private final byte[] testData = "Some
data".getBytes(StandardCharsets.UTF_8);
+ private byte[] getTestData() {
Review Comment:
This would be better named `cloneTestData` IMO to make clear to tests what
kind of data we are dealing with.
--
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]
