Merge pull request #496 from groue/feature/fix-495

Fix crash when trying to perform fetchCount or ValueObservation.trackCount on joined query

GRDB/QueryInterface/SQLGenerationContext.swift

@@ -19,10 +19,16 @@ 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)
+            }
+        }
         return SQLGenerationContext(
             arguments: [],
-            resolvedNames: aliases.resolvedNames,
-            qualifierNeeded: aliases.count > 1)
+            resolvedNames: uniqueAliases.resolvedNames,
+            qualifierNeeded: uniqueAliases.count > 1)
     }
 
     /// Used for TableRecord.selectionSQL

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

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):

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 "*"
     }
 

GRDB/Utils/OrderedDictionary.swift

@@ -9,34 +9,68 @@
 ///     dict["qux"] // nil
 ///     dict.map { $0.key } // ["foo", "bar"], in this order.
 struct OrderedDictionary<Key: Hashable, Value> {
-    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.
     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
+    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
     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 }!
@@ -48,7 +82,7 @@ struct OrderedDictionary<Key: Hashable, Value> {
     /// with the values transformed by the given closure.
     func mapValues<T>(_ transform: (Value) throws -> T) rethrows -> OrderedDictionary<Key, T> {
         return try reduce(into: OrderedDictionary<Key, T>()) { dict, pair in
-            dict.append(value: try transform(pair.value), forKey: pair.key)
+            dict.appendValue(try transform(pair.value), forKey: pair.key)
         }
     }
 }
@@ -68,15 +102,21 @@ extension OrderedDictionary: Collection {
         return i + 1
     }
 
-    subscript(position: Int) -> (key: Key, value: Value) {
+     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)...) {
         self.keys = elements.map { $0.0 }
-        self.values = Dictionary(uniqueKeysWithValues: elements)
+        self.dictionary = Dictionary(uniqueKeysWithValues: elements)
+    }
+}
+
+extension Dictionary {
+    init(_ orderedDictionary: OrderedDictionary<Key, Value>) {
+        self = orderedDictionary.dictionary
     }
 }

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)
+        }
+    }
 }