Gorm Broken SQL generation after v.1.25.6/7 (nested Where?)

GORM Playground Link

https://github.com/go-gorm/playground/pull/689

Description

It looks like something in v.1.25.6 changed / broke, how SQL (at least for Postgres) is generated. Especially in the case of nested WHERE (+ JOIN?) like shown below. It was the smallest example I could create, but maybe it also occures only with nested WHERE?

As this changes the result completely and outputing simply all results, this could be a critical thing. We use this kind of query to combine search-text with user permissions, and in this case it makes user permissions completely ignored through the first users.name ILIKE '%%' OR. Notice the additional users.name ILIKE '%%' OR companies.name ILIKE '%%' AND.

Copies from playground PR:

query := DB.Table("users").
        Select("companies.country", "COUNT(DISTINCT users.name)").
        Joins("LEFT JOIN companies ON users.company_id = companies.id").
        Group("companies.country")
    query.Where(query.Where("users.name ILIKE ?", "%%").
        Or("companies.name ILIKE ?", "%%"))
    query.Where("users.something IN (?)", []string{"1"})

Query v.1.25.5:

SELECT companies.country, COUNT(DISTINCT users.name)
FROM "users"
         LEFT JOIN companies ON users.company_id = companies.id
WHERE (users.name ILIKE '%%' OR companies.name ILIKE '%%')
  AND users.something IN ('1')
GROUP BY "companies"."country";

Result: [{DE 1} {GB 1}]

Query v1.25.6+:

SELECT companies.country, COUNT(DISTINCT users.name)
FROM "users"
         LEFT JOIN companies ON users.company_id = companies.id
WHERE users.name ILIKE '%%'
   OR companies.name ILIKE '%%' AND (users.name ILIKE '%%' OR companies.name ILIKE '%%') AND users.something IN ('1')
GROUP BY "companies"."country";

Result: [{DE 2} {GB 1} {US 1}]

I was not able to track this issue down to a specific change. If I can further support, please get back

Comment From: janisz

I think I found a broken generation in different part as well. In my case migration is not working due to broken sql that uses $2 but not $1 resulting in insufficient arguments - https://github.com/stackrox/stackrox/pull/9921

https://github.com/stackrox/stackrox/actions/runs/7900053395?pr=9921#summary-21560778111

Comment From: urashidmalik

existing unique keys are also broken reverting back to 1.25.5 works

Comment From: a631807682

refer to https://gorm.io/docs/method_chaining.html#Reusability-and-Safety

query := DB.Table("users").
        Select("companies.country", "COUNT(DISTINCT users.name)").
        Joins("LEFT JOIN companies ON users.company_id = companies.id").
        Group("companies.country")
-   query.Where(query.Where("users.name ILIKE ?", "%%").
+   query.Where(query.Session(&gorm.Session{}).Where("users.name ILIKE ?", "%%").
        Or("companies.name ILIKE ?", "%%"))
    query.Where("users.something IN (?)", []string{"1"})

Comment From: chbiel

@a631807682 although that seem to fix it, the documentation at https://gorm.io/docs/advanced_query.html#Group-Conditions clearly says that the way without query.Session(&gorm.Session{}) should work as well.

further the documentation you linked is about "reusing queries", what that does not happen here. We have one query with different sub conditions only split into different calls, to make it more readable (or in our concrete case have a function that adds the Where conditions) and as linked above, that should not break.

Comment From: chbiel

@jinzhu as the issue is now open for nearly two weeks, do you have an idea what broke that behaviour? or do you at least agree that it's a critical bug?

Comment From: a631807682

@a631807682 although that seem to fix it, the documentation at gorm.io/docs/advanced_query.html#Group-Conditions clearly says that the way without query.Session(&gorm.Session{}) should work as well.

further the documentation you linked is about "reusing queries", what that does not happen here. We have one query with different sub conditions only split into different calls, to make it more readable (or in our concrete case have a function that adds the Where conditions) and as linked above, that should not break.

This is as expected, for more detailed information refer to https://github.com/go-gorm/gorm/issues/6811#issuecomment-1916381803 https://github.com/go-gorm/gorm/issues/5182#issuecomment-1075953290

Comment From: chbiel

Hmm. I still don't agree. The documentation https://gorm.io/docs/advanced_query.html#Group-Conditions generates (at least in the code outline) the correct SQL.

Or is your point about the separation?

query := DB.Table("users").Where("users.something IN (?)", []string{"1"})
query.Where(query.Where("users.name ILIKE ?", "%%").Or("companies.name ILIKE ?", "%%"))

vs.

query := DB.Table("users").Where("users.something IN (?)", []string{"1"}).Where(query.Where("users.name ILIKE ?", "%%").Or("companies.name ILIKE ?", "%%"))

Comment From: a631807682

The chainable api does not change the DB instance, it returns a new copy. But query, as a new copy, will be changed by chainable api.

// query change by nested where
query.Where(query.Where("users.name ILIKE ?", "%%").Or("companies.name ILIKE ?", "%%"))
// query or DB will not change
query.Where(DB.Where("users.name ILIKE ?", "%%").Or("companies.name ILIKE ?", "%%"))

Comment From: chbiel

OK I understand, thanks for clearification.

But I would still say, that the documentation at https://gorm.io/docs/advanced_query.html#Group-Conditions is not correct about that, as it uses lowercase db suggesting it is a local variable that is reused here (like we used it).

So the output of

db.Where(
  db.Where("pizza = ?", "pepperoni").Where(db.Where("size = ?", "small").Or("size = ?", "medium")),
).Or(
  db.Where("pizza = ?", "hawaiian").Where("size = ?", "xlarge"),
).Find(&Pizza{})

is not

SELECT *
FROM `pizzas`
WHERE (pizza = "pepperoni" AND (size = "small" OR size = "medium")) OR (pizza = "hawaiian" AND size = "xlarge")

but

SELECT *
FROM `pizzas`
WHERE (pizza = "hawaiian" AND size = "xlarge") AND -- db.Where("pizza = ?", "hawaiian").Where("size = ?", "xlarge")
    (size = "small" OR size = "medium") AND -- db.Where("size = ?", "small").Or("size = ?", "medium")
    (pizza = "pepperoni" AND (size = "small" OR size = "medium")) AND -- db.Where("pizza = ?", "pepperoni").Where(db.Where("size = ?", "small").Or("size = ?", "medium")),
    ((pizza = "pepperoni" AND (size = "small" OR size = "medium")) OR (pizza = "hawaiian" AND size = "xlarge")) -- all combined

because the inner db.Wheres also modify the query directly.

To get the SQL from the example, it would need to be

db.Where(
  DB.Where("pizza = ?", "pepperoni").Where(DB.Where("size = ?", "small").Or("size = ?", "medium")),
).Or(
  DB.Where("pizza = ?", "hawaiian").Where("size = ?", "xlarge"),
).Find(&Pizza{})

Or do I still not get something here?