Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 20 additions & 10 deletions storage/sql.go
Original file line number Diff line number Diff line change
Expand Up @@ -214,8 +214,7 @@ func (s *SQLAdapter) GetContext(ctx context.Context, dest any, filter map[string
if len(filter) == 0 {
return errors.New("filtering is required when getting a resource")
}
query, bindings := s.buildQuery(filter)
result := s.dbWithCtx(ctx).Where(query, bindings).Find(dest)
result := s.applyFilter(s.dbWithCtx(ctx), filter).Find(dest)
if result.RowsAffected == 0 {
return ErrNotFound
}
Expand All @@ -230,8 +229,7 @@ func (s *SQLAdapter) UpdateContext(ctx context.Context, item any, filter map[str
if len(filter) == 0 {
return errors.New("filtering is required when updating a resource")
}
query, bindings := s.buildQuery(filter)
result := s.dbWithCtx(ctx).Where(query, bindings).Save(item)
result := s.applyFilter(s.dbWithCtx(ctx), filter).Save(item)
return result.Error
}

Expand All @@ -243,8 +241,7 @@ func (s *SQLAdapter) DeleteContext(ctx context.Context, item any, filter map[str
if len(filter) == 0 {
return errors.New("filtering is required when deleting a resource")
}
query, bindings := s.buildQuery(filter)
result := s.dbWithCtx(ctx).Where(query, bindings).Delete(item)
result := s.applyFilter(s.dbWithCtx(ctx), filter).Delete(item)
return result.Error
}

Expand Down Expand Up @@ -342,8 +339,7 @@ func (s *SQLAdapter) ListContext(ctx context.Context, dest any, sortKey string,
}
return s.executePaginatedQuery(ctx, dest, sortKey, sortDirection, limit, cursor, func(q *gorm.DB) *gorm.DB {
if len(filter) > 0 {
query, bindings := s.buildQuery(filter)
return q.Where(query, bindings)
return s.applyFilter(q, filter)
}
return q
})
Expand Down Expand Up @@ -402,8 +398,7 @@ func (s *SQLAdapter) CountContext(ctx context.Context, dest any, filter map[stri
q := s.dbWithCtx(ctx).Model(dest)

if len(filter) > 0 {
query, bindings := s.buildQuery(filter)
q = q.Where(query, bindings)
q = s.applyFilter(q, filter)
}

var total int64
Expand All @@ -414,6 +409,21 @@ func (s *SQLAdapter) CountContext(ctx context.Context, dest any, filter map[stri
return total, nil
}

// applyFilter builds a WHERE clause from the filter map and attaches it to
// the given gorm session. When the bindings map is empty (e.g. a filter like
// {"deleted_at": nil} which produces only an "IS NULL" clause) we must NOT
// forward an empty map[string]any{} as a parameter to GORM — pgx5 in
// PreferSimpleProtocol mode cannot encode it and fails with
// "unable to encode map[string]interface{}{} into text format for unknown
// type (OID 0): cannot find encode plan". Pass the query alone instead.
func (s *SQLAdapter) applyFilter(q *gorm.DB, filter map[string]any) *gorm.DB {
query, bindings := s.buildQuery(filter)
if len(bindings) == 0 {
return q.Where(query)
}
return q.Where(query, bindings)
}

func (s *SQLAdapter) Query(dest any, statement string, limit int, cursor string, params ...map[string]any) (string, error) {
return s.QueryContext(context.Background(), dest, statement, limit, cursor, params...)
}
Expand Down
65 changes: 65 additions & 0 deletions storage/sql_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -302,3 +302,68 @@ func TestMemoryAdapterDelegatesNonContextCountAndQuery(t *testing.T) {
t.Fatalf("expected memory.Query to propagate not-implemented error")
}
}

// nilFilterItem has a nullable column so we can build filters whose
// only entry has a nil value, exercising buildQuery's IS NULL branch.
type nilFilterItem struct {
Id string `json:"id" gorm:"primaryKey;column:id"`
DeletedAt *string `json:"deleted_at" gorm:"column:deleted_at"`
}

func (nilFilterItem) TableName() string { return "nil_filter_items" }

// TestSQLAdapterNilOnlyFilterDoesNotForwardEmptyBindings pins the fix
// for the pgx5 simple-protocol crash where buildQuery returned an empty
// map[string]any{} as bindings for nil-only filters and gorm forwarded
// it to pgx as a parameter ("unable to encode map[string]interface{}{}
// into text format for unknown type (OID 0): cannot find encode plan").
//
// applyFilter must call q.Where(query) with NO variadic args when
// bindings are empty. Sqlite tolerates the broken shape, so this test
// exercises behavioral correctness (nil-only filters return the right
// rows) — if a future cleanup reintroduces empty-map forwarding, this
// keeps passing under sqlite but the matching pgx5 path will regress.
// The unit invariant we lock in here is "Count/Get/List with a nil-only
// filter return the expected rows without error".
func TestSQLAdapterNilOnlyFilterDoesNotForwardEmptyBindings(t *testing.T) {
m := storage.GetMemoryAdapterInstance()
if err := m.Execute(`CREATE TABLE IF NOT EXISTS nil_filter_items (id TEXT PRIMARY KEY, deleted_at TEXT)`); err != nil {
t.Fatalf("create table: %v", err)
}
if err := m.Execute(`DELETE FROM nil_filter_items`); err != nil {
t.Fatalf("truncate: %v", err)
}
sql := m.DB

if err := sql.Create(&nilFilterItem{Id: "live"}); err != nil {
t.Fatalf("Create live: %v", err)
}
deletedTs := "2026-01-01"
if err := sql.Create(&nilFilterItem{Id: "gone", DeletedAt: &deletedTs}); err != nil {
t.Fatalf("Create gone: %v", err)
}

total, err := sql.Count(&[]nilFilterItem{}, map[string]any{"deleted_at": nil})
if err != nil {
t.Fatalf("Count with nil-only filter: %v", err)
}
if total != 1 {
t.Fatalf("Count = %d; want 1 (only the non-deleted row)", total)
}

var got nilFilterItem
if err := sql.Get(&got, map[string]any{"deleted_at": nil}); err != nil {
t.Fatalf("Get with nil-only filter: %v", err)
}
if got.Id != "live" {
t.Fatalf("Get id = %q; want %q", got.Id, "live")
}

var listed []nilFilterItem
if _, err := sql.List(&listed, "id", map[string]any{"deleted_at": nil}, 10, ""); err != nil {
t.Fatalf("List with nil-only filter: %v", err)
}
if len(listed) != 1 || listed[0].Id != "live" {
t.Fatalf("List = %+v; want one row id=live", listed)
}
}
Comment on lines +328 to +369

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial | ⚡ Quick win

Extend this regression test to cover Update and Delete paths changed in this PR.

Count/Get/List are covered, but Update/Delete were also switched to applyFilter. Adding one nil-only filter assertion for each will lock in coverage for all touched entrypoints.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@storage/sql_test.go` around lines 328 - 369, Extend
TestSQLAdapterNilOnlyFilterDoesNotForwardEmptyBindings to also assert the Update
and Delete code paths: call sql.Update (targeting e.g. the non-key field or a
dummy field) with filter map[string]any{"deleted_at": nil} and verify only the
"live" row is affected (check the row was updated and the "gone" row remains
unchanged), and call sql.Delete with the same nil-only filter and verify only
the "live" row is removed (confirm Count/List/Get reflect that deletion while
the "gone" row remains). Use the existing nilFilterItem records ("live" and
"gone") and the same sql variable and filter map to locate the code paths for
sql.Update and sql.Delete.