Skip to content

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

Open
darakanoit wants to merge 3 commits into
goravel:masterfrom
darakanoit:darakanoit/#1002
Open

darakanoit wants to merge 3 commits into
goravel:masterfrom
darakanoit:darakanoit/#1002

Conversation

@darakanoit

@darakanoit darakanoit commented Sep 27, 2026 •

Copy link
Copy Markdown
Contributor

📑 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

@darakanoit
darakanoit requested a review from a team as a code owner September 27, 2026 22:58
@codecov

codecov Bot commented Sep 27, 2026 •

Copy link
Copy Markdown

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

Copy link
Copy Markdown
Contributor

🤖 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

Copy link
Copy Markdown
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

WhereIn does not escape column name

2 participants