From 5574fdae8883bf50c845e285554f8d1c08a3e33a Mon Sep 17 00:00:00 2001 From: Thomas Miller Date: Fri, 6 Feb 2026 06:30:30 +0000 Subject: [PATCH 1/3] fix: implements a behaviour change to zero slice values This commit changes the contract that GetAll has with the caller around how slices are populated for queries. Prior to this change GetAll would append all query values to the slice supplied resulting in existing slice elements being retained. This poses a problem when failed transactions are retried because should a call have previously succeeded but the transaction fails for some other reason then the slice will get the same values again or at least the changed subset. I am unsure if this behaviour was on purpose in the original implementation but it does fight the standard SQL package where values are always overwritten in a query. If a previous caller to this func was relying on this behaviour then it makes sense that a AppendAll variant could also be provided. Due to the change in behaviour of this commit it MUST be rolled out in a version bump of Sqlair. I have also added regression testing to the package to catch this going forward. --- package_test.go | 74 +++++++++++++++++++++++++++++++++++++++++++++++-- sqlair.go | 6 ++++ 2 files changed, 78 insertions(+), 2 deletions(-) diff --git a/package_test.go b/package_test.go index 436b634e..752de899 100644 --- a/package_test.go +++ b/package_test.go @@ -727,6 +727,76 @@ func (s *PackageSuite) TestGetAllErrors(c *C) { } } +// TestGetAllMutatesExistingSliceValues is a regression test to assert a +// behaviour change made to [Query.GetAll]. Prior to this calling [Query.GetAll] +// with one or more slice values that had pre existing elements len(s) > 0 would +// maintain the existing elements in the slice. +// +// This is a problem for transaction closures that are retried due to failures +// and may have had a successful read into a slice prior to a transaction +// failure. +func (s *PackageSuite) TestGetAllMutatesExistingSliceValues(c *C) { + db, tables := s.personAndAddressDB(c) + defer dropTables(c, db, tables...) + + stmt, err := sqlair.Prepare("SELECT &Person.* FROM person", Person{}) + c.Assert(err, IsNil) + + var dbVals []Person + err = db.Query(nil, stmt).GetAll(&dbVals) + c.Assert(err, IsNil) + + initialLen := len(dbVals) + c.Check(initialLen >= 0, Equals, true, + Commentf("expected at least one or more person records")) + + err = db.Query(nil, stmt).GetAll(&dbVals) + c.Assert(err, IsNil) + + c.Check(len(dbVals), Equals, initialLen) +} + +// TestGetAllMutatesMultipleExistingSliceValues is a regression test to assert a +// behaviour change made to [Query.GetAll]. Prior to this calling [Query.GetAll] +// with one or more slice values that had pre existing elements len(s) > 0 would +// maintain the existing elements in the slice. +// +// This is a problem for transaction closures that are retried due to failures +// and may have had a successful read into a slice prior to a transaction +// failure. +func (s *PackageSuite) TestGetAllMutatesMultipleExistingSliceValues(c *C) { + db, tables := s.personAndAddressDB(c) + defer dropTables(c, db, tables...) + + stmt, err := sqlair.Prepare(` +SELECT person.* AS &Person.*, + address.* AS &Address.* +FROM person +INNER JOIN address ON person.address_id = address.id +`, Address{}, Person{}) + c.Assert(err, IsNil) + + var ( + addressDBVals []Address + personDBVals []Person + ) + err = db.Query(nil, stmt).GetAll(&personDBVals, &addressDBVals) + c.Assert(err, IsNil) + + initialAddressLen := len(personDBVals) + c.Check(initialAddressLen >= 0, Equals, true, + Commentf("expected at least one or more address records")) + initialPersonLen := len(personDBVals) + c.Check(initialPersonLen >= 0, Equals, true, + Commentf("expected at least one or more person records")) + + err = db.Query(nil, stmt).GetAll(&personDBVals, &addressDBVals) + c.Assert(err, IsNil) + + c.Check(len(addressDBVals), Equals, initialAddressLen) + c.Check(len(personDBVals), Equals, initialPersonLen) +} + func (s *PackageSuite) TestRun(c *C) { db, tables := s.personAndAddressDB(c) defer dropTables(c, db, tables...) @@ -1118,10 +1188,10 @@ func (s *PackageSuite) TestTransactionErrors(c *C) { // Test error when running query after rollback against the public error variable. tx, err = db.Begin(ctx, nil) c.Assert(err, IsNil) - + err = tx.Rollback() c.Assert(err, IsNil) - + err = tx.Query(ctx, insertStmt, &derek).Run() if !errors.Is(err, sql.ErrTxDone) { c.Errorf("expected %q, got %q", sql.ErrTxDone, err) diff --git a/sqlair.go b/sqlair.go index ad23d0b1..0025e338 100644 --- a/sqlair.go +++ b/sqlair.go @@ -317,6 +317,9 @@ func (o *Outcome) Result() sql.Result { // A pointer to an empty [Outcome] struct may be provided as the first output // variable to get information about query execution. // +// A provided slice that already contains one or more values will have its +// length reset to 0 before scanning into the slice. +// // [ErrNoRows] will be returned if no rows are found. func (q *Query) GetAll(sliceArgs ...any) (err error) { if q.err != nil { @@ -348,6 +351,9 @@ func (q *Query) GetAll(sliceArgs ...any) (err error) { if sliceVal.Kind() != reflect.Slice { return fmt.Errorf("need pointer to slice, got pointer to %s", sliceVal.Kind()) } + // Set the length of the slice value back to zero maintaing any existing + // allocated capacity. + sliceVal.SetLen(0) sliceVals = append(sliceVals, sliceVal) } From 042ded860c0f2ceca42409a035c669ed7b481e9a Mon Sep 17 00:00:00 2001 From: Thomas Miller Date: Thu, 12 Feb 2026 00:39:44 +0000 Subject: [PATCH 2/3] docs: fix typo in func docs around behaviour --- sqlair.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sqlair.go b/sqlair.go index 0025e338..74e4b0d9 100644 --- a/sqlair.go +++ b/sqlair.go @@ -351,8 +351,8 @@ func (q *Query) GetAll(sliceArgs ...any) (err error) { if sliceVal.Kind() != reflect.Slice { return fmt.Errorf("need pointer to slice, got pointer to %s", sliceVal.Kind()) } - // Set the length of the slice value back to zero maintaing any existing - // allocated capacity. + // Set the length of the slice value back to zero maintaining any + // existing capacity. sliceVal.SetLen(0) sliceVals = append(sliceVals, sliceVal) } From dee79e3690a43f64904d8f8703e2371c6febed58 Mon Sep 17 00:00:00 2001 From: Thomas Miller Date: Thu, 12 Feb 2026 00:45:54 +0000 Subject: [PATCH 3/3] docs: fix incorrect link to ErrTxDone This commit fixes a broken link toa variable ErrTxDone that does not reside in the sqlair package but in the database/sql package. --- docs/howto/tx.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/howto/tx.md b/docs/howto/tx.md index df80a2d7..c5315db8 100644 --- a/docs/howto/tx.md +++ b/docs/howto/tx.md @@ -54,5 +54,5 @@ if err != nil { :class: tip [TX.Commit](https://pkg.go.dev/github.com/canonical/sqlair#TX.Commit), [TX.Rollback](https://pkg.go.dev/github.com/canonical/sqlair#TX.Rollback), -[sqlair.ErrTXDone](https://pkg.go.dev/github.com/canonical/sqlair#ErrTXDone) +[sql.ErrTXDone](https://pkg.go.dev/database/sql#ErrTxDone) ```