fix: row editing wrote to the wrong row, and eight other defects measured on live engines - #969
Merged
Merged
Conversation
… one row The key an inline edit builds its WHERE on is a guess: the first field called id or ending in _id. On a result that carries a foreign key rather than the table's own key - SELECT category_id, product_name FROM products - the guess lands on category_id, and the UPDATE rewrites every product in that category. Measured on PostgreSQL 16: one cell edited, fifteen rows changed, one statement reported as accepted. Two questions now stand between an edit and a write, and both had to be asked. IS IT THE TABLE'S COLUMN. A name is not a provenance. SELECT ROW_NUMBER() OVER (ORDER BY product_name) AS product_id, product_name FROM products puts 1, 2, 3 in a field called product_id; products really has a product_id; and the UPDATEs land on whichever products those are, not on the rows on screen. Measured, two cells edited, two rows written, neither of them visible, both reported accepted. SELECT sku AS product_id is the same thing spelled shorter. selectsPlainColumn reads the select list and requires the field to be the column itself - a star, or a reference whose alias, if any, names what it already names. DOES IT ADDRESS ONE ROW PER VALUE. A grouped count over the distinct keys, bound rather than interpolated: as many distinct keys as rows on screen, as many groups back as distinct keys, every group holding one row. Each clause replaced a version measured letting a write through. Counting per row sent IN (87, 87, 87) for three rows sharing order_id 87 and wrote all three to all three. An ungrouped total passed abc and ABC on MySQL's case-insensitive collation, where WHERE k = 'abc' writes to both. String() alone collapsed SQLite's text '1' and integer 1, and two MySQL BIGINTs past 2^53 that mysql2 rounds to the same number. The check goes to /api/db/transaction when a transaction is open, because that is where the UPDATEs go: asked on a pooled connection it cannot see a row the transaction has not committed, and the apply refuses for ever with a sentence that is false about a row the user is looking at. It asks for one more row than there are keys, because the default page is 500 and groups that fell off would read as rows that are gone. A key that cannot be read as text at all - a value with a null prototype - is refused before anything is built, where it used to throw as the statement was assembled and take the apply down with no write and no message. A null key is refused too. Fewer rows than keys gets its own sentence, because it says nothing about whether the column tells them apart. A check that could not be run is not a check that passed, and the pending edits are kept in every case.
… a diff is asked for #884 moved Current Schema off the explorer's cached copy and onto a read of the connection, but that read sits in an effect keyed on [connection] alone, so it happens once and not again while the panel stays open. The sequence the Diff tab exists for - snapshot, change the database, compare - still answered "No differences found". Measured against PostgreSQL 16 with the panel left open. Two moments read the connection now, and between them they are the sequence: a snapshot reads it, and choosing a target reads it again, because that is when a person asks to be told the difference. Freshening only the snapshot was tried first and is not enough - the other side stays at the moment of the snapshot. Two snapshots compared against each other read nothing: neither side is the database. Three reads can be in flight at once, so they are sequenced through useReadGeneration, which the repository already states once for this problem. It replaces comparing the connection OBJECT, which is not safe: activeConnection is a useMemo over a prop the embedded host supplies, so a host handing over a fresh array per render produces a fresh object per render and a read would be discarded on a connection that never changed. The rest is what an awaited read in a click handler needs and did not have: a re-entrancy guard in a ref, because two Enter presses land in the same tick; setSnapshotting(false) in a finally, or a superseded read leaves the button reading Reading... for the life of the panel; the label input and Cancel disabled while the read is out; the storage write inside the try, because a localStorage quota refusal is an ordinary outcome for a whole schema; and a snapshot that was overtaken saying so rather than returning quietly, which saved nothing and said nothing while the button went back to Save. The failure banner carries the connection it was about, so a failure on a database the user has left does not sit over the one they are looking at, and a later read of that database that works clears it.
Eleven of them, all copy-and-run, and two guards so the twelfth cannot get in quietly. The Koyeb deploy button prefilled ADMIN_PASSWORD and USER_PASSWORD with set_a_real_password. That reads as a placeholder and works as a password: measured against a running container, the standard user account signed in with it. There is no placeholder that fixes it, so the button carries neither, and USER_EMAIL goes with them. Unset, the two behave differently and both answers are safe: ADMIN_PASSWORD is generated on first run and printed to the log, while USER_PASSWORD is never generated - getAuthUsers adds that account only when it is set, so without it there is no second account at all. .env.example was the worst of them, because README says to copy it and run it: ADMIN_PASSWORD, USER_PASSWORD and a 36-character JWT_SECRET, which clears the 32-character minimum and is also what saved connection passwords are sealed with when STORAGE_ENCRYPTION_KEY is unset. README's own docker run block had LibreDB.2026 twice with the same string underneath as the login to use, and the same 36-character secret, which was in docs/DISTRIBUTION.md twice and in packaging/linux/env, installed to /etc/libredb-studio/env by every .deb and .rpm. DOCKERHUB.md is the Docker Hub landing page and carried a password through both its examples. docs/SEED_CONNECTIONS.md carried MyAdmin123 and MyUser123 through both of its. README's env block, docs/MFA.md, docs/DISTRIBUTION.md and CONTRIBUTING.md carried your_secure_admin_password and admin123, which are the same thing wearing politer names. All of them are empty now rather than replaced, and each block says where the generated password is printed. The one secret that remains is under the minimum, so a deployment left as it stands stops at boot and says why. Two guards, because three of these were removed by hand this week and nothing stopped the next: published-credentials.test.ts fails on any ADMIN_PASSWORD or USER_PASSWORD assignment and any JWT_SECRET of 32 characters or more across README.md, DOCKERHUB.md, docs/ and deploy/; and the Koyeb test now parses every Koyeb URL in the file with URL rather than reading the first matching line with a regular expression over one spelling. It caught one of four ways of putting a password back before that and catches all four now. Not touched: --set secrets.adminPassword=MyAdmin123 in twenty-one places of Helm documentation. The chart's values are empty and the template requires them, so nothing is carried into a deployment - the reader types it and can see they are typing a password.
… row mysql2 hands any integer beyond 2^53 to JavaScript as a rounded number. Two rows whose ids differ in the last digit therefore arrive in the browser as the same value, and the inline editor's key guard is satisfied by that: it asks the engine about the rounded id, gets one row back, and sends the UPDATE. Measured against MySQL 8.4.11 with the real provider: the statement landed on the neighbouring row. buildPoolConfig now sets supportBigNumbers on the base config, so it covers both the form-fields path and a pasted connection string. Only values mysql2 judges too large come back as strings, and its threshold is anything above Number.MAX_SAFE_INTEGER, so 2^53 exactly is a string too even though its value was never in doubt. Everything narrower is untouched, measured one at a time: a small INT, COUNT(*), SUM over small values, a BIGINT holding a small value, an AUTO_INCREMENT id, DECIMAL, and 2^53 - 1. One other shape changed, and it was already broken: an UNSIGNED BIGINT at the top of its range read back as 18446744073709552000, which is not the stored value. It is now the digits the row holds. bigNumberStrings was deliberately not set: it would turn every BIGINT into a string, including the small ones.
The same hole as MySQL, and worse on the driver the Docker image runs. Measured with the real provider on both: bun:sqlite silently rounded 9007199254740993 to ...992 and the inline editor then updated the neighbouring row, while node:sqlite threw ERR_OUT_OF_RANGE and refused to read the row at all. Two spellings of the same defect, one of them silent. Turning on the drivers' big-integer flag alone is not the fix. Measured: every integer becomes a BigInt, including 1 and COUNT(*), rows are sent to the browser as JSON, JSON.stringify refuses BigInt, and the connection stops opening. 180 passing became 149 passing with 31 failures, 16 of them the database failing to open. So the flag is on for both drivers, spelled differently for each, and the conversion happens at the provider boundary: a value that fits comes back as a number, a value that does not comes back as its decimal string, and no BigInt leaves the provider. The boundary is Number.MAX_SAFE_INTEGER, which is what the libsql driver already uses. Small integers are unchanged, including COUNT(*), rowid, length(), CAST and PRAGMA columns, measured one at a time. Reading correctly then opened the other half. A column with no declared type, or declared BLOB, has no affinity, and SQLite never compares a string equal to an integer, so sending that id back matched nothing and the user was told no rows changed. toSQLiteBindValue makes the bind side the exact inverse of the read side: only a digit string the read side could itself have produced goes back as a 64-bit integer. A leading zero, a leading plus, whitespace, a trailing .0, exponent form, the empty string, anything inside the safe range and anything wider than 64 bits all stay text, so a genuinely textual key still behaves as text.
The read side was already right here: decodeInteger returns a value outside the safe range as a string, and the transport's own type admits it. The send side was not. encodeValue passed that string through as text, and a column with no affinity never compares text equal to an integer, so the row could be read and then not updated. Measured against sqld 0.24.33 in a container, with the real provider: reading 9007199254740993 and sending it back changed one row in an INTEGER column and one in a TEXT column, and zero rows in a BLOB column and in a column with no declaration at all. The user was told nothing had changed. encodeValue now mirrors decodeInteger through isDecodedInteger: only a digit string the read side could itself have produced is sent as an integer. Measured on the live server, a leading zero, a leading plus, whitespace, a trailing .0, exponent form, the empty string, a value inside the safe range and one wider than 64 bits all stay text, and so does a genuine text key that happens to be digits.
…e guard .env.example carried STORAGE_ENCRYPTION_KEY as a commented line reading your_32_character_random_string_here. It looks like a placeholder and it is thirty-six characters, which clears the thirty-two-character minimum, so it works. A copy left untouched is not affected, because the line is commented out. What the file invites is uncommenting it, which is what a commented example is for, and the reader who does that seals every saved connection password with a key published in this repository. It now reads too-short-generate-your-own, twenty-seven characters. With server-side storage on - STORAGE_PROVIDER set to sqlite or postgres - uncommenting it as it stands stops the server at startup and says the key is too short. The file ships STORAGE_PROVIDER=local, where the key is never read, so on the settings it ships with nothing changes either way. The comment above the line states that condition and gives the command to generate a real key. The guard that was added with the credentials removal only looked at README.md, DOCKERHUB.md, docs/ and deploy/. Measured against nine ways of putting a credential back, it missed most of them. It now also reads render.yaml, the root Dockerfile and .github/workflows, accepts .txt as well, catches the camelCase adminPassword and userPassword spellings, catches STORAGE_ENCRYPTION_KEY, catches a value followed by another flag or a comment on the same line, and catches a key and value split across two lines. All nine attempts are caught and ten innocent shapes stay silent.
…y edited
Every refusal the inline editor can show was read against the situation that
fires it. Four of them were saying something that was not true of what was in
front of the user, and all four were measured live on PostgreSQL 16.15 and MySQL
8.4.11 through the real provider.
A key holding binary data got "some of the rows you edited are no longer in the
table. Run the query again." The rows were still there, and running the query
again produced the same unusable key, so the advice looped forever. A bytea, a
BINARY(16) and the {type:"Buffer"} shape a driver puts on the wire are now
refused before the engine is asked at all, with a sentence about the value
itself.
A date-time key was fixed only on the path the browser does not use. Over
/api/db/query a Date becomes an ISO string, the check saw a string and let it
through, and the old false sentence came back with count(*) sitting at 2. The
check now reads the column type the result already carries rather than the look
of the value, so a column that genuinely holds the text of a date still works,
proven by editing one end to end.
A fractional key was refused as a value that "does not reach the table as the row
holds it". Measured: 0.30000000000000004 in a double precision column reached the
table exactly, and WHERE found the row. It is now written, and what is still
refused is refused with a sentence that is true of it.
A large-magnitude float was refused as an out-of-range integer. 1e21 sits exactly
in a 64-bit float and both engines matched the row. The out-of-range refusal is
right for an integer column, where past 2^53 the browser holds a different value
than the row does, so the decision is now made on the declared column type rather
than the size of the number.
One refusal was also half-blind: when the engine answered for fewer keys than
were sent it said only that the key does not tell the rows apart, and dropped the
more important half. It now says both, that a row may have gone since the read
and that the engine may fold two keys into one, without asserting either.
Eight defects in the comparison panel, each measured before it was changed and each pinned by a test that fails without the fix. A snapshot whose read failed used to clear its own warning on the next render, so the user saw the panel go quiet and had no way to know nothing had been saved. The warning now stays until it is dismissed, and there is a Dismiss button to do it with. Leaving the Diff tab unmounts the panel, and a read still in flight went on to write its snapshot afterwards. The cleanup supersedes both counters now, and four state writes that ran after an await are guarded, so nothing the panel started touches storage or its own state once it is gone. Pressing Enter twice was guarded but the guard was measured by nothing: removing it left every test green. Without it the database is read twice, one snapshot is saved, and a "Press Save again" banner appears that the user did nothing to earn. Both halves are now tested. Choosing the target that was already chosen re-rendered nothing and so re-read nothing, and there was no refresh control anywhere, which left leaving the tab and coming back as the only way to see current data. There is a Refresh button, its read goes through the same generation counter, and two quick clicks start one read rather than two. Snapshot ids came from Date.now(), so two taken in the same millisecond shared an id: React warned about duplicate keys and deleting one deleted both. Worse, the store keeps fifty, and when a selected snapshot fell off the end the lookup returned nothing and an empty array was handed to the diff, which then reported that every table in the database had been removed. Ids come from the repository's own generator now, and a snapshot that cannot be found says so instead of being drawn as an empty database. The remote fetch was sequenced for its writes but not for the busy indicator: a superseded read could take "Fetching..." down while a newer one was still outstanding. And choosing a stored snapshot did not supersede a remote read at all, so a fetch the user had abandoned could land afterwards and take the target back, with the indicator left hanging. Both orderings now end with the panel showing the thing the user chose last.
…ched A failed remote fetch said nothing at all. "Fetching..." disappeared, the target stayed where it was, and the error went to the log. The comparison on screen was still the old one, and nothing told the reader it was not the database they had just asked for. The failure now appears in its own strip, built the same way as the snapshot failure strip and dismissed the same way. It keeps its own state rather than sharing the snapshot one, because that strip is tied to the connection on screen while this is about a different database, and pressing Save should not clear it. Only the fetch the user is waiting on can raise it: a superseded fetch that fails late stays silent, which is checked by the same generation counter that already guards the writes. The guard on the mount-time read was measured before this change and caught by nothing.
An earlier commit on this branch removed SQL Server from the list of engines agent mode reads. That was wrong and it was unrelated to what the commit was for. mssql.ts implements queryReadOnly, and engine-support.ts lists four engines, not three, which is what the engine-support test reads from the real providers. It was also applied unevenly, so the documents contradicted each other: README, DOCKERHUB and FEATURES said three engines while AGENT_GUIDE and the Chinese, Japanese and Hindi READMEs still said four. FEATURES went further and listed three engines in one sentence and then called them "those two engines". The English text is restored and the sentence that counted them is right again. The four-layer read-only profile SQL Server needs, which has no read-only transaction of its own, is described where the other three engines' profiles are. README also carried four source references with line numbers that do not hold those symbols - postgres.ts:915, sqlite.ts:537, duckdb/index.ts:525 and runtime.ts:199. The line numbers are gone rather than corrected, because they go stale on the next edit and the file name alone is enough to find the symbol. The Spanish and Urdu READMEs never carried this claim, so nothing changed there.
The coverage gate was red at 59092 of 59094 lines. The two uncovered lines were the scalar-bigint and typed-array branches of the driver's record normalizer, both added by the 64-bit integer commit on this branch, and the repository requires every line. The seam they sit on is typed unknown deliberately: the statement interface declares all() as unknown[] and get() as unknown, so what arrives is whatever the injected driver returns. Measured against both shipped drivers, for all(), get(), run(), a miss and a PRAGMA read, every answer is a row object, a null miss or run()'s info object, and a BLOB is a cell inside a row rather than the record itself. So neither branch is on a path those two take today. They are what stops a driver that answers with a bare cell - a values mode, or the future row-returning method the module's own comment warns about - from handing a BigInt out of the provider or walking a typed array cell by cell. They are driven through the injectable constructor the module already exposes for exactly this, so no driver is mocked and no source changed. Each test states what it kills: removing the scalar branch sends a BigInt through JSON.stringify, which refuses it outright, and removing the typed-array branch writes converted cells back into a typed array that will not hold them.
…e four holes in the guard A reviewer put working passwords into this repository four ways and the guard that is supposed to stop that stayed green each time. Every one is now caught, with the nearest innocent shape written beside it so the next person it stops does not delete it. A Markdown table row was the worst of them, because a variable table is how these files list their settings: the name is one cell and the value is the next, and nothing between them is an assignment. Only a value cell written as a code span with no space in it counts, which is what tells the value column from the description column beside it. The other three were one rule being too strict. `NAME=value` had to be followed by a comment, a pipe, another flag or the end of the line, so `docker run -e ADMIN_PASSWORD=Secret123 imagename` and `export ADMIN_PASSWORD=Secret123 && echo` both read as prose. A sentence does not write an equals sign, so an `=` no longer asks what follows it; `NAME: value` still does, which is what keeps `ADMIN_PASSWORD: generated on first run` a sentence. And a quoted value was read to the first space, so a password with a space in it looked like one word followed by prose - the same hole hid a secret at full length, and a secret is judged by its length. One value had to be excused: deploy/azure/src/install.sh writes `printf 'ADMIN_PASSWORD=%s\n'` and pours the password in from a variable. A run made entirely of format specifiers is the hole, not the value. docs/API_DOCS.md was still handing readers admin123 in two login examples, a cURL block and a fetch block, which is the same string removed from CONTRIBUTING.md by hand earlier. They now carry a placeholder and a line saying the real password is generated on first run and printed to the log. The guard reads login bodies now, found by the `email` key beside the password. Connection bodies are deliberately left alone: the password in one of those is the reader's own database, sampled as postgres or password123, and nothing here is reachable with it.
… its integer column
Every other provider sends the declared column types alongside the rows. The local
SQLite provider never did, and until this branch nothing noticed: the exporter
falls back to the JavaScript type of the value, and a SQLite integer used to
arrive as a number.
The 64-bit fix on this branch changed that. An id past 2^53 now arrives as a
string, so a table carrying one exported as `"id" TEXT` with the value in quotes,
and a user moving that table to another engine got a text column where they had an
integer one. Measured: exported, restored, and `typeof(id)` came back `text`.
libsql was unaffected the whole time, because it reports its declarations.
This follows the libsql path exactly, down to omitting the key when the map is
empty rather than sending `{}`. The two drivers spell the declarations
differently, so one read-only accessor is bridged in sqlite-driver.ts the way
`safeIntegers` and `inTransaction` already are, and nothing new reaches a shared
surface.
It is read after the rows, not before, because bun's `declaredTypes` raises until
the statement has run. bun also publishes `columnTypes`, which is not this: it is
the runtime class of the value, so a REAL column reads FLOAT and an undeclared one
reads INTEGER, and it raises on `PRAGMA journal_mode`. Measured on Bun 1.4.0 and
not used.
An absent declaration stays absent. An expression, a literal, an aggregate, a
function call, a PRAGMA column and a column declared with no type all get no
entry, because two decisions on this branch now read this map and a guess in it
would be worse than nothing.
Not fixed here: a SQLite REAL key is still refused by the row editor, which asks
whether the column is a 64-bit float and does not count `real` because on
PostgreSQL and Trino it is 32 bits. On SQLite it is 64. That answer belongs in the
editor, with the engine in hand.
…there The editor asks whether a fractional key's column is a 64-bit float, because a value read at a different width than the row holds can miss the row or write to the wrong one. `real` and `float` were left out of the safe list, and that was right for the engines it was written against. SQLite has no 32-bit float. REAL is the only float width it has, and FLOAT, DOUBLE and DOUBLE PRECISION are accepted spellings of the same storage class. Measured live: all four report the same decltype, `typeof()` answers `real` for each, 0.30000000000000004 reads back exactly, and `WHERE k_id = 1.5` matched one row while the editor was refusing that same edit. The sentence it gave - that nothing said the column was a 64-bit float - was false, because the declaration said so. This only became visible now that the SQLite provider reports its declarations. So the decision reads the engine as well as the name, from the connection the caller already holds. Nothing new is threaded through the module. Each engine was measured rather than assumed. PostgreSQL `real` and `float4` are four bytes by `pg_column_size` and `0.1::real::float8` is 0.10000000149011612, so they stay refused. MySQL FLOAT compared false against 0.1 in the same row where DOUBLE compared true, so FLOAT stays refused. DuckDB hands 32-bit values over as 0.10000000149011612, so REAL and FLOAT stay refused. libsql is SQLite and behaves as SQLite, checked against a running server. Trino REAL and SQL Server `float` could not be reached from here, so both keep the refusal: an unmeasured engine gets the closed side, which is what this rule chose in the first place.
… body as it is written
An adversarial reviewer broke this guard three ways and proved each by reproduction.
It read only the cell immediately after the name, and only a backticked span. The
real tables here are shaped Variable, Required, Description, so the value lives in
the third cell, written inside the description as "(default: LibreDB.2026)". Seven
shapes of that got past, including the plain and bolded spellings.
Reading more cells is what made the second finding possible: docs/HELM_CHART.md
has a Source column whose cells are chart value paths, so a row there reported
secrets.adminPassword as a published password, and
secrets.jwtSecret.fromExistingSecret is thirty-seven characters, which would have
failed the secret-length test outright. A guard that fires on innocent text is the
one that gets deleted by the next person it stops.
So position is not what decides a cell any more: the table's own header does. A
column headed Value, Default or Example holds values; a "default: x" written in
words is an assignment wherever it appears; a two-column table's single marked
cell is a value; with no header at all the cell after the name is read, as before.
A value that spells the variable's own name back - secrets.adminPassword,
admin-password - is a reference, not a value.
The third was the JSON rule requiring double quotes around the email key. The
file it was written for does not use them: docs/API_DOCS.md wrote
`{ email: 'admin@libredb.org', password: 'admin123' }`, a bare key and single
quotes, and that one escaped while its sibling was caught. The test that claimed
to cover "the two shapes in the file" had hand-converted this one to double
quotes - a line that never existed there. It now uses the real shape.
Measured rather than argued: a credential line injected into README.md,
DOCKERHUB.md, docs/API_DOCS.md, deploy/railway, deploy/koyeb and docs/HELM_CHART
is caught in all six, and six innocent rows added to the real HELM_CHART table
stay silent in all six. Eight tests became ten and none of the eight was weakened.
… now does docs/providers/README.md says the code, the document and the provider's integration test move together in the same pull request. This branch changed three providers and left their documents behind, and one of them now said something false. mysql.md said a BIGINT written as 9007199254740993 comes back as 9007199254740992. That was true until supportBigNumbers was set. Measured again on MySQL 8.4.11 with mysql2 3.24.4, both protocols: without the flag both rows read back 9007199254740992 and the unsigned ceiling read 18446744073709552000; with it they are the digits the rows hold. INT, COUNT(*), AUTO_INCREMENT and 2^53 - 1 are still numbers, and 2^53 exactly is a string, because mysql2's threshold sits above the safe range. sqlite.md had nothing about either change, and one of them is a new capability. The document now carries the 64-bit integer work in both directions - bun:sqlite rounded silently while node:sqlite raised, the boundary is the safe-integer limit, and a text bind matched no row in a BLOB or untyped column where an integer bind matches one - and the declared column types this provider never used to report, including why they are read after the rows rather than before and why bun's columnTypes is not the same thing as its declaredTypes. libsql.md described only the reading half of section 3.4. The send side is measured against a live sqld 0.24.33 and written beside it: the round trip changes one row in all four column types now, where a text bind used to change none in a BLOB or untyped column. Every claim was measured for this document rather than copied from the commit that made the change. BACKLOG's count of the drivers that fill columnTypes is updated with it, since SQLite is now one of them.
The guard that stops a working password being published has to contain password-shaped strings, because that is its subject. It carried six that read like real ones, and a secret scanner on the pull request reported all six. They are renamed to values that say what they are - example-not-a-real-password, example-fake-login and the rest - and each was measured afterwards to confirm the guard still reads it as a value rather than as a stand-in. That distinction is the whole risk in this change: the guard deliberately ignores a leading dollar sign, braces, angle brackets and a `your-` prefix, so a fixture that drifted into one of those shapes would leave the test green while measuring nothing. Turning one fixture into a stand-in was tried and drove a test red, which is what the renaming had to preserve. The two length-dependent assertions are untouched and still exact at forty and thirty-two characters, because a secret is judged by its length. Two sentences elsewhere spelled a removed password out while explaining that a value reading as a placeholder still works as one - one in the Koyeb deploy notes and one in its test. The sentences are gone; the paragraph around them and the assertion they described are unchanged. Ten tests before, ten after, ninety-nine assertions in both.
The renamed fixture is a different width, so the line it sits on no longer fits the formatter's wrap. Content unchanged.
The renamed connection-body fixtures are a different width, so the lines they sit on no longer fit the formatter's wrap. Content unchanged.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…on Windows The guard walked the documentation with join(), which spells a path with backslashes on Windows. Everything downstream compares against forward slashes: the readable-file pattern anchors on `(^|/)`, and the `.github/workflows/` prefix that exempts the values CI passes to the build. With backslashes that prefix never matched, so CI's own placeholders were reported as published credentials, and the file list assertion failed on a path it had spelled itself. Measured on windows-latest: three of this file's tests failed there and nowhere else. Paths are built with a forward slash now, which Node reads on every platform, and join() is no longer imported.
Member
|
Reviewed at What I verified rather than took on trust. I mutated the new predicates one at a time and re-ran their tests: 1.
|
cevheri
added
enhancement
… the text it was given Review finding 1. SQLite has no date type. A column declared DATE, DATETIME or TIMESTAMP holds the text or the integer it was handed, and both drivers give that value back verbatim, so it is its own identity and matches when it is sent back. The instant refusal fired on it anyway and said the value does not reach the table as the row holds it, which is false on that engine. It is a regression this branch introduced rather than an old gap: before the declared-types work SQLite reported no column types at all, the column read as undeclared, and the apply went through. The fix is the shape already used one set below for `real`, which is the same situation of one type word meaning two things across engines. Measured live through the hook on bun:sqlite and on a libsql server, for DATE, DATETIME and TIMESTAMP and for four value shapes: a date, a space-separated timestamp, an ISO timestamp and an integer epoch. All are stored verbatim, all match on the way back, and the UPDATE changes exactly one row. A Date OBJECT is still refused on these engines, because there the round trip is not faithful and no driver here produces one for SQLite anyway. The refusal is unchanged where it is right, measured rather than assumed: PostgreSQL 16.15 `timestamp` and MySQL 8.4.11 `DATETIME(6)` both matched zero rows for the ISO text the browser holds while the rows sat in the table.
…ction's id Review findings 2 and 3, which had to be read together because closing one by the obvious route opens the other. Refresh was not locked while a snapshot was reading, although the label, Save and Cancel all are, and it calls into the same generation counter. So typing a label, pressing Save and pressing Refresh before the read landed destroyed the snapshot and left a banner the user had not earned. The same thing happened without any button: a parent that rebuilt the connection object with the same id restarted the read, which the embedded shell does through a useMemo over a host-supplied prop. The second finding pulled the other way. Matching a read by `connection.id` lets a connection edited in place - pointed at another host or database, same id - match the PREVIOUS read, so the old database's objects were presented as Current Schema until the new read landed. Both were one question asked twice. The read-starting effect and the read-matching guard both want to know whether a read goes to the same DATABASE, and that is now a content key derived from the request body `readLiveSchema` actually sends, so the two cannot drift apart. The id keeps the question it does answer - which entry in the user's snapshot list a failure belongs to, where an in-place edit should keep the banner and choosing another connection should not - and the comment says which question each one answers. Three tests changed their trigger rather than their claim, because the trigger was the defect: two now refresh, one edits a connection in place.
…e probe saw Review findings 4 and 5. The credential sweep shortened JWT_SECRET to nine characters inside the block headed "Production (strict mode, explicit secrets)", which also turns bootstrap off. Run as written, the container exits 1 at startup saying the secret is too short, and nothing in the block said that was deliberate. It now substitutes `openssl rand -base64 32` on the docker run line, which is neither a published secret nor a broken command, and matches the fix line the preflight banner itself prints. Measured both ways: exit 1 with the nine-character value, HTTP 200 with the substitution. The same substitution is wrong in the other two places the sweep touched, and measuring said so. `packaging/linux/env` is read by systemd as an environment file, not by a shell: the substitution arrives as its own 26 characters of text, which is under the minimum and would stop startup. The snap drop-in rejects it outright as an invalid environment block, because systemd splits the line on the space. Both now ship the variable commented out with the generate command beside it, which is what the file's own TOTP entry already does. The affinity comment in the libsql transport attributed its probe to SQLite 3.47.0. Running the pinned tag answers 3.45.1, matching what the provider document recorded for the same probe; 3.47.0 is what `:latest` and Turso Cloud answer, which is what the other comment in that file is about and is correct there.
…ranch did not touch Asked for in review. The value of the entry is the part that was measured rather than assumed, and measuring changed the claim. Three of the five have no rounding to report: DuckDB 1.5.5, ClickHouse 26.8.6.5 and Cassandra 5.0.9 all hand a past-2^53 integer back as its full decimal string. SQL Server 12.7 returns `bigint` as a string too and only rounds `numeric(38,0)`. Oracle 26ai through oracledb 6.10 is the one that rounds a `NUMBER`, and it returned the neighbouring row's key. So the display defect is Oracle's and, for decimals, SQL Server's - not all five. `docs/providers/mssql.md` section 5.3 says `BIGINT` rounds, which its own 5.4 contradicts, and D18 repeats it; correcting both is in the new entry's Done when. The write path was checked rather than asserted: a rounded whole-number key is refused at use-inline-editing.ts:391 before the engine is asked, measured on live Oracle and SQL Server, so no wrong row is written through it. That check found one case it does not cover, filed separately: a FRACTIONAL Oracle `NUMBER(20,4)` key is not refused, two rows collapse onto the same number, the guard is answered "one row", and the UPDATE lands on the neighbour and is reported as a success. Same class as the defect this branch exists to remove, on a provider it does not touch. The duplicated bind-side invariant is recorded too, with the shared-hook precedent this repository already sets, rather than being extracted here.
…t way Review finding 6. The bound, the nineteen-digit shape and the predicate over them existed in both the SQLite driver and the libsql transport, and the second one's own docblock said why that is a hazard: the two providers hand out the same shape, so they must accept the same shape back, and nothing held them to each other. They had already drifted in the half that is prose. The libsql docblock lists exponent form and `'9e15'` among the shapes the rule cannot emit; the SQLite one listed neither. The code was still identical, checked entry by entry. It is stated once now in `sqlite-int64.ts`, placed beside the other small SQL-family module that exists for the same reason, and both providers read it. Behaviour is unchanged, measured through each provider's own surface rather than the shared module's: seven values that convert and twenty-one that stay text, on libsql against a running server and on both SQLite drivers in process, plus a full read-then-update round trip on a column with no affinity. The part that keeps it done is a test that reads the provider directory from disk and parses it, so a driver added next year is scanned whether or not anyone adds it to a list, and the bound, the digit shape and the safe-range bigint may be written in one file only. A correctly written copy passes every behaviour test, which is why the check has to look at the source. Demonstrated by adding a copy to the libsql transport: the test named all four spellings and went red. The backlog entry that recorded this duplication is removed, since this closes it.
… address the neighbour Found while recording what 64-bit reads do on the drivers this branch does not touch, and reproduced on live Oracle 26ai: two rows whose keys differ in the last decimal place, `NUMBER(20,4)` holding ...6.7891 and ...6.8, both arrive as 1234567890123456.8. The editor asked the engine about that value, was answered one row, sent the UPDATE to the NEIGHBOUR, and reported Changes Applied. Same class as the defect this branch exists to remove, on a provider it does not otherwise change. Oracle `NUMBER` carries up to 38 decimal digits and a JavaScript double carries about 16, so a value out of a column that holds more digits than a double can may be a rounding of the row's own. The refusal now says that rather than asserting the value is unreachable, because on the same engine a `NUMBER(10)` primary key and a `BINARY_DOUBLE` both round-trip exactly and are still written. The cost is stated where the rule is: a fractional Oracle key that happens to fit, like 123456.7891, used to be written and is now refused, because nothing in the value tells it apart from one that was rounded. The closed side is the safe side, which is what every other rule in this file chose. Measured on the other engines with high-precision decimals rather than assumed: PostgreSQL `numeric`, MySQL `DECIMAL` on both protocols and DuckDB `DECIMAL` all hand the value over as a string, so there is no rounding to guard and nothing changed for them. SQLite and libsql are exempt because there a declaration is an affinity, not a type, and the value is already the double itself. The backlog entry is reduced rather than removed. What is left is a key that comes out of a decimal column looking like a whole number - `NUMBER(38,20)` holding 5.00000000000000000001 and 5 both arrive as 5 - which the scale would separate, and the shared column-types module drops the scale deliberately.
Member
Author
|
All six are in, at 1. 2 and 3. Both were one question asked twice. The read-starting effect and the read-matching guard both want to know whether a read goes to the same database, so that is now a content key derived from the request body 4. The pinned tag answers 3.45.1; the comment now says so. 5. 6. Done, in Two things from the backlog entry you asked for. Your five-engine expectation is wrong on three of them, measured: DuckDB 1.5.5, ClickHouse 26.8.6.5 and Cassandra 5.0.9 all return a past-2^53 integer as its full decimal string, and SQL Server returns And checking the write path found it is not a display defect everywhere. On Oracle a fractional key is rounded on the way out, so two rows differing in the last decimal place both arrive as one value, the guard is answered one row, and the UPDATE lands on the neighbour and reports Changes Applied. Reproduced live and fixed here, since it is the same defect this branch exists to remove. The cost is stated at the rule: a fractional Oracle key that happens to fit is now refused too, because nothing in the value tells it apart. PostgreSQL What is left of that entry is a key that comes out of a decimal column looking whole - On your WhatsApp question: there is no issue for this to close. All 44 open ones read; the eight adjacent to what this touches are about other things. #884 was closed on 18 Sep by 954/955, and this completes the half its own commit message flagged as unfinished, so the body says One gap worth knowing: the credentials guard's roots do not include the repository's |
…one float width settle its own Two false refusals found in review, both of the kind this file exists to remove: work the engine performs correctly, stopped. A ClickHouse Decimal was refused because the type word was read and the parameters thrown away. ClickHouse is the engine that NAMES its precision, which is the fact Oracle withholds and the reason the refusal was written. Measured on ClickHouse 26.8.6.5, two rows one unit apart in the last place, read back as JSON.parse leaves them: at Decimal(15, 6) they stay two doubles and WHERE matched one row, its own; at Decimal(16, 6) they become one double and it matched the NEIGHBOUR; at 18 and 38 the same. So fifteen digits is the last width a double carries exactly, a declaration naming fifteen or fewer cannot round, and one naming more - or naming none, which is Oracle - stays refused. A SQLite key from a column declared DATE was refused as a number that might be the printed form of a 32-bit float. SQLite has no 32-bit float; the file says so two sets above, for `real`. Measured on bun:sqlite with one table per declaration: DATE holding a julian day, REAL, NUMERIC, INTEGER, BLOB and a column declared nothing all hand the value over as the double the file holds, and WHERE matched one row in every one. So on those engines the declaration does not narrow anything and the length rule has nothing to catch. One test asserted the opposite of that measurement - that TEXT or NUMERIC says nothing about the width - and is rewritten to what the engine does, with the measurement beside it. TEXT is kept as a case for the opposite reason: it hands the value over as the STRING "1.5", so a fractional number never reaches this rule from such a column, and the old case drove a shape the engine does not produce. A column the result declares NOTHING about stays refused: that is not a column declared nothing, it is an expression, and no engine can be asked the width of one.
…tabase The fix that stopped a rebuilt connection object from destroying an in-flight snapshot named five fields as not addressing a database, typed into the component. Five was not the list. Measured: press Save and let `queryTimeout` change while the read is in flight - no host, no database, no principal - and the snapshot is superseded, nothing written, "the schema was read again before this finished. Press Save again" on screen. The same defect the maintainer filed, through a field the list never named. This repository already answers the question three times, and the fourth answer was the weakest. `CONNECTION_RELEVANCE` in `use-connection-payload.ts` is the one that holds: it is typed `Record<keyof DatabaseConnection, FieldRelevance>`, so a field added to the connection fails the BUILD until someone classifies it. The key is derived from it now, and the hand-written list is gone. The component's own comment argued against a positive list, on the ground that a field such a list forgot would silently match a read to a database it was not taken from. True of a list someone types, and not true of one the compiler completes - which is the difference worth stating, and it is stated where the function lives. The nested containers are read the same way: an absent SSL block is not an empty one, and the bastion fingerprint stays cosmetic, because pinning a host we were already going to use repoints nothing.
…he bound written as a power Twenty-one references of the form (#N) were added across this branch and none of them is an issue about this work. They are the numbers of a working list kept while it was being written, and in this repository that spelling means an issue: #42 is comprehensive error handling, #44 a Helm chart, #45 Helm chart hardening, #4 the MongoDB provider. Checked one by one against GitHub, and against main, which carries none of them. The genuine citations this branch uses - #273, #881, #884 - are on main already and are left alone. The anti-drift guard is strengthened where it was weakest. It read the bounds as digits, so a copy spelling them `2n ** 63n - 1n` and `2n ** 53n - 1n`, with the pattern built from a string, passed 109 of 109 - reproduced by planting exactly that in a new provider directory. It now reads a bigint power of two as well, for those two exponents only, because any other power of two in a provider is ordinary arithmetic and a guard that reported those is a guard the next contributor deletes. The same planted copy is now reported by line and by name. The docblock said the guard could not see that spelling, which was honest and is no longer true; it now says what the guard does see, what it still cannot - a bound assembled some third way - and why the source guard is what covers a driver that does not exist yet.
…ule the rule moved to `mssql.md` contradicted itself in three places and this branch had measured which half was wrong without correcting it. Section 5.3 said BIGINT, DECIMAL, NUMERIC and MONEY all arrive as JavaScript numbers and lose precision; 5.4 said BIGINT and DECIMAL arrive as strings and cited 5.3 for it; the closing summary repeated 5.3. Measured on SQL Server 2022 (16.0.4295.3) through tedious 20.0.0, one row: BIGINT 9007199254740993 comes back as the string "9007199254740993", every digit intact. DECIMAL(38,10) 1234567890123456.7891234567 comes back as the number 1234567890123456.8, and MONEY 922337203685477.5807 as 922337203685477.6. So the precision warning belongs to the three that round and not to BIGINT, and 5.4's own evidence already said so - its probe exported BIGINT as NVARCHAR(MAX) and DECIMAL(10,2) as FLOAT, which is a string and a number read the right way round. The SQLite and libsql documents still described the 64-bit bind rule as living in the SQLite driver, and the module it moved to was named in no document at all. Both now point at it where the rule is explained, and the SQLite file inventory lists it beside the driver adapter.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Every defect here was reproduced on a running engine before it was changed, and each fix is held by a test that fails without it.
The row editor wrote to the wrong row
Three drivers handed integers past 2^53 to JavaScript rounded, so two rows whose ids differ in the last digit arrived in the browser as the same value. The inline editor's key guard asked the engine about the rounded id, got one row back, and sent the UPDATE to the neighbour. Measured on MySQL 8.4.11 and on both SQLite drivers;
node:sqliteraisedERR_OUT_OF_RANGEinstead, which is the same defect wearing a louder spelling.MySQL now sets
supportBigNumbers. SQLite and libsql convert at the provider boundary: a value that fits comes back a number, one that does not comes back its decimal string, and no BigInt leaves a provider. The bind side is the exact inverse, so only a digit string the read side could itself have produced goes back as a 64-bit integer, and a leading zero, a leading plus, a trailing.0, exponent form and anything wider than 64 bits all stay text.Six refusal messages were untrue of what fired them
The editor refuses an edit it cannot address safely, and six of its sentences said something false about the value in front of the user. A binary key was told its rows were gone from the table while they were still there, with advice that looped forever. A date-time key was fixed only on the path the browser does not use. A fractional key was refused as unreachable when the engine matched it exactly. A large float was refused as an out-of-range integer. A grouped-count answer reported half of what it knew.
The decisions now read the declared column type carried with the result, and the engine as well as the type name, because
realis 32 bits on PostgreSQL and 64 on SQLite. Each engine was measured rather than assumed; the ones that could not be reached keep the refusal.The schema diff panel
Eight defects: a snapshot failure that cleared itself silently, a read that wrote after the panel was unmounted, a double-Enter guard nothing measured, no way to re-read without leaving the tab,
Date.now()snapshot ids that collided so deleting one deleted two, a missing snapshot drawn as an empty database so the panel reported every table removed, a busy indicator a superseded read could clear, and a remote fetch that could take the target back after the user had chosen something else. A failed remote fetch now says so instead of going quiet.Published credentials
Eleven working credentials were removed from the documentation, and the example env file's encryption key is now below the minimum so uncommenting it stops the server rather than sealing saved passwords with a published key. A guard fails the build on the next one: measured against a credential injected into six real files, and silent on six innocent rows of the real chart table.
Provider documents
docs/providers/{mysql,sqlite,libsql}.mdare updated in the same PR, asdocs/providers/README.mdrequires, with the measurements taken for those documents rather than copied.Checks
typecheck,format,lint,knip,build,build:lib,attw,chart:check,readme:check,security:check,channels:showcase:check,distribution:checkall pass. 569 test files, 18511 passing, and line coverage is 100 percent at 59114 of 59114.Three independent reviewers read earlier states of this branch and rejected it three times, for a red coverage gate, a documentation regression, two commit messages asserting measurements that did not hold, and four shapes of credential the guard let through. Those are the reasons the branch looks the way it does.
Refs #884