fix(storage/sql): skip empty bindings to GORM Where for nil-only filters#173
Open
Lutherwaves wants to merge 2 commits into
Open
fix(storage/sql): skip empty bindings to GORM Where for nil-only filters#173Lutherwaves wants to merge 2 commits into
Lutherwaves wants to merge 2 commits into
Conversation
Lutherwaves
force-pushed
the
fix/buildquery-empty-bindings-pgx-simple-protocol
branch
from
d304a47 to
79b7488
Compare
Contributor
Author
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe SQL adapter now centralizes filter application in a new internal applyFilter helper that omits empty parameter bindings when calling GORM's Where; GetContext, UpdateContext, DeleteContext, ListContext (filtered), and CountContext (filtered) were updated to use it, and a test verifies nil-only filters don't forward empty bindings. ChangesFilter Parameter Handling Refinement
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with 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.
Inline comments:
In `@storage/sql_test.go`:
- Around line 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.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1fee8c0f-faf4-48aa-9669-c66d24a2b0b3
📒 Files selected for processing (2)
storage/sql.gostorage/sql_test.go
Comment on lines
+328
to
+369
| 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) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 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.
When a filter contains only nil values (e.g. {"deleted_at": nil}),
buildQuery returns the SQL clause "deleted_at IS NULL" plus an empty
map[string]any{} as bindings. Forwarding that empty map as a parameter
to gorm's .Where(query, bindings) is fine on most drivers, but under
pgx5 with PreferSimpleProtocol: true (which is enabled by the default
Postgres setup in OpenConnection) it fails with:
unable to encode map[string]interface{}{} into text format for
unknown type (OID 0): cannot find encode plan
This 500s every Count/Get/List/Update/Delete call where every filter
value happens to be nil — a common shape for "non-deleted rows" counts.
Fix by introducing applyFilter, which forwards bindings only when the
map is non-empty. All five SQLAdapter methods that previously passed
buildQuery's output to .Where now go through it.
No behavior change for filters with at least one non-nil value.
Pins the fix in the previous commit by exercising Count/Get/List with a
nil-only filter ({"deleted_at": nil}). Sqlite tolerates the broken empty-
map shape that pgx5 simple-protocol rejects, so the test asserts the
behavioral contract — nil-only filters return the expected rows — rather
than the wire-level crash. If a future cleanup reintroduces empty
map[string]any{} forwarding to gorm.Where, this test still passes under
sqlite but the matching pgx5 path will regress; the comment block on the
test documents that linkage so the next reader knows what to test against
a real Postgres harness.
Lutherwaves
force-pushed
the
fix/buildquery-empty-bindings-pgx-simple-protocol
branch
from
403465e to
39f53b5
Compare
Lutherwaves
added
bug
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
SQLAdapter.{Get,List,Update,Delete,Count}accept amap[string]anyfilter. When every value in the filter isnil,buildQueryproduces a clause likecol IS NULLplus an emptymap[string]any{}of bindings. That empty map is forwarded to GORM as a parameter to.Where(query, bindings).Under pgx5 with
PreferSimpleProtocol: true(which is whatOpenConnectionconfigures for Postgres), the empty map cannot be encoded:The call returns an error for shapes that should be valid:
Filters with at least one non-nil value (e.g.
{"id": id, "deleted_at": nil}) work fine because the bindings map is non-empty.Fix
Add
SQLAdapter.applyFilter(q, filter)that callsbuildQueryand forwards bindings to.Whereonly when the bindings map is non-empty. The five methods that fedbuildQueryoutput into.Wherenow route through it.No behavior change for filters with at least one non-nil value.
Test plan
go build ./...go vet ./...TestSQLAdapterNilOnlyFilterDoesNotForwardEmptyBindingscoversCount,Get,Listwith nil-only filters. The in-memory sqlite adapter tolerates the broken empty-map shape, so the test asserts behavioral correctness rather than the wire-level crash; the comment block on the test documents what to verify against a real pgx5 simple-protocol harness.Summary by CodeRabbit