Skip to content

fix(datagrid): carry every per-table setting through a rename and a connection delete - #2746

Merged
datlechin merged 1 commit into
mainfrom
fix/per-table-settings-lifecycle
Sep 11, 2026
Merged

fix(datagrid): carry every per-table setting through a rename and a connection delete#2746
datlechin merged 1 commit into
mainfrom
fix/per-table-settings-lifecycle

Conversation

@datlechin

@datlechin datlechin commented Sep 11, 2026

Copy link
Copy Markdown
Member

Summary

Five stores keep settings keyed by connection, database, schema and table. Each one had to be wired by hand into three lifecycle events: a table rename, a database or schema rename, and a connection delete. Two were wired into none of them.

Store Table rename Container rename Connection delete
FilterSettingsStorage yes, but the browse search stayed behind yes yes
FileColumnLayoutPersister yes yes no: file and synced records left behind
HighlightRuleStorage yes yes yes, but the .unreadable.json copy stayed
ValueDisplayFormatStorage (Display As) no no no
ForeignKeyLabelColumnStore no no no

So a Display As format or a foreign key label was lost on any rename and outlived the connection it belonged to. A deleted connection's column layouts stayed on disk and in iCloud.

Fix

  • New TableScopedSettingsStore protocol (renameTable, renameContainer, purgeConnections) and TableScopedSettingsRegistry.stores, which lists all five shared instances.
  • movePerTableSettings, retargetContainer and ConnectionLocalState.purge loop over the registry, so a new store is wired into all three events by being registered.
  • Existing methods that already had the protocol's shape are renamed, not wrapped, so each operation has one entry point: renameLastFilters/renameScope/removeFilters(for:), rename/renameScope on the layout persister, and rename/renameScope/removeRules on highlight rules. Every caller and test is updated.
  • The two UserDefaults stores conform through new KeyValueStore.keys(withPrefix:) plus moveValue/moveValues/removeValues helpers, and TableScope.storagePrefix(connectionId:). A table rename moves one exact key, so orders_archive is untouched when orders is renamed. The container prefix ends in a separator, so shopping is untouched when shop is renamed.
  • Display As: purge also removes the pre-scope legacy keys, and a table rename carries a legacy key over to the new name.
  • Column layout purge deletes every purged connection's file first, then tombstones all of their records in one call, per the sync delete ordering rule. That call is a new batch SyncChangeTracker.markDeleted(_:ids:), backed by SyncMetadataStorage.removeDirty(_ ids:) and addTombstones. It does one read-modify-write and posts one change notification, where 300 saved layouts would otherwise post 300 notifications and rewrite a growing tombstone list 300 times. On a delete that arrived from iCloud, the tracker is already suppressed inside applyRemoteChanges, so no tombstones are pushed back.
  • A table rename now moves the saved browse search too. It only moved on a container rename before.
  • A layout rename where the old and new names match is a no-op. Before, it wrote a tombstone for a live record.

Highlight rules (#2738) are on main as 8a9ed73a3, so they conform here. The purge fix for their unreadable file needs no CHANGELOG line because the feature is unreleased.

Import and export are not affected: ConnectionExportEnvelope carries connections, groups, tags and credentials only.

Known gap, not fixed here: foreign key labels on MySQL and MariaDB

The foreign key picker keys its label through ForeignKeyLookupService.tableScope(from:reference:), which puts referencedSchema in the schema slot. On MySQL that value is REFERENCED_TABLE_SCHEMA, which is the database name, so a label is saved under (db: shop, schema: shop, table: users). A table rename builds (db: shop, schema: nil, table: users), the same scope every other per-table store uses, so the label key is not found and the label is still lost. A database rename rewrites the prefix but leaves schema: shop, so the picker then looks under store.store.

The fix is to make the label key the referenced table's own scope, which needs a rule for engines whose "schema" is a database (and for cross-database references), plus a migration for labels already saved. That is a change to the picker's key, not to the lifecycle wiring, so it is left for a follow-up. PostgreSQL, SQL Server, Oracle and other engines with a real schema layer are fixed by this PR.

Tests

  • ValueDisplayFormatStorageTests, ForeignKeyLabelColumnStoreTests: table, schema and database rename, purge, and the longer-name traps (orders_archive, public_old, shopping, chinook_backup). Display As also covers legacy key migration on rename and legacy key removal on purge.
  • FilterSettingsStorageTests: a table rename moves filters and browse search, read back from a fresh instance. A schema rename moves browse search.
  • ColumnLayoutSyncTests: purge removes the file, leaves other connections alone, and tombstones exactly the purged records. A rename tombstones the old record and marks the new one dirty.
  • HighlightRuleStorageTests: purge removes the unreadable file.
  • SyncChangeTrackerTests: a batch markDeleted clears each dirty flag and tombstones each id once, an empty batch does nothing, and suppression makes it a no-op.
  • New TableScopedSettingsRegistryTests: purge reaches every store once with every id, an empty purge reaches none, and a source scan of TablePro/Core/Storage fails if a class keyed by TableScope, CompositeStorageKey or ColumnLayoutTableKey does not conform or is not registered.

No UI test: the behaviour is storage bookkeeping, covered at the store and registry level.

Build passes. The nine affected suites run 102 cases, all passing: ValueDisplayFormatStorageTests, ForeignKeyLabelColumnStoreTests, FilterSettingsStorageTests, HighlightRuleStorageTests, ColumnLayoutSyncTests, FileColumnLayoutPersisterTests, TableScopedSettingsRegistryTests, TableScopeTests and SyncChangeTrackerTests. SwiftLint strict is clean over all 20 changed files, the test target and Packages/ included.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant