fix(plugins): stop generating SQL DDL for engines that have none - #2896
Merged
Merged
Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Automations to automatically generate PRs for you. |
|
好耶QWQ |
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 #2884.
The bug
Deleting an Elasticsearch index from the sidebar generated
DROP TABLE "test_index"and failed withInvalid response: Enter a request like: GET /my-index/_search, which is the plugin's own console parser rejecting the text before anything reached the cluster.Root cause
PluginDatabaseDrivergivesdropObjectStatementandtruncateTableStatementsa default ofnil, documented "return nil to use app-level fallback".nilsays two different things. On MySQL it means the generic DDL is right for me, which is why eleven SQL plugins never implement either hook. On Elasticsearch it means I have no such statement at all.PluginDriverAdaptercould not tell those apart and resolved the ambiguity by inventing SQL (PluginDriverAdapter.swift:806and:797), so a REST engine was handed SQL DDL. The Redis plugin already returned""fromdropObjectStatementwith the comment "Return empty string to prevent adapter from synthesizing SQL DROP": a plugin author had hit this and worked around it with a sentinel.This was never one engine. Truncate is the same fallback on the neighbouring menu item, and
TableOperationEligibility.canTruncateonly inspected the object's kind, never the engine, while the sidebar appended Delete unconditionally.The fix
The adapter stops guessing, and the menu stops offering what the driver would refuse. That second half matters: returning
nilwithout gating the menu would turn a visible error into a silent no-op.App
SQLDDLFallbackPolicystates per engine whether the app may write DDL at all. Neither of the engine's other language facts can answer it: DynamoDB writes PartiQL and so declares an editor language of.sqlwhile returningnilfrom both hooks on purpose, and the SQL dialect descriptor defaults tonilon several engines that do haveDROP TABLE.PluginDriverAdapter.dropObjectStatement/truncateTableStatementsreturn optionals and fabricate only where the policy allows.tableOperationEligibility(for:isReadOnly:)resolves the answer per object name, so a plugin that refuses one object and not another is respected.TableOperationSQLBuilder.generatethrowsobjectOperationUnsupportedfor a staged ref the engine has no statement for, instead of coercing it to"". The menus keep one off the queue, but a driver answers from live session state, so a queued Redis truncate can stop being expressible between staging and Save. Dropping it silently would run the rest of the plan while the coordinator recorded every staged object as done.TableOperationEligibilitygained the engine dimension, all-or-nothing over a selection for the reason Truncate already gave.SidebarViewModel.batchToggleDelete, and the Database-menu Truncate and Edit-menu Delete validators, which were reaching the same staging calls ungated.Hiding rather than disabling follows what
DatabaseTreeMenuSpecalready does for Rename, Truncate and container Drop.Plugins
dropObjectStatementreturnsDELETE /<index>; truncate staysnil""sentinel removed;FLUSHDBrefused unless the row is the session's own databasetruncateTableStatementsreturnsdeleteMany({})dropObjectStatementreturnsDELETE /v1/schema/<Class>Elasticsearch returns plain console text rather than the tagged base64 form used for row writes, because all three channels read that one string: the confirmation dialog shows it verbatim,
QueryClassifierreads the leading verb to tier it destructive, andexecuteConsolesends it. A tagged blob would have shown the user base64 to approve and classified a drop as an ordinary write. That last point is why Typesense changed too: its tagged drop was classifying as.write, not.destructive, so it skipped the destructive-consent gate.Elasticsearch has no truncate.
_delete_by_queryis asynchronous, reports conflicts per document and keeps the mapping, so it is a bulk delete rather than a truncate, and Truncate is now hidden there instead of offering something that means something else.No PluginKit change and no ABI bump: neither method is on the
DatabaseDriverprotocol, so making them optional is app-internal.Tests
SQLDDLFallbackPolicyTestsscansDatabaseType.swiftfor declared constants so a new engine cannot inherit an answer by accident, and scans the plugin sources so an engine declaring a non-SQL editor language can never be left fabricating. The app holds the policy and the plugin holds the statement, so nothing else forces those to agree.ElasticsearchOperationsTestsround-trips the generated statement back throughElasticsearchConsoleParser, so the dialog can never again show text the driver rejects, and covers the names refused as multi-target.TypesenseOperationsConsoleTextTestsandWeaviateOperationsTestsround-trip those statements too, and assert a Typesense drop and truncate now classify as destructive.TableOperationSQLBuilderTestscovers the rejection: one inexpressible ref rejects the whole batch rather than quietly running the rest.TableOperationEligibilityEngineTestsand new cases inPluginDriverAdapterTableOpsTests,DatabaseTreeMenuSpecTestsandSidebarContextMenuLogicTests.The existing adapter tests all build on
.postgresqland pass unchanged, which is the check that no SQL engine regressed.No UI automation: the flow needs a live Elasticsearch cluster and a registry plugin that never loads under XCTest, so it does not run deterministically.
Verified
Build PASS. 213 cases across 14 suites PASS. The Weaviate package suite passes. Each of the six changed plugin schemes builds.
swiftlint --strictis clean over all 33 changed files, passed by explicit path because.swiftlint.ymlscopes toTableProandPackages. Both docs scripts pass.AllPluginsreports one error, in the vendoredoracle-nioSPM checkout (@TaskLocalmacro expansion). It is pre-existing, no Oracle file is in this change, and there are zero errors underPlugins/.Delivery
The app change ships with the next release. Elasticsearch, etcd, Redis, MongoDB, Weaviate and Typesense are registry plugins and need re-release; Elasticsearch also through
scripts/release-plugin-for-shipped-app.shso 0.74.0 users get the fix without waiting for an app release.Not in this change
SELECT * FROM "<index>"the same way. I wrote the fix, found that the browse query withsize: 0would export an empty file, and reverted it: it needs realsearch_afterstreaming and a live cluster to verify.TableProMobile/Views/TableListView.swift:149,175hardcodesDROP TABLE/TRUNCATE TABLEin the view for every engine, with no driver hook at all. On iOS a Redis key long-press still offers Drop Table and sendsDROP TABLE "session:42". The fix there is a different shape and would need the policy moved to a shared package.Review
Reviewed by Codex (
review --scope working-tree), which raised seven findings. Six were real and are fixed here: the silent omission at save time, the Redis flush race that the rejection above closes, aSidebarViewModelTestsregression from resolving capability through a hard-wiredDatabaseManagerlookup, a queued operation that could no longer be unstaged once it stopped being expressible, an Edit > Delete item left enabled as a no-op, and a Swift 6 actor-isolation error on thePluginManagerassertion. The seventh, that the coverage test read the mutable plugin registry, was fixed differently: it now scansDatabaseType.swiftfor declared constants, which is deterministic and is the real list of engines.