Repository navigation
feat(transfer): copy table rows between PostgreSQL and PostgreSQL/SQLite connections - #1421
Asymtode712 wants to merge 13 commits into
Conversation
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Incremental review of
Files Reviewed (2 files)
Notes: static review only; code was not compiled or tests run. Previous Review Summaries (8 snapshots, latest commit 58413bf)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 58413bf)Status: No Issues Found | Recommendation: Merge Incremental review of
No defects were found in the changed code. One behavior to be aware of: the check blocks rather than allows when identity is unprovable, so two genuinely different servers with the same schema and table name are blocked if the role cannot read Files Reviewed (16 files)
Notes: static review only; code was not compiled or tests run. Previous review (commit 21c87cb)Status: No Issues Found | Recommendation: Merge Incremental review of
No new issues were found in the changed code. Files Reviewed (2 files)
Notes: static review only; code was not compiled or tests run. Previous review (commit cd6d2fe)Status: No Issues Found | Recommendation: Merge Incremental review of No new issues were found, and the previously reviewed code is unchanged. Files Reviewed (1 file)
Notes: static review only; code was not compiled or tests run. Previous review (commit 9e4c46e)Status: No Issues Found | Recommendation: Merge Incremental review of The merge integrates cleanly with the features that landed on main in the meantime (table export/import to SQL,
No new issues were found. Files Reviewed (merge integration points and formatting commit)
Notes: static review only; code was not compiled or tests run. Previous review (commit a384eab)Status: No Issues Found | Recommendation: Merge Incremental review of Files Reviewed (6 files)
Notes: static review only; code was not compiled or tests run. Previous review (commit 3a6d445)Status: No Issues Found | Recommendation: Merge Incremental review of Files Reviewed (1 file)
Notes: static review only; code was not compiled or tests run. Previous review (commit 77c29c8)Status: No Issues Found | Recommendation: Merge Incremental review of
Files Reviewed (4 files)
Notes: static review only; code was not compiled or tests run. Previous review (commit 4682db5)Status: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (37 files)
Notes: static review only; code was not compiled or tests run. Authorization (source and target connection checks, SQLite/local-path guard), serde/TS type alignment, identifier quoting, bind-parameter limits and batch transaction handling were checked and found sound. Reviewed by claude-sonnet-5.5 · Input: 258 · Output: 2K · Cached: 246.2K Review guidance: REVIEW.md from base branch |
debba
left a comment
debba
left a comment
There was a problem hiding this comment.
Thanks for making the transfer plan explicit, rebuilding it before writing, and reporting committed batches separately from a failed or cancelled batch. Keeping this first version append-only with a published supported-pair matrix makes the scope clear.
All 15 targeted frontend tests and typecheck pass on this branch. I have not validated the Rust integration tests or the real database/file workflow yet; the host Rust build stops at missing pkg-config/GLib prerequisites.
I haven't found a blocking issue in the diff. I'll run end-to-end tests for further verification, then proceed with the merge if those checks pass.
…s-between-database-connections # Conflicts: # packages/web-ui/src/api/contract.ts # packages/web-ui/src/components/layout/ExplorerSidebar.tsx # packages/web-ui/src/utils/tableTransfer.ts # packages/web-ui/tests/utils/tableTransfer.test.ts # src-tauri/src/application/database_transfers.rs # src-tauri/src/application/table_transfers.rs # src-tauri/src/application/table_transfers_tests.rs # src-tauri/src/transport/desktop/dump_commands.rs # src-tauri/src/transport/desktop/mod.rs # src-tauri/src/transport/web/rpc.rs # src-tauri/src/transport/web/rpc_tests.rs
debba
left a comment
debba
left a comment
There was a problem hiding this comment.
I found a blocking self-copy case in the real web app. Two saved PostgreSQL connections that resolve to the same database can bypass the same-table check, so the transfer writes back into its source.
To reproduce:
- Save connections A and B for the same PostgreSQL server and database.
- Create a table without a primary key and insert two rows:
CREATE TABLE public.alias_copy_test(id integer, note text); INSERT INTO public.alias_copy_test VALUES (1, 'first'), (2, 'second');
- From that table on A, choose Transfer to another connection, select B, and choose the same schema and table.
- Review the plan and start the transfer.
Expected: the plan blocks this as source equals target before any write.
Actual: Start is enabled, the app reports Completed / Rows written: 2, and the original table now contains four rows, two copies of each row. I reproduced this twice with my local PostgreSQL fixture. Both run requests returned HTTP 200, with no console or backend exception.
resolve() currently passes source_connection_id == target_connection_id into plan_tables(). Different saved IDs therefore bypass the guard even when the resolved database, schema and table are identical. Please check the resolved endpoint/table identity across connection aliases and add a regression test with two saved IDs pointing to the same database.
The normal 1,201-row transfer between different tables passed, including a full source/target value comparison, and the 14 live transfer tests passed. This self-copy case still needs to be fixed before merging.
|
@debba I have fixed the self-copy case. The same-table check compared saved connection IDs, so two saved connections to the same PostgreSQL database bypassed it. It now compares the resolved endpoints (driver, host/port and database, or SQLite file, plus schema and table), and tunnels only match when provably the same route. Plan and run share this path, so the plan reports sourceEqualsTarget and Start stays disabled. I added regression tests for the two-saved-IDs case, loopback host spellings, tunnel routes and SQLite path aliases. Could you re-run your live PostgreSQL scenario? |
debba
left a comment
debba
left a comment
There was a problem hiding this comment.
Thanks for replacing the saved-ID comparison with endpoint checks and covering loopback spellings, different databases, and tunnel routes. The 19 frontend tests pass, and all 19 Rust transfer tests pass with PostgreSQL 16 available on port 54320.
There is still a self-copy path through equivalent hostnames. I connected to the same test database using localhost. and 127.0.0.1 with the same port and database; both connections reported the same inet_server_addr(), inet_server_port() and current_database(). Running the exact same_host helper from this head returns false for that pair, while localhost versus 127.0.0.1 returns true.
reaches_same_database therefore treats these two saved connections as different databases. With the same schema and table, is_same_table stays false and the sourceEqualsTarget guard is bypassed again. The same issue applies to two DNS aliases for one PostgreSQL endpoint.
Please compare the effective database/table identity in a way that covers equivalent DNS names, or reject cases where that identity cannot be established safely. Add a regression through the plan/run path with two saved connections, rather than only manually constructed parameter comparisons.
For a live regression fixture, create public.alias_copy_test(id integer, note text) without a primary key and insert (1, 'first'), (2, 'second'). Save A with host localhost. and B with host 127.0.0.1, both pointing to the same server port and database. Planning A's table into B's same table should block; a direct run should also reject it, and the row count should remain two. Drop the fixture table after testing.
I'm requesting changes because the guard still permits a transfer back into its source. I verified the hostname comparison and the shared PostgreSQL endpoint directly; I did not repeat this variant in the UI.
|
@debba Fixed this... Host-name comparison can't prove two saved connections reach different databases, so when names and strings don't settle it, the plan now asks each PostgreSQL server for its identity (cluster |
debba
left a comment
debba
left a comment
There was a problem hiding this comment.
Thanks for adding the server identity check and the regression coverage for aliases. The frontend transfer suites pass on this head with 15 tests.
The CI test job is currently blocked by a Rust compilation error. In src/table_transfer/tests.rs, the TableColumn initializer is missing the default_sql and on_update fields added on main. CI reports E0063 before it can run the library tests. Please update the fixture for the current model and rerun the tests.
I have not yet repeated the live PostgreSQL alias transfer test on this head, so the passing frontend tests should not be read as validation of the server identity path.
…s-between-database-connections
|
@debba I have made the changes, the CI is green now, kindly review this PR |
NewtTheWolf
left a comment
NewtTheWolf
left a comment
There was a problem hiding this comment.
Thanks for filling the new TableColumn fields and updating the transfer tests. I rechecked 3b648c0: typecheck, the 31 affected frontend tests and all 2,516 Rust library tests pass.
I also reran the 22 table-row-transfer tests against a dedicated PostgreSQL 17 fixture with both IPv4 and IPv6 loopback bindings. None were skipped. PostgreSQL-to-PostgreSQL and PostgreSQL-to-SQLite transfers pass, and both localhost/127.0.0.1 alias cases reject copying a table into itself while preserving its two source rows.
The compilation and live alias-test concerns from the earlier review are resolved. Approved.
Summary
Part of #1405
Adds a guided source-to-target table transfer (first milestone of the "Transfer selected tables directly between database connections" feature request).
get_table_transfer_capabilitiespublishes the supported driver pairs. Everything else is rejected at plan time and hidden in the UI. Plugin drivers don't get this feature (a plugin target/source needs its own exact-conversion contract); the menu entry is hidden for them.plan_table_transferresolves column mapping (by name, or explicit mapping/skip) and the conversion rule for each column. It blocks on: missing source/target table, unmapped or unknown columns, generated or unsupported types, incompatible types, required target columns with no source, same source/target table, read-only target. The plan is rebuilt inrun_table_transfer; a client-supplied plan is never trusted.bytea,numeric(as exact text), timestamps and JSON are preserved.fail(stop at first rejected row) orskip(ON CONFLICT DO NOTHING, unique/PK conflicts only). No TRUNCATE, DROP, upsert or table creation.REPEATABLE READtransaction, one batch at a time (default 500, max 5000). Each batch commits in its own target transaction. A failing batch rolls back only itself.readonlycapability blocks the plan; the production write confirmation runs before starting; web requests naming a SQLite/local-path connection as source or target need local-admin authorization. Progress events and errors never contain row values (database errors are reduced to SQLSTATE + constraint/column names).application/table_transfers.rsand the puretable_transfermodule; thin Tauri commands intransport/desktop/dump_commands.rs; matching web RPC commands and atable_transfer_progressevent.cargo check --no-default-featurespasses.TableTransferModal(target → review plan → run) opened from the table context menu. Keyboard accessible with visible focus, design tokens only, strings added to all 13 locales.Related: #1292 (schema comparison), #709 / #1361 (single-table SQL export/import). This transfers rows directly and does not overlap with them.
Demo
Screen.Recording.2026-11-06.at.1.30.39.PM.mov
Limitations (by design for this milestone)
interval,char(n)are reported as unsupported and block the plan.Impact analysis
GitNexus was not available in my environment, so I did a manual caller search for the existing symbols I touched:
DatabaseTransferCommand(+4 variants; callers:api.rs,rpc.rs,rpc_tests.rs),DumpCancellationState(+1 field; one struct literal updated),cancel_session_jobs,emit_progress(nowpub(crate)),authorize_saved_access, webauthorization_for. No behaviour change for dump/import.Testing
cargo test --libwith a local PostgreSQL 16 on127.0.0.1:54320(same fixture astests/postgres_integration; the transfer tests skip when it isn't reachable): real PostgreSQL → SQLite and PostgreSQL → PostgreSQL transfers across batches (exact values, NUL bytes inbytea, Unicode, numeric(30,10), timestamptz, jsonb), duplicate handling, skip mode, lossy rejection with nothing written, cancellation keeping committed batches, missing target table never created.pnpm typecheck,pnpm lint,pnpm test(8157 passing; 14 new).persists_and_deletes_custom_themes_in_the_shared_config_directory,announces_committed_personal_theme_changes) also fail on a cleanmain.Open in [code]smith · Need help on this PR? Tag
@codesmith-botwith what you need.