Use params API #72

joshiggins · 2025-08-04T12:24:56Z

This PR makes pyrqlite use the params API instead of client side string substitution.

It fixes an issue where placeholder characters could not appear inside string literals in the SQL statement.

It also potentially improves security by allowing the server to prepare and bind the parameters.

These changes should not break existing use cases for pyrqlite.

… client; rewrite original substition function to only check enough params are provided

…types are already serialised by json.dumps in an acceptable way and specific adapters have been removed

…dict is provided as parameters

…instead of the raw JSON item

…ms argument

Copilot

Pull Request Overview

This PR migrates pyrqlite from client-side string substitution to using the rqlite params API for parameter binding. This change improves security by allowing server-side parameter preparation and binding, and fixes issues where placeholder characters could appear inside string literals.

Refactored parameter handling to use JSON-based parameter passing instead of string substitution
Updated adapter functions to prepare Python values for JSON serialization rather than string formatting
Enhanced parameter validation with proper string literal parsing to avoid false placeholder matches

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File	Description
src/test/test_dbapi.py	Removes expected failure decorator and adds iterator methods to test parameter sequence handling
src/pyrqlite/extensions.py	Updates adapters to prepare values for JSON serialization instead of string formatting
src/pyrqlite/cursors.py	Replaces string substitution with params API, adds string literal parsing and parameter validation

src/pyrqlite/cursors.py

Copilot · 2025-08-04T12:25:23Z

src/test/test_dbapi.py

                assert x == 0
+                if x >= self.__len__():


This bounds check will never trigger because it's placed after assert x == 0. The assertion will fail for any x >= 1, making the IndexError check unreachable. Consider removing the assertion or restructuring the logic.

Suggested change

assert x == 0

if x >= self.__len__():

if x < 0 or x >= self.__len__():

Removed redundant bounds check

… test object was modified to be iterable but still with a single item

otoolep · 2025-08-07T02:53:14Z

Hi @joshiggins -- this ready for review?

cc @zmedico

joshiggins · 2025-08-11T12:27:46Z

Yes ready for review, thanks

otoolep

Is there a unit test you can add that would fail before your change, but now passes?

otoolep · 2025-08-11T17:09:31Z

src/test/test_dbapi.py

+
            def __getitem__(self, x):
-                assert x == 0
+                assert x == 0 


Trailing whitespace -- please remove.

…t interpreted as parameter placeholders

joshiggins · 2025-08-12T19:41:10Z

@otoolep added 2 tests to check that qmark and colon characters appearing inside string literals in the SQL statement are not interpreted as parameter placeholders.

Failure message before this change:

FAILED src/test/test_dbapi.py::CursorTests::test_CheckExecuteWithColonInString - sqlite3.ProgrammingError: parameter required but not given: create table testc(id integer primary key, name text ...
FAILED src/test/test_dbapi.py::CursorTests::test_CheckExecuteWithQmarkInString - sqlite3.ProgrammingError: parameter required but not given: create table testq(id integer primary key, name text ...

Also I enabled the test test_CheckExecuteArgStringWithZeroByte which was an expected failure before but is passing now.

It looked like previously the zero byte was treated somewhere along the line as a string terminator and rqlite ends up getting a truncated SQL statement. When it's a bound parameter this test passes since the 5 characters (one of them being the zero byte) are stored and returned as expected.

Failure message before this change:

FAILED src/test/test_dbapi.py::CursorTests::test_CheckExecuteArgStringWithZeroByte - sqlite3.Error: {"error": "unrecognized token: \"'Hu\""}

Finally I enabled test_CheckUnsupportedDict as there are other tests for named params support and this one seems good to have.

otoolep · 2025-08-14T23:14:52Z

Just so we're clear, can you tell me what you mean by "client side substitution"? Are you saying this library would rewrite, say, ? with actual values? I have not studied this library closely, as it was written by others. If so, perhaps this was probably before rqlite support proper parameterized queries.

otoolep · 2025-08-14T23:15:59Z

rqlite API and parameters: https://rqlite.io/docs/api/api/#parameterized-statements

Surely there is no need to manipulate SQL statements strings. It's up to users to get them right, no?

joshiggins · 2025-08-15T10:08:53Z

Are you saying this library would rewrite, say, ? with actual values?

Yes, exactly. The original method is here

pyrqlite/src/pyrqlite/cursors.py

Line 91 in 46c2c95

def _substitute_params(self, operation, parameters):

otoolep · 2025-08-19T17:50:05Z

src/pyrqlite/cursors.py

        '''
-        SQLite natively supports only the types TEXT, INTEGER, REAL, BLOB and
-        NULL
+        This function removes string literals from the SQL operation so we


@joshiggins -- this is what I don't understand then. Why modify the SQL string at all? Should it not be supplied in the SQL strings place holder in the Params API? The SQLite code will take care of it.

Oh, I see, I had the wrong mental model for this change (I'm not super familiar with this library). This is about breaking apart the SQL entered by the client library so the API call to rqlite can be made correctly. Let me take a look now.

otoolep · 2025-08-21T09:29:32Z

@joshiggins -- I've got a question for you. This change appears to be checking the parameter count within the SQL to the parameters supplied by the user. Why do that? Why not just push the stuff into the rqlite API as it, and let it return errors?

What I mean is look at the db2 docs, an example:

https://docs.python.org/3/library/sqlite3.html#sqlite3.Cursor.execute

If I was to code a client library I would just take the SQL string, shove that in the HTTP API request to rqlite, and then look at the type of parameters. If it's a list form the HTTP API request one way, if it's a dict, for the HTTP API request another way.

This library was probably like this already, but I don't see any point in checking that the user has supplied the right number of parameters for the number of '?' or named params (as a dictionary) in the SQL query. rqlite will check all that, and return an error to the user. if there is an error Is there a good reason to also do the checking in a client library like this one? It could be brittle. By simply focusing on building the HTTP API request and sending that the rqlite, rqlite will do the checking for you in a bullet-proof manner (since rqlite in turn will use its copy of SQLite to check).

zmedico · 2025-09-23T03:10:04Z

src/pyrqlite/cursors.py

+            statements = json.dumps([self._get_operation_with_params(operation, parameters)])
+        except TypeError as e:
+            raise InterfaceError(e)



This could catch an unexpected TypeError, so I would prefer that _get_operation_with_params internally converted TypeError to InterfaceError if needed.

A TypeError from json.dumps can be handled separately like this:

statements = self._get_operation_with_params(operation, parameters) try: statements = json.dumps([statements]) except TypeError as e: raise InterfaceError(e)

zmedico · 2025-09-23T03:10:58Z

src/pyrqlite/cursors.py

+            try:
+                statements.append(self._get_operation_with_params(operation, parameters))
+            except TypeError as e:
+                raise InterfaceError(e)


This could catch an unexpected TypeError, so I would prefer that _get_operation_with_params internally converted TypeError to InterfaceError if needed.

There's a json.dumps(statements) call later in this function, and we want to do the TypeError to InterfaceError conversion for that.

zmedico · 2025-10-17T02:54:35Z

src/pyrqlite/extensions.py

+def _adapt_bytes(value):
+    # Use byte array for the params API
+    if not isinstance(value, bytes):
+        value = value.encode('utf-8')


Can't we safely omit this isinstance check because _adapt_from_python will only pass in bytes type here?

joshiggins added 7 commits August 4, 2025 11:55

make the test sequence like object L() iterable

4b595ff

send adapted parameters to rqlite instead of substituting them in the…

fd57e70

… client; rewrite original substition function to only check enough params are provided

update adapters to produce values suitable for the rqlite JSON; many …

f3e61cc

…types are already serialised by json.dumps in an acceptable way and specific adapters have been removed

test_CheckExecuteArgStringWithZeroByte is now passing

a5007c8

improve clarity of comment around checking named param when a default…

3c1e3f8

…dict is provided as parameters

improve clarity of error message from the API showing the error text …

d44dedd

…instead of the raw JSON item

simplify control flow of _resolve_named_params by removing named_para…

35f4124

…ms argument

Copilot AI review requested due to automatic review settings August 4, 2025 12:24

Copilot AI reviewed Aug 4, 2025

View reviewed changes

joshiggins added 2 commits August 4, 2025 13:34

remove redundant assert from test_CheckExecuteParamSequence since the…

a1ae708

… test object was modified to be iterable but still with a single item

revert last commit and remove redundant bounds check instead

9f9937d

otoolep reviewed Aug 11, 2025

View reviewed changes

src/test/test_dbapi.py Outdated

def __getitem__(self, x):

assert x == 0

assert x == 0

Copy link

Member

otoolep Aug 11, 2025

Choose a reason for hiding this comment

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

Trailing whitespace -- please remove.

joshiggins added 3 commits August 12, 2025 19:41

remove trailing whitespace

b6e2a57

add tests to check that qmark and colon inside string literals are no…

dac03cf

…t interpreted as parameter placeholders

enable test_CheckUnsupportedDict as named params are working

4cd934d

otoolep reviewed Aug 19, 2025

View reviewed changes

zmedico requested changes Sep 23, 2025

View reviewed changes

zmedico reviewed Oct 17, 2025

View reviewed changes

	assert x == 0
	if x >= self.__len__():
	if x < 0 or x >= self.__len__():

Use params API #72

Are you sure you want to change the base?

Use params API #72

Uh oh!

Conversation

joshiggins commented Aug 4, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Copilot AI Aug 4, 2025

Choose a reason for hiding this comment

Uh oh!

joshiggins Aug 4, 2025

Choose a reason for hiding this comment

Uh oh!

otoolep commented Aug 7, 2025

Uh oh!

joshiggins commented Aug 11, 2025

Uh oh!

otoolep left a comment

Choose a reason for hiding this comment

Uh oh!

otoolep Aug 11, 2025

Choose a reason for hiding this comment

Uh oh!

joshiggins commented Aug 12, 2025

Uh oh!

otoolep commented Aug 14, 2025

Uh oh!

otoolep commented Aug 14, 2025

Uh oh!

joshiggins commented Aug 15, 2025

Uh oh!

otoolep Aug 19, 2025

Choose a reason for hiding this comment

Uh oh!

otoolep Aug 21, 2025

Choose a reason for hiding this comment

Uh oh!

otoolep commented Aug 21, 2025

Uh oh!

zmedico Sep 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zmedico Sep 23, 2025

Choose a reason for hiding this comment

Uh oh!

zmedico Oct 17, 2025

Choose a reason for hiding this comment

Uh oh!

zmedico Oct 17, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

zmedico Sep 23, 2025 •

edited

Loading