Skip to content

refactor(sqlite): do not borrow bound values, delete lifetime on SqliteArguments #3957

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

Merged
merged 5 commits into from
Aug 19, 2025

Conversation

iamjpotts
Copy link
Contributor

@iamjpotts iamjpotts commented Aug 1, 2025

Fixes an inconsistency with allowed usage of references as arguments to query macros in the context of the sqlite feature compared to features for other db platforms, such as Postgres. The inconsistency is demonstrated in the first commit - this commit adds a test which would fail to compile without the remaining commits in the pr.

Compare the test added with this pr vs the existing test by the same name for Postgres:

The existing test for Postgres compiles and passes because PgArguments does not have a lifetime parameter. The new test for sqlite would fail to compile because SqliteArguments had a lifetime parameter which causes the return value of the query! macro to be borrowing a reference to a value that is an argument to query! but goes out of scope when query! returns.

Does your PR solve an issue?

Yes - cannot pass a reference as a bind argument to query! unless the reference is established before the query! macro call (e.g. by assigning it to a variable with let).

Is this a breaking change?

Yes, due to:

  • Deletion of lifetime parameter on SqliteArguments and SqliteArgumentValue
  • Deletion of into_static() on both of those types
  • Introduction of SqliteArgumentsBuffer type to wrap Vec<SqliteArgumentValue>
  • Changing SqliteArgumentValue::Text to store an Arc<String> instead of Cow<str>, and adding ::TextSlice for the Arc<str> variant

Commits

The last commit (changing AnyArguments::convert_to to convert_into) could be moved out of this pr and into a new one focused on removing the lifetime arguments on AnyArguments and AnyArgumentValue to add similar support for references in the bind parameters when using the Any driver.

@iamjpotts iamjpotts changed the title Jp/sqlite arg lifetime bug(sqlite): query macro argument lifetime use inconsistent with other db platforms Aug 1, 2025
@iamjpotts iamjpotts changed the title bug(sqlite): query macro argument lifetime use inconsistent with other db platforms fix(sqlite): query macro argument lifetime use inconsistent with other db platforms Aug 1, 2025
@iamjpotts iamjpotts force-pushed the jp/sqlite-arg-lifetime branch from e9f5f5c to 8bbbf1c Compare August 1, 2025 23:29
@iamjpotts iamjpotts marked this pull request as ready for review August 1, 2025 23:40
@iamjpotts iamjpotts force-pushed the jp/sqlite-arg-lifetime branch 2 times, most recently from c3f788b to 73ea968 Compare August 10, 2025 17:01
…arguments by removing lifetime parameter from SqliteArguments

Signed-off-by: Joshua Potts <[email protected]>

Signed-off-by: Joshua Potts <[email protected]>
…ents, and SqliteArgumentsBuffer

Signed-off-by: Joshua Potts <[email protected]>
@iamjpotts iamjpotts force-pushed the jp/sqlite-arg-lifetime branch from 73ea968 to eee73fc Compare August 19, 2025 01:58
Comment on lines +15 to +17
Text(Arc<String>),
TextSlice(Arc<str>),
Blob(Arc<Vec<u8>>),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since we're copying at this level, we could actually avoid a copy later if we tell SQLite the pointer is going to remain valid: https://sqlite.org/c3ref/bind_blob.html

The fifth argument to the BLOB and string binding interfaces controls or indicates the lifetime of the object referenced by the third parameter. These three options exist: (1) A destructor to dispose of the BLOB or string after SQLite has finished with it may be passed. It is called to dispose of the BLOB or string even if the call to the bind API fails, except the destructor is not called if the third parameter is a NULL pointer or the fourth parameter is negative. (2) The special constant, SQLITE_STATIC, may be passed to indicate that the application remains responsible for disposing of the object. In this case, the object and the provided pointer to it must remain valid until either the prepared statement is finalized or the same SQL parameter is bound to something else, whichever occurs sooner. (3) The constant, SQLITE_TRANSIENT, may be passed to indicate that the object is to be copied prior to the return from sqlite3_bind_*(). The object and pointer to it must remain valid until then. SQLite will then manage the lifetime of its private copy.

We currently pass SQLITE_TRANSIENT. Theoretically SQLITE_STATIC should be perfectly fine, though I'm leaning toward passing a destructor and letting SQLite manage the lifetime as that should be less to worry about.

I'm not saying we have to add that here, I just wanted to note this somewhere for posterity.

@abonander abonander added breaking db:sqlite Related to SQLite labels Aug 19, 2025
@abonander abonander changed the title fix(sqlite): query macro argument lifetime use inconsistent with other db platforms refactor(sqlite): do not borrow bound values, delete lifetime on SqliteArguments Aug 19, 2025
@abonander abonander merged commit ff93aa0 into launchbadge:main Aug 19, 2025
104 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking db:sqlite Related to SQLite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants