Implement remote libSQL DDL support and refactor DDL builders - #4

Review — PR #4

Overall the PR is solid on the host-contract side (payload shapes, metadata key renames, URI scheme passthrough were all checked against the tabularis host source and hold up). However, 5 real bugs were confirmed empirically against the PR's own embedded libSQL fork, plus a manifest issue and a few smaller items.

Confirmed bugs (should be fixed before merge)

1. file:/// URIs on Unix silently open the wrong database — src/client.rs:287
expand_path strips the entire file:/// prefix, so file:///data/app.db (the exact form shown in the manifest's own connection_string_example) becomes the relative path data/app.db. Builder::new_local then silently creates/opens an empty ./data/app.db instead of /data/app.db. The previous code stripped only file:, leaving ///data/app.db, which POSIX resolves absolutely. The only test covers the Windows file:///C:/... form, where dropping the third slash happens to be correct.

2. FK add/drop strips NOT NULL and DEFAULT from the rewritten column — src/handlers/ddl.rs:398
The column definition is rebuilt from PRAGMA table_info using only the type (cell 2), ignoring notnull (3) and dflt_value (4) from the same row. Verified: on user_id INT NOT NULL DEFAULT 7 REFERENCES users(id), after drop_foreign_key the column is just INT — both constraints are gone. Same applies to get_create_foreign_key_sql.

3. Dropping a composite FK reports success while doing nothing — src/handlers/ddl.rs:420
For FOREIGN KEY(a,b) REFERENCES p(x,y), rewriting a single column doesn't touch the table-level constraint: PRAGMA foreign_key_list still shows both rows afterwards, but the handler returns Ok, so the host UI shows the FK as dropped.

4. Changing a column's type/default/nullability destroys its existing FK — src/handlers/ddl.rs:221
Same root cause as #2: ALTER COLUMN x TO x <def> replaces the entire column definition, but build_alter_column_sql rebuilds it only from the dialog's fields. Retyping user_id INT REFERENCES users(id) to BIGINT silently drops the REFERENCES clause (and any column-level UNIQUE/CHECK).

5. Simultaneous rename + retype silently discards the retype — src/handlers/ddl.rs:213
When names differ, the builder early-returns with only RENAME COLUMN, dropping any type/default/NOT NULL changes with no error. Since the handler already returns Vec<String>, it could emit the rename followed by the ALTER COLUMN with no contract change.

Bugs 2, 4 and 5 share one root cause: the ALTER COLUMN builders never carry the column's full existing definition through the rewrite.

Manifest icon doesn't resolve anywhere

.tabularium sets "icon": "sqlite", but the Tabularium spec defines icon as a path or URL to a square logo, so the registry renders nothing. In the host app, EngineCard.tsx resolves symbolic icon names only for builtin drivers — third-party plugins fall back to the generic database glyph. Fix: ship a square logo in the repo (e.g. assets/icon.png, as the CSV plugin does) and point the manifest at it. (FYI: the BigQuery plugin has the same bug with "icon": "cloud".)

Other findings

• Fragile trigger parser — src/handlers/metadata.rs:138: timing/event are classified via contains() over the whole trigger SQL, so body keywords override the header. The common audit trigger AFTER UPDATE ... BEGIN INSERT INTO log ... reports event=INSERT; the PR's own test passes only by luck of branch order. Parse the header before the ON keyword instead.
• README contradicts the manifest and the code — README.md:79 (and lines 55–57): it claims create_foreign_keys is false and that local ALTER COLUMN / FK drop fail, while .tabularium sets true and the PR's own tests (local_alter_column_retypes_through_the_fork, drop_foreign_key_local_rewrites_the_schema) prove local support works.
• Duplicated FK-name scheme — the fk_{table}_{from}_{id} format is hardcoded both in ddl.rs:430 (reverse-mapping) and metadata.rs:63 (generation). If either changes, drop_foreign_key silently stops matching every FK the plugin itself reports. Extract a shared fk_name(table, column, id) helper.
• Full futures facade for a single block_on — Cargo.toml:26: use futures = { version = "0.3", default-features = false, features = ["executor"] } (or futures-executor directly) to cut the dependency tree and compile time.
• block_on per row instead of per query — src/client.rs:102: the fetch loop enters a fresh executor for every row (10k rows = 10k+1 waker/parker setups on the hottest path). Wrap the whole loop in a single block_on(async { ... }).

@debba one thing I haven't included here is a fix to the justfile for the windows dev-install script but also for the other OS installation locations.

Thanks for this, it was a real issue, but I merged this TabularisDB/tabularis#258 in latest release, so probably won't work now :D

@debba Addressed all feedback items.

Full futures facade for a single block_on — Cargo.toml:26: use futures = { version = "0.3", default-features = false, features = ["executor"] } (or futures-executor directly) to cut the dependency tree and compile time.

Made the change but goal unmet. cargo tree -i futures shows libsql core feature hard-requires dep:futures (core = [..., "dep:futures", ...] in libsql 0.9.30). Facade still compiles; futures-executor was already in tree via libsql's facade. Swap = dependency accuracy only (we use one symbol, not the facade), no compile-time or tree savings.

@debba one thing I haven't included here is a fix to the justfile for the windows dev-install script but also for the other OS installation locations.

Thanks for this, it was a real issue, but I merged this TabularisDB/tabularis#258 in latest release, so probably won't work now :D

Fixed: Update installation paths

@jonaspm You’re right, thanks for verifying it. Since libsql already requires the futures facade, my claim about reducing the dependency tree and compile time doesn’t hold here. Using futures-executor is still more precise for our direct usage, but brings no practical build savings. Consider this item resolved. If you agree, I think we’re ready to merge and release the plugin.

Merging 🎉