Skip to content

[COLLECTIONS-885] PatriciaTrie. Prevent silent mutations, prevents CMEs when iterating. #672

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
griffinjm wants to merge 1 commit into
base: master
Choose a base branch
from
+234 −79
Open

[COLLECTIONS-885] PatriciaTrie. Prevent silent mutations, prevents CMEs when iterating. #672

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/main/java/org/apache/commons/collections4/trie/AbstractBitwiseTrie.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,9 +169,9 @@ final K castKey(final Object key) {
}

/**
* A utility method for calling {@link KeyAnalyzer#compare(Object, Object)}
* A null-safe utility method for calling {@link KeyAnalyzer#compare(Object, Object)}
*/
final boolean compareKeys(final K key, final K other) {
final boolean keysAreEqual(final K key, final K other) {
Copy link
Author

@griffinjm griffinjm Mar 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed as this method name is a little misleading, it's actually checking that the keys are equal, in a null-safe manner, returning a boolean, not an int as you would expect.

if (key == null) {
return other == null;
}
Expand Down
155 changes: 78 additions & 77 deletions src/main/java/org/apache/commons/collections4/trie/AbstractPatriciaTrie.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1331,24 +1331,6 @@ TrieEntry<K, V> addEntry(final TrieEntry<K, V> entry, final int lengthInBits) {
* than or equal to the given key, or null if there is no such key.
*/
TrieEntry<K, V> ceilingEntry(final K key) {
// Basically:
// Follow the steps of adding an entry, but instead...
//
// - If we ever encounter a situation where we found an equal
// key, we return it immediately.
//
// - If we hit an empty root, return the first iterable item.
//
// - If we have to add a new item, we temporarily add it,
// find the successor to it, then remove the added item.
//
// These steps ensure that the returned value is either the
// entry for the key itself, or the first entry directly after
// the key.

// TODO: Cleanup so that we don't actually have to add/remove from the
// tree. (We do it here because there are other well-defined
// functions to perform the search.)
final int lengthInBits = lengthInBits(key);

if (lengthInBits == 0) {
Expand All @@ -1359,19 +1341,31 @@ TrieEntry<K, V> ceilingEntry(final K key) {
}

final TrieEntry<K, V> found = getNearestEntryForKey(key, lengthInBits);
if (compareKeys(key, found.key)) {
if (keysAreEqual(key, found.key)) {
return found;
}

final int bitIndex = bitIndex(key, found.key);
if (KeyAnalyzer.isValidBitIndex(bitIndex)) {
final TrieEntry<K, V> added = new TrieEntry<>(key, null, bitIndex);
addEntry(added, lengthInBits);
incrementSize(); // must increment because remove will decrement
final TrieEntry<K, V> ceil = nextEntry(added);
removeEntry(added);
modCount -= 2; // we didn't really modify it.
return ceil;
if (!isBitSet(key, bitIndex, lengthInBits)) {
// search key < found.key
// found is a ceiling candidate, walk backward to find the smallest entry still >= key
TrieEntry<K, V> ceiling = found;
TrieEntry<K, V> prev = previousEntry(found);
while (prev != null && !prev.isEmpty() && getKeyAnalyzer().compare(key, prev.key) <= 0) {
ceiling = prev;
prev = previousEntry(prev);
}
return ceiling;
} else {
// search key > found.key
// walk forward to find the first entry.key > key
TrieEntry<K, V> next = nextEntry(found);
while (next != null && getKeyAnalyzer().compare(key, next.key) > 0) {
next = nextEntry(next);
}
return next;
}
}
if (KeyAnalyzer.isNullBitKey(bitIndex)) {
if (!root.isEmpty()) {
Expand Down Expand Up @@ -1416,7 +1410,7 @@ public boolean containsKey(final Object k) {
final K key = castKey(k);
final int lengthInBits = lengthInBits(key);
final TrieEntry<K, V> entry = getNearestEntryForKey(key, lengthInBits);
return !entry.isEmpty() && compareKeys(key, entry.key);
return !entry.isEmpty() && keysAreEqual(key, entry.key);
}

/**
Expand Down Expand Up @@ -1463,9 +1457,6 @@ public K firstKey() {
* less than or equal to the given key, or null if there is no such key.
*/
TrieEntry<K, V> floorEntry(final K key) {
// TODO: Cleanup so that we don't actually have to add/remove from the
// tree. (We do it here because there are other well-defined
// functions to perform the search.)
final int lengthInBits = lengthInBits(key);

if (lengthInBits == 0) {
Expand All @@ -1476,19 +1467,30 @@ TrieEntry<K, V> floorEntry(final K key) {
}

final TrieEntry<K, V> found = getNearestEntryForKey(key, lengthInBits);
if (compareKeys(key, found.key)) {
if (keysAreEqual(key, found.key)) {
return found;
}

final int bitIndex = bitIndex(key, found.key);
if (KeyAnalyzer.isValidBitIndex(bitIndex)) {
final TrieEntry<K, V> added = new TrieEntry<>(key, null, bitIndex);
addEntry(added, lengthInBits);
incrementSize(); // must increment because remove will decrement
final TrieEntry<K, V> floor = previousEntry(added);
removeEntry(added);
modCount -= 2; // we didn't really modify it.
return floor;
if (isBitSet(key, bitIndex, lengthInBits)) {
TrieEntry<K, V> floor = found;
TrieEntry<K, V> next = nextEntry(found);
while (next != null && getKeyAnalyzer().compare(key, next.key) >= 0) {
floor = next;
next = nextEntry(next);
}
return floor;
} else {
TrieEntry<K, V> prev = previousEntry(found);
while (prev != null && !prev.isEmpty() && getKeyAnalyzer().compare(key, prev.key) < 0) {
prev = previousEntry(prev);
}
if (prev == null || prev.isEmpty()) {
return null;
}
return prev;
}
}
if (KeyAnalyzer.isNullBitKey(bitIndex)) {
if (!root.isEmpty()) {
Expand Down Expand Up @@ -1561,7 +1563,7 @@ TrieEntry<K, V> getEntry(final Object k) {

final int lengthInBits = lengthInBits(key);
final TrieEntry<K, V> entry = getNearestEntryForKey(key, lengthInBits);
return !entry.isEmpty() && compareKeys(key, entry.key) ? entry : null;
return !entry.isEmpty() && keysAreEqual(key, entry.key) ? entry : null;
}

/**
Expand Down Expand Up @@ -1632,9 +1634,6 @@ public SortedMap<K, V> headMap(final K toKey) {
* or null if no such entry exists.
*/
TrieEntry<K, V> higherEntry(final K key) {
// TODO: Cleanup so that we don't actually have to add/remove from the
// tree. (We do it here because there are other well-defined
// functions to perform the search.)
final int lengthInBits = lengthInBits(key);

if (lengthInBits == 0) {
Expand All @@ -1651,19 +1650,27 @@ TrieEntry<K, V> higherEntry(final K key) {
}

final TrieEntry<K, V> found = getNearestEntryForKey(key, lengthInBits);
if (compareKeys(key, found.key)) {
if (keysAreEqual(key, found.key)) {
return nextEntry(found);
}

final int bitIndex = bitIndex(key, found.key);
if (KeyAnalyzer.isValidBitIndex(bitIndex)) {
final TrieEntry<K, V> added = new TrieEntry<>(key, null, bitIndex);
addEntry(added, lengthInBits);
incrementSize(); // must increment because remove will decrement
final TrieEntry<K, V> ceil = nextEntry(added);
removeEntry(added);
modCount -= 2; // we didn't really modify it.
return ceil;
if (!isBitSet(key, bitIndex, lengthInBits)) {
TrieEntry<K, V> ceiling = found;
TrieEntry<K, V> prev = previousEntry(found);
while (prev != null && !prev.isEmpty() && getKeyAnalyzer().compare(key, prev.key) <= 0) {
ceiling = prev;
prev = previousEntry(prev);
}
return ceiling;
} else {
TrieEntry<K, V> next = nextEntry(found);
while (next != null && getKeyAnalyzer().compare(key, next.key) > 0) {
next = nextEntry(next);
}
return next;
}
}
if (KeyAnalyzer.isNullBitKey(bitIndex)) {
if (!root.isEmpty()) {
Expand Down Expand Up @@ -1729,43 +1736,37 @@ public K lastKey() {
* strictly less than the given key, or null if there is no such key.
*/
TrieEntry<K, V> lowerEntry(final K key) {
// Basically:
// Follow the steps of adding an entry, but instead...
//
// - If we ever encounter a situation where we found an equal
// key, we return it's previousEntry immediately.
//
// - If we hit root (empty or not), return null.
//
// - If we have to add a new item, we temporarily add it,
// find the previousEntry to it, then remove the added item.
//
// These steps ensure that the returned value is always just before
// the key or null (if there was nothing before it).

// TODO: Cleanup so that we don't actually have to add/remove from the
// tree. (We do it here because there are other well-defined
// functions to perform the search.)
final int lengthInBits = lengthInBits(key);

if (lengthInBits == 0) {
return null; // there can never be anything before root.
}

final TrieEntry<K, V> found = getNearestEntryForKey(key, lengthInBits);
if (compareKeys(key, found.key)) {
if (keysAreEqual(key, found.key)) {
return previousEntry(found);
}

final int bitIndex = bitIndex(key, found.key);
if (KeyAnalyzer.isValidBitIndex(bitIndex)) {
final TrieEntry<K, V> added = new TrieEntry<>(key, null, bitIndex);
addEntry(added, lengthInBits);
incrementSize(); // must increment because remove will decrement
final TrieEntry<K, V> prior = previousEntry(added);
removeEntry(added);
modCount -= 2; // we didn't really modify it.
return prior;
if (isBitSet(key, bitIndex, lengthInBits)) {
TrieEntry<K, V> floor = found;
TrieEntry<K, V> next = nextEntry(found);
while (next != null && getKeyAnalyzer().compare(key, next.key) >= 0) {
floor = next;
next = nextEntry(next);
}
return floor;
} else {
TrieEntry<K, V> prev = previousEntry(found);
while (prev != null && !prev.isEmpty() && getKeyAnalyzer().compare(key, prev.key) < 0) {
prev = previousEntry(prev);
}
if (prev == null || prev.isEmpty()) {
return null;
}
return prev;
}
}
if (KeyAnalyzer.isNullBitKey(bitIndex)) {
return null;
Expand Down Expand Up @@ -2028,7 +2029,7 @@ public V put(final K key, final V value) {
}

final TrieEntry<K, V> found = getNearestEntryForKey(key, lengthInBits);
if (compareKeys(key, found.key)) {
if (keysAreEqual(key, found.key)) {
if (found.isEmpty()) { // <- must be the root
incrementSize();
} else {
Expand Down Expand Up @@ -2104,7 +2105,7 @@ public V remove(final Object k) {
TrieEntry<K, V> path = root;
while (true) {
if (current.bitIndex <= path.bitIndex) {
if (!current.isEmpty() && compareKeys(key, current.key)) {
if (!current.isEmpty() && keysAreEqual(key, current.key)) {
return removeEntry(current);
}
return null;
Expand Down
Loading