sqlite: add tagged template

[0hmX]

Closes#57570

This pr introduces the support for tagged templates and an LRU to cache the templates. We introduced a new object called SqlTagStore that holds the ref to Lru. This acts as the main object that allows us to use tagged templates.

Look at the test file test-sqlite-template-tag.js to see how the api is designed.

[0hmX]

looking forward to your answers cc @nodejs/sqlite

[RafaelGSS]

cc: @geeksilva97@miguelmarcondesf

[jasnell]

Since the API requires passing in the database instance, I'd prefer something like...

constdb=newDatabaseSync(":memory:");consttemplate=db.createSqlTag();

As opposed to a standalone top-level function.

[geeksilva97]

Hello @0hmX . I wonder how this will fit the current API interface. I like @jasnell's suggestion. Do you think we could make it like the following?

constdb=newDatabaseSync(":memory:");consttemplate=db.createSqlTag();template.run`...`;template.get`...`template.all`...`

EDIT: I'm not sure if my question makes sense. I asked about it in the issue to get more information.

[geeksilva97]

From a technical point of view, @0hmX . This PR needs tests and documentation.

The cache is important in the implementation since statements are meant to be reused. With template tags, we miss this. In such a situation, the cache is important, as in Matteo's implementation.

I also wonder if we could have this implementation on C++ side, as all the implementations of node:sqlite module.

[0hmX]

@geeksilva97 and @jasnell, thank you for the feedback.

One of the key features that tag templates could bring is SQL syntax highlighting. However, this will only work if we have a pattern like this:

const willSyntaxHighlight = sql`SELECT * FROM users WHERE age = ${age}` // will get syntax highlighted with extensions const willNotSyntaxHighlight = `SELECT * FROM users WHERE age = ${age}` 

This is the pattern I see in all the extensions that allow you to highlight a string template inside ts or js for SQL in VS Code.

We need to have a template function called sql in front. Therefore, we should export a top-level function called SQL that returns an object with the raw SQL string and parameters. I have implemented this in C++ and added it! This means we will need to add support for this custom object in database.exec (we will convert the custom object into a raw string) and database.prepare.

const db = new DatabaseSync(":memory:");
const template = db.createSqlTag();

I think the name should be changed to something like db.createCache(). as this is creating a cache, storing the SQL string, and checking if a prepared statement is already made for it and calling the appropriate method on it and returning the outputs.

the final Api should look something like this

const{ sql, DatabaseSync }=require("node:sql")constdb=newDatabaseSync(":memory:")// adding sql template function in front for syntax highlight, else it's the same as using a multiline stringdb.exec(sql`CREATE TABLE contacts ( id INTEGER PRIMARY KEY AUTOINCREMENT, name TEXT NOT NULL, phone TEXT NOT NULL UNIQUE, email TEXT NOT NULL UNIQUE)`)// same as using a multiline stringdb.exec(sql`INSERT INTO contacts (name, phone, email) VALUES (${"Alice"}, ${"111-222-3333"}, ${"[email protected]"})`)db.exec(sql`INSERT INTO contacts (name, phone, email) VALUES (${"Bob"}, ${"444-555-6666"}, ${"[email protected]"})`)db.exec(sql`INSERT INTO contacts (name, phone, email) VALUES (${"Charlie"}, ${"777-888-9999"}, ${"[email protected]"})`)constcache=db.createCache()constemailDomain="%@example.com"cache.all(sql`SELECT * FROM contacts WHERE email LIKE ${emailDomain}`)constcontactId=2cache.get(sql`SELECT * FROM contacts WHERE id = ${contactId}`)constnamePrefix="C%"cache.iterate(sql`SELECT * FROM contacts WHERE name LIKE ${namePrefix} ORDER BY name`)

[bakkot]

One of the key features that tag templates could bring is SQL syntax highlighting. However, this will only work if we have a pattern like this: [...] We need to have a template function called sql in front.

VSCode supports more complex patterns than just literally an identifier, I'm almost certain. It can be made to handle db.prepare or anything.sql just as well as a bare sql. See this repo for an example.

// same as using a multiline string

It should very much not be the same as using a multiline string. The whole point of tagged templates - literally the only reason they are in the language - is that unlike strings they allow using a representation which is immune to problems like sql injection. From your implementation it looks like you're correctly avoiding sql injections, but this is something which should be emphasized in the docs and tested extensively. Users need to be aware that omitting the tag is not safe; it's not just a convenience for getting syntax highlighting.

On that note, you should also be doing parsing of the string up front: users should get an error as soon as they write sql`invalid`; rather than needing to actually pass it somewhere to get an error. Note that the first argument to the template tag is always the same value on subsequent calls so that it can be used as a key in a cache instead of needing to check every time the function is called. Probably the easiest thing is to prepare the statement right away, and store the prepared statement in a cache.

[codecov]

Codecov Report

❌ Patch coverage is 69.44444% with 121 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.24%. Comparing base (14c68e3) to head (bcb2d75).
⚠️ Report is 255 commits behind head on main.

Files with missing lines	Patch %	Lines
src/node_sqlite.cc	67.97%	54 Missing and 60 partials ⚠️
src/lru_cache-inl.h	88.88%	2 Missing and 2 partials ⚠️
src/node_sqlite.h	0.00%	2 Missing ⚠️
src/quic/data.cc	50.00%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #58748 +/- ## ========================================== - Coverage 89.93% 88.24% -1.69%  ========================================== Files 669 702 +33 Lines 197473 207051 +9578 Branches 38588 39804 +1216 ========================================== + Hits 177604 182722 +5118 - Misses 12284 16310 +4026 - Partials 7585 8019 +434 

Files with missing lines	Coverage Δ
src/quic/data.h	63.63% <ø> (+25.17%)	⬆️
src/quic/data.cc	54.75% <50.00%> (+15.38%)	⬆️
src/node_sqlite.h	80.39% <0.00%> (-3.29%)	⬇️
src/lru_cache-inl.h	88.88% <88.88%> (ø)
src/node_sqlite.cc	79.37% <67.97%> (-1.91%)	⬇️

... and 109 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[0hmX]

The PR is as stable as it gets now with all the tests passing. I need reviews on the PR. Look at the test file test-sqlite-template-tag.js to see how the api is designed.

cc: @jasnell@geeksilva97@bakkot also @mcollina (as creator of the main issues)

[mcollina]

lgtm

[mcollina]

lgtm

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/69148/

[0hmX]

It failed because of some disk space issues, as I can see in the logs! Hopefully that is fixed!
Give it a re-run!

cc: @geeksilva97@gurgunday@lemire

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/69233/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/69261/

[nodejs-github-bot]

Landed in 97e55f8

[ghost]

I found this new features in the release notes. Thank you for the great work.

Just a question.

Why is there chosen for a sql.all`SELECT * FROM users` syntax instead of sql`SELECT * FROM users`.all()?
Then there would only be one tagged template function named sql.

Something like this: (without caching, error handling, ...)

exportfunctioncreateSqlTag(db){return(strings, ...values)=>{constsql=strings.join("?");conststmt=db.prepare(sql);constresult={ stmt,replaceValues: (...newValues)=>{values=newValues;returnresult;},run: ()=>stmt.run(...values),get: ()=>stmt.get(...values),all: ()=>stmt.all(...values),};returnresult;};}

It would also be easier to reuse the prepared statement. Not sure if caching here is needed.

[Pumpman828]

Thank you

[jasnell]

FWIW, this had a few issues with error handling on the c++ side... #60040

[Pumpman828]

Awesome

[aduh95]

This doesn't land cleanly on v22.x-staging, and would require a manual backport PR if we want it there