mvysny / vok-orm

Mapping rows from a SQL database to POJOs in its simplest form
MIT License
23 stars 4 forks source link

No SQL Converter for 'in' and 'not' #15

Closed anm-cb closed 2 years ago

anm-cb commented 2 years ago

A while ago the conversions for the InFilter and the NotFilter got removed, so even though they are available in vok-dataloader, they cannot be used anymore.

(KProperty1-InFilter-Extension does not work either currently, but that's another repo, could not figure out how to write this extension any better).

mvysny commented 2 years ago

Thank you for letting me know. Perhaps the best way is to bring those conversions back. However, can you please share an example code which demoes the issue? You can create an example code in https://gitlab.com/mvysny/vok-orm-playground .

anm-cb commented 2 years ago
        Person.findAllBy {
            Person::maritalStatus `in` setOf(MaritalStatus.Divorced, MaritalStatus.Widowed)
        }

        Person.findAllBy {
            !(Person::maritalStatus `in` setOf(MaritalStatus.Divorced, MaritalStatus.Widowed))
        }

As mentioned the in Filter does not currently work, I made a merge-request in vok-dataloader.

I would have made a merge request here as well, but I'm not sure how deep I'd have to go, so far I'd have simply added something like

            is NotFilter -> {
                val c: ParametrizedSql = filter.child.toParametrizedSql(clazz, variant)
                ParametrizedSql("not (${c.sql92})", c.sql92Parameters)
            }
            is InFilter -> {
                val sql92 : String = filter.value.joinToString(", ", prefix = "$databaseColumnName in (", postfix = ")") { "'$it'" }
                ParametrizedSql(sql92, mapOf())
            }

the the DefaultFilterToSqlConverter.

In the dataloader-isses it seemed you wanted to remove the not filter?

(Thanks for all your vaadin-on-kotlin and *-orm work btw. :))

mvysny commented 2 years ago

(Thanks for all your vaadin-on-kotlin and *-orm work btw. :)) No problem, thank you so much for using it :)

I think I remember why I removed the in and not filters. This fancy Kotlin syntax was only meant to support basic use-cases, to prevent users from "abusing" it, making the expressions really complex and shooting themselves in the foot. I originally believed that if you are in need of in and not operators then you might be better off writing the SQL in native form (as a SQL command). Especially this holds for not since it effectively forces you to use parenthesis.

However, your examples look simple and I kinda like the in operator now (unsure about not but let's see how well it will work :-D ). I give them a green-light, let's include them into vok-dataloader and vok-orm :muscle:

I've noticed your PRs for vok-dataloader - big thanks! Let me take a look at those during the weekend :muscle:

mvysny commented 2 years ago

@anm-cb I've merged stuff into vok-dataloader; please let me know whether you have more PRs planned - if not, I can release a new version of vok-dataloader. The not operator is missing I think - would you be interested in making a PR for this please?

anm-cb commented 2 years ago

Thanks, I've got no more PRs planned for vok-dataloader, but I'm trying to make a PR for vok-orm tomorrow.

mvysny commented 2 years ago

Awesome, thank you so much for your contributions :bow: I've released the newest vok-dataloader and upgraded vok-orm to use it.

anm-cb commented 2 years ago

Do you have an idea howto make the Infilter-Converter in DefaultFilterToSqlConverter work properly? Initially I'd have implemented it like this:

            is InFilter -> {
                val sql92 : String = filter.value.joinToString(", ", prefix = "$databaseColumnName in (", postfix = ")") { "'$it'" }
                ParametrizedSql(sql92, mapOf())
            }

(as above). It would probably be better to add the entries of the list as parameters to the ParametrizedSql? But the method is not really suitable for that. I starting to regret the InFilter a little again. :see_no_evil: I'd probably use a method in my code to generate an OR-linked statement now. Another problem is probably that an empty IN () is illegal in SQL.

mvysny commented 2 years ago

Hmmm converting the parameters to string via '$it' won't fly with numbers. I'd suggest to pass the entire set as a parameter, and let's see what will happen :-D Something like "$databaseColumnName in %1", then pass the entire set as a parameter.

The easiest way to validate is to run the tests on your machine - vok-orm will start databases as docker images. Alternatively you can leave that to github :+1: - simply create a PR and github should run all tests on all databases.

If an empty set in an IN clause would be illegal, we can for example ban it in the "InFilter" class. Or emit false or 1==0 since no records would match anyway.

anm-cb commented 2 years ago

There's a check in vok-dataloader that makes sure that collections cannot be empty so I skipped that handling here.

mvysny commented 2 years ago

Awesome work, thank you, I think this ticket was closed by 45be17cb45df73a93e577454db9610a0dd7be6b7 and PR #16