From 97f1df2c7c92f9e2486d6b75ed38831ff35a1f19 Mon Sep 17 00:00:00 2001 From: Eno Thereska Date: Fri, 29 Apr 2016 19:38:17 +0100 Subject: [PATCH 1/2] Fix equality semantics of KeyValue --- .../org/apache/kafka/streams/KeyValue.java | 16 ++--- .../apache/kafka/streams/KeyValueTest.java | 65 +++++++++++++------ 2 files changed, 52 insertions(+), 29 deletions(-) diff --git a/streams/src/main/java/org/apache/kafka/streams/KeyValue.java b/streams/src/main/java/org/apache/kafka/streams/KeyValue.java index 58f2083b8457b..64b38cdfbf36c 100644 --- a/streams/src/main/java/org/apache/kafka/streams/KeyValue.java +++ b/streams/src/main/java/org/apache/kafka/streams/KeyValue.java @@ -63,22 +63,22 @@ public String toString() { } @Override - public boolean equals(Object other) { - if (this == other) + public boolean equals(Object obj) { + if (this == obj) return true; - if (other instanceof KeyValue) { - KeyValue otherKV = (KeyValue) other; - - return key == null ? otherKV.key == null : key.equals(otherKV.key) - && value == null ? otherKV.value == null : value.equals(otherKV.value); - } else { + if (!(obj instanceof KeyValue)) { return false; } + + KeyValue other = (KeyValue) obj; + return (this.key == null ? other.key == null : this.key.equals(other.key)) + && (this.value == null ? other.value == null : this.value.equals(other.value)); } @Override public int hashCode() { return Objects.hash(key, value); } + } diff --git a/streams/src/test/java/org/apache/kafka/streams/KeyValueTest.java b/streams/src/test/java/org/apache/kafka/streams/KeyValueTest.java index 47c8ecd1c2280..6d23fe60c2d90 100644 --- a/streams/src/test/java/org/apache/kafka/streams/KeyValueTest.java +++ b/streams/src/test/java/org/apache/kafka/streams/KeyValueTest.java @@ -24,27 +24,50 @@ public class KeyValueTest { - private KeyValue kv1a = new KeyValue<>("key1", 1L); - private KeyValue kv1b = new KeyValue<>("key1", 1L); - private KeyValue kv2 = new KeyValue<>("key2", 2L); - private KeyValue kv3 = new KeyValue<>("key3", 3L); - @Test - public void testEquals() { - assertTrue(kv1a.equals(kv1a)); - assertTrue(kv1a.equals(kv1b)); - assertTrue(kv1b.equals(kv1a)); - assertFalse(kv1a.equals(kv2)); - assertFalse(kv1a.equals(kv3)); - assertFalse(kv2.equals(kv3)); - assertFalse(kv1a.equals(null)); - } + public void shouldHaveSaneEqualsAndHashCode() { + KeyValue kv = KeyValue.pair("key1", 1L); + KeyValue copyOfKV = KeyValue.pair(kv.key, kv.value); - @Test - public void testHashcode() { - assertTrue(kv1a.hashCode() == kv1b.hashCode()); - assertFalse(kv1a.hashCode() == kv2.hashCode()); - assertFalse(kv1a.hashCode() == kv3.hashCode()); - assertFalse(kv2.hashCode() == kv3.hashCode()); + // Reflexive + assertTrue(kv.equals(kv)); + assertTrue(kv.hashCode() == kv.hashCode()); + + // Symmetric + assertTrue(kv.equals(copyOfKV)); + assertTrue(kv.hashCode() == copyOfKV.hashCode()); + assertTrue(copyOfKV.hashCode() == kv.hashCode()); + + // Transitive + KeyValue copyOfCopyOfKV = KeyValue.pair(copyOfKV.key, copyOfKV.value); + assertTrue(copyOfKV.equals(copyOfCopyOfKV)); + assertTrue(copyOfKV.hashCode() == copyOfCopyOfKV.hashCode()); + assertTrue(kv.equals(copyOfCopyOfKV)); + assertTrue(kv.hashCode() == copyOfCopyOfKV.hashCode()); + + // Inequality scenarios + assertFalse("must be false for null", kv.equals(null)); + assertFalse("must be false if key is non-null and other key is null", kv.equals(KeyValue.pair(null, kv.value))); + assertFalse("must be false if value is non-null and other value is null", kv.equals(KeyValue.pair(kv.key, null))); + KeyValue differentKeyType = KeyValue.pair(1L, kv.value); + assertFalse("must be false for different key types", kv.equals(differentKeyType)); + KeyValue differentValueType = KeyValue.pair(kv.key, "anyString"); + assertFalse("must be false for different value types", kv.equals(differentValueType)); + KeyValue differentKeyValueTypes = KeyValue.pair(1L, "anyString"); + assertFalse("must be false for different key and value types", kv.equals(differentKeyValueTypes)); + assertFalse("must be false for different types of objects", kv.equals(new Object())); + + KeyValue differentKey = KeyValue.pair(kv.key + "suffix", kv.value); + assertFalse("must be false if key is different", kv.equals(differentKey)); + assertFalse("must be false if key is different", differentKey.equals(kv)); + + KeyValue differentValue = KeyValue.pair(kv.key, kv.value + 1L); + assertFalse("must be false if value is different", kv.equals(differentValue)); + assertFalse("must be false if value is different", differentValue.equals(kv)); + + KeyValue differentKeyAndValue = KeyValue.pair(kv.key + "suffix", kv.value + 1L); + assertFalse("must be false if value is different", kv.equals(differentKeyAndValue)); + assertFalse("must be false if value is different", differentKeyAndValue.equals(kv)); } -} + +} \ No newline at end of file From 17f450764687f32387f22a58ab17bb29096b43bd Mon Sep 17 00:00:00 2001 From: "Michael G. Noll" Date: Fri, 29 Apr 2016 15:03:35 -0700 Subject: [PATCH 2/2] Fix typo in test asserts --- .../src/test/java/org/apache/kafka/streams/KeyValueTest.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/streams/src/test/java/org/apache/kafka/streams/KeyValueTest.java b/streams/src/test/java/org/apache/kafka/streams/KeyValueTest.java index 6d23fe60c2d90..805fa18369e62 100644 --- a/streams/src/test/java/org/apache/kafka/streams/KeyValueTest.java +++ b/streams/src/test/java/org/apache/kafka/streams/KeyValueTest.java @@ -66,8 +66,8 @@ public void shouldHaveSaneEqualsAndHashCode() { assertFalse("must be false if value is different", differentValue.equals(kv)); KeyValue differentKeyAndValue = KeyValue.pair(kv.key + "suffix", kv.value + 1L); - assertFalse("must be false if value is different", kv.equals(differentKeyAndValue)); - assertFalse("must be false if value is different", differentKeyAndValue.equals(kv)); + assertFalse("must be false if key and value are different", kv.equals(differentKeyAndValue)); + assertFalse("must be false if key and value are different", differentKeyAndValue.equals(kv)); } } \ No newline at end of file