fix: [#1002] quote column names in ORM where and order methods

[darakanoit]

📑 Description

Closes goravel/goravel#1002

WhereIn and the other column based helpers of the ORM put the column into the SQL string as is:

func (r *Query) WhereIn(column string, values []any) contractsorm.Query {
	return r.Where(fmt.Sprintf("%s IN ?", column), values)
}

A reserved or case sensitive column name breaks the query on every driver
(WHERE group IN (...) is a syntax error).

What changed

The column is now passed to GORM as a clause.Column, so GORM quotes it when the query is built, with the dialect of
the connection that runs it ("group" on Postgres, SQLite and SQL Server, `group` on MySQL). Quoting at call time
would use the wrong dialect for a model whose Connection() differs from the query's connection. This applies to:

WhereIn, OrWhereIn, WhereNotIn, OrWhereNotIn, WhereBetween, WhereNotBetween, OrWhereBetween,
OrWhereNotBetween, WhereNull, OrWhereNull, WhereNotNull, WhereAll, WhereAny, WhereNone,
OrderBy, OrderByDesc.

As agreed in the issue, only plain identifiers are quoted: group and users.group (→ "users"."group").
Anything else is passed through unchanged, so existing code keeps working:

• expressions: LOWER(name);
• JSON selectors: data->name, which buildWhere still compiles through the grammar;
• names that are already quoted: "group".

OrderBy/OrderByDesc quote the column for asc and desc (GORM omits ASC: ORDER BY "order"). Any other
direction, e.g. DESC NULLS LAST, keeps the raw form, since GORM can't express it for a column. OrderByRaw and the
deprecated Order are raw by definition and are not touched.

Behavior change

On Postgres a quoted identifier is case sensitive, so WhereIn("Name", ...) against a column named name used to match
through case folding and now fails. This is consistent with Where("Name", ...), which already quotes, and with Laravel,
which always wraps column names.

Limitations

Only plain identifiers are quoted, anything else still goes into the SQL as is. This fixes reserved and case sensitive
names, but it does not make these methods safe for a column name taken from user input: WhereIn("id) OR 1=1 --", ...)
is not a plain identifier and is passed through. Such input still has to be checked against an allow list. Closing this
fully would need Laravel's rule (always wrap, expressions go through a raw expression), which we decided against in the
issue to keep existing code working.

Plain identifiers are ASCII only, so a non ASCII column name is not quoted and behaves as before.

Verification

	before	after
database/gorm TestQuoteColumns (methods above, built without a GORM instance)	fail	pass
database/gorm TestQuoteColumns (expressions, JSON selector, quoted name, custom direction stay raw)	pass	pass
TestCustomConnectionQuotesColumns (MySQL default connection, model on Postgres)	pass	pass
TestToSqlTestSuite/TestQuoteColumns (19 cases for the methods above)	fail	pass
TestToSqlTestSuite/TestQuoteColumns (expression and quoted name stay unchanged)	pass	pass
TestIsPlainIdentifier	—	pass
go test ./...	pass	pass
cd tests && go test ./... (MySQL, Postgres, SQLite, SQL Server)	pass	pass

Scope

• database/gorm/query.go: the methods above pass plain columns as clause.Column
• database/gorm/utils.go: isPlainIdentifier, columnCondition
• database/gorm/query_test.go, database/gorm/utils_test.go: unit tests
• tests/query_test.go, tests/models.go: TestCustomConnectionQuotesColumns
• tests/to_sql_test.go: the generated SQL on Postgres

No public API changed. database/db has the same problem and will be fixed in a follow up.

✅ Checks

• Added test cases for my code

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.13%. Comparing base (9385000) to head (e2bb64a).
⚠️ Report is 2 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1565      +/-   ##
==========================================
+ Coverage   72.84%   73.13%   +0.28%     
==========================================
  Files         412      412              
  Lines       26846    26868      +22     
==========================================
+ Hits        19557    19649      +92     
+ Misses       7287     7217      -70     
  Partials        2        2              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[goravel-coder]

🤖 Automated review

This is an AI-generated code review. Please double-check each finding before acting. You don't need to address every issue if they are inaccurate, but please point them out if any exist.

Summary

This PR makes the ORM's column-based helpers (WhereIn, WhereNull, OrderBy, etc.) quote plain identifiers with the driver's own quote character so reserved/case-sensitive column names work. The approach is sound for a single connection, but quoting is baked in at call time using the query's current dialect, which regresses models that use a different connection.

Verdict

• [NEW] Must Fix: 1 · Should Fix: 2 · Nits: 2

Findings

Must Fix

1. database/gorm/query.go:1826 Column quoted with the wrong connection's dialect
   Problem: quoteColumn calls r.instance.Statement.Quote when WhereIn/OrderBy are invoked, but the model's own connection is only resolved later by refreshConnection (query.go:1830), which copies the already-quoted strings into the target connection's query.
   Impact: A model whose Connection() differs from the default driver (a supported, tested feature — see tests/models.go Product/Box) gets the default driver's quote marks, e.g. backticks sent to Postgres, so the query errors; before this change the column was unquoted and GORM quoted it correctly at build time.
   Fix: Keep the raw column in the stored condition and quote it during buildWhere/buildOrder on the query returned by refreshConnection, or resolve the model connection before quoting.

   // before
   return r.Where(fmt.Sprintf("%s IN ?", r.quoteColumn(column)), values)
   // after
   // keep the raw column; quote it in buildWhere/buildOrder once
   // refreshConnection has resolved the target connection's dialector
   return r.Where(fmt.Sprintf("%s IN ?", column), values)

Should Fix

1. database/gorm/query.go:1821 Panic when the database is not configured
   Problem: quoteColumn dereferences r.instance with no nil check, and BuildGorm returns a nil *gorm.DB when Database == "" (database/driver/gorm.go:44).
   Impact: WhereIn, OrderBy and the other changed helpers now panic at call time on an unconfigured connection (verified with a standalone program); previously they only failed later at execution.
   Fix: Return the column unchanged when r.instance or its Statement is nil.

   // before
   return r.instance.Statement.Quote(column)
   // after
   if r.instance == nil || r.instance.Statement == nil {
       return column
   }
   return r.instance.Statement.Quote(column)

2. tests/to_sql_test.go:328 Quoting only tested against Postgres
   Problem: The new SQL-level test uses the suite's single Postgres query (SetupSuite builds NewTestQueryBuilder().Postgres(...)), so only double-quote identifiers are asserted.
   Impact: Wrong-dialect output (see the Must Fix finding) and broken backtick/bracket quoting pass CI unnoticed, even though NewTestQueryBuilder().All(...) can exercise every driver.
   Fix: Run the same table against the MySQL/SQLite/SQL Server queries, or assert each driver's expected quote character.

   // before
   postgresTestQuery := NewTestQueryBuilder().Postgres("", false)
   // after
   for name, q := range NewTestQueryBuilder().All("", false) {
       t.Run(name, func(t *testing.T) { /* same table */ })
   }

Nits

1. database/gorm/utils_test.go:49 Subtests named from raw column input
   Problem: t.Run(test.column, ...) uses the raw value as the name, so the empty-string case is a blank subtest and whitespace/injection cases are hard to read.
   Impact: A failing case is hard to locate in CI output.
   Fix: Add an explicit name field to the table, or format the value with %q.

   // before
   t.Run(test.column, func(t *testing.T) {
   // after
   t.Run(fmt.Sprintf("%q", test.column), func(t *testing.T) {

2. database/gorm/utils.go:8 Identifier pattern accepts ASCII only
   Problem: The regex allows only [A-Za-z0-9_], so columns with non-ASCII characters or $ are classified as expressions and skip quoting; the pattern also has no explanatory comment.
   Impact: Reserved or case-sensitive columns using those characters remain broken on drivers that accept them.
   Fix: Widen the character class where drivers allow it, or add a comment documenting the ASCII-only limitation (already noted in the PR body).

   // before
   var plainIdentifierRegex = regexp.MustCompile(`^[A-Za-z_][A-Za-z0-9_]*(\.[A-Za-z_][A-Za-z0-9_]*)*$`)
   // after
   // ASCII-only plain/dotted identifiers; other input is treated as an expression

---

Note: No prior automated review rounds were found on this PR, so there was nothing to reconcile or tick. OrderByRaw/Order and the aggregate helpers (Sum/Avg/Min/Max) were left untouched by design/scope and are not counted here.

[darakanoit]

Addressed in e2bb64a.

1. Wrong connection's dialect: confirmed. With MySQL as the default connection and a model on Postgres, WhereIn sent `name` IN (...) to Postgres. Columns are now passed to gorm as clause.Column, so gorm quotes them when the query is built, with the dialect of the connection that runs it. OrderBy uses clause.OrderByColumn for asc/desc; any other direction keeps the raw form, since gorm can't express it for a column.
2. Panic without a database: gone with the same change, these methods no longer touch the gorm instance. The unit test builds the query without one.
3. Postgres only: running the table on every driver wouldn't catch 1, it needs two connections with different quotes. TestCustomConnectionQuotesColumns covers exactly that (MySQL default, model on Postgres) and fails before the fix.
4. Subtest names: added.
5. ASCII only: documented above the regex.

[goravel-coder]

🤖 Automated review

This is an AI-generated code review. Please double-check each finding before acting. You don't need to address every issue if they are inaccurate, but please point them out if any exist.

Summary

Round 2 reworks the quoting so plain columns are handed to GORM as clause.Column and quoted at build time by the connection that runs the query — that genuinely resolves the wrong-dialect and unconfigured-database findings from round 1. What remains is test hygiene (a container leak and tests that do not actually assert quoting) plus a few small consistency nits.

Verdict

• [NEW] Must Fix: 0 · Should Fix: 1 · Nits: 7
• ROUND 1 Must Fix: 0 · Should Fix: 1 · Nits: 0 · Resolved: 4

Findings

Should Fix

1. tests/query_test.go:5024 New integration test leaves its containers running
   Problem: TestCustomConnectionQuotesColumns starts a MySQL and a Postgres container through NewTestQueryBuilder() but never calls docker.Shutdown(), while the neighbouring TestCustomConnection does.
   Impact: Every run leaves two containers behind until they are stopped by hand, which piles up on CI runners and developer machines.
   Fix: Shut both containers down at the end of the test (a t.Cleanup is a good spot).

   // after
   for _, q := range []*TestQuery{mysqlTestQuery, postgresTestQuery} {
       docker, err := q.Driver().Docker()
       assert.NoError(t, err)
       assert.NoError(t, docker.Shutdown())
   }

Nits

1. tests/query_test.go:5047 Test would pass without any quoting
   Problem: Every column used here (name, weight, height, id) is a valid unquoted identifier on Postgres, so the test still passes if the quoting this PR adds is removed.
   Impact: A regression that drops quoting goes unnoticed; only the wrong-dialect mistake is caught.
   Fix: Use a reserved-word column (for example group/order) in at least one case, or assert the generated SQL.

   // after
   query: query.WhereIn("group", []any{"a", "b"}) // needs a reserved-word column on the model

2. database/gorm/utils.go:22 GORM type stored in the shared driver contract
   Problem: columnCondition puts a gorm.io/gorm/clause.Column into contractsdriver.Where.Args, and contracts/database/driver is the driver-agnostic contract shared with the database/db builder.
   Impact: The contract now carries a GORM-only value that another consumer of Where.Args cannot interpret, and the new unit test enshrines it.
   Fix: Keep the marker driver-neutral, or quote through the shared grammar so both builders agree.

   // before
   return "?", []any{clause.Column{Name: column}}
   // after
   return "?", []any{contractsdriver.Identifier(column)}

3. database/gorm/query.go:597 Reserved column is not quoted for a custom direction
   Problem: OrderBy only quotes when the direction is exactly ASC/DESC; any other direction (for example DESC NULLS LAST) falls through to OrderByRaw, which leaves the column raw.
   Impact: OrderBy("group") is fixed but OrderBy("group", "DESC NULLS LAST") still breaks on a reserved column.
   Fix: Quote the column for every direction, e.g. with clause.OrderBy binding a clause.Column plus the raw direction expression.

   // before
   if isPlainIdentifier(column) {
       switch strings.ToUpper(strings.TrimSpace(orderDirection)) {
       // ...
   }
   return r.OrderByRaw(fmt.Sprintf("%s %s", column, orderDirection))
   // after
   // bind the column as clause.Column for every direction

4. database/gorm/query.go:1024 Variable named query holds a placeholder, not a query
   Problem: query, args := columnCondition(column) is embedded as fmt.Sprintf("%s IN ?", query), so query is "?" (or a raw column), while the multi-column callers call the same value columnQuery.
   Impact: The misleading name makes the ~14 call sites harder to read and easier to misuse in future edits.
   Fix: Rename to columnQuery (or condition) and align the columnCondition doc comment.

   // before
   query, args := columnCondition(column)
   return r.Where(fmt.Sprintf("%s IN ?", query), append(args, values)...)
   // after
   columnQuery, args := columnCondition(column)
   return r.Where(fmt.Sprintf("%s IN ?", columnQuery), append(args, values)...)

5. database/gorm/query.go:1284 addOrder copies the conditions but appends to the original
   Problem: addOrder assigns conditions := r.conditions and then appends r.conditions.order, while the sibling addWhere appends to the local copy.
   Impact: The snapshot-then-mutate pattern is inconsistent, which invites a bug if the copy ever becomes independent.
   Fix: Use the local variable, as addWhere does.

   // before
   conditions.order = deep.Append(r.conditions.order, order)
   // after
   conditions.order = deep.Append(conditions.order, order)

6. database/gorm/query_test.go:941 Expression IN only exercised with one value
   Problem: The non-identifier IN path is tested with a single value only, so a multi-value slice is never asserted.
   Impact: A change that spreads the value slice into separate arguments would not be caught.
   Fix: Add a multi-value case to the table.

   // before
   return query.WhereIn("LOWER(name)", []any{"a"})
   // after
   return query.WhereIn("LOWER(name)", []any{"a", "b"})

7. database/gorm/utils_test.go:23 Identifier regex tests miss whitespace and non-ASCII input
   Problem: TestIsPlainIdentifier has no cases for inputs like " group", "group ", or a non-ASCII name.
   Impact: A regex change that starts accepting surrounding whitespace or non-ASCII letters would pass unnoticed.
   Fix: Add cases asserting whitespace-padded and non-ASCII names are rejected.

   // after
   {name: "leading space", column: " group", expected: false},
   {name: "non ascii", column: "größe", expected: false},