garydgregory commented on code in PR #799:
URL: https://github.com/apache/commons-io/pull/799#discussion_r2428743796
##########
src/main/java/org/apache/commons/io/channels/CloseShieldChannel.java:
##########
@@ -66,10 +69,32 @@ public static <T extends Channel> T wrap(final T channel) {
// Collect only Channel sub-interfaces.
final Set<Class<?>> set = collectChannelInterfaces(channel.getClass(),
new LinkedHashSet<>());
// fallback to root surface
- return (T) Proxy.newProxyInstance(channel.getClass().getClassLoader(),
// use delegate's loader
+ return (Channel)
Proxy.newProxyInstance(channel.getClass().getClassLoader(), // use delegate's
loader
set.isEmpty() ? new Class<?>[] { Channel.class } :
set.toArray(EMPTY), new CloseShieldChannelHandler(channel));
}
+ /**
+ * Wraps a channel to shield it from being closed.
+ *
+ * @param channel The underlying channel to shield, not {@code null} and
must implement {@code type}.
+ * @param type The interface the returned proxy must implement;
+ * the proxy will also implement all other {@link Channel}
sub-interfaces that the delegate implements.
+ * @param <T> A type that extends {@link Channel}.
+ * @return A proxy that shields {@code close()} and enforces closed
semantics on other calls.
+ * @throws IllegalArgumentException if {@code type} is not an interface or
if {@code channel} does not implement {@code type}.
+ */
+ @SuppressWarnings({ "unchecked", "resource" }) // caller closes
+ public static <T extends Channel> T wrap(final T channel, Class<T> type) {
+ Objects.requireNonNull(type, "type");
+ if (!type.isInterface()) {
+ throw new IllegalArgumentException(type.getName() + " is not an
interface");
+ }
+ if (!type.isInstance(channel)) {
+ throw new IllegalArgumentException("channel of type " +
channel.getClass().getName() + " is not an instance of " + type.getName());
Review Comment:
I was a bit surprised to see an ISE instead of a `ClassCastException`, which
the call to `cast()` checks _again_, so I'm not sure this needs to be here at
all when `cast()` provides a more precise exception.
WRT error messages in general, the sentence should start with a capital
letter. In general, it would be nice to match the exception message terminology
with the Javadoc: "... doesn't implement..."
##########
src/main/java/org/apache/commons/io/channels/CloseShieldChannel.java:
##########
@@ -66,10 +69,32 @@ public static <T extends Channel> T wrap(final T channel) {
// Collect only Channel sub-interfaces.
final Set<Class<?>> set = collectChannelInterfaces(channel.getClass(),
new LinkedHashSet<>());
// fallback to root surface
- return (T) Proxy.newProxyInstance(channel.getClass().getClassLoader(),
// use delegate's loader
+ return (Channel)
Proxy.newProxyInstance(channel.getClass().getClassLoader(), // use delegate's
loader
set.isEmpty() ? new Class<?>[] { Channel.class } :
set.toArray(EMPTY), new CloseShieldChannelHandler(channel));
}
+ /**
+ * Wraps a channel to shield it from being closed.
Review Comment:
Is this API for cases like `FileChannel`? Are there others?
`SelectableChannel`?
If so, it might be worth documenting that this is for the case represented
by `FileChannel`, which implements many `Channel` interfaces and you, the
developer, have to pick one.
But this is really a shortcoming of our implementation because we use
proxies. So it is unfortunate that an implementation detail (proxies) forces us
to provide a new public API. Or, should we special-case `FileChannel` and
create a shield class for it?
##########
src/main/java/org/apache/commons/io/channels/CloseShieldChannel.java:
##########
@@ -66,10 +69,32 @@ public static <T extends Channel> T wrap(final T channel) {
// Collect only Channel sub-interfaces.
final Set<Class<?>> set = collectChannelInterfaces(channel.getClass(),
new LinkedHashSet<>());
// fallback to root surface
- return (T) Proxy.newProxyInstance(channel.getClass().getClassLoader(),
// use delegate's loader
+ return (Channel)
Proxy.newProxyInstance(channel.getClass().getClassLoader(), // use delegate's
loader
set.isEmpty() ? new Class<?>[] { Channel.class } :
set.toArray(EMPTY), new CloseShieldChannelHandler(channel));
}
+ /**
+ * Wraps a channel to shield it from being closed.
+ *
+ * @param channel The underlying channel to shield, not {@code null} and
must implement {@code type}.
+ * @param type The interface the returned proxy must implement;
+ * the proxy will also implement all other {@link Channel}
sub-interfaces that the delegate implements.
+ * @param <T> A type that extends {@link Channel}.
+ * @return A proxy that shields {@code close()} and enforces closed
semantics on other calls.
+ * @throws IllegalArgumentException if {@code type} is not an interface or
if {@code channel} does not implement {@code type}.
+ */
+ @SuppressWarnings({ "unchecked", "resource" }) // caller closes
Review Comment:
"unchecked" is not needed.
##########
src/main/java/org/apache/commons/io/channels/CloseShieldChannel.java:
##########
@@ -53,11 +57,10 @@ private static Set<Class<?>> collectChannelInterfaces(final
Class<?> type, final
* Wraps a channel to shield it from being closed.
*
* @param channel The underlying channel to shield, not {@code null}.
- * @param <T> Any Channel type (interface or class).
* @return A proxy that shields {@code close()} and enforces closed
semantics on other calls.
*/
@SuppressWarnings({ "unchecked", "resource" }) // caller closes
- public static <T extends Channel> T wrap(final T channel) {
+ public static Channel wrap(final Channel channel) {
Review Comment:
I wouldn't want to lose generics here.
##########
src/test/java/org/apache/commons/io/channels/CloseShieldChannelTest.java:
##########
@@ -116,8 +122,8 @@ void testCloseIsShielded(final Class<? extends Channel>
channelClass) throws Exc
@Test
void testDoesNotDoubleWrap() {
final ByteChannel channel = mock(ByteChannel.class);
- final ByteChannel shield1 = CloseShieldChannel.wrap(channel);
- final ByteChannel shield2 = CloseShieldChannel.wrap(shield1);
+ final ByteChannel shield1 = CloseShieldChannel.wrap(channel,
ByteChannel.class);
+ final ByteChannel shield2 = CloseShieldChannel.wrap(shield1,
ByteChannel.class);
Review Comment:
We shouldn't _remove_ existing test code for an existing API that work as
they are. There is no point in losing the generics here. It feels like a step
backward. Since both APIs represent slightly different and overlapping cases,
we should test both, IOW, keep the old, add some new.
--
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]
