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

Feature request: add offset to SQLite's error mesages #1391

Open
rozenmd opened this issue Nov 6, 2023 · 3 comments
Open

Feature request: add offset to SQLite's error mesages #1391

rozenmd opened this issue Nov 6, 2023 · 3 comments
Labels
feature request Request for Workers team to add a feature

Comments

@rozenmd
Copy link
Contributor

rozenmd commented Nov 6, 2023

The issue

While looking into cloudflare/workers-sdk#4343 I noticed a difference between how workerd/d1 reports errors, and how SQLite shell reports errors:

wrangler d1 execute DBNAME --local --command='CREATE TABLE accounts (
  id tinytext NOT NULL,
  username varchar(32) NOT NULL,
  disabled tinyint(1) NOT NULL DEFAULT 0,
  email varchar(255) NOT NULL,
  password tinytext NOT NULL,
  oplvl tinyint(4) NOT NULL DEFAULT 0,
  created timestamp NOT NULL DEFAULT current_timestamp(),
  lastchange timestamp NOT NULL DEFAULT current_timestamp(),
  token tinytext NOT NULL DEFAULT '',
  session tinytext NOT NULL DEFAULT ''
);'

X [ERROR] near "(": syntax error

vs

sqlite> CREATE TABLE accounts (
  id tinytext NOT NULL,
  username varchar(32) NOT NULL,
  disabled tinyint(1) NOT NULL DEFAULT 0,
  email varchar(255) NOT NULL,
  password tinytext NOT NULL,
  oplvl tinyint(4) NOT NULL DEFAULT 0,
  created timestamp NOT NULL DEFAULT current_timestamp(),
  lastchange timestamp NOT NULL DEFAULT current_timestamp(),
  token tinytext NOT NULL DEFAULT '',
  session tinytext NOT NULL DEFAULT ''
);
Parse error: near "(": syntax error
  eated timestamp NOT NULL DEFAULT current_timestamp(),   lastchange timestamp N
                                      error here ---^

Some context

It turns out this is because the sqlite shell implements nice errors here: https://github.com/sqlite/sqlite/blob/master/src/shell.c.in#L3097-L3101, while regular sqlite doesn't return the exact location of the error.

Potential fix

Back in SQLite Release 3.38.4, we got access to the sqlite3_error_offset() interface:

Added the sqlite3_error_offset() interface, which can sometimes help to localize an SQL error to a specific character in the input SQL text, so that applications can provide better error messages.

Using it, we could make the error message returned from workerd more like:

X [ERROR] near "(" at offset 17: syntax error

rather than:

X [ERROR] near "(": syntax error
@KianNH
Copy link
Contributor

KianNH commented Nov 6, 2023

I had a quick try at implementing sqlite3_error_offset() into workerd's SQLite errors and had success with it - in the event that the error was not related to a specific token then sqlite3_error_offset() would return -1.

image

(when provided an input of SELECT ;)

If it's possible to further emulate the shells behaviour, given we have access to the input SQL, and do a 'error here' arrow then that'd be awesome.

https://github.com/sqlite/sqlite/blob/ff9ff548018bbe276dbd7bf5418af8bdfd659a24/src/shell.c.in#L3028-L3032

@james-pre
Copy link

@rozenmd @KianNH

Thank you both for taking my feedback in stride!

@KianNH
Copy link
Contributor

KianNH commented Nov 6, 2023

No problem. We should also be able to implement the input & arrow indicator that SQLite's shell has too.

image

@jasnell jasnell added the feature request Request for Workers team to add a feature label Jan 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Request for Workers team to add a feature
Projects
None yet
Development

No branches or pull requests

4 participants