sqlite: add tagged template #58748
sqlite: add tagged template #58748
Conversation
|
looking forward to your answers cc @nodejs/sqlite |
|
Since the API requires passing in the database instance, I'd prefer something like... const db = new DatabaseSync(":memory:");
const template = db.createSqlTag();As opposed to a standalone top-level function. |
|
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? const db = new DatabaseSync(":memory:");
const template = 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. |
|
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 |
|
@geeksilva97 and @jasnell, thank you for the feedback.
const { sql, DatabaseSync } = require("node:sql")
const db = new DatabaseSync(":memory:")
// adding sql template function in front for syntax highlight, else it's the same as using a multiline string
db.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 string
db.exec(sql`INSERT INTO contacts (name, phone, email) VALUES (${"Alice"}, ${"111-222-3333"}, ${"alice@example.com"})`)
db.exec(sql`INSERT INTO contacts (name, phone, email) VALUES (${"Bob"}, ${"444-555-6666"}, ${"bob@example.com"})`)
db.exec(sql`INSERT INTO contacts (name, phone, email) VALUES (${"Charlie"}, ${"777-888-9999"}, ${"charlie@example.com"})`)
const cache = db.createCache()
const emailDomain = "%@example.com"
cache.all(sql`SELECT * FROM contacts WHERE email LIKE ${emailDomain}`)
const contactId = 2
cache.get(sql`SELECT * FROM contacts WHERE id = ${contactId}`)
const namePrefix = "C%"
cache.iterate(sql`SELECT * FROM contacts WHERE name LIKE ${namePrefix} ORDER BY name`) |
VSCode supports more complex patterns than just literally an identifier, I'm almost certain. It can be made to handle
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 |
f5396ab to
b42e601
Compare
243ade6 to
e7d49d2
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #58748 +/- ##
==========================================
- Coverage 90.16% 89.95% -0.22%
==========================================
Files 636 650 +14
Lines 188060 192491 +4431
Branches 36899 37734 +835
==========================================
+ Hits 169568 173148 +3580
- Misses 11235 11872 +637
- Partials 7257 7471 +214
🚀 New features to boost your workflow:
|
a5260f2 to
d0b31a0
Compare
|
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) |
|
Left one minor comment about docs. Other than that, LGTM. |
Adding tagged template and LRU cache for prepared statements in SQLite. Co-authored-by: Edy Silva <edigleyssonsilva@gmail.com>
Failed to start CI⚠ Commits were pushed since the last approving review: ⚠ - sqlite: add tagged template support ✘ Refusing to run CI on potentially unsafe PRhttps://github.com/nodejs/node/actions/runs/16691132856 |
|
Is this a flaky test? Both CI runs failed because of test/client-proxy/test-https-proxy-request-invalid-char-in-url.mjs. Should I rebase my changes on the latest main commit? Will that help? |
|
@0hmX I don't know about this particular failure, but flaky tests are definitively a possibility. I have restarted the test. |
|
The CI failures are completely different from the previous one. is this normal? |
Yeah it is 😔 |
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.