Using an @ in a string passed to Select results in incorrect query

[markhildreth-gravity]

GORM Playground Link

go-gorm/playground#764

Description

When using the @ character in a string passed to Select(), the remaining strings passed to select are ignored.

For example, these two queries...

DB.Table("users").Select("name = 'test example.com' as is_example", "age").Scan(&results).Error
DB.Table("users").Select("name = '[email protected]' as is_example", "age").Scan(&results).Error

... generates the following SQL, respectively...

SELECT name = 'test example.com' as is_example,age FROM `users`
SELECT name = '[email protected]' as is_example FROM `users`

Note how in the second, age is left off. This only seems to happen if the @ is included in the first string.

[ivila]

@markhildreth-gravity I think the root cause is here:

gorm/chainable_api.go

Line 141 in 52e3b35

} else if strings.Count(v, "@") > 0 && len(args) > 0 {

If it finds @ in your first string(aka the query parameter), it treat your parameters as NamedArgument

a quick fix is change your codes to:

// make query an slice of string, so that it won't go to the unexpected branch
DB.Table("users").Select([]string{"name = '[email protected]' as is_example", "age"}).Scan(&results).Error

[markhildreth-gravity]

@ivila Thanks for the quick response. That makes sense why the code isn't working as epxected, although I will say it's not immediately as a developer that the code is wrong. I should point out this was a quick case to reproduce, but the issue originally occurred because I was using @ as part of using PostgreSQL's range operations.

I'd like to recommend the following:

1.) Change the docs to always use the slice type when listing multiple items:

Anywhere in the docs where Select() is used as a list of values (rather than the "template, ...args" usage), always use the slice notation. For example, when the docs show examples like these, it's not immediately clear that this is only a "list of strings" approach because of the contents of the first string. So the example in the docs that shows this...

// Querying multiple columns
db.Select("name", "age").Scan(&users)
db.Select("name", "age").Find(&users)

...could be changed to this...

// Querying multiple columns
db.Select([]string{ "name", "age" }).Scan(&users)
db.Select([]string{ "name", "age" }).Find(&users)

2.) Ideally, the two use cases (list of strings vs string template w/ args) are split into different methods (e.g., a Select and SelectTmpl). Or change the Select some other way to make it more obvious that there are two different ways that Select can be used and you have to explicitly opt into one of them. Obviously, this is a breaking change, so this is more a suggestion for any future major version.

[boichee]

I happen to have been looking at exactly this code in gorm today, trying to figure out how I might be able to add some custom finishers.

If you check out line 141 in chainable_api.go you'll see that if the first argument to Select() is a string, and you provide additional variadic arguments, gorm starts looking for placeholders (einther ? or @). It does this somewhat naively, just by counting the number of placeholders present in the string. Unfortunately, in your case the @ in the first argument is actually meant to be an email.

@markhildreth-gravity my recommendation would actually just be to reverse the order of the parameters you pass to Select() so its like this:

DB.Table("users").Select("age", "name = '[email protected]' as is_example").Scan(&results).Error

That should allow the @ in the email to bypass that check for placeholders and I'd think that would make it work the way you expect.

---

Code from gorm itself that I referenced is this (starting at line 135). In the following code v is the first argument passed to Select:

        case string:
		if strings.Count(v, "?") >= len(args) && len(args) > 0 {
			tx.Statement.AddClause(clause.Select{
				Distinct:   db.Statement.Distinct,
				Expression: clause.Expr{SQL: v, Vars: args},
			})
		} else if strings.Count(v, "@") > 0 && len(args) > 0 {
			tx.Statement.AddClause(clause.Select{
				Distinct:   db.Statement.Distinct,
				Expression: clause.NamedExpr{SQL: v, Vars: args},
			})
		} else {
			tx.Statement.Selects = []string{v}

			for _, arg := range args {
				switch arg := arg.(type) {
				case string:
					tx.Statement.Selects = append(tx.Statement.Selects, arg)
				case []string:
					tx.Statement.Selects = append(tx.Statement.Selects, arg...)
				default:
					tx.Statement.AddClause(clause.Select{
						Distinct:   db.Statement.Distinct,
						Expression: clause.Expr{SQL: v, Vars: args},
					})
					return
				}
			}

			if clause, ok := tx.Statement.Clauses["SELECT"]; ok {
				clause.Expression = nil
				tx.Statement.Clauses["SELECT"] = clause
			}
		}

[markhildreth-gravity]

@markhildreth-gravity my recommendation would actually just be to reverse the order of the parameters you pass to Select() so its like this:

DB.Table("users").Select("age", "name = '[email protected]' as is_example").Scan(&results).Error

Thanks for looking into this. The problem I had was already solved by passing my values as a slice. And it seems like switching the argument ordering as you recommend would also solve the specific issue that I found myself in. However, I would still prefer using a string slice when the goal of the method is to give a list of columns to return, as it makes it less likely that a change to the arguments might errantly cause Gorm to switch to "template" mode.

Put another way, if I write code like this...

func BuildQuery(db *gorm.DB, a string, b string) *gorm.DB {
   return db.Select(a, b)
}

...it's impossible to know which "mode" Gorm will use. It entirely depends on what the value of a is. That makes understanding and using the library difficult. However, if I do the following...

func BuildQuery(db *gorm.DB, a string, b string) *gorm.DB {
   return db.Select([]string{ a, b })
}

... then I know for sure that Gorm will ALWAYS treat the strings as a list of columns, rather than templates.

As I mentioned, changing the API so that the two modes occur using different methods would IMO be a better solution, but that's obvious a breaking change.