stephenafamo / bob

SQL query builder and ORM/Factory generator for Go with support for PostgreSQL, MySQL and SQLite
https://bob.stephenafamo.com
MIT License
775 stars 39 forks source link

Make query condition for single-column primary key simpler #88

Closed inoc603 closed 1 year ago

inoc603 commented 1 year ago

The current implementation for DeleteAll and UpdateAll uses tuple comparison (I'm not sure what's the right name for it) as primary key condition. So even if a table has a single column primary key, we would still generate queries like this:

models.VideoSlice{{ID: 1}, {ID: 2}, {ID: 3}}.DeleteAll(ctx, bob.Debug(db))
DELETE FROM `videos` AS `videos`
WHERE ((`id`) IN ((?), (?), (?)))

I'm working with a project that uses some order version of polardb, which does not seem to support conditions like WHERE (id) in ((?), (?)). While I'm not asking bob to support every mysql variation out there, maybe we can make the query simpler for tables with single-column primary key?

DELETE FROM `videos` AS `videos`
WHERE (`id` IN (?, ?, ?))

I think all we need is something like this in methods like DeleteMany:

    if len(pks) == 1 {
        pkValues := make([]bob.Expression, len(pkVals))
        for i, pair := range pkVals {
            pkValues[i] = pair[0]
        }

        q.Apply(dm.Where(
            Quote(pks[0]).In(pkValues...),
        ))
    } else {
        pkPairs := make([]bob.Expression, len(pkVals))
        for i, pair := range pkVals {
            pkPairs[i] = Group(pair...)
        }

        pkGroup := make([]bob.Expression, len(pks))
        for i, pk := range pks {
            pkGroup[i] = Quote(pk)
        }

        q.Apply(dm.Where(
            Group(pkGroup...).In(pkPairs...),
        ))
    }

Is there anything else we need to consider? I'm happy to create a PR for it.

stephenafamo commented 1 year ago

I'm conflicted about what to do.

On one hand, I am quite hesitant to include a change in Bob to support older versions of DB engines.
As an example, the MySQL dialect in Bob is strictly for v8.0+ since 5.7 is almost at the EOL.

On the other hand, this seems like a relatively minor change to include.

inoc603 commented 1 year ago

The feeling's mutual. Maybe we can put aside the 'supporting old DB engines' part, and think of this as generating simpler query for the majority of schemas which have single-column primary key.

Please see https://github.com/stephenafamo/bob/pull/89 for my attempts on this. I'm not sure where should I add tests for this though.

stephenafamo commented 1 year ago

In the refactoring, it seems I have found a way to solve this without too much hassle.

Take a look at the refactor-table-methods branch and try it out.

There are A LOT of breaking changes, and a few things do not fully work yet, that's why #94 is only a draft PR at the moment

stephenafamo commented 1 year ago

Implemented in #94