Skip to content
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.

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

MultiValueMapAdapter.getFirst fails with IndexOutOfBoundsException in case of empty List #25140

Closed
vlavasa opened this issue May 27, 2020 · 12 comments
Assignees
Labels
in: core Issues in core modules (aop, beans, core, context, expression) status: backported An issue that has been backported to maintenance branches type: bug A general bug
Milestone

Comments

@vlavasa
Copy link

vlavasa commented May 27, 2020

If You create MultiValueMap via MultiValueMapAdapter, and value list is empty, call of method getFirst fail on IndexOutOfBounds exception:

private static class MultiValueMapAdapter<K, V> implements MultiValueMap<K, V>, Serializable {

		private final Map<K, List<V>> map;

		public MultiValueMapAdapter(Map<K, List<V>> map) {
			Assert.notNull(map, "'map' must not be null");
			this.map = map;
		}

		@Override
		@Nullable
		public V getFirst(K key) {
			List<V> values = this.map.get(key);
			return (values != null ? values.get(0) : null);
		}

Good solution is:

return (values != null && !values.isEmpty() ? values.get(0) : null);
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label May 27, 2020
@jhoeller jhoeller changed the title Bug in org.springframework.util.CollectionUtils.MultiValueMapAdapter MultiValueMapAdapter.getFirst fails with IndexOutOfBoundsException in case of empty List May 27, 2020
@jhoeller jhoeller self-assigned this May 27, 2020
@jhoeller jhoeller added in: core Issues in core modules (aop, beans, core, context, expression) type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels May 27, 2020
@jhoeller jhoeller added this to the 5.2.7 milestone May 27, 2020
@jhoeller
Copy link
Contributor

The common scenario is that such a list either contains one or more values or isn't set at all (in case of no value associated with a key). That said, point taken, we should align this with the existing LinkedMultiValueMap implementation where such a defensive emptiness check is present already.

@jhoeller jhoeller added the for: backport-to-5.1.x Marks an issue as a candidate for backport to 5.1.x label May 27, 2020
@spring-projects-issues spring-projects-issues added status: backported An issue that has been backported to maintenance branches and removed for: backport-to-5.1.x Marks an issue as a candidate for backport to 5.1.x labels May 27, 2020
@vlavasa
Copy link
Author

vlavasa commented May 27, 2020

The common scenario is that such a list either contains one or more values or isn't set at all (in case of no value associated with a key). That said, point taken, we should align this with the existing LinkedMultiValueMap implementation where such a defensive emptiness check is present already.

OK, but the adapter is used in method:
org.springframework.util.CollectionUtils#toMultiValueMap

and there is accepted any Map<K, List> map

@jhoeller
Copy link
Contributor

Good point, it can be constructed with any target map content, including empty list values there. Also, an addAll call with an empty list argument will also lead to such empty list state, just like in LinkedMultiValueMap. I was just pointing out why it is uncommon to encounter empty lists there, otherwise it would be surprising for this bug to remain unnoticed so many years.

Thanks for raising this, in any case! I've marked it for a fix in the upcoming 5.2.7 and 5.1.16 releases.

@midumitrescu
Copy link
Contributor

midumitrescu commented May 28, 2020

@jhoeller could I give it a go?

I have also prepared unit tests.

midumitrescu added a commit to midumitrescu/spring-framework that referenced this issue May 28, 2020
@jhoeller
Copy link
Contributor

@midumitrescu I got a local commit for 5.2.x staged along with a few others, using the same code that we have in LinkedMultiValueMap (with an && !values.isEmpty() check); I'll forward-merge this to master ASAP. So thanks for volunteering but I'm afraid it's effectively done already :-)

If you got unit tests for MultiValueMapAdapter, I'd appreciate if you could subsequently submit them as a separate PR. We got quite a few tests for LinkedMultiValueMap but no isolated ones for CollectionUtils.toMultiValueMap, as far as I can see, so there's definitely a gap to close there.

midumitrescu added a commit to midumitrescu/spring-framework that referenced this issue May 28, 2020
midumitrescu added a commit to midumitrescu/spring-framework that referenced this issue May 28, 2020
@jhoeller
Copy link
Contributor

jhoeller commented May 28, 2020

@midumitrescu I see that your commit uses CollectionUtils.firstElement, another interesting solution. I nevertheless prefer alignment with the LinkedMultiValueMap implementation without any further method delegation involved, as in my locally staged commit.

To be clear about the unit tests, we don't have any at the moment. We'd benefit from complete unit tests for CollectionUtils.toMultiValueMap, to the extent that LinkedMultiValueMapTests covers the typical interaction patterns already. A contribution would be very welcome there!

@midumitrescu
Copy link
Contributor

midumitrescu commented May 28, 2020

@jhoeller Sorry, I did not see your message in time. I'll close my PR.

To not forget,

toSingleValueMap

also calls

list.get(0)

Producing basically the same ArrayIndexOutOfBoundsException.

By not wanting to duplicate the behavior I came to

Collections.firstElement()

Just let me know when to add the unit tests.
The thing is, I created only those necessary for fixing this bug.

However, I can gladly also complete them with some meaningful ones.

@jhoeller
Copy link
Contributor

That's a good catch indeed, my 5.2.x commit has this aligned with the LinkedMultiValueMap implementation as well where we are performing the same !values.isEmpty() check in toSingleValueMap. It's a matter of implementation style and therefore always debatable; we tend to keep our low-level utilities as decoupled from our own convenience methods as possible. From that perspective, LinkedMultiValueMap should not depend on CollectionUtils (which is meant to be working on top of collection implementations), and the same goes for the internal MultiValueMapAdapter... even if the latter is nested within CollectionUtils itself.

As for the unit tests, LinkedMultiValueMapTests is a good starting point. MultiValueMapAdapter should receive the same coverage there. So if you got some cycles left for volunteering, a PR for that part would be much appreciated :-) Otherwise we can also cover the unit tests at a later point.

@midumitrescu
Copy link
Contributor

midumitrescu commented May 28, 2020

@jhoeller I think I needed a bit of time to understand what you mean by aligning implementations.

Are you meaning

	public static <K, V> MultiValueMap<K, V> toMultiValueMap(Map<K, List<V>> map) {
		return new LinkedMultiValueMap<>(map);
	}

Or is the MultiValueMapAdapter really needed?

midumitrescu added a commit to midumitrescu/spring-framework that referenced this issue May 28, 2020
@jhoeller
Copy link
Contributor

At this point it's just about coding the same methods the same way in both implementations. You got a point though, except for LinkedMultiValueMap always using an independent internal LinkedHashMap (and not a given external Map as-is), the implementations are almost the same. I'll see what I can do to unify them, maybe extracting a common base class... although we'd only do that in our recent branches, not in the backports.

@midumitrescu
Copy link
Contributor

midumitrescu commented May 28, 2020

Understood. I would be interested in giving a hand there, if needed.

If not, I will attend the unit tests now.

midumitrescu added a commit to midumitrescu/spring-framework that referenced this issue May 28, 2020
midumitrescu added a commit to midumitrescu/spring-framework that referenced this issue May 28, 2020
midumitrescu added a commit to midumitrescu/spring-framework that referenced this issue May 28, 2020
midumitrescu added a commit to midumitrescu/spring-framework that referenced this issue May 28, 2020
@jhoeller
Copy link
Contributor

jhoeller commented May 29, 2020

I ended up moving the existing MultiValueMapAdapter class to top level (but package-visible), extending it in LinkedMultiValueMap and therefore reusing most of the implementation (and putting it into a single place for maintenance purposes).

jhoeller added a commit that referenced this issue May 29, 2020
This commit extracts MultiValueMapAdapter from CollectionUtils and reuses its implementation as base class of LinkedMultiValueMap.

Closes gh-25140
jhoeller added a commit that referenced this issue Jun 10, 2020
This commit extracts MultiValueMapAdapter from CollectionUtils and reuses its implementation as base class of LinkedMultiValueMap.

Closes gh-25140
FelixFly pushed a commit to FelixFly/spring-framework that referenced this issue Aug 16, 2020
This commit extracts MultiValueMapAdapter from CollectionUtils and reuses its implementation as base class of LinkedMultiValueMap.

Closes spring-projectsgh-25140
zx20110729 pushed a commit to zx20110729/spring-framework that referenced this issue Feb 18, 2022
This commit extracts MultiValueMapAdapter from CollectionUtils and reuses its implementation as base class of LinkedMultiValueMap.

Closes spring-projectsgh-25140
snicoll pushed a commit that referenced this issue Aug 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: core Issues in core modules (aop, beans, core, context, expression) status: backported An issue that has been backported to maintenance branches type: bug A general bug
Projects
None yet
Development

No branches or pull requests

4 participants