Skip to content

[COLLECTIONS-732] Document that MultiValuedMap.mapIterator() does not support setValue() - #748

Open
sharanggupta wants to merge 1 commit into
apache:masterfrom
sharanggupta:COLLECTIONS-732-mapiterator-setvalue-javadoc
Open

sharanggupta wants to merge 1 commit into
apache:masterfrom
sharanggupta:COLLECTIONS-732-mapiterator-setvalue-javadoc

Conversation

@sharanggupta

Copy link
Copy Markdown

Fixes COLLECTIONS-732.

What

MultiValuedMap.mapIterator() returns an iterator whose setValue(Object) always throws UnsupportedOperationException (the MultiValuedMapEntry instances it iterates are read-only), but neither the interface Javadoc nor the implementation said so, which is what the ticket asked for; @kinow invited a Javadoc PR there in 2020.

How

Javadoc only, no code change:

  • MultiValuedMap#mapIterator(): says that the iterator visits one key-value mapping at a time, that the implementations in this library do not support MapIterator#setValue(Object) and throw UnsupportedOperationException from it, and how to replace a value instead (remove()/removeMapping + put).
  • AbstractMultiValuedMap#mapIterator(): {@inheritDoc} plus the same note for this implementation.
  • AbstractMultiValuedMap.MultiValuedMapIterator#setValue(Object): documents the IllegalStateException before next() and the UnsupportedOperationException otherwise.

The behaviour itself is already pinned by AbstractMultiValuedMapTest.testMapIteratorUnsupportedSet, so no test was added. Verified locally with the default mvn goal on Java 17 (full test suite: 22,257 tests, plus RAT, Javadoc, japicmp, Checkstyle and PMD).

Before you push a pull request, review this list:

  • 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? Claude (Anthropic) helped draft the Javadoc wording; I reviewed it against the code and ran the build.
  • 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.

… support setValue()

The iterator returned by mapIterator() iterates read-only
MultiValuedMapEntry instances, so MapIterator.setValue(Object) always
throws UnsupportedOperationException, but neither the interface nor the
implementation said so.

Say it in the Javadoc of MultiValuedMap.mapIterator(), of the
AbstractMultiValuedMap override and of the iterator's setValue(), and
point to remove() plus put() as the way to replace a value. No code
change; the behaviour is already pinned by
AbstractMultiValuedMapTest.testMapIteratorUnsupportedSet().

@kinow kinow 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.

I've allowed GH Actions to run. I haven't looked at the API lately, so I can do a review but it will take a few days until I have time to sync the code and read it. @garydgregory feel free to review it before if you'd like 👍

Thanks for the PR @sharanggupta !

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