Fix Map HashCode Collision (#44)

felangel · Nov 24, 2019 · 90e970e · 90e970e
1 parent 7d65ce9
commit 90e970e
Show file tree

Hide file tree

Showing 4 changed files with 85 additions and 9 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,3 +1,7 @@
+# 1.0.1
+
+- Fix `hashCode` collisions with `Map` properties ([#43](https://github.com/felangel/equatable/issues/43)).
+
 # 1.0.0
 
 - Update hashCode implementation to use `Jenkins Hash` ([#39](https://github.com/felangel/equatable/issues/39)).

diff --git a/lib/src/equatable_utils.dart b/lib/src/equatable_utils.dart
@@ -15,7 +15,7 @@ bool equals(List list1, List list2) {
     final unit1 = list1[i];
     final unit2 = list2[i];
 
-    if (unit1 is Iterable || unit1 is List || unit1 is Map || unit1 is Set) {
+    if (unit1 is Iterable || unit1 is Map) {
       if (!_equality.equals(unit1, unit2)) return false;
     } else if (unit1?.runtimeType != unit2?.runtimeType) {
       return false;
@@ -31,13 +31,11 @@ bool equals(List list1, List list2) {
 int _combine(int hash, dynamic object) {
   if (object is Map) {
     object.forEach((key, value) {
-      hash = _combine(hash, [key, value]);
+      hash = hash ^ _combine(hash, [key, value]);
     });
     return hash;
   }
-  if (object is Iterable) {
-    return mapPropsToHashCode(object);
-  }
+  if (object is Iterable) return mapPropsToHashCode(object);
   hash = 0x1fffffff & (hash + object.hashCode);
   hash = 0x1fffffff & (hash + ((0x0007ffff & hash) << 10));
   return hash ^ (hash >> 6);

diff --git a/pubspec.yaml b/pubspec.yaml
@@ -1,6 +1,6 @@
 name: equatable
 description: An abstract class that helps to implement equality without needing to explicitly override == and hashCode.
-version: 1.0.0
+version: 1.0.1
 author: Felix Angelov <[email protected]>
 homepage: https://github.com/felangel/equatable
 

diff --git a/test/equatable_test.dart b/test/equatable_test.dart
@@ -442,6 +442,38 @@ void main() {
       );
       expect(instanceA == instanceB, false);
     });
+
+    test('should return false when values only differ in list', () {
+      final instanceA = ComplexEquatable(
+        name: 'Joe',
+        age: 40,
+        hairColor: Color.black,
+        children: ['Bob'],
+      );
+      final instanceB = ComplexEquatable(
+        name: 'Joe',
+        age: 40,
+        hairColor: Color.black,
+        children: ['Bobby'],
+      );
+      expect(instanceA == instanceB, false);
+    });
+
+    test('should return false when values only differ in single property', () {
+      final instanceA = ComplexEquatable(
+        name: 'Joe',
+        age: 40,
+        hairColor: Color.black,
+        children: ['Bob'],
+      );
+      final instanceB = ComplexEquatable(
+        name: 'Joe',
+        age: 41,
+        hairColor: Color.black,
+        children: ['Bob'],
+      );
+      expect(instanceA == instanceB, false);
+    });
   });
 
   group('Json Equatable', () {
@@ -592,29 +624,71 @@ void main() {
         expect(instanceA == instanceB, true);
         expect(instanceA.hashCode == instanceB.hashCode, true);
       });
+
+      test(
+          'should return different hashCode when instance properties are different',
+          () {
+        final instanceA = SimpleEquatable<List>(["A", "B"]);
+        final instanceB = SimpleEquatable<List>(["B"]);
+
+        expect(instanceA != instanceB, true);
+        expect(instanceA.hashCode != instanceB.hashCode, true);
+      });
+
+      test(
+          'should return different hashCode when instance properties are modified',
+          () {
+        final list = ["A", "B"];
+        final instanceA = SimpleEquatable<List>(list);
+        final hashCodeA = instanceA.hashCode;
+        list.removeLast();
+        final hashCodeB = instanceA.hashCode;
+        expect(hashCodeA != hashCodeB, true);
+      });
     });
 
     group('Map Equatable', () {
-      test('should return when values are same', () {
+      test('should return true when values are same', () {
         final instanceA = SimpleEquatable<Map>({1: "A", 2: "B"});
         final instanceB = SimpleEquatable<Map>({1: "A", 2: "B"});
         expect(instanceA == instanceB, true);
         expect(instanceA.hashCode == instanceB.hashCode, true);
       });
 
-      test('should return when values are different', () {
+      test('should return false when values are different', () {
         final instanceA = SimpleEquatable<Map>({1: "A", 2: "B"});
         final instanceB = SimpleEquatable<Map>({1: "a", 2: "b"});
         expect(instanceA != instanceB, true);
         expect(instanceA.hashCode != instanceB.hashCode, true);
       });
 
-      test('should return when values are different', () {
+      test('should return false when values are different', () {
         final instanceA = SimpleEquatable<Map>({1: "A", 2: "B"});
         final instanceB = SimpleEquatable<Map>({1: "C", 2: "D"});
         expect(instanceA != instanceB, true);
         expect(instanceA.hashCode != instanceB.hashCode, true);
       });
+
+      test(
+          'should return different hashCode when instance properties are different',
+          () {
+        final instanceA = SimpleEquatable<Map>({1: "A", 2: "B"});
+        final instanceB = SimpleEquatable<Map>({2: "B"});
+
+        expect(instanceA != instanceB, true);
+        expect(instanceA.hashCode != instanceB.hashCode, true);
+      });
+
+      test(
+          'should return different hashCode when instance properties are modified',
+          () {
+        final map = {1: "A", 2: "B"};
+        final instanceA = SimpleEquatable<Map>(map);
+        final hashCodeA = instanceA.hashCode;
+        map.remove(1);
+        final hashCodeB = instanceA.hashCode;
+        expect(hashCodeA != hashCodeB, true);
+      });
     });
 
     group('Set Equatable', () {