From 4aecad36492d544fbd7076b0754419f921d15bce Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gwendal=20Roue=CC=81?= Date: Wed, 6 Mar 2019 21:58:36 +0100 Subject: [PATCH 1/6] Failing test for https://github.com/groue/GRDB.swift/issues/495 --- .../AssociationBelongsToSQLTests.swift | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/Tests/GRDBTests/AssociationBelongsToSQLTests.swift b/Tests/GRDBTests/AssociationBelongsToSQLTests.swift index 3f619c67bb..97af38506b 100644 --- a/Tests/GRDBTests/AssociationBelongsToSQLTests.swift +++ b/Tests/GRDBTests/AssociationBelongsToSQLTests.swift @@ -873,4 +873,22 @@ class AssociationBelongsToSQLTests: GRDBTestCase { static let parent2 = belongsTo(Parent.self, using: ForeignKeys.parent2) } } + + // Regression test for https://github.com/groue/GRDB.swift/issues/495 + func testFetchCount() throws { + let dbQueue = try makeDatabaseQueue() + try dbQueue.write { db in + try db.create(table: "a") { t in + t.autoIncrementedPrimaryKey("id") + } + try db.create(table: "b") { t in + t.autoIncrementedPrimaryKey("id") + t.column("aId", .integer).references("a") + } + struct A: TableRecord { } + struct B: TableRecord { } + let request = B.joining(required: B.belongsTo(A.self)) + let _ = try request.fetchCount(db) + } + } } From 8fbc3ca8752685d5140eda64f3caf494f838c99e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gwendal=20Roue=CC=81?= Date: Wed, 6 Mar 2019 22:09:01 +0100 Subject: [PATCH 2/6] From future import OrderedDictionary --- GRDB/QueryInterface/SQLRelation.swift | 10 ++-- GRDB/Utils/OrderedDictionary.swift | 79 +++++++++++++++++++++------ 2 files changed, 66 insertions(+), 23 deletions(-) diff --git a/GRDB/QueryInterface/SQLRelation.swift b/GRDB/QueryInterface/SQLRelation.swift index 7187abdb18..144d911852 100644 --- a/GRDB/QueryInterface/SQLRelation.swift +++ b/GRDB/QueryInterface/SQLRelation.swift @@ -81,9 +81,9 @@ extension SQLRelation { // can't merge fatalError("The association key \"\(key)\" is ambiguous. Use the Association.forKey(_:) method is order to disambiguate.") } - relation.joins.append(value: mergedJoin, forKey: key) + relation.joins.appendValue(mergedJoin, forKey: key) } else { - relation.joins.append(value: join, forKey: key) + relation.joins.appendValue(join, forKey: key) } return relation } @@ -390,13 +390,13 @@ extension SQLRelation { // can't merge return nil } - mergedJoins.append(value: mergedJoin, forKey: key) + mergedJoins.appendValue(mergedJoin, forKey: key) } else { - mergedJoins.append(value: join, forKey: key) + mergedJoins.appendValue(join, forKey: key) } } for (key, join) in other.joins where mergedJoins[key] == nil { - mergedJoins.append(value: join, forKey: key) + mergedJoins.appendValue(join, forKey: key) } // replace selection unless empty diff --git a/GRDB/Utils/OrderedDictionary.swift b/GRDB/Utils/OrderedDictionary.swift index c4fb386095..d6a6a7ab4c 100644 --- a/GRDB/Utils/OrderedDictionary.swift +++ b/GRDB/Utils/OrderedDictionary.swift @@ -9,37 +9,74 @@ /// dict["qux"] // nil /// dict.map { $0.key } // ["foo", "bar"], in this order. struct OrderedDictionary { - private var keys: [Key] - private var values: [Key: Value] + private(set) var keys: [Key] + private(set) var dictionary: [Key: Value] + + var values: [Value] { + return keys.map { dictionary[$0]! } + } /// Creates an empty ordered dictionary. init() { - self.keys = [] - self.values = [:] + keys = [] + dictionary = [:] + } + + /// Creates an empty ordered dictionary. + init(minimumCapacity: Int) { + keys = [] + keys.reserveCapacity(minimumCapacity) + dictionary = Dictionary(minimumCapacity: minimumCapacity) } /// Returns the value associated with key, or nil. + @inlinable subscript(_ key: Key) -> Value? { - return values[key] + get { return dictionary[key] } + set { + if let value = newValue { + updateValue(value, forKey: key) + } else { + removeValue(forKey: key) + } + } } /// Appends the given value for the given key. /// /// - precondition: There is no value associated with key yet. - mutating func append(value: Value, forKey key: Key) { - guard values.updateValue(value, forKey: key) == nil else { + mutating func appendValue(_ value: Value, forKey key: Key) { + guard updateValue(value, forKey: key) == nil else { fatalError("key is already defined") } + } + + /// Updates the value stored in the dictionary for the given key, or + /// appnds a new key-value pair if the key does not exist. + /// + /// Use this method instead of key-based subscripting when you need to know + /// whether the new value supplants the value of an existing key. If the + /// value of an existing key is updated, updateValue(_:forKey:) returns the + /// original value. If the given key is not present in the dictionary, this + /// method appends the key-value pair and returns nil. + @discardableResult + @inlinable + mutating func updateValue(_ value: Value, forKey key: Key) -> Value? { + if let oldValue = dictionary.updateValue(value, forKey: key) { + return oldValue + } keys.append(key) + return nil } /// Removes the value associated with key. @discardableResult + @usableFromInline mutating func removeValue(forKey key: Key) -> Value? { - guard let value = values.removeValue(forKey: key) else { + guard let value = dictionary.removeValue(forKey: key) else { return nil } - let index = keys.index { $0 == key }! + let index = keys.firstIndex { $0 == key }! keys.remove(at: index) return value } @@ -48,35 +85,41 @@ struct OrderedDictionary { /// with the values transformed by the given closure. func mapValues(_ transform: (Value) throws -> T) rethrows -> OrderedDictionary { return try reduce(into: OrderedDictionary()) { dict, pair in - dict.append(value: try transform(pair.value), forKey: pair.key) + dict.appendValue(try transform(pair.value), forKey: pair.key) } } } extension OrderedDictionary: Collection { - typealias Index = Int + @usableFromInline typealias Index = Int - var startIndex: Int { + @usableFromInline var startIndex: Int { return 0 } - var endIndex: Int { + @usableFromInline var endIndex: Int { return keys.count } - func index(after i: Int) -> Int { + @usableFromInline func index(after i: Int) -> Int { return i + 1 } - subscript(position: Int) -> (key: Key, value: Value) { + @usableFromInline subscript(position: Int) -> (key: Key, value: Value) { let key = keys[position] - return (key: key, value: values[key]!) + return (key: key, value: dictionary[key]!) } } extension OrderedDictionary: ExpressibleByDictionaryLiteral { - init(dictionaryLiteral elements: (Key, Value)...) { + @usableFromInline init(dictionaryLiteral elements: (Key, Value)...) { self.keys = elements.map { $0.0 } - self.values = Dictionary(uniqueKeysWithValues: elements) + self.dictionary = Dictionary(uniqueKeysWithValues: elements) + } +} + +extension Dictionary { + init(_ orderedDictionary: OrderedDictionary) { + self = orderedDictionary.dictionary } } From ee481b2327c457884d8a39be77758b95de21a2b8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gwendal=20Roue=CC=81?= Date: Wed, 6 Mar 2019 22:09:31 +0100 Subject: [PATCH 3/6] Fix #495 --- .../QueryInterface/SQLGenerationContext.swift | 9 +++++-- .../SQLSelectQueryGenerator.swift | 16 ++++++++++-- .../SQLSelectable+QueryInterface.swift | 25 ++++++++++++++++--- 3 files changed, 42 insertions(+), 8 deletions(-) diff --git a/GRDB/QueryInterface/SQLGenerationContext.swift b/GRDB/QueryInterface/SQLGenerationContext.swift index 7a98561dd4..a28be67adb 100644 --- a/GRDB/QueryInterface/SQLGenerationContext.swift +++ b/GRDB/QueryInterface/SQLGenerationContext.swift @@ -19,10 +19,15 @@ public struct SQLGenerationContext { /// Used for SQLSelectQuery.makeSelectStatement() and SQLSelectQuery.makeDeleteStatement() static func queryGenerationContext(aliases: [TableAlias]) -> SQLGenerationContext { + let uniqueAliases = aliases.reduce(into: [TableAlias]()) { + if !$0.contains($1) { + $0.append($1) + } + } return SQLGenerationContext( arguments: [], - resolvedNames: aliases.resolvedNames, - qualifierNeeded: aliases.count > 1) + resolvedNames: uniqueAliases.resolvedNames, + qualifierNeeded: uniqueAliases.count > 1) } /// Used for TableRecord.selectionSQL diff --git a/GRDB/QueryInterface/SQLSelectQueryGenerator.swift b/GRDB/QueryInterface/SQLSelectQueryGenerator.swift index edf3c3571b..977786d19c 100644 --- a/GRDB/QueryInterface/SQLSelectQueryGenerator.swift +++ b/GRDB/QueryInterface/SQLSelectQueryGenerator.swift @@ -224,9 +224,12 @@ private struct SQLQualifiedRelation { /// All aliases, including aliases of joined relations var allAliases: [TableAlias] { - return joins.reduce(into: [alias]) { - $0.append(contentsOf: $1.value.relation.allAliases) + var aliases = [alias] + for join in joins.values { + aliases.append(contentsOf: join.relation.allAliases) } + aliases.append(contentsOf: source.allAliases) + return aliases } /// The source @@ -366,6 +369,15 @@ private enum SQLQualifiedSource { } } + var allAliases: [TableAlias] { + switch self { + case .table(_, let alias): + return [alias] + case .query(let query): + return query.relation.allAliases + } + } + init(_ source: SQLSource) { switch source { case .table(let tableName, let alias): diff --git a/GRDB/QueryInterface/SQLSelectable+QueryInterface.swift b/GRDB/QueryInterface/SQLSelectable+QueryInterface.swift index 411b888299..08fbe4c486 100644 --- a/GRDB/QueryInterface/SQLSelectable+QueryInterface.swift +++ b/GRDB/QueryInterface/SQLSelectable+QueryInterface.swift @@ -78,10 +78,27 @@ extension QualifiedAllColumns : SQLSelectable { } func countedSQL(_ context: inout SQLGenerationContext) -> String { - if context.qualifier(for: alias) != nil { - // SELECT COUNT(t.*) is invalid SQL - fatalError("Not implemented, or invalid query") - } + // TODO: restore the check below. + // + // It is currently disabled because of AssociationAggregateTests.testHasManyIsEmpty: + // + // let request = Team.having(Team.players.isEmpty) + // try XCTAssertEqual(request.fetchCount(db), 1) + // + // This should build the trivial count query `SELECT COUNT(*) FROM (SELECT ...)` + // + // Unfortunately, we don't support anonymous table aliases that would be + // required here. Because we don't support anonymous tables aliases, + // everything happens as if we wanted to generate + // `SELECT COUNT(team.*) FROM (SELECT ...)`, which is invalid SQL. + // + // So let's always return `*`, and fix this later. + +// if context.qualifier(for: alias) != nil { +// // SELECT COUNT(t.*) is invalid SQL +// fatalError("Not implemented, or invalid query") +// } + return "*" } From 5bed0ac58ec9bf9cd76c00afdb3c37b21284c55c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gwendal=20Roue=CC=81?= Date: Wed, 6 Mar 2019 22:12:54 +0100 Subject: [PATCH 4/6] Too much future --- GRDB/Utils/OrderedDictionary.swift | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/GRDB/Utils/OrderedDictionary.swift b/GRDB/Utils/OrderedDictionary.swift index d6a6a7ab4c..e0a5207ed7 100644 --- a/GRDB/Utils/OrderedDictionary.swift +++ b/GRDB/Utils/OrderedDictionary.swift @@ -30,7 +30,6 @@ struct OrderedDictionary { } /// Returns the value associated with key, or nil. - @inlinable subscript(_ key: Key) -> Value? { get { return dictionary[key] } set { @@ -60,7 +59,6 @@ struct OrderedDictionary { /// original value. If the given key is not present in the dictionary, this /// method appends the key-value pair and returns nil. @discardableResult - @inlinable mutating func updateValue(_ value: Value, forKey key: Key) -> Value? { if let oldValue = dictionary.updateValue(value, forKey: key) { return oldValue @@ -71,7 +69,6 @@ struct OrderedDictionary { /// Removes the value associated with key. @discardableResult - @usableFromInline mutating func removeValue(forKey key: Key) -> Value? { guard let value = dictionary.removeValue(forKey: key) else { return nil @@ -91,28 +88,28 @@ struct OrderedDictionary { } extension OrderedDictionary: Collection { - @usableFromInline typealias Index = Int + typealias Index = Int - @usableFromInline var startIndex: Int { + var startIndex: Int { return 0 } - @usableFromInline var endIndex: Int { + var endIndex: Int { return keys.count } - @usableFromInline func index(after i: Int) -> Int { + func index(after i: Int) -> Int { return i + 1 } - @usableFromInline subscript(position: Int) -> (key: Key, value: Value) { + subscript(position: Int) -> (key: Key, value: Value) { let key = keys[position] return (key: key, value: dictionary[key]!) } } extension OrderedDictionary: ExpressibleByDictionaryLiteral { - @usableFromInline init(dictionaryLiteral elements: (Key, Value)...) { + init(dictionaryLiteral elements: (Key, Value)...) { self.keys = elements.map { $0.0 } self.dictionary = Dictionary(uniqueKeysWithValues: elements) } From d566838dc55cf8477b88cb22c534f5eb913e6852 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gwendal=20Roue=CC=81?= Date: Wed, 6 Mar 2019 22:22:35 +0100 Subject: [PATCH 5/6] Restore missing comment --- GRDB/QueryInterface/SQLGenerationContext.swift | 1 + 1 file changed, 1 insertion(+) diff --git a/GRDB/QueryInterface/SQLGenerationContext.swift b/GRDB/QueryInterface/SQLGenerationContext.swift index a28be67adb..6a42c4790e 100644 --- a/GRDB/QueryInterface/SQLGenerationContext.swift +++ b/GRDB/QueryInterface/SQLGenerationContext.swift @@ -19,6 +19,7 @@ public struct SQLGenerationContext { /// Used for SQLSelectQuery.makeSelectStatement() and SQLSelectQuery.makeDeleteStatement() static func queryGenerationContext(aliases: [TableAlias]) -> SQLGenerationContext { + // Unique aliases, but with preserved ordering, so that we have stable SQL generation let uniqueAliases = aliases.reduce(into: [TableAlias]()) { if !$0.contains($1) { $0.append($1) From b0638707396ace88cfab11c5f011545e2ce32df3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gwendal=20Roue=CC=81?= Date: Thu, 7 Mar 2019 05:06:58 +0100 Subject: [PATCH 6/6] Too much future: firstIndex is not available on Xcode 9.4- --- GRDB/Utils/OrderedDictionary.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/GRDB/Utils/OrderedDictionary.swift b/GRDB/Utils/OrderedDictionary.swift index e0a5207ed7..126c2f97c0 100644 --- a/GRDB/Utils/OrderedDictionary.swift +++ b/GRDB/Utils/OrderedDictionary.swift @@ -73,7 +73,7 @@ struct OrderedDictionary { guard let value = dictionary.removeValue(forKey: key) else { return nil } - let index = keys.firstIndex { $0 == key }! + let index = keys.index { $0 == key }! keys.remove(at: index) return value }