Fix SQLite generated table column metadata - #581
Conversation
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (2 files)
Previous Review Summary (commit 4764730)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 4764730)Status: No Issues Found | Recommendation: Merge Files Reviewed (2 files)
Reviewed by laguna-s-2.1:free · Input: 87.2K · Output: 15.5K · Cached: 22.4K |
debba
left a comment
There was a problem hiding this comment.
Thanks for looking into this, but I don't think this actually fixes #162.
The repro in the issue is a plain table with a generated column, no view involved. I tested against a real database (system SQLite 3.51 and the bundled 3.46 behave the same):
PRAGMA table_infoon the table only returnsudate, the generated column is missing.table_xinforeturns it (hidden=3for STORED,hidden=2for VIRTUAL). That mismatch between column metadata and theSELECT *data is exactly the grid misalignment in the issue's screenshot.- On a view over that table,
table_infoalready returns all the columns, same output astable_xinfo. Columns coming from generated columns are just ordinary view columns.
So the change in get_view_columns is a no-op for this bug. The functions the grid actually uses for tables, get_columns (src-tauri/src/drivers/sqlite/mod.rs:67) and get_all_columns_batch (mod.rs:160), still use table_info and are still broken.
The new test also passes without the fix: I reverted line 841 back to table_info, rebuilt, and test_get_view_columns_includes_generated_table_columns still passes. Note that the test command in the description uses --no-run, so it compiles the test but never runs it, which is probably how this slipped through.
For a proper fix I'd suggest:
- Switch
get_columnsandget_all_columns_batchtotable_xinfo. Keeping the view change is fine, it's harmless. - Be careful with writes: generated columns can't be inserted or updated, and
TableColumnhas nois_generatedflag, so exposing them in table metadata may let the row editor build INSERT/UPDATE statements that SQLite rejects. Thehiddenvalue fromtable_xinfo(2 = virtual, 3 = stored) is what you need to flag them. - The regression test should target
get_columnson the table itself, and should be verified to fail on main before the fix.
Happy to re-review once it's updated.
|
Updated this based on the review. Changes made:
Validation:
The Rust test compiles locally. Direct execution on my Windows environment exits before the harness runs with |
Summary
Testing
Note: the targeted Rust test compiles locally, but direct execution on this Windows environment exits before the harness runs with STATUS_ENTRYPOINT_NOT_FOUND.
Fixes #162