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

Include "entrypoint" option on --load-extension? #1784

Closed
asg017 opened this issue Aug 16, 2022 · 2 comments · Fixed by #1789
Closed

Include "entrypoint" option on --load-extension? #1784

asg017 opened this issue Aug 16, 2022 · 2 comments · Fixed by #1789

Comments

@asg017
Copy link
Collaborator

asg017 commented Aug 16, 2022

Problem

SQLite extensions have the option to define multiple "entrypoints" in each loadable extension. For example, the upcoming version of sqlite-lines will have 2 entrypoints: the default sqlite3_lines_init (which SQLite will automatically guess for) and sqlite3_lines_noread_init. The sqlite3_lines_noread_init version omits functions that read from the filesystem, which is necessary for security purposes when running untrusted SQL (which Datasette does).

(Similar multiple entrypoints will also be added for sqlite-http).

The --load-extension flag, however, doesn't give the option to specify a different entrypoint, so the default one is always used.

Proposal

I want there to be a new command line option of the --load-extension flag to specify a custom entrypoint like so:

datasette my.db \
  --load-extension ./lines0 sqlite3_lines0_noread_init

Then, under the hood, this line of code:

conn.execute("SELECT load_extension(?)", [extension])

Would look something like this:

 conn.execute("SELECT load_extension(?, ?)", [extension, entrypoint]) 

One potential problem: For backward compatibility, I'm not sure if Click allows cli flags to have variable number of options ("arity"). So I guess it could also use a : delimiter like --static:

datasette my.db \
  --load-extension ./lines0:sqlite3_lines0_noread_init

Or maybe even a new flag name?

datasette my.db \
  --load-extension-entrypoint ./lines0 sqlite3_lines0_noread_init

Personally I prefer the : option... and maybe even --load-extension -> --load? Definitely out of scope for this issue tho

datasette my.db \
  --load./lines0:sqlite3_lines0_noread_init
@simonw
Copy link
Owner

simonw commented Aug 16, 2022

I didn't know SQLite could do this.

I agree with you - I like the : option you described there.

I'd like to stick with --load-extension - --load is general enough that I might have it do something else in the future, and this is a rare enough advanced feature that I'm OK with it being verbose.

@simonw
Copy link
Owner

simonw commented Aug 16, 2022

Do you want to write a PR for this?

Ideally I'd want this to be covered by a test, but it looks like --load-extension isn't covered by tests at all yet. It's a tricky thing to add to the unit tests because there are plenty of Python installs out there (including the one on my Mac) that don't support .enable_load_extension() at all.

So maybe a test which uses @pytest.mark.skipIf() to skip itself if the environment it is running in doesn't support that feature?

But we'd also need to bundle a compiled SQLite extension to use in the test, which would be a slightly weird thing to have in the repo.

We do have one previous example of something like that though: https://github.com/simonw/datasette/blob/main/tests/spatialite.db and the script that builds it https://github.com/simonw/datasette/blob/main/tests/build_small_spatialite_db.py

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 a pull request may close this issue.

2 participants