diff --git a/CHANGELOG.md b/CHANGELOG.md index 61f8a5c10d..b6f68b567a 100644 --- a/CHANGELOG.md +++ b/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. - #### 4.x Releases @@ -61,9 +59,11 @@ GRDB adheres to [Semantic Versioning](https://semver.org/), with one exception: - [0.110.0](#01100), ... - + +**Fixed** + +- [#677](https://github.com/groue/GRDB.swift/pull/677): Fix associations altered by another association ## 4.8.0 diff --git a/GRDB/QueryInterface/SQL/SQLAssociation.swift b/GRDB/QueryInterface/SQL/SQLAssociation.swift index 6419373fe9..44a6b7e920 100644 --- a/GRDB/QueryInterface/SQL/SQLAssociation.swift +++ b/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)) + } + } } diff --git a/GRDB/QueryInterface/SQL/SQLRelation.swift b/GRDB/QueryInterface/SQL/SQLRelation.swift index ec7f167eb6..90cd7d86d3 100644 --- a/GRDB/QueryInterface/SQL/SQLRelation.swift +++ b/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) } } diff --git a/GRDB/Utils/OrderedDictionary.swift b/GRDB/Utils/OrderedDictionary.swift index 20a3388c25..bb1c725828 100644 --- a/GRDB/Utils/OrderedDictionary.swift +++ b/GRDB/Utils/OrderedDictionary.swift @@ -17,6 +17,12 @@ struct OrderedDictionary { 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 { } } } + + func filter(_ isIncluded: ((key: Key, value: Value)) throws -> Bool) rethrows -> OrderedDictionary { + let dictionary = try self.dictionary.filter(isIncluded) + let keys = self.keys.filter(dictionary.keys.contains) + return OrderedDictionary(keys: keys, dictionary: dictionary) + } } extension OrderedDictionary: Collection { diff --git a/Tests/GRDBTests/AssociationHasManySQLTests.swift b/Tests/GRDBTests/AssociationHasManySQLTests.swift index af7b336d50..99443d3fbf 100644 --- a/Tests/GRDBTests/AssociationHasManySQLTests.swift +++ b/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 + """) + } + } + } } diff --git a/Tests/GRDBTests/AssociationHasManyThroughSQLTests.swift b/Tests/GRDBTests/AssociationHasManyThroughSQLTests.swift index 4c8c585bbf..269b1885e2 100644 --- a/Tests/GRDBTests/AssociationHasManyThroughSQLTests.swift +++ b/Tests/GRDBTests/AssociationHasManyThroughSQLTests.swift @@ -313,4 +313,125 @@ class AssociationHasManyThroughSQLTests: GRDBTestCase { """) } } + + func testAssociationFilteredByOtherAssociation() throws { + struct Pet: TableRecord { + static let child = belongsTo(Child.self) + } + struct Toy: TableRecord { } + struct Child: TableRecord { + static let toy = hasOne(Toy.self) + static let pets = hasMany(Pet.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 db.create(table: "pet") { t in + t.column("childId", .integer).references("child") + } + } + + try dbQueue.inDatabase { db in + do { + let association = Parent.hasMany( + Pet.self, + through: Parent.children.joining(required: Child.toy), + using: Child.pets) + try assertEqualSQL(db, Parent.all().including(required: association), """ + SELECT "parent".*, "pet".* \ + FROM "parent" \ + JOIN "child" ON "child"."parentId" = "parent"."id" \ + JOIN "toy" ON "toy"."childId" = "child"."rowid" \ + JOIN "pet" ON "pet"."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" \ + JOIN "pet" ON "pet"."childId" = "child"."rowid" + """) + try assertEqualSQL(db, Parent().request(for: association), """ + SELECT "pet".* \ + FROM "pet" \ + JOIN "child" ON ("child"."rowid" = "pet"."childId") AND ("child"."parentId" = 1) \ + JOIN "toy" ON "toy"."childId" = "child"."rowid" + """) + } + do { + let association = Parent.hasMany( + Pet.self, + through: Parent.children.filter(sql: "1 + 1"), + using: Child.pets.joining(required: Pet.child.filter(sql: "1").joining(required: Child.toy))) + try assertEqualSQL(db, Parent.all().including(required: association), """ + SELECT "parent".*, "pet".* \ + FROM "parent" \ + JOIN "child" "child1" ON ("child1"."parentId" = "parent"."id") AND (1 + 1) \ + JOIN "pet" ON "pet"."childId" = "child1"."rowid" \ + JOIN "child" "child2" ON ("child2"."rowid" = "pet"."childId") AND (1) \ + JOIN "toy" ON "toy"."childId" = "child2"."rowid" + """) + try assertEqualSQL(db, Parent.all().joining(required: association), """ + SELECT "parent".* \ + FROM "parent" \ + JOIN "child" "child1" ON ("child1"."parentId" = "parent"."id") AND (1 + 1) \ + JOIN "pet" ON "pet"."childId" = "child1"."rowid" \ + JOIN "child" "child2" ON ("child2"."rowid" = "pet"."childId") AND (1) \ + JOIN "toy" ON "toy"."childId" = "child2"."rowid" + """) + try assertEqualSQL(db, Parent().request(for: association), """ + SELECT "pet".* \ + FROM "pet" \ + JOIN "child" "child1" ON ("child1"."rowid" = "pet"."childId") AND (1) \ + JOIN "toy" ON "toy"."childId" = "child1"."rowid" \ + JOIN "child" "child2" ON ("child2"."rowid" = "pet"."childId") AND (1 + 1) AND ("child2"."parentId" = 1) + """) + } + do { + let association = Parent.hasMany( + Pet.self, + through: Parent.children.filter(sql: "1 + 1"), + using: Child.pets) + .joining(required: Pet.child.filter(sql: "1").joining(required: Child.toy)) + try assertEqualSQL(db, Parent.all().including(required: association), """ + SELECT "parent".*, "pet".* \ + FROM "parent" \ + JOIN "child" "child1" ON ("child1"."parentId" = "parent"."id") AND (1 + 1) \ + JOIN "pet" ON "pet"."childId" = "child1"."rowid" \ + JOIN "child" "child2" ON ("child2"."rowid" = "pet"."childId") AND (1) \ + JOIN "toy" ON "toy"."childId" = "child2"."rowid" + """) + try assertEqualSQL(db, Parent.all().joining(required: association), """ + SELECT "parent".* \ + FROM "parent" \ + JOIN "child" "child1" ON ("child1"."parentId" = "parent"."id") AND (1 + 1) \ + JOIN "pet" ON "pet"."childId" = "child1"."rowid" \ + JOIN "child" "child2" ON ("child2"."rowid" = "pet"."childId") AND (1) \ + JOIN "toy" ON "toy"."childId" = "child2"."rowid" + """) + try assertEqualSQL(db, Parent().request(for: association), """ + SELECT "pet".* \ + FROM "pet" \ + JOIN "child" "child1" ON ("child1"."rowid" = "pet"."childId") AND (1) \ + JOIN "toy" ON "toy"."childId" = "child1"."rowid" \ + JOIN "child" "child2" ON ("child2"."rowid" = "pet"."childId") AND (1 + 1) AND ("child2"."parentId" = 1) + """) + } + } + } } diff --git a/Tests/GRDBTests/AssociationPrefetchingSQLTests.swift b/Tests/GRDBTests/AssociationPrefetchingSQLTests.swift index 8d4d38f16e..091af750d3 100644 --- a/Tests/GRDBTests/AssociationPrefetchingSQLTests.swift +++ b/Tests/GRDBTests/AssociationPrefetchingSQLTests.swift @@ -1537,4 +1537,39 @@ class AssociationPrefetchingSQLTests: GRDBTestCase { } } } + + func testAssociationFilteredByOtherAssociation() throws { + let dbQueue = try makeDatabaseQueue() + try dbQueue.write { db in + // Plain request + do { + let request = A + .including(all: A + .hasMany( + D.self, + through: A.hasMany(C.self) + .joining(required: C.belongsTo(A.self).filter(sql: "1")), + using: C.hasMany(D.self)) + .orderByPrimaryKey()) + .filter(sql: "1 + 1") + .orderByPrimaryKey() + + sqlQueries.removeAll() + _ = try Row.fetchAll(db, request) + + let selectQueries = sqlQueries.filter { $0.contains("SELECT") && !$0.contains("sqlite_") } + XCTAssertEqual(selectQueries, [ + """ + SELECT * FROM "a" WHERE 1 + 1 ORDER BY "cola1" + """, + """ + SELECT "d".*, "c"."colc2" AS "grdb_colc2" \ + FROM "d" \ + JOIN "c" ON ("c"."colc1" = "d"."cold2") AND ("c"."colc2" IN (1, 2, 3)) \ + JOIN "a" ON ("a"."cola1" = "c"."colc2") AND (1) \ + ORDER BY "d"."cold1" + """]) + } + } + } }