fix(sidebar): stop a period in a quoted name merging two objects' identities - #3068
Merged
Merged
Conversation
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.
Defect
TableInfo.idjoined a table's schema and name with a bare period, so two different tables could get one id:a, tableb.ca.b.c_TABLEa.b, tableca.b.c_TABLEa.ba.b_TABLEa, tableba.b_TABLEA period is legal inside a quoted identifier on PostgreSQL, SQL Server, Oracle, SQLite and the rest, so both pairs are real tables a server will hold at once.
RoutineInfo.id,UserDefinedTypeInfo.idandTriggerInfo.idhad the same shape, andRoutineInfoandUserDefinedTypeInfodefine==andhashthrough their id, so two such routines or types were not just keyed alike, they compared equal.Every consumer that keys, deduplicates or diffs by these ids dropped one object of each pair:
SchemaService.allLoadedTablesdeduplicates by id, and it feeds Open Quickly,SchemaRefreshServiceandCatalogEditAdoption: with schemasaanda.bboth loaded, one of the two tables vanished from Open Quickly.DatabaseTreeFilterdeduplicates tables, routines, triggers and types by id, so the sidebar filter showed one row where there were two.SQLSchemaProvider.mergeTablesdeduplicates autocomplete's table list by id, so completing aftera.could loseb.conce"a.b".cwas cached.DatabaseTreeMetadataService.partitionCountsChangedkeys counts by id, so it compared one table's partition count against the other's and reported a change that never happened.RecentTableRow,DatabaseTreeTableRefand the routine, trigger and type row refs build outline item ids from these, so the Recent section and the tree could merge two rows.RowImportSheet(ForEach(availableTables, id: \.id)) andTableTransferSheet's selection binding key rows by the table id.The tree's routine, trigger and type rows had a second copy of the defect one level up:
DatabaseTreeRoutineRef,DatabaseTreeTriggerRefandDatabaseTreeUserTypeRefjoined database, schema and the object id with a bare|, whileDatabaseTreeTableRefandPartitionInfobeside them already escaped it, each with its own private copy of the escaping.Root cause
No shared way to build an identity out of names a user chose. Each type joined its parts by hand, some escaped and most did not.
Fix
IdentityPath(TablePro/Models/Database/IdentityPath.swift) joins components with a separator and escapes a backslash or that separator inside a component. Every id above now goes through it, and the two private escape helpers inDatabaseTreeTableRefandPartitionInfoare gone.A component that holds neither character is emitted unchanged, so every id for a name without a period or backslash is byte-identical to before. That matters because Open Quickly persists these ids in its frecency store. Only objects whose names hold a period or backslash get a new id, and those were the ones colliding.
The display spellings (
qualifiedName) are untouched.What stays as it was, deliberately:
TriggerInfo.idstill omits a missing schema or table, so a trigger with a schema and no table and one with a table and no schema can still render alike. Keeping positions would change the id of every trigger on an engine without schemas, which the frecency store holds, and one listing stamps one schema on every trigger, so the two shapes do not meet in one list.RoutineInfo.idstill joins its discriminator with_, which a name can contain. Escaping_would change the id of every routine with an underscore in its name. An engine reports a discriminator for all of its routines or for none, so the two shapes do not meet in one list either.Not in this PR
Open Quickly's own row id for a table in the current connection is
QuickSwitcherItem.tableItemId, which still joins schema and name with a bare period onmain. #3060 escapes it the same way, so this PR leaves those lines alone rather than conflict with it. With this PR alone, both tables now reach Open Quickly; with #3060 as well, each row is selectable on its own.tableItemIdcan move ontoIdentityPathonce both are in.The compare feature has the same shape in its own ids (
TableDiffResult,DataComparePlan,SchemaSyncOperation,CompareObjectIdentity) and also keys its snapshot maps by the dotted qualified name, and it persists plan ids in sync profiles. Fixing the ids alone there would split one key into two spellings, so it needs its own change. The Backup sheet'sObjectRow.iddoubles as the row's visible label, so it needs identity and display separated first.Tests
New tests:
IdentityPathTests(new suite, 5 tests, one parameterized over.and|), plus cases inTableInfoTests,RoutineInfoTests,TriggerInfoTests,UserDefinedTypeInfoTests,SchemaServiceTests,DatabaseTreeFilterTests,DatabaseTreeNodeTestsandPartitionCountRefreshTests. Each collision case asserts two objects keep two ids, andplainNamesKeepTheirIdplus the existing literal-id tests pin the unchanged bytes. The old formulas give one id to every pair these tests use (a.b.c_TABLEtwice,a.b_TABLEtwice,FUNCTION_a.b.ctwice,type_a.b.ctwice,a.b.c.audittwice,a|b||FUNCTION_ftwice), so each collision case fails onmain. That comes from the formulas rather than a run: two attempts to run these suites with the source change reverted each waited outverify.sh's 30 minutes behind other builds on this machine and ran nothing.verify.sh testover those nine suites andSidebarPartitionRowTests, PASS, 127 cases executed, 0 failed, every new case present in the log.verify.sh lintover the 15 changed Swift files as file paths, 0 violations.review: one finding, the Open Quickly row id above, left to feat(sidebar): find tables in every schema from Open Quickly and the sidebar filter #3060. Codexadversarial-review: no material findings.Found while investigating #3048 (see #3060).