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

fix foreach bug : non-prefixed item should not bind to context #423

Closed
wants to merge 1 commit into from
Closed

Conversation

Youmoo
Copy link

@Youmoo Youmoo commented Jun 13, 2015

Here is a demo to reproduce the bug.
Due to the bug, The two following xmls result in different results(which are supposed to be identical):

<select id="flavorOne" resultType="mybatis.TestModal">
        select id,bill_no as billNo from test
        <where>
            <trim suffixOverrides=",">
                <if test="id!=null">
                    id=#{id},
                </if>
                <if test="idList!=null">
                    and id in
                    <foreach collection="idList" item="id" open="(" close=")" separator=",">
                        #{id}
                    </foreach>
                </if>
            </trim>
        </where>
    </select>

vs

    <select id="flavorTwo" resultType="mybatis.TestModal">
        select id,bill_no as billNo from test
        <where>
            <trim suffixOverrides=",">
                <if test="idList!=null">
                    id in
                    <foreach collection="idList" item="id" open="(" close=")" separator=",">
                        #{id}
                    </foreach>
                </if>
                <if test="id!=null">
                    and id=#{id},
                </if>
            </trim>
        </where>
    </select>

@Youmoo Youmoo changed the title fix foreach bug : non-prefiexed item should not bind to context fix foreach bug : non-prefixed item should not bind to context Jun 13, 2015
@Youmoo
Copy link
Author

Youmoo commented Jun 13, 2015

The Travis CI build failed, but I find the code in org.apache.ibatis.builder.xml.dynamic.DynamicSqlSourceTest#shouldIterateOnceForEachItemInCollection is itself a wrong test case:

        final String expected = "SELECT * FROM BLOG WHERE ID in (  one = ? AND two = ? AND three = ? )";

expected is not valid sql, is it written that way intentionally? Got confused..

@harawata
Copy link
Member

Hi,

Thanks for the PR, but the fix breaks existing test cases (five errors were reported other than the failure you pointed out).

Actually, this issue was reported as #49 before.
Please see the @emacarron comment on the ticket.

p.s.
I was wondering how could MyBatis know which 'id' to use when you write #{id} in the foreach loop...
You really should use a unique item variable name, IMHO.

Regards,
Iwao

@harawata harawata closed this Jun 15, 2015
@Youmoo
Copy link
Author

Youmoo commented Jun 15, 2015

Hi. @harawata I supposed #{id} in the foreach loop to be scoped inside the for loop and should not be accessible outside the for loop.
The document may clarify that accordingly.

@harawata
Copy link
Member

PR with documentation changes are also welcome!
The source of the doc is in the following directory in xdoc format.
https://github.com/mybatis/mybatis-3/tree/master/src/site

Note that the PR must includes the English version as we use it for localization.

Regards,
Iwao

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.

None yet

2 participants