foreach unexpectedly treats List as Map

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

foreach unexpectedly treats List as Map

Josh Littlefield
When upgrading a project from iBATIS 3.0 to MyBatis 3.4.6, we had an unexpected issue. A <foreach> in a Mapper XML file was causing a runtime error.
The issue was because the Collection being iterated was a List<Pair<Integer,Intger>>, where Pair is org.apache.commons.lang3.tuple.Pair, which implements Map.Entry.

We were expecting to get the List items and then refer to #{item.left} and #{item.right}, but we instead got the Map iteration behavior, because we (sort of unknowingly -- old code) were iterating a Collection of Map.Entry objects. This took some time to figure out.
The fix was a simple change to how we use the <foreach>, but the problem is hard to find in complex mappers (this was turned up in automated testing, but that depends on really good test coverage). I wonder if a property on the foreach to force an iterator behavior would help. Even a Map would work with this by using #{item.key} and #{item.value}, and it would make the usage more obvious in the Mapper XML.

Thought I'd ask this list before opening an issue or proposing an enhancement.

Thanks.

--
You received this message because you are subscribed to the Google Groups "mybatis-user" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: foreach unexpectedly treats List as Map

Iwao AVE!
Hi Josh,

I'm sorry about your troubles.
Could you post the mapper method declaration, XML statement and stack trace?

We are trying very hard to keep the number of options minimum and, if I understand correctly, the proposed foreach property couldn't have saved your time spent on figuring out the cause of the error.
So, I think we should look for a way to improve the error message instead.

Regards,
Iwao



On Thu, Dec 13, 2018 at 12:20 AM Josh Littlefield <[hidden email]> wrote:
When upgrading a project from iBATIS 3.0 to MyBatis 3.4.6, we had an unexpected issue. A <foreach> in a Mapper XML file was causing a runtime error.
The issue was because the Collection being iterated was a List<Pair<Integer,Intger>>, where Pair is org.apache.commons.lang3.tuple.Pair, which implements Map.Entry.

We were expecting to get the List items and then refer to #{item.left} and #{item.right}, but we instead got the Map iteration behavior, because we (sort of unknowingly -- old code) were iterating a Collection of Map.Entry objects. This took some time to figure out.
The fix was a simple change to how we use the <foreach>, but the problem is hard to find in complex mappers (this was turned up in automated testing, but that depends on really good test coverage). I wonder if a property on the foreach to force an iterator behavior would help. Even a Map would work with this by using #{item.key} and #{item.value}, and it would make the usage more obvious in the Mapper XML.

Thought I'd ask this list before opening an issue or proposing an enhancement.

Thanks.

--
You received this message because you are subscribed to the Google Groups "mybatis-user" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "mybatis-user" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: foreach unexpectedly treats List as Map

Josh Littlefield
Hi. Just hit this in two more places. It's just so subtle for a developer to know that when they need to iterate a List or Set with <foreach>, if the elements in the collection being iterated happen to implement Map.Entry, then they have to iterate them differently than they normally would.

I wasn't sure the best way to demonstrate this, so I forked the repo and added a failing test. Actually it's 2 failing examples, and 2 non-failing examples, just to show the issue is simply in the <foreach> usage.


The exceptions are like:
[ERROR] shouldCountByIdListBad  Time elapsed: 0.123 s  <<< ERROR!
org.apache.ibatis.exceptions.PersistenceException:

### Error querying database.  Cause: org.apache.ibatis.reflection.ReflectionException: There is no getter for property named 'left' in 'class java.lang.Integer'
### Cause: org.apache.ibatis.reflection.ReflectionException: There is no getter for property named 'left' in 'class java.lang.Integer'
    at org.apache.ibatis.submitted.foreach_pair.ForEachPairTest.shouldCountByIdListBad(ForEachPairTest.java:70)
Caused by: org.apache.ibatis.reflection.ReflectionException: There is no getter for property named 'left' in 'class java.lang.Integer'
    at org.apache.ibatis.submitted.foreach_pair.ForEachPairTest.shouldCountByIdListBad(ForEachPairTest.java:70)


On Wed, Dec 12, 2018 at 1:20 PM Iwao AVE! <[hidden email]> wrote:
Hi Josh,

I'm sorry about your troubles.
Could you post the mapper method declaration, XML statement and stack trace?

We are trying very hard to keep the number of options minimum and, if I understand correctly, the proposed foreach property couldn't have saved your time spent on figuring out the cause of the error.
So, I think we should look for a way to improve the error message instead.

Regards,
Iwao



On Thu, Dec 13, 2018 at 12:20 AM Josh Littlefield <[hidden email]> wrote:
When upgrading a project from iBATIS 3.0 to MyBatis 3.4.6, we had an unexpected issue. A <foreach> in a Mapper XML file was causing a runtime error.
The issue was because the Collection being iterated was a List<Pair<Integer,Intger>>, where Pair is org.apache.commons.lang3.tuple.Pair, which implements Map.Entry.

We were expecting to get the List items and then refer to #{item.left} and #{item.right}, but we instead got the Map iteration behavior, because we (sort of unknowingly -- old code) were iterating a Collection of Map.Entry objects. This took some time to figure out.
The fix was a simple change to how we use the <foreach>, but the problem is hard to find in complex mappers (this was turned up in automated testing, but that depends on really good test coverage). I wonder if a property on the foreach to force an iterator behavior would help. Even a Map would work with this by using #{item.key} and #{item.value}, and it would make the usage more obvious in the Mapper XML.

Thought I'd ask this list before opening an issue or proposing an enhancement.

Thanks.

--
You received this message because you are subscribed to the Google Groups "mybatis-user" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "mybatis-user" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "mybatis-user" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: foreach unexpectedly treats List as Map

Iwao AVE!
Hi Josh,

Thanks for the test case!
It's perfect for explaining the issue.

The behavior (i.e. key->index, value->item) is how foreach handles Map.Entry and it's actually pretty handy.
I would say it is quite unusual that some class 'happens to implement' Map.Entry (I checked a few other Pair implementations and they didn't implement Map.Entry).
And it is difficult for MyBatis to explain this error better than the current message because there is no information about the original XML when the statement is executed, unfortunately.

This may not be the advice you are looking for, but this kind of problem is relatively easier to detect with a tool like IDE plugin.
With the Eclipse plugin [1], for example, content assist would show you that 'item' is Integer, not Pair.
(validation error would be more ideal, but it's not implemented yet :P)

image.png

There also are several plugins for IDEA (I've never tried myself).
(...there could be more)

If you develop MyBatis applications constantly, you might want to give one of them a try.


Regards,
Iwao

On Fri, Jan 25, 2019 at 8:48 AM Josh Littlefield <[hidden email]> wrote:
Hi. Just hit this in two more places. It's just so subtle for a developer to know that when they need to iterate a List or Set with <foreach>, if the elements in the collection being iterated happen to implement Map.Entry, then they have to iterate them differently than they normally would.

I wasn't sure the best way to demonstrate this, so I forked the repo and added a failing test. Actually it's 2 failing examples, and 2 non-failing examples, just to show the issue is simply in the <foreach> usage.


The exceptions are like:
[ERROR] shouldCountByIdListBad  Time elapsed: 0.123 s  <<< ERROR!
org.apache.ibatis.exceptions.PersistenceException:

### Error querying database.  Cause: org.apache.ibatis.reflection.ReflectionException: There is no getter for property named 'left' in 'class java.lang.Integer'
### Cause: org.apache.ibatis.reflection.ReflectionException: There is no getter for property named 'left' in 'class java.lang.Integer'
    at org.apache.ibatis.submitted.foreach_pair.ForEachPairTest.shouldCountByIdListBad(ForEachPairTest.java:70)
Caused by: org.apache.ibatis.reflection.ReflectionException: There is no getter for property named 'left' in 'class java.lang.Integer'
    at org.apache.ibatis.submitted.foreach_pair.ForEachPairTest.shouldCountByIdListBad(ForEachPairTest.java:70)


On Wed, Dec 12, 2018 at 1:20 PM Iwao AVE! <[hidden email]> wrote:
Hi Josh,

I'm sorry about your troubles.
Could you post the mapper method declaration, XML statement and stack trace?

We are trying very hard to keep the number of options minimum and, if I understand correctly, the proposed foreach property couldn't have saved your time spent on figuring out the cause of the error.
So, I think we should look for a way to improve the error message instead.

Regards,
Iwao



On Thu, Dec 13, 2018 at 12:20 AM Josh Littlefield <[hidden email]> wrote:
When upgrading a project from iBATIS 3.0 to MyBatis 3.4.6, we had an unexpected issue. A <foreach> in a Mapper XML file was causing a runtime error.
The issue was because the Collection being iterated was a List<Pair<Integer,Intger>>, where Pair is org.apache.commons.lang3.tuple.Pair, which implements Map.Entry.

We were expecting to get the List items and then refer to #{item.left} and #{item.right}, but we instead got the Map iteration behavior, because we (sort of unknowingly -- old code) were iterating a Collection of Map.Entry objects. This took some time to figure out.
The fix was a simple change to how we use the <foreach>, but the problem is hard to find in complex mappers (this was turned up in automated testing, but that depends on really good test coverage). I wonder if a property on the foreach to force an iterator behavior would help. Even a Map would work with this by using #{item.key} and #{item.value}, and it would make the usage more obvious in the Mapper XML.

Thought I'd ask this list before opening an issue or proposing an enhancement.

Thanks.

--
You received this message because you are subscribed to the Google Groups "mybatis-user" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "mybatis-user" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "mybatis-user" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "mybatis-user" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
For more options, visit https://groups.google.com/d/optout.