From f10bee905737b912cb8606649aeb8c4317b6dd43 Mon Sep 17 00:00:00 2001 From: Sergey Skrobotov Date: Tue, 10 Dec 2019 02:29:44 -0800 Subject: [PATCH] Change DefaultByteBufHolder.equals() to treat instances of different classes as not equal (#9855) # Motivation: `DefaultByteBufHolder.equals()` considers another object equal if it's an instance of `ByteBufferHolder` and if the contents of two objects are equal. However, the behavior of `equals` method is not a part of the `ByteBufHolder` contract so `DefaultByteBufHolder`'s version may be causing violation of the symmetric property if other classes have different logic. There are already a few classes that are affected by this: `DefaultHttp2GoAwayFrame`, `DefaultHttp2UnknownFrame`, and `SctpMessage` are all overriding `equals` method breaking the symmetric property. Another effect of this behavior is that all instances with empty data are considered equal. That may not be desireable in the situations when instances are created for predefined constants, e.g. `FullBulkStringRedisMessage.NULL_INSTANCE` and `FullBulkStringRedisMessage.EMPTY_INSTANCE` in `codec-redis`. # Modification: Make `DefaultByteBufHolder.equals()` implementation only work for the objects of the same class. # Result: - The symmetric property of the `equals` method is restored for the classes in question. - Instances of different classes are not considered equal even if the content of the data they hold are the same. --- .../io/netty/buffer/DefaultByteBufHolder.java | 15 ++++- .../buffer/DefaultByteBufHolderTest.java | 60 +++++++++++++++++++ .../codec/http2/Http2DefaultFramesTest.java | 44 ++++++++++++++ .../handler/codec/redis/RedisDecoderTest.java | 8 +++ 4 files changed, 125 insertions(+), 2 deletions(-) create mode 100644 codec-http2/src/test/java/io/netty/handler/codec/http2/Http2DefaultFramesTest.java diff --git a/buffer/src/main/java/io/netty/buffer/DefaultByteBufHolder.java b/buffer/src/main/java/io/netty/buffer/DefaultByteBufHolder.java index 5c7707b9bf..f69fbea16e 100644 --- a/buffer/src/main/java/io/netty/buffer/DefaultByteBufHolder.java +++ b/buffer/src/main/java/io/netty/buffer/DefaultByteBufHolder.java @@ -135,13 +135,24 @@ public class DefaultByteBufHolder implements ByteBufHolder { return StringUtil.simpleClassName(this) + '(' + contentToString() + ')'; } + /** + * This implementation of the {@code equals} operation is restricted to + * work only with instances of the same class. The reason for that is that + * Netty library already has a number of classes that extend {@link DefaultByteBufHolder} and + * override {@code equals} method with an additional comparison logic and we + * need the symmetric property of the {@code equals} operation to be preserved. + * + * @param o the reference object with which to compare. + * @return {@code true} if this object is the same as the obj + * argument; {@code false} otherwise. + */ @Override public boolean equals(Object o) { if (this == o) { return true; } - if (o instanceof ByteBufHolder) { - return data.equals(((ByteBufHolder) o).content()); + if (o != null && getClass() == o.getClass()) { + return data.equals(((DefaultByteBufHolder) o).data); } return false; } diff --git a/buffer/src/test/java/io/netty/buffer/DefaultByteBufHolderTest.java b/buffer/src/test/java/io/netty/buffer/DefaultByteBufHolderTest.java index 4c60d0e7b6..6fd8aca731 100644 --- a/buffer/src/test/java/io/netty/buffer/DefaultByteBufHolderTest.java +++ b/buffer/src/test/java/io/netty/buffer/DefaultByteBufHolderTest.java @@ -42,4 +42,64 @@ public class DefaultByteBufHolderTest { copy.release(); } } + + @SuppressWarnings("SimplifiableJUnitAssertion") + @Test + public void testDifferentClassesAreNotEqual() { + // all objects here have EMPTY_BUFFER data but are instances of different classes + // so we want to check that none of them are equal to another. + ByteBufHolder dflt = new DefaultByteBufHolder(Unpooled.EMPTY_BUFFER); + ByteBufHolder other = new OtherByteBufHolder(Unpooled.EMPTY_BUFFER, 123); + ByteBufHolder constant1 = new DefaultByteBufHolder(Unpooled.EMPTY_BUFFER) { + // intentionally empty + }; + ByteBufHolder constant2 = new DefaultByteBufHolder(Unpooled.EMPTY_BUFFER) { + // intentionally empty + }; + try { + // not using 'assertNotEquals' to be explicit about which object we are calling .equals() on + assertFalse(dflt.equals(other)); + assertFalse(dflt.equals(constant1)); + assertFalse(constant1.equals(dflt)); + assertFalse(constant1.equals(other)); + assertFalse(constant1.equals(constant2)); + } finally { + dflt.release(); + other.release(); + constant1.release(); + constant2.release(); + } + } + + private static class OtherByteBufHolder extends DefaultByteBufHolder { + + private final int extraField; + + OtherByteBufHolder(final ByteBuf data, final int extraField) { + super(data); + this.extraField = extraField; + } + + @Override + public boolean equals(final Object o) { + if (this == o) { + return true; + } + if (o == null || getClass() != o.getClass()) { + return false; + } + if (!super.equals(o)) { + return false; + } + final OtherByteBufHolder that = (OtherByteBufHolder) o; + return extraField == that.extraField; + } + + @Override + public int hashCode() { + int result = super.hashCode(); + result = 31 * result + extraField; + return result; + } + } } diff --git a/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2DefaultFramesTest.java b/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2DefaultFramesTest.java new file mode 100644 index 0000000000..86457d4d1d --- /dev/null +++ b/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2DefaultFramesTest.java @@ -0,0 +1,44 @@ +/* + * Copyright 2019 The Netty Project + * + * The Netty Project licenses this file to you under the Apache License, version 2.0 (the + * "License"); you may not use this file except in compliance with the License. You may obtain a + * copy of the License at: + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software distributed under the License + * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express + * or implied. See the License for the specific language governing permissions and limitations under + * the License. + */ + +package io.netty.handler.codec.http2; + +import io.netty.buffer.DefaultByteBufHolder; +import io.netty.buffer.Unpooled; +import org.junit.Test; + +import static org.junit.Assert.assertFalse; + +public class Http2DefaultFramesTest { + + @SuppressWarnings("SimplifiableJUnitAssertion") + @Test + public void testEqualOperation() { + // in this case, 'goAwayFrame' and 'unknownFrame' will also have an EMPTY_BUFFER data + // so we want to check that 'dflt' will not consider them equal. + DefaultHttp2GoAwayFrame goAwayFrame = new DefaultHttp2GoAwayFrame(1); + DefaultHttp2UnknownFrame unknownFrame = new DefaultHttp2UnknownFrame((byte) 1, new Http2Flags((short) 1)); + DefaultByteBufHolder dflt = new DefaultByteBufHolder(Unpooled.EMPTY_BUFFER); + try { + // not using 'assertNotEquals' to be explicit about which object we are calling .equals() on + assertFalse(dflt.equals(goAwayFrame)); + assertFalse(dflt.equals(unknownFrame)); + } finally { + goAwayFrame.release(); + unknownFrame.release(); + dflt.release(); + } + } +} diff --git a/codec-redis/src/test/java/io/netty/handler/codec/redis/RedisDecoderTest.java b/codec-redis/src/test/java/io/netty/handler/codec/redis/RedisDecoderTest.java index 6d9bd19de1..c62ab40f76 100644 --- a/codec-redis/src/test/java/io/netty/handler/codec/redis/RedisDecoderTest.java +++ b/codec-redis/src/test/java/io/netty/handler/codec/redis/RedisDecoderTest.java @@ -306,4 +306,12 @@ public class RedisDecoderTest { ReferenceCountUtil.release(msg); ReferenceCountUtil.release(childBuf); } + + @Test + public void testPredefinedMessagesNotEqual() { + // both EMPTY_INSTANCE and NULL_INSTANCE have EMPTY_BUFFER as their 'data', + // however we need to check that they are not equal between themselves. + assertNotEquals(FullBulkStringRedisMessage.EMPTY_INSTANCE, FullBulkStringRedisMessage.NULL_INSTANCE); + assertNotEquals(FullBulkStringRedisMessage.NULL_INSTANCE, FullBulkStringRedisMessage.EMPTY_INSTANCE); + } }