database type #104
database type #104
Conversation
…te an instance without options, return the type derived from databases.json (or the name)
|
Analyzing https://github.com/observablehq/charts/commit/809883a018be17afb982b838152d9d7811bc98a8 + https://github.com/observablehq/charts/commit/2de479e4df0f239c9b2bfde0f257051c30c2d736 by @mbostock:
|
|
|
||
| /** | ||
| * Reads partial database configurations from the .observable/databases.json file. | ||
| * For security reasons, no identifiers or passwords are returned. |
There was a problem hiding this comment.
Not sure this redaction accomplishes anything, since any process can read the .observable/databases.json file directly. I guess if we want to call this getDatabaseTypes then we could have a Map<string, string> that tells you the database types… but it’s probably just fine to load the unredacted database configs, too.
| // if options are passed, don't change them | ||
| if (node.arguments.length !== 1) return; |
There was a problem hiding this comment.
We should pass {dialect} as a third argument, and not allow more than two arguments (or fewer than one) argument, and not allow spread arguments.
| else if (name === "sqlite") type = "sqlite"; | ||
| else if (/\.duckdb$/i.test(name)) type = "duckdb"; | ||
| else if (/\.db$/i.test(name)) type = "sqlite"; | ||
| else throw new Error(`database not found: ${name}`); |
There was a problem hiding this comment.
This needs to be a SyntaxError, not an internal Error. Ideally in the .duckdb and .db case we’d also check that the files exist (and SyntaxError if they don’t), but I’m not sure if that’s possible in this context. Or one possibility is that we find the .duckdb and .db files ahead of time when we call getDatabaseConfigs (because these files implicitly represent databases).
mbostock
left a comment
mbostock
left a comment
There was a problem hiding this comment.
Thinking aloud, I wonder if rewriting DatabaseClient calls is correct, or if this should be more like what we’ve done with FileAttachment in the past (which also is yet to be implemented in Notebook Kit), which is that you register the databases separately when initializing the runtime…
This is a suggestion to surface the database's
typein theDatabaseClientoptions.When we create an instance without options, we return the type derived from
databases.json(or the database name, when applicable).This allows to write SQL statements that depends on the database's dialect.
TODO
{dialect}as a third argumentdatabase.dialect, per the spec https://observablehq.com/@observablehq/database-client-specification#%C2%A75