fix(export): follow the source engine's dialect when exporting query results as SQL - #2632
Merged
Conversation
…results as SQL Claude-Session: https://claude.ai/code/session_011EqgjCjCAU6tiiVmnMpF86
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Automations to automatically generate PRs for you. |
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 #2630.
What was wrong
Three defects, all one theme: a dialect-dependent decision taken without the dialect that was already in hand.
SQLExportPluginderivesSqlDialect.from(databaseTypeId:)in six places and writes the correct-- Database Type: MySQLheader from the same field, which is why the header was right while the body was wrong.1. The query-results export had no driver.
ExportDialog.swift:1103was the only one of three export modes that built itsExportServicewithout one; its initializer was even commented "no driver needed".QueryResultExportDataSourcethen fell back toSQLEscapingfor bothquoteIdentifierandescapeStringLiteral.2.
DROP ... CASCADEwas emitted unconditionally, for every engine and both export paths, and for views, types and sequences as well as tables.3. Both query paths asked for structure and drop they cannot deliver.
ExportService.swift:252/:320built the export table with[structure, drop, data]all true whilefetchTableDDLreturns"", so the dump carriedDROP TABLE <the source table>with noCREATE TABLEafter it. The issue's own pasted output shows exactly that.Measured, not assumed
Against a live MariaDB 12.3 and sqlite3:
DROP TABLE IF EXISTS "fields" CASCADE;ERROR 1064 ... near '"fields" CASCADE'near "CASCADE": syntax errorDROP TABLE IF EXISTS `fields` CASCADE;DROP TABLE IF EXISTS "fields";INSERT INTO "fields" ("id") VALUES (1);The report blamed two things and one is innocent. MySQL's
DROP TABLEgrammar does list[RESTRICT | CASCADE], and the manual says outright: "The RESTRICT and CASCADE keywords do nothing. They are permitted to make porting easier." The double quotes are the whole syntax error on MySQL.CASCADEis still a real bug elsewhere: SQLite rejects it, and Drop is on by default, so the ordinary File > Export SQL dump to SQLite has been un-importable too.A second defect on the same line that nobody reported, and it is worse. The same nil driver dropped to ANSI string escaping.
C:\temp\next(hex433a5c74656d705c6e657874, 12 bytes) exported today re-imports into MariaDB as433A09656D700A657874, 10 bytes:\tbecame a tab and\na newline. Silent corruption, discovered only when someone restores.Before and after, same server
The fix
Patch for 1 and 2, refactor for 3.
DatabaseManager.driver(for:), the installed handle, no lease and no liveness requirement (CLAUDE.md: the driver "is never nil'd"). The common path is now the engine's own exact functions.resolveSQLDialect+quoteIdentifierFromDialect, which already handles SQL Server's[/].escapeStringLiteralFromDialectwas extracted fromSQLRowToStatementConverter's private copy rather than written a second time, and it escapes the same nine characters the MySQL driver does, so a dump taken with a driver and one taken without are the same file.CASCADEfollowsDriverPlugin.supportsCascadeDrop, the capability that already drives the sidebar's Cascade checkbox. PostgreSQL, Redshift, CockroachDB, PGlite, Snowflake and Dameng keep the clause; every other engine loses one that was either a no-op or a syntax error. The default isfalse, the answer that is never a syntax error.[structure, drop, data], MQL declares[drop, indexes, data], so clearing index 0 and 1 would have turned off MQL's indexes and left its drop on.One collateral fix rides along because the fix depends on it: Snowflake declared
requiresBackslashEscapingInLiteralson its driver and nothing on either descriptor, so the two paths escaped the same value differently. Dameng cannot be fixed the same way, because it detects its escaping at connect time; that is documented in the helper rather than papered over.Review
Codex is out of credits until Sep 7 (
You've hit your usage limit), so the second-model review wasSkill(code-review), not Codex. It found 15 issues; 11 are fixed here.The most valuable one caught a fix of mine that did not work. I had declared Snowflake's backslash escaping on the curated snapshot, but
buildMetadataSnapshottakeseditor.sqlDialectfromdriverType.sqlDialectand replaces the curated one outright once the plugin loads. Snowflake is registry-only, so the plugin is always installed for real use: the fix was inert, and my test passed only because plugins never load under XCTest. It is now declared onSnowflakePlugin.swifttoo, and the test says which one it covers.Also fixed from the review: the two query data sources carried a dead
supportsCascadeDropthat was the only reason their inits needed@MainActor; a dead storeddriver; a doc comment orphaned onto the wrong function; a tautological test that asserted the value it had just passed in; a mismatched-length defaults array read positionally; and a settings race where my copied export harness and the existing one both did capture-reset-restore over the sameUserDefaultskey while Swift Testing runs suites in parallel. Both suites now share one serialized harness.Verification
swiftlint --strictoverTablePro,Plugins,TableProTests: 0 violations.PluginExportDataSourcehas no conformer in any plugin bundle. No version bump, no plugin re-release.docs: house style and source claims agree.SQLExportandMQLExportboth build.AllPluginsfails locally on the knownoracle-nio@TaskLocalmacro issue (both errors from that package); CI covers it.No UI automation: nothing in this change is reachable from a deterministic UI flow, since the defect is in the text of a written file rather than in a control.
Behaviour change worth knowing
A query-results SQL export is now
INSERT-only. That is correct, and the option tree is hidden in that mode so nobody chose otherwise, but it is visible.For an ad-hoc query the
INSERTnames the export's file name, which for a query tab defaults toquery_resultsand so names no real table. That was equally true before, alongside aDROPof the same name, so this is strictly an improvement rather than a regression, but it is not yet right; renaming the file renames the table. Reported rather than fixed, because a result set genuinely has no table behind it and picking one is a product decision.https://claude.ai/code/session_011EqgjCjCAU6tiiVmnMpF86