-
Notifications
You must be signed in to change notification settings - Fork 6
fix(db): rewrite ILIKE for SQLite, and stop tracking database sidecars #543
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,4 +4,7 @@ build | |
| .idea | ||
| config/config.yaml/cli | ||
| config/* | ||
| *.db | ||
| *.db | ||
| *.db-shm | ||
| *.db-wal | ||
| *.db-journal | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,215 @@ | ||
| package postgres | ||
|
|
||
| import ( | ||
| "context" | ||
| "database/sql" | ||
| "database/sql/driver" | ||
| "fmt" | ||
| "strings" | ||
| "sync" | ||
| ) | ||
|
|
||
| // SQLite has no ILIKE operator, and unlike a missing function it cannot be | ||
| // polyfilled: ILIKE is a parser keyword, so SQLite rejects the statement before | ||
| // any user-defined function could run. | ||
| // | ||
| // The operator appears in roughly fifty places, but almost none of them can be | ||
| // fixed at the call site. buncolgen and querybuilder precompute their SQL | ||
| // fragments into package-level values at init, long before configuration is | ||
| // read, and most of buncolgen is generated. Rewriting the statement as it | ||
| // reaches the driver is therefore the only single point that covers all of them. | ||
| // | ||
| // The substitution is sound because SQLite's LIKE is already case-insensitive | ||
| // for ASCII, which is what ILIKE is used for here. It is not equivalent for | ||
| // non-ASCII text, and that is one more reason SQLite is development-only. | ||
| const sqliteRewriteDriverName = "sqlite-trenova" | ||
|
|
||
| var registerSQLiteRewriteDriver sync.Once | ||
|
|
||
| // rewriteILike replaces the ILIKE operator with LIKE outside string literals and | ||
| // quoted identifiers, so a value or column name containing the word is untouched. | ||
| func rewriteILike(query string) string { | ||
| if !containsFold(query, "ilike") { | ||
| return query | ||
| } | ||
|
|
||
| var ( | ||
| out strings.Builder | ||
| inSingle bool | ||
| inDouble bool | ||
| i int | ||
| ) | ||
|
|
||
| out.Grow(len(query)) | ||
|
|
||
| for i < len(query) { | ||
| ch := query[i] | ||
|
|
||
| switch { | ||
| case inSingle: | ||
| if ch == '\'' { | ||
| inSingle = false | ||
| } | ||
| case inDouble: | ||
| if ch == '"' { | ||
| inDouble = false | ||
| } | ||
| case ch == '\'': | ||
| inSingle = true | ||
| case ch == '"': | ||
| inDouble = true | ||
| case (ch == 'i' || ch == 'I') && isILikeAt(query, i): | ||
| out.WriteString("LIKE") | ||
| i += len("ILIKE") | ||
| continue | ||
|
Comment on lines
+48
to
+64
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win 🧩 Analysis chain🌐 Web query:
💡 Result: In SQLite, there are four ways to quote an identifier [1][2]: - Double quotes ("identifier"): This is the standard SQL way to quote identifiers [3]. - Square brackets ([identifier]): Included for compatibility with MS Access and SQL Server [1][2]. - Grave accents ( Citations:
🏁 Script executed: #!/bin/bash
set -eu
file="services/tms/internal/infrastructure/postgres/sqlitedriver.go"
printf '%s\n' '--- file outline ---'
ast-grep outline "$file" || true
printf '%s\n' '--- relevant source ---'
sed -n '1,180p' "$file"
printf '%s\n' '--- related tests and usages ---'
rg -n -C 3 'rewriteILike|isILikeAt|containsFold|ILIKE|sqlitedriver' services/tms
printf '%s\n' '--- repository status ---'
git diff --statRepository: emoss08/Trenova Length of output: 50371 🏁 Script executed: #!/bin/bash
set -eu
python3 - <<'PY'
import sqlite3
def is_identifier_byte(b):
return b == "_" or 97 <= b <= 122 or 65 <= b <= 90 or 48 <= b <= 57
def is_ilike_at(query, i):
keyword = "ilike"
if i + len(keyword) > len(query):
return False
if query[i:i+len(keyword)].lower() != keyword:
return False
if i > 0 and is_identifier_byte(ord(query[i-1])):
return False
end = i + len(keyword)
return end == len(query) or not is_identifier_byte(ord(query[end]))
def rewrite_ilike(query):
if "ilike" not in query.lower():
return query
out = []
in_single = False
in_double = False
i = 0
while i < len(query):
ch = query[i]
if in_single:
if ch == "'":
in_single = False
elif in_double:
if ch == '"':
in_double = False
elif ch == "'":
in_single = True
elif ch == '"':
in_double = True
elif ch in "iI" and is_ilike_at(query, i):
out.append("LIKE")
i += len("ILIKE")
continue
out.append(ch)
i += 1
return "".join(out)
cases = [
"SELECT `ILIKE` FROM t",
"SELECT [ILIKE] FROM t",
"SELECT \"ILIKE\" FROM t",
"SELECT foo$ILIKE FROM t",
"SELECT $ILIKE FROM t",
"SELECT foo_ILIKE FROM t",
"SELECT name ILIKE ?",
]
for query in cases:
print(f"{query!r} -> {rewrite_ilike(query)!r}")
print("--- SQLite token/parse probes ---")
db = sqlite3.connect(":memory:")
db.execute("CREATE TABLE t (`ILIKE` TEXT, [ILIKE] TEXT, foo$ILIKE TEXT)")
for query, params in [
("SELECT `ILIKE` FROM t", ()),
("SELECT [ILIKE] FROM t", ()),
("SELECT foo$ILIKE FROM t", ()),
("SELECT $ILIKE", {"ILIKE": "value"}),
]:
try:
db.execute(query, params)
print(f"accepted: {query!r}")
except Exception as exc:
print(f"rejected: {query!r}: {exc}")
PYRepository: emoss08/Trenova Length of output: 669 🏁 Script executed: #!/bin/bash
set -eu
python3 - <<'PY'
import sqlite3
def is_identifier_byte(b):
return b == "_" or 97 <= b <= 122 or 65 <= b <= 90 or 48 <= b <= 57
def is_ilike_at(query, i):
keyword = "ilike"
if i + len(keyword) > len(query):
return False
if query[i:i+len(keyword)].lower() != keyword:
return False
if i > 0 and is_identifier_byte(ord(query[i-1])):
return False
end = i + len(keyword)
return end == len(query) or not is_identifier_byte(ord(query[end]))
def rewrite_ilike(query):
if "ilike" not in query.lower():
return query
out = []
in_single = False
in_double = False
i = 0
while i < len(query):
ch = query[i]
if in_single:
if ch == "'":
in_single = False
elif in_double:
if ch == '"':
in_double = False
elif ch == "'":
in_single = True
elif ch == '"':
in_double = True
elif ch in "iI" and is_ilike_at(query, i):
out.append("LIKE")
i += len("ILIKE")
continue
out.append(ch)
i += 1
return "".join(out)
cases = [
"SELECT `ILIKE` FROM t",
"SELECT [ILIKE] FROM t",
'SELECT "ILIKE" FROM t',
"SELECT foo$ILIKE FROM t",
"SELECT $ILIKE FROM t",
"SELECT foo_ILIKE FROM t",
"SELECT name ILIKE ?",
]
for query in cases:
print(f"{query!r} -> {rewrite_ilike(query)!r}")
print("--- SQLite token/parse probes ---")
db = sqlite3.connect(":memory:")
db.execute("CREATE TABLE t (`ILIKE` TEXT, [ILIKE] TEXT, foo$ILIKE TEXT)")
for query, params in [
("SELECT `ILIKE` FROM t", ()),
("SELECT [ILIKE] FROM t", ()),
("SELECT foo$ILIKE FROM t", ()),
("SELECT $ILIKE", {"ILIKE": "value"}),
]:
try:
db.execute(query, params)
print(f"accepted: {query!r}")
except Exception as exc:
print(f"rejected: {query!r}: {exc}")
PYRepository: emoss08/Trenova Length of output: 669 🏁 Script executed: #!/bin/bash
set -eu
python3 - <<'PY'
import sqlite3
def probe(schema, query, params=()):
db = sqlite3.connect(":memory:")
try:
db.execute(schema)
db.execute(query, params)
print(f"accepted: {query!r}")
except Exception as exc:
print(f"rejected: {query!r}: {exc}")
probe("CREATE TABLE t (`ILIKE` TEXT)", "SELECT `ILIKE` FROM t")
probe("CREATE TABLE t ([ILIKE] TEXT)", "SELECT [ILIKE] FROM t")
probe("CREATE TABLE t (foo$ILIKE TEXT)", "SELECT foo$ILIKE FROM t")
probe("SELECT 1", "SELECT $ILIKE", {"ILIKE": "value"})
probe("SELECT 1", "SELECT $ILIKE", {"ILIKE": 1})
PYRepository: emoss08/Trenova Length of output: 309 Protect all SQLite identifier forms before rewriting.
🤖 Prompt for AI Agents |
||
| } | ||
|
|
||
| out.WriteByte(ch) | ||
| i++ | ||
| } | ||
|
|
||
| return out.String() | ||
| } | ||
|
|
||
| func isILikeAt(query string, i int) bool { | ||
| const keyword = "ilike" | ||
|
|
||
| if i+len(keyword) > len(query) { | ||
| return false | ||
| } | ||
|
|
||
| if !strings.EqualFold(query[i:i+len(keyword)], keyword) { | ||
| return false | ||
| } | ||
|
|
||
| if i > 0 && isIdentifierByte(query[i-1]) { | ||
| return false | ||
| } | ||
|
|
||
| end := i + len(keyword) | ||
|
|
||
| return end == len(query) || !isIdentifierByte(query[end]) | ||
| } | ||
|
|
||
| func isIdentifierByte(b byte) bool { | ||
| return b == '_' || | ||
| (b >= 'a' && b <= 'z') || | ||
| (b >= 'A' && b <= 'Z') || | ||
| (b >= '0' && b <= '9') | ||
| } | ||
|
|
||
| func containsFold(haystack, needle string) bool { | ||
| return strings.Contains(strings.ToLower(haystack), needle) | ||
| } | ||
|
|
||
| // openSQLiteDB returns a database handle whose statements pass through | ||
| // rewriteILike on the way to modernc.org/sqlite. | ||
| func openSQLiteDB(dsn string) (*sql.DB, error) { | ||
| base, err := sql.Open(sqliteDriverName, dsn) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to open sqlite database: %w", err) | ||
| } | ||
|
|
||
| inner := base.Driver() | ||
| if closeErr := base.Close(); closeErr != nil { | ||
| return nil, fmt.Errorf("failed to close probe sqlite handle: %w", closeErr) | ||
| } | ||
|
|
||
| registerSQLiteRewriteDriver.Do(func() { | ||
| sql.Register(sqliteRewriteDriverName, &rewriteDriver{inner: inner}) | ||
| }) | ||
|
|
||
| return sql.OpenDB(&rewriteConnector{dsn: dsn, inner: inner}), nil | ||
| } | ||
|
|
||
| type rewriteDriver struct { | ||
| inner driver.Driver | ||
| } | ||
|
|
||
| func (d *rewriteDriver) Open(name string) (driver.Conn, error) { | ||
| conn, err := d.inner.Open(name) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return &rewriteConn{inner: conn}, nil | ||
| } | ||
|
|
||
| type rewriteConnector struct { | ||
| dsn string | ||
| inner driver.Driver | ||
| } | ||
|
|
||
| func (c *rewriteConnector) Connect(_ context.Context) (driver.Conn, error) { | ||
| conn, err := c.inner.Open(c.dsn) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return &rewriteConn{inner: conn}, nil | ||
| } | ||
|
|
||
| func (c *rewriteConnector) Driver() driver.Driver { | ||
| return &rewriteDriver{inner: c.inner} | ||
| } | ||
|
|
||
| // rewriteConn deliberately implements only the prepare path. database/sql falls | ||
| // back to Prepare when a connection does not advertise QueryerContext or | ||
| // ExecerContext, which keeps every statement going through the rewrite rather | ||
| // than slipping past it on a fast path. | ||
| type rewriteConn struct { | ||
| inner driver.Conn | ||
| } | ||
|
|
||
| func (c *rewriteConn) Prepare(query string) (driver.Stmt, error) { | ||
| return c.inner.Prepare(rewriteILike(query)) | ||
| } | ||
|
|
||
| func (c *rewriteConn) PrepareContext( | ||
| ctx context.Context, | ||
| query string, | ||
| ) (driver.Stmt, error) { | ||
| if preparer, ok := c.inner.(driver.ConnPrepareContext); ok { | ||
| return preparer.PrepareContext(ctx, rewriteILike(query)) | ||
| } | ||
|
|
||
| return c.inner.Prepare(rewriteILike(query)) | ||
| } | ||
|
|
||
| func (c *rewriteConn) Close() error { return c.inner.Close() } | ||
|
|
||
| func (c *rewriteConn) Begin() (driver.Tx, error) { //nolint:staticcheck // required by driver.Conn | ||
| return c.inner.Begin() //nolint:staticcheck // delegating to the wrapped driver | ||
| } | ||
|
|
||
| func (c *rewriteConn) BeginTx(ctx context.Context, opts driver.TxOptions) (driver.Tx, error) { | ||
| if beginner, ok := c.inner.(driver.ConnBeginTx); ok { | ||
| return beginner.BeginTx(ctx, opts) | ||
| } | ||
|
|
||
| return c.inner.Begin() //nolint:staticcheck // fallback for drivers without BeginTx | ||
| } | ||
|
|
||
| func (c *rewriteConn) Ping(ctx context.Context) error { | ||
| if pinger, ok := c.inner.(driver.Pinger); ok { | ||
| return pinger.Ping(ctx) | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| func (c *rewriteConn) ResetSession(ctx context.Context) error { | ||
| if resetter, ok := c.inner.(driver.SessionResetter); ok { | ||
| return resetter.ResetSession(ctx) | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| func (c *rewriteConn) IsValid() bool { | ||
| if validator, ok := c.inner.(driver.Validator); ok { | ||
| return validator.IsValid() | ||
| } | ||
|
|
||
| return true | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,120 @@ | ||
| package postgres | ||
|
|
||
| import ( | ||
| "path/filepath" | ||
| "testing" | ||
|
|
||
| "github.com/stretchr/testify/assert" | ||
| "github.com/stretchr/testify/require" | ||
|
Comment on lines
+7
to
+8
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win Use the Go standard testing package. Replace Testify assertions with As per coding guidelines, “Use Go's standard testing package for Go tests.” Also applies to: 61-62, 71-72, 81-92, 100-101, 110-119 🤖 Prompt for AI AgentsSource: Coding guidelines |
||
| ) | ||
|
|
||
| func TestRewriteILike(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| query string | ||
| want string | ||
| }{ | ||
| { | ||
| name: "leaves a query without the operator alone", | ||
| query: `SELECT * FROM t WHERE a = ?`, | ||
| want: `SELECT * FROM t WHERE a = ?`, | ||
| }, | ||
| { | ||
| name: "rewrites the operator", | ||
| query: `SELECT * FROM t WHERE name ILIKE ?`, | ||
| want: `SELECT * FROM t WHERE name LIKE ?`, | ||
| }, | ||
| { | ||
| name: "rewrites regardless of case", | ||
| query: `SELECT * FROM t WHERE name iLiKe ?`, | ||
| want: `SELECT * FROM t WHERE name LIKE ?`, | ||
| }, | ||
| { | ||
| name: "rewrites NOT ILIKE", | ||
| query: `SELECT * FROM t WHERE name NOT ILIKE ?`, | ||
| want: `SELECT * FROM t WHERE name NOT LIKE ?`, | ||
| }, | ||
| { | ||
| name: "rewrites every occurrence", | ||
| query: `SELECT * FROM t WHERE a ILIKE ? OR b ILIKE ?`, | ||
| want: `SELECT * FROM t WHERE a LIKE ? OR b LIKE ?`, | ||
| }, | ||
| { | ||
| name: "leaves string literals untouched", | ||
| query: `SELECT * FROM t WHERE note = ' ILIKE ' AND name ILIKE ?`, | ||
| want: `SELECT * FROM t WHERE note = ' ILIKE ' AND name LIKE ?`, | ||
| }, | ||
| { | ||
| name: "leaves quoted identifiers untouched", | ||
| query: `SELECT "ilike" FROM t WHERE name ILIKE ?`, | ||
| want: `SELECT "ilike" FROM t WHERE name LIKE ?`, | ||
| }, | ||
| { | ||
| name: "does not touch a column whose name merely contains the word", | ||
| query: `SELECT * FROM t WHERE ilike_count > ? AND unilike > ?`, | ||
| want: `SELECT * FROM t WHERE ilike_count > ? AND unilike > ?`, | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| assert.Equal(t, tt.want, rewriteILike(tt.query)) | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func TestSQLiteDriverExecutesILike(t *testing.T) { | ||
| ctx := t.Context() | ||
| path := filepath.Join(t.TempDir(), "ilike.db") | ||
|
|
||
| db, err := openSQLiteDB("file:" + path) | ||
| require.NoError(t, err) | ||
| t.Cleanup(func() { _ = db.Close() }) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win Handle Both callbacks discard a possible close error. Report the error with
As per coding guidelines, “Handle every error path explicitly; never swallow errors.” 📍 Affects 1 file
🤖 Prompt for AI AgentsSource: Coding guidelines |
||
|
|
||
| _, err = db.ExecContext(ctx, `CREATE TABLE people (name TEXT)`) | ||
| require.NoError(t, err) | ||
|
|
||
| _, err = db.ExecContext(ctx, `INSERT INTO people (name) VALUES ('Alice'), ('bob')`) | ||
| require.NoError(t, err) | ||
|
|
||
| var name string | ||
| require.NoError( | ||
| t, | ||
| db.QueryRowContext(ctx, `SELECT name FROM people WHERE name ILIKE ?`, "alice").Scan(&name), | ||
| "ILIKE must survive the rewrite and execute on SQLite", | ||
| ) | ||
| assert.Equal(t, "Alice", name, "ILIKE must stay case-insensitive after the rewrite") | ||
|
|
||
| require.NoError( | ||
| t, | ||
| db.QueryRowContext(ctx, `SELECT name FROM people WHERE name ILIKE ?`, "BOB").Scan(&name), | ||
| ) | ||
| assert.Equal(t, "bob", name) | ||
| } | ||
|
|
||
| func TestSQLiteDriverPreservesLiteralContainingILike(t *testing.T) { | ||
| ctx := t.Context() | ||
| path := filepath.Join(t.TempDir(), "literal.db") | ||
|
|
||
| db, err := openSQLiteDB("file:" + path) | ||
| require.NoError(t, err) | ||
| t.Cleanup(func() { _ = db.Close() }) | ||
|
|
||
| _, err = db.ExecContext(ctx, `CREATE TABLE notes (body TEXT)`) | ||
| require.NoError(t, err) | ||
|
|
||
| _, err = db.ExecContext(ctx, `INSERT INTO notes (body) VALUES ('uses ILIKE here')`) | ||
| require.NoError(t, err) | ||
|
|
||
| var body string | ||
| require.NoError( | ||
| t, | ||
| db.QueryRowContext(ctx, `SELECT body FROM notes WHERE body ILIKE ?`, "%ilike%").Scan(&body), | ||
| ) | ||
| assert.Equal( | ||
| t, | ||
| "uses ILIKE here", | ||
| body, | ||
| "a stored value containing ILIKE must not be rewritten", | ||
| ) | ||
| } | ||
|
Comment on lines
+66
to
+120
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win Tag-gate the SQLite driver integration tests. These tests create a SQLite database and execute through the wrapped driver. Move them to a separate As per coding guidelines, “Integration tests must be tag-gated with the integration build tag.” 🤖 Prompt for AI AgentsSource: Coding guidelines |
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Remove the added explanatory Go comments.
The applicable rule prohibits added Go comments. Keep only required linter directives.
As per coding guidelines, “Do not add comments to Go code.”
Also applies to: 29-30, 105-106, 156-159
🤖 Prompt for AI Agents
Source: Coding guidelines