Skip to content

Fix nextKey skipping a key and throwing for keys not in the map - #728

Open
rootvector2 wants to merge 1 commit into
apache:masterfrom
rootvector2:sorted-map-nextkey-absent
Open

Fix nextKey skipping a key and throwing for keys not in the map#728
rootvector2 wants to merge 1 commit into
apache:masterfrom
rootvector2:sorted-map-nextkey-absent

Conversation

@rootvector2

Copy link
Copy Markdown
Contributor

AbstractSortedMapDecorator.nextKey walks tailMap(key) and discards the first element to step over key itself, but never checks that key is present. When key is absent that first element is the successor, so it gets dropped and the wrong key comes back; when key is at or past the end tailMap is empty and it.next() throws NoSuchElementException instead of returning null. DualTreeBidiMap.nextKey repeats the same tailMap().iterator().next() shape and fails the same two ways, since its isEmpty() guard only covers a fully empty map.

Return null when the map does not contain key, matching the documented null if no match contract and every other nextKey in the library (AbstractLinkedMap, ListOrderedMap, TreeBidiMap, PatriciaTrie all return null for an absent key); the skip-first logic then runs only for a present key, the case it was written for. FixedSizeSortedMap and UnmodifiableSortedMap inherit the decorator fix. Found by auditing the OrderedMap.nextKey implementations against the contract.

  • Read the contribution guidelines for this project.
  • Read the ASF Generative Tooling Guidance if you use Artificial Intelligence (AI).
  • I used AI to create any part of, or all of, this pull request. Which AI tool was used to create this pull request, and to what extent did it contribute?
  • Run a successful build using the default Maven goal with mvn; that's mvn on the command line by itself.
  • Write unit tests that match behavioral changes, where the tests fail if the changes to the runtime are not applied. This may not always be possible, but it is a best practice.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Each commit in the pull request should have a meaningful subject line and body. Note that a maintainer may squash commits during the merge process.

@garydgregory garydgregory changed the title fix nextKey skipping a key and throwing for keys not in the map Fix nextKey skipping a key and throwing for keys not in the map Aug 20, 2026

@garydgregory garydgregory left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hello @rootvector2

Thank you for the PR. How about adding tests for:

  • An explicit test for DualTreeBidiMap.nextKey() with an absent key that is between existing keys. The abstract test uses getOtherKeys() which are outside the sample range, so the bug would still be hidden if the key were in-range.
  • An explicit test for UnmodifiableSortedMap.nextKey(). It inherits the decorator fix but has no dedicated test.

Should the code be using NavigableMap.higherKey() when available? TreeMap implements NavigableMap, so higherKey(key) would give the correct successor in one call and would be null for absent keys automatically.

Should we guard the OrderedMap branch in DualTreeBidiMap.nextKey() for consistency; either ensure the delegated implementation also checks presence or apply the same containsKey guard before delegation.

Isn't there a similar problem with previousKey()? That could be addressed in a different PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants