Skip to content

Commit

Permalink
Fix ImmutableTree incorrectly store hash collided value (#184)
Browse files Browse the repository at this point in the history 
* Update TreeTests to cover the hash collision bug

* Fix ImmutableTree incorrectly store hash collided value
  • Loading branch information
@foriequal0
foriequal0 authored Feb 18, 2025
1 parent e1972b5 commit 54bf6a5
Show file tree
Hide file tree
Showing 2 changed files with 52 additions and 50 deletions.
9 changes: 5 additions & 4 deletions src/Utils/Data/Immutable/ImmutableTree.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -408,11 +408,12 @@ private ImmutableTree<TKey, TValue> CheckCollision(int hash, TKey key, TValue va
return new ImmutableTree<TKey, TValue>(hash, this.storedKey!, this.storedValue!, this.leftNode!, this.rightNode!,
ImmutableBucket<TKey, TValue>.Empty.Add(key, value));

var @new = updateDelegate == null || forceUpdate
var collision = byRef ? this.collisions.GetOrDefaultByRef(key) : this.collisions.GetOrDefaultByValue(key);
var @new = collision == null || updateDelegate == null || forceUpdate
? value
: updateDelegate(this.storedValue!, value);
: updateDelegate(collision, value);

if (ReferenceEquals(@new, this.storedValue))
if (ReferenceEquals(@new, collision))
return this;

return new ImmutableTree<TKey, TValue>(hash, this.storedKey!, this.storedValue!, this.leftNode!, this.rightNode!,
Expand Down Expand Up @@ -738,4 +739,4 @@ internal class ImmutableRefTreeDebugView<TValue> where TValue : class

[DebuggerBrowsable(DebuggerBrowsableState.RootHidden)]
public TValue[] Items => tree.Walk().ToArray();

Check warning on line 741 in src/Utils/Data/Immutable/ImmutableTree.cs

View workflow job for this annotation

GitHub Actions / Run analysis & code coverage 

Refactor 'Items' into a method, properties should not copy collections. (https://rules.sonarsource.com/csharp/RSPEC-2365)
}
}
93 changes: 47 additions & 46 deletions test/DataTests/TreeTests.cs
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using System;
using System.Collections.Generic;
using Stashbox.Tests.Utils;
using Stashbox.Utils.Data;
using Stashbox.Utils.Data.Immutable;
Expand All @@ -17,38 +18,38 @@ public void Test_Collision_Check()
var a = new A();
var b = new B();
var c = new C();
var imTree = ImmutableTree<object, object>.Empty;
var tree = new HashTree<object, object>();
var imTree = ImmutableTree<object, List<object>>.Empty;
var tree = new HashTree<object, List<object>>();

imTree = imTree.AddOrUpdate(key1, a, false);
imTree = imTree.AddOrUpdate(key2, b, false);
tree.Add(key1, a);
tree.Add(key2, b);

Assert.Equal(a, imTree.GetOrDefaultByValue(key1));
Assert.Equal(b, imTree.GetOrDefaultByValue(key2));
Assert.Equal(a, tree.GetOrDefault(key1));
Assert.Equal(b, tree.GetOrDefault(key2));
imTree = imTree.AddOrUpdate(key1, [a], false);
imTree = imTree.AddOrUpdate(key2, [b], false);
tree.Add(key1, [a]);
tree.Add(key2, [b]);

Assert.Equal([a], imTree.GetOrDefaultByValue(key1));
Assert.Equal([b], imTree.GetOrDefaultByValue(key2));
Assert.Equal([a], tree.GetOrDefault(key1));
Assert.Equal([b], tree.GetOrDefault(key2));

imTree = imTree.AddOrUpdate(key2, c, false, (_, n) => n);
imTree = imTree.AddOrUpdate(key2, [c], false, (list, n) => [..list, ..n]);

Assert.Equal(a, imTree.GetOrDefaultByValue(key1));
Assert.Equal(c, imTree.GetOrDefaultByValue(key2));
Assert.Equal([a], imTree.GetOrDefaultByValue(key1));
Assert.Equal([b, c], imTree.GetOrDefaultByValue(key2));

imTree = imTree.AddOrUpdate(key2, b, false, true);
imTree = imTree.AddOrUpdate(key2, [b], false, true);

Assert.Equal(a, imTree.GetOrDefaultByValue(key1));
Assert.Equal(b, imTree.GetOrDefaultByValue(key2));
Assert.Equal([a], imTree.GetOrDefaultByValue(key1));
Assert.Equal([b], imTree.GetOrDefaultByValue(key2));

imTree = imTree.UpdateIfExists(key2, false, _ => c);
imTree = imTree.UpdateIfExists(key2, false, list => [..list, c]);

Assert.Equal(a, imTree.GetOrDefaultByValue(key1));
Assert.Equal(c, imTree.GetOrDefaultByValue(key2));
Assert.Equal([a], imTree.GetOrDefaultByValue(key1));
Assert.Equal([b, c], imTree.GetOrDefaultByValue(key2));

imTree = imTree.UpdateIfExists(key2, b, false);
imTree = imTree.UpdateIfExists(key2, [b], false);

Assert.Equal(a, imTree.GetOrDefaultByValue(key1));
Assert.Equal(b, imTree.GetOrDefaultByValue(key2));
Assert.Equal([a], imTree.GetOrDefaultByValue(key1));
Assert.Equal([b], imTree.GetOrDefaultByValue(key2));
}

[Fact]
Expand All @@ -59,38 +60,38 @@ public void Test_Ref_Collision_Check()
var a = new A();
var b = new B();
var c = new C();
var imTree = ImmutableTree<Type, object>.Empty;
var tree = new HashTree<Type, object>();
var imTree = ImmutableTree<Type, List<object>>.Empty;
var tree = new HashTree<Type, List<object>>();

imTree = imTree.AddOrUpdate(key1, a, true);
imTree = imTree.AddOrUpdate(key2, b, true);
tree.Add(key1, a);
tree.Add(key2, b);

Assert.Equal(a, imTree.GetOrDefaultByRef(key1));
Assert.Equal(b, imTree.GetOrDefaultByRef(key2));
Assert.Equal(a, tree.GetOrDefault(key1));
Assert.Equal(b, tree.GetOrDefault(key2));
imTree = imTree.AddOrUpdate(key1, [a], true);
imTree = imTree.AddOrUpdate(key2, [b], true);
tree.Add(key1, [a]);
tree.Add(key2, [b]);

Assert.Equal([a], imTree.GetOrDefaultByRef(key1));
Assert.Equal([b], imTree.GetOrDefaultByRef(key2));
Assert.Equal([a], tree.GetOrDefault(key1));
Assert.Equal([b], tree.GetOrDefault(key2));

imTree = imTree.AddOrUpdate(key2, c, true, (_, n) => n);
imTree = imTree.AddOrUpdate(key2, [c], true, (list, n) => [..list, ..n]);

Assert.Equal(a, imTree.GetOrDefaultByRef(key1));
Assert.Equal(c, imTree.GetOrDefaultByRef(key2));
Assert.Equal([a], imTree.GetOrDefaultByRef(key1));
Assert.Equal([b, c], imTree.GetOrDefaultByRef(key2));

imTree = imTree.AddOrUpdate(key2, b, true, true);
imTree = imTree.AddOrUpdate(key2, [b], true, true);

Assert.Equal(a, imTree.GetOrDefaultByRef(key1));
Assert.Equal(b, imTree.GetOrDefaultByRef(key2));
Assert.Equal([a], imTree.GetOrDefaultByRef(key1));
Assert.Equal([b], imTree.GetOrDefaultByRef(key2));

imTree = imTree.UpdateIfExists(key2, true, _ => c);
imTree = imTree.UpdateIfExists(key2, true, list => [..list, c]);

Assert.Equal(a, imTree.GetOrDefaultByRef(key1));
Assert.Equal(c, imTree.GetOrDefaultByRef(key2));
Assert.Equal([a], imTree.GetOrDefaultByRef(key1));
Assert.Equal([b, c], imTree.GetOrDefaultByRef(key2));

imTree = imTree.UpdateIfExists(key2, b, true);
imTree = imTree.UpdateIfExists(key2, [b], true);

Assert.Equal(a, imTree.GetOrDefaultByRef(key1));
Assert.Equal(b, imTree.GetOrDefaultByRef(key2));
Assert.Equal([a], imTree.GetOrDefaultByRef(key1));
Assert.Equal([b], imTree.GetOrDefaultByRef(key2));
}

[Fact]
Expand Down

0 comments on commit 54bf6a5

Please sign in to comment.