fix(mysql,postgres): omit unmeasured database size instead of publishing 0 bytes - #627
Conversation
…ing 0 bytes Route both providers' overview size reads through measuredNullableAggregate() so a missing result row or a row without the size column publishes neither databaseSizeBytes nor a formatted size: databaseSize stays "N/A", exactly as mssql.ts and oracle.ts already do (libredb#585/libredb#601). A returned SQL NULL (an empty database) remains a measured zero and is still published as 0 / "0 B". Tests, one per provider suite in the libredb#601 shape: row without the column, no result row, and a non-finite value each assert databaseSizeBytes is absent and databaseSize is "N/A"; the anti-vacuity twin pins a returned NULL at 0. Closes libredb#621
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
cevheri
left a comment
There was a problem hiding this comment.
Thank you for this. Two things before the technical part, because they matter more than the diff.
First, about the assignment. #621 was claimed at 13:04 UTC and your commit is dated 13:14 UTC. A change of this shape, two providers plus both provider docs plus eight tests, does not get written in ten minutes, so your work was clearly already in flight when the claim landed. GitHub does not notify you about a new comment on an issue you already have open. Nothing here is your fault, and nothing about it counts against this PR. I have written to the claimant separately.
Second, this is your first pull request on GitHub, and it is a good one. You found mssql.ts on your own and followed it line for line, you kept the triad in lockstep the way CLAUDE.md asks, and you wrote the anti-vacuity twin without being told twice. That last part is the one most contributors skip.
What I measured
I do not review these by reading the diff. Every row below is a mutation run against your head commit.
| Probe | Result |
|---|---|
| baseline | mysql 133 pass, postgres 190 pass, matching your PR body exactly |
the issue's Done-when: guard becomes if (!row) return undefined |
1 fail in each suite |
treat a returned SQL NULL as unmeasured |
a database that measures zero bytes fails in each suite |
revert the MySQL call site to parseInt(... || "0") |
3 fail |
| revert the PostgreSQL call site | 3 fail |
The Done-when is satisfied in both suites and the twin is live in both. Worth noting that your no result row test on PostgreSQL passes before and after the fix, because the old code threw on rows[0].database_size and the catch was accidentally correct. The issue predicted that. It is still the right test to have: it pins the behaviour so the next refactor cannot lose it.
Two changes, then this goes in
1. The required check never ran past step nine. Lint, Typecheck and Build and Engine Smoke - Build Payload are both red on one root cause:
tests/integration/db/postgres-provider.test.ts(2359,38): error TS2554: Expected 1 arguments, but got 2.
defaultMockQuery at line 115 takes one argument. The four new tests pass (sql, params). Drop the params parameter and the second argument in those four blocks and it resolves. bun test cannot see this because Bun strips types instead of checking them, which is exactly why CLAUDE.md puts the whole gate set before "done":
bun run format && bun run lint && bun run typecheck && bun run knip && bun run test && bun run build
Biome also wants the three-condition if in the MySQL tests wrapped; bun run format:fix writes it. One thing to take from this beyond the fix: the formatter is the ninth of seventeen steps in that job, and the job halts there, so lint, typecheck, knip, build, build:lib and attw were never measured at all. A red check hides how much is still unknown.
I ran the fix locally to be sure nothing else is hiding behind it. Fourteen lines, and format, lint, typecheck and knip all pass with the suites at 133 and 190.
2. The PostgreSQL prose states something the engine does not do. Both the test comment and the new paragraph in docs/providers/postgres.md say that pg_database_size($1) answers NULL when the aggregate has nothing to measure, and that a returned NULL means an empty database measuring zero. I measured both engines:
postgres:18 CREATE DATABASE probe; SELECT pg_database_size('probe'), pg_size_pretty(...), ... IS NULL;
-> 7774735 | 7593 kB | f
mysql:latest SELECT SUM(DATA_LENGTH+INDEX_LENGTH), ... IS NULL FROM information_schema.tables WHERE table_schema='probe';
-> NULL | 1
Your MySQL paragraph is correct: SUM() over an empty schema really does answer one NULL row. PostgreSQL is different on two counts. pg_database_size() is a function, not an aggregate, and it never returns NULL for a database that exists; and an empty PostgreSQL database is roughly 7.4 MB of catalog, never zero. So please reword that paragraph and the test comment to say what is true there: the returned-NULL case guards the shared helper's contract, which is worth pinning, and the three unmeasured states are the ones the engine can actually produce. Keep the test. Only the explanation needs to change.
While you are in that file, OVERVIEW_SIZE_SQL still selects pg_size_pretty(pg_database_size($1)) as database_size and nothing reads it any more, since the size string now comes from formatBytes(). mssql.ts selects only size_bytes. Dropping that column finishes the move to the model you followed.
None of this changes your approach. It is a formatter run, a signature, and a paragraph. Push to the same branch and I will remeasure.
|
FYI @dvd233 |
Two contributors reached #621 ten minutes apart: it was claimed at 13:04 UTC and #627 arrived at 13:16 with a commit dated 13:14. The author of the pull request could not have seen the claim, because GitHub sends no notification for a comment on an issue you already have open. The claim bullet read as an exclusive hold and said nothing about that case, so it left the outcome looking like a judgement of one of the two people. It is not one. State the rule the collision was actually decided by: the clock rather than the claim, delivered work reviewed on its merits, nobody asked to write the same change twice, and the nearest open issue plus review credit for whoever does not land it. No test. The paragraph carries no cross-file invariant to guard, and pinning its prose verbatim would fail on the next honest edit while proving nothing. The two-week staleness window above it is unchanged and still open to review.
Closes #621
What changed
getOverview()on the MySQL and PostgreSQL providers published a confident0/"0 bytes"whenever the size statement answered empty (no result row, or a row without the size column). The Storage tab keys its whole breakdown offdatabaseSizeBytes !== undefined, so that fabricated zero rendered a wrong "0 B" measurement instead of "no storage size information available".Both providers now route their size reads through
measuredNullableAggregate(row, column)— the helper #585/#601 introduced for MSSQL and Oracle:mysql.ts):parseInt(sizeRows[0]?.size_bytes || "0")→measuredNullableAggregate(sizeRows[0], "size_bytes");databaseSizeinitialised to"N/A"and only formatted when the figure exists;databaseSizeBytesis omitted via conditional spread.postgres.ts): dropped the|| "0 bytes"/parseInt(... || "0")pair for the same helper + conditional spread.A returned SQL
NULL— an empty database — is a measured zero and is still published as0/"0 B"; only the unmeasured states go absent.Tests (shape of #601, one per provider suite)
"databaseSizeBytes" in overviewisfalse,databaseSizeis"N/A"NULL→ still publishes measured0/"0 B"(anti-vacuity twin)Both suites green locally:
mysql-provider.test.ts133 pass / 0 fail,postgres-provider.test.ts190 pass / 0 fail, 100% line coverage on the touched providers andmeasured-aggregate.ts.Docs
Added the short size paragraph to
docs/providers/mysql.mdanddocs/providers/postgres.mdstating which states are measured and which are absent (mirrors the mssql/oracle wording per the triad rule).