From e23d30f927a27ee4faac6b9abaed4408e6749a7a Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 3 Nov 2026 08:25:28 +0000 Subject: [PATCH 1/3] Fix five more bugs: phone DB pushes on reads, JSON export, json/xml edits, env bools, SQLite row counts - On-device SQLite pushes the file back only when a statement changed it (total_changes / schema_version / write keyword), once per transaction. - JSON export writes DateTime/BigInt as text and keeps repeated columns. - Postgres cell edits on json and xml columns compare as jsonb / text. - DBSCOPE_AUTO_READ_ONLY and DBSCOPE_AUTO_ASSIGN_IP accept 1/0, yes/no. - SqliteExecutor reports 0 affected rows for DDL. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01EqzDdWjzbRhR11uqy5k4wL --- BUGS.md | 12 +- CHANGELOG.md | 5 + docs/architecture.md | 2 +- lib/connection/cli_auto_connect.dart | 28 ++++- lib/db/dml.dart | 27 ++++- lib/db/sqlite_executor.dart | 16 ++- lib/grid/export_io.dart | 15 +-- lib/grid/export_json.dart | 24 ++++ lib/grid/export_web.dart | 13 +- .../lib/dbscope_sqflite_plugin.dart | 1 + .../lib/src/executor.dart | 93 +++++++++----- pubspec.lock | 2 +- pubspec.yaml | 3 + test/cli_auto_connect_test.dart | 37 ++++++ test/device_sqlite_push_test.dart | 114 ++++++++++++++++++ test/dml_live_pg_test.dart | 89 ++++++++++++++ test/dml_test.dart | 32 +++++ test/export_json_test.dart | 34 ++++++ test/sqlite_executor_test.dart | 41 +++++++ 19 files changed, 514 insertions(+), 74 deletions(-) create mode 100644 lib/grid/export_json.dart create mode 100644 test/cli_auto_connect_test.dart create mode 100644 test/device_sqlite_push_test.dart create mode 100644 test/dml_live_pg_test.dart create mode 100644 test/export_json_test.dart create mode 100644 test/sqlite_executor_test.dart diff --git a/BUGS.md b/BUGS.md index 7b9597e..4f76dee 100644 --- a/BUGS.md +++ b/BUGS.md @@ -26,13 +26,13 @@ rg -n --max-count 5 '' BUGS.md -### Every SELECT that returns rows on a phone's database pushes the file back to the device +### Editing a point, path or polygon cell on Postgres always fails -- **Where.** `packages/dbscope_sqflite_plugin/lib/src/executor.dart:209-210` (`_writesToFile` returns true when `result.affectedRows > 0`), fed by `lib/db/sqlite_executor.dart` (`_resultFromSelect` reports `affectedRows: rows.length` for every result set, so a read of 3 rows says 3). -- **Failure.** Open a table (or run any query that returns rows) on a USB-connected phone's app database: the read comes back with `affectedRows > 0`, so `DeviceSqliteExecutor.execute` calls `_pushBack()` and copies the whole `.db` back onto the device and deletes its `-wal`/`-shm` after every page of every grid. Each read costs an adb/devicectl round trip, and each push is the exact operation the dialog warns can corrupt the database or lose the app's own writes if the app is running, though the user only read data. -- **Noticed.** 2026-11-03, while wiring the statement timeout through `DeviceSqliteExecutor`. -- **Confidence.** likely (both lines are plain in the code; not run against a phone). -- **Why not fixed on the spot.** Outside the task (the BUGS.md sweep). The fix is to decide "wrote" from the statement (`!PreparedStatement.isReadOnly`, or `_db.totalChanges` before and after) instead of the row count, which needs the inner executor to expose it. +- **Where.** `lib/db/dml.dart` (`CellEditor._postgresGuard`). +- **Failure.** Edit a `point` (also `path`, `polygon`) cell in a table grid: the optimistic guard `"p" IS NOT DISTINCT FROM $3` fails with `operator does not exist: point = unknown`, because these types have no `=` operator. Casting both sides to text doesn't help: the driver decodes the old value into a `Point` whose `toString()` (`Point(1.0,2.0)`) is not Postgres's text form (`(1,2)`), so the guard matches no row and every save reads as a conflict. +- **Noticed.** 2026-11-03, while fixing the same failure for `json` and `xml` columns (checked against a local Postgres 16). +- **Confidence.** confirmed (for `point`, by running the UPDATE; `path` and `polygon` by the Postgres docs). +- **Why not fixed on the spot.** Needs a decision: compare with the type's `~=` "same as" operator, render the driver's geometric objects back to Postgres text, or make these columns read-only in the grid. ## Accepted limitations diff --git a/CHANGELOG.md b/CHANGELOG.md index 47b78c7..7a1e91b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,11 @@ ## 2026-11-03 +- SQLite reports 0 rows affected for DDL instead of the count from the previous write (2026-11-03) +- DBSCOPE_AUTO_READ_ONLY (and DBSCOPE_AUTO_ASSIGN_IP) accept 1/0, yes/no, on/off in any case; =1 used to open the connection read-write (2026-11-03) +- Cells in json and xml columns can be edited on Postgres; the save used to fail every time with "operator does not exist" (2026-11-03) +- JSON export of a query result no longer fails on date and timestamp columns, and no longer drops a column whose name repeats (2026-11-03) +- On a phone's app database, reads no longer push the whole file back to the device; only statements that change the file do, and writes inside BEGIN … COMMIT push once at the COMMIT (2026-11-03) - Fix Postgres query result headers showing Type(oid:N) instead of the type name (2026-11-03) - Fix job tabs showing a fast job as running forever when it finished before the tab subscribed (2026-11-03) - Fix one window closing a stream (job progress, profile or metadata updates) belonging to another window (2026-11-03) diff --git a/docs/architecture.md b/docs/architecture.md index 007fe8d..ca3c617 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -137,7 +137,7 @@ dbscope supports more than one kind of database backend. The core never talks to - Profile-kind id on disk stays `'sqflite_remote'` (unchanged from the prior VM-service implementation) so `ConnectionKindRegistry` lookups still match; profile *extras* are different (`deviceKind`, `deviceId`, `appId`, `dbPath`) so old VM-service profiles must be re-saved. - Add-Connection dialog (`profile_dialog.dart`) is a three-step picker: device → app → on-device .db file. Single-device / single-DB cases auto-select. Apps are filtered to "has reachable data" via `androidAppHasData` / `iosAppHasData`. Files are filtered to `.db`/`.sqlite`/`.sqlite3`/`.db3`, excluding `-wal`/`-shm`/`-journal` sidecars (they're paired automatically at open time). - Open flow (`DeviceSqliteExecutor.open` in `executor.dart`): pull `.db` plus any `-wal` and `-shm` sidecar into a temp dir, run `PRAGMA wal_checkpoint(TRUNCATE)` to fold WAL into the main file, switch to `journal_mode = DELETE` so subsequent local writes don't recreate WAL sidecars, then wrap the resulting `sqlite3.Database` in a regular `SqliteExecutor`. -- Write path: every `execute` is delegated to the inner `SqliteExecutor`, then if the statement was a write (affectedRows > 0, or a non-SELECT keyword), the main `.db` is pushed back to the device and stale `-wal`/`-shm` on device are deleted. Push uses `adb push /data/local/tmp/ + run-as cp` on Android (because `adb push` can't write directly into `/data/data//`) and `devicectl device copy to` on iOS — see `Backend.push` in `packages/app_data_explorer/lib/app_data_explorer.dart`. +- Write path: every `execute` is delegated to the inner `SqliteExecutor`, then if the statement changed the file (SQLite's `total_changes()` or `schema_version` moved, or a write keyword with no counter such as VACUUM or `PRAGMA x = y`), the main `.db` is pushed back once no transaction is open (a `BEGIN … COMMIT` pushes once, at the COMMIT). The result's `affectedRows` is not used: for a SELECT it is the row count to the device and stale `-wal`/`-shm` on device are deleted. Push uses `adb push /data/local/tmp/ + run-as cp` on Android (because `adb push` can't write directly into `/data/data//`) and `devicectl device copy to` on iOS — see `Backend.push` in `packages/app_data_explorer/lib/app_data_explorer.dart`. - Concurrency caveat shown in the dialog as a banner: the file is a snapshot. If the device app writes between pull and push, those writes are lost; pushing while the app holds the DB open can corrupt it. Users are told to force-stop the app before editing. - The executor reports `kind == kSqliteKind` so `SchemaCatalog` reuses the sqlite branch. Capabilities: `sessionReadOnly: true`, `dml: true`, `transactions: false` (each push is an independent network round-trip, can't span a multi-statement TX), `cancelByClose: false`. - macOS sandbox is **disabled** in `macos/Runner/{DebugProfile,Release}.entitlements` because `Process.run('adb', ...)` / `Process.run('xcrun', ...)` doesn't work inside the sandbox. Do not ship this build to the App Store. diff --git a/lib/connection/cli_auto_connect.dart b/lib/connection/cli_auto_connect.dart index afc42c4..f5df3a3 100644 --- a/lib/connection/cli_auto_connect.dart +++ b/lib/connection/cli_auto_connect.dart @@ -18,7 +18,7 @@ import 'profile.dart'; /// Required: `DBSCOPE_AUTO_HOST`, `DBSCOPE_AUTO_DATABASE`, /// `DBSCOPE_AUTO_USERNAME`. Optional: `DBSCOPE_AUTO_PORT` (default 5432), /// `DBSCOPE_AUTO_PASSWORD`, `DBSCOPE_AUTO_NAME` (the connection's label), -/// `DBSCOPE_AUTO_READ_ONLY` (`true`/`false`). Set +/// `DBSCOPE_AUTO_READ_ONLY` (`true`/`false`, also `1`/`0`, `yes`/`no`). Set /// `DBSCOPE_AUTO_CLOUDSQL_INSTANCE` (`project:region:instance`) to open a /// Cloud SQL profile instead of a plain Postgres one — dbscope then /// launches its own Cloud SQL Auth Proxy (see @@ -31,8 +31,10 @@ import 'profile.dart'; /// own identity (host/port/database/username), so re-running `start_dbscope.sh` /// updates the same saved profile instead of piling up a fresh one in the /// picker every launch. -ConnectionProfile? autoConnectProfileFromEnv() { - final env = processEnvironment(); +/// +/// [environment] defaults to the process environment (tests pass their own). +ConnectionProfile? autoConnectProfileFromEnv([Map? environment]) { + final env = environment ?? processEnvironment(); final host = env['DBSCOPE_AUTO_HOST']; if (host == null || host.isEmpty) return null; final database = env['DBSCOPE_AUTO_DATABASE'] ?? 'postgres'; @@ -40,8 +42,7 @@ ConnectionProfile? autoConnectProfileFromEnv() { final port = int.tryParse(env['DBSCOPE_AUTO_PORT'] ?? '') ?? 5432; final password = env['DBSCOPE_AUTO_PASSWORD'] ?? ''; final name = env['DBSCOPE_AUTO_NAME'] ?? '$username@$host/$database'; - final readOnlyRaw = env['DBSCOPE_AUTO_READ_ONLY']; - final readOnly = readOnlyRaw == null ? null : readOnlyRaw == 'true'; + final readOnly = parseEnvBool(env['DBSCOPE_AUTO_READ_ONLY']); final id = const Uuid().v5( Namespace.url.value, 'dbscope-cli:$host:$port:$database:$username', @@ -67,7 +68,7 @@ ConnectionProfile? autoConnectProfileFromEnv() { gcpProject: env['DBSCOPE_AUTO_GCP_PROJECT'] ?? segments.first, instanceName: segments.last, localPort: port, - assignPublicIp: (env['DBSCOPE_AUTO_ASSIGN_IP'] ?? 'true') != 'false', + assignPublicIp: parseEnvBool(env['DBSCOPE_AUTO_ASSIGN_IP']) ?? true, ); return ConnectionProfile( id: id, @@ -82,3 +83,18 @@ ConnectionProfile? autoConnectProfileFromEnv() { extra: cloudSql.toExtra(), ); } + +/// `true`/`1`/`yes`/`on` and `false`/`0`/`no`/`off`, in any case; null for +/// anything else (unset, empty, a typo), so the profile's read-only +/// heuristic decides. A strict `== 'true'` read `DBSCOPE_AUTO_READ_ONLY=1` +/// or `=TRUE` as read-write, on the variable people set for production. +bool? parseEnvBool(String? raw) { + switch (raw?.trim().toLowerCase()) { + case 'true' || '1' || 'yes' || 'on': + return true; + case 'false' || '0' || 'no' || 'off': + return false; + default: + return null; + } +} diff --git a/lib/db/dml.dart b/lib/db/dml.dart index 95a6d14..34e6032 100644 --- a/lib/db/dml.dart +++ b/lib/db/dml.dart @@ -33,25 +33,42 @@ class CellEditor { final oldPlaceholder = '\$${params.length}'; final qualified = quoteQualified(policy.schema, policy.table); - // SQLite before 3.39 (Android 13 and older phones) has no - // `IS NOT DISTINCT FROM`; its `IS` is the same NULL-safe comparison and - // works on every version. - final nullSafeEq = conn.kind == kSqliteKind ? 'IS' : 'IS NOT DISTINCT FROM'; // CURRENT_TIMESTAMP rather than now(): it works on Postgres and SQLite. final touches = [ for (final touchColumn in policy.touchColumns) if (touchColumn != policy.column) ', ${quoteIdent(touchColumn)} = CURRENT_TIMESTAMP', ].join(); + // SQLite before 3.39 (Android 13 and older phones) has no + // `IS NOT DISTINCT FROM`; its `IS` is the same NULL-safe comparison and + // works on every version. + final guard = conn.kind == kSqliteKind + ? '${quoteIdent(policy.column)} IS $oldPlaceholder' + : _postgresGuard(policy, oldPlaceholder); final sql = 'UPDATE $qualified ' 'SET ${quoteIdent(policy.column)} = \$1$touches ' 'WHERE ${pkPlaceholders.join(' AND ')} ' - 'AND ${quoteIdent(policy.column)} $nullSafeEq $oldPlaceholder'; + 'AND $guard'; final result = await conn.execute(sql, positional: params); return UpdateResult(affectedRows: result.affectedRows, newValue: parsed); } + /// The optimistic-concurrency guard on Postgres: the column still holds + /// the value the user saw. `json` and `xml` have no `=` operator, so a + /// plain `IS NOT DISTINCT FROM` fails every edit of such a column + /// ("operator does not exist: json = unknown"); compare them as `jsonb` + /// (key order and whitespace don't count as a change) and as text. + static String _postgresGuard(EditPolicy policy, String oldPlaceholder) { + final column = quoteIdent(policy.column); + final cast = switch (policy.typeName.toLowerCase()) { + 'json' => '::jsonb', + 'xml' => '::text', + _ => '', + }; + return '$column$cast IS NOT DISTINCT FROM $oldPlaceholder$cast'; + } + Future deleteRow({ required String schema, required String table, diff --git a/lib/db/sqlite_executor.dart b/lib/db/sqlite_executor.dart index a4f7d65..bd6fcc4 100644 --- a/lib/db/sqlite_executor.dart +++ b/lib/db/sqlite_executor.dart @@ -117,6 +117,7 @@ class SqliteExecutor implements DbExecutor, StatementTimeoutCapable { // SELECT) through here when the SQL doesn't carry placeholders. For // DML we need `_db.updatedRows` to surface the real affected count; // for SELECT we fall back to the row count from the result set. + final changesBefore = _totalChanges(); final result = _db.select(sql); if (cancelToken?.isCancelled ?? false) { return const DbResult(columns: [], rows: []); @@ -126,7 +127,7 @@ class SqliteExecutor implements DbExecutor, StatementTimeoutCapable { return DbResult( columns: base.columns, rows: base.rows, - affectedRows: _db.updatedRows, + affectedRows: _changedRows(changesBefore), ); } return base; @@ -139,6 +140,7 @@ class SqliteExecutor implements DbExecutor, StatementTimeoutCapable { CancelToken? cancelToken, }) { final statement = _db.prepare(sql); + final changesBefore = _totalChanges(); try { // Always step through `select`: it is safe for statements that return // no rows, and it never throws a result set away. Guessing from the @@ -156,7 +158,7 @@ class SqliteExecutor implements DbExecutor, StatementTimeoutCapable { return DbResult( columns: const [], rows: const [], - affectedRows: _db.updatedRows, + affectedRows: _changedRows(changesBefore), ); } return _resultFromSelect(resultSet); @@ -165,6 +167,16 @@ class SqliteExecutor implements DbExecutor, StatementTimeoutCapable { } } + int _totalChanges() => + (_db.select('SELECT total_changes()').first.columnAt(0) as int?) ?? 0; + + /// Rows the statement just run inserted, updated or deleted. + /// `sqlite3_changes()` alone keeps the count of the last INSERT / UPDATE / + /// DELETE, so a `CREATE TABLE` after an UPDATE of 5 rows reported 5; it + /// only counts when this statement changed something. + int _changedRows(int totalChangesBefore) => + _totalChanges() == totalChangesBefore ? 0 : _db.updatedRows; + DbResult _resultFromSelect(sq.ResultSet resultSet) { final columns = []; for (var i = 0; i < resultSet.columnNames.length; i++) { diff --git a/lib/grid/export_io.dart b/lib/grid/export_io.dart index a3e0258..25391da 100644 --- a/lib/grid/export_io.dart +++ b/lib/grid/export_io.dart @@ -1,4 +1,3 @@ -import 'dart:convert'; import 'dart:io'; import 'package:file_selector/file_selector.dart'; @@ -7,6 +6,7 @@ import 'package:sqlite3/sqlite3.dart' as sq; import '../catalog/pg_type.dart'; import '../grid/data_grid.dart'; import '../util/sql_quote.dart'; +import 'export_json.dart'; import 'export_names.dart'; class ResultExporter { @@ -56,17 +56,10 @@ class ResultExporter { ], ); if (location == null) return null; - final colNames = data.columns.map((column) => column.name).toList(); - final out = >[]; - for (final row in data.rows) { - final entry = {}; - for (var i = 0; i < colNames.length; i++) { - entry[colNames[i]] = row[i]; - } - out.add(entry); - } final file = File(location.path); - await file.writeAsString(const JsonEncoder.withIndent(' ').convert(out)); + await file.writeAsString( + encodeRowsAsJson([for (final c in data.columns) c.name], data.rows), + ); return location.path; } diff --git a/lib/grid/export_json.dart b/lib/grid/export_json.dart new file mode 100644 index 0000000..c9078b4 --- /dev/null +++ b/lib/grid/export_json.dart @@ -0,0 +1,24 @@ +import 'dart:convert'; + +import 'export_names.dart'; + +/// The JSON export of a result set: an array with one object per row. +/// +/// Values the Postgres driver hands back that `jsonEncode` can't take on +/// its own (a `DateTime` for every date and timestamp column, `BigInt`, +/// intervals, geometric types) are written as text, so exporting any +/// result with a timestamp column doesn't fail. Repeated column names +/// (`SELECT a.id, b.id …`) get suffixes, so no column overwrites another. +String encodeRowsAsJson(List columnNames, List> rows) { + final keys = uniqueColumnNames(columnNames); + final out = [ + for (final row in rows) + {for (var i = 0; i < keys.length; i++) keys[i]: i < row.length ? row[i] : null}, + ]; + return const JsonEncoder.withIndent(' ', _toEncodable).convert(out); +} + +Object? _toEncodable(Object? value) { + if (value is DateTime) return value.toUtc().toIso8601String(); + return value.toString(); +} diff --git a/lib/grid/export_web.dart b/lib/grid/export_web.dart index 97d887b..5dd16bc 100644 --- a/lib/grid/export_web.dart +++ b/lib/grid/export_web.dart @@ -4,11 +4,11 @@ // disk. There is no sqlite export: [ResultExporter.supportsSqliteExport] is // false and the UI hides the `.db` button. -import 'dart:convert'; import 'package:flutter/services.dart'; import '../grid/data_grid.dart'; +import 'export_json.dart'; class ResultExporter { /// Whether [exportSqlite] can write a file on this platform. @@ -35,18 +35,9 @@ class ResultExporter { } static Future exportJson(GridData data) async { - final colNames = data.columns.map((column) => column.name).toList(); - final out = >[]; - for (final row in data.rows) { - final entry = {}; - for (var i = 0; i < colNames.length; i++) { - entry[colNames[i]] = row[i]; - } - out.add(entry); - } await Clipboard.setData( ClipboardData( - text: const JsonEncoder.withIndent(' ').convert(out), + text: encodeRowsAsJson([for (final c in data.columns) c.name], data.rows), ), ); return 'clipboard'; diff --git a/packages/dbscope_sqflite_plugin/lib/dbscope_sqflite_plugin.dart b/packages/dbscope_sqflite_plugin/lib/dbscope_sqflite_plugin.dart index de5c342..1659539 100644 --- a/packages/dbscope_sqflite_plugin/lib/dbscope_sqflite_plugin.dart +++ b/packages/dbscope_sqflite_plugin/lib/dbscope_sqflite_plugin.dart @@ -17,4 +17,5 @@ // `lib/main.dart` // 4. delete any saved profiles of this kind from the connection picker. +export 'src/executor.dart' show DeviceSqliteExecutor; export 'src/plugin.dart' show SqfliteRemoteKindPlugin, SqfliteRemotePlugin, kSqfliteRemoteKind; diff --git a/packages/dbscope_sqflite_plugin/lib/src/executor.dart b/packages/dbscope_sqflite_plugin/lib/src/executor.dart index 22760f3..bc4fcae 100644 --- a/packages/dbscope_sqflite_plugin/lib/src/executor.dart +++ b/packages/dbscope_sqflite_plugin/lib/src/executor.dart @@ -5,9 +5,10 @@ // into the main file, set journal_mode=DELETE so subsequent local // writes don't recreate WAL sidecars, then wrap a `package:sqlite3` // Database in a regular SqliteExecutor. -// - Execute: delegate to the inner executor. If the statement was a -// write (affectedRows > 0, or any non-SELECT), push the file back to -// the device and delete any stale -wal/-shm sidecars there. +// - Execute: delegate to the inner executor. If the statement changed +// the file (rows or schema changed, or a non-read keyword such as +// VACUUM), push the file back to the device once no transaction is +// open, and delete any stale -wal/-shm sidecars there. // - Close: dispose the inner executor and remove the temp dir. // // Concurrency caveat (documented in the profile dialog too): the file is @@ -22,6 +23,7 @@ import 'package:dbscope/db/executor.dart'; import 'package:dbscope/db/sqlite_executor.dart'; import 'package:dbscope/db/statement_timeout.dart'; import 'package:dbscope/query/cancel_token.dart'; +import 'package:dbscope/util/sql_parse.dart'; import 'package:sqlite3/sqlite3.dart' as sq; class DeviceSqliteExecutor implements DbExecutor, StatementTimeoutCapable { @@ -46,6 +48,9 @@ class DeviceSqliteExecutor implements DbExecutor, StatementTimeoutCapable { final String _localDbPath; var _closed = false; + /// The local file has changes the device hasn't received yet. + var _unpushed = false; + /// Pulls [remoteDbPath] (and best-effort its `-wal` sidecar — never the /// `-shm`, sqlite always rebuilds it) into a fresh temp directory, then /// opens it. Pulling .db and -wal is inherently non-atomic over an adb @@ -146,14 +151,21 @@ class DeviceSqliteExecutor implements DbExecutor, StatementTimeoutCapable { if (_closed) { throw StateError('DeviceSqliteExecutor is closed.'); } + final changesBefore = _totalChanges(); + final schemaBefore = _schemaVersion(); final result = await _inner.execute( sql, positional: positional, named: named, cancelToken: cancelToken, ); - if (_writesToFile(sql, result)) { + if (_changedFile(sql, changesBefore, schemaBefore)) _unpushed = true; + // Inside an open transaction (a `BEGIN` from a Query tab) the change + // isn't committed yet: pushing now would send the device a file the + // ROLLBACK may still undo. Push once the transaction has ended. + if (_unpushed && _db.autocommit) { await _pushBack(); + _unpushed = false; } return result; } @@ -202,36 +214,55 @@ class DeviceSqliteExecutor implements DbExecutor, StatementTimeoutCapable { } } - /// Best-effort detection of "this statement mutated the file". SELECT, - /// EXPLAIN, PRAGMA-as-query, etc. don't need a push. DDL (CREATE/DROP) - /// reports affectedRows = 0 but does mutate, so we also peek at the - /// statement keyword. - static bool _writesToFile(String sql, DbResult result) { - if (result.affectedRows > 0) return true; - final trimmed = _strip(sql).toUpperCase(); - if (trimmed.startsWith('SELECT')) return false; - if (trimmed.startsWith('WITH')) return false; - if (trimmed.startsWith('EXPLAIN')) return false; - if (trimmed.startsWith('VALUES')) return false; - if (trimmed.startsWith('PRAGMA')) { - // Some PRAGMAs mutate (e.g. user_version = N, journal_mode = X). - // Detect by presence of '='. - return trimmed.contains('='); - } - // Anything else (CREATE/ALTER/DROP/INSERT/UPDATE/DELETE/REPLACE/ - // VACUUM/REINDEX/...) is a mutation. - return true; + /// `total_changes()` counts every row INSERT / UPDATE / DELETE on this + /// connection, including ones inside a `WITH … AS (DELETE …)`. + int _totalChanges() => + (_db.select('SELECT total_changes()').first.columnAt(0) as int?) ?? 0; + + /// Bumped by every schema change (CREATE / ALTER / DROP). + int _schemaVersion() => + (_db.select('PRAGMA schema_version').first.columnAt(0) as int?) ?? 0; + + /// Whether [sql] changed the local file, so the device needs it back. + /// + /// Decided from what the statement did, not from the result's row + /// count: the inner executor reports a SELECT's row count as + /// `affectedRows`, so reading three rows looked like three writes and + /// pushed the whole file to the phone after every grid page. Row and + /// schema changes are counted by SQLite itself; the leading keyword only + /// covers writes neither counter sees (VACUUM, `PRAGMA user_version = 2`). + bool _changedFile(String sql, int changesBefore, int schemaBefore) { + if (_totalChanges() != changesBefore) return true; + if (_schemaVersion() != schemaBefore) return true; + return keywordMayWrite(sql); } - static String _strip(String sql) { - var remaining = sql.trimLeft(); - // Strip single-line comments. - while (remaining.startsWith('--')) { - final nl = remaining.indexOf('\n'); - if (nl < 0) return ''; - remaining = remaining.substring(nl + 1).trimLeft(); + /// Statements that may write without counting a changed row or a schema + /// change. Reads, and transaction control (whose writes were counted by + /// the statements inside the transaction), are false. + static bool keywordMayWrite(String sql) { + final words = bareWords(sql, dialect: SqlDialect.sqlite); + if (words.isEmpty) return false; + switch (words.first) { + case 'SELECT': + case 'WITH': + case 'EXPLAIN': + case 'VALUES': + case 'BEGIN': + case 'COMMIT': + case 'END': + case 'ROLLBACK': + case 'SAVEPOINT': + case 'RELEASE': + return false; + case 'PRAGMA': + // Some PRAGMAs write (`user_version = N`); a read has no `=`. + return stripCommentsAndLeading(sql, dialect: SqlDialect.sqlite).contains('='); + default: + // INSERT/UPDATE/DELETE/REPLACE/CREATE/DROP/ALTER/VACUUM/REINDEX/ + // ANALYZE (writes sqlite_stat1 without counting a change)/… + return true; } - return remaining; } static String _basename(String path) { diff --git a/pubspec.lock b/pubspec.lock index 63a5d17..4f1949d 100644 --- a/pubspec.lock +++ b/pubspec.lock @@ -10,7 +10,7 @@ packages: source: hosted version: "0.2.2" app_data_explorer: - dependency: transitive + dependency: "direct dev" description: path: "packages/app_data_explorer" relative: true diff --git a/pubspec.yaml b/pubspec.yaml index 915a084..fe7ee66 100644 --- a/pubspec.yaml +++ b/pubspec.yaml @@ -58,6 +58,9 @@ dependencies: yaml: ^3.1.4 dev_dependencies: + # Tests fake the on-device `Backend` from this package. + app_data_explorer: + path: packages/app_data_explorer flutter_lints: ^6.0.0 flutter_test: sdk: flutter diff --git a/test/cli_auto_connect_test.dart b/test/cli_auto_connect_test.dart new file mode 100644 index 0000000..b0ffc33 --- /dev/null +++ b/test/cli_auto_connect_test.dart @@ -0,0 +1,37 @@ +// Contract: autoConnectProfileFromEnv reads DBSCOPE_AUTO_* variables. +// * No DBSCOPE_AUTO_HOST, no profile. +// * DBSCOPE_AUTO_READ_ONLY accepts true/1/yes/on and false/0/no/off in any +// case. Anything else leaves it to the name heuristic, never to +// read-write: `=1` on a production profile must not open it writable. + +import 'package:dbscope/connection/cli_auto_connect.dart'; +import 'package:flutter_test/flutter_test.dart'; + +void main() { + Map env([Map extra = const {}]) => { + 'DBSCOPE_AUTO_HOST': 'db.example.com', + 'DBSCOPE_AUTO_NAME': 'reporting', + ...extra, + }; + + test('no host, no profile', () { + expect(autoConnectProfileFromEnv(const {}), isNull); + }); + + test('READ_ONLY=1 and =TRUE open read-only', () { + for (final value in ['1', 'TRUE', 'true', 'Yes', 'on']) { + expect(autoConnectProfileFromEnv(env({'DBSCOPE_AUTO_READ_ONLY': value}))!.readOnly, isTrue, reason: value); + } + }); + + test('READ_ONLY=0 and =false open read-write', () { + for (final value in ['0', 'false', 'NO', 'off']) { + expect(autoConnectProfileFromEnv(env({'DBSCOPE_AUTO_READ_ONLY': value}))!.readOnly, isFalse, reason: value); + } + }); + + test('an unrecognised READ_ONLY value falls back to the name heuristic', () { + final prod = autoConnectProfileFromEnv(env({'DBSCOPE_AUTO_NAME': 'prod', 'DBSCOPE_AUTO_READ_ONLY': 'y3s'}))!; + expect(prod.readOnly, isTrue); + }); +} diff --git a/test/device_sqlite_push_test.dart b/test/device_sqlite_push_test.dart new file mode 100644 index 0000000..900bf9a --- /dev/null +++ b/test/device_sqlite_push_test.dart @@ -0,0 +1,114 @@ +// Contract: DeviceSqliteExecutor (on-device SQLite, pull-edit-push) pushes +// the local file back to the phone only when a statement changed it, and +// only once no transaction is open. +// * A SELECT that returns rows never pushes (it used to: the row count was +// read as "rows affected"). +// * Row writes (also inside a WITH, or an INSERT … RETURNING), schema +// changes and keyword-only writes (PRAGMA user_version = N) push once. +// * Writes inside BEGIN … COMMIT push once, at the COMMIT. + +import 'dart:io'; + +import 'package:app_data_explorer/app_data_explorer.dart'; +import 'package:dbscope_sqflite_plugin/dbscope_sqflite_plugin.dart'; +import 'package:flutter_test/flutter_test.dart'; +import 'package:sqlite3/sqlite3.dart' as sq; + +/// A phone whose app container is a local directory. Counts pushes. +class _FakeDevice implements Backend { + _FakeDevice(this.root); + final Directory root; + var pushes = 0; + + File _file(String rel) => File('${root.path}/$rel'); + + @override + String get deviceLabel => 'fake'; + @override + String get appLabel => 'app'; + @override + Future> ls(String relPath) async => const []; + @override + Future> lsRecursive(String relPath) async => const []; + @override + Future clearDir(String relDir) async {} + @override + Future rm(String relPath) async { + final file = _file(relPath); + if (file.existsSync()) file.deleteSync(); + } + + @override + Future pull(String remoteRelPath, String localPath) async { + final file = _file(remoteRelPath); + if (!file.existsSync()) throw const FileSystemException('absent'); + file.copySync(localPath); + } + + @override + Future push(String localPath, String remoteRelPath) async { + pushes++; + File(localPath).copySync(_file(remoteRelPath).path); + } +} + +void main() { + late Directory root; + late _FakeDevice device; + late DeviceSqliteExecutor executor; + + setUp(() async { + root = Directory.systemTemp.createTempSync('dbscope_fake_device_'); + final db = sq.sqlite3.open('${root.path}/app.db'); + db.execute('CREATE TABLE t (id INTEGER PRIMARY KEY, name TEXT)'); + db.execute("INSERT INTO t (name) VALUES ('a'), ('b'), ('c')"); + db.dispose(); + device = _FakeDevice(root); + executor = await DeviceSqliteExecutor.open(backend: device, remoteDbPath: 'app.db'); + }); + + tearDown(() async { + await executor.close(); + root.deleteSync(recursive: true); + }); + + test('a SELECT that returns rows does not push', () async { + final result = await executor.execute('SELECT * FROM t'); + expect(result.rows, hasLength(3)); + await executor.execute(r'SELECT * FROM t WHERE id > $1', positional: [1]); + await executor.execute('/* c */ SELECT 1'); + await executor.execute('PRAGMA table_info(t)'); + expect(device.pushes, 0); + }); + + test('an UPDATE pushes once and the device gets the change', () async { + await executor.execute(r'UPDATE t SET name = $1 WHERE id = $2', positional: ['z', 1]); + expect(device.pushes, 1); + final onDevice = sq.sqlite3.open('${root.path}/app.db'); + addTearDown(onDevice.dispose); + expect(onDevice.select('SELECT name FROM t WHERE id = 1').first['name'], 'z'); + }); + + test('a write inside WITH, and INSERT RETURNING, push', () async { + await executor.execute('WITH x AS (SELECT 1) DELETE FROM t WHERE id = 3'); + expect(device.pushes, 1); + await executor.execute(r'INSERT INTO t (name) VALUES ($1) RETURNING id', positional: ['d']); + expect(device.pushes, 2); + }); + + test('schema changes and writing pragmas push', () async { + await executor.execute('CREATE TABLE u (id INTEGER)'); + expect(device.pushes, 1); + await executor.execute('PRAGMA user_version = 7'); + expect(device.pushes, 2); + }); + + test('writes inside a transaction push once, at COMMIT', () async { + await executor.execute('BEGIN'); + await executor.execute("INSERT INTO t (name) VALUES ('x')"); + await executor.execute("INSERT INTO t (name) VALUES ('y')"); + expect(device.pushes, 0); + await executor.execute('COMMIT'); + expect(device.pushes, 1); + }); +} diff --git a/test/dml_live_pg_test.dart b/test/dml_live_pg_test.dart new file mode 100644 index 0000000..f51d5ea --- /dev/null +++ b/test/dml_live_pg_test.dart @@ -0,0 +1,89 @@ +// Cell edits against a real Postgres, for the column types whose +// optimistic-concurrency guard can't be checked by looking at the SQL: +// `json` and `xml` have no `=` operator, so the guard must still parse and +// match the value the driver handed back when the row was read. Skipped +// unless DBSCOPE_LIVE_PG is set: +// +// DBSCOPE_LIVE_PG=1 PGHOST=localhost PGUSER=postgres PGPASSWORD=… \ +// fvm flutter test test/dml_live_pg_test.dart +// +// Creates and drops a temporary table; touches nothing else. + +import 'dart:io'; + +import 'package:dbscope/catalog/pg_type.dart'; +import 'package:dbscope/db/dml.dart'; +import 'package:dbscope/db/edit_policy.dart'; +import 'package:dbscope/db/postgres_executor.dart'; +import 'package:dbscope/db/postgres_plugin.dart'; +import 'package:flutter_test/flutter_test.dart'; + +String _env(String name, String fallback) => Platform.environment[name] ?? fallback; + +EditPolicy _policy(String column, PgType type, String typeName) => EditPolicy( + schema: 'pg_temp', + table: 'dbscope_dml_live', + pkColumns: const ['id'], + column: column, + type: type, + typeName: typeName, + nullable: true, + isGenerated: false, + isIdentity: false, + readOnly: false, + multiLine: false, + nullIfBlank: false, +); + +void main() { + final enabled = Platform.environment['DBSCOPE_LIVE_PG'] != null; + + group('live Postgres cell edits', skip: !enabled, () { + late PostgresExecutor executor; + + setUpAll(() async { + executor = PostgresExecutor( + await PostgresKindPlugin.openConnection( + host: _env('PGHOST', 'localhost'), + port: int.parse(_env('PGPORT', '5432')), + database: _env('PGDATABASE', 'postgres'), + username: _env('PGUSER', 'postgres'), + password: _env('PGPASSWORD', ''), + ), + ); + await executor.execute( + 'CREATE TEMP TABLE dbscope_dml_live (id int PRIMARY KEY, doc json, x xml)', + ); + await executor.execute( + '''INSERT INTO dbscope_dml_live VALUES (1, '{"a": 1}', '')''', + ); + }); + + tearDownAll(() => executor.close()); + + Future read(String column) async => + (await executor.execute('SELECT $column FROM dbscope_dml_live WHERE id = 1')).rows.first.first; + + test('a json cell edit matches the value read back and saves', () async { + final result = await CellEditor(executor).updateCell( + policy: _policy('doc', PgType.json, 'json'), + pkValues: {'id': 1}, + oldValue: await read('doc'), + newValueRaw: '{"a": 2}', + ); + expect(result.affectedRows, 1); + expect(await read('doc'), {'a': 2}); + }); + + test('an xml cell edit matches the value read back and saves', () async { + final result = await CellEditor(executor).updateCell( + policy: _policy('x', PgType.unknown, 'xml'), + pkValues: {'id': 1}, + oldValue: await read('x'), + newValueRaw: '', + ); + expect(result.affectedRows, 1); + expect(await read('x'), ''); + }); + }); +} diff --git a/test/dml_test.dart b/test/dml_test.dart index b45dc5c..18ac012 100644 --- a/test/dml_test.dart +++ b/test/dml_test.dart @@ -4,6 +4,8 @@ // `IS NOT DISTINCT FROM $oldValue` (docs/architecture.md, SQL safety invariants), // spelled `IS $oldValue` on SQLite, which before 3.39 (older phones) has no // IS NOT DISTINCT FROM. +// * json and xml have no `=` on Postgres, so their guard compares as +// jsonb and as text (a bare IS NOT DISTINCT FROM fails every edit). // * Soft delete stamps CURRENT_TIMESTAMP, which Postgres and SQLite share. // * Identifiers go through quoteIdent — no raw interpolation of user // column/table names. @@ -92,6 +94,36 @@ void main() { expect(call.positional, ['6', 't1', 'u9', 5]); }); + test('compares a json column as jsonb, which has an equality operator', () async { + final executor = FakeDbExecutor(); + executor.enqueueAffected(1); + await CellEditor(executor).updateCell( + policy: _policy(column: 'doc', type: PgType.json, typeName: 'json'), + pkValues: {'id': 1}, + oldValue: {'a': 1}, + newValueRaw: '{"a": 2}', + ); + expect( + executor.calls.single.sql, + contains(r'AND "doc"::jsonb IS NOT DISTINCT FROM $3::jsonb'), + ); + }); + + test('compares an xml column as text', () async { + final executor = FakeDbExecutor(); + executor.enqueueAffected(1); + await CellEditor(executor).updateCell( + policy: _policy(column: 'x', typeName: 'xml', type: PgType.unknown), + pkValues: {'id': 1}, + oldValue: '', + newValueRaw: '', + ); + expect( + executor.calls.single.sql, + contains(r'AND "x"::text IS NOT DISTINCT FROM $3::text'), + ); + }); + test('escapes embedded double quotes in identifiers', () async { final executor = FakeDbExecutor(); executor.enqueueAffected(1); diff --git a/test/export_json_test.dart b/test/export_json_test.dart new file mode 100644 index 0000000..194ddf6 --- /dev/null +++ b/test/export_json_test.dart @@ -0,0 +1,34 @@ +// Contract: encodeRowsAsJson (the JSON result export) never fails on a +// value the database driver returns, and never drops a column. +// * DateTime is written as a UTC ISO-8601 string, other non-JSON values +// (BigInt, driver objects) as their text. Bytes stay an array of ints. +// * Repeated column names get suffixes instead of overwriting each other. + +import 'dart:convert'; +import 'dart:typed_data'; + +import 'package:dbscope/grid/export_json.dart'; +import 'package:flutter_test/flutter_test.dart'; + +void main() { + test('writes timestamps and big integers instead of failing', () { + final json = encodeRowsAsJson( + ['at', 'raw', 'big', 'doc'], + [ + [DateTime.utc(2026, 1, 2, 3, 4, 5), Uint8List.fromList([1, 2, 3]), BigInt.parse('123456789012345678901234567890'), {'a': 1}], + ], + ); + expect(jsonDecode(json), [ + {'at': '2026-01-02T03:04:05.000Z', 'raw': [1, 2, 3], 'big': '123456789012345678901234567890', 'doc': {'a': 1}}, + ]); + }); + + test('keeps every column when two share a name', () { + final json = encodeRowsAsJson(['id', 'id'], [ + [1, 2], + ]); + expect(jsonDecode(json), [ + {'id': 1, 'id_2': 2}, + ]); + }); +} diff --git a/test/sqlite_executor_test.dart b/test/sqlite_executor_test.dart new file mode 100644 index 0000000..aaea677 --- /dev/null +++ b/test/sqlite_executor_test.dart @@ -0,0 +1,41 @@ +// Contract: SqliteExecutor reports affectedRows for the statement it just +// ran: the rows an INSERT / UPDATE / DELETE changed, and 0 for a statement +// that changed none (DDL, a no-match UPDATE), never the count left over +// from an earlier write. + +import 'package:dbscope/db/sqlite_executor.dart'; +import 'package:flutter_test/flutter_test.dart'; +import 'package:sqlite3/sqlite3.dart' as sq; + +void main() { + late SqliteExecutor executor; + + setUp(() { + final db = sq.sqlite3.openInMemory(); + executor = SqliteExecutor(db); + db.execute('CREATE TABLE t (id INTEGER PRIMARY KEY, name TEXT)'); + db.execute("INSERT INTO t (name) VALUES ('a'), ('b'), ('c')"); + }); + + tearDown(() => executor.close()); + + test('an UPDATE reports the rows it changed', () async { + final result = await executor.execute("UPDATE t SET name = 'z'"); + expect(result.affectedRows, 3); + }); + + test('DDL after an UPDATE reports 0, not the UPDATE count', () async { + await executor.execute("UPDATE t SET name = 'z'"); + final result = await executor.execute('CREATE TABLE u (id INTEGER)'); + expect(result.affectedRows, 0); + }); + + test('a bound UPDATE that matches nothing reports 0', () async { + await executor.execute("UPDATE t SET name = 'z'"); + final result = await executor.execute( + r'UPDATE t SET name = $1 WHERE id = $2', + positional: ['y', 99], + ); + expect(result.affectedRows, 0); + }); +} From 5d802488a32578c19509314dd41e2f09e1452d25 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 3 Nov 2026 08:32:22 +0000 Subject: [PATCH 2/3] Fix editing geometric cells on Postgres The optimistic guard has no usable = for point/line/lseg/box/path/polygon/ circle, and the driver can't bind most of them back. Send the old value as Postgres text and compare the server's text of both sides. The editor starts from Postgres syntax instead of the driver's toString. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01EqzDdWjzbRhR11uqy5k4wL --- BUGS.md | 8 ------ CHANGELOG.md | 1 + lib/db/dml.dart | 25 ++++++++++++++----- lib/db/pg_geometry.dart | 27 ++++++++++++++++++++ lib/grid/inputs/column_input.dart | 6 ++++- test/dml_live_pg_test.dart | 41 ++++++++++++++++++++++++++++--- test/dml_test.dart | 21 ++++++++++++++-- test/pg_geometry_test.dart | 28 +++++++++++++++++++++ 8 files changed, 136 insertions(+), 21 deletions(-) create mode 100644 lib/db/pg_geometry.dart create mode 100644 test/pg_geometry_test.dart diff --git a/BUGS.md b/BUGS.md index 4f76dee..03ceab1 100644 --- a/BUGS.md +++ b/BUGS.md @@ -26,14 +26,6 @@ rg -n --max-count 5 '' BUGS.md -### Editing a point, path or polygon cell on Postgres always fails - -- **Where.** `lib/db/dml.dart` (`CellEditor._postgresGuard`). -- **Failure.** Edit a `point` (also `path`, `polygon`) cell in a table grid: the optimistic guard `"p" IS NOT DISTINCT FROM $3` fails with `operator does not exist: point = unknown`, because these types have no `=` operator. Casting both sides to text doesn't help: the driver decodes the old value into a `Point` whose `toString()` (`Point(1.0,2.0)`) is not Postgres's text form (`(1,2)`), so the guard matches no row and every save reads as a conflict. -- **Noticed.** 2026-11-03, while fixing the same failure for `json` and `xml` columns (checked against a local Postgres 16). -- **Confidence.** confirmed (for `point`, by running the UPDATE; `path` and `polygon` by the Postgres docs). -- **Why not fixed on the spot.** Needs a decision: compare with the type's `~=` "same as" operator, render the driver's geometric objects back to Postgres text, or make these columns read-only in the grid. - ## Accepted limitations Bugs we decided not to fix. Each one says why, and what would change the decision. diff --git a/CHANGELOG.md b/CHANGELOG.md index bc610a3..f9f5127 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ ## 2026-11-03 +- Cells in point, line, lseg, box, path, polygon and circle columns can be edited on Postgres; the save used to fail every time, and the editor now starts from Postgres syntax like (1,2) (2026-11-03) - SQLite reports 0 rows affected for DDL instead of the count from the previous write (2026-11-03) - DBSCOPE_AUTO_READ_ONLY (and DBSCOPE_AUTO_ASSIGN_IP) accept 1/0, yes/no, on/off in any case; =1 used to open the connection read-write (2026-11-03) - Cells in json and xml columns can be edited on Postgres; the save used to fail every time with "operator does not exist" (2026-11-03) diff --git a/lib/db/dml.dart b/lib/db/dml.dart index 34e6032..131ca9f 100644 --- a/lib/db/dml.dart +++ b/lib/db/dml.dart @@ -4,6 +4,7 @@ import '../catalog/pg_type.dart'; import '../db/executor.dart'; import '../util/sql_quote.dart'; import 'edit_policy.dart'; +import 'pg_geometry.dart'; import 'sqlite_kind.dart'; class UpdateResult { @@ -29,7 +30,11 @@ class CellEditor { params.add(pkValues[primaryKey]); pkPlaceholders.add('${quoteIdent(primaryKey)} = \$${params.length}'); } - params.add(oldValue); + params.add( + conn.kind == kSqliteKind || !pgGeometricTypes.contains(policy.typeName.toLowerCase()) + ? oldValue + : pgGeometryText(oldValue), + ); final oldPlaceholder = '\$${params.length}'; final qualified = quoteQualified(policy.schema, policy.table); @@ -55,13 +60,21 @@ class CellEditor { } /// The optimistic-concurrency guard on Postgres: the column still holds - /// the value the user saw. `json` and `xml` have no `=` operator, so a - /// plain `IS NOT DISTINCT FROM` fails every edit of such a column - /// ("operator does not exist: json = unknown"); compare them as `jsonb` - /// (key order and whitespace don't count as a change) and as text. + /// the value the user saw. `json`, `xml` and the geometric types have no + /// usable `=` operator, so a plain `IS NOT DISTINCT FROM` fails every edit + /// of such a column ("operator does not exist: json = unknown"). + /// - json: compared as `jsonb` (key order and whitespace don't count). + /// - xml: compared as text. + /// - geometric: the old value is sent as Postgres text ([pgGeometryText]), + /// parsed by the server into the column's type and printed back, so + /// both sides are the server's own text form of the value. static String _postgresGuard(EditPolicy policy, String oldPlaceholder) { final column = quoteIdent(policy.column); - final cast = switch (policy.typeName.toLowerCase()) { + final typeName = policy.typeName.toLowerCase(); + if (pgGeometricTypes.contains(typeName)) { + return '$column::text IS NOT DISTINCT FROM ($oldPlaceholder::$typeName)::text'; + } + final cast = switch (typeName) { 'json' => '::jsonb', 'xml' => '::text', _ => '', diff --git a/lib/db/pg_geometry.dart b/lib/db/pg_geometry.dart new file mode 100644 index 0000000..9db7642 --- /dev/null +++ b/lib/db/pg_geometry.dart @@ -0,0 +1,27 @@ +import 'package:postgres/postgres.dart' as pg; + +/// Postgres's geometric types. None of them has a usable `=` (point, path +/// and polygon have none at all; box and circle compare areas), and the +/// driver can't send most of them back as a bind parameter, so the edit +/// guard compares them as Postgres text (see `CellEditor`). +const pgGeometricTypes = {'point', 'line', 'lseg', 'box', 'path', 'polygon', 'circle'}; + +/// [value] in Postgres's input syntax when it is one of the driver's +/// geometric objects (`Point(1.0,2.0)` → `(1.0,2.0)`), else [value] +/// unchanged. Dart's shortest round-trip doubles parse back to the same +/// float8, so the server reads exactly the value it sent. +Object? pgGeometryText(Object? value) => switch (value) { + pg.Point() => _point(value), + pg.Line() => '{${value.a},${value.b},${value.c}}', + pg.LineSegment() => '[${_point(value.p1)},${_point(value.p2)}]', + pg.Box() => '${_point(value.p1)},${_point(value.p2)}', + // Path extends Polygon, so it has to come first. + pg.Path() => value.open ? '[${_points(value)}]' : '(${_points(value)})', + pg.Polygon() => '(${_points(value)})', + pg.Circle() => '<${_point(value.center)},${value.radius}>', + _ => value, +}; + +String _point(pg.Point point) => '(${point.latitude},${point.longitude})'; + +String _points(pg.Polygon polygon) => polygon.points.map(_point).join(','); diff --git a/lib/grid/inputs/column_input.dart b/lib/grid/inputs/column_input.dart index b197d19..360a235 100644 --- a/lib/grid/inputs/column_input.dart +++ b/lib/grid/inputs/column_input.dart @@ -9,6 +9,7 @@ import 'package:widgets/widgets.dart'; import '../../catalog/pg_type.dart'; import '../../db/edit_policy.dart'; +import '../../db/pg_geometry.dart'; part 'parts/bool_input.dart'; part 'parts/date_input.dart'; @@ -188,7 +189,10 @@ class _ColumnInputField extends StatelessWidget { @override Widget build(BuildContext context) { final raw = value; - final asString = raw == null ? null : (raw is String ? raw : raw.toString()); + // Geometric values start out in Postgres's own syntax, so saving an + // untouched `(1,2)` works (the driver's `Point(1.0,2.0)` is not input + // Postgres accepts). + final asString = raw == null ? null : (raw is String ? raw : '${pgGeometryText(raw)}'); if (policy.enumValues != null && policy.enumValues!.isNotEmpty) { return _EnumInput( values: policy.enumValues!, diff --git a/test/dml_live_pg_test.dart b/test/dml_live_pg_test.dart index f51d5ea..6faa20b 100644 --- a/test/dml_live_pg_test.dart +++ b/test/dml_live_pg_test.dart @@ -1,7 +1,8 @@ // Cell edits against a real Postgres, for the column types whose // optimistic-concurrency guard can't be checked by looking at the SQL: -// `json` and `xml` have no `=` operator, so the guard must still parse and -// match the value the driver handed back when the row was read. Skipped +// `json`, `xml` and the geometric types have no usable `=` operator, so the +// guard must still parse and match the value the driver handed back when +// the row was read. Skipped // unless DBSCOPE_LIVE_PG is set: // // DBSCOPE_LIVE_PG=1 PGHOST=localhost PGUSER=postgres PGPASSWORD=… \ @@ -52,10 +53,11 @@ void main() { ), ); await executor.execute( - 'CREATE TEMP TABLE dbscope_dml_live (id int PRIMARY KEY, doc json, x xml)', + 'CREATE TEMP TABLE dbscope_dml_live (id int PRIMARY KEY, doc json, x xml, ' + 'p point, pa path, po polygon, l line, ls lseg, b box, c circle)', ); await executor.execute( - '''INSERT INTO dbscope_dml_live VALUES (1, '{"a": 1}', '')''', + '''INSERT INTO dbscope_dml_live VALUES (1, '{"a": 1}', '', '(1.5,-2)', '[(0,0),(1,1)]', '((0,0),(1,1),(1,0))', '{1,-1,0.25}', '[(0,0),(1,1)]', '((0,0),(1,1))', '<(0,0),1>')''', ); }); @@ -85,5 +87,36 @@ void main() { expect(result.affectedRows, 1); expect(await read('x'), ''); }); + + for (final (column, type, newValue) in const [ + ('p', 'point', '(3,4)'), + ('pa', 'path', '((0,0),(2,2))'), + ('po', 'polygon', '((0,0),(2,2),(2,0))'), + ('l', 'line', '{2,-1,0}'), + ('ls', 'lseg', '[(0,0),(2,2)]'), + ('b', 'box', '(2,2),(0,0)'), + ('c', 'circle', '<(1,1),2>'), + ]) { + test('a $type cell edit matches the value read back and saves', () async { + final editor = CellEditor(executor); + final policy = _policy(column, PgType.unknown, type); + final result = await editor.updateCell( + policy: policy, + pkValues: {'id': 1}, + oldValue: await read(column), + newValueRaw: newValue, + ); + expect(result.affectedRows, 1); + // An old value that no longer matches is a conflict, not a silent + // overwrite. + final stale = await editor.updateCell( + policy: policy, + pkValues: {'id': 1}, + oldValue: null, + newValueRaw: newValue, + ); + expect(stale.affectedRows, 0); + }); + } }); } diff --git a/test/dml_test.dart b/test/dml_test.dart index 18ac012..8646bac 100644 --- a/test/dml_test.dart +++ b/test/dml_test.dart @@ -4,8 +4,10 @@ // `IS NOT DISTINCT FROM $oldValue` (docs/architecture.md, SQL safety invariants), // spelled `IS $oldValue` on SQLite, which before 3.39 (older phones) has no // IS NOT DISTINCT FROM. -// * json and xml have no `=` on Postgres, so their guard compares as -// jsonb and as text (a bare IS NOT DISTINCT FROM fails every edit). +// * json, xml and geometric types have no usable `=` on Postgres, so their +// guard compares as jsonb, as text, and as the server's text of the old +// value sent in Postgres syntax (a bare IS NOT DISTINCT FROM fails every +// edit). test/dml_live_pg_test.dart runs these against a real server. // * Soft delete stamps CURRENT_TIMESTAMP, which Postgres and SQLite share. // * Identifiers go through quoteIdent — no raw interpolation of user // column/table names. @@ -18,6 +20,7 @@ import 'package:dbscope/db/dml.dart'; import 'package:dbscope/db/edit_policy.dart'; import 'package:dbscope/db/sqlite_kind.dart'; import 'package:flutter_test/flutter_test.dart'; +import 'package:postgres/postgres.dart' as pg; import 'support/fake_executor.dart'; @@ -124,6 +127,20 @@ void main() { ); }); + test('compares a point column as server text of the old value', () async { + final executor = FakeDbExecutor(); + executor.enqueueAffected(1); + await CellEditor(executor).updateCell( + policy: _policy(column: 'p', typeName: 'point', type: PgType.unknown), + pkValues: {'id': 1}, + oldValue: const pg.Point(1, 2), + newValueRaw: '(3,4)', + ); + final call = executor.calls.single; + expect(call.sql, contains(r'AND "p"::text IS NOT DISTINCT FROM ($3::point)::text')); + expect(call.positional!.last, '(1.0,2.0)'); + }); + test('escapes embedded double quotes in identifiers', () async { final executor = FakeDbExecutor(); executor.enqueueAffected(1); diff --git a/test/pg_geometry_test.dart b/test/pg_geometry_test.dart new file mode 100644 index 0000000..ec8617f --- /dev/null +++ b/test/pg_geometry_test.dart @@ -0,0 +1,28 @@ +// Contract: pgGeometryText writes the Postgres driver's geometric objects +// in Postgres's input syntax (so the server can parse them back for the +// cell-edit guard and the editor), and leaves every other value alone. + +import 'package:dbscope/db/pg_geometry.dart'; +import 'package:flutter_test/flutter_test.dart'; +import 'package:postgres/postgres.dart' as pg; + +void main() { + test('writes each geometric type in Postgres input syntax', () { + const a = pg.Point(1.5, -2); + const b = pg.Point(0, 0); + expect(pgGeometryText(a), '(1.5,-2.0)'); + expect(pgGeometryText(pg.Line(1, -1, 0.25)), '{1.0,-1.0,0.25}'); + expect(pgGeometryText(pg.LineSegment(a, b)), '[(1.5,-2.0),(0.0,0.0)]'); + expect(pgGeometryText(pg.Box(a, b)), '(1.5,0.0),(0.0,-2.0)'); + expect(pgGeometryText(pg.Path([a, b], open: true)), '[(1.5,-2.0),(0.0,0.0)]'); + expect(pgGeometryText(pg.Path([a, b], open: false)), '((1.5,-2.0),(0.0,0.0))'); + expect(pgGeometryText(pg.Polygon([a, b])), '((1.5,-2.0),(0.0,0.0))'); + expect(pgGeometryText(pg.Circle(b, 2)), '<(0.0,0.0),2.0>'); + }); + + test('leaves other values unchanged', () { + expect(pgGeometryText('x'), 'x'); + expect(pgGeometryText(3), 3); + expect(pgGeometryText(null), isNull); + }); +} From b2e7bf42f44e60a0612d84c833bdae840dfff1a7 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 3 Nov 2026 08:33:41 +0000 Subject: [PATCH 3/3] Use const point lists in the geometry test Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01EqzDdWjzbRhR11uqy5k4wL --- test/pg_geometry_test.dart | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/pg_geometry_test.dart b/test/pg_geometry_test.dart index ec8617f..3567b5f 100644 --- a/test/pg_geometry_test.dart +++ b/test/pg_geometry_test.dart @@ -14,9 +14,9 @@ void main() { expect(pgGeometryText(pg.Line(1, -1, 0.25)), '{1.0,-1.0,0.25}'); expect(pgGeometryText(pg.LineSegment(a, b)), '[(1.5,-2.0),(0.0,0.0)]'); expect(pgGeometryText(pg.Box(a, b)), '(1.5,0.0),(0.0,-2.0)'); - expect(pgGeometryText(pg.Path([a, b], open: true)), '[(1.5,-2.0),(0.0,0.0)]'); - expect(pgGeometryText(pg.Path([a, b], open: false)), '((1.5,-2.0),(0.0,0.0))'); - expect(pgGeometryText(pg.Polygon([a, b])), '((1.5,-2.0),(0.0,0.0))'); + expect(pgGeometryText(pg.Path(const [a, b], open: true)), '[(1.5,-2.0),(0.0,0.0)]'); + expect(pgGeometryText(pg.Path(const [a, b], open: false)), '((1.5,-2.0),(0.0,0.0))'); + expect(pgGeometryText(pg.Polygon(const [a, b])), '((1.5,-2.0),(0.0,0.0))'); expect(pgGeometryText(pg.Circle(b, 2)), '<(0.0,0.0),2.0>'); });