diff --git a/queries/qm/query_mods.go b/queries/qm/query_mods.go index 2d21211bf..087c30d9b 100644 --- a/queries/qm/query_mods.go +++ b/queries/qm/query_mods.go @@ -193,6 +193,21 @@ func Distinct(clause string) QueryMod { } } +type scopedExprParensQueryMod struct{} + +// Apply implements QueryMod.Apply. +func (qm scopedExprParensQueryMod) Apply(q *queries.Query) { + queries.SetScopedExprParens(q) +} + +// ScopedExprParens confines the suppression of automatic where-clause +// parenthesization to the conditions inside the Expr that requested it. +// Without it a single Expr leaves every other condition unbracketed, so a +// condition holding a bare OR binds at the top level and swallows the rest. +func ScopedExprParens() QueryMod { + return scopedExprParensQueryMod{} +} + type withQueryMod struct { clause string args []interface{} diff --git a/queries/query.go b/queries/query.go index d9ccd3160..9f5d0a9b4 100644 --- a/queries/query.go +++ b/queries/query.go @@ -44,6 +44,8 @@ type Query struct { offset int forlock string distinct string + + scopedExprParens bool } // Applicator exists only to allow @@ -244,6 +246,13 @@ func SetDistinct(q *Query, distinct string) { q.distinct = distinct } +// SetScopedExprParens confines the suppression of automatic where-clause +// parenthesization to the conditions inside the Expr that requested it, +// instead of disabling it for the whole query. +func SetScopedExprParens(q *Query) { + q.scopedExprParens = true +} + // SetCount on the query. func SetCount(q *Query) { q.count = true diff --git a/queries/query_builders.go b/queries/query_builders.go index 7422de43d..251c55103 100644 --- a/queries/query_builders.go +++ b/queries/query_builders.go @@ -337,13 +337,18 @@ func whereClause(q *Query, startAt int) (string, []interface{}) { return "", nil } + // Without scopedExprParens a single paren marker anywhere turns automatic + // bracketing off for every condition; with it, bracketing is decided per + // condition from the nesting depth, so the pre-scan has nothing to answer. manualParens := false -ManualParen: - for _, w := range q.where { - switch w.kind { - case whereKindLeftParen, whereKindRightParen: - manualParens = true - break ManualParen + if !q.scopedExprParens { + ManualParen: + for _, w := range q.where { + switch w.kind { + case whereKindLeftParen, whereKindRightParen: + manualParens = true + break ManualParen + } } } @@ -352,6 +357,7 @@ ManualParen: var args []interface{} notFirstExpression := false + depth := 0 buf.WriteString(" WHERE ") for _, where := range q.where { if notFirstExpression && where.kind != whereKindRightParen { @@ -364,9 +370,15 @@ ManualParen: notFirstExpression = true } + // A condition inside a paren group is already bounded by that group. + autoWrap := !manualParens + if q.scopedExprParens { + autoWrap = depth == 0 + } + switch where.kind { case whereKindNormal: - if !manualParens { + if autoWrap { buf.WriteByte('(') } if q.dialect.UseIndexPlaceholders { @@ -376,15 +388,17 @@ ManualParen: } else { buf.WriteString(where.clause) } - if !manualParens { + if autoWrap { buf.WriteByte(')') } args = append(args, where.args...) case whereKindLeftParen: buf.WriteByte('(') + depth++ notFirstExpression = false case whereKindRightParen: buf.WriteByte(')') + depth-- case whereKindIn: ln := len(where.args) // WHERE IN () is invalid sql, so it is difficult to simply run code like: @@ -404,11 +418,11 @@ ManualParen: // probably needs adjustment, or the user is passing in invalid clauses. if matches == nil { clause, count := convertInQuestionMarks(q.dialect.UseIndexPlaceholders, where.clause, startAt, 1, ln) - if !manualParens { + if autoWrap { buf.WriteByte('(') } buf.WriteString(clause) - if !manualParens { + if autoWrap { buf.WriteByte(')') } args = append(args, where.args...) @@ -440,13 +454,13 @@ ManualParen: leftClause = strings.Join(cols, ",") } rightClause, rightCount := convertInQuestionMarks(q.dialect.UseIndexPlaceholders, rightSide, startAt+leftCount, groupAt, ln-leftCount) - if !manualParens { + if autoWrap { buf.WriteByte('(') } buf.WriteString(leftClause) buf.WriteString(" IN ") buf.WriteString(rightClause) - if !manualParens { + if autoWrap { buf.WriteByte(')') } startAt += leftCount + rightCount diff --git a/queries/scoped_expr_parens_test.go b/queries/scoped_expr_parens_test.go new file mode 100644 index 000000000..a3357809a --- /dev/null +++ b/queries/scoped_expr_parens_test.go @@ -0,0 +1,89 @@ +package queries_test + +import ( + "testing" + + "github.com/volatiletech/sqlboiler/drivers" + "github.com/volatiletech/sqlboiler/queries" + "github.com/volatiletech/sqlboiler/queries/qm" +) + +// selectPrefix is what a select with no columns and one FROM table renders +// before its WHERE clause. +const selectPrefix = `SELECT * FROM "t"` + +// TestScopedExprParens pins both halves of the ScopedExprParens contract: a +// query that does not apply the mod renders exactly as it did before the mod +// existed, and one that does keeps automatic parentheses on every condition +// outside the Expr group that suppressed them. +// +// The file is package queries_test because queries/qm imports queries, so an +// in-package test could not reach the mod constructors. +func TestScopedExprParens(t *testing.T) { + t.Parallel() + + // An Expr group holding a bare OR. Every case carries it, so the + // suppression that ScopedExprParens confines is always triggered. + exprGroup := qm.Expr(qm.Where("a = 1"), qm.Or2(qm.Where("b = 2"))) + + tests := []struct { + name string + optIn bool + mods []qm.QueryMod + expect string + }{ + { + name: "without the mod an Expr unbrackets the whole query", + optIn: false, + mods: []qm.QueryMod{exprGroup, qm.Where("c = 3 or d = 4")}, + expect: ` WHERE (a = 1 OR b = 2) AND c = 3 or d = 4`, + }, + { + name: "with the mod conditions outside the group keep brackets", + optIn: true, + mods: []qm.QueryMod{exprGroup, qm.Where("c = 3 or d = 4")}, + expect: ` WHERE (a = 1 OR b = 2) AND (c = 3 or d = 4)`, + }, + { + name: "nested groups gain brackets only at the top level", + optIn: true, + mods: []qm.QueryMod{ + qm.Expr(qm.Where("a = 1"), qm.Expr(qm.Where("b = 2"))), + qm.Where("c = 3 or d = 4"), + }, + expect: ` WHERE (a = 1 AND (b = 2)) AND (c = 3 or d = 4)`, + }, + { + name: "an IN condition outside the group keeps brackets", + optIn: true, + mods: []qm.QueryMod{exprGroup, qm.WhereIn("c in ?", 1, 2)}, + expect: ` WHERE (a = 1 OR b = 2) AND ("c" IN ($1,$2))`, + }, + { + // The empty-IN path writes its own brackets and returns early, + // so it must not pick up a second pair. + name: "an empty IN condition stays singly bracketed", + optIn: true, + mods: []qm.QueryMod{exprGroup, qm.WhereIn("c in ?")}, + expect: ` WHERE (a = 1 OR b = 2) AND (1=0)`, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + q := &queries.Query{} + queries.SetDialect(q, &drivers.Dialect{LQ: '"', RQ: '"', UseIndexPlaceholders: true}) + queries.SetFrom(q, "t") + qm.Apply(q, test.mods...) + if test.optIn { + qm.Apply(q, qm.ScopedExprParens()) + } + + result, _ := queries.BuildQuery(q) + expect := selectPrefix + test.expect + ";" + if result != expect { + t.Errorf("Mismatch between expect and result:\n%s\n%s\n", expect, result) + } + }) + } +}