stephenafamo / bob

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

Invalid SQL for In operator with empty slice values. #194

Closed atzedus closed 3 months ago

atzedus commented 3 months ago
vals1 := []bob.Expression{}
fmt.Println(psql.Quote("id").In(vals1...).String())
// ("id" IN )

vals2 := []any{}
fmt.Println(psql.Quote("id").In(psql.Arg(vals2...)).String())
// ("id" IN ())

Is this expected behavior or is it a bug?

In sqlboiler similar expressions generates code like ("id" IN (1=0))

stephenafamo commented 3 months ago

I think it is fair that trying to compare against an empty slice will be invalid. Maybe the better thing would be to throw an error instead 🤔 .

Can you help me understand why you would expect this to generate ("id" IN (1=0), or if you think another result would be more appropriate, let me know.

jacobmolby commented 3 months ago

@stephenafamo From SQLBoiler's code:

// WHERE IN () is invalid sql, so it is difficult to simply run code like:
// for _, u := range model.Users(qm.WhereIn("id IN ?",uids...)).AllP(db) {
//    ...
// }
// instead when we see empty IN we produce 1=0 so it can still be chained
// with other queries

https://github.com/volatiletech/sqlboiler/blob/master/queries/query_builders.go#L394C4-L399C25

stephenafamo commented 3 months ago

Wouldn't this be a little dangerous?

I am not sure this is the better user experience.

jacobmolby commented 3 months ago

No me neither. I'm especially concerned about if you querying a boolean column.

I also think it might be the better choice to keep it as is.

If needed, another query mod could be made with the other behavior.

atzedus commented 3 months ago

So, i try to explain source of my question.

From my experience, in many other ORM this is expected behavior.

For example (Eloquent ORM - Laravel (php)):

DB::table('products')->whereIn('id', [])->get()
// SQL: select * from `products` where 0 = 1

Bun (golang)

q = q.Where("user_id IN (?)", bun.In([]int64{}))
// SQL: user_id IN (NULL)

GORM (golang)

db.Where("user_id IN ?", []string{}).Find(&products)
// SQL: SELECT * FROM `products` WHERE user_id IN (NULL)

So, sqlboiler! :) Currently we in migration process from sqlboiler to bob, and this is why I did not expect this behavior.

stephenafamo commented 3 months ago

I think we can take a page out of Bun/GORM which sets the value to NULL. This will always be false so there is no potential bugs.

We cannot use Eloquent's method of doing 0 = 1 (which will also be evaluated as false) since Bob's structure makes it difficult to rewrite the left side of the expression based on the contents of the right side.

Try out #195 if that fixes the issue @atzedus

go get github.com/stephenafamo/bob@01e3503
atzedus commented 3 months ago

Thank you, i will test it! I think everything will be fine. I would like to clarify my expectations... For empty slice my expectations was not IN (NULL), IN (0=1), IN (false) or something else. Expectations is not to get SQL error in this case, but just empty response for SELECT or no action for DELETE, otherwise i need to write code like:

mods := []bob.Mod[*dialect.SelectQuery]{}
if len(statusIDs) != 0 {
    mods = append(mods, models.SelectWhere.Products.StatusID.IN(statusIDs)
}
stephenafamo commented 3 months ago

For empty slice my expectations was not IN (NULL), IN (0=1), IN (false) or something else.

IN (NULL) will not match anything and will result in an empty response.