Grouped SQL Filters (#104)

* Wrap filters in groups in SQLQueryConverter

* Made test Galexy model soft-deletable

* Store 2 models in FluentBenchmarker.testUpdate test case

* Import Foundation.Date to Galaxy.swift file

* Added 'deleted_at' field to GalaxyMigration database schema

* Fix expected count of keys in FluentBenchmarker.testParentSerialization test case

* Remove top-level parentheses from SQL query filters

* Revert changes to Galaxy test model and FluentBenchmarker.testUpdate test case

* Use Trash model to test soft-delete model behaviour

* Temporarily remove filter grouping to ensure tests fail

* Use equality operator instead of prefix check in FluentBenchmark.testUpdate

* Revert all changes in SQLQueryConverter.filters(_:) method

* Ensure all contents of Trash instances are the same before update in FluentBecnhmarker.testUpdate test case

* Changed Trash.id property type to UUID

* Call .create and .update for FluentBenchmarker.testUpdate Trash models instead of .save

* Defined Trash.id as non-auto in TrashMigration

* Create new Trach model to run update

* Require Trash 'id' values to be unique

* Set .deletedAt properties for Trash models in FluentBenchmark.testUpdate

* Group nested predicate groups for SQL queries

* Use 'UNIQUE' custom constraint on 'id' field in TrashMigration

* Don't set 'deleted_at' field when updating Trash model in FluentBenchmark.testUpdate test case

* fix tests

* rm extra changes

Sources/FluentBenchmark/FluentBenchmarker.swift

@@ -46,6 +46,7 @@ public final class FluentBenchmarker {
         try self.testFieldFilter()
         try self.testJoinedFieldFilter()
         try self.testSameChildrenFromKey()
+        try self.testSoftDeleteWithQuery()
     }
 
     public func testCreate() throws {
@@ -713,61 +714,27 @@ public final class FluentBenchmarker {
     }
 
     public func testSoftDelete() throws {
-        final class SoftDeleteUser: Model {
-            static let schema = "sd_users"
-
-            @ID(key: "id")
-            var id: Int?
-
-            @Field(key: "name")
-            var name: String
-
-            @Timestamp(key: "deleted_at", on: .delete)
-            var deletedAt: Date?
-
-            init() { }
-            init(id: Int? = nil, name: String) {
-                self.id = id
-                self.name = name
-            }
-        }
-
-        struct SDUserMigration: Migration {
-            func prepare(on database: Database) -> EventLoopFuture<Void> {
-                return database.schema("sd_users")
-                    .field("id", .int, .identifier(auto: true))
-                    .field("name", .string, .required)
-                    .field("deleted_at", .datetime)
-                    .create()
-            }
-
-            func revert(on database: Database) -> EventLoopFuture<Void> {
-                return database.schema("sd_users").delete()
-            }
-        }
-
-
         func testCounts(allCount: Int, realCount: Int) throws {
-            let all = try SoftDeleteUser.query(on: self.database).all().wait()
+            let all = try Trash.query(on: self.database).all().wait()
             guard all.count == allCount else {
                 throw Failure("all count should be \(allCount)")
             }
-            let real = try SoftDeleteUser.query(on: self.database).withDeleted().all().wait()
+            let real = try Trash.query(on: self.database).withDeleted().all().wait()
             guard real.count == realCount else {
                 throw Failure("real count should be \(realCount)")
             }
         }
 
         try runTest(#function, [
-            SDUserMigration(),
+            TrashMigration(),
         ]) {
             // save two users
-            try SoftDeleteUser(name: "A").save(on: self.database).wait()
-            try SoftDeleteUser(name: "B").save(on: self.database).wait()
+            try Trash(contents: "A").save(on: self.database).wait()
+            try Trash(contents: "B").save(on: self.database).wait()
             try testCounts(allCount: 2, realCount: 2)
 
             // soft-delete a user
-            let a = try SoftDeleteUser.query(on: self.database).filter(\.$name == "A").first().wait()!
+            let a = try Trash.query(on: self.database).filter(\.$contents == "A").first().wait()!
             try a.delete(on: self.database).wait()
             try testCounts(allCount: 1, realCount: 2)
 
@@ -1557,6 +1524,30 @@ public final class FluentBenchmarker {
         }
     }
 
+    public func testSoftDeleteWithQuery() throws {
+        try runTest(#function, [
+            TrashMigration()
+        ]) {
+            // a is scheduled for soft-deletion
+            let a = Trash(contents: "a")
+            a.deletedAt = Date(timeIntervalSinceNow: 50)
+            try a.create(on: self.database).wait()
+
+            // b is not soft-deleted
+            let b = Trash(contents: "b")
+            try b.create(on: self.database).wait()
+
+            // select for non-existing c, expect 0
+            // without proper query serialization this may
+            // return a. see:
+            // https://github.com/vapor/fluent-kit/pull/104
+            let trash = try Trash.query(on: self.database)
+                .filter(\.$contents == "c")
+                .all().wait()
+            XCTAssertEqual(trash.count, 0)
+        }
+    }
+
     // MARK: Utilities
 
     struct Failure: Error {

Sources/FluentBenchmark/Trash.swift

@@ -0,0 +1,40 @@
+import Foundation
+import FluentKit
+
+final class Trash: Model {
+    static let schema = "trash"
+
+    @ID(key: "id")
+    var id: UUID?
+
+    @Field(key: "contents")
+    var contents: String
+
+    @Timestamp(key: "deleted_at", on: .delete)
+    var deletedAt: Date?
+
+    init() { }
+
+    init(id: UUID? = nil, contents: String, deletedAt: Date? = nil) {
+        if let id = id {
+            self.id = id
+            self._id.exists = true
+        }
+        self.contents = contents
+        self.deletedAt = deletedAt
+    }
+}
+
+struct TrashMigration: Migration {
+    func prepare(on database: Database) -> EventLoopFuture<Void> {
+        return database.schema(Trash.schema)
+            .field("id", .uuid, .identifier(auto: false), .custom("UNIQUE"))
+            .field("contents", .string, .required)
+            .field("deleted_at", .datetime)
+            .create()
+    }
+
+    func revert(on database: Database) -> EventLoopFuture<Void> {
+        return database.schema(Trash.schema).delete()
+    }
+}

Sources/FluentSQL/SQLQueryConverter.swift

@@ -77,7 +77,7 @@ public struct SQLQueryConverter {
         guard !filters.isEmpty else {
             return nil
         }
-        
+
         return SQLList(
             items: filters.map(self.filter),
             separator: SQLBinaryOperator.and
@@ -237,7 +237,13 @@ public struct SQLQueryConverter {
         case .custom(let any):
             return custom(any)
         case .group(let filters, let relation):
-            return SQLList(items: filters.map(self.filter), separator: self.relation(relation))
+            // <item> OR <item> OR <item>
+            let expression = SQLList(
+                items: filters.map(self.filter),
+                separator: self.relation(relation)
+            )
+            // ( <expr> )
+            return SQLGroupExpression(expression)
         }
     }