Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix insertMany([Encodable]) failing due to asymmetric setters #1138

Merged
merged 3 commits into from
Jul 2, 2022
Merged

Fix insertMany([Encodable]) failing due to asymmetric setters #1138

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ba5165a
fix insertMany() failing with encodables which have assymmetric optio…
geoffmacd Jul 1, 2022
c4bdad8
lint
geoffmacd Jul 1, 2022
abff512
feedback
geoffmacd Jul 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
88 changes: 82 additions & 6 deletions Sources/SQLite/Typed/Coding.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,12 +86,22 @@ extension QueryType {
/// - Returns: An `INSERT` statement for the encodable objects
public func insertMany(_ encodables: [Encodable], userInfo: [CodingUserInfoKey: Any] = [:],
otherSetters: [Setter] = []) throws -> Insert {
let combinedSetters = try encodables.map { encodable -> [Setter] in
let encoder = SQLiteEncoder(userInfo: userInfo)
let combinedSettersWithoutNils = try encodables.map { encodable -> [Setter] in
let encoder = SQLiteEncoder(userInfo: userInfo, forcingNilValueSetters: false)
try encodable.encode(to: encoder)
return encoder.setters + otherSetters
}
return self.insertMany(combinedSetters)
// requires the same number of setters per encodable
guard Set(combinedSettersWithoutNils.map(\.count)).count == 1 else {
// asymmetric sets of value insertions (some nil, some not), requires NULL value to satisfy INSERT query
let combinedSymmetricSetters = try encodables.map { encodable -> [Setter] in
let encoder = SQLiteEncoder(userInfo: userInfo, forcingNilValueSetters: true)
try encodable.encode(to: encoder)
return encoder.setters + otherSetters
}
return self.insertMany(combinedSymmetricSetters)
}
return self.insertMany(combinedSettersWithoutNils)
}

/// Creates an `INSERT ON CONFLICT DO UPDATE` statement, aka upsert, by encoding the given object
Expand Down Expand Up @@ -165,9 +175,11 @@ private class SQLiteEncoder: Encoder {

let encoder: SQLiteEncoder
let codingPath: [CodingKey] = []
let forcingNilValueSetters: Bool

init(encoder: SQLiteEncoder) {
init(encoder: SQLiteEncoder, forcingNilValueSetters: Bool = false) {
self.encoder = encoder
self.forcingNilValueSetters = forcingNilValueSetters
}

func superEncoder() -> Swift.Encoder {
Expand Down Expand Up @@ -202,6 +214,46 @@ private class SQLiteEncoder: Encoder {
encoder.setters.append(Expression(key.stringValue) <- value)
}

func encodeIfPresent(_ value: Int?, forKey key: SQLiteEncoder.SQLiteKeyedEncodingContainer<Key>.Key) throws {
if let value = value {
encoder.setters.append(Expression(key.stringValue) <- value)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't this just delegate to encode like in the default protocol implementation?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, this would be better, updating that

} else if forcingNilValueSetters {
encoder.setters.append(Expression<Int?>(key.stringValue) <- nil)
}
}

func encodeIfPresent(_ value: Bool?, forKey key: Key) throws {
if let value = value {
encoder.setters.append(Expression(key.stringValue) <- value)
} else if forcingNilValueSetters {
encoder.setters.append(Expression<Bool?>(key.stringValue) <- nil)
}
}

func encodeIfPresent(_ value: Float?, forKey key: Key) throws {
if let value = value {
encoder.setters.append(Expression(key.stringValue) <- Double(value))
} else if forcingNilValueSetters {
encoder.setters.append(Expression<Double?>(key.stringValue) <- nil)
}
}

func encodeIfPresent(_ value: Double?, forKey key: Key) throws {
if let value = value {
encoder.setters.append(Expression(key.stringValue) <- value)
} else if forcingNilValueSetters {
encoder.setters.append(Expression<Double?>(key.stringValue) <- nil)
}
}

func encodeIfPresent(_ value: String?, forKey key: MyKey) throws {
if let value = value {
encoder.setters.append(Expression(key.stringValue) <- value)
} else if forcingNilValueSetters {
encoder.setters.append(Expression<String?>(key.stringValue) <- nil)
}
}

Copy link
Collaborator

@jberkel jberkel Jul 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

were all these previously automatically provided by the protocol?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, I believe so. although this is not very clear from the docs. this behavior forces the setter to be NULL in the case where forcingNilValueSetters. There isn't another way it seems

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

defined here: https://github.com/apple/swift/blob/main/stdlib/public/core/Codable.swift#L5752

it's a shame these have all to be overridden, but I guess there's no cleaner solution

func encode<T>(_ value: T, forKey key: Key) throws where T: Swift.Encodable {
switch value {
case let data as Data:
Expand All @@ -217,6 +269,28 @@ private class SQLiteEncoder: Encoder {
}
}

func encodeIfPresent<T>(_ value: T?, forKey key: Key) throws where T: Swift.Encodable {
guard let value = value else {
guard forcingNilValueSetters else {
return
}
encoder.setters.append(Expression<String?>(key.stringValue) <- nil)
return
}
switch value {
case let data as Data:
encoder.setters.append(Expression(key.stringValue) <- data)
case let date as Date:
encoder.setters.append(Expression(key.stringValue) <- date.datatypeValue)
case let uuid as UUID:
encoder.setters.append(Expression(key.stringValue) <- uuid.datatypeValue)
default:
let encoded = try JSONEncoder().encode(value)
let string = String(data: encoded, encoding: .utf8)
encoder.setters.append(Expression(key.stringValue) <- string)
}
}

func encode(_ value: Int8, forKey key: Key) throws {
throw EncodingError.invalidValue(value, EncodingError.Context(codingPath: codingPath,
debugDescription: "encoding an Int8 is not supported"))
Expand Down Expand Up @@ -274,9 +348,11 @@ private class SQLiteEncoder: Encoder {
fileprivate var setters: [Setter] = []
let codingPath: [CodingKey] = []
let userInfo: [CodingUserInfoKey: Any]
let forcingNilValueSetters: Bool

init(userInfo: [CodingUserInfoKey: Any]) {
init(userInfo: [CodingUserInfoKey: Any], forcingNilValueSetters: Bool = false) {
self.userInfo = userInfo
self.forcingNilValueSetters = forcingNilValueSetters
}

func singleValueContainer() -> SingleValueEncodingContainer {
Expand All @@ -288,7 +364,7 @@ private class SQLiteEncoder: Encoder {
}

func container<Key>(keyedBy type: Key.Type) -> KeyedEncodingContainer<Key> where Key: CodingKey {
KeyedEncodingContainer(SQLiteKeyedEncodingContainer(encoder: self))
KeyedEncodingContainer(SQLiteKeyedEncodingContainer(encoder: self, forcingNilValueSetters: forcingNilValueSetters))
}
}

Expand Down
22 changes: 22 additions & 0 deletions Tests/SQLiteTests/QueryIntegrationTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,28 @@ class QueryIntegrationTests: SQLiteTestCase {
XCTAssertEqual(2, id)
}

func test_insert_many_encodables() throws {
let table = Table("codable")
try db.run(table.create { builder in
builder.column(Expression<Int?>("int"))
builder.column(Expression<String?>("string"))
builder.column(Expression<Bool?>("bool"))
builder.column(Expression<Double?>("float"))
builder.column(Expression<Double?>("double"))
builder.column(Expression<Date?>("date"))
builder.column(Expression<UUID?>("uuid"))
})

let value1 = TestOptionalCodable(int: 5, string: "6", bool: true, float: 7, double: 8,
date: Date(timeIntervalSince1970: 5000), uuid: testUUIDValue)
let valueWithNils = TestOptionalCodable(int: nil, string: nil, bool: nil, float: nil, double: nil, date: nil, uuid: nil)
try db.run(table.insertMany([value1, valueWithNils]))

let rows = try db.prepare(table)
let values: [TestOptionalCodable] = try rows.map({ try $0.decode() })
XCTAssertEqual(values.count, 2)
}

func test_upsert() throws {
try XCTSkipUnless(db.satisfiesMinimumVersion(minor: 24))
let fetchAge = { () throws -> Int? in
Expand Down
12 changes: 6 additions & 6 deletions Tests/SQLiteTests/QueryTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -365,21 +365,21 @@ class QueryTests: XCTestCase {
)
}

func test_insert_many_encodable() throws {
func test_insert_many_encodables() throws {
let emails = Table("emails")
let value1 = TestCodable(int: 1, string: "2", bool: true, float: 3, double: 4,
date: Date(timeIntervalSince1970: 0), uuid: testUUIDValue, optional: nil, sub: nil)
let value2 = TestCodable(int: 2, string: "3", bool: true, float: 3, double: 5,
date: Date(timeIntervalSince1970: 0), uuid: testUUIDValue, optional: nil, sub: nil)
date: Date(timeIntervalSince1970: 0), uuid: testUUIDValue, optional: "optional", sub: nil)
let value3 = TestCodable(int: 3, string: "4", bool: true, float: 3, double: 6,
date: Date(timeIntervalSince1970: 0), uuid: testUUIDValue, optional: nil, sub: nil)
let insert = try emails.insertMany([value1, value2, value3])
assertSQL(
"""
INSERT INTO \"emails\" (\"int\", \"string\", \"bool\", \"float\", \"double\", \"date\", \"uuid\")
VALUES (1, '2', 1, 3.0, 4.0, '1970-01-01T00:00:00.000', 'E621E1F8-C36C-495A-93FC-0C247A3E6E5F'),
(2, '3', 1, 3.0, 5.0, '1970-01-01T00:00:00.000', 'E621E1F8-C36C-495A-93FC-0C247A3E6E5F'),
(3, '4', 1, 3.0, 6.0, '1970-01-01T00:00:00.000', 'E621E1F8-C36C-495A-93FC-0C247A3E6E5F')
INSERT INTO \"emails\" (\"int\", \"string\", \"bool\", \"float\", \"double\", \"date\", \"uuid\", \"optional\", \"sub\")
VALUES (1, '2', 1, 3.0, 4.0, '1970-01-01T00:00:00.000', 'E621E1F8-C36C-495A-93FC-0C247A3E6E5F', NULL, NULL),
(2, '3', 1, 3.0, 5.0, '1970-01-01T00:00:00.000', 'E621E1F8-C36C-495A-93FC-0C247A3E6E5F', 'optional', NULL),
(3, '4', 1, 3.0, 6.0, '1970-01-01T00:00:00.000', 'E621E1F8-C36C-495A-93FC-0C247A3E6E5F', NULL, NULL)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the new codable decode behavior adds these NULLs correcting the issue

""".replacingOccurrences(of: "\n", with: ""),
insert
)
Expand Down
20 changes: 20 additions & 0 deletions Tests/SQLiteTests/TestHelpers.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,3 +145,23 @@ class TestCodable: Codable, Equatable {
lhs.sub == rhs.sub
}
}

struct TestOptionalCodable: Codable, Equatable {
let int: Int?
let string: String?
let bool: Bool?
let float: Float?
let double: Double?
let date: Date?
let uuid: UUID?

init(int: Int?, string: String?, bool: Bool?, float: Float?, double: Double?, date: Date?, uuid: UUID?) {
self.int = int
self.string = string
self.bool = bool
self.float = float
self.double = double
self.date = date
self.uuid = uuid
}
}