-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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.
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
Fix insertMany([Encodable]) failing due to asymmetric setters #1138
Fix insertMany([Encodable]) failing due to asymmetric setters #1138
Conversation
encoder.setters.append(Expression<String?>(key.stringValue) <- nil) | ||
} | ||
} | ||
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
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) |
There was a problem hiding this comment.
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
Sources/SQLite/Typed/Coding.swift
Outdated
@@ -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) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
Would it be possible to cut a new release with this change? |
Fixes #1130, original PR (by me) was #1048
The problem was if you are trying to INSERT many rows, and some of those rows have different quantity of setters due to optional values, the INSERT query will fail with
all VALUES must have the same number of terms in "INSERT INTO...
We needed to either guarantee that each
Encodable
will have the same number of setters OR force equivalent numbers of setters by adding NULL where the optional value is nil. This can be done with theencodeIfPresent
methods onKeyedEncodingContainerProtocol
inSQLiteKeyedEncodingContainer