search

kysely-org / kysely

A type-safe typescript SQL query builder
https://kysely.dev
MIT License
9.9k stars 249 forks source link

feat: add safe empty where in plugin #925

Open austinwoon opened 3 months ago

austinwoon commented 3 months ago

Addresses concerns #709 with a plugin.

vercel[bot] commented 3 months ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
kysely ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 18, 2024 9:46am
austinwoon commented 2 months ago

@igalklebanov ive addressed all of the PR comments, could i check if this is ready to be merged?

igalklebanov commented 1 month ago

@austinwoon CI seems to run forever, can you check this?

austinwoon commented 1 month ago

@austinwoon CI seems to run forever, can you check this?

This problem seems to not be from my PR. I checked out master and also have a pending test that never stops running. The pipeline looks to be similar as well from the latest run.

could you check on this?

igalklebanov commented 1 month ago

@austinwoon CI seems to run forever, can you check this?

This problem seems to not be from my PR. I checked out master and also have a pending test that never stops running. The pipeline looks to be similar as well from the latest run.

could you check on this?

Well, it is happening only in this PR. See for yourself - https://github.com/kysely-org/kysely/actions?query=workflow%3Atests. This means, the test suite introduced in this PR is causing it. Probably a connection is left open somewhere.

igalklebanov commented 1 month ago

I think that in general, this plugin (which is not a core functionality) is unnecessarily overly tested - 65 lines of source code vs. 828 lines of test code. Take CamelCasePlugin for example, a more complex and highly used non-core functionality, it has 329 lines of source code and only 353 lines of test code that cover all the major spots.

Why does it matter? well, that's more stuff to maintain (and who gets to maintain it over time?), and more time spent waiting on tests.

I'd cut down on setup and teardown code. If all you truly needed was an extra column, add it to the general test setup code - in this case, the existing test tables don't require any additional columns for this suite (there's a name column, and a nullable column for you to use). And as my previous comments suggested, I'd cut down on redundant/irrelevant test cases, and not execute against a database engine in every case.

If the (null) approach requires special test cases to be added, and 1=0 doesn't, and both achieve the same results, just use 1=0. Keep It Simple. Also, if one approach might behave differently between database engines, opt for the approach that's universal. We're developing plugins for 4 core database engines, and for any SQL database engine out there.

By passing a ValueNode.createImmediate(null) instead of null in the transformation, you'll cut down on dialect-specific SQL string assertions, since nulls are now hard-coded in the query and not passed as parameters - parameters add dialect-specific placeholders.

austinwoon commented 1 month ago

I think that in general, this plugin (which is not a core functionality) is unnecessarily overly tested - 65 lines of source code vs. 828 lines of test code. Take CamelCasePlugin for example, a more complex and highly used non-core functionality, it has 329 lines of source code and only 353 lines of test code that cover all the major spots.

Why does it matter? well, that's more stuff to maintain (and who gets to maintain it over time?), and more time spent waiting on tests.

I'd cut down on setup and teardown code. If all you truly needed was an extra column, add it to the general test setup code - in this case, the existing test tables don't require any additional columns for this suite (there's a name column, and a nullable column for you to use). And as my previous comments suggested, I'd cut down on redundant/irrelevant test cases, and not execute against a database engine in every case.

If the (null) approach requires special test cases to be added, and 1=0 doesn't, and both achieve the same results, just use 1=0. Keep It Simple. Also, if one approach might behave differently between database engines, opt for the approach that's universal. We're developing plugins for 4 core database engines, and for any SQL database engine out there.

By passing a ValueNode.createImmediate(null) instead of null in the transformation, you'll cut down on dialect-specific SQL string assertions, since nulls are now hard-coded in the query and not passed as parameters - parameters add dialect-specific placeholders.

Thanks for the feedback!

Even if we were to go with the 1=0 approach (which i did tinker with in my local branch), i wasnt thinking of reducing test suite.

The initial thought was to make this plugin have as much tests as possible since to cover all unexpected "side-effects" that could happen

I will look into rewriting the suite to cut down as much tests when I have spare time outside of work.

Also, point taken on the "core" functionality feedback. Alternatively, i can publish this under my own third party package. I understand the huge time and effort that goes into maintaining an OSS library. The last thing I want to do is add more work on reviewing/maintenance just to support non-core work. Please feel free to close this PR, I will create a separate library if you choose to do so.