bamaer opened a new issue, #8277:
URL: https://github.com/apache/hop/issues/8277
### Apache Hop version?
SNAPSHOT-20260906 (main, at 00b036b902)
### Java version?
21
### Operating system
macOS
### What happened?
The Database perspective is a good addition to Hop Gui. A non-modal SQL
editor next to a project connection tree is something the modal `SqlEditor`
dialog could never be, and the `IDatabaseWorkbenchHost` split that lets the
same widget run as a perspective, a floating window and a dock tab is a nice
piece of design.
What follows is nine hardening items from a follow-up code read and manual
testing of `f6c2d411b0` (#8212). None of them are objections to the feature.
Each is independent; happy to split if that is easier. Line numbers are against
`f6c2d411b0`..`00b036b902` — nothing in that range touches the files cited.
---
## 1. SQL tab memory is shared by every workbench instance, last writer wins
Three `DatabaseWorkbench` instances can be alive at once: the perspective's,
created in `initialize()` and kept for the session because `HopGui` initializes
every perspective at startup (`HopGui.java:917`), plus the floating window's
(`DatabaseWorkbenchDialog.java:126`) and the dock's
(`DatabaseWorkbenchViews.java:91`). All three read and write one audit entry,
group = namespace / type = `database-sql-tabs`. Each calls `restore(this)` in
its constructor (`DatabaseWorkbench.java:258`), and the only guard,
`hasSqlEditorTabs()`, inspects that instance's own empty list — so each opens
independent copies. `save()` then snapshots only its own tabs as a whole-entry
replace, with no merge or version check (`DatabaseSqlTabMemory.java:117-141`).
The debounce runnable is per instance, so none can suppress another's write.
**Repro** (no live database needed):
1. Database perspective: select a connection, open a SQL tab, type `select
1`.
2. Click **Float** — the perspective flushes to the audit entry (`:1008`),
then the window restores an independent copy.
3. In the floating window change it to `select 2`, close the window
(`saveNow` at `:252`).
4. Back in the perspective, without touching the tab, click **Float** again.
Expected `select 2`; actual `select 1`, because step 4 flushed the stale
instance first. A restart also comes back with `select 1`. Same via **Dock**,
via any edit in the perspective after step 3, or on quit.
*Note:* clicking an already-selected tab is not a trigger (`CTabFolder`
notifies only on index change), and the javadoc's "same mechanism as terminal
tabs" (`DatabaseSqlTabMemory.java:35`) does not hold — terminals live in a
single `HopGuiBottomDock` and cannot race. No file on disk is affected; what is
lost is unsaved buffer content.
---
## 2. Table preview SQL omits the prefix-style row limit
`previewSelectSql` (`DatabaseWorkbench.java:973-978`) appends
`getLimitClause(rowLimit)` and never calls `getLimitClausePrefix`, which has no
occurrence under `ui/`. The two forms are mutually exclusive by design
(`IDatabase.java:249-251`) and the override sets are disjoint — 23 dialects
override one, 10 the other, none both — so for the prefix dialects
`getLimitClause` falls through to `""` and the preview carries no limit.
Affected (eleven): Access, Cache, Firebird, Informix, Interbase, Iris,
MsSqlServer, Sybase, SybaseIQ, Teradata, and MsSqlServerNative, which inherits
the MSSQL prefix and overrides neither.
`Database.getFirstRows` asks for both forms (`Database.java:3845-3850`) —
that is `3b3dc8fdf4`, *"Let a database limit rows before the query as well as
after it, fixes #8013"*, an ancestor of this work.
**Repro:** MS SQL Server connection, "Quote all fields" off, default limit
1000. Connect, expand a schema, Preview a plain-named table. Expected `SELECT
TOP 1000 * FROM dbo.mytable`, actual `SELECT * FROM dbo.mytable`.
The grid is still capped — `setQueryLimit`/`getRows` apply the limit
independently of the SQL text. What is unbounded is the statement the user can
edit, copy or save, and the statement the server receives.
*Adjacent, pre-dating this:* `GenericDatabaseMeta` delegates
`getLimitClause` (`:473-479`) but has no `getLimitClausePrefix` delegation, so
a Generic connection on a prefix dialect loses it everywhere, `getFirstRows`
included.
---
## 3. Failures are captured but never shown
`runOperation` (`DatabaseWorkbench.java:1257-1271`) is the single funnel for
all background work: it catches `Exception`, calls `operation.fail(...)` and
refreshes the panel. No log, no dialog, no rethrow. `fail()` stores the text in
`errorMessage`, but `git grep getErrorMessage` across `ui`, `rap`, `rcp` and
`core` returns no hits, and nothing renders it — the panel has four columns
(Description, Connection, Status, Elapsed) and no per-row tooltip. The
project's own test pins this:
`DatabaseOperationsPanelTest.formatStatusLineOmitsBlankConnection` calls
`fail("boom")` and asserts the line starts with `Connect - Failed - `.
In the SQL editor there is a second effect: `messages` and `queryResults`
are locals inside the lambda (`DatabaseSqlEditorTab.java:399`), and
`connect`/`getRows`/`execStatement` all throw past the terminal `asyncExec(()
-> { resultsPanel.show(...); showResults(); })` at `:433-438` — the only call
site of `show()`, which is also the only thing that clears old result tabs.
**Repro:** run a valid query in a SQL tab, then replace it with `SELECT *
FROM does_not_exist` and Run. Expected the driver error in the Results/Messages
tab; actual the panel still shows the previous run's rows and messages, with
`Execute SQL on <conn> - <conn> - Failed - <elapsed>` as the only new signal.
Variants: on the first run in a fresh tab there is no visible output at all
(the panel is still collapsed and `showResults()` is in the skipped block); on
Run All, messages from statements that already succeeded are discarded with the
exception; a wrong password on a tree double-click gives only `Connect to
<name> - Failed`, and triggering that connect from Run silently drops the
queued execution, since `afterConnected` runs only inside the success
continuation (`:765-774`).
One case *is* surfaced: a `getObjectDdl` failure in table-info is caught and
written into the DDL tab as `-- <message>`
(`DatabaseTableInfoTab.java:199-203`).
*For contrast:* the modal `SqlEditor` this replaces raises an `ErrorDialog`
on a failed SELECT (`:430`), on failed DDL (`:474`) and a `MessageBox` on a
failed connect (`:498`). Silent background failure has one precedent,
`HopGuiSearchResultsPanel.java:529-536`, but even that logs the exception.
---
## 4. Save in the floating window and the dock targets the main window's file
Every save path resolves through `HopGui.getActiveFileTypeHandler()` →
`getActivePerspective().getActiveFileTypeHandler()` (`HopGui.java:2405-2406`).
`activePerspective` is written only by `setActivePerspective`, and neither the
floating window nor the dock sets it — both host a `DatabaseWorkbench` outside
the perspective system. So while a SQL tab has focus, File > Save and the Save
toolbar button (`HopGui.java:1379` → `HopGuiFileDelegate.fileSave:244`) act on
whatever the main window's active perspective has selected. Enablement comes
from that same handler (`HopGui.java:2394`), so Save looks available because
the *other* file is dirty.
**Repro:**
1. Open a pipeline (an Explorer perspective tab — there is no separate
Pipeline perspective on this branch).
2. Tools > Database window. Select a connection, open a SQL editor tab, type
something.
3. With the floating window focused and on top, click Save.
Expected the SQL buffer saved, or a `*.sql` Save As dialog for an untitled
one. Actual the pipeline is saved, silently — `fileSave` finds its filename
already set and calls `typeHandler.save()` (`HopGuiFileDelegate.java:250-256`).
Nothing writes the SQL buffer. The dock tab behaves the same.
`isCloseable()` has the same defect on the close path — it calls
`fileDelegate.fileSaveAs()` (`DatabaseSqlEditorTab.java:704`) instead of its
own `saveAs(String)` (`:575`), then returns `!changed`, still false, so the tab
also refuses to close. That path is rarely reached in practice: closing the
floating *window* prompts for nothing, because its shell dispose listener only
stores geometry (`DatabaseWorkbenchDialog.java:135-143`) and never consults the
open tabs.
*Note:* `git grep fileSaveAs` returns four call sites, no other
`isCloseable()` among them. House style is the opposite —
`HopGuiPipelineGraph.isCloseable` calls `BaseDialog.presentFileDialog` on
itself; `BaseExplorerFileTypeHandler` and `MetadataEditor` call their own
`save()`.
---
## 5. Closed tabs leave their content control alive, so dispose-wired
cleanup never runs
`CTabItem.dispose()` does not dispose the item's control — `destroyItem`
only calls `setVisible(false)` (SWT 3.134.0 `CTabItem.java:130-140`; RAP RWT
4.7.0 identical, so Hop Web does not differ). `disposeTab`
(`DatabaseWorkbench.java:1321-1332`) disposes only the item, and the content
control was parented to the folder, not the item (`:1035, 1055`).
`DatabaseSqlEditorTab` registers a Display-wide `SWT.KeyDown` filter in its
constructor and wires removal to `control.addDisposeListener` (`:242-248`), so
closing a SQL tab leaves both the widget tree and the filter in place until
shell teardown. `DatabaseResultsPanel.show()` in the same code uses the pattern
that works — `item.getControl().dispose()` before `item.dispose()` (`:114-116`).
**Repro** (debugger or heap dump; nothing visible in the GUI): open and
close a SQL tab repeatedly. The breakpoint at `:243` is never hit, and a heap
dump shows one retained `DatabaseSqlEditorTab` per closed tab.
`DatabaseTableInfoTab` leaks its widget tree the same way.
*Scope.* The retained filters are inert (`isInThisEditor` returns false for
an orphan), so the cost is retention, not behaviour. Disposing only the
`CTabItem` is existing house style (`ExplorerPerspective.java:2431`,
`MetadataPerspective.java:3291`), and `HopGuiBottomDock.closeTab` — **not
touched by this PR**, it comes from `04e0b6e11e` (#7356) — already leaks
`HopGuiSearchResultsPanel` identically. Hosting `DatabaseWorkbench` there does
raise the stakes: the orphan keeps its `MetadataChanged`/`ProjectActivated`
registrations (`:231, 239`) and neither guard stops it, so it keeps running
`loadAll()` and `rebuildTree()`. Count invocations rather than expecting
silence — the perspective's own instance already answers every metadata change
at baseline. Whether that half is fixed here or in the dock is a separate call.
---
## 6. A file-backed SQL tab registers with the file-refresh delegate but has
no `reload()`
`openSqlTab` registers every file-backed tab
(`DatabaseWorkbench.java:1057-1060`) into a single `HashMap`, so a second
registration for the same path replaces the first. `DatabaseSqlEditorTab` does
not override `reload()`, inheriting the empty default
(`IHopFileTypeHandler.java:167`), so the monitor still fires and lands on a
no-op — displacing the Explorer's `SqlExplorerFileTypeHandler`, whose inherited
`reload()` does re-read the file. Closing the tab then removes the watch for
everyone: `disposeTab` calls `remove(getFilename())` guarded only for null
(`:1321-1327`), and `DefaultFileMonitor` keeps one agent per name with no
reference counting.
**Repro.** Requires "Reload file if changed on filesystem" in Configuration
> General options — **off by default** (`PropsUi.java:552-554`), and with it
off none of this happens.
1. Explorer perspective: open `queries.sql`, change it outside Hop, wait ~2s
— it refreshes (control step).
2. In that editor's toolbar click **Open in database** and pick a connection.
3. Change the file again. Expected: refreshes as in step 1. Actual: neither
tab refreshes.
4. Close the Database SQL tab, change the file again. Expected: the Explorer
tab refreshes again. Actual: still nothing — the file is unwatched entirely and
cannot recover until that tab is closed and reopened.
*Note:* registering without a `reload()` override is not new —
`ExplorerPerspective.addFile` registers every `IExplorerFileTypeHandler`, and
`Svg`, `GitInfo` and `Sas` define none. The un-refcounted `remove` on close is
the part with a cross-perspective consequence.
---
## 7. `SELECT … FROM … INTO …` changes classification in
`Database.execStatements`
`execStatements` previously used `startsWith("SELECT") &&
!matches("(?is)^(select\\s.*\\sinto\\s).*")`, treating any SELECT containing a
whitespace-delimited INTO as a non-query. It now calls
`SqlQueryClassifier.isQuery` (`Database.java:1620`), whose `hasIntoAtDepthZero`
returns false as soon as it meets FROM or WHERE at depth zero
(`SqlQueryClassifier.java:223-225`), so an INTO after FROM is never reached.
Compiling the current classifier against the old predicate:
```
SELECT id, name FROM customers INTO OUTFILE '/tmp/c.csv' old=false
new=true
SELECT col FROM t INTO @var old=false
new=true
SELECT * INTO dest FROM src old=false
new=false (unchanged)
```
`true` routes to `executeQuery`, `false` routed to `execute` — and these
forms return no result set. The outcome is driver-dependent (established by
running each driver's own classification code, no live server):
mysql-connector-j **9.7.0** rejects it before the statement is sent;
**9.0.0/9.1.0** do not, so it executes and fails later at `rs.next()` as
`HopDatabaseException("Couldn't get row from result set")`;
**mariadb-java-client 3.5.7** returns an empty result set under its default
`permitNoResults`, so it is not observable there.
This reaches beyond the perspective — `execStatements` is called from the
SQL workflow action (`ActionSql.java:181`), Execute SQL script
(`ExecSql.java:203, 309`), Execute SQL script for each row, the DDL transform,
Snowflake `WarehouseManager`, `SqlStatementsDialog`, and `Database`'s own
connect-SQL and `lockTables`. The first two are gated on flags that default to
false, i.e. the changed path is the default.
**Repro.** MySQL with mysql-connector-j 9.7.0. SQL workflow action, "Send
SQL as single statement" unchecked (the default; checking it bypasses this
path), running `SELECT id FROM customers INTO @last_id;`. Expected
(pre-`f6c2d411b0`): executed with `Statement.execute`, `@last_id` set. Actual:
routed through `executeQuery` and rejected. Control: `SELECT id INTO @last_id
FROM customers;` still works, isolating clause order. Use `INTO @var` rather
than `INTO OUTFILE` — OUTFILE needs the FILE privilege and a matching
`secure_file_priv`, which would fail on both builds and mask the difference.
**This is not a pure regression, and a straight revert would reintroduce the
opposite defect.** The old regex had no notion of string literals or
subqueries, so `SELECT * FROM t WHERE note = 'insert into x'` was also
classified as a non-query; the new classifier handles that correctly, and the
sibling change in `BaseDatabaseMeta.getSqlScriptStatements` is a strict
improvement over `startsWith("SELECT") || startsWith("show")`. What is needed
is INTO detection that continues past FROM.
*Coverage:* `SqlQueryClassifierTest.selectIntoIsNotAQuery` asserts only
`SELECT * INTO dest FROM src`, and the HOP-2584 integration workflow uses
`select 1 as a, 2 as b into t`, which has no FROM. Neither covers this ordering.
---
## 8. `openSqlTab` ignores its `dirty` argument for tabs with no filename
`dirty` is read at exactly one place, `tab.applyBuffer(buffer, dirty)`
(`DatabaseWorkbench.java:1039`), reachable only when `filename` is non-empty
*and* `buffer` is non-null. With an empty filename the method takes
`setInitialText(...)` (`:1052`), which suppresses modify and leaves `changed`
false. Three callers pass `dirty = true` with no filename: `openSuggestedSql`
(`:1106`, behind the SQL button of Table Output and ~16 other dialogs),
`generateDdl` (`:936`), and `restoreSqlTab`'s filename-less branch (`:1151`).
The same argument value produces two different outcomes depending on whether a
filename is set.
**Repro:** right-click a table > **Generate DDL**, then close the tab with
its X *without typing in the editor* (one keystroke fires the ModifyListener
and masks it). Actual: title not bold, closes with no save prompt. Expected:
bold title and a "Save file?" prompt, as on the file-backed path.
*Scope:* no SQL text is lost — `snapshotOf` persists the buffer whenever the
filename is empty, regardless of `dirty`. Related: the tab-reuse loop at
`:1024-1033` is gated on the same non-empty filename, so filename-less opens
never dedupe and each click of a dialog's SQL button adds another persisted tab.
---
## 9. The operations list accumulates for the session
`operations` is a plain `ArrayList` and the table is a non-virtual SWT
`Table`, so every entry is a materialised `TableItem`
(`DatabaseOperationsPanel.java:67, 93, 161`). Nothing removes from either — no
accessor, no context menu, and the four toolbar actions are Kill selected,
Minimize, Kill current, Expand; `cancelAll` cancels and removes nothing. There
is no cap, eviction or Clear action. For the perspective-hosted workbench this
spans the whole session, and a project switch does not clear it, so rows naming
a previous project's connections stay in the table.
**Repro:** rows accrue **per user action, not per statement** —
`runOperation` creates one `DatabaseOperation` per call, from four sites:
connect, generate DDL, execute SQL and open table info. Expand the operations
panel and connect / preview / run / open table info repeatedly; every row from
the start of the session is still listed, and `refreshElapsed` walks the whole
list every 500ms while anything is running.
*Retention detail:* `DatabaseOperation.database` is never cleared. The
attached `Database` has been closed, so no connection, statement or tunnel is
held — but `disconnect()` never clears `dbmd` (`Database.java:151`, nulled only
in the constructor), and the table-info path calls `getDatabaseMetaData()`, so
those operations keep a driver `DatabaseMetaData` and typically its closed
`Connection` reachable.
---
*From a code read of `f6c2d411b0` plus manual testing, cross-checked against
the SWT 3.134.0 and RAP 4.7.0 sources and the JDBC drivers named in item 7.
Items needing a live server or a debugger are marked as such in place.*
### Issue Priority
Priority: 2
### Issue Component
Component: Hop Gui, Component: Database
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
