MyBatis version

3.5.1

Database vendor and version

MySQL 5.7.19

Test case or example project

SELECT a.id, a.name, a.oid
    FROM A a
        INNER JOIN B b on a.id = b.aid
        INNER JOIN C c on b.did = c.did
    WHERE
        c.uid = #{uid}
        AND a.flag = 1
    <if test="dids != null">
        AND c.did IN
        <foreach collection="dids" item="did" separator="," open="(" close=")">
            #{did}
        </foreach>
    </if>

    # complex search
    <if test="keyword != null">
        AND (a.name LIKE CONCAT('%', #{keyword},'%')
        <if test="searchingIds != null">
            OR a.id IN
            <foreach collection="searchingIds" item="searchingId" separator="," open="(" close=")">
                #{searchingId}
            </foreach>
        </if>
        )
    </if>
    <if test="keyword == null and searchingIds != null">
        AND a.id IN
        <foreach collection="searchingIds" item="searchingId" separator="," open="(" close=")">
            #{searchingId}
        </foreach>
    </if>

    # whitelist
    <if test="allowedIds != null">
        AND a.id IN
        <foreach collection="allowedIds" item="allowedId" separator="," open="(" close=")">
            #{allowedId}
        </foreach>
    </if>

    # blacklist
    <if test="deniedIds != null">
        AND a.id NOT IN
        <foreach collection="deniedIds" item="deniedId" separator="," open="(" close=")">
            #{deniedId}
        </foreach>
    </if>

Well someone would say, "Hey check if it's empty before call the SQL". In fact empty condition is normal case. I don't think it's always a good practice to check it in business code. I have to write it out of the <foreach> tag instead of using open="(" close=")"

Steps to reproduce

When any of the collections above is empty, there will be sql error. Generated sql has no parentheses after IN.

Expected result

...
AND a.id IN ()
AND a.id NOT IN ()

Actual result

...
AND a.id IN
AND a.id NOT IN

Comment From: ghost

Hi, @CodeInDreams :

Why don't you...

<if test="dids != null">
    AND c.did IN (
        <foreach collection="dids" item="did" separator=",">
            #{did}
        </foreach>
    )
</if>

...?

Comment From: CodeInDreams

Hi, @CodeInDreams :

Why don't you...

<if test="dids != null"> AND c.did IN ( <foreach collection="dids" item="did" separator=","> #{did} </foreach> ) </if>

...?

Yes right now I'm using it exactly this way (also I had mentioned this workaround above). Thank you!
But <foreach> has issue anyway. Accept it and fix it pleaaase.

Comment From: ghost

Hi, @CodeInDreams :

I'm not a MyBatis developer, so I cannot accept nor "fix" this. Just came here to check something else and answered your issue to alleviate the MyBatis team.

Anyway, what you are proposing is not a bugfix, but a breaking behaviour change (any application currently using foreach tag may break).

Consider these currently legit pieces of code:

<foreach collection="conditions" index="column" item="value" open="WHERE " separator=" AND ">
    ${column} = #{value}
</foreach>

<foreach collection="sort" item="s" open="ORDER BY " separator=",">
    ${s}
</foreach>

If the proposed behaviour were accepted, all applications upgrading to a newer MyBatis version with the new behaviour should review (and potentially change) all their foreach constructions. And the proposed behaviour, when undesired like in those two examples, requires a noisy <trim...><foreach...> construction that leads to a less readable code than the current behaviour / workaround.

Comment From: CodeInDreams

Hi, @CodeInDreams :

I'm not a MyBatis developer, so I cannot accept nor "fix" this. Just came here to check something else and answered your issue to alleviate the MyBatis team.

Anyway, what you are proposing is not a bugfix, but a breaking behaviour change (any application currently using foreach tag may break).

Consider these currently legit pieces of code:

<foreach collection="conditions" index="column" item="value" open="WHERE " separator=" AND "> ${column} = #{value} </foreach>

<foreach collection="sort" item="s" open="ORDER BY " separator=","> ${s} </foreach>

If the proposed behaviour were accepted, all applications upgrading to a newer MyBatis version with the new behaviour should review (and potentially change) all their foreach constructions. And the proposed behaviour, when undesired like in those two examples, requires a noisy <trim...><foreach...> construction that leads to a less readable code than the current behaviour / workaround.

Thanks for help again. Parden me I did't memtion you are 'developer of mybatis'. No one is reponsible to 'fix' it.

I don't think <foreach> is designed for something like open="ORDER BY ". It's a little tricky and hard to understand for others. If everyone have to use 

(
        <foreach collection="dids" item="did" separator=",">
            #{did}
        </foreach>
)

or java code check to avoid a sql error. Then why not make it easier and clear to use. We can even use plan jdbc but here we are. This is only a small difference but many a little makes a mickle. Right?

I accept your example pattern but I'll use this instead:

<if test="sort != null and sort.size() > 0">
    <foreach collection="sort" item="s" separator=",">
        ${s}
    </foreach>
</if>

Another is the same.

Comment From: ghost

Thanks for help again. Parden me I did't memtion you are 'developer of mybatis'. No one is reponsible to 'fix' it.

No problem :) I just wanted to clarify that, despite I'm answering the issue, I'm not a MyBatis developer and my opinion shouldn't be considered more than a particular opinion.

I don't think <foreach> is designed for something like open="ORDER BY ". It's a little tricky and hard to understand for others.

How is that different than open="(" close=")" ?

If everyone have to use <snippet omitted>

Not everyone "has to"... I provided you two counter-examples.

Then why not make it easier and clear to use. (...) This is only a small difference but many a little makes a mickle. Right?

There are two scenarios: 1. then open/close is required even if no items (your IN ()), and 2. the open/close should not be present if there are no items (my WHERE or ORDER BY).

I think the current behaviour favours 2, and there is an easy and clear workaround to achieve 1 (it is just a matter of writing the open/close clauses outside the foreach tag).

On the other hand, the proposed behaviour favours 1, but requires a more complex construction to achieve 2 (involving if or trim clauses). And, again, it is a breaking change that would force applications to stay in the latest version without the change, or review all their mappers for foreach usages.

Comment From: harawata

Hello @CodeInDreams ,

As @nsanchob pointed out, current behavior may be more flexible. Besides, both syntax (with and without parentheses) are invalid, aren't they?

If you want to omit the condition when the collection is empty, you should just write the AND xxx IN part in open.

<foreach collection="dids" item="did" separator=","
    open="AND c.did IN (" close=")">
  #{did}
</foreach>

This example also shows the current behavior's superiority.

Comment From: CodeInDreams

@harawata I get the point. But I still think write the main sql statement as part of the xml attribute is bad practice. If you meant that's designed on purpose (should be used this way), then I'll close the issue and use my ugly way. btw, make and xxx in inside the open is not equal to write parentheses outside the tag.