mybatis/mybatis-dynamic-sql

when i use isIn(emptyList) where condition, it would not render to sql

clcy1243 opened this issue · 15 comments

clcy1243 commented

code example

select(s -> s.where(id, isIn(new ArrayList<>()));

will render like this

select * from table

it should be like

select * from table where id in ()

and mysql where throw exception when execute this sql, it's what my want,
so i think there is a bug,
i found this code at org.mybatis.dynamic.sql.where.render.WhereConditionVisitor<T>

    public Optional<FragmentAndParameters> visit(AbstractListValueCondition<T> condition) {
        FragmentCollector fc = (FragmentCollector)condition.mapValues(this::toFragmentAndParameters).collect(FragmentCollector.collect());
        return fc.isEmpty() ? Optional.empty() : FragmentAndParameters.withFragment(condition.renderCondition(this.columnName(), fc.fragments())).withParameters(fc.parameters()).buildOptional();
    }

if list value condition accept an empty list, fc.isEmpty() will be true, this condition where be deleted

could anyone fix this?

jeffgbutler commented

I don't agree this is a bug - I think it is actually a good thing that the library won't generate invalid SQL. I think it would be best to check to see if the list is empty before trying to execute the statement. If it is a possibility in your app that the list could be empty, then do you really want to rely on causing a database exception as a kind of validation?

Honestly it wouldn't be too hard to provide an option to render these conditions even when they are empty. But I'm not convinced that is a good idea.

Thoughts???

clcy1243 commented

I don't agree this is a bug - I think it is actually a good thing that the library won't generate invalid SQL. I think it would be best to check to see if the list is empty before trying to execute the statement. If it is a possibility in your app that the list could be empty, then do you really want to rely on causing a database exception as a kind of validation?

Honestly it wouldn't be too hard to provide an option to render these conditions even when they are empty. But I'm not convinced that is a good idea.

Thoughts???

if i want ignore invalid condition, i will use IsInWhenPresent but not IsIn, i think there is IsInWhenPresent method, so IsIn mean it will render this condition even if it is invalid

clcy1243 commented

there is two method do same thing with diff name, may be there should has one method named IsInRequired?

jeffgbutler commented

IsInWhenPresent filters null values out of the list. It will still not render if the list ends up being empty.

clcy1243 commented

with 1.3.7 generator, i use Example, when i use andIdIn(emptyList), will throw exception,
so i should change my code to use mybatis-dynamic-sql,
when i use isIn , i should valid my param

clcy1243 commented

there is a problem, when i build sql with not valid param, it where change more data than what i want it modify,
and i should check param each step now

jeffgbutler commented

I get it. I really do think you should be validating, but I understand it could be a change to how you're working now and not doing it would cause undesirable side effects.

I've been experimenting a bit. I can enable you to write a condition that will force the exception with a very simple code change. I'll push something to address this shortly.

clcy1243 commented

Thanks for understanding

clcy1243 commented

Here is 4:00am, I will try it when I waked up.

clcy1243 commented

i code like this


    public static <T> IsIn<T> isInRequired(Collection<T> values) {
        if (CollectionUtils.isEmpty(values)) {
            throw new ZhqcException(WmsResponseEnum.CUSTOMIZE_MSG, "param could not be empty at where in condition");
        }
        return IsIn.of(values);
    }

to fix my codes now.

ur idea is so good, I don't try to expland where codition before,
I think i can use mybatis-dynamic-sql do more amazing things than before.

jeffgbutler commented

@clcy1243 I like your solution a lot! This is much better than forcing the library to generate invalid SQL. I'm going to remove the change I made and use your example in the docs.

zhenchuan9r commented

I would like to view this as a bug.
It is very contradicting normal understanding of the declaration of the where clause.
It is at least a very unfriendly design.
But thanks for changing the design to a normal track.

jeffgbutler commented

@zhenchuan9r If you have found a bug, please open a new issue. It is ineffective to comment on an issue that has been closed for three years.

zhenchuan9r commented

@zhenchuan9r If you have found a bug, please open a new issue. It is ineffective to comment on an issue that has been closed for three years.

I encountered the same problem as this issue.
As this issue has already been resolved, it is not necessary for me to open a new issue.