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

Prevent foreach xml tag from polluting the global context #966

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@nyer

nyer commented Apr 5, 2017

Mybatis currently shares one global bindings context during dynamic sql building and parametemapping construction. It is ok at most time, but when it comes to the foreach xml tag, something wrong will occur. Like the following mybatis mapper:

<select id="selectList" parameterType="java.util.Map">
select * from tb
<where>
  <if test="status != null>
    and status = #{status}
  </if>

  <if test="statusList != null">
    and status in
    <foreach collecion="statusList" item="status" open="( " separator=" , " close=" )">
      #{status}
    </foreach>
  </if>
</where>
</select>

When variable status is passed as 5, and statusList is passed as [4,5,6],it is normal when final sql will equals to select * from tb where status = 5. However, the final sql is select * from tb where status =6. The reason is that the variable status in foreach tag overwrites the outside variable with the same name. It behaves like the dynamic scope feature in programming language. It is error-prone.
Please review. thank you

@harawata

This comment has been minimized.

Show comment
Hide comment
@harawata

harawata Apr 7, 2017

Member

@nyer ,
I have not had the time to review, but please add test case(s).
Thank you!

Member

harawata commented Apr 7, 2017

@nyer ,
I have not had the time to review, but please add test case(s).
Thank you!

@nyer

This comment has been minimized.

Show comment
Hide comment
@nyer

nyer Apr 7, 2017

@harawata test case is added.
Thank you!

nyer commented Apr 7, 2017

@harawata test case is added.
Thank you!

@harawata

This comment has been minimized.

Show comment
Hide comment
@harawata

harawata Apr 11, 2017

Member

Thanks for the update, @nyer !
I will look into it.

Member

harawata commented Apr 11, 2017

Thanks for the update, @nyer !
I will look into it.

@Youmoo

This comment has been minimized.

Show comment
Hide comment
@Youmoo

Youmoo Apr 18, 2017

Similar to #423 ?

Youmoo commented Apr 18, 2017

Similar to #423 ?

@harawata

This comment has been minimized.

Show comment
Hide comment
@harawata

harawata Apr 19, 2017

Member

@Youmoo Yes, it is the same issue, but this patch does not break the existing test cases.

I may need some time to review this.

Member

harawata commented Apr 19, 2017

@Youmoo Yes, it is the same issue, but this patch does not break the existing test cases.

I may need some time to review this.

@nyer

This comment has been minimized.

Show comment
Hide comment
@nyer

nyer Apr 20, 2017

@harawata , I have updated the patch in case of null value.

nyer commented Apr 20, 2017

@harawata , I have updated the patch in case of null value.

@harawata harawata closed this in bae02af May 24, 2017

@harawata

This comment has been minimized.

Show comment
Hide comment
@harawata

harawata May 24, 2017

Member

@nyer ,
Sorry for a late response!
After taking a deeper look at this issue, I think I found a simpler solution (see bae02af).
Could you try the latest snapshot and see if the fix is OK?

Note that it still is a bad idea to use a conflicting variable name for item/index attribute.

Member

harawata commented May 24, 2017

@nyer ,
Sorry for a late response!
After taking a deeper look at this issue, I think I found a simpler solution (see bae02af).
Could you try the latest snapshot and see if the fix is OK?

Note that it still is a bad idea to use a conflicting variable name for item/index attribute.

@harawata harawata added this to the 3.4.5 milestone May 24, 2017

@harawata harawata self-assigned this May 24, 2017

@harawata harawata added bug enhancement and removed bug labels May 24, 2017

@nyer

This comment has been minimized.

Show comment
Hide comment
@nyer

nyer May 25, 2017

@harawata ,
It works, and it is awesome.
Thank your for your work.

nyer commented May 25, 2017

@harawata ,
It works, and it is awesome.
Thank your for your work.

@harawata

This comment has been minimized.

Show comment
Hide comment
@harawata

harawata May 25, 2017

Member

You are very welcome. Thank you both for your contribution!

Member

harawata commented May 25, 2017

You are very welcome. Thank you both for your contribution!

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