canonical / canonical/sqlair

`SELECT ... WHERE col = ($foo[:])` behaves inconsistantly based on the length of foo

Open
#193 1 comment 0 reactions 0 assignees View on GitHub
Dominant language
Go
Stars
19
Forks
10
PR merge metrics
No merged PRs in 30d

Description

`SELECT ... WHERE col = ($foo[:])` has a nasty gotcha.

If the query happens to use a `foo` of length 1, the query evalutes nicely to `WHERE col = val`.

However, if `foo` contains multiple values, we (correctly imo) error out with `row value misued`.

This is a nasty gotcha because in test cases it is not at all unusual to test with only one value in `foo`, which will result in a silent false positive. So it's very possible that unit tests, integration tests, and manual tests will miss this failure under certain circumstances

Also, `SELECT ... WHERE col = ($foo[:])` and the correct syntax `SELECT ... WHERE col IN ($foo[:])` are very similar, it's very likely a reviewer will miss this error

## Replication steps

```golang
package juju

import (
"context"
"testing"

"github.com/canonical/sqlair"
"github.com/juju/tc"

schematesting "github.com/juju/juju/domain/schema/testing"
)

type suite struct {
schematesting.ModelSuite
}

func TestSuite(t *testing.T) {
tc.Run(t, &suite{})
}

type (
life struct {
ID int `db:"id"`
Value string `db:"value"`
}
ids []int
)

var stmt = sqlair.MustPrepare(`
SELECT &life.*
FROM life
WHERE id = ($ids[:])
`, life{}, ids{})

func (s *suite) TestTestSingular(c *tc.C) {
lifeIDs := ids([]int{0})
err := s.TxnRunner().Txn(c.Context(), func(ctx context.Context, tx *sqlair.TX) error {
return tx.Query(ctx, stmt, lifeIDs).GetAll(&[]life{})
})
c.Assert(err, tc.ErrorIsNil)
}

func (s *suite) TestTestMultiple(c *tc.C) {
lifeIDs := ids([]int{0, 1})
err := s.TxnRunner().Txn(c.Context(), func(ctx context.Context, tx *sqlair.TX) error {
return tx.Query(ctx, stmt, lifeIDs).GetAll(&[]life{})
})
c.Assert(err, tc.ErrorIsNil)
}
```
Results in:
```
$ go test ./
--- FAIL: TestSuite (0.12s)
--- FAIL: TestSuite/TestTestMultiple (0.07s)
/home/jack/playground/main_test.go:44
main_test.go:56:
c.Assert(err, tc.ErrorIsNil)
... value *errors.Err = &errors.unformatter{message:"", cause:sqlite3.Error{Code:1, ExtendedCode:1, SystemErrno:0x0, err:"row value misused"}, previous:(*errors.Err)(0xc0004e1590), function:"github.com/juju/retry.Call", line:188} ("row value misused")
... error stack:
row value misused
github.com/juju/juju/internal/database/testing.(*txnRunner).Txn.func1.(*RetryingTxnRunner).Txn.2:166:
github.com/juju/juju/internal/database/txn.(*RetryingTxnRunner).run:322:
github.com/juju/juju/internal/database/testing.(*txnRunner).Txn.func1:34:
github.com/juju/juju/internal/database/txn.newOptions.DefaultRetryStrategy.func1.1:342:
github.com/juju/retry.Call:188:
FAIL
FAIL juju 0.132s
FAIL
```

Notice that only one of the tests has failed. ad the only difference is the length of `lifeIDs`

Contributor guide

Open the contributing guide

Research direction

Start by running the provided Go reproduction and compare TestTestSingular with TestTestMultiple, then trace how the SQL query expands ($ids[:]) before execution. Identify the query-processing entry point responsible for the differing behavior and add regression coverage for both one-value and multiple-value inputs. Done means the two cases behave consistently without the current silent false positive or row-value error.

Written by the indexing model from the issue text.

Assessment

Tech stack
go, sql, sqlite
Domain
databases
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
45/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.