diff --git a/docs/API_DOCS.md b/docs/API_DOCS.md index 916607bcd..01cb1c9ee 100644 --- a/docs/API_DOCS.md +++ b/docs/API_DOCS.md @@ -778,7 +778,8 @@ admin routes use. "password": "secret" }, "type": "vacuum", - "target": "users" + "target": "users", + "container": "app" } ``` @@ -789,6 +790,13 @@ admin routes use. | `connection` | object | Yes | Database connection configuration | | `type` | string | Yes | Maintenance operation type | | `target` | string | No | Target table name or PID (for kill). Also selects the *placement* the request is validated as: absent or empty means whole-database, any name means one object | +| `container` | string | No | The container the target lives in, as the row carries it in `schemaName`: the schema on PostgreSQL and SQL Server, the database on ClickHouse, the bucket on a document store. A non-string value (an object, a number, an array, `null`) returns `400`. Absent or empty means the request names no container and the provider falls back to its own reading of `target` | + +`container` is what disambiguates a target whose namespace the name alone cannot settle: +`app.orders` and `public.orders` carry the same `target` and different `container` values, and the +provider qualifies with it rather than splitting the name. Engines with one attached namespace +(SQLite, libSQL, Trino's query-id `kill`) ignore it; each provider's own meaning is in +`docs/providers/.md`. The maintenance audit event records it beside `target`. **Maintenance Types:** @@ -833,7 +841,14 @@ admin routes use. The handler validates against the target provider's capabilities: `type` is required (`{ "error": "Maintenance type is required" }`), the provider must support maintenance at all, and the requested operation must be in that provider's supported set (see the matrix above) — otherwise a `400` is returned listing what the provider does support. -A fourth `400` gates what the operation may be *pointed at*. Each provider declares that separately +`container` is type-checked before any provider is opened: a value that is neither absent nor a string +answers `{ "error": "\"container\" must be a string naming the target's container" }` with `400`. +Without this the value reached the provider's identifier escaper, where it failed as +`identifier.replace is not a function` and the caller read a `500` for a malformed request. An +empty string is not malformed: it reads as a request that named no container, the same way an empty +`target` reads as the whole-database form. + +A fifth `400` gates what the operation may be *pointed at*. Each provider declares that separately (`maintenanceOperationSpecs`, documented per engine under `docs/providers/`), and `target` selects which half of the declaration this request is: absent or empty is a whole-database request, a name is a per-object one. When the provider says that placement is not offered for this operation while diff --git a/docs/BACKLOG.md b/docs/BACKLOG.md index b2af0000c..767388f85 100644 --- a/docs/BACKLOG.md +++ b/docs/BACKLOG.md @@ -28,7 +28,7 @@ None of it is a GitHub issue. **Sections** - [SQL statement reading](#sql-statement-reading) — S2–S6 · 4 -- [Drivers and connections](#drivers-and-connections) — D1-D129, U17 · 74 +- [Drivers and connections](#drivers-and-connections) — D1-D129, U17 · 73 - [Value interpolation](#value-interpolation) — V1 - [Row editing](#row-editing) — R1–R3 · 3 - [Studio UI and query execution](#studio-ui-and-query-execution) — X2-X19, U2-U54 · 42 @@ -516,47 +516,6 @@ current database, and that permission cannot be granted in `master`. rather than published. Measured on a real instance with a login that has neither grant, because the whole entry rests on a permission boundary no fixture can prove. -### D49. Per-table maintenance drops the schema, so every table outside the default one refuses - -Found 2026-08-27 in the BROWSER while registering `duckdb` (issue #424). Not DuckDB's defect - the -provider is the half that behaves - and no gate could have caught it: the six local gates, 100% -line coverage and a four-lens adversarial review all passed over it, because the two halves are -correct in isolation and only the running product puts them together. - -`TablesTab.tsx:390` calls `handleMaintenance(type, table.tableName)` - the BARE table name - from a -row whose very next line (`:350`) renders `table.schemaName` beside it. Every provider's -`qualifyMaintenanceTarget` then supplies a default schema for an unqualified target: -`postgres.ts:1287` returns `"public." + escapeIdentifier(target)`, and -`duckdb/index.ts:712` returns `"main".""`. So the statement names a table that is not there. - -Measured on DuckDB v1.5.5, clicking **Analyze Table** on the `analytics.events` row: - -``` -Catalog Error: Table with name events does not exist! Did you mean "analytics.events"? -LINE 1: ANALYZE "main"."events" -``` - -`POST /api/db/maintenance` answers 400 and the panel prints the engine's message, so it is visible -rather than silent - but the button cannot succeed on any table outside the default schema, on any -engine. It went unnoticed because the fixtures the other engines are exercised with keep their -tables in the default schema; DuckDB is simply the first whose fixture carries a second one. - -This is #U9 one layer up. #U9 was an operation DECLARED in the wrong placement (Oracle offered -`optimize` per table, and the target it sent was rejected); this is the right placement sending an -under-qualified target. - -Deliberately not fixed in the provider PR that found it. The one-line repair - passing -`` `${table.schemaName}.${table.tableName}` `` - changes the target string reaching all TWELVE -providers that implement `runMaintenance` (postgres, mysql, mssql, oracle, sqlite, libsql, duckdb, -clickhouse, cassandra, druid, trino, search), and each has its own qualification and its own -statement grammar: SQLite has no user schemas, MySQL's `OPTIMIZE TABLE` takes `db.table`, and the -HTTP engines build their own paths. That is a twelve-engine live verification, not a provider -change. - -**Done when:** the row passes the qualified name, every one of the twelve providers has been -measured against a table outside its default schema (or recorded as having no such concept), and a -component test pins the target the row sends so it cannot silently revert to the bare name. - ### D51. Four providers degrade a refused monitoring read to no rows, then read the absent row as 0 Found 2026-08-27 in the #517 review, which asked whether the search provider really held the last @@ -1779,13 +1738,13 @@ Not fixed there: the cache key is shared by every engine. Found 2026-09-24 in the browser while verifying #843 (PR #1106), on a connection opened with `appdata` whose tree also lists `analytics`. Both hold a collection called `events`. -The tree's **Validate Collection** on `analytics > events` opens `/admin/operations?path=analytics&path=events`, and that page lists the connected database's collections, `appdata . events` among them: `getTableStats()`, `getIndexStats()` and `runMaintenance()` in `src/lib/db/providers/document/mongodb.ts` all read `this.db`, the connected database, and `runMaintenance(type, target)` takes a bare collection name. +The tree's **Validate Collection** on `analytics > events` opens `/admin/operations?path=analytics&path=events`, and that page lists the connected database's collections, `appdata . events` among them: `getTableStats()`, `getIndexStats()` and `runMaintenance()` in `src/lib/db/providers/document/mongodb.ts` all read `this.db`, the connected database, and since #1091 `runMaintenance()` takes the row's container but refuses one that is not the connected database. So the row a person presses for the collection they chose is the connected database's same-named collection, which is the shape #843 removed from the query path. Not fixed in #1106, which is scoped to the statement grammar. -The target is a bare string in `runMaintenance(type, target)`'s contract for every provider, so passing a path is the D49 change, and the monitoring tabs are session-scoped on every engine. +Since #1091 the contract carries the container, `runMaintenance(type, target, container)`, so what is left is the provider running in a database other than the connected one, and the monitoring tabs, which are session-scoped on every engine. -**Done when:** a MongoDB maintenance target names its database, the deep link either opens the collection's own database or refuses a path outside the connected one, and a test pins that `Validate` on `analytics.events` reaches `analytics`. +**Done when:** the deep link either opens the collection's own database or refuses a path outside the connected one, and a test pins that `Validate` on `analytics.events` reaches `analytics`. ### D119. On RisingWave every column reads nullable, a `NOT NULL` column and a primary key included diff --git a/docs/DATABASE_PROVIDERS.md b/docs/DATABASE_PROVIDERS.md index 660f10f6a..0accf2a79 100644 --- a/docs/DATABASE_PROVIDERS.md +++ b/docs/DATABASE_PROVIDERS.md @@ -242,7 +242,7 @@ interface DatabaseProvider { getHealth(): Promise; // Maintenance operations - runMaintenance(type: MaintenanceType, target?: string): Promise; + runMaintenance(type: MaintenanceType, target?: string, container?: string): Promise; // Validation validate(): void; diff --git a/docs/providers/clickhouse.md b/docs/providers/clickhouse.md index 73bd0a86e..a066dad8c 100644 --- a/docs/providers/clickhouse.md +++ b/docs/providers/clickhouse.md @@ -123,16 +123,18 @@ ClickHouseProvider (clickhouse/index.ts) Couchbase does — because the dialect really is standard on the points the shared helpers care about: double-quoted identifiers and `LIMIT n OFFSET m` are both correct here, live-verified (`SELECT "id" FROM "probe"` and the bare unquoted form both parse). This is exactly the case -[`docs/ADDING_A_PROVIDER.md`](../ADDING_A_PROVIDER.md) names ClickHouse for. Only `prepareQuery()` -is overridden, for the trailing-clause trap in [§3.8](#38-the-preparequery-override). +[`docs/ADDING_A_PROVIDER.md`](../ADDING_A_PROVIDER.md) names ClickHouse for. `escapeIdentifier()` +carries one dialect correction, below; `prepareQuery()` is overridden for the trailing-clause trap +in [§3.8](#38-the-preparequery-override). ### 2.3 What `SQLBaseProvider` gives for free -`ClickHouseProvider` reuses these inherited members rather than reimplementing them: +`ClickHouseProvider` reuses these inherited members rather than reimplementing them, except where +the table says otherwise: | Member | Purpose | |--------|---------| -| `escapeIdentifier()` | Double-quoted, since `this.type` (`clickhouse`) falls through to the default branch — the same quoting PostgreSQL uses. Both quoted and unquoted forms parse (live-verified) | +| `escapeIdentifier()` | **Overridden here.** Double-quoted, the same quoting PostgreSQL uses, plus a doubled BACKSLASH: the inherited form doubles only the quote character, and a backslash is an ESCAPE inside a quoted identifier on this engine, so a name ending in one swallowed its own closing quote (#1091 review). See [§8](#8-maintenance) | | `buildLimitClause()` | `LIMIT n` / `LIMIT n OFFSET m` | | `shouldEnableSSL()` | Inherited but **never called**, and deliberately so. It infers TLS from substrings in the host (`cloud`, `aws`, …), which would silently switch a self-hosted node whose hostname merely contains one of them. TLS here comes from the connection's own `ssl` config or from an `https://` scheme, never from a guess ([§4.3](#43-tls)) | | `prepareQuery()` (base) | The shared query limiter; `ClickHouseProvider` calls it first and only overrides the trailing-clause case | @@ -1264,8 +1266,10 @@ curl -s "http://127.0.0.1:8123/?user=libredb&password=$CH_PASSWORD&database=demo ### 6.3 Object edit (#789) -This engine is a REFUSAL, and the reason is that no measured escaper exists for its identifiers. -A backslash inside a quoted identifier is an ESCAPE in both the double-quote and the backtick form on 26.7.1.1315, and all three identifier quoters in this tree emit `"x\"` for the name `x\`, so the statement a plan would carry is not the statement the author addressed. +This engine is a REFUSAL, recorded because no escaper for its identifiers had been measured. +A backslash inside a quoted identifier is an ESCAPE in both the double-quote and the backtick form on 26.7.1.1315, and the shared quoters (the default branch of `SQLBaseProvider.escapeIdentifier`, `quoteIdentifier` in [`src/lib/sql/identifier.ts`](../../src/lib/sql/identifier.ts) and `escapeIdentifier` in [`pool-manager.ts`](../../src/lib/db/utils/pool-manager.ts)) emit `"x\"` for the name `x\`, so a statement built through any of them is not the statement the author addressed. +Since #1091 this provider's own `escapeIdentifier()` escapes the backslash as well, and it is measured ([§8](#8-maintenance)). +No edit statement is built through it and none has been measured, so the refusal stands. One question here is UNMEASURED and is recorded as such rather than answered: whether a dictionary's credential is really redacted in the text the Phase 2 read returns. No kind here declares `acceptsSourceEdits`, and `tests/isolated/object-edit-declarations.test.ts` is what holds that absence and this section together. @@ -1300,10 +1304,15 @@ Two honest zeroes in the overview, so neither reads as a measurement: ## 8. Maintenance -`runMaintenance(type, target?)` +`runMaintenance(type, target?, container?)` ([`index.ts`](../../src/lib/db/providers/sql/clickhouse/index.ts)). `optimize` and `kill` **require** a target; `analyze` does not. +A `container` is the DATABASE the row carries as `schemaName` (#772), used as the database outright: +`database.table` cannot be told apart from a name that contains a dot, while a container is already +the database on its own. Without one the old reading stands, splitting the name and falling back to +the pinned database. + | Type | ClickHouse action | Notes | |------|--------------------|-------| | `optimize` | `OPTIMIZE TABLE . FINAL` | Merges the table down to one part per partition and applies pending mutations — the operation a ClickHouse user reaches for where another engine would vacuum. A target is mandatory, because `OPTIMIZE` names a table | @@ -1317,6 +1326,15 @@ Calling `runMaintenance` with one directly throws a `QueryError` naming the thre operations. A target is qualified through `escapeIdentifier()` (`"database"."table"`, defaulting the database to the pinned one when the target names none), so a hostile or oddly-named table cannot break out of the generated statement. +That helper is OVERRIDDEN here rather than inherited, and the reason is the identifier escape +measured in [§6.3](#63-object-edit-789): a backslash inside a quoted identifier is an escape on this +engine, so the inherited form, which doubles only the quote character, left a name ending in one +with its closing quote swallowed and the rest of the statement reparsed around it. A container of +`x\` was the reachable case (#1091 review): the target that followed became more statement text +rather than a second segment. The override escapes the backslash first, the order `literal()` in +`objects.ts` uses. +Live-verified on 26.7.1.1315 against the fixture's own `` demo.`bs_one\` ``: the target `bs_one\`, the target `demo.bs_one\`, and the target `bs_one\` with the container `demo` all optimize it, where the inherited spelling answers `Double quoted string is not closed` on the same table. +The container `x\` with the target `.t FINAL SETTINGS optimize_throw_if_noop = 1 --` is refused with `UNKNOWN_DATABASE`, for a database named `x\`. ### Where each operation may be offered (`maintenanceOperationSpecs`) diff --git a/docs/providers/couchbase.md b/docs/providers/couchbase.md index fdc3d3c11..0bcad5ace 100644 --- a/docs/providers/couchbase.md +++ b/docs/providers/couchbase.md @@ -1114,13 +1114,20 @@ edge one. Omitted, the same panels render `N/A` / "Not measured" and score the c ## 8. Maintenance -`runMaintenance(type, target?)` +`runMaintenance(type, target?, container?)` ([`index.ts`](../../src/lib/db/providers/document/couchbase/index.ts)). All three operations **require** a target. +A `container` is the row's `schemaName` (#772), and the keyspace it addresses is decided from it: +the bucket's own name (the only Tables row this provider has, `getTableStats()`) means the +bucket's default collection, so the row's Analyze button addresses `` `bucket`.`_default`.`_default` `` +rather than a scope that does not exist; any other container is the SCOPE the collection sits in, +used as one instead of being parsed back out of the display name. Without a container the +display-name rule stands: `scope.collection`, or the default scope for a bare name. + | Type | Couchbase action | Notes | |------|------------------|-------| -| `analyze` | `UPDATE STATISTICS FOR INDEX ALL` | **Enterprise Edition only.** A Community cluster answers "'Update Statistics' is an enterprise level feature." — returned verbatim as a failed result, not swallowed or reworded | +| `analyze` | `UPDATE STATISTICS FOR INDEX ALL` | **Enterprise Edition only.** A Community cluster answers "'Update Statistics' is an enterprise level feature.", returned verbatim as a failed result, not swallowed or reworded. The success reply names the same keyspace the statement addressed (``Updated statistics for `travel`.`inventory`.`hotel` ``), so a row whose target is the bucket cannot report as if the bucket itself had been touched (#1091 review) | | `reindex` | `BUILD INDEX ON (...)` over the keyspace's deferred indexes | Reports "No deferred indexes on X" when there are none | | `kill` | `DELETE FROM system:active_requests WHERE requestId = $1` | Target is the request id shown in active sessions | diff --git a/docs/providers/duckdb.md b/docs/providers/duckdb.md index 9f5693449..837254459 100644 --- a/docs/providers/duckdb.md +++ b/docs/providers/duckdb.md @@ -1052,6 +1052,12 @@ per-entity control would fail at the point the user clicked it. `runMaintenance` withheld types **here**, naming the reason, rather than sending a statement the engine will reject with wording about a keyword the user never typed. +`runMaintenance(type, target?, container?)` takes a `container` as the SCHEMA the row carries as +`schemaName` (#772), the same reading PostgreSQL uses: the schema is quoted whole and prefixed to +the quoted table name, and never recovered by splitting the target - a schema is allowed to contain +a dot. Without one the old readings stand: `schema.table` is quoted part by part and a bare name +falls back to `main`. + --- ## 9. Capabilities & labels diff --git a/docs/providers/libsql.md b/docs/providers/libsql.md index 624563dcd..14a5f510e 100644 --- a/docs/providers/libsql.md +++ b/docs/providers/libsql.md @@ -864,6 +864,9 @@ Measured through the provider against both deployments (fixture: 2 tables, 3 and | `check` | globally | `PRAGMA integrity_check`, and the ANSWER is read — a corrupt database reports damage in its row while the statement itself succeeds | | `vacuum`, `analyze`, `optimize`, `kill` | withheld | Refused by the server (§3.5); a direct API call is refused by the provider with the reason | +A `container` is deliberately ignored (#772): a libSQL connection resolves names against its one +attached database, exactly as `sqlite.ts` does. + --- ## 9. Capabilities & labels diff --git a/docs/providers/mongodb.md b/docs/providers/mongodb.md index ff564c96a..5678daba1 100644 --- a/docs/providers/mongodb.md +++ b/docs/providers/mongodb.md @@ -913,9 +913,15 @@ something was measured: ## 8. Maintenance -`runMaintenance(type, target?)` ([`mongodb.ts`](../../src/lib/db/providers/document/mongodb.ts)) +`runMaintenance(type, target?, container?)` ([`mongodb.ts`](../../src/lib/db/providers/document/mongodb.ts)) maps the generic operations onto MongoDB admin commands: +A `container` is a DATABASE name (#772). The provider is bound to one database and no admin command +can retarget mid-command, so the bound name is accepted and any OTHER name is refused with +`bound to the database ""` rather than quietly acted on against the wrong one. The comparison +uses `getDatabaseName()`, the name `connect()` opened - a connection-string connection sets no +`config.database`, and comparing with that alone refused the bound database itself. + | Type | MongoDB action | |------|----------------| | `analyze` | `validate` (one collection, or every collection) | diff --git a/docs/providers/mssql.md b/docs/providers/mssql.md index 780ce720f..563fa4aae 100644 --- a/docs/providers/mssql.md +++ b/docs/providers/mssql.md @@ -1251,8 +1251,10 @@ boundary preserves those states without a falsy test that would erase a genuine ## 9. Maintenance -`runMaintenance(type, target?)` ([`mssql.ts`](../../src/lib/db/providers/sql/mssql.ts)); targets -are bracket-escaped (`]` → `]]`): +`runMaintenance(type, target?, container?)` ([`mssql.ts`](../../src/lib/db/providers/sql/mssql.ts)); targets +are bracket-escaped (`]` → `]]`). A `container` is the SCHEMA the row carries as `schemaName` +(#772), emitted as `[schema].[table]`; without one a bare target keeps the previous reading, where +the connected default schema applies. | Type | With target | Without target | |------|-------------|----------------| diff --git a/docs/providers/mysql.md b/docs/providers/mysql.md index 040505c42..3ffe04803 100644 --- a/docs/providers/mysql.md +++ b/docs/providers/mysql.md @@ -1528,8 +1528,11 @@ that is what MySQL itself calls index bytes. ## 9. Maintenance -`runMaintenance(type, target?)` ([`mysql.ts`](../../src/lib/db/providers/sql/mysql.ts)); targets -are backtick-quoted via `escapeIdentifier()`: +`runMaintenance(type, target?, container?)` ([`mysql.ts`](../../src/lib/db/providers/sql/mysql.ts)); targets +are backtick-quoted via `escapeIdentifier()`. A `container` is the DATABASE the row carries as +`schemaName` (#772), and it qualifies the target only when it names a database OTHER than the +connected one: a MySQL statement already resolves a bare table inside the connected database, so +the same name as a prefix adds nothing. | Type | With target | Without target | |------|-------------|----------------| diff --git a/docs/providers/oracle.md b/docs/providers/oracle.md index 2262b8a19..e307a8618 100644 --- a/docs/providers/oracle.md +++ b/docs/providers/oracle.md @@ -1744,12 +1744,18 @@ boundary preserves those states without a falsy test that would erase a genuine ## 9. Maintenance -`runMaintenance(type, target?)` ([`oracle.ts`](../../src/lib/db/providers/sql/oracle.ts)): +`runMaintenance(type, target?, container?)` ([`oracle.ts`](../../src/lib/db/providers/sql/oracle.ts)): + +A `container` is the OWNER the row carries as `schemaName` (#772). It moves every read off the +`USER_*` catalogs and onto their `ALL_*` twins with the owner bound, passes that owner as the +first `GATHER_TABLE_STATS` / `GATHER_SCHEMA_STATS` argument in place of `USER`, and qualifies +each `ALTER INDEX ""."" REBUILD`. Without one the connected user is the owner, +which is the reading every column below used before the parameter existed. | Type | With target | Without target | |------|-------------|----------------| -| `analyze` | `DBMS_STATS.GATHER_TABLE_STATS(USER, '')` | `DBMS_STATS.GATHER_SCHEMA_STATS(USER)` | -| `optimize` | rebuild the indexes THAT TABLE owns: `SELECT INDEX_NAME FROM USER_INDEXES WHERE TABLE_NAME = :t AND INDEX_TYPE = 'NORMAL'`, then `ALTER INDEX "" REBUILD` for each (own try/catch) | rebuild **every** normal user index (`USER_INDEXES`, each in its own try/catch) | +| `analyze` | `DBMS_STATS.GATHER_TABLE_STATS(, '')` | `DBMS_STATS.GATHER_SCHEMA_STATS()` | +| `optimize` | rebuild the indexes THAT TABLE owns: `SELECT INDEX_NAME FROM USER_INDEXES WHERE TABLE_NAME = :t AND INDEX_TYPE = 'NORMAL'` (or `ALL_INDEXES` with `OWNER = :owner`), then `ALTER INDEX "" REBUILD` for each (own try/catch) | rebuild **every** normal user index (`USER_INDEXES` / `ALL_INDEXES`, each in its own try/catch) | | `kill` | `ALTER SYSTEM KILL SESSION ''` | throws (`SID,SERIAL#` required) | `getCapabilities().maintenanceOperations = ['analyze', 'optimize', 'kill']`. Targets are diff --git a/docs/providers/postgres.md b/docs/providers/postgres.md index 3d6edab91..18e68fa65 100644 --- a/docs/providers/postgres.md +++ b/docs/providers/postgres.md @@ -1159,10 +1159,14 @@ Monitoring never hard-fails on a missing optional feature: ### 3.6 Safe maintenance targets -`qualifyMaintenanceTarget()` ([`postgres.ts`](../../src/lib/db/providers/sql/postgres.ts)) quotes -maintenance targets through `escapeIdentifier()`: a bare name defaults to the `public` schema; a -`schema.table` target is quoted per-part. This prevents identifier injection in `VACUUM`/`ANALYZE`/ -`REINDEX` statements (which cannot use bind parameters for object names). +`qualifyMaintenanceTarget(target, container)` ([`postgres.ts`](../../src/lib/db/providers/sql/postgres.ts)) +quotes maintenance targets through `escapeIdentifier()`. A caller that passes a `container` (the +`schemaName` the table row already carries) gets that schema, quoted whole, prefixed to the quoted +table name: the schema is never recovered by splitting the name, because a schema is allowed to +contain a dot and the split would land on the wrong side. Without a container the older readings +stay, so a bare name defaults to the `public` schema and a `schema.table` target is quoted per-part. +This prevents identifier injection in `VACUUM`/`ANALYZE`/`REINDEX` statements (which cannot use +bind parameters for object names). --- @@ -1642,7 +1646,7 @@ A pooled client left `idle in transaction` poisons every later user of that stor ## 9. Maintenance -`runMaintenance(type, target?)` ([`postgres.ts`](../../src/lib/db/providers/sql/postgres.ts)), +`runMaintenance(type, target?, container?)` ([`postgres.ts`](../../src/lib/db/providers/sql/postgres.ts)), with targets quoted via [§3.6](#36-safe-maintenance-targets): | Type | With target | Without target | diff --git a/docs/providers/sqlite.md b/docs/providers/sqlite.md index 31b9bf246..716276766 100644 --- a/docs/providers/sqlite.md +++ b/docs/providers/sqlite.md @@ -1071,8 +1071,10 @@ that very measurement, which is the mistake `|| 0` was making. ## 8. Maintenance -`runMaintenance(type, target?)` ([`sqlite.ts`](../../src/lib/db/providers/sql/sqlite.ts)); `analyze` -and `reindex` targets are quoted via `escapeIdentifier()`: +`runMaintenance(type, target?, container?)` ([`sqlite.ts`](../../src/lib/db/providers/sql/sqlite.ts)); `analyze` +and `reindex` targets are quoted via `escapeIdentifier()`. A `container` is deliberately ignored +(#772): SQLite resolves a bare name against the attached database it was opened on, always `main` +for this provider, and the file has no second namespace to name. | Type | Action | |------|--------| diff --git a/docs/providers/trino.md b/docs/providers/trino.md index cbf0473f1..30aff6f7c 100644 --- a/docs/providers/trino.md +++ b/docs/providers/trino.md @@ -1601,6 +1601,9 @@ every connector decides for itself whether it implements it, and measured, the ` answers `This connector does not support analyze` and no connector on the probe cluster implements it. A button that always fails is worse than a stated reason. +A `container` is deliberately ignored (#772): the only operation this provider performs is `kill`, +whose target is a query id rather than an object inside any namespace. + ### Where each operation may be offered (`maintenanceOperationSpecs`) Declaring that an operation EXISTS is not enough to put a button on it: two engines that diff --git a/src/app/api/db/maintenance/route.ts b/src/app/api/db/maintenance/route.ts index 1bd39ac85..236fd693f 100644 --- a/src/app/api/db/maintenance/route.ts +++ b/src/app/api/db/maintenance/route.ts @@ -24,7 +24,7 @@ export async function POST(request: Request) { try { const body = await request.json(); - const { type, target } = body; + const { type, target, container } = body; const connection = await resolveConnection(body, guard.session); @@ -32,6 +32,24 @@ export async function POST(request: Request) { return NextResponse.json({ error: "Maintenance type is required" }, { status: 400 }); } + // `container` is the row's `schemaName`: a string naming the container the target lives in, + // or absent for a request that names none. A non-string one is a malformed request, and it + // is answered HERE rather than allowed through to a provider, where the first use - an + // `identifier.replace` - threw a TypeError and the client read a 500 for a request this + // route could have refused by reading the field's type (#1091 review). Nothing is opened + // and nothing is run for it. + if (container !== undefined && typeof container !== "string") { + return NextResponse.json( + { error: `"container" must be a string naming the target's container` }, + { status: 400 }, + ); + } + + // An EMPTY string reads as the absence of a container, the same way an empty `target` reads + // as the whole-database form below. Nothing here may hand a provider a value that its own + // falsy test would have refused anyway, because the audit row below records what arrived. + const requestedContainer: string | undefined = container || undefined; + const provider = await getOrCreateProvider(connection); const capabilities = provider.getCapabilities(); @@ -102,7 +120,7 @@ export async function POST(request: Request) { } const startTime = Date.now(); - const result = await provider.runMaintenance(type, target); + const result = await provider.runMaintenance(type, target, requestedContainer); const duration = Date.now() - startTime; // Isolated in its own try/catch: runMaintenance() above has already succeeded and its result @@ -114,6 +132,12 @@ export async function POST(request: Request) { type: type === "kill" ? "kill_session" : "maintenance", action: type.toUpperCase(), target: target || "all", + // The container the request named, omitted when it named none. `app.orders` and + // `public.orders` recorded identically while this row carried only `target`, and an + // operator reconstructing what was done to a database could not tell the two apart + // (#1091 review). Optional on the EVENT the way `reason` and `bucket` are, so a + // whole-database row does not grow a field claiming a container it never had. + container: requestedContainer, connectionName: connection.name || connection.database || "unknown", user: guard.session.username || "admin", // The engine's verdict, not the request's. `runMaintenance` resolving is only the diff --git a/src/app/api/db/objects/edit-apply/route.ts b/src/app/api/db/objects/edit-apply/route.ts index 162f7f242..dc8a2d7af 100644 --- a/src/app/api/db/objects/edit-apply/route.ts +++ b/src/app/api/db/objects/edit-apply/route.ts @@ -43,17 +43,17 @@ const ROUTE = "api/db/objects/edit-apply"; * happens: the path fails closed on an unauditable write rather than performing it silently. That * is `src/lib/db/operations/execution.ts:11-20`'s rule for the agent path, applied here for the * same reason. The OUTCOME event is emitted after and IS wrapped, which is - * `src/app/api/db/maintenance/route.ts:108-131`'s rule and its stated reason: the engine has + * `src/app/api/db/maintenance/route.ts:123-152`'s rule and its stated reason: the engine has * already acted, and a broken sink must never turn a completed apply into a 500 that invites a * retry that would be a SECOND DDL. * * WHAT AN EVENT MAY NEVER CARRY: the statement, the command payload, the reader's text, the * pre-image, the engine's message, the engine's code, the revision token, the plan token, or any - * other part of the plan. `src/lib/audit.ts:456-461` forbids SQL text, request bodies and raw - * `Error.message` by name, `AuditReason` is closed precisely so no path can put a driver string - * into a record, and `MAX_AUDIT_FIELD_LENGTH` is 254, so an unvalidated string would be TRUNCATED - * rather than refused. What the two events carry instead is the object address, the kind, the part, - * the resolved strategy and one correlation id, which is `plan.planId`. + * other part of the plan. `emitAuditEvent`'s docblock in `src/lib/audit.ts` forbids SQL text, + * request bodies and raw `Error.message` by name, `AuditReason` is closed precisely so no path can + * put a driver string into a record, and `MAX_AUDIT_FIELD_LENGTH` is 254, so an unvalidated string + * would be TRUNCATED rather than refused. What the two events carry instead is the object address, + * the kind, the part, the resolved strategy and one correlation id, which is `plan.planId`. * * `target` IS `plan.path.join("/")`, which is design 5.5's own spelling and is used here unchanged, * with its limit named rather than discovered: it is a FOURTH spelling of a path key in a diff --git a/src/components/admin/tabs/OperationsTab.tsx b/src/components/admin/tabs/OperationsTab.tsx index c6c68dbd6..a42cbeb6f 100644 --- a/src/components/admin/tabs/OperationsTab.tsx +++ b/src/components/admin/tabs/OperationsTab.tsx @@ -240,12 +240,15 @@ export function OperationsTab() { [], ); - const handleRunMaintenance = async (type: string, target?: string) => { + // Same reason as TablesTab: `table.schemaName` is the namespace for every engine, and the + // row keys on it already. The log entry keeps naming the table alone, since that is what an + // operator reads back (#772). + const handleRunMaintenance = async (type: string, target?: string, container?: string) => { const actionId = `${type}-${target || "global"}`; setActionLoading(actionId); const start = Date.now(); try { - const success = await runMaintenance(type, target); + const success = await runMaintenance(type, target, container); const duration = Date.now() - start; addLogEntry(type.toUpperCase(), target || "all", success ? "success" : "failure", duration); } catch { @@ -598,7 +601,7 @@ export function OperationsTab() { variant="ghost" className={`w-7 h-7 text-fg-muted ${hover}`} title={label} - onClick={() => handleRunMaintenance(type, table.tableName)} + onClick={() => handleRunMaintenance(type, table.tableName, table.schemaName)} disabled={!!actionLoading} > {actionLoading === `${type}-${table.tableName}` ? ( diff --git a/src/components/monitoring/tabs/TablesTab.tsx b/src/components/monitoring/tabs/TablesTab.tsx index e331135a6..497e8d864 100644 --- a/src/components/monitoring/tabs/TablesTab.tsx +++ b/src/components/monitoring/tabs/TablesTab.tsx @@ -168,7 +168,7 @@ function bloatBadgeVariant(ratio: number): "destructive" | "outline" | "secondar interface TablesTabProps { data: MonitoringData | null; loading: boolean; - onRunMaintenance: (type: string, target?: string) => Promise; + onRunMaintenance: (type: string, target?: string, container?: string) => Promise; isAdmin?: boolean; /** * The connected provider's declared capabilities (issue #272). Undefined while @@ -255,9 +255,13 @@ export function TablesTab({ data, loading, onRunMaintenance, isAdmin = true, cap capabilities?.supportsMaintenance === true && capabilities.maintenanceOperations.includes("vacuum"); const vacuumStateKnown = !vacuumUnsupported && !statsAbsent; - const handleMaintenance = async (type: MaintenanceType, tableName: string) => { + // `schemaName` travels beside the table name because the table's namespace is not the same + // thing to every engine: a schema for PostgreSQL, the database for MySQL, an owner for + // Oracle. The row already renders `${schemaName}.${tableName}`, so the value was one + // property away and was being dropped here (#772). + const handleMaintenance = async (type: MaintenanceType, tableName: string, container?: string) => { setActionLoading(`${type}-${tableName}`); - await onRunMaintenance(type, tableName); + await onRunMaintenance(type, tableName, container); setActionLoading(null); }; @@ -428,7 +432,7 @@ export function TablesTab({ data, loading, onRunMaintenance, isAdmin = true, cap variant="ghost" size="icon" className={className} - onClick={() => handleMaintenance(type, table.tableName)} + onClick={() => handleMaintenance(type, table.tableName, table.schemaName)} disabled={!!actionLoading} title={label} > diff --git a/src/hooks/use-monitoring-data.ts b/src/hooks/use-monitoring-data.ts index e465e3784..af8821130 100644 --- a/src/hooks/use-monitoring-data.ts +++ b/src/hooks/use-monitoring-data.ts @@ -20,7 +20,7 @@ interface UseMonitoringDataReturn { setRefreshInterval: (ms: number) => void; refresh: () => Promise; killSession: (pid: number | string) => Promise; - runMaintenance: (type: string, target?: string) => Promise; + runMaintenance: (type: string, target?: string, container?: string) => Promise; } const DEFAULT_REFRESH_INTERVAL = 30000; // 30 seconds @@ -250,7 +250,7 @@ export function useMonitoringData( ); const runMaintenance = useCallback( - async (type: string, target?: string): Promise => { + async (type: string, target?: string, container?: string): Promise => { const currentConnection = connectionRef.current; if (!currentConnection) return false; @@ -261,6 +261,7 @@ export function useMonitoringData( body: JSON.stringify({ type, target, + container, ...buildConnectionPayload(currentConnection), }), }); diff --git a/src/lib/audit.ts b/src/lib/audit.ts index 1bd745c70..21263810c 100644 --- a/src/lib/audit.ts +++ b/src/lib/audit.ts @@ -187,6 +187,18 @@ export interface AuditEvent { type: AuditEventType; action: string; target: string; + /** + * The CONTAINER the target was addressed in, when the request named one: the schema of a + * PostgreSQL table, the database on ClickHouse, the bucket on a document store. It is what + * tells `app.orders` apart from `public.orders` in this log (#1091 review), so a maintenance + * row is two facts rather than one. + * + * Optional, and set only by the maintenance route: every other event's `target` already names + * a route rather than an object, and a field that claimed a container there would be a value + * its writer never meant. `toAuditLine` omits it entirely when it is unset, the way `reason` + * and `bucket` are omitted, so the line's shape does not grow a null. + */ + container?: string; connectionName?: string; user: string; result: "success" | "failure"; @@ -513,6 +525,7 @@ interface AuditLogLine { reason?: AuditReason; ip?: string; connection?: string; + container?: string; duration_ms?: number; bucket?: string; correlation_id?: string; @@ -531,6 +544,9 @@ function toAuditLine(event: AuditEvent): AuditLogLine { ...(event.reason ? { reason: event.reason } : {}), ...(event.ip && event.ip !== UNKNOWN_ADDRESS ? { ip: event.ip } : {}), ...(event.connectionName ? { connection: event.connectionName } : {}), + // The container beside the route, and omitted on the same terms: an event that named none + // must not publish a `container: null` a parser would read as a value (#1091 review). + ...(event.container ? { container: event.container } : {}), ...(event.bucket ? { bucket: event.bucket } : {}), ...(event.correlationId ? { correlation_id: event.correlationId } : {}), // Number.isFinite excludes NaN and +/-Infinity: JSON.stringify(NaN) silently produces `null`, diff --git a/src/lib/db/base-provider.ts b/src/lib/db/base-provider.ts index 53db0dbce..527210fe5 100644 --- a/src/lib/db/base-provider.ts +++ b/src/lib/db/base-provider.ts @@ -181,7 +181,11 @@ export abstract class BaseDatabaseProvider implements DatabaseProvider { ): Promise; public abstract getHealth(): Promise; - public abstract runMaintenance(type: MaintenanceType, target?: string): Promise; + public abstract runMaintenance( + type: MaintenanceType, + target?: string, + container?: string, + ): Promise; // Monitoring methods (must be implemented by subclasses) public abstract getOverview(): Promise; diff --git a/src/lib/db/providers/document/couchbase/index.ts b/src/lib/db/providers/document/couchbase/index.ts index 40a93bda3..4ee1ab055 100644 --- a/src/lib/db/providers/document/couchbase/index.ts +++ b/src/lib/db/providers/document/couchbase/index.ts @@ -94,7 +94,13 @@ import { SCOPES_SQL, } from "./objects"; import { comparePaths } from "@/lib/db/object-path"; -import { CouchbaseError, type CouchbaseQueryResult, type CouchbaseRow, type CouchbaseTransport } from "./transport"; +import { + CouchbaseError, + type CouchbaseQueryResult, + type CouchbaseRow, + type CouchbaseTransport, + type Keyspace, +} from "./transport"; // ============================================================================ // Constants @@ -1318,10 +1324,10 @@ export class CouchbaseProvider extends BaseDatabaseProvider { // Maintenance // ========================================================================== - public async runMaintenance(type: MaintenanceType, target?: string): Promise { + public async runMaintenance(type: MaintenanceType, target?: string, container?: string): Promise { const transport = this.requireTransport(); const { result, executionTime } = await this.measureExecution(() => - this.guarded(() => this.dispatchMaintenance(transport, type, target)), + this.guarded(() => this.dispatchMaintenance(transport, type, target, container)), ); return { ...result, executionTime }; } @@ -1330,12 +1336,13 @@ export class CouchbaseProvider extends BaseDatabaseProvider { transport: CouchbaseTransport, type: MaintenanceType, target?: string, + container?: string, ): Promise> { switch (type) { case "analyze": - return this.updateStatistics(transport, this.requireTarget(type, target)); + return this.updateStatistics(transport, this.requireTarget(type, target), container); case "reindex": - return this.buildDeferredIndexes(transport, this.requireTarget(type, target)); + return this.buildDeferredIndexes(transport, this.requireTarget(type, target), container); case "kill": return this.cancelRequest(transport, this.requireTarget(type, target)); } @@ -1358,12 +1365,18 @@ export class CouchbaseProvider extends BaseDatabaseProvider { private async updateStatistics( transport: CouchbaseTransport, target: string, + container?: string, ): Promise> { - const keyspace = keyspacePath(keyspaceFromDisplayName(this.bucket, target)); + const keyspace = keyspacePath(this.maintenanceKeyspace(target, container)); try { await transport.query(`UPDATE STATISTICS FOR ${keyspace} INDEX ALL`, { timeoutMs: this.queryTimeout }); - return { success: true, message: `Updated statistics for ${target}` }; + // The KEYSPACE, not the bare target. The statement addresses three segments, and a reply + // naming only the target reported `Updated statistics for travel` - the bucket - after + // touching its default collection alone, which is a different thing from what an operator + // would read (#1091 review). The path is the same value the statement carried, spelled by + // the same helper, so the two cannot describe different keyspaces. + return { success: true, message: `Updated statistics for ${keyspace}` }; } catch (error) { // UPDATE STATISTICS is Enterprise-only; a Community Edition cluster // answers "'Update Statistics' is an enterprise level feature." That @@ -1373,11 +1386,39 @@ export class CouchbaseProvider extends BaseDatabaseProvider { } } + /** + * The keyspace a maintenance call addresses, container and all. + * + * A container is the row's `schemaName`. For this provider every Tables row is the + * bucket-level one: `getTableStats()` reports the bucket under BOTH `schemaName` and + * `tableName`, and that row means the bucket's DEFAULT collection - the placement + * `resolveKeyspaceOf()` gives every bucket-level catalog row (`objects.ts`). Reading that + * bucket back as a scope built `travel`.`travel`.`travel`, which is no keyspace at all + * (#1091 review). A container that is any other name is the scope the collection sits in, + * so it is used as one rather than parsed out of a display name: the row already knows + * where it lives, and `keyspaceFromDisplayName` cannot tell a scope name from a collection + * name that contains a dot (#772). Without a container the display-name reading stands. + */ + private maintenanceKeyspace(target: string, container?: string): Keyspace { + if (container === this.bucket) { + return { + bucket: this.bucket, + scope: COUCHBASE_DEFAULT_SCOPE, + collection: target === this.bucket ? COUCHBASE_DEFAULT_COLLECTION : target, + }; + } + if (container) { + return { bucket: this.bucket, scope: container, collection: target }; + } + return keyspaceFromDisplayName(this.bucket, target); + } + private async buildDeferredIndexes( transport: CouchbaseTransport, target: string, + container?: string, ): Promise> { - const keyspace = keyspaceFromDisplayName(this.bucket, target); + const keyspace = this.maintenanceKeyspace(target, container); const deferred = await transport.query(DEFERRED_INDEX_SQL, { args: [keyspace.bucket, keyspace.scope, keyspace.collection], timeoutMs: CATALOG_TIMEOUT_MS, diff --git a/src/lib/db/providers/document/mongodb.ts b/src/lib/db/providers/document/mongodb.ts index 0172f0945..e81c91110 100644 --- a/src/lib/db/providers/document/mongodb.ts +++ b/src/lib/db/providers/document/mongodb.ts @@ -1265,7 +1265,29 @@ export class MongoDBProvider extends BaseDatabaseProvider { // Maintenance Operations // ============================================================================ - public async runMaintenance(type: MaintenanceType, target?: string): Promise { + /** + * A maintenance command runs on the database the provider is bound to, and MongoDB has no + * way to retarget one mid-command. A container naming a DIFFERENT database is refused + * rather than quietly acted on against the bound one, which is what #843 is about; the + * bound database itself is accepted so a caller that echoes it back still works. + * + * The comparison is against `getDatabaseName()`, the name `connect()` actually opened, and + * not against `config.database` alone: a connection-string connection sets no + * `config.database`, so comparing with it refused the database the provider IS bound to and + * every per-collection button on that connection answered `bound to the database ""`. + */ + private assertContainerIsBound(container?: string): void { + const bound = this.getDatabaseName(); + if (container && container !== bound) { + throw new QueryError( + `This connection is bound to the database "${bound}", so it cannot run maintenance in "${container}".`, + "mongodb", + ); + } + } + + public async runMaintenance(type: MaintenanceType, target?: string, container?: string): Promise { + this.assertContainerIsBound(container); this.ensureConnected(); const { result, executionTime } = await this.measureExecution(async () => { diff --git a/src/lib/db/providers/sql/clickhouse/index.ts b/src/lib/db/providers/sql/clickhouse/index.ts index f1bc0f0d7..060acd08b 100644 --- a/src/lib/db/providers/sql/clickhouse/index.ts +++ b/src/lib/db/providers/sql/clickhouse/index.ts @@ -602,6 +602,32 @@ export class ClickHouseProvider extends SQLBaseProvider { }; } + // ========================================================================== + // SQL dialect overrides + // ========================================================================== + + /** + * A double-quoted identifier, with the BACKSLASH escaped before the quote. + * + * The inherited escaper doubles only the quote character, and on this engine a backslash + * inside a quoted identifier is processed as an ESCAPE - MEASURED (#789 probe 11): a table + * created as `"x\\"` stores `hex(name) = 785C`, exactly one trailing backslash, and a name + * ending in one therefore SWALLOWS its own closing quote while the parser keeps reading into + * whatever the name was followed by. Over a maintenance target that is statement injection + * rather than a quoting inconvenience (#1091 review): a container of `x\\` emitted through the + * inherited spelling turns the target that follows into more statement text. `objects.ts` + * documents the same measurement where it explains why that file's reads take no identifier + * position at all. + * + * The ORDER is the one `literal()` in `objects.ts` uses, and it is forced: doubling the quote + * first would leave the backslash that precedes the original quote looking like an escape of + * the quote's own doubled pair. + */ + protected override escapeIdentifier(identifier: string): string { + const escaped = identifier.replace(/\\/g, "\\\\").replace(/"/g, '""'); + return `"${escaped}"`; + } + /** * The inherited limiter appends `LIMIT n` at the very END of the statement, * and ClickHouse allows `FORMAT x` and `SETTINGS ...` as TRAILING clauses, so @@ -1062,10 +1088,10 @@ export class ClickHouseProvider extends SQLBaseProvider { // Maintenance // ========================================================================== - public async runMaintenance(type: MaintenanceType, target?: string): Promise { + public async runMaintenance(type: MaintenanceType, target?: string, container?: string): Promise { const transport = this.requireTransport(); const { result, executionTime } = await this.measureExecution(() => - this.guarded(() => this.dispatchMaintenance(transport, type, target)), + this.guarded(() => this.dispatchMaintenance(transport, type, target, container)), ); return { ...result, executionTime }; @@ -1075,15 +1101,16 @@ export class ClickHouseProvider extends SQLBaseProvider { transport: ClickHouseTransport, type: MaintenanceType, target?: string, + container?: string, ): Promise> { switch (type) { case "optimize": - return this.optimizeTable(transport, this.requireTarget(type, target)); + return this.optimizeTable(transport, this.requireTarget(type, target), container); // No target is legitimate here, unlike optimize: MaintenanceModal's global // Analyze button sends none, and a database's parts are as well defined as a // table's. Demanding one made a control the UI always offers always fail. case "analyze": - return this.describeParts(transport, target); + return this.describeParts(transport, target, container); case "kill": return this.cancelQueryById(transport, this.requireTarget(type, target)); } @@ -1103,7 +1130,13 @@ export class ClickHouseProvider extends SQLBaseProvider { return target; } - private qualify(target: string): string { + private qualify(target: string, container?: string): string { + // A caller-supplied container removes the dot ambiguity that `splitTarget` documents + // below: `database.table` cannot be told apart from a name that contains a dot, while a + // container is already the database on its own. + if (container) { + return `${this.escapeIdentifier(container)}.${this.escapeIdentifier(target)}`; + } const [database, table] = splitTarget(target, this.pinnedDatabase); return `${this.escapeIdentifier(database)}.${this.escapeIdentifier(table)}`; } @@ -1116,8 +1149,9 @@ export class ClickHouseProvider extends SQLBaseProvider { private async optimizeTable( transport: ClickHouseTransport, target: string, + container?: string, ): Promise> { - await transport.query(`OPTIMIZE TABLE ${this.qualify(target)} FINAL`); + await transport.query(`OPTIMIZE TABLE ${this.qualify(target, container)} FINAL`); return { success: true, message: `Optimized ${target}` }; } @@ -1130,10 +1164,16 @@ export class ClickHouseProvider extends SQLBaseProvider { private async describeParts( transport: ClickHouseTransport, target?: string, + container?: string, ): Promise> { - // Without a target the scope is the whole pinned database, which is what the - // global Analyze button asks for. - const [database, table] = target ? splitTarget(target, this.pinnedDatabase) : [this.pinnedDatabase, undefined]; + // Without a target the scope is the whole pinned database, which is what the global + // Analyze button asks for. A container states the database outright, so the name is not + // split to recover one; without a container the old reading stands. + const [database, table] = container + ? [container, target] + : target + ? splitTarget(target, this.pinnedDatabase) + : [this.pinnedDatabase, undefined]; const scope = target ?? database; const where = table ? `database = ${literal(database)} AND table = ${literal(table)}` diff --git a/src/lib/db/providers/sql/duckdb/index.ts b/src/lib/db/providers/sql/duckdb/index.ts index 79f56ec8c..9bbbd8ee1 100644 --- a/src/lib/db/providers/sql/duckdb/index.ts +++ b/src/lib/db/providers/sql/duckdb/index.ts @@ -1272,7 +1272,12 @@ export class DuckDBProvider extends SQLBaseProvider { * resolved into `main`, DuckDB's default schema; `schema.table` is quoted part by * part. Mirrors `postgres.ts`'s `qualifyMaintenanceTarget`. */ - private qualifyMaintenanceTarget(target: string): string { + private qualifyMaintenanceTarget(target: string, container?: string): string { + // A caller-supplied container is authoritative: `main` is only the fallback for a name + // that arrives without one, and a container can itself contain a dot. + if (container) { + return this.escapeIdentifier(container) + "." + this.escapeIdentifier(target); + } if (target.includes(".")) { return target .split(".") @@ -1282,11 +1287,11 @@ export class DuckDBProvider extends SQLBaseProvider { return `${this.escapeIdentifier(DEFAULT_SCHEMA)}.${this.escapeIdentifier(target)}`; } - public async runMaintenance(type: MaintenanceType, target?: string): Promise { + public async runMaintenance(type: MaintenanceType, target?: string, container?: string): Promise { this.ensureConnected(); const { result, executionTime } = await this.measureExecution(async () => { - const qualified = target ? this.qualifyMaintenanceTarget(target) : ""; + const qualified = target ? this.qualifyMaintenanceTarget(target, container) : ""; let sql = ""; switch (type) { diff --git a/src/lib/db/providers/sql/libsql/index.ts b/src/lib/db/providers/sql/libsql/index.ts index 965d3c6b5..0d501797e 100644 --- a/src/lib/db/providers/sql/libsql/index.ts +++ b/src/lib/db/providers/sql/libsql/index.ts @@ -449,8 +449,11 @@ export class LibSQLProvider extends SQLBaseProvider { * `check` reads the answer rather than the status: `PRAGMA integrity_check` * succeeds as a statement and reports the damage in its row, so a provider that * only checked for an exception would report a corrupt database as healthy. + * + * `container` is deliberately ignored, for the same reason as `sqlite.ts`: a libSQL + * connection resolves names against its one attached database (#772). */ - public async runMaintenance(type: MaintenanceType, target?: string): Promise { + public async runMaintenance(type: MaintenanceType, target?: string, _container?: string): Promise { const transport = this.requireTransport(); const { result, executionTime } = await this.measureExecution(async () => { diff --git a/src/lib/db/providers/sql/mssql.ts b/src/lib/db/providers/sql/mssql.ts index 0a1559985..bb8086ec0 100644 --- a/src/lib/db/providers/sql/mssql.ts +++ b/src/lib/db/providers/sql/mssql.ts @@ -3148,7 +3148,19 @@ export class MSSQLProvider extends SQLBaseProvider { // Maintenance Operations // ============================================================================ - public async runMaintenance(type: MaintenanceType, target?: string): Promise { + /** + * A maintenance target as a schema-qualified identifier, using the `[bracket]` quoting this + * provider already escapes with. A container is honoured when the caller sends one; a bare + * name keeps the previous reading, where the connected default schema applies. + */ + private qualifyMaintenanceTarget(target: string, container?: string): string { + if (container) { + return `${this.escapeIdentifier(container)}.${this.escapeIdentifier(target)}`; + } + return this.escapeIdentifier(target); + } + + public async runMaintenance(type: MaintenanceType, target?: string, container?: string): Promise { this.ensureConnected(); const { result, executionTime } = await this.measureExecution(async () => { @@ -3158,7 +3170,7 @@ export class MSSQLProvider extends SQLBaseProvider { switch (type) { case "analyze": if (target) { - sql = `UPDATE STATISTICS [${target.replace(/\]/g, "]]")}]`; + sql = `UPDATE STATISTICS ${this.qualifyMaintenanceTarget(target, container)}`; } else { sql = `EXEC sp_updatestats`; } @@ -3168,7 +3180,7 @@ export class MSSQLProvider extends SQLBaseProvider { break; case "optimize": if (target) { - sql = `ALTER INDEX ALL ON [${target.replace(/\]/g, "]]")}] REBUILD`; + sql = `ALTER INDEX ALL ON ${this.qualifyMaintenanceTarget(target, container)} REBUILD`; } else { sql = REBUILD_ALL_INDEXES_SQL; } diff --git a/src/lib/db/providers/sql/mysql.ts b/src/lib/db/providers/sql/mysql.ts index ff8d43900..d445fc12f 100644 --- a/src/lib/db/providers/sql/mysql.ts +++ b/src/lib/db/providers/sql/mysql.ts @@ -2937,7 +2937,21 @@ export class MySQLProvider extends SQLBaseProvider { // Maintenance Operations // ============================================================================ - public async runMaintenance(type: MaintenanceType, target?: string): Promise { + /** + * A maintenance target as a `database.table` identifier, qualified only when the caller's + * container is a database OTHER than the connected one. A MySQL statement already resolves + * a bare table inside the connected database, so qualifying with the same name would add a + * prefix the engine reads as redundant, and a container that is not a database at all is + * not something this engine can act on. + */ + private qualifyMaintenanceTarget(target: string, container?: string): string { + if (container && container !== this.config.database) { + return `${this.escapeIdentifier(container)}.${this.escapeIdentifier(target)}`; + } + return this.escapeIdentifier(target); + } + + public async runMaintenance(type: MaintenanceType, target?: string, container?: string): Promise { this.ensureConnected(); const { result, executionTime } = await this.measureExecution(async () => { @@ -2952,7 +2966,9 @@ export class MySQLProvider extends SQLBaseProvider { case "analyze": case "optimize": case "check": { - const tables = target ? this.escapeIdentifier(target) : await this.getAllTablesForMaintenance(conn); + const tables = target + ? this.qualifyMaintenanceTarget(target, container) + : await this.getAllTablesForMaintenance(conn); // An empty database joined to an empty list, and `OPTIMIZE TABLE ` alone is // a syntax error - measured through the provider against a database with no // tables on 2026-08-25: "You have an error in your SQL syntax ... near ''". diff --git a/src/lib/db/providers/sql/oracle.ts b/src/lib/db/providers/sql/oracle.ts index 5c69a93f3..d495a1ba9 100644 --- a/src/lib/db/providers/sql/oracle.ts +++ b/src/lib/db/providers/sql/oracle.ts @@ -210,6 +210,26 @@ const SCHEMA_NORMAL_INDEXES_SQL = `SELECT INDEX_NAME FROM USER_INDEXES WHERE IND */ const TABLE_IS_KNOWN_SQL = `SELECT TABLE_NAME FROM USER_TABLES WHERE TABLE_NAME = :tableName`; +// ============================================================================ +// Owner-aware twins (#772) +// ---------------------------------------------------------------------------- +// The `USER_*` views above answer for the CONNECTED user only, which is why a table +// owned by anyone else read as "this schema owns no TABLE named ...". `ALL_*` carries +// an OWNER column, so the same questions are asked with the owner bound. Kept as +// separate statements rather than adding an `OR :owner IS NULL` to the originals: a +// predicate that switches off is a predicate the optimizer cannot plan around, and the +// two call shapes want different proofs. +// ============================================================================ +const OWNED_TABLE_INDEXES_SQL = `SELECT INDEX_NAME + FROM ALL_INDEXES + WHERE OWNER = :owner AND TABLE_NAME = :tableName AND INDEX_TYPE = 'NORMAL'`; + +const OWNED_SCHEMA_NORMAL_INDEXES_SQL = `SELECT INDEX_NAME + FROM ALL_INDEXES + WHERE OWNER = :owner AND INDEX_TYPE = 'NORMAL'`; + +const OWNED_TABLE_IS_KNOWN_SQL = `SELECT TABLE_NAME FROM ALL_TABLES WHERE OWNER = :owner AND TABLE_NAME = :tableName`; + // ============================================================================ // Object surface SQL (#789) // ---------------------------------------------------------------------------- @@ -2450,7 +2470,7 @@ export class OracleProvider extends SQLBaseProvider { // Maintenance Operations // ============================================================================ - public async runMaintenance(type: MaintenanceType, target?: string): Promise { + public async runMaintenance(type: MaintenanceType, target?: string, container?: string): Promise { this.ensureConnected(); const { result, executionTime } = await this.measureExecution(async () => { @@ -2459,16 +2479,23 @@ export class OracleProvider extends SQLBaseProvider { conn = await this.pool!.getConnection(); let sql = ""; + // What stands in the owner position of the DBMS_STATS calls: the connected user + // when the caller named no container, else the container as a quoted literal. + // `USER` is a keyword rather than a string, so it is NOT quoted. + const ownerArg = container ? `'${container.replace(/'/g, "''")}'` : "USER"; + switch (type) { case "analyze": - if (target) { - sql = `BEGIN DBMS_STATS.GATHER_TABLE_STATS(USER, '${target.replace(/'/g, "''")}'); END;`; - } else { - sql = `BEGIN DBMS_STATS.GATHER_SCHEMA_STATS(USER); END;`; - } + // `USER` is the connected user, so an owner named by the caller is passed + // through instead. Both arguments are inline-escaped literals because + // DBMS_STATS takes no binds for them, and an owner is upper-cased the way the + // data dictionary stores it unless the caller quoted the identifier. + sql = target + ? `BEGIN DBMS_STATS.GATHER_TABLE_STATS(${ownerArg}, '${target.replace(/'/g, "''")}'); END;` + : `BEGIN DBMS_STATS.GATHER_SCHEMA_STATS(${ownerArg}); END;`; break; case "optimize": - return await this.rebuildIndexes(conn, target); + return await this.rebuildIndexes(conn, target, container); case "kill": if (!target) { throw new QueryError("Target SID,SERIAL# is required for kill operation", "oracle"); @@ -2532,17 +2559,26 @@ export class OracleProvider extends SQLBaseProvider { private async rebuildIndexes( conn: oracledb.Connection, target?: string, + owner?: string, ): Promise<{ success: boolean; message: string }> { // The table name is a bind here, unlike the inline-escaped literals elsewhere in - // runMaintenance: this one sits in a WHERE clause, which does take a bind. - const indexes = target - ? await conn.execute(TABLE_INDEXES_SQL, [target], { outFormat: oracledb.OUT_FORMAT_OBJECT }) - : await conn.execute(SCHEMA_NORMAL_INDEXES_SQL, [], { outFormat: oracledb.OUT_FORMAT_OBJECT }); + // runMaintenance: this one sits in a WHERE clause, which does take a bind. An owner + // switches the question to the `ALL_*` catalogs, which answer for any schema rather + // than the connected one. + const indexes = owner + ? target + ? await conn.execute(OWNED_TABLE_INDEXES_SQL, [owner, target], { outFormat: oracledb.OUT_FORMAT_OBJECT }) + : await conn.execute(OWNED_SCHEMA_NORMAL_INDEXES_SQL, [owner], { outFormat: oracledb.OUT_FORMAT_OBJECT }) + : target + ? await conn.execute(TABLE_INDEXES_SQL, [target], { outFormat: oracledb.OUT_FORMAT_OBJECT }) + : await conn.execute(SCHEMA_NORMAL_INDEXES_SQL, [], { outFormat: oracledb.OUT_FORMAT_OBJECT }); const rows = (indexes.rows || []) as Record[]; if (target && rows.length === 0) { - const known = await conn.execute(TABLE_IS_KNOWN_SQL, [target], { outFormat: oracledb.OUT_FORMAT_OBJECT }); + const known = owner + ? await conn.execute(OWNED_TABLE_IS_KNOWN_SQL, [owner, target], { outFormat: oracledb.OUT_FORMAT_OBJECT }) + : await conn.execute(TABLE_IS_KNOWN_SQL, [target], { outFormat: oracledb.OUT_FORMAT_OBJECT }); if (((known.rows || []) as unknown[]).length === 0) { return { success: false, @@ -2553,7 +2589,7 @@ export class OracleProvider extends SQLBaseProvider { // too, and blaming only the spelling would misdirect a caller who spelled it // right. `USER_TABLES` does hold a materialized view's container table, so that // case reaches the rebuild rather than this branch. - message: `OPTIMIZE failed: this schema owns no TABLE named ${target}. A view or a synonym has no index to rebuild, and an unquoted name is folded to upper case, so a lower-case spelling will not match the catalog.`, + message: `OPTIMIZE failed: ${owner ? `the schema "${owner}"` : "this schema"} owns no TABLE named ${target}. A view or a synonym has no index to rebuild, and an unquoted name is folded to upper case, so a lower-case spelling will not match the catalog.`, }; } } @@ -2566,8 +2602,13 @@ export class OracleProvider extends SQLBaseProvider { // and this reported success in 14 ms. let firstFailure: string | undefined; for (const row of rows) { + // With an owner this list came from `ALL_INDEXES`, which answers for any schema, so the + // rebuild names that owner too: a bare `ALTER INDEX` rebuilds in the CONNECTED schema, + // which is not the schema the indexes were read from (#1091 review). + const indexName = `"${String(row.INDEX_NAME).replace(/"/g, '""')}"`; + const qualified = owner ? `"${owner.replace(/"/g, '""')}".${indexName}` : indexName; try { - await conn.execute(`ALTER INDEX "${String(row.INDEX_NAME).replace(/"/g, '""')}" REBUILD`); + await conn.execute(`ALTER INDEX ${qualified} REBUILD`); rebuilt++; } catch (error) { // One index failing is still a completed run (an offline tablespace or an unusable diff --git a/src/lib/db/providers/sql/postgres.ts b/src/lib/db/providers/sql/postgres.ts index e863ee5aa..e0e324d90 100644 --- a/src/lib/db/providers/sql/postgres.ts +++ b/src/lib/db/providers/sql/postgres.ts @@ -4182,8 +4182,15 @@ export class PostgresProvider extends SQLBaseProvider { * Bare table names default to the public schema; "schema.table" is quoted * per-part. Returns an empty string when no target is given. */ - private qualifyMaintenanceTarget(target?: string): string { + private qualifyMaintenanceTarget(target?: string, container?: string): string { if (!target) return ""; + // An explicit container wins over anything the name appears to carry: a container can + // legitimately contain a dot, and splitting a name to recover it is the ambiguity this + // parameter exists to remove. Without one the old readings stay, so existing callers do + // not change behaviour. + if (container) { + return this.escapeIdentifier(container) + "." + this.escapeIdentifier(target); + } if (target.includes(".")) { return target .split(".") @@ -4193,16 +4200,16 @@ export class PostgresProvider extends SQLBaseProvider { return "public." + this.escapeIdentifier(target); } - public async runMaintenance(type: MaintenanceType, target?: string): Promise { + public async runMaintenance(type: MaintenanceType, target?: string, container?: string): Promise { this.ensureConnected(); const { result, executionTime } = await this.measureExecution(async () => { const client = await this.pool!.connect(); try { let sql = ""; - // Resolve target into a schema-qualified, quoted identifier (defaults to - // the public schema for bare names; "schema.table" is also supported). - const qualifiedTarget = this.qualifyMaintenanceTarget(target); + // Resolve target into a schema-qualified, quoted identifier: the caller's container + // when there is one, else "schema.table", else the public schema for bare names. + const qualifiedTarget = this.qualifyMaintenanceTarget(target, container); switch (type) { case "vacuum": diff --git a/src/lib/db/providers/sql/sqlite.ts b/src/lib/db/providers/sql/sqlite.ts index 4ed988ec8..1c8dcd31e 100644 --- a/src/lib/db/providers/sql/sqlite.ts +++ b/src/lib/db/providers/sql/sqlite.ts @@ -2034,7 +2034,12 @@ export class SQLiteProvider extends SQLBaseProvider { // Maintenance Operations // ============================================================================ - public async runMaintenance(type: MaintenanceType, target?: string): Promise { + /** + * `container` is deliberately ignored: SQLite resolves a bare name against the attached + * database it was opened on, always `main` for this provider, and the file has no second + * namespace to name (#772). The parameter is accepted so the shared contract holds. + */ + public async runMaintenance(type: MaintenanceType, target?: string, _container?: string): Promise { this.ensureConnected(); const { result, executionTime } = await this.measureExecution(async () => { diff --git a/src/lib/db/providers/sql/trino/index.ts b/src/lib/db/providers/sql/trino/index.ts index 27c1a8a60..8b98726fd 100644 --- a/src/lib/db/providers/sql/trino/index.ts +++ b/src/lib/db/providers/sql/trino/index.ts @@ -2008,8 +2008,11 @@ export class TrinoProvider extends SQLBaseProvider { * swallowed here, unlike in `cancelQuery`: a user who typed a query id into a * maintenance panel has asked a direct question, and "that statement is not * running" is the answer. + * + * `container` is deliberately ignored: the only operation this provider performs is + * `kill`, whose target is a query id rather than an object inside any namespace (#772). */ - public async runMaintenance(type: MaintenanceType, target?: string): Promise { + public async runMaintenance(type: MaintenanceType, target?: string, _container?: string): Promise { const transport = this.requireTransport(); if (type !== "kill") { diff --git a/src/lib/db/types.ts b/src/lib/db/types.ts index ab1b4c88a..4f88f9d41 100644 --- a/src/lib/db/types.ts +++ b/src/lib/db/types.ts @@ -1267,8 +1267,13 @@ export interface DatabaseProvider { * Run maintenance operations * @param type - Type of maintenance operation * @param target - Optional target (table name or process ID) + * @param container - Optional namespace the target lives in. What a container means is + * per-engine and the provider decides: a schema for PostgreSQL, DuckDB and SQL Server, a + * database for MySQL and ClickHouse, an owner for Oracle, a bucket or scope for Couchbase, + * the attached database for SQLite and libSQL. Providers that cannot act on one ignore it + * rather than guessing a dialect from the target string. */ - runMaintenance(type: MaintenanceType, target?: string): Promise; + runMaintenance(type: MaintenanceType, target?: string, container?: string): Promise; /** * Validate provider configuration diff --git a/tests/api/db/maintenance.test.ts b/tests/api/db/maintenance.test.ts index c48bce449..13e330de1 100644 --- a/tests/api/db/maintenance.test.ts +++ b/tests/api/db/maintenance.test.ts @@ -163,6 +163,99 @@ describe("POST /api/db/maintenance", () => { expect(data.message).toBe("OK"); }); + // #772: the wire contract carries the table's container beside its target, and the route + // passes it through untouched. Reading only `target` was the whole defect - every provider + // then had to guess a namespace from a bare name. + test("passes the container through to the provider", async () => { + const req = createMockRequest("/api/db/maintenance", { + method: "POST", + body: { type: "vacuum", target: "users", container: "reporting", connection: validConnection }, + }); + + const res = await POST(req as never); + + expect(res.status).toBe(200); + expect(mockProvider.runMaintenance).toHaveBeenCalledWith("vacuum", "users", "reporting"); + }); + + // #1091 review: the audit row recorded only `target`, so `app.orders` and `public.orders` + // logged identically. The container is what tells them apart. + test("the audit event records the container beside the target", async () => { + const req = createMockRequest("/api/db/maintenance", { + method: "POST", + body: { type: "vacuum", target: "orders", container: "app", connection: validConnection }, + }); + + await POST(req as never); + + expect(mockAuditPush).toHaveBeenCalledTimes(1); + const event = mockAuditPush.mock.calls[0]![0] as Record; + expect(event.target).toBe("orders"); + expect(event.container).toBe("app"); + }); + + test("a whole-database request records no container, and the target keeps its `all` fallback", async () => { + const req = createMockRequest("/api/db/maintenance", { + method: "POST", + body: { type: "vacuum", connection: validConnection }, + }); + + await POST(req as never); + + expect(mockAuditPush).toHaveBeenCalledTimes(1); + const event = mockAuditPush.mock.calls[0]![0] as Record; + expect(event.target).toBe("all"); + expect(event.container).toBeUndefined(); + }); + + test("a request with no container reaches the provider with it undefined", async () => { + const req = createMockRequest("/api/db/maintenance", { + method: "POST", + body: { type: "vacuum", target: "users", connection: validConnection }, + }); + + await POST(req as never); + + expect(mockProvider.runMaintenance).toHaveBeenCalledWith("vacuum", "users", undefined); + }); + + // #1091 review: a non-string container used to reach the provider, where it failed as + // `identifier.replace is not a function` - a 500 for what is a malformed request. The route + // answers 400 and runs nothing. + test.each<[string, unknown]>([ + ["an object", { schema: "app" }], + ["a number", 7], + ["an array", ["app"]], + ["null", null], + ["false", false], + ])("a non-string container (%s) answers 400 and runs nothing", async (_label, container) => { + const req = createMockRequest("/api/db/maintenance", { + method: "POST", + body: { type: "vacuum", target: "users", container, connection: validConnection }, + }); + + const res = await POST(req as never); + const data = await parseResponseJSON<{ error: string }>(res); + + expect(res.status).toBe(400); + expect(data.error).toContain("container"); + expect(mockProvider.runMaintenance).not.toHaveBeenCalled(); + expect(mockAuditPush).not.toHaveBeenCalled(); + }); + + test("an empty-string container is a container the caller omitted, not a malformed one", async () => { + // The same reading `target: ""` gets below: a falsy string is the absence of the field. + const req = createMockRequest("/api/db/maintenance", { + method: "POST", + body: { type: "vacuum", target: "users", container: "", connection: validConnection }, + }); + + const res = await POST(req as never); + + expect(res.status).toBe(200); + expect(mockProvider.runMaintenance).toHaveBeenCalledWith("vacuum", "users", undefined); + }); + // Threat: runMaintenance() has already completed by the time emitAuditEvent runs (see the route's // own comment). Before this fix, a broken audit sink shared the operation's try/catch, so a // throw here would 500 a client that must not be told to retry an operation - possibly a diff --git a/tests/api/db/objects/edit-apply.test.ts b/tests/api/db/objects/edit-apply.test.ts index db850ab16..d3763871f 100644 --- a/tests/api/db/objects/edit-apply.test.ts +++ b/tests/api/db/objects/edit-apply.test.ts @@ -253,7 +253,7 @@ describe("POST /api/db/objects/edit-apply", () => { }); test("the OUTCOME event's sink throwing leaves the 200 intact", async () => { - // `src/app/api/db/maintenance/route.ts:108-131` verbatim, including its stated reason: the + // `src/app/api/db/maintenance/route.ts:123-152` verbatim, including its stated reason: the // engine has already acted and a broken sink must not turn a completed apply into a 500 that // invites a retry that would be a SECOND DDL. const sealed = await mintValidPlan(); @@ -332,7 +332,7 @@ describe("POST /api/db/objects/edit-apply", () => { test("the audit names the connection and the caller, and falls back through the arms in order", async () => { // Fix round 1, finding 8. `connectionName` is `name || database || "unknown"`, inherited - // verbatim from `src/app/api/db/maintenance/route.ts:117`, and BOTH its fallback arms had no + // verbatim from `src/app/api/db/maintenance/route.ts:138`, and BOTH its fallback arms had no // population: the harness fixture always carries a name, so `?? "unknown"` on that line killed // nothing. `resolveConnection` returns an INLINE caller-supplied connection object verbatim, so // a connection whose `name` is empty is the caller's to send and is the live population here. diff --git a/tests/components/admin/OperationsTab.test.tsx b/tests/components/admin/OperationsTab.test.tsx index 654c8dc97..76b02e199 100644 --- a/tests/components/admin/OperationsTab.test.tsx +++ b/tests/components/admin/OperationsTab.test.tsx @@ -707,7 +707,7 @@ describe("OperationsTab", () => { fireEvent.click(analyzeBtn!.closest("button")!); }); - expect(mockRunMaintenance).toHaveBeenCalledWith("analyze", undefined); + expect(mockRunMaintenance).toHaveBeenCalledWith("analyze", undefined, undefined); // Operation log should appear with success expect(queryByText("Operation Log (this session)")).not.toBeNull(); expect(queryByText("ANALYZE")).not.toBeNull(); @@ -724,7 +724,7 @@ describe("OperationsTab", () => { await act(async () => { fireEvent.click(vacuumBtn!.closest("button")!); }); - expect(mockRunMaintenance).toHaveBeenCalledWith("vacuum", undefined); + expect(mockRunMaintenance).toHaveBeenCalledWith("vacuum", undefined, undefined); expect(queryByText("VACUUM")).not.toBeNull(); }); @@ -739,7 +739,7 @@ describe("OperationsTab", () => { await act(async () => { fireEvent.click(reindexBtn!.closest("button")!); }); - expect(mockRunMaintenance).toHaveBeenCalledWith("reindex", undefined); + expect(mockRunMaintenance).toHaveBeenCalledWith("reindex", undefined, undefined); expect(queryByText("REINDEX")).not.toBeNull(); }); @@ -815,7 +815,7 @@ describe("OperationsTab", () => { fireEvent.click(buttons[0]!); }); - expect(mockRunMaintenance).toHaveBeenCalledWith("analyze", "users"); + expect(mockRunMaintenance).toHaveBeenCalledWith("analyze", "users", "public"); }); test("per-table vacuum button calls runMaintenance with table name", async () => { @@ -832,7 +832,7 @@ describe("OperationsTab", () => { fireEvent.click(buttons[1]!); }); - expect(mockRunMaintenance).toHaveBeenCalledWith("vacuum", "users"); + expect(mockRunMaintenance).toHaveBeenCalledWith("vacuum", "users", "public"); }); // ========================================================================= @@ -1695,7 +1695,7 @@ describe("OperationsTab", () => { fireEvent.click(button!); }); - expect(mockRunMaintenance).toHaveBeenCalledWith("optimize", undefined); + expect(mockRunMaintenance).toHaveBeenCalledWith("optimize", undefined, undefined); }); test("an operation with no whole-database form gets no global card", async () => { @@ -1768,7 +1768,7 @@ describe("OperationsTab", () => { }); // The target is what made this control honest: "users" is the collection row. - expect(mockRunMaintenance).toHaveBeenCalledWith("reindex", "users"); + expect(mockRunMaintenance).toHaveBeenCalledWith("reindex", "users", "public"); }); test("an operation that ignores its target gets no per-row control", async () => { diff --git a/tests/components/monitoring/TablesTab.test.tsx b/tests/components/monitoring/TablesTab.test.tsx index 951150571..756e8739f 100644 --- a/tests/components/monitoring/TablesTab.test.tsx +++ b/tests/components/monitoring/TablesTab.test.tsx @@ -174,21 +174,21 @@ describe("TablesTab", () => { expect(reindexButton).not.toBeNull(); fireEvent.click(analyzeButton!); - expect(onRunMaintenance).toHaveBeenCalledWith("analyze", "users"); + expect(onRunMaintenance).toHaveBeenCalledWith("analyze", "users", "public"); expect(vacuumButton!.disabled).toBe(true); await waitFor(() => { expect(vacuumButton!.disabled).toBe(false); }); fireEvent.click(vacuumButton!); - expect(onRunMaintenance).toHaveBeenCalledWith("vacuum", "users"); + expect(onRunMaintenance).toHaveBeenCalledWith("vacuum", "users", "public"); expect(reindexButton!.disabled).toBe(true); await waitFor(() => { expect(reindexButton!.disabled).toBe(false); }); fireEvent.click(reindexButton!); - expect(onRunMaintenance).toHaveBeenCalledWith("reindex", "users"); + expect(onRunMaintenance).toHaveBeenCalledWith("reindex", "users", "public"); }); test("shows non-admin placeholder for actions", () => { @@ -271,7 +271,7 @@ describe("TablesTab", () => { fireEvent.click(reindexButton!); await waitFor(() => { - expect(onRunMaintenance).toHaveBeenCalledWith("reindex", "users"); + expect(onRunMaintenance).toHaveBeenCalledWith("reindex", "users", "public"); }); }); @@ -620,7 +620,7 @@ describe("per-row controls follow the provider's own declaration", () => { // Oracle's "Rebuild Indexes" is `optimize`, and the target is the TABLE - the // shape that answered ORA-01418 for a table name before this change. - await waitFor(() => expect(onRunMaintenance).toHaveBeenCalledWith("optimize", "users")); + await waitFor(() => expect(onRunMaintenance).toHaveBeenCalledWith("optimize", "users", "public")); }); test("a provider that declares no specs keeps the pre-#U9 three controls", () => { diff --git a/tests/hooks/use-monitoring-data.test.ts b/tests/hooks/use-monitoring-data.test.ts index 9185a4a5f..2fcce8c90 100644 --- a/tests/hooks/use-monitoring-data.test.ts +++ b/tests/hooks/use-monitoring-data.test.ts @@ -310,6 +310,32 @@ describe("useMonitoringData", () => { expect(mockToastSuccess).toHaveBeenCalled(); }); + // #772: the container travels beside the target, so a provider can act on the namespace the + // row named instead of guessing one from a bare name. + test("runMaintenance sends the container it was given", async () => { + const fetchMock = mockGlobalFetch({ + "/api/db/monitoring": { ok: true, json: mockMonitoringResponse }, + "/api/db/maintenance": { ok: true, json: { success: true, message: "VACUUM completed" } }, + }); + + const { result } = renderHook(() => useMonitoringData(mockConnection)); + + await waitFor(() => { + expect(result.current.data).not.toBeNull(); + }); + + await act(async () => { + await result.current.runMaintenance("vacuum", "users", "reporting"); + }); + + const maintenanceCall = fetchMock.mock.calls.find( + (call) => typeof call[0] === "string" && call[0].includes("/api/db/maintenance"), + ); + const body = JSON.parse(maintenanceCall![1]!.body as string); + expect(body.target).toBe("users"); + expect(body.container).toBe("reporting"); + }); + // ── a refused operation is reported as refused, not as a green tick ──────── test("runMaintenance reports an HTTP 200 carrying success:false as a failure", async () => { diff --git a/tests/integration/db/clickhouse-provider.test.ts b/tests/integration/db/clickhouse-provider.test.ts index 2ae13ff31..727f68814 100644 --- a/tests/integration/db/clickhouse-provider.test.ts +++ b/tests/integration/db/clickhouse-provider.test.ts @@ -1534,6 +1534,76 @@ describe("ClickHouseProvider maintenance", () => { expect(sqlWith("OPTIMIZE")).toBe('OPTIMIZE TABLE "demo"."we""ird" FINAL'); }); + // #772: `schemaName` is the DATABASE on ClickHouse, so a container replaces the split of + // the name rather than being recovered from it - which is the ambiguity that matters for a + // name containing a dot. + test("a container is the database outright, and the name is not split", async () => { + const provider = await connectProvider(); + + await provider.runMaintenance("optimize", "audit", "default"); + + expect(sqlWith("OPTIMIZE")).toBe('OPTIMIZE TABLE "default"."audit" FINAL'); + }); + + test("a container is used whole even when it contains a dot", async () => { + const provider = await connectProvider(); + + await provider.runMaintenance("optimize", "audit", "my.db"); + + expect(sqlWith("OPTIMIZE")).toBe('OPTIMIZE TABLE "my.db"."audit" FINAL'); + }); + + test("a bare target with no container keeps the pinned-database reading", async () => { + const provider = await connectProvider(); + + await provider.runMaintenance("optimize", "users"); + + expect(sqlWith("OPTIMIZE")).toBe('OPTIMIZE TABLE "demo"."users" FINAL'); + }); + + // 3c. escapeIdentifier() dialect override (#1091 review) + // + // A backslash inside a quoted identifier is processed as an ESCAPE on this engine, so the + // inherited escaper - which doubles only the quote character - let a name ENDING in a backslash + // swallow its own closing quote and the rest of the statement was reparsed around it. The + // override escapes the backslash first, the order `literal()` in objects.ts uses. + test("escapeIdentifier doubles a backslash as well as the quote", () => { + const provider = new ClickHouseProvider(makeConnection()); + + const escape = (identifier: string) => + (provider as unknown as { escapeIdentifier(identifier: string): string }).escapeIdentifier(identifier); + + expect(escape("users")).toBe('"users"'); + expect(escape('we"ird')).toBe('"we""ird"'); + // The trailing backslash is the whole point: a run-based escaper that only doubles a + // backslash with a character after it leaves this one swallowing the closing quote. + expect(escape("x\\")).toBe('"x\\\\"'); + expect(escape('a\\"b')).toBe('"a\\\\""b"'); + }); + + test("a target ending in a backslash keeps its closing quote", async () => { + const provider = await connectProvider(); + + await provider.runMaintenance("optimize", "bs_one\\"); + + expect(sqlWith("OPTIMIZE")).toBe('OPTIMIZE TABLE "demo"."bs_one\\\\" FINAL'); + }); + + test("a container ending in a backslash cannot swallow the quote that closes it", async () => { + // The request from the review: container `x\` with a target that reads as trailing clauses. + // Emitted through the inherited escaper, the container's closing quote was consumed by the + // backslash and the target became part of the identifier rather than a second segment. + const provider = await connectProvider(); + + await provider.runMaintenance("optimize", ".t FINAL SETTINGS optimize_throw_if_noop = 1 --", "x\\"); + + const sent = sqlWith("OPTIMIZE"); + expect(sent).toBe('OPTIMIZE TABLE "x\\\\".".t FINAL SETTINGS optimize_throw_if_noop = 1 --" FINAL'); + // The naive spelling - one backslash, so the quote after it is escaped rather than closing - + // is what the override exists to prevent. + expect(sent).not.toContain('"x\\".'); + }); + test("analyze reports the part statistics ClickHouse keeps instead of computing new ones", async () => { // There is no ANALYZE: a MergeTree's statistics are its parts, and they are // always current. Reporting them is the honest equivalent of the operation. diff --git a/tests/integration/db/couchbase-provider.test.ts b/tests/integration/db/couchbase-provider.test.ts index 99c324154..01170a553 100644 --- a/tests/integration/db/couchbase-provider.test.ts +++ b/tests/integration/db/couchbase-provider.test.ts @@ -917,6 +917,81 @@ describe("CouchbaseProvider maintenance", () => { expect(bodyOf("UPDATE STATISTICS").statement).toBe("UPDATE STATISTICS FOR `travel`.`inventory`.`hotel` INDEX ALL"); }); + test("the analyze reply names the keyspace that was touched, not the bare target", async () => { + // The statement addresses three segments, so a reply naming only the target could report + // `travel` - the BUCKET - after touching only its default collection (#1091 review). + const provider = await connectProvider(); + + const result = await provider.runMaintenance("analyze", "inventory.hotel"); + + expect(result.message).toBe("Updated statistics for `travel`.`inventory`.`hotel`"); + }); + + test("analyze addresses `_default`.`_default` when the bucket row sends its bucket as the container", async () => { + // The only Tables row this provider has is the bucket-level one, whose `schemaName` AND + // `tableName` are both the bucket (`getTableStats()`). Reading the container as a scope + // built `travel`.`travel`.`travel`, which is no keyspace at all; what that row addresses + // is the default collection, the placement `resolveKeyspaceOf()` gives it (#1091 review). + const provider = await connectProvider(); + + const result = await provider.runMaintenance("analyze", BUCKET, BUCKET); + + expect(result.success).toBe(true); + expect(bodyOf("UPDATE STATISTICS").statement).toBe( + "UPDATE STATISTICS FOR `travel`.`_default`.`_default` INDEX ALL", + ); + }); + + test("any other container is used as the scope rather than parsed out of the name", async () => { + const provider = await connectProvider(); + + const result = await provider.runMaintenance("analyze", "hotel", "inventory"); + + expect(result.success).toBe(true); + expect(bodyOf("UPDATE STATISTICS").statement).toBe("UPDATE STATISTICS FOR `travel`.`inventory`.`hotel` INDEX ALL"); + }); + + test("the bucket as a container places a bare collection in the default scope", async () => { + // The flattening rule: a bare collection name IS a `_default` scope collection + // (`keyspaceDisplayName`), so a container that adds no scope keeps that reading. + const provider = await connectProvider(); + + const result = await provider.runMaintenance("analyze", "hotel", BUCKET); + + expect(result.success).toBe(true); + expect(bodyOf("UPDATE STATISTICS").statement).toBe("UPDATE STATISTICS FOR `travel`.`_default`.`hotel` INDEX ALL"); + }); + + test("a bare target with no container keeps the `_default` scope reading", async () => { + const provider = await connectProvider(); + + await provider.runMaintenance("analyze", "hotel"); + + expect(bodyOf("UPDATE STATISTICS").statement).toBe("UPDATE STATISTICS FOR `travel`.`_default`.`hotel` INDEX ALL"); + }); + + test("quotes a container and a target that carry a backtick", async () => { + const provider = await connectProvider(); + + const result = await provider.runMaintenance("analyze", "hot`el", "inv`entory"); + + expect(result.success).toBe(true); + expect(bodyOf("UPDATE STATISTICS").statement).toBe( + "UPDATE STATISTICS FOR `travel`.`inv``entory`.`hot``el` INDEX ALL", + ); + }); + + test("reindex resolves the bucket row's deferred indexes at the default collection", async () => { + const provider = await connectProvider(); + deferredIndexRows = [{ index_name: "idx_city" }]; + + const result = await provider.runMaintenance("reindex", BUCKET, BUCKET); + + expect(result.success).toBe(true); + expect(bodyOf("BUILD INDEX").statement).toBe("BUILD INDEX ON `travel`.`_default`.`_default`(`idx_city`)"); + expect(bodyOf("deferred").args).toEqual(["travel", "_default", "_default"]); + }); + test("analyze surfaces the Community Edition rejection verbatim", async () => { const provider = await connectProvider(); queryHandler = () => errorPayload(3230, "'Update Statistics' is an enterprise level feature."); diff --git a/tests/integration/db/duckdb-provider.test.ts b/tests/integration/db/duckdb-provider.test.ts index 60988a2ec..e52d46ce5 100644 --- a/tests/integration/db/duckdb-provider.test.ts +++ b/tests/integration/db/duckdb-provider.test.ts @@ -820,17 +820,17 @@ describe("runMaintenance()", () => { }); /* - The other side of that default, and the reason D49 exists. + The other side of that default. A caller that sends a BARE name for a table living outside `main` gets `main`, and the engine refuses because that table is not there. The refusal is the CORRECT behaviour for this provider - guessing which schema the caller meant would act on a table nobody named - so it is pinned here rather than repaired: the repair belongs to the caller. - Measured in the browser on 2026-08-27: the Tables panel's per-row Analyze button sends + Measured in the browser on 2026-08-27: the Tables panel's per-row Analyze button sent `table.tableName` without the `table.schemaName` it renders beside it, so clicking it on the - `analytics.events` row produced exactly this refusal. That is a shared-component defect - reaching all twelve providers that implement `runMaintenance`, filed as D49. + `analytics.events` row produced exactly this refusal. #772 repaired the caller, which now + sends the schema as the container (the test below). */ test("a bare target naming a table outside main is refused, and the message names the real one", async () => { provider = await seededMemoryProvider(); @@ -848,6 +848,28 @@ describe("runMaintenance()", () => { await expect(provider.runMaintenance("optimize", "customers")).rejects.toThrow(/takes no target/); }); + // #772: `schemaName` is the schema on DuckDB exactly as it is on PostgreSQL, and a caller + // that sends it gets that schema directly - never a re-split of the name. + test("a container qualifies the target instead of the main fallback", async () => { + provider = await seededMemoryProvider(); + + await expect(provider.runMaintenance("analyze", "events", "analytics")).resolves.toMatchObject({ success: true }); + }); + + test("a container is used whole even when it contains a dot", async () => { + provider = await seededMemoryProvider(); + await provider.query('CREATE SCHEMA "odd.schema"'); + await provider.query('CREATE TABLE "odd.schema".events (id BIGINT)'); + + await expect(provider.runMaintenance("analyze", "events", "odd.schema")).resolves.toMatchObject({ success: true }); + }); + + test("a bare target with no container still falls back to main", async () => { + provider = await seededMemoryProvider(); + + await expect(provider.runMaintenance("analyze", "customers")).resolves.toMatchObject({ success: true }); + }); + test.each(["reindex", "check", "kill"] as const)("%s is refused with the reason it is not offered", async (type) => { provider = await seededMemoryProvider(); diff --git a/tests/integration/db/libsql-provider.test.ts b/tests/integration/db/libsql-provider.test.ts index 80735ae38..034f8d523 100644 --- a/tests/integration/db/libsql-provider.test.ts +++ b/tests/integration/db/libsql-provider.test.ts @@ -618,8 +618,11 @@ describe("LibSQLProvider runMaintenance", () => { expect(await provider.runMaintenance("reindex")).toMatchObject({ success: true }); expect(await provider.runMaintenance("reindex", "probe_customers")).toMatchObject({ success: true }); + // #772: a container is ignored - the connection resolves names against its one + // attached database, exactly as sqlite.ts does. + expect(await provider.runMaintenance("reindex", "probe_customers", "main")).toMatchObject({ success: true }); - expect(sentStatements()).toEqual(["REINDEX", 'REINDEX "probe_customers"']); + expect(sentStatements()).toEqual(["REINDEX", 'REINDEX "probe_customers"', 'REINDEX "probe_customers"']); await provider.disconnect(); }); diff --git a/tests/integration/db/mongodb-provider.test.ts b/tests/integration/db/mongodb-provider.test.ts index b208cc984..b07b0312e 100644 --- a/tests/integration/db/mongodb-provider.test.ts +++ b/tests/integration/db/mongodb-provider.test.ts @@ -1204,6 +1204,40 @@ describe("MongoDBProvider", () => { expect(result.message).toContain("Compacted"); }); + // #1091 review: the refusal compared `config.database`, and a connection-string connection + // sets no `config.database` - so it refused the database the provider IS bound to and every + // per-collection button answered `bound to the database ""`. The comparison is against the + // name `getDatabaseName()` resolves and `connect()` opens. + test("a connection-string connection accepts the database it is bound to", async () => { + const provider = new MongoDBProvider({ + ...baseConfig, + host: undefined, + database: undefined, + connectionString: "mongodb://remote:27017/fromstring", + }); + await provider.connect(); + + const result = await provider.runMaintenance("analyze", "users", "fromstring"); + + expect(result.success).toBe(true); + await provider.disconnect(); + }); + + test("a container naming another database is refused, and the sentence names the bound one", async () => { + const provider = new MongoDBProvider({ + ...baseConfig, + host: undefined, + database: undefined, + connectionString: "mongodb://remote:27017/fromstring", + }); + await provider.connect(); + + await expect(provider.runMaintenance("vacuum", "users", "elsewhere")).rejects.toThrow( + 'bound to the database "fromstring"', + ); + await provider.disconnect(); + }); + test("unsupported maintenance type throws", async () => { await expect(provider.runMaintenance("flush" as never)).rejects.toThrow(); }); @@ -1798,7 +1832,7 @@ describe("object surface", () => { test("declares columns on every kind it has, and both answer a usable column shape", async () => { const kinds = objectProvider.getCapabilities().objectKinds ?? []; // BOTH, and there is no third: `describeObject` samples documents the same way for a - // collection and for a view (`mongodb.ts:1818-1821`), so no kind here abstains and the + // collection and for a view (its docblock in `mongodb.ts`), so no kind here abstains and the // expectation above has to say `noAbstainingKinds`. expect(kinds.filter((kind) => kind.hasColumns === true).map((kind) => kind.id)).toEqual(["collection", "view"]); expect(kinds.filter((kind) => kind.hasColumns !== true).map((kind) => kind.id)).toEqual([]); @@ -1839,9 +1873,9 @@ describe("object surface", () => { absentSource: { path: ["app", "no_such_view"], kind: "view" }, // Every kind this engine has declares `hasColumns`, so invariant 8's negative // direction iterates zero times and certifies nothing unless it is said out loud. - // There is no schema to read here: a collection and a view both get their fields - // SAMPLED from documents by the same code path (`mongodb.ts:1818-1821`), so there is - // no kind left that could abstain. + // There is no schema to read here: a collection and a view both get their fields SAMPLED from + // documents by the same code path (`describeObject` in `mongodb.ts`), so there is no kind + // left that could abstain. noAbstainingKinds: true, }); }); diff --git a/tests/integration/db/mssql-provider.test.ts b/tests/integration/db/mssql-provider.test.ts index 9e84579ef..9f9b0343c 100644 --- a/tests/integration/db/mssql-provider.test.ts +++ b/tests/integration/db/mssql-provider.test.ts @@ -1358,6 +1358,54 @@ describe("MSSQLProvider", () => { expect(capturedSql).toContain("UPDATE STATISTICS"); }); + // #772: `schemaName` is the schema on SQL Server, and the row holds it. A container + // qualifies the target; without one the connected default schema applies. + test("a container schema-qualifies the target rather than escaping it whole", async () => { + let capturedSql = ""; + mockQueryFn = async (sql: string) => { + capturedSql = sql; + return defaultQuery(sql); + }; + + await provider.connect(); + await provider.runMaintenance("analyze", "Orders", "reporting"); + + expect(capturedSql).toBe("UPDATE STATISTICS [reporting].[Orders]"); + }); + + test("optimize qualifies the same way, and a bare name stays unqualified", async () => { + const captured: string[] = []; + mockQueryFn = async (sql: string) => { + captured.push(sql); + return defaultQuery(sql); + }; + + await provider.connect(); + await provider.runMaintenance("optimize", "Orders", "reporting"); + await provider.runMaintenance("optimize", "Orders"); + + // `connect()` runs its own `SELECT 1` probe first, so the two maintenance statements + // are selected by prefix rather than positionally. + const rebuilds = captured.filter((sql) => sql.startsWith("ALTER INDEX ALL ON")); + expect(rebuilds).toEqual([ + "ALTER INDEX ALL ON [reporting].[Orders] REBUILD", + "ALTER INDEX ALL ON [Orders] REBUILD", + ]); + }); + + test("a container and a target that carry a bracket are both escaped", async () => { + let capturedSql = ""; + mockQueryFn = async (sql: string) => { + capturedSql = sql; + return defaultQuery(sql); + }; + + await provider.connect(); + await provider.runMaintenance("analyze", "Or]ders", "rep]orting"); + + expect(capturedSql).toBe("UPDATE STATISTICS [rep]]orting].[Or]]ders]"); + }); + test("analyze without target calls sp_updatestats", async () => { let capturedSql = ""; mockQueryFn = async (sql: string) => { diff --git a/tests/integration/db/mysql-provider.test.ts b/tests/integration/db/mysql-provider.test.ts index a7379f665..467a6ca4b 100644 --- a/tests/integration/db/mysql-provider.test.ts +++ b/tests/integration/db/mysql-provider.test.ts @@ -1292,6 +1292,69 @@ describe("MySQLProvider", () => { expect(analyzeSql).toContain("`orders`"); }); + // #772: `schemaName` on MySQL is the DATABASE, so a container is honored only when it is + // a database OTHER than the connected one - a statement already resolves a bare table + // inside the connected database, and qualifying with the same name adds nothing. + test("a container naming another database qualifies the target", async () => { + const executedStatements: string[] = []; + mockExecuteFn = (sql: string) => { + executedStatements.push(sql); + return defaultMockExecute(sql); + }; + + provider = new MySQLProvider(makeMySQLConfig()); + await provider.connect(); + await provider.runMaintenance("optimize", "users", "archive"); + + const optimizeSql = executedStatements.find((s) => s.startsWith("OPTIMIZE TABLE")); + expect(optimizeSql).toBe("OPTIMIZE TABLE `archive`.`users`"); + }); + + test("a container naming the connected database stays unqualified", async () => { + const executedStatements: string[] = []; + mockExecuteFn = (sql: string) => { + executedStatements.push(sql); + return defaultMockExecute(sql); + }; + + provider = new MySQLProvider(makeMySQLConfig()); + await provider.connect(); + await provider.runMaintenance("check", "users", "testdb"); + + const checkSql = executedStatements.find((s) => s.startsWith("CHECK TABLE")); + expect(checkSql).toBe("CHECK TABLE `users`"); + }); + + test("a bare target with no container keeps the connected-database reading", async () => { + const executedStatements: string[] = []; + mockExecuteFn = (sql: string) => { + executedStatements.push(sql); + return defaultMockExecute(sql); + }; + + provider = new MySQLProvider(makeMySQLConfig()); + await provider.connect(); + await provider.runMaintenance("analyze", "users"); + + const analyzeSql = executedStatements.find((s) => s.startsWith("ANALYZE TABLE")); + expect(analyzeSql).toBe("ANALYZE TABLE `users`"); + }); + + test("quotes a database and a table that carry a backtick", async () => { + const executedStatements: string[] = []; + mockExecuteFn = (sql: string) => { + executedStatements.push(sql); + return defaultMockExecute(sql); + }; + + provider = new MySQLProvider(makeMySQLConfig()); + await provider.connect(); + await provider.runMaintenance("optimize", "us`ers", "arch`ive"); + + const optimizeSql = executedStatements.find((s) => s.startsWith("OPTIMIZE TABLE")); + expect(optimizeSql).toBe("OPTIMIZE TABLE `arch``ive`.`us``ers`"); + }); + test("kill without target throws QueryError", async () => { provider = new MySQLProvider(makeMySQLConfig()); await provider.connect(); diff --git a/tests/integration/db/oracle-provider.test.ts b/tests/integration/db/oracle-provider.test.ts index 99203221a..0d97a2664 100644 --- a/tests/integration/db/oracle-provider.test.ts +++ b/tests/integration/db/oracle-provider.test.ts @@ -1551,6 +1551,67 @@ describe("OracleProvider", () => { expect(captured.some((sql) => sql.includes('ALTER INDEX "U9_PROBE" REBUILD'))).toBe(false); }); + // #1091 review: with an owner the index list comes from `ALL_INDEXES`, which answers for + // any schema - but a bare `ALTER INDEX "X" REBUILD` rebuilds in the CONNECTED user's + // schema, which is not the schema the list was read from. The rebuild names the owner. + test("optimize with an owner rebuilds in THAT schema rather than the connected one", async () => { + const captured: string[] = []; + let indexQueryBinds: unknown; + mockExecuteFn = async (sql: string, binds?: unknown) => { + captured.push(sql); + const upper = sql.toUpperCase(); + if (upper.includes("ALL_INDEXES") && upper.includes("TABLE_NAME =")) { + indexQueryBinds = binds; + return { + rows: [{ INDEX_NAME: "IDX_RPT_CITY" }], + metaData: [{ name: "INDEX_NAME" }], + }; + } + return defaultExecute(sql); + }; + + await provider.connect(); + const result = await provider.runMaintenance("optimize", "RPT_CUSTOMERS", "REPORTING"); + + expect(result.success).toBe(true); + // Both arguments are bound: owner first, then the table name. + expect(indexQueryBinds).toEqual(["REPORTING", "RPT_CUSTOMERS"]); + expect(captured).toContain('ALTER INDEX "REPORTING"."IDX_RPT_CITY" REBUILD'); + // The unqualified spelling is what rebuilt in the wrong schema; it must not appear. + expect(captured.some((sql) => sql.includes('ALTER INDEX "IDX_RPT_CITY" REBUILD'))).toBe(false); + }); + + test("analyze with an owner gathers statistics FOR that owner", async () => { + let capturedSql = ""; + mockExecuteFn = async (sql: string) => { + capturedSql = sql; + return defaultExecute(sql); + }; + + await provider.connect(); + const result = await provider.runMaintenance("analyze", "RPT_CUSTOMERS", "REPORTING"); + + expect(result.success).toBe(true); + // The owner is an inline-escaped literal because DBMS_STATS takes no binds; `USER` + // would mean the connected user, which is the wrong schema here. + expect(capturedSql).toContain("GATHER_TABLE_STATS('REPORTING', 'RPT_CUSTOMERS')"); + expect(capturedSql).not.toContain("GATHER_TABLE_STATS(USER"); + }); + + test("without an owner the owner position stays USER", async () => { + let capturedSql = ""; + mockExecuteFn = async (sql: string) => { + capturedSql = sql; + return defaultExecute(sql); + }; + + await provider.connect(); + await provider.runMaintenance("analyze", "USERS"); + + // The bare-name reading is unchanged: the connected user is the owner. + expect(capturedSql).toContain("GATHER_TABLE_STATS(USER, 'USERS')"); + }); + test("optimize on a table with no rebuildable index succeeds having rebuilt nothing", async () => { // A heap table with no index is an ordinary state, and so is a table whose only // index is the LOB index the catalog query filters out (the live probe's diff --git a/tests/integration/db/postgres-provider.test.ts b/tests/integration/db/postgres-provider.test.ts index c920d07e5..f85950d72 100644 --- a/tests/integration/db/postgres-provider.test.ts +++ b/tests/integration/db/postgres-provider.test.ts @@ -2276,6 +2276,48 @@ describe("PostgresProvider", () => { expect(capturedSql).not.toContain("public."); }); + // The container parameter exists because the name alone cannot say which schema it came + // from: `schemaName` is a schema for PostgreSQL, the database for MySQL, an owner for + // Oracle, and the monitoring page already renders it beside every table (#772). A name + // that carries a dot cannot stand in for it, since a container can contain one. + test("a container qualifies the target, and the name is not re-split", async () => { + provider = new PostgresProvider(makePgConfig()); + await provider.connect(); + let capturedSql = ""; + mockQueryFn = (sql: string) => { + capturedSql = sql; + return defaultMockQuery(sql); + }; + await provider.runMaintenance("vacuum", "users", "reporting"); + expect(capturedSql).toContain('"reporting"."users"'); + expect(capturedSql).not.toContain("public."); + }); + + test("a container containing a dot survives the qualifier", async () => { + provider = new PostgresProvider(makePgConfig()); + await provider.connect(); + let capturedSql = ""; + mockQueryFn = (sql: string) => { + capturedSql = sql; + return defaultMockQuery(sql); + }; + await provider.runMaintenance("analyze", "users", "my.schema"); + expect(capturedSql).toContain('"my.schema"."users"'); + }); + + test("without a container the old readings stay", async () => { + provider = new PostgresProvider(makePgConfig()); + await provider.connect(); + let capturedSql = ""; + mockQueryFn = (sql: string) => { + capturedSql = sql; + return defaultMockQuery(sql); + }; + await provider.runMaintenance("vacuum", "reporting.MonthlySummary"); + expect(capturedSql).toContain('"reporting"."MonthlySummary"'); + expect(capturedSql).not.toContain('"reporting.MonthlySummary"'); + }); + test("kill with valid PID returns success", async () => { provider = new PostgresProvider(makePgConfig()); await provider.connect(); diff --git a/tests/integration/db/sqlite-provider.test.ts b/tests/integration/db/sqlite-provider.test.ts index 43915da6b..d1a2988fb 100644 --- a/tests/integration/db/sqlite-provider.test.ts +++ b/tests/integration/db/sqlite-provider.test.ts @@ -545,6 +545,19 @@ describe("SQLiteProvider", () => { expect(tables.rows.length).toBe(1); }); + test("a container changes nothing here, because the file has no second namespace", async () => { + // #772: SQLite always resolves against the attached `main`, so the container is + // deliberately ignored rather than becoming a qualifier that cannot exist. + provider = new SQLiteProvider(makeSQLiteConfig()); + await provider.connect(); + await provider.query("CREATE TABLE mt (id INTEGER PRIMARY KEY, v TEXT)"); + + const result = await provider.runMaintenance("analyze", "mt", "main"); + + expect(result.success).toBe(true); + expect(result.message).toContain("ANALYZE"); + }); + test("reindex does not execute a statement smuggled through the target identifier", async () => { provider = new SQLiteProvider(makeSQLiteConfig()); await provider.connect(); diff --git a/tests/integration/db/trino-provider.test.ts b/tests/integration/db/trino-provider.test.ts index 310116b56..d1bd753b8 100644 --- a/tests/integration/db/trino-provider.test.ts +++ b/tests/integration/db/trino-provider.test.ts @@ -1289,6 +1289,20 @@ describe("TrinoProvider maintenance", () => { ); }); + test("a container changes nothing, because the target is a query id rather than an object", async () => { + // #772: the only operation here is `kill`, whose target addresses no namespace, so the + // container is deliberately ignored rather than turned into a qualifier. + const provider = await connectProvider(); + overrideSurface("CALL system.runtime.kill_query", (id) => ({ + body: page(id, [], [], { updateType: "CALL" }), + })); + + const result = await provider.runMaintenance("kill", "20260820_001943_00041_chvb7", "hive"); + + expect(result.success).toBe(true); + expect(sqlWith("kill_query")).toContain("query_id => '20260820_001943_00041_chvb7'"); + }); + test("says it only asked, because the target's own exchange is what observes the kill", async () => { const provider = await connectProvider(); overrideSurface("CALL system.runtime.kill_query", (id) => ({ body: page(id, [], [], { updateType: "CALL" }) })); diff --git a/tests/security/audit-redaction.test.ts b/tests/security/audit-redaction.test.ts index 6c44e406d..7919c8dbd 100644 --- a/tests/security/audit-redaction.test.ts +++ b/tests/security/audit-redaction.test.ts @@ -23,6 +23,7 @@ const ALLOWED_KEYS = new Set([ "reason", "ip", "connection", + "container", "duration_ms", "bucket", "correlation_id", @@ -97,6 +98,45 @@ describe("emitAuditEvent", () => { expect(String(line.correlation_id).length).toBe(254); }); + test("carries the container a maintenance call addressed, so two schemas cannot log alike", () => { + // #1091 review: `app.orders` and `public.orders` used to record identically, because the + // event carried only `target`. The container is what tells the two apart in the one channel + // an operator reconstructs a maintenance operation from. + const line = captureLine(() => + emitAuditEvent({ + type: "maintenance", + action: "VACUUM", + target: "orders", + container: "app", + user: "admin", + result: "success", + }), + ); + + for (const key of Object.keys(line)) { + expect({ key, allowed: ALLOWED_KEYS.has(key) }).toEqual({ key, allowed: true }); + } + expect(line.container).toBe("app"); + expect(line.route).toBe("orders"); + }); + + test("omits the container entirely for an event that does not set one", () => { + // The field is optional on the line the way `reason` and `bucket` are: a whole-database + // request, or any event that never had a container, must not grow a null. + const line = captureLine(() => + emitAuditEvent({ + type: "login_failure", + action: "login", + target: "POST /api/auth/login", + user: "admin@libredb.org", + result: "failure", + reason: "bad_credentials", + }), + ); + + expect(Object.hasOwn(line, "container")).toBe(false); + }); + test("cannot be made to forge a second log line through the actor field", () => { const line = captureLine(() => emitAuditEvent({ diff --git a/tests/security/object-edit-audit.test.ts b/tests/security/object-edit-audit.test.ts index 78b4ea03a..48b8fb0ff 100644 --- a/tests/security/object-edit-audit.test.ts +++ b/tests/security/object-edit-audit.test.ts @@ -20,11 +20,12 @@ import { * * WHAT THIS SUITE IS FOR, as distinct from `tests/api/db/objects/edit-apply.test.ts`. That suite * asks whether the route behaves; this one asks the two questions the posture page's row makes a - * claim about, and it asks them over the AUTHORITATIVE channel. `src/lib/audit.ts:480-489` states - * which channel that is: the in-process ring buffer is a convenience view for the admin UI, and the - * one JSON line per event on stdout is the record a log pipeline consumes. A leak that reaches the - * ring and not stdout, and the reverse, are two different defects, so both are asserted separately - * and both carry their own mutation in this task's work file. + * claim about, and it asks them over the AUTHORITATIVE channel. `emitAuditEvent`'s docblock in + * `src/lib/audit.ts` states which channel that is: the in-process ring buffer is a convenience view + * for the admin UI, and the one JSON line per event on stdout is the record a log pipeline + * consumes. A leak that reaches the ring and not stdout, and the reverse, are two different + * defects, so both are asserted separately and both carry their own mutation in this task's work + * file. * * RULING 1c IS WHAT IS UNDER TEST: an apply emits a new `AuditEventType` arm, `object_edit`, and * its outcome cannot be a boolean, because seven measured paths across five engines succeed while diff --git a/tests/unit/published-credentials.test.ts b/tests/unit/published-credentials.test.ts index a8a064a9c..71bc2fe82 100644 --- a/tests/unit/published-credentials.test.ts +++ b/tests/unit/published-credentials.test.ts @@ -473,8 +473,8 @@ describe("the documentation publishes no credential that works", () => { ]); expect(caught(`{ password: 'example-fake-login', email: 'admin@libredb.org' }`)).toEqual(["example-fake-login"]); - // An unquoted value is an expression, not a literal: this is what docs/API_DOCS.md:1773 - // reads now, and it publishes nothing. + // An unquoted value is an expression, not a literal: this is what the login example under + // "JavaScript/TypeScript Examples" in docs/API_DOCS.md reads now, and it publishes nothing. expect( caught(`body: JSON.stringify({ email: 'admin@libredb.org', password: process.env.ADMIN_PASSWORD }),`), ).toEqual([]); @@ -665,8 +665,9 @@ describe("the documentation publishes no credential that works", () => { const optional = "| `USER_PASSWORD` | No | Never generated - the account exists only when you set it |"; expect(caught(required + optional, "USER_PASSWORD")).toEqual([]); - // docs/API_DOCS.md:1835 is a row ABOUT `USER_EMAIL` that names `USER_PASSWORD` in passing - // and carries a default of its own. The subject of a row is the cell the name fills. + // The `USER_EMAIL` row under "Environment Variables" in docs/API_DOCS.md is a row ABOUT + // `USER_EMAIL` that names `USER_PASSWORD` in passing and carries a default of its own. The + // subject of a row is the cell the name fills. const other = "| `USER_EMAIL` | No | Login email (default `user@libredb.org`, only read when `USER_PASSWORD` is set) |"; expect(caught(required + other, "USER_PASSWORD")).toEqual([]);