Skip to content

fix(mssql,oracle): omit unknown overview size bytes - #579

Merged
cevheri merged 1 commit into
libredb:mainfrom
cnYui:fix/mssql-oracle-omit-unknown-size-bytes
Sep 6, 2026
Merged

cevheri merged 1 commit into
libredb:mainfrom
cnYui:fix/mssql-oracle-omit-unknown-size-bytes

Conversation

@cnYui

@cnYui cnYui commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Description

getOverview() in both providers initialised databaseSizeBytes to 0 beside a databaseSize of "0 bytes", ran the size statement inside a try whose catch was empty, and returned the field unconditionally — so a statement that never answered published a measured-looking zero. DatabaseOverview.databaseSizeBytes is optional precisely so that cannot happen (src/lib/db/types.ts: "absence and zero are different facts"), and both files already get this right for activeConnections, spread conditionally two lines above the defect in the same return.

What the fabrication bought on screen: StorageTab.tsx keys its whole breakdown off databaseSizeBytes !== undefined, so the 0 drew that breakdown instead of "No storage size information available." — and drew it against per-table bytes from getTableStats(), a separate read that does not share the size statement's failure. Every share is gated on totalSize > 0, so all three bars stayed empty and 0 - tables - indexes went negative, which the remainder row refuses as N/A. The tab presented, as a measurement, a breakdown whose every element either disagreed with the total or declined to answer.

@cevheri — I took your correction from the issue: the databaseSize string moves with the bytes. It is now initialised to "N/A" rather than "0 bytes", which is the pairing merged in #569 (libSQL) and #517 (the search provider), and what getHealth() in both of these same files already does.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Code refactoring
  • Performance improvement
  • Test addition or update

Related Issue

Closes #565

Changes Made

  • src/lib/db/providers/sql/mssql.ts, src/lib/db/providers/sql/oracle.tslet databaseSizeBytes: number | undefined and ...(databaseSizeBytes === undefined ? {} : { databaseSizeBytes }) in the return, mirroring the activeConnections line above it; let databaseSize = "N/A".
  • A measured 0 still reports 0. The Number(... || 0) fold inside the try is deliberately unchanged, and measuredNumber is deliberately not used here: an Oracle SUM(BYTES) over a schema that owns no segment answers NULL, and that is a measurement, not an unknown. Only the catch path leaves the field absent. Both arms are pinned by tests.
  • tests/integration/db/oracle-provider.test.ts — "degrades to defaults when every statistics query fails" asserted expect(overview.databaseSizeBytes).toBe(0), i.e. the suite pinned the fabrication; it now asserts the key is absent and the string is "N/A". New case: "a schema that owns no segment keeps its measured zero size".
  • tests/integration/db/mssql-provider.test.ts — the mirror pair, next to the existing activeConnections pair.
  • docs/providers/mssql.md, docs/providers/oracle.md — a new 7.3 When the database size is not measurable beside 7.1 and 7.2, plus the getOverview() row in the §8 table. Provider triad moves together.

Two things I deliberately did not do

  • maxConnections is untouched. Its docblock says 0 there means "no limit published", so 0 and absence are the same fact for the ceiling and different facts for the size.
  • Neither 7.3 section names a cause for the failure, and both say so explicitly. sys.database_files is database-scoped and USER_SEGMENTS describes the caller's own objects, so neither is gated by the server-level / V_$ grants that §7.1 and §7.2 measured refusals against — and I have not measured a failure of either statement on a live instance. That is the argument for the guard rather than a gap in it: a request timeout, a pool fault mid-overview and a missing view all reach the same catch in the same shape, so it must not publish a figure for any of them.

Testing

  • I have tested this locally
  • I have added/updated tests
  • All existing tests pass — with one environment caveat, below

I wrote the failing tests first. Before the fix:

$ bun test tests/integration/db/mssql-provider.test.ts tests/integration/db/oracle-provider.test.ts
      expect("databaseSizeBytes" in overview).toBe(false);
                                             ^
error: expect(received).toBe(expected)
Expected: false
Received: true
(fail) MSSQLProvider > getOverview() > a refused size read leaves overview databaseSizeBytes absent, never a measured 0
(fail) OracleProvider > getOverview() > degrades to defaults when every statistics query fails

 255 pass
 2 fail
Ran 257 tests across 2 files.

After the fix:

$ bun test tests/integration/db/mssql-provider.test.ts tests/integration/db/oracle-provider.test.ts
 257 pass
 0 fail
 663 expect() calls
Ran 257 tests across 2 files. [503.00ms]

The whole provider suite, run on this branch and on main for comparison. Same three pre-existing failures either way — all of them in duckdb-provider.test.ts, where the native addon does not open on my machine (openDuckDBClient at src/lib/db/providers/sql/duckdb/client.ts:180) — and my three new tests are the only difference:

$ bun test tests/integration/db            # this branch
 1519 pass
 3 fail
Ran 1522 tests across 17 files. [4.95s]

$ git stash && bun test tests/integration/db      # main, unmodified
 1516 pass
 3 fail
Ran 1519 tests across 17 files. [4.67s]

tests/api gives 423 pass / 6 fail on both, and the two logs are byte-identical apart from timestamps — so the failure set is provably unchanged. The consumer components are green:

$ bun test tests/components/monitoring/StorageTab.test.tsx \
           tests/components/monitoring/OverviewTab.test.tsx \
           tests/components/admin/OverviewTab.test.tsx
 73 pass
 0 fail
Ran 73 tests across 3 files. [13.76s]

Coverage of the two changed files, measured from their own suites (lcov, DA: lines):

src/lib/db/providers/sql/mssql.ts    DA lines: 642   uncovered: 0
src/lib/db/providers/sql/oracle.ts   DA lines: 745   uncovered: 0

The rest of the local gate:

$ bun run format      # Checked 984 files in 811ms. No fixes applied.
$ bun run lint        # 0 errors, 132 warnings — 371 warning lines, byte-identical to main
$ bun run typecheck   # tsc --noEmit, clean
$ bun run knip        # clean (2 pre-existing config hints)
$ bun run readme:check              # OK: 16 engines and the install commands match
$ bun run chart:check               # OK: chart 0.1.58 / appVersion 0.13.7 in sync
$ bun run channels:showcase:check   # up to date
$ bun run security:check            # OK: 18 controls documented, 19 security tests accounted for
$ bun run build                     # exit 0

The caveat, stated plainly: I could not get a clean full bun run test on this machine (Windows), and I am not going to claim one. helm, zip and bash are not on my PATH, so the Helm chart, packaging and shell-script suites error out before they assert anything — the same Executable not found in $PATH: "helm" that #569's author reported — and running tests/unit as one process reproduces the mock.module() cross-contamination CLAUDE.md warns about (tests/unit/db/factory.test.ts then hangs in pool-manager.test.ts; that file passes 53/53 in isolation). None of those files import mssql.ts or oracle.ts. What I did run is above, including the A/B against unmodified main, so CI is the authority on the full gate and the 100% coverage merge.

Test Environment

  • LibreDB Studio Version: 0.13.7 (main @ a59fb8b)
  • Browser: n/a — provider-level change, verified through the mock-based provider suites
  • OS: Windows 11
  • Node.js/Bun Version: Bun 1.3.5, Node 24.11.0
  • Database Type: MSSQL and Oracle (mock-based; no server required)

Checklist

  • My code follows the project's code style guidelines
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have updated the documentation accordingly
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published

Additional Notes

Scope is MSSQL and Oracle only, per the issue — SQLite and LibreDB are #546.

🤖 Generated with Claude Code

getOverview() in both providers initialised databaseSizeBytes to 0 beside a
databaseSize of "0 bytes", ran the size statement inside a try whose catch was
empty, and returned the field unconditionally - so a statement that never
answered published a measured-looking zero. StorageTab.tsx keys its whole
breakdown off `databaseSizeBytes !== undefined`, so that fabricated 0 drew the
breakdown over a database it never measured, against per-table bytes from
getTableStats(), a separate read that does not share the failure.

The field is now spread only when the statement answered, mirroring the
activeConnections line two lines above it, and databaseSize moves with it -
"N/A" while the bytes are unknown, as in the merged libSQL (libredb#569) and search
(libredb#517) shapes, since both monitoring tabs render that string as the headline
size. A genuine reading of 0 stays 0: the `|| 0` fold inside the try is
deliberate, because an Oracle SUM over a schema that owns no segment is a
measured zero rather than an unknown, so only the catch path leaves the field
absent.

Both provider test files pin the absence and the measured zero, and
docs/providers/{mssql,oracle}.md gain a "7.3 When the database size is not
measurable" section beside 7.1 and 7.2.

Fixes libredb#565

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@codecov

codecov Bot commented Sep 6, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@cevheri

cevheri commented Sep 6, 2026

Copy link
Copy Markdown
Member

Merging. Gates green, the triad moves together, and the "N/A" plus conditional-spread pairing matches what search/index.ts:864-871 already does, so the #517 precedent holds. The old test asserting databaseSizeBytes was 0 was pinning the defect; replacing it rather than adding beside it was the right call.

One residual, in the line you deliberately kept:

Number(sizeRes.recordset[0]?.size_bytes || 0)           // mssql.ts
Number(((sizeRes.rows || []) as ...)[0]?.TOTAL || 0)    // oracle.ts

Your comment says a SUM over no row answers NULL, and that is a measured zero. Correct - but the ?. in front folds no row at all into the same 0, which are the two facts this PR exists to separate. Narrow, since a bare aggregate returns one row, and not worth holding the PR for.

Filed as #585, and offered to you first if you want it.

@cevheri
cevheri merged commit 54312ec into libredb:main Sep 6, 2026
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

MSSQL and Oracle providers report databaseSizeBytes as 0 when the size statement fails

2 participants