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

Handle JSON better #107

Merged
merged 9 commits into from
Jan 18, 2024
Merged

Handle JSON better #107

merged 9 commits into from
Jan 18, 2024

Conversation

gwynne
Copy link
Member

@gwynne gwynne commented Jan 18, 2024

These changes are now available in 4.4.2

This fixes an issue that arose with the release of SQLite 3.45.0, which includes support for a new "JSONB" internal representation. As a side effect, textual JSON data presented to SQLite as a BLOB is incorrectly treated as JSONB by the database, resulting in inexplicable errors when attempts are made to read the JSON back out again. Since we should always have been sending JSON to the database as TEXT in the first place, this is considered a general bugfix rather than purely a compatibility update.

Unblocks vapor/sqlite-nio#62.

…th SQLite 3.45.0 and above, due to the new JSONB support changing the interpretation of JSON data in BLOB form (and is what should've been happening all along regardless).
@gwynne gwynne added bug Something isn't working semver-patch Internal changes only labels Jan 18, 2024
@gwynne gwynne requested review from 0xTim, MahdiBM and ptoffy January 18, 2024 06:54
@codecov-commenter
Copy link

codecov-commenter commented Jan 18, 2024

Codecov Report

Attention: Patch coverage is 66.12903% with 21 lines in your changes missing coverage. Please review.

Project coverage is 83.56%. Comparing base (a5146ba) to head (5734414).
Report is 6 commits behind head on main.

Files Patch % Lines
Sources/SQLiteKit/SQLiteDataEncoder.swift 47.05% 18 Missing ⚠️
Sources/SQLiteKit/SQLiteDataDecoder.swift 86.36% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #107      +/-   ##
==========================================
+ Coverage   83.33%   83.56%   +0.22%     
==========================================
  Files           7        7              
  Lines         222      219       -3     
==========================================
- Hits          185      183       -2     
+ Misses         37       36       -1     
Files Coverage Δ
Sources/SQLiteKit/SQLiteConfiguration.swift 100.00% <ø> (ø)
Sources/SQLiteKit/SQLiteConnection+SQLKit.swift 85.71% <100.00%> (ø)
Sources/SQLiteKit/SQLiteConnectionSource.swift 97.22% <ø> (ø)
Sources/SQLiteKit/SQLiteDataDecoder.swift 90.62% <86.36%> (-0.29%) ⬇️
Sources/SQLiteKit/SQLiteDataEncoder.swift 58.33% <47.05%> (+0.33%) ⬆️

Copy link
Member

@0xTim 0xTim left a comment

Choose a reason for hiding this comment

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

Honestly surprised there wasn't more needed from turning on Sendable checking!

@@ -82,7 +82,7 @@ internal struct _SQLiteDatabaseVersion: SQLDatabaseReportedVersion {
}

private struct _SQLiteSQLDatabase: SQLDatabase {
let database: SQLiteDatabase
let database: any SQLiteDatabase
Copy link

Choose a reason for hiding this comment

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

FYI they're dropping the ExistentialAny upcoming feature. I personally liked it. Not sure what exactly are their plans.

Copy link
Member Author

Choose a reason for hiding this comment

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

Citation or it didn't happen 😜 What an absurd thing to do...

@gwynne
Copy link
Member Author

gwynne commented Jan 18, 2024

Honestly surprised there wasn't more needed from turning on Sendable checking!

@0xTim Well, it's not a fair test, at the moment - once SQLKit gets its Sendable going (I've got most of it done in my local clone already), there'll be more warnings to solve in all the downstream dependencies (basically everything except async-kit and the three -nios), even if they were "done" before.

@gwynne gwynne merged commit e9fd69b into main Jan 18, 2024
13 checks passed
@gwynne gwynne deleted the handle-json-better branch January 18, 2024 12:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working semver-patch Internal changes only
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants