Merge pull request #677 from groue/dev/fixAssociationFiltering

Fix associations altered by another association

CHANGELOG.md

@@ -6,9 +6,7 @@ All notable changes to this project will be documented in this file.
 GRDB adheres to [Semantic Versioning](https://semver.org/), with one exception: APIs flagged [**:fire: EXPERIMENTAL**](README.md#what-are-experimental-features). Those are unstable, and may break between any two minor releases of the library.
 
 
-<!--
 [Next Release](#next-release)
--->
 
 
 #### 4.x Releases
@@ -61,9 +59,11 @@ GRDB adheres to [Semantic Versioning](https://semver.org/), with one exception:
 - [0.110.0](#01100), ...
 
 
-<!--
 ## Next Release
--->
+
+**Fixed**
+
+- [#677](https://github.com/groue/GRDB.swift/pull/677): Fix associations altered by another association
 
 
 ## 4.8.0

GRDB/QueryInterface/SQL/SQLAssociation.swift

@@ -55,7 +55,7 @@ public /* TODO: internal */ struct SQLAssociation {
     // All steps, from pivot to destination. Never empty.
     private(set) var steps: [SQLAssociationStep]
     var keyPath: [String] {
-        return steps.map { $0.key.name(for: $0.cardinality) }
+        return steps.map { $0.keyName }
     }
 
     var destination: SQLAssociationStep {
@@ -189,25 +189,27 @@ public /* TODO: internal */ struct SQLAssociation {
         //     through: Origin.hasMany(Pivot.self),
         //     via: Pivot.belongsTo(Destination.self))
         // Origin(id: 1).request(for: association)
-        let reversedSteps = zip(steps, steps.dropFirst())
-            .map { (step, nextStep) -> SQLAssociationStep in
-                // Intermediate steps are not included in the selection, and
-                // don't have any child.
-                let relation = step.relation.select([]).deletingChildren()
+        var filteredSteps = steps
+        filteredSteps[0] = pivot.mapRelation { _ in filteredPivotRelation }
+        let reversedSteps = zip(filteredSteps, filteredSteps.dropFirst())
+            .map({ (step, nextStep) -> SQLAssociationStep in
+                // Intermediate steps are not selected, and including(all:)
+                // children are useless:
+                let relation = step.relation
+                    .selectOnly([])
+                    .filteringChildren { $0.kind.cardinality == .toOne }
+
+                // Don't interfere with user-defined keys that could be added later
+                let key = step.key.mapName { "grdb_\($0)" }
+
                 return SQLAssociationStep(
-                    key: step.key,
+                    key: key,
                     condition: nextStep.condition.reversed,
                     relation: relation,
-                    cardinality: step.cardinality)
-            }
+                    cardinality: .toOne)
+            })
             .reversed()
-
-        var reversedAssociation = SQLAssociation(steps: Array(reversedSteps))
-        // Replace pivot with the filtered one (not included in the selection,
-        // without children).
-        reversedAssociation = reversedAssociation.mapDestinationRelation { _ in
-            filteredPivotRelation.select([]).deletingChildren()
-        }
+        let reversedAssociation = SQLAssociation(steps: Array(reversedSteps))
         return destination.relation.appendingChild(for: reversedAssociation, kind: .oneRequired)
     }
 }
@@ -218,6 +220,10 @@ struct SQLAssociationStep {
     var relation: SQLRelation
     var cardinality: SQLAssociationCardinality
 
+    var keyName: String {
+        return key.name(for: cardinality)
+    }
+
     func mapRelation(_ transform: (SQLRelation) -> SQLRelation) -> SQLAssociationStep {
         return SQLAssociationStep(
             key: key,
@@ -350,4 +356,17 @@ enum SQLAssociationKey {
             return name
         }
     }
+
+    func mapName(_ transform: (String) throws -> String) rethrows -> SQLAssociationKey {
+        switch self {
+        case .inflected(let name):
+            return try .inflected(transform(name))
+        case .fixedSingular(let name):
+            return try .fixedSingular(transform(name))
+        case .fixedPlural(let name):
+            return try .fixedPlural(transform(name))
+        case .fixed(let name):
+            return try .fixed(transform(name))
+        }
+    }
 }

GRDB/QueryInterface/SQL/SQLRelation.swift

@@ -167,8 +167,14 @@ struct SQLRelation {
             case .allPrefetched:
                 return [child.makeAssociationForKey(key)]
             case .oneOptional, .oneRequired, .allNotPrefetched:
-                return child.relation.prefetchedAssociations.map {
-                    $0.through(child.makeAssociationForKey(key))
+                return child.relation.prefetchedAssociations.map { association in
+                    // Remove redundant pivot child
+                    let pivotKey = association.pivot.keyName
+                    let child = child.mapRelation { relation in
+                        assert(relation.children[pivotKey] != nil)
+                        return relation.removingChild(forKey: pivotKey)
+                    }
+                    return association.through(child.makeAssociationForKey(key))
                 }
             }
         }
@@ -196,6 +202,14 @@ extension SQLRelation {
         return relation
     }
 
+    /// Removes all selections from chidren
+    func selectOnly(_ selection: [SQLSelectable]) -> SQLRelation {
+        var relation = self
+        relation.selection = selection
+        relation.children = relation.children.mapValues { $0.mapRelation { $0.selectOnly([]) } }
+        return relation
+    }
+
     func annotated(with selection: [SQLSelectable]) -> SQLRelation {
         var relation = self
         relation.selection.append(contentsOf: selection)
@@ -239,12 +253,6 @@ extension SQLRelation {
 }
 
 extension SQLRelation {
-    func deletingChildren() -> SQLRelation {
-        var relation = self
-        relation.children = [:]
-        return relation
-    }
-
     /// Returns a relation extended with an association.
     ///
     /// This method provides support for public joining methods such
@@ -359,6 +367,18 @@ extension SQLRelation {
         }
         return relation
     }
+
+    func removingChild(forKey key: String) -> SQLRelation {
+        var relation = self
+        relation.children.removeValue(forKey: key)
+        return relation
+    }
+
+    func filteringChildren(_ included: (Child) throws -> Bool) rethrows -> SQLRelation {
+        var relation = self
+        relation.children = try relation.children.filter { try included($1) }
+        return relation
+    }
 }
 
 extension SQLRelation: _JoinableRequest {
@@ -616,16 +636,16 @@ struct SQLAssociationCondition: Equatable {
             // Join on a multiple columns.
             // ((table.a = 1) AND (table.b = 2)) OR ((table.a = 3) AND (table.b = 4)) ...
             return leftRows
-                .map { leftRow in
+                .map({ leftRow in
                     // (table.a = 1) AND (table.b = 2)
                     columnMappings
-                        .map { columns -> SQLExpression in
+                        .map({ columns -> SQLExpression in
                             let rightColumn = QualifiedColumn(columns.right, alias: rightAlias)
                             let leftValue = leftRow[columns.left] as DatabaseValue
                             return rightColumn == leftValue
-                        }
+                        })
                         .joined(operator: .and)
-                }
+                })
                 .joined(operator: .or)
         }
     }

GRDB/Utils/OrderedDictionary.swift

@@ -17,6 +17,12 @@ struct OrderedDictionary<Key: Hashable, Value> {
         return keys.map { dictionary[$0]! }
     }
 
+    private init(keys: [Key], dictionary: [Key: Value]) {
+        assert(Set(keys) == Set(dictionary.keys))
+        self.keys = keys
+        self.dictionary = dictionary
+    }
+
     /// Creates an empty ordered dictionary.
     init() {
         keys = []
@@ -108,6 +114,12 @@ struct OrderedDictionary<Key: Hashable, Value> {
             }
         }
     }
+
+    func filter(_ isIncluded: ((key: Key, value: Value)) throws -> Bool) rethrows -> OrderedDictionary<Key, Value> {
+        let dictionary = try self.dictionary.filter(isIncluded)
+        let keys = self.keys.filter(dictionary.keys.contains)
+        return OrderedDictionary(keys: keys, dictionary: dictionary)
+    }
 }
 
 extension OrderedDictionary: Collection {

Tests/GRDBTests/AssociationHasManySQLTests.swift

@@ -764,4 +764,75 @@ class AssociationHasManySQLTests: GRDBTestCase {
             }
         }
     }
+
+    func testAssociationFilteredByOtherAssociation() throws {
+        struct Toy: TableRecord { }
+        struct Child: TableRecord {
+            static let toy = hasOne(Toy.self)
+            static let parent = belongsTo(Parent.self)
+        }
+        struct Parent: TableRecord, EncodableRecord {
+            static let children = hasMany(Child.self)
+            func encode(to container: inout PersistenceContainer) {
+                container["id"] = 1
+            }
+        }
+
+        let dbQueue = try makeDatabaseQueue()
+        try dbQueue.inDatabase { db in
+            try db.create(table: "parent") { t in
+                t.autoIncrementedPrimaryKey("id")
+            }
+            try db.create(table: "child") { t in
+                t.column("parentId", .integer).references("parent")
+            }
+            try db.create(table: "toy") { t in
+                t.column("childId", .integer).references("child")
+            }
+        }
+
+        try dbQueue.inDatabase { db in
+            do {
+                let association = Parent.children.joining(required: Child.toy)
+                try assertEqualSQL(db, Parent.all().including(required: association), """
+                    SELECT "parent".*, "child".* \
+                    FROM "parent" JOIN "child" ON "child"."parentId" = "parent"."id" \
+                    JOIN "toy" ON "toy"."childId" = "child"."rowid"
+                    """)
+                try assertEqualSQL(db, Parent.all().joining(required: association), """
+                    SELECT "parent".* FROM "parent" \
+                    JOIN "child" ON "child"."parentId" = "parent"."id" \
+                    JOIN "toy" ON "toy"."childId" = "child"."rowid"
+                    """)
+                try assertEqualSQL(db, Parent().request(for: association), """
+                    SELECT "child".* \
+                    FROM "child" \
+                    JOIN "toy" ON "toy"."childId" = "child"."rowid" \
+                    WHERE "child"."parentId" = 1
+                    """)
+            }
+            do {
+                // not a realistic use case, but testable anyway
+                let association = Parent.children.joining(required: Child.parent)
+                try assertEqualSQL(db, Parent.all().including(required: association), """
+                    SELECT "parent1".*, "child".* \
+                    FROM "parent" "parent1" \
+                    JOIN "child" ON "child"."parentId" = "parent1"."id" \
+                    JOIN "parent" "parent2" ON "parent2"."id" = "child"."parentId"
+                    """)
+                try assertEqualSQL(db, Parent.all().joining(required: association), """
+                    SELECT "parent1".* \
+                    FROM "parent" "parent1" \
+                    JOIN "child" ON "child"."parentId" = "parent1"."id" \
+                    JOIN "parent" "parent2" ON "parent2"."id" = "child"."parentId"
+                    """)
+                try assertEqualSQL(db, Parent().request(for: association), """
+                    SELECT "child".* \
+                    FROM "child" \
+                    JOIN "parent" ON "parent"."id" = "child"."parentId" \
+                    WHERE "child"."parentId" = 1
+                    """)
+            }
+        }
+    }
 }