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

Canned queries with named parameters fail with error against SQLite 3.46.0 #2353

Closed
simonw opened this issue Jun 11, 2024 · 13 comments
Closed
Labels

Comments

@simonw
Copy link
Owner

simonw commented Jun 11, 2024

Reported on Discord: https://discord.com/channels/823971286308356157/823971286941302908/1248924498128932914

I managed to replicate it locally using this trick https://til.simonwillison.net/sqlite/sqlite-version-macos-python

DYLD_LIBRARY_PATH=$PWD datasette fixtures.db -c datasette.yml --pdb

Where datasette.yml has this:

databases:
  fixtures:
    queries:
      demo: select * from sqlite_master where tbl_name = :table

Then navigate to http://127.0.0.1:8001/fixtures/demo

  File "/opt/homebrew/Caskroom/miniconda/base/lib/python3.10/site-packages/datasette/utils/__init__.py", line 1147, in derive_named_parameters
    return [row["p4"].lstrip(":") for row in results if row["opcode"] == "Variable"]
  File "/opt/homebrew/Caskroom/miniconda/base/lib/python3.10/site-packages/datasette/utils/__init__.py", line 1147, in <listcomp>
    return [row["p4"].lstrip(":") for row in results if row["opcode"] == "Variable"]
AttributeError: 'NoneType' object has no attribute 'lstrip'

Within the --pdb debugger I could see this:

(Pdb) pprint([dict(r) for r in results.rows])
[{'addr': 0,
  'comment': None,
  'opcode': 'Init',
  'p1': 0,
  'p2': 13,
  'p3': 0,
  'p4': None,
  'p5': 0},
 {'addr': 1,
  'comment': None,
  'opcode': 'OpenRead',
  'p1': 0,
  'p2': 1,
  'p3': 0,
  'p4': '5',
  'p5': 0},
 {'addr': 2,
  'comment': None,
  'opcode': 'Rewind',
  'p1': 0,
  'p2': 12,
  'p3': 0,
  'p4': None,
  'p5': 0},
 {'addr': 3,
  'comment': None,
  'opcode': 'Column',
  'p1': 0,
  'p2': 2,
  'p3': 1,
  'p4': None,
  'p5': 0},
 {'addr': 4,
  'comment': None,
  'opcode': 'Ne',
  'p1': 2,
  'p2': 11,
  'p3': 1,
  'p4': 'BINARY-8',
  'p5': 82},
 {'addr': 5,
  'comment': None,
  'opcode': 'Column',
  'p1': 0,
  'p2': 0,
  'p3': 3,
  'p4': None,
  'p5': 0},
 {'addr': 6,
  'comment': None,
  'opcode': 'Column',
  'p1': 0,
  'p2': 1,
  'p3': 4,
  'p4': None,
  'p5': 0},
 {'addr': 7,
  'comment': None,
  'opcode': 'Column',
  'p1': 0,
  'p2': 2,
  'p3': 5,
  'p4': None,
  'p5': 0},
 {'addr': 8,
  'comment': None,
  'opcode': 'Column',
  'p1': 0,
  'p2': 3,
  'p3': 6,
  'p4': None,
  'p5': 0},
 {'addr': 9,
  'comment': None,
  'opcode': 'Column',
  'p1': 0,
  'p2': 4,
  'p3': 7,
  'p4': None,
  'p5': 0},
 {'addr': 10,
  'comment': None,
  'opcode': 'ResultRow',
  'p1': 3,
  'p2': 5,
  'p3': 0,
  'p4': None,
  'p5': 0},
 {'addr': 11,
  'comment': None,
  'opcode': 'Next',
  'p1': 0,
  'p2': 3,
  'p3': 0,
  'p4': None,
  'p5': 1},
 {'addr': 12,
  'comment': None,
  'opcode': 'Halt',
  'p1': 0,
  'p2': 0,
  'p3': 0,
  'p4': None,
  'p5': 0},
 {'addr': 13,
  'comment': None,
  'opcode': 'Transaction',
  'p1': 0,
  'p2': 0,
  'p3': 35,
  'p4': '0',
  'p5': 1},
 {'addr': 14,
  'comment': None,
  'opcode': 'Variable',
  'p1': 1,
  'p2': 2,
  'p3': 0,
  'p4': None,
  'p5': 0},
 {'addr': 15,
  'comment': None,
  'opcode': 'Goto',
  'p1': 0,
  'p2': 1,
  'p3': 0,
  'p4': None,
  'p5': 0}]

Note that the Variable opcode now has p4 as None - and the name of the variable no longer appears in any of the opcodes!

@simonw
Copy link
Owner Author

simonw commented Jun 11, 2024

Code at fault:

@documented
async def derive_named_parameters(db: "Database", sql: str) -> List[str]:
"""
Given a SQL statement, return a list of named parameters that are used in the statement
e.g. for ``select * from foo where id=:id`` this would return ``["id"]``
"""
explain = "explain {}".format(sql.strip().rstrip(";"))
possible_params = _re_named_parameter.findall(sql)
try:
results = await db.execute(explain, {p: None for p in possible_params})
return [row["p4"].lstrip(":") for row in results if row["opcode"] == "Variable"]
except sqlite3.DatabaseError:
return possible_params

@simonw
Copy link
Owner Author

simonw commented Jun 11, 2024

The key problem here is that opcodes are not a stable feature of SQLite, so it was always risk to attempt to use them in this way.

@simonw
Copy link
Owner Author

simonw commented Jun 11, 2024

We need to ensure SQLite 3.46.0 is covered in the new tests here:

@simonw
Copy link
Owner Author

simonw commented Jun 11, 2024

Posted about this on the SQLite forum - https://sqlite.org/forum/forumpost/0adbc56447 - but since opcodes are an undocumented and unsupported interface I don't expect them to fix it.

@simonw
Copy link
Owner Author

simonw commented Jun 11, 2024

This mechanism here was already supposed to catch things if SQLite broke the mechanism I was using:

except sqlite3.DatabaseError:
return possible_params

But it didn't think to catch AttributeError.

simonw added a commit that referenced this issue Jun 11, 2024
@simonw simonw added the bug label Jun 11, 2024
@simonw
Copy link
Owner Author

simonw commented Jun 11, 2024

Here's the macOS SQLite 3.46.0 libsqlite3.0.dylib I was using: https://static.simonwillison.net/static/2024/libsqlite3.0.dylib

@simonw
Copy link
Owner Author

simonw commented Jun 12, 2024

Here's the commit that removed this feature: https://sqlite.org/src/info/dd5977c9a8a418be

Via https://sqlite.org/forum/forumpost/1cafc721009cef7f

simonw added a commit to asg017/datasette that referenced this issue Jun 12, 2024
@simonw
Copy link
Owner Author

simonw commented Jun 12, 2024

I manually tested the patch to 0.64.x like this:

DYLD_LIBRARY_PATH=sqlite-3.46.0 datasette fixtures.db -m issue-2353.yaml -p 8034

Then visited http://127.0.0.1:8034/fixtures/demo?table=facetable and it worked.

I confirmed that the page had an error before applying that cherry-pick.

simonw added a commit that referenced this issue Jun 12, 2024
simonw added a commit that referenced this issue Jun 12, 2024
@simonw
Copy link
Owner Author

simonw commented Jun 12, 2024

Here's the full set of changes going out in 0.64.7: 0.64.6...0.64.7

@simonw
Copy link
Owner Author

simonw commented Jun 12, 2024

@simonw
Copy link
Owner Author

simonw commented Jun 12, 2024

Blogged about the release here: https://simonwillison.net/2024/Jun/12/datasette-0647/

@simonw simonw closed this as completed Jun 12, 2024
simonw added a commit that referenced this issue Jun 12, 2024
simonw added a commit that referenced this issue Jun 12, 2024
simonw added a commit to asg017/datasette that referenced this issue Jun 12, 2024
simonw added a commit that referenced this issue Jun 21, 2024
@simonw
Copy link
Owner Author

simonw commented Jul 31, 2024

Need to forward-port this to Datasette 1.0.

@simonw simonw reopened this Jul 31, 2024
@simonw
Copy link
Owner Author

simonw commented Jul 31, 2024

Oh it's in Datasette 1.0 already, just not yet in a release: d118d5c

@simonw simonw closed this as completed Jul 31, 2024
@simonw simonw mentioned this issue Aug 5, 2024
simonw added a commit that referenced this issue Aug 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant