From c6a4cd951e3efee3acbc98b3c56d77bdc026408e Mon Sep 17 00:00:00 2001 From: Ngo Quoc Dat Date: Wed, 23 Sep 2026 14:31:25 +0700 Subject: [PATCH] fix(sidebar): stop a period in a quoted name merging two objects' identities --- CHANGELOG.md | 1 + .../Database/DatabaseTreeTableRef.swift | 8 +-- TablePro/Models/Database/IdentityPath.swift | 23 ++++++++ TablePro/Models/Database/PartitionInfo.swift | 8 +-- TablePro/Models/Query/QueryResult.swift | 10 ++-- TablePro/Models/Query/RoutineInfo.swift | 5 +- .../Models/Query/UserDefinedTypeInfo.swift | 2 +- TablePro/Views/Sidebar/DatabaseTreeView.swift | 6 +-- .../Services/Query/SchemaServiceTests.swift | 18 +++++++ TableProTests/Models/IdentityPathTests.swift | 54 +++++++++++++++++++ TableProTests/Models/RoutineInfoTests.swift | 27 ++++++++++ TableProTests/Models/TableInfoTests.swift | 20 +++++++ .../Models/UserDefinedTypeInfoTests.swift | 9 ++++ .../Sidebar/DatabaseTreeFilterTests.swift | 26 +++++++++ .../Views/Sidebar/DatabaseTreeNodeTests.swift | 41 ++++++++++++++ .../Sidebar/SidebarPartitionRowTests.swift | 10 ++++ 16 files changed, 243 insertions(+), 25 deletions(-) create mode 100644 TablePro/Models/Database/IdentityPath.swift create mode 100644 TableProTests/Models/IdentityPathTests.swift diff --git a/CHANGELOG.md b/CHANGELOG.md index e2e438bd59..2bcf77b3c5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -321,6 +321,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Destination folder and the first database reading as one path in the backup result sheet. (#3046) - Only the last line of a failed backup's error shown, which on `pg_dump` is the hint rather than the cause. - Backup failure reported as an exit code alone when the tool wrote its message and exited at once. +- Table, routine or type missing from the sidebar or Open Quickly when a period in its quoted name matched another's. ### Security diff --git a/TablePro/Models/Database/DatabaseTreeTableRef.swift b/TablePro/Models/Database/DatabaseTreeTableRef.swift index 9dbe51f15b..e34d3b36c9 100644 --- a/TablePro/Models/Database/DatabaseTreeTableRef.swift +++ b/TablePro/Models/Database/DatabaseTreeTableRef.swift @@ -34,7 +34,7 @@ struct DatabaseTreeTableRef: Hashable, Identifiable, Sendable { /// quoted one may contain anything. Joined raw, schema `a|b` with table `c` and schema `a` /// with table `b|c` produced one id for two objects, and this id keys the outline's rows. var id: String { - "\(Self.escaped(database))|\(Self.escaped(schema))|\(Self.escaped(table.id))" + IdentityPath.joined([database ?? "", schema ?? "", table.id], separator: "|") } /// The schema the statement should qualify with, which is the row's own before the table's. @@ -51,10 +51,4 @@ struct DatabaseTreeTableRef: Hashable, Identifiable, Sendable { var favoriteSchema: String? { table.schema?.nilIfEmpty } - - private static func escaped(_ value: String?) -> String { - (value ?? "") - .replacingOccurrences(of: "\\", with: "\\\\") - .replacingOccurrences(of: "|", with: "\\|") - } } diff --git a/TablePro/Models/Database/IdentityPath.swift b/TablePro/Models/Database/IdentityPath.swift new file mode 100644 index 0000000000..d9212fe685 --- /dev/null +++ b/TablePro/Models/Database/IdentityPath.swift @@ -0,0 +1,23 @@ +import Foundation + +internal enum IdentityPath { + internal static func joined(_ components: [String], separator: Unicode.Scalar) -> String { + components + .map { escaped($0, separator: separator) } + .joined(separator: String(separator)) + } + + internal static func qualified(name: String, schema: String?) -> String { + guard let schema, !schema.isEmpty else { return escaped(name, separator: ".") } + return joined([schema, name], separator: ".") + } + + private static func escaped(_ component: String, separator: Unicode.Scalar) -> String { + guard component.unicodeScalars.contains(where: { $0 == separator || $0 == "\\" }) else { + return component + } + return component + .replacingOccurrences(of: "\\", with: "\\\\") + .replacingOccurrences(of: String(separator), with: "\\" + String(separator)) + } +} diff --git a/TablePro/Models/Database/PartitionInfo.swift b/TablePro/Models/Database/PartitionInfo.swift index 5159b7501e..4e916f2ebf 100644 --- a/TablePro/Models/Database/PartitionInfo.swift +++ b/TablePro/Models/Database/PartitionInfo.swift @@ -30,13 +30,7 @@ struct PartitionInfo: Identifiable, Hashable, Sendable { /// raw, schema `a.b` with name `c` and schema `a` with name `b.c` produce one id for two /// partitions, and this id keys the outline's rows. var id: String { - [parentPartitionName, schema, name].map(Self.escaped).joined(separator: "|") - } - - private static func escaped(_ value: String?) -> String { - (value ?? "") - .replacingOccurrences(of: "\\", with: "\\\\") - .replacingOccurrences(of: "|", with: "\\|") + IdentityPath.joined([parentPartitionName ?? "", schema ?? "", name], separator: "|") } init( diff --git a/TablePro/Models/Query/QueryResult.swift b/TablePro/Models/Query/QueryResult.swift index b8c3deac6f..d3c713e6f9 100644 --- a/TablePro/Models/Query/QueryResult.swift +++ b/TablePro/Models/Query/QueryResult.swift @@ -112,10 +112,7 @@ enum DatabaseError: Error, LocalizedError { /// Information about a database table struct TableInfo: Identifiable, Hashable, Sendable { var id: String { - if let schema, !schema.isEmpty { - return "\(schema).\(name)_\(type.rawValue)" - } - return "\(name)_\(type.rawValue)" + "\(IdentityPath.qualified(name: name, schema: schema))_\(type.rawValue)" } let name: String let type: TableType @@ -433,7 +430,10 @@ struct TriggerInfo: Identifiable, Hashable { let attributes: [ObjectAttribute] var id: String { - [schema, table, name].compactMap { $0?.isEmpty == false ? $0 : nil }.joined(separator: ".") + IdentityPath.joined( + [schema, table, name].compactMap { $0?.isEmpty == false ? $0 : nil }, + separator: "." + ) } init( diff --git a/TablePro/Models/Query/RoutineInfo.swift b/TablePro/Models/Query/RoutineInfo.swift index a981c27603..c1d87174f2 100644 --- a/TablePro/Models/Query/RoutineInfo.swift +++ b/TablePro/Models/Query/RoutineInfo.swift @@ -69,10 +69,11 @@ struct RoutineInfo: Identifiable, Hashable, Sendable { } var id: String { + let path = IdentityPath.qualified(name: name, schema: schema) guard let discriminator else { - return "\(kind.rawValue)_\(qualifiedName)" + return "\(kind.rawValue)_\(path)" } - return "\(kind.rawValue)_\(qualifiedName)_\(discriminator)" + return "\(kind.rawValue)_\(path)_\(discriminator)" } /// Equality follows `id` alone so a Set, a Dictionary and an outline view can never disagree diff --git a/TablePro/Models/Query/UserDefinedTypeInfo.swift b/TablePro/Models/Query/UserDefinedTypeInfo.swift index 5f09855bcb..a9f48beab2 100644 --- a/TablePro/Models/Query/UserDefinedTypeInfo.swift +++ b/TablePro/Models/Query/UserDefinedTypeInfo.swift @@ -122,7 +122,7 @@ struct UserDefinedTypeInfo: Identifiable, Hashable, Sendable { /// qualified name is the whole identity. The definition and the labels are deliberately left /// out: an edited enum must still be the same row. var id: String { - "type_\(qualifiedName)" + "type_\(IdentityPath.qualified(name: name, schema: schema))" } static func == (lhs: UserDefinedTypeInfo, rhs: UserDefinedTypeInfo) -> Bool { diff --git a/TablePro/Views/Sidebar/DatabaseTreeView.swift b/TablePro/Views/Sidebar/DatabaseTreeView.swift index 017e55a706..d50f21be2b 100644 --- a/TablePro/Views/Sidebar/DatabaseTreeView.swift +++ b/TablePro/Views/Sidebar/DatabaseTreeView.swift @@ -12,7 +12,7 @@ struct DatabaseTreeRoutineRef: Identifiable, Equatable { let routine: RoutineInfo var id: String { - "\(database ?? "")|\(schema ?? "")|\(routine.id)" + IdentityPath.joined([database ?? "", schema ?? "", routine.id], separator: "|") } var objectRef: DatabaseObjectRef { @@ -26,7 +26,7 @@ struct DatabaseTreeTriggerRef: Identifiable, Equatable { let trigger: TriggerInfo var id: String { - "\(database ?? "")|\(schema ?? "")|\(trigger.id)" + IdentityPath.joined([database ?? "", schema ?? "", trigger.id], separator: "|") } var objectRef: DatabaseObjectRef { @@ -40,7 +40,7 @@ struct DatabaseTreeUserTypeRef: Identifiable, Equatable { let type: UserDefinedTypeInfo var id: String { - "\(database ?? "")|\(schema ?? "")|\(type.id)" + IdentityPath.joined([database ?? "", schema ?? "", type.id], separator: "|") } var objectRef: DatabaseObjectRef { diff --git a/TableProTests/Core/Services/Query/SchemaServiceTests.swift b/TableProTests/Core/Services/Query/SchemaServiceTests.swift index 2b507c3800..e3646c226d 100644 --- a/TableProTests/Core/Services/Query/SchemaServiceTests.swift +++ b/TableProTests/Core/Services/Query/SchemaServiceTests.swift @@ -53,6 +53,24 @@ struct SchemaServiceTests { #expect(matching.count == 1) } + @Test("allLoadedTables keeps two tables whose names differ only in where a period sits") + func allLoadedTablesKeepsDottedNamesApart() async { + let connectionId = UUID() + let driver = MockDatabaseDriver() + driver.schemaTablesToReturn = [ + "a": [TableInfo(name: "b.c", type: .table, rowCount: 0, schema: "a")], + "a.b": [TableInfo(name: "c", type: .table, rowCount: 0, schema: "a.b")] + ] + + let service = SchemaService() + await service.loadSchemaObjects(connectionId: connectionId, schema: "a", driver: driver) + await service.loadSchemaObjects(connectionId: connectionId, schema: "a.b", driver: driver) + + let loaded = service.allLoadedTables(for: connectionId) + #expect(loaded.count == 2) + #expect(Set(loaded.map(\.schema)) == ["a", "a.b"]) + } + @Test("allLoadedTables is empty for a connection with no loaded state") func allLoadedTablesEmptyWhenNothingLoaded() { let service = SchemaService() diff --git a/TableProTests/Models/IdentityPathTests.swift b/TableProTests/Models/IdentityPathTests.swift new file mode 100644 index 0000000000..da96af7556 --- /dev/null +++ b/TableProTests/Models/IdentityPathTests.swift @@ -0,0 +1,54 @@ +import Foundation +@testable import TablePro +import Testing + +@Suite("IdentityPath") +struct IdentityPathTests { + @Test("Components holding neither the separator nor a backslash join unchanged") + func plainComponentsJoinUnchanged() { + #expect(IdentityPath.joined(["public", "orders", "audit"], separator: ".") == "public.orders.audit") + #expect(IdentityPath.joined(["db", "", "users_TABLE"], separator: "|") == "db||users_TABLE") + #expect(IdentityPath.qualified(name: "orders", schema: "sales") == "sales.orders") + #expect(IdentityPath.qualified(name: "orders", schema: nil) == "orders") + #expect(IdentityPath.qualified(name: "orders", schema: "") == "orders") + } + + @Test("A separator inside a component is escaped") + func separatorInsideComponentIsEscaped() { + #expect(IdentityPath.qualified(name: "b.c", schema: "a") == "a.b\\.c") + #expect(IdentityPath.qualified(name: "c", schema: "a.b") == "a\\.b.c") + #expect(IdentityPath.joined(["a|b", "c"], separator: "|") == "a\\|b|c") + } + + @Test("A backslash is escaped too, or a trailing one would swallow the separator") + func backslashIsEscaped() { + #expect(IdentityPath.qualified(name: "b", schema: "a\\") == "a\\\\.b") + #expect(IdentityPath.qualified(name: "a.b", schema: nil) == "a\\.b") + #expect(IdentityPath.qualified(name: "b", schema: "a\\") != IdentityPath.qualified(name: "a.b", schema: nil)) + } + + @Test("Only the requested separator is escaped") + func otherSeparatorsStayRaw() { + #expect(IdentityPath.joined(["a.b", "c"], separator: "|") == "a.b|c") + #expect(IdentityPath.joined(["a|b", "c"], separator: ".") == "a|b.c") + } + + @Test("Distinct component lists never share a path", arguments: ["." as Unicode.Scalar, "|" as Unicode.Scalar]) + func distinctListsNeverCollide(separator: Unicode.Scalar) { + let pieces = ["", "a", "b", "a.b", "a|b", "a\\", "\\", ".", "|", "b.", "b|", "\\."] + var lists: [[String]] = pieces.map { [$0] } + for first in pieces { + for second in pieces { + lists.append([first, second]) + } + } + var seen: [String: [String]] = [:] + for list in lists { + let path = IdentityPath.joined(list, separator: separator) + if let earlier = seen[path] { + #expect(earlier == list, "\(earlier) and \(list) share \(path)") + } + seen[path] = list + } + } +} diff --git a/TableProTests/Models/RoutineInfoTests.swift b/TableProTests/Models/RoutineInfoTests.swift index ea495ab8ab..277cd33add 100644 --- a/TableProTests/Models/RoutineInfoTests.swift +++ b/TableProTests/Models/RoutineInfoTests.swift @@ -69,6 +69,19 @@ struct RoutineInfoTests { #expect(routine.id == "PROCEDURE_app.do_thing") } + @Test("A period inside a quoted schema or routine name keeps two routines apart") + func periodInsideNameKeepsRoutinesApart() { + let dottedName = RoutineInfo(name: "b.c", kind: .function, schema: "a") + let dottedSchema = RoutineInfo(name: "c", kind: .function, schema: "a.b") + let unqualified = RoutineInfo(name: "a.b", kind: .function) + let qualified = RoutineInfo(name: "b", kind: .function, schema: "a") + + #expect(dottedName != dottedSchema) + #expect(unqualified != qualified) + #expect(Set([dottedName, dottedSchema, unqualified, qualified]).count == 4) + #expect(dottedName.qualifiedName == "a.b.c") + } + @Test("Return type is never used as the overload discriminator") func returnTypeIsNotADiscriminator() { let a = RoutineInfo(name: "f", kind: .function, schema: "public", returnType: "integer") @@ -105,6 +118,20 @@ struct TriggerInfoTests { #expect(trigger.qualifiedName == "orders.audit") } + @Test("A period inside a quoted schema or table name keeps two triggers apart") + func periodInsideNameKeepsTriggersApart() { + let dottedTable = TriggerInfo( + name: "audit", timing: "AFTER", event: "INSERT", statement: "", + table: "b.c", schema: "a" + ) + let dottedSchema = TriggerInfo( + name: "audit", timing: "AFTER", event: "INSERT", statement: "", + table: "c", schema: "a.b" + ) + #expect(dottedTable.id != dottedSchema.id) + #expect(dottedTable.qualifiedName == "b.c.audit") + } + @Test("A trigger with no table falls back to its name") func tablelessFallback() { let trigger = TriggerInfo(name: "audit", timing: "AFTER", event: "INSERT", statement: "") diff --git a/TableProTests/Models/TableInfoTests.swift b/TableProTests/Models/TableInfoTests.swift index d414f13d82..98422622db 100644 --- a/TableProTests/Models/TableInfoTests.swift +++ b/TableProTests/Models/TableInfoTests.swift @@ -52,6 +52,26 @@ struct TableInfoTests { #expect(info.id == "analytics.events_TABLE") } + @Test("A period inside a quoted schema or table name does not merge two tables' ids") + func periodInsideNameKeepsIdsDistinct() { + let dottedTable = TableInfo(name: "b.c", type: .table, rowCount: nil, schema: "a") + let dottedSchema = TableInfo(name: "c", type: .table, rowCount: nil, schema: "a.b") + let unqualified = TableInfo(name: "a.b", type: .table, rowCount: nil) + let qualified = TableInfo(name: "b", type: .table, rowCount: nil, schema: "a") + let trailingBackslash = TableInfo(name: "b", type: .table, rowCount: nil, schema: "a\\") + + let ids = [dottedTable, dottedSchema, unqualified, qualified, trailingBackslash].map(\.id) + #expect(Set(ids).count == ids.count) + #expect(Dictionary(grouping: [dottedTable, dottedSchema], by: \.id).count == 2) + } + + @Test("A name with no period or backslash keeps the id it always had") + func plainNamesKeepTheirId() { + #expect(TableInfo(name: "events", type: .view, rowCount: nil, schema: "analytics").id == "analytics.events_VIEW") + #expect(TableInfo(name: "user_log", type: .materializedView, rowCount: nil).id == "user_log_MATERIALIZED VIEW") + #expect(TableInfo(name: "orders", type: .table, rowCount: nil, schema: "").id == "orders_TABLE") + } + @Test("Same table name in different schemas has distinct id, equality, and hash") func testCrossSchemaDistinctIdentity() { let a = TableInfo(name: "orders", type: .table, rowCount: nil, schema: "dataset_a") diff --git a/TableProTests/Models/UserDefinedTypeInfoTests.swift b/TableProTests/Models/UserDefinedTypeInfoTests.swift index 5c909a9c7a..65b17f7c5f 100644 --- a/TableProTests/Models/UserDefinedTypeInfoTests.swift +++ b/TableProTests/Models/UserDefinedTypeInfoTests.swift @@ -30,6 +30,15 @@ struct UserDefinedTypeInfoTests { #expect(one.qualifiedName == "app.mood") } + @Test("A period inside a quoted schema or type name keeps two types apart") + func periodInsideNameKeepsTypesApart() { + let dottedName = UserDefinedTypeInfo(name: "b.c", kind: .enumeration, schema: "a") + let dottedSchema = UserDefinedTypeInfo(name: "c", kind: .enumeration, schema: "a.b") + #expect(dottedName != dottedSchema) + #expect(Set([dottedName, dottedSchema]).count == 2) + #expect(dottedName.qualifiedName == dottedSchema.qualifiedName) + } + @Test("A type with no schema is named bare") func bareName() { let type = UserDefinedTypeInfo(name: "mood", kind: .enumeration) diff --git a/TableProTests/Views/Sidebar/DatabaseTreeFilterTests.swift b/TableProTests/Views/Sidebar/DatabaseTreeFilterTests.swift index b044aab3d4..08dd07f890 100644 --- a/TableProTests/Views/Sidebar/DatabaseTreeFilterTests.swift +++ b/TableProTests/Views/Sidebar/DatabaseTreeFilterTests.swift @@ -20,6 +20,32 @@ struct DatabaseTreeFilterTests { #expect(result.map(\.name) == ["users", "orders"]) } + @Test("filteredTables keeps two tables whose qualified names differ only in where a period sits") + func filteredTablesKeepsDottedNamesApart() { + let tables = [ + TableInfo(name: "b.c", type: .table, rowCount: 0, schema: "a"), + TableInfo(name: "c", type: .table, rowCount: 0, schema: "a.b"), + TableInfo(name: "a.b", type: .table, rowCount: 0), + TableInfo(name: "b", type: .table, rowCount: 0, schema: "a") + ] + #expect(DatabaseTreeFilter.filteredTables(tables, searchText: "").count == 4) + #expect(DatabaseTreeFilter.filteredTables(tables, searchText: "c").count == 2) + } + + @Test("filteredRoutines and filteredUserTypes keep objects whose schema holds a period") + func filteredRoutinesAndTypesKeepDottedNamesApart() { + let routines = [ + RoutineInfo(name: "b.c", kind: .function, schema: "a"), + RoutineInfo(name: "c", kind: .function, schema: "a.b") + ] + let types = [ + UserDefinedTypeInfo(name: "b.c", kind: .enumeration, schema: "a"), + UserDefinedTypeInfo(name: "c", kind: .enumeration, schema: "a.b") + ] + #expect(DatabaseTreeFilter.filteredRoutines(routines, searchText: "").count == 2) + #expect(DatabaseTreeFilter.filteredUserTypes(types, searchText: "").count == 2) + } + @Test("filteredTables keeps only substring matches when searching") func filteredTablesSearch() { let tables = [table("users"), table("orders"), table("invoices")] diff --git a/TableProTests/Views/Sidebar/DatabaseTreeNodeTests.swift b/TableProTests/Views/Sidebar/DatabaseTreeNodeTests.swift index 57096f07ce..f4bff23bff 100644 --- a/TableProTests/Views/Sidebar/DatabaseTreeNodeTests.swift +++ b/TableProTests/Views/Sidebar/DatabaseTreeNodeTests.swift @@ -29,6 +29,47 @@ struct DatabaseTreeNodeTests { #expect(Set([databaseId, schemaId, tableId, tableGroupId, otherSchemaGroupId]).count == 5) } + @Test("A pipe inside a database or schema name does not merge two object rows") + func pipeInsideContainerNameKeepsRowsApart() { + let routine = RoutineInfo(name: "f", kind: .function) + let trigger = TriggerInfo(name: "audit", timing: "AFTER", event: "INSERT", statement: "") + let type = UserDefinedTypeInfo(name: "mood", kind: .enumeration) + + let routineIds = [ + DatabaseTreeRoutineRef(database: "a|b", schema: nil, routine: routine).id, + DatabaseTreeRoutineRef(database: "a", schema: "b|", routine: routine).id + ] + let triggerIds = [ + DatabaseTreeTriggerRef(database: "a|b", schema: nil, trigger: trigger).id, + DatabaseTreeTriggerRef(database: "a", schema: "b|", trigger: trigger).id + ] + let typeIds = [ + DatabaseTreeUserTypeRef(database: "a|b", schema: nil, type: type).id, + DatabaseTreeUserTypeRef(database: "a", schema: "b|", type: type).id + ] + + #expect(Set(routineIds).count == 2) + #expect(Set(triggerIds).count == 2) + #expect(Set(typeIds).count == 2) + #expect( + DatabaseTreeRoutineRef(database: "shop", schema: "public", routine: routine).id + == "shop|public|FUNCTION_f" + ) + } + + @Test("A table row keeps its id apart from one whose schema holds the period instead") + func periodInsideTableNameKeepsTableRowsApart() { + let dottedTable = DatabaseTreeTableRef( + database: "shop", schema: nil, table: TableInfo(name: "b.c", type: .table, rowCount: 0, schema: "a") + ) + let dottedSchema = DatabaseTreeTableRef( + database: "shop", schema: nil, table: TableInfo(name: "c", type: .table, rowCount: 0, schema: "a.b") + ) + #expect(dottedTable.id != dottedSchema.id) + #expect(DatabaseTreeNode.tableId(dottedTable) != DatabaseTreeNode.tableId(dottedSchema)) + #expect(tableRef("users").id == "shop|public|users_TABLE") + } + private func partitionRef( _ name: String, parent: String = "orders", diff --git a/TableProTests/Views/Sidebar/SidebarPartitionRowTests.swift b/TableProTests/Views/Sidebar/SidebarPartitionRowTests.swift index b3dadcc8f5..09c51f6ef9 100644 --- a/TableProTests/Views/Sidebar/SidebarPartitionRowTests.swift +++ b/TableProTests/Views/Sidebar/SidebarPartitionRowTests.swift @@ -113,6 +113,16 @@ struct PartitionCountRefreshTests { #expect(!DatabaseTreeMetadataService.partitionCountsChanged(from: before, to: after)) } + @Test("Two tables whose names differ only in where a period sits keep their own counts") + func dottedNamesKeepTheirOwnCounts() { + let tables = [ + TableInfo(name: "c", type: .table, rowCount: nil, schema: "a.b", partitionCount: 1), + TableInfo(name: "b.c", type: .table, rowCount: nil, schema: "a", partitionCount: 2) + ] + + #expect(!DatabaseTreeMetadataService.partitionCountsChanged(from: .loaded(tables), to: .loaded(tables))) + } + @Test("A first load is a change, and a failed refresh is not") func absentAndFailedStates() { let loaded: MetadataLoadState<[TableInfo]> = .loaded([table("events", partitionCount: 1)])