From 3eac3cd8cbcdb7e9ce922c24c801368239b9654c Mon Sep 17 00:00:00 2001 From: aydinomer00 <109145643+aydinomer00@users.noreply.github.com> Date: Sat, 4 Jan 2025 22:03:13 +0300 Subject: [PATCH] fix(limit): set default limit to math.MaxInt when only offset is provided - Ensures MySQL compatibility by always providing a LIMIT when OFFSET is used - Updates tests to reflect new behavior - Improves code organization and documentation --- clause/limit.go | 31 ++++++++++----- clause/limit_test.go | 91 ++++++++++++++++++++++++++++++++++---------- 2 files changed, 93 insertions(+), 29 deletions(-) diff --git a/clause/limit.go b/clause/limit.go index 99e51cf5..c302d596 100644 --- a/clause/limit.go +++ b/clause/limit.go @@ -17,17 +17,16 @@ func (limit Limit) Name() string { // Build constructs the LIMIT clause func (limit Limit) Build(builder Builder) { - // If only offset is defined and limit is nil, set limit to math.MaxInt - if limit.Limit == nil && limit.Offset > 0 { - maxInt := math.MaxInt - limit.Limit = &maxInt - } + // NOT: We don't auto-set limit here. We only rely on the final struct's Limit and Offset. + // Any "auto offset => limit" logic is handled in MergeClause. if limit.Limit != nil && *limit.Limit >= 0 { builder.WriteString("LIMIT ") builder.AddVar(builder, *limit.Limit) } + if limit.Offset > 0 { + // Add space if LIMIT was set if limit.Limit != nil && *limit.Limit >= 0 { builder.WriteByte(' ') } @@ -41,16 +40,30 @@ func (limit Limit) MergeClause(clause *Clause) { clause.Name = "" if v, ok := clause.Expression.(Limit); ok { + // 1) Merge offset + if limit.Offset == 0 && v.Offset > 0 { + limit.Offset = v.Offset + } else if limit.Offset < 0 { + // Negative offset => 0 + limit.Offset = 0 + } + + // 2) Merge limit if (limit.Limit == nil || *limit.Limit == 0) && v.Limit != nil { limit.Limit = v.Limit } - if limit.Offset == 0 && v.Offset > 0 { - limit.Offset = v.Offset - } else if limit.Offset < 0 { - limit.Offset = 0 + // 3) If final limit is negative => treat it as nil (meaning "no limit") + if limit.Limit != nil && *limit.Limit < 0 { + limit.Limit = nil } } + // 4) If offset > 0 but limit is still nil, set limit to math.MaxInt + if limit.Offset > 0 && limit.Limit == nil { + maxInt := math.MaxInt + limit.Limit = &maxInt + } + clause.Expression = limit } diff --git a/clause/limit_test.go b/clause/limit_test.go index 36b89b8d..36c50819 100644 --- a/clause/limit_test.go +++ b/clause/limit_test.go @@ -13,68 +13,119 @@ func TestLimit(t *testing.T) { limit10 := 10 limit50 := 50 limitNeg10 := -10 + results := []struct { Clauses []clause.Interface Result string Vars []interface{} }{ + // case #0 - limit10 offset20 { - []clause.Interface{clause.Select{}, clause.From{}, clause.Limit{ - Limit: &limit10, - Offset: 20, - }}, + []clause.Interface{ + clause.Select{}, clause.From{}, + clause.Limit{Limit: &limit10, Offset: 20}, + }, "SELECT * FROM `users` LIMIT ? OFFSET ?", []interface{}{limit10, 20}, }, + // case #1 - limit0 { - []clause.Interface{clause.Select{}, clause.From{}, clause.Limit{Limit: &limit0}}, + []clause.Interface{ + clause.Select{}, clause.From{}, + clause.Limit{Limit: &limit0}, + }, "SELECT * FROM `users` LIMIT ?", []interface{}{limit0}, }, + // case #2 - limit0 offset0 => offset0 is effectively ignored { - []clause.Interface{clause.Select{}, clause.From{}, clause.Limit{Limit: &limit0}, clause.Limit{Offset: 0}}, + []clause.Interface{ + clause.Select{}, clause.From{}, + clause.Limit{Limit: &limit0}, clause.Limit{Offset: 0}, + }, "SELECT * FROM `users` LIMIT ?", []interface{}{limit0}, }, + // case #3 - only offset=20 + // MySQL demands limit if offset>0 => math.MaxInt { - // Updated test case: only Offset is given, so now we expect math.MaxInt as LIMIT - []clause.Interface{clause.Select{}, clause.From{}, clause.Limit{Offset: 20}}, + []clause.Interface{ + clause.Select{}, clause.From{}, + clause.Limit{Offset: 20}, + }, "SELECT * FROM `users` LIMIT ? OFFSET ?", []interface{}{math.MaxInt, 20}, }, + // case #4 - multiple offsets (20 -> 30) { - []clause.Interface{clause.Select{}, clause.From{}, clause.Limit{Offset: 20}, clause.Limit{Offset: 30}}, - "SELECT * FROM `users` OFFSET ?", - []interface{}{30}, + []clause.Interface{ + clause.Select{}, clause.From{}, + clause.Limit{Offset: 20}, clause.Limit{Offset: 30}, + }, + "SELECT * FROM `users` LIMIT ? OFFSET ?", + []interface{}{math.MaxInt, 30}, }, + // case #5 - merge offset20 with limit10 { - []clause.Interface{clause.Select{}, clause.From{}, clause.Limit{Offset: 20}, clause.Limit{Limit: &limit10}}, + []clause.Interface{ + clause.Select{}, clause.From{}, + clause.Limit{Offset: 20}, clause.Limit{Limit: &limit10}, + }, "SELECT * FROM `users` LIMIT ? OFFSET ?", []interface{}{limit10, 20}, }, + // case #6 - merge offset20->30 with limit10 { - []clause.Interface{clause.Select{}, clause.From{}, clause.Limit{Limit: &limit10, Offset: 20}, clause.Limit{Offset: 30}}, + []clause.Interface{ + clause.Select{}, clause.From{}, + clause.Limit{Limit: &limit10, Offset: 20}, + clause.Limit{Offset: 30}, + }, "SELECT * FROM `users` LIMIT ? OFFSET ?", []interface{}{limit10, 30}, }, + // case #7 - negative offset => offset=0 => "SELECT * FROM `users` LIMIT 10" { - []clause.Interface{clause.Select{}, clause.From{}, clause.Limit{Limit: &limit10, Offset: 20}, clause.Limit{Offset: 30}, clause.Limit{Offset: -10}}, + []clause.Interface{ + clause.Select{}, clause.From{}, + clause.Limit{Limit: &limit10, Offset: 20}, + clause.Limit{Offset: 30}, + clause.Limit{Offset: -10}, + }, "SELECT * FROM `users` LIMIT ?", []interface{}{limit10}, }, + // case #8 - negative limit => treat as nil => offset=30 => => "LIMIT ? OFFSET ?" { - []clause.Interface{clause.Select{}, clause.From{}, clause.Limit{Limit: &limit10, Offset: 20}, clause.Limit{Offset: 30}, clause.Limit{Limit: &limitNeg10}}, - "SELECT * FROM `users` OFFSET ?", - []interface{}{30}, + []clause.Interface{ + clause.Select{}, clause.From{}, + // Start with limit10 offset20 + clause.Limit{Limit: &limit10, Offset: 20}, + // Then change offset to 30 + clause.Limit{Offset: 30}, + // Then set limit to negative => remove limit => offset>0 => limit=math.MaxInt + clause.Limit{Limit: &limitNeg10}, + }, + "SELECT * FROM `users` LIMIT ? OFFSET ?", + []interface{}{math.MaxInt, 30}, }, + // case #9 - changing limit from 10 -> 50, offset=30 { - []clause.Interface{clause.Select{}, clause.From{}, clause.Limit{Limit: &limit10, Offset: 20}, clause.Limit{Offset: 30}, clause.Limit{Limit: &limit50}}, + []clause.Interface{ + clause.Select{}, clause.From{}, + clause.Limit{Limit: &limit10, Offset: 20}, + clause.Limit{Offset: 30}, + clause.Limit{Limit: &limit50}, + }, "SELECT * FROM `users` LIMIT ? OFFSET ?", []interface{}{limit50, 30}, }, - // New test example: if only Offset is defined, Limit should become math.MaxInt + // case #10 - only offset=100 => "LIMIT ? OFFSET ?", math.MaxInt, 100 { - []clause.Interface{clause.Select{}, clause.From{}, clause.Limit{Offset: 100}}, + []clause.Interface{ + clause.Select{}, clause.From{}, + clause.Limit{Offset: 100}, + }, "SELECT * FROM `users` LIMIT ? OFFSET ?", []interface{}{math.MaxInt, 100}, },