From 44f6d3de98febf9d9c1df25cd5c23076e3d00a07 Mon Sep 17 00:00:00 2001 From: ZhuchkaTriplesix Date: Mon, 27 Jul 2026 21:53:09 +0300 Subject: [PATCH] fix: address code-review follow-ups from epic #463 Skip LIMIT rewriting inside SQL string/dollar quotes, delete partial exports on failure, clear stale PG tree on error, queue theme watcher refreshes while in flight, and clear Redis TYPE/TTL error on success. Closes #464 Closes #465 Closes #466 Closes #467 Closes #468 --- lib/core/database/sql_limit.dart | 106 ++++++++++++++++-- lib/core/theme/theme_folder_watcher.dart | 16 ++- .../connections_panel_pg_tree.dart | 8 +- lib/features/redis/redis_keys_view.dart | 2 +- lib/shared/services/data_export_service.dart | 6 + test/core/database/sql_limit_test.dart | 28 +++++ .../core/theme/theme_folder_watcher_test.dart | 36 ++++++ 7 files changed, 189 insertions(+), 13 deletions(-) diff --git a/lib/core/database/sql_limit.dart b/lib/core/database/sql_limit.dart index 40211e7..cbb16c3 100644 --- a/lib/core/database/sql_limit.dart +++ b/lib/core/database/sql_limit.dart @@ -25,6 +25,84 @@ final _fetchFirst = RegExp( caseSensitive: false, ); +/// Masks SQL string / identifier / dollar-quoted literals with spaces so +/// keyword regexes do not match inside quotes (same length, same offsets). +String maskSqlLiteralsForLimitScan(String sql) { + final out = StringBuffer(); + var i = 0; + while (i < sql.length) { + final c = sql.codeUnitAt(i); + + // Single-quoted string; '' is an escaped quote. + if (c == 0x27 /* ' */) { + out.write(' '); + i++; + while (i < sql.length) { + out.write(' '); + if (sql.codeUnitAt(i) == 0x27) { + if (i + 1 < sql.length && sql.codeUnitAt(i + 1) == 0x27) { + out.write(' '); + i += 2; + continue; + } + i++; + break; + } + i++; + } + continue; + } + + // Double-quoted identifier. + if (c == 0x22 /* " */) { + out.write(' '); + i++; + while (i < sql.length) { + out.write(' '); + if (sql.codeUnitAt(i) == 0x22) { + if (i + 1 < sql.length && sql.codeUnitAt(i + 1) == 0x22) { + out.write(' '); + i += 2; + continue; + } + i++; + break; + } + i++; + } + continue; + } + + // Dollar-quoted string: $tag$ ... $tag$ + if (c == 0x24 /* $ */) { + final tagEnd = sql.indexOf('\$', i + 1); + if (tagEnd != -1) { + final tag = sql.substring(i, tagEnd + 1); + final close = sql.indexOf(tag, tagEnd + 1); + if (close != -1) { + final end = close + tag.length; + out.write(' ' * (end - i)); + i = end; + continue; + } + } + } + + out.write(sql[i]); + i++; + } + return out.toString(); +} + +Match? _firstMatchOutsideLiterals(RegExp pattern, String sql) { + final masked = maskSqlLiteralsForLimitScan(sql); + return pattern.firstMatch(masked); +} + +bool _hasMatchOutsideLiterals(RegExp pattern, String sql) { + return _firstMatchOutsideLiterals(pattern, sql) != null; +} + /// Injects or clamps a `LIMIT` on read-only queries (`SELECT`, `WITH`, `VALUES`). /// /// - No `LIMIT` / `FETCH … ONLY` → appends `LIMIT [limit]`. @@ -33,6 +111,7 @@ final _fetchFirst = RegExp( /// - `FETCH FIRST/NEXT n ROWS ONLY` where `n > limit` → clamped. /// - Non-select statements are returned unchanged. /// +/// Matches ignore `LIMIT` / `FETCH` text inside string or quoted identifiers. /// Trailing semicolons are preserved after an injected clause. String injectSqlLimit(String sql, int limit) { if (limit <= 0) return sql; @@ -48,37 +127,44 @@ String injectSqlLimit(String sql, int limit) { return sql; } - if (_limitAll.hasMatch(sql)) { - return sql.replaceFirst(_limitAll, 'LIMIT $limit'); + final limitAllMatch = _firstMatchOutsideLiterals(_limitAll, sql); + if (limitAllMatch != null) { + return sql.replaceRange( + limitAllMatch.start, + limitAllMatch.end, + 'LIMIT $limit', + ); } - final limitMatch = _limitCount.firstMatch(sql); + final limitMatch = _firstMatchOutsideLiterals(_limitCount, sql); if (limitMatch != null) { final existing = int.tryParse(limitMatch.group(1)!); if (existing == null || existing <= limit) { return sql; } final offsetPart = limitMatch.group(2) ?? ''; - return sql.replaceFirst( - limitMatch.group(0)!, + return sql.replaceRange( + limitMatch.start, + limitMatch.end, 'LIMIT $limit$offsetPart', ); } - final fetchMatch = _fetchFirst.firstMatch(sql); + final fetchMatch = _firstMatchOutsideLiterals(_fetchFirst, sql); if (fetchMatch != null) { final existing = int.tryParse(fetchMatch.group(1)!); if (existing == null || existing <= limit) { return sql; } - return sql.replaceFirst( - fetchMatch.group(0)!, + return sql.replaceRange( + fetchMatch.start, + fetchMatch.end, 'FETCH FIRST $limit ROWS ONLY', ); } - if (RegExp(r'\bLIMIT\b', caseSensitive: false).hasMatch(sql) || - RegExp(r'\bFETCH\b', caseSensitive: false).hasMatch(sql)) { + if (_hasMatchOutsideLiterals(RegExp(r'\bLIMIT\b', caseSensitive: false), sql) || + _hasMatchOutsideLiterals(RegExp(r'\bFETCH\b', caseSensitive: false), sql)) { // Unrecognized LIMIT/FETCH shape — leave unchanged. return sql; } diff --git a/lib/core/theme/theme_folder_watcher.dart b/lib/core/theme/theme_folder_watcher.dart index 123bb15..3905ecc 100644 --- a/lib/core/theme/theme_folder_watcher.dart +++ b/lib/core/theme/theme_folder_watcher.dart @@ -24,6 +24,8 @@ class ThemeFolderWatcher { bool _started = false; bool _refreshInFlight = false; bool _pendingStructural = false; + bool _queuedRefresh = false; + bool _queuedStructural = false; bool get isStarted => _started; @@ -75,6 +77,8 @@ class ThemeFolderWatcher { _started = false; _refreshInFlight = false; _pendingStructural = false; + _queuedRefresh = false; + _queuedStructural = false; await Future.delayed(const Duration(milliseconds: 150)); } @@ -96,7 +100,11 @@ class ThemeFolderWatcher { } Future _triggerRefresh({required bool structuralChange}) async { - if (_refreshInFlight) return; + if (_refreshInFlight) { + _queuedRefresh = true; + _queuedStructural = _queuedStructural || structuralChange; + return; + } _refreshInFlight = true; try { await _onThemesChanged(structuralChange: structuralChange); @@ -104,6 +112,12 @@ class ThemeFolderWatcher { debugPrint('ThemeFolderWatcher: refresh failed ($error)'); } finally { _refreshInFlight = false; + if (_queuedRefresh) { + final structural = _queuedStructural; + _queuedRefresh = false; + _queuedStructural = false; + unawaited(_triggerRefresh(structuralChange: structural)); + } } } diff --git a/lib/features/connections/connections_panel_pg_tree.dart b/lib/features/connections/connections_panel_pg_tree.dart index 868cf1b..51ead1e 100644 --- a/lib/features/connections/connections_panel_pg_tree.dart +++ b/lib/features/connections/connections_panel_pg_tree.dart @@ -686,6 +686,12 @@ class _PgSchemaNodeState extends State<_PgSchemaNode> { setState(() { _error = e.toString(); _loading = false; + _loaded = false; + _tables = []; + _views = []; + _matviews = []; + _functions = []; + _sequences = []; }); } finally { lease?.release(); @@ -773,7 +779,7 @@ class _PgSchemaNodeState extends State<_PgSchemaNode> { ], ), ), - if (_loaded) ...[ + if (_loaded && _error == null) ...[ _PgObjectGroup( connection: widget.connection, databaseName: widget.databaseName, diff --git a/lib/features/redis/redis_keys_view.dart b/lib/features/redis/redis_keys_view.dart index 6b396aa..071827d 100644 --- a/lib/features/redis/redis_keys_view.dart +++ b/lib/features/redis/redis_keys_view.dart @@ -105,7 +105,7 @@ class _RedisKeysViewState extends material.State { _keys.addAll(infos); _cursor = nextCursor; _hasMore = nextCursor != 0; - if (typeTtlError != null) _error = typeTtlError; + _error = typeTtlError; }); } diff --git a/lib/shared/services/data_export_service.dart b/lib/shared/services/data_export_service.dart index bef5df0..ac282c8 100644 --- a/lib/shared/services/data_export_service.dart +++ b/lib/shared/services/data_export_service.dart @@ -357,6 +357,12 @@ class DataExportService { try { await sink?.close(); } catch (_) {} + try { + final partial = File(path); + if (await partial.exists()) { + await partial.delete(); + } + } catch (_) {} return SaveExportOutcome.error; } } diff --git a/test/core/database/sql_limit_test.dart b/test/core/database/sql_limit_test.dart index 24d586a..b630c05 100644 --- a/test/core/database/sql_limit_test.dart +++ b/test/core/database/sql_limit_test.dart @@ -111,5 +111,33 @@ void main() { '-- comment\nSELECT * FROM t\nLIMIT 100', ); }); + + test('does not clamp LIMIT text inside string literals', () { + expect( + injectSqlLimit( + "SELECT * FROM t WHERE note = 'Use LIMIT 999999 rows'", + 5000, + ), + "SELECT * FROM t WHERE note = 'Use LIMIT 999999 rows'\nLIMIT 5000", + ); + expect( + injectSqlLimit( + "SELECT * FROM t WHERE note = 'LIMIT 999999' LIMIT 999999", + 5000, + ), + "SELECT * FROM t WHERE note = 'LIMIT 999999' LIMIT 5000", + ); + }); + + test('does not treat LIMIT inside dollar quotes as a clause', () { + expect( + injectSqlLimit( + r"SELECT $$LIMIT 999999$$ AS x FROM t", + 100, + ), + r"SELECT $$LIMIT 999999$$ AS x FROM t" + '\nLIMIT 100', + ); + }); }); } diff --git a/test/core/theme/theme_folder_watcher_test.dart b/test/core/theme/theme_folder_watcher_test.dart index 7608464..a9980e8 100644 --- a/test/core/theme/theme_folder_watcher_test.dart +++ b/test/core/theme/theme_folder_watcher_test.dart @@ -147,6 +147,42 @@ void main() { expect(refreshCount, 0); await watcher.stop(); }); + + test('queues refresh while one is in flight and preserves structural', + () async { + final started = Completer(); + final release = Completer(); + final calls = []; + + final watcher = ThemeFolderWatcher( + themesDirectory: () async => themesDir, + onThemesChanged: ({required bool structuralChange}) async { + calls.add(structuralChange); + if (!started.isCompleted) started.complete(); + await release.future; + }, + debounce: const Duration(milliseconds: 40), + ); + + await watcher.start(); + if (!watcher.isStarted) return; + + await File(p.join(themesDir.path, 'a.json')).writeAsString('{}'); + await started.future.timeout(const Duration(seconds: 2)); + final callsDuringFlight = calls.length; + + // While first refresh is blocked, enqueue a structural event. + await Directory(p.join(themesDir.path, 'new_ext')).create(); + await Future.delayed(const Duration(milliseconds: 80)); + + release.complete(); + await Future.delayed(const Duration(milliseconds: 200)); + + expect(calls.length, greaterThan(callsDuringFlight)); + expect(calls.skip(callsDuringFlight), contains(true)); + + await watcher.stop(); + }); }); group('ThemeController folder watcher', () {