simonw / datasette

An open source multi-tool for exploring and publishing data
https://datasette.io
Apache License 2.0
9.06k stars 648 forks source link

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

Closed simonw closed 2 weeks ago

simonw commented 2 weeks ago

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 commented 2 weeks ago

Code at fault: https://github.com/simonw/datasette/blob/7437d40e5dd4d614bb769e16c0c1b96c6c19647f/datasette/utils/__init__.py#L1137-L1150

simonw commented 2 weeks ago

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 commented 2 weeks ago

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

simonw commented 2 weeks ago

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 commented 2 weeks ago

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

https://github.com/simonw/datasette/blob/7437d40e5dd4d614bb769e16c0c1b96c6c19647f/datasette/utils/__init__.py#L1149-L1150

But it didn't think to catch AttributeError.

simonw commented 2 weeks ago

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

simonw commented 2 weeks ago

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

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

simonw commented 2 weeks ago

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 commented 2 weeks ago

Here's the full set of changes going out in 0.64.7: https://github.com/simonw/datasette/compare/0.64.6...0.64.7

simonw commented 2 weeks ago

Released: https://pypi.org/project/datasette/0.64.7/ / https://docs.datasette.io/en/stable/changelog.html#v0-64-7

simonw commented 2 weeks ago

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