fix(datagrid): declare write intent when opening a transaction - #2011
Merged
Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
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
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.
Fixes the read-only transaction failure reported in discussion #2009, plus the two things that made it impossible for the reporter to diagnose.
The reported error
A user saving to a MySQL production connection got this on every save:
They believed TablePro's Safe Mode had locked the connection to read-only and could not be changed back. Their Safe Mode was set to "Safe Mode", not "Read-Only".
Root cause
Three separate defects. Only the first produced the error.
1. We opened a bare transaction.
MySQLPluginDriver.beginTransaction()sentSTART TRANSACTIONwith no access mode. MySQL seeds each transaction's access mode from the session variabletransaction_read_only, so on a session where that is ON the transaction opens read-only and the first DML throws 1792.Confirmed against
sql/transaction.ccin mysql-server 8.0: theREAD WRITEbranch oftrans_begincallscheck_readonly(), which tests only the server--read-onlyoption and nevertransaction_read_only. SoSTART TRANSACTION READ WRITEclears a session-level read-only default with no privilege required. On a genuineread_only=ONreplica it fails at BEGIN with 1290 ("running with the --read-only option") instead of a confusing mid-transaction 1792, which is earlier and self-describing.PostgreSQL had the same latent bug: it had no
beginTransactionoverride at all and inherited PluginKit's bareBEGIN, which inheritsdefault_transaction_read_only.2. Safe Mode edits in the connection form never reached an open session.
ConnectionFormCoordinator.save()persists and sendsconnectionUpdated, whose only subscriber wasWelcomeViewModel.ConnectionSession.safeModeLevelis seeded once at connect, so the form edit was invisible to the open window until reconnect, and the toolbar badge (which reads the live session) visibly disagreed with the form.docs/features/safe-mode.mdxalready documented the intended behaviour: "There is no session-only override."3. The raw driver string was surfaced with no explanation, twice, while TablePro's own Safe Mode denial used the same words ("connection is read-only") for a different layer.
Changes
Transaction access mode (ABI-additive). New
PluginTransactionAccessModeand abeginTransaction(mode:)requirement onPluginDatabaseDriverwith a default that falls back to the existing parameterless call. The publishedbeginTransaction()is untouched, so every already-built registry plugin keeps loading and keeps its current behaviour. MySQL sendsSTART TRANSACTION READ WRITE; the sharedLibPQBackedDriverextension sendsBEGIN READ WRITE, covering PostgreSQL, CockroachDB, and Redshift. SQLite, MSSQL, ClickHouse and Oracle are deliberately unchanged: the first three have no access-mode syntax, and a read-only Oracle session already fails with a self-describing ORA-16000.All seven call sites now declare intent. The one mixed site,
QueryExecutionCoordinator+Parameters, derives it fromOperationKind.worst(of:), so an all-SELECTscript still sends a bareBEGIN. That matters: forcing read-write there would have broken replica browsing that works today.Live session reconciliation.
DatabaseManagersubscribes toconnectionUpdatedand reconciles the session's safe mode from storage through the existingsetSafeModeLevelsink. That covers the connection form, an iCloud pull fromSyncCoordinator, and bulk updates (nilpayload). Chosen over havingConnectionFormCoordinatorcallsetSafeModeLeveldirectly becausesafeModeLevelis a synced field and a change made on another Mac needs the same reconciliation. Also fixed a staleconnection.safeModeLevelread inTableStructureView+Schema.Error attribution. New
DatabaseWriteRejectionDiagnosisdetects SQLSTATE25006(shared by MySQL 1792, MariaDB 1792, and PostgreSQLread_only_sql_transaction) plus MySQL native codes 1290/1836/1874, which are all HY000. It produces a description and a recovery suggestion whose load-bearing sentence says the server enforced this, not TablePro's Safe Mode.AlertHelper.showErrorSheetgained an optionalrecoverySuggestion:(defaulted, so existing callers are unaffected). TablePro's own denials in the execution gate, structure view, and AI tool approval now name Safe Mode explicitly.ABI
scripts/check-pluginkit-abi.sh mainreports a diff with 0 removed symbols: one added protocol requirement with a default implementation, one added public enum. Additive per CLAUDE.md, socurrentPluginKitVersionstays at its current value and norelease-all-plugins.shrun is needed. Needs theabi-additivelabel.Tests
18 new tests, all passing (35 total in the touched suites, 0 failures):
MySQLTransactionStatementTests,PostgreSQLTransactionStatementTests: the SQL builders. These would have caught the original bug.TransactionAccessModePolicyTests: the regression guard that an all-read script never declares write intent, and that one write in a script promotes the whole transaction.PluginDriverAdapterTransactionTests: the adapter forwards the mode, and a plugin built before the mode existed still opens a transaction (the ABI-safety guarantee).DatabaseWriteRejectionDiagnosisTests: SQLSTATE and native-code classification, non-matches, and that the recovery text names Safe Mode.SafeModeMigrationTests: four new tests covering form-edit reconciliation, bulk reconciliation, the no-session case, and the event-driven path.swiftlint lint --strictis clean.Not included
Read-OnlySafe Mode level to avoid the vocabulary collision with server read-only. It is a real collision, but it spans macOS, iOS, the strings catalog, docs, and the connection URL scheme. Rewording the denial messages addresses the reported confusion.MainContentCoordinator.connectionreactive. Every other connection-form field (name, color, host) is equally stale in an open window until reconnect. Same class of bug, separate change.