fix(editor): stop completing another schema's tables without their schema - #3070
Merged
Merged
Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Automations to automatically generate PRs for you. |
Signed-off-by: Ngô Quốc Đạt <datlechin@gmail.com>
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.
The defect
On PostgreSQL with a tab on
public, typingSELECT * FROM attendance.completesattendance's tables, which is right. From then on, a plainSELECT * FROM timesofferstimesheetand inserts it bare. The server answersrelation "timesheet" does not exist, becauseattendanceis not on thesearch_path.The same happens without any dot completion: expand
attendancein the sidebar and every one of its tables is offered bare in FROM, JOIN, INTO, DROP, CREATE INDEX, CREATE VIEW and at statement start.Root cause
SQLSchemaProvider.tablesis every table the scope's owner loaded. For the browse scope that is the union of every schema the sidebar has expanded (SchemaRefreshService.syncAutocompleteProviderfills it fromSchemaService.allLoadedTables). Two readers treated it as "names this statement can write without a schema":tableCompletionItems(), behind every bare table slot.resolveAlias, which matchedtimesheet.to a table only another schema holds and then fetched its columns from the tab's own schema.On top of that,
tableCompletionItems(inSchema:)folded the tables it fetched on demand intotablesthroughmergeTables. That made it a second, unordered writer of a listSchemaRefreshServiceandSchemaProviderRegistryeach own alone, and the widening lasted until the next reset.The fix
tablesResolvableUnqualifiedderives the bare-name set fromtablesandgetDefaultSchema(), the provider's existing answer to "the schema a table can be named in without its schema" (the engine's implicit schema, otherwise the schema the scope's driver is on). A table with no schema, and every table on an engine that reports no current schema (MySQL, SQLite, MongoDB and the rest), stays in it.tableCompletionItems()andresolveAliasread it.resetForDatabase.mergeTablesis gone, sotableshas one writer again. Concurrent completions of one schema await the fetch in flight, the wayloadTaskandfieldPathscoalesce, and a fetch that a database switch overtook answers its own caller without writing into the new scope. An empty schema is cached as an answer; a failed fetch is not.getTables(), the AI context and the Copilot preamble keep the full union. They already render every table outside the default schema qualified throughSchemaQualifiedName. No API that Open Quickly calls onmainchanged.This is a refactor of the provider's table model rather than a patch on one method: the list keeps one meaning, and the two readers that needed a narrower one get it from a single derived property, so a third writer that widens the list cannot bring the bug back.
Behaviour chosen: exclude, not qualify
Bare table completion now offers only tables the server resolves without a schema. A table anywhere else is reached by typing its schema and a dot, which lists that schema's tables whether or not the sidebar has opened it.
Why not offer
attendance.timesheetin the bare list instead:schema.as the way into another one.Tests
SQLSchemaProviderUnqualifiedScopeTests, 10 cases:timesheetofferedattendance.leaves bare completion andgetTables()aloneSELECT * FROM timesafterSELECT * FROM attendance., throughSQLCompletionProvidertimesheetofferedresolveAlias("timesheet")with no reference naming itBefore the fix (provider restored to
main, tests unchanged): 10 executed, 8 failed, 2 passed, every failure on the expectation it names (labels → ["users", "timesheet"],script.calls → 2,fresh → ["timesheet"]).After the fix, the new suite plus the 20 suites that own the provider, its registry and the completion layer: 428 executed, 411 passed.
SQLSchemaProviderUnqualifiedScopeTests10/10,SQLSchemaProviderTests30/30,SQLSchemaProviderFallbackTests18/18,SQLCompletionProviderTests127/127,SchemaProviderRegistryTests19/19,SchemaRefreshServiceTests11/11,SchemaContextForAITests6/6,CopilotPreambleBuilderTests1/1, and the rest of the completion suites all green. The 17 failures are allQuickSwitcherViewModelTestsfilter cases that sleep a fixed 200ms for a 40ms debounce; none touches the provider, and the identical 17 failed on another branch without this change while the machine's load average was above 400. The build is the one the test step compiled.swiftlint lint --strict: 0 violations.verify.sh docs: pass.Review
Codex could not run either pass: the workspace hit its usage limit until Sep 29. The
code-reviewfallback ended on an API session limit before reporting anything. A separate reviewer on another model read the diff and traced the reentrancy fence inonDemandTables(inSchema:), the task's isolation, howgetDefaultSchema()is populated on the browse and tab scopes, and the remaining readers oftables, and found no defect.Known limitation, unchanged by this PR
On SQL Server,
switchSchemamoves the plugin'scurrentSchemaas a browse cursor, while bare names in user SQL resolve through the login's default schema (SCHEMA_NAME(), read at connect). With the tab on a non-default schema, bare completion now offers that schema's tables, as it did before, and no longer offers an expandeddbo's. The AI context, the Copilot preamble and the fallback column labels already readgetDefaultSchema()the same way. Separating "browsed schema" from "schema bare names resolve in" needs a driver-level value and a PluginKit change.Docs:
docs/features/autocomplete.mdx, the Schema names section, now says where a bare name completes from and how to reach another schema.Found while investigating #3048 (see #3060).