Skip to content
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

Sort params named_matches by len #60

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

theraspb3rry
Copy link

This searches for larger (more specific) matches first so that for example gameid and game get matched independently. I found this operated differently from the sqlite3 library, here's some test code.

import pyrqlite.dbapi2 as dbapi2

connection = dbapi2.connect(host="localhost", port=4001)
cursor = connection.cursor()
values = {
    "id": 42069,
    "round": 2,
    "roundname": "Round 2"
}
cursor.execute("CREATE TABLE IF NOT EXISTS foo ( id integer not null primary key, round integer not null, roundname text not null)")

cursor.execute("INSERT INTO foo VALUES(:id, :round, :roundname)", values)

Without the sort, it replaces both round strings first, without giving the chance of replacing roundname to its correct value.

I also added _ to regex to match for params specified as match_id, and from looking at the sqlite docs it seems like table names have a very loose definition for what passes as a table name (with quotes apparently its free game). Not sure how you'd want to handle this one, but from what I remember the regular sqlite3 library did handle the underscores without issue.

Please let me know if you'd like me to modify anything or add a test to catch these in the future. Cheers!

This searches for larger (more specific) matches first so that
for example matchid and match get matched independently.

Added _ to regex to match for params specified as match_id
@@ -97,7 +97,7 @@ def _substitute_params(self, operation, parameters):
param_matches = 0

qmark_re = re.compile(r"(\?)")
named_re = re.compile(r"(:{1}[a-zA-Z]+?\b)")
named_re = re.compile(r"(:{1}[a-zA-Z_]+?\b)")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we could also support $ since I found this:
https://github.com/sqlite/sqlite/blob/master/test/e_expr.test#L546

Identifiers in SQLite consist of alphanumeric, '_' and '$' characters

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also we could possibly support @ instead of colon:
https://github.com/sqlite/sqlite/blob/master/test/e_expr.test#L559

hasn't been properly implemeneted yet
@theraspb3rry
Copy link
Author

Actually after doing a little bit more thinking I think it makes more sense to actually refactor this part so we pass the parameters straight to rqlite so we can utilize the existing parametrization api, I assume when this was written rqlite didn't support this yet.

I'll have more of a play to see if I can get this to the point where I can pass all existing tests as long as this makes sense @zmedico ?

@zmedico
Copy link
Collaborator

zmedico commented Apr 1, 2024

Actually after doing a little bit more thinking I think it makes more sense to actually refactor this part so we pass the parameters straight to rqlite so we can utilize the existing parametrization api, I assume when this was written rqlite didn't support this yet.

Yes, this was introduced in rqlite/rqlite#673 and it would be great to use it.

I'll have more of a play to see if I can get this to the point where I can pass all existing tests as long as this makes sense @zmedico ?

Yes, please do!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants