cgsqlite,.: fix regression with empty strings#122
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a regression where empty strings were incorrectly treated as NULL values when binding parameters to SQLite statements. The issue was caused by unsafe.StringData("") potentially returning nil, which SQLite interpreted as NULL.
Key Changes:
- Fixed the empty string handling in
cgosqlite/cgosqlite.goby using a backing array to guarantee a non-nil pointer - Added comprehensive regression test
TestEmptyStringNotNullto verify empty strings are properly distinguished from NULL values
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| cgosqlite/cgosqlite.go | Replaced unsafe.StringData("") with a backing array approach to ensure empty strings have a non-nil pointer representation |
| sqlite_test.go | Added comprehensive test verifying empty strings are stored and retrieved correctly, distinct from NULL values |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
7a3a7ec to
80d0e2b
Compare
|
What was the point of that unreviewed change anyway? What was wrong with bind_text64_empty? |
I noticed the comment over the functions was wrong, and realized they no longer had a good reason to be. It should have been a no-op, but the stringdata behavior caught me, and that's my bad. |
A final step refactor in 81bdfd0 introduced a bug where empty strings became NULL in some code paths. This fixes the regression and introduces a test to prevent reoccurrence. Updates tailscale/corp#9199
80d0e2b to
cc162a8
Compare
A final step refactor in 81bdfd0 introduced a bug where empty strings became NULL in some code paths.
This fixes the regression and introduces a test to prevent reoccurrence.
Updates tailscale/corp#9199