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.

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

Adds support for datetime string formats #42

Merged
merged 1 commit into from
Mar 13, 2023

Conversation

NeedleInAJayStack
Copy link
Contributor

Previously, dates were only interpreted from Double or Int SQLite representations, which caused issues with decoding values of CURRENT_DATE and CURRENT_TIMESTAMP, which output formatted strings. This PR adds support to interpret dates from strings, matching the formats output by the date() and datetime() SQLite functions, which includes the CURRENT_DATE and CURRENT_TIMESTAMP constants.

This correctly interprets dates generated the `date` and `datetime` sqlite functions, which includes the `CURRENT_DATE` and `CURRENT_TIMESTAMP` constants.
@0xTim 0xTim requested a review from gwynne March 12, 2023 12:02
@codecov-commenter
Copy link

Codecov Report

Merging #42 (56e7c67) into main (0c5014e) will increase coverage by 2.35%.
The diff coverage is 96.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #42      +/-   ##
==========================================
+ Coverage   45.68%   48.03%   +2.35%     
==========================================
  Files           7        7              
  Lines         510      535      +25     
==========================================
+ Hits          233      257      +24     
- Misses        277      278       +1     
Impacted Files Coverage Δ
Sources/SQLiteNIO/SQLiteDataConvertible.swift 38.05% <96.00%> (+16.46%) ⬆️

@@ -123,6 +123,12 @@ extension Date: SQLiteDataConvertible {
value = v
case .integer(let v):
value = Double(v)
case .text(let v):
guard let d = dateTimeFormatter.date(from: v) ?? dateFormatter.date(from: v) else {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not a huge fan of the "try one and then the other" approach this requires, but I can't think of a good alternative either.

@NeedleInAJayStack
Copy link
Contributor Author

Looks like the CI failed on a ThreadSanitizer warning in FluentBenchmark.testPerformance_siblings on the final iteration of the PersonSeed migration:

INSERT INTO "people" ("first_name", "last_name", "id") VALUES (?1, ?2, ?3) ["Foo #600", "Bar", "EAD5F1B5-561F-475C-A3B8-9AC0BB270E6C"]

I'm not sure how my changes would impact that - @gwynne do you have any insights?

@NeedleInAJayStack
Copy link
Contributor Author

Nevermind, sorry - it looks like that issue is occurring in other PRs as well.

@gwynne
Copy link
Member

gwynne commented Mar 12, 2023

Yeah, that's a known threading bug in SQLite itself, I've been going back and forth on whether to suppress it or patch SQLite to "fix" it

@gwynne
Copy link
Member

gwynne commented Mar 13, 2023

Deliberately merging without a semver- label in order to release all three pending PRs together.

@gwynne gwynne merged commit bb2f3cb into vapor:main Mar 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants