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

Add Coding support #733

Merged
merged 6 commits into from
Sep 28, 2017
Merged

Add Coding support #733

merged 6 commits into from
Sep 28, 2017

Conversation

drewag
Copy link

@drewag drewag commented Sep 25, 2017

  • Insert or update from an Encodable type
  • Decode a row into a Decodable type

Closes #727

- Insert or update from an Encodable type
- Decode a row into a Decodable type

Closes #727
@jberkel jberkel mentioned this pull request Sep 25, 2017
@jberkel
Copy link
Collaborator

jberkel commented Sep 26, 2017

/Users/travis/build/stephencelis/SQLite.swift/Tests/SQLiteTests/QueryTests.swift:291: error: -[SQLite_swift_Unit_Tests.QueryTests test_update_encodable_with_nested_encodable] : 

XCTAssertEqual failed: 

("UPDATE "emails" SET "int" = 1, "string" = '2', "bool" = 1, "float" = 3.0, "double" = 4.0, "sub" = x'7b22626f6f6c223a747275652c22737472696e67223a2232222c22666c6f6174223a332c22696e74223a312c22646f75626c65223a347d'")

is not equal to 

("UPDATE "emails" SET "int" = 1, "string" = '2', "bool" = 1, "float" = 3.0, "double" = 4.0, "sub" = x'7b22626f6f6c223a747275652c22666c6f6174223a332c22646f75626c65223a342c22696e74223a312c22737472696e67223a2232227d'") 

@drewag
Copy link
Author

drewag commented Sep 26, 2017

How do I run this configuration to reproduce the error? (preferably in Xcode)

@jberkel
Copy link
Collaborator

jberkel commented Sep 26, 2017

$ cd Tests/CocoaPods
$ make test VALIDATOR_SUBSPEC="standard" 
[...]
> Pod installation complete! There are 2 dependencies from the Podfile and 1 total pod installed.
   Building with xcodebuild.
xcodebuild clean build 
-workspace /some/path/SQLite.swift/App.xcworkspace 
-scheme App -configuration Release 
CODE_SIGN_IDENTITY=- -sdk iphonesimulator
 -destination id=C1943A91-1C60-4111-8918-23D1C0D3186B
[...]
[CTRL+C]
Interrupted. Exiting...
$

open App.xcworkspace in Xcode, select SQLite.swift-Unit Tests scheme, ⌘U

Runs ok on my machine though.

@drewag
Copy link
Author

drewag commented Sep 26, 2017

Ok ya, the problem is that hex string for the encoded JSON. The JSON is sometimes coming out in a different order. I might have to pull the hex string out of the SQL, convert it back into a JSON object, and validate the object. Any thoughts on an easier option?

@jberkel
Copy link
Collaborator

jberkel commented Sep 26, 2017

maybe JSONEncoder.OutputFormatting.sortedKeys?

Why is the JSON stored in the DB as hex? wouldn't it be easier to just store the data as string?

@drewag
Copy link
Author

drewag commented Sep 26, 2017

sortedKeys is only available on High Sierra. I think I am just going generate the desired JSON in the test directly from the object.

I don't have a strong reason for storing it as data except it would be an extra conversion before inserting it (the JSONEncoder exports data). Do you think I should store it as a string? (not sure if there is any space/performance implications)

@jberkel
Copy link
Collaborator

jberkel commented Sep 26, 2017

Ah, is this just the JSON UTF-8 data? I assumed it was base64 or similar. Sqlite has good JSON support so it wouldn't make sense to encode it in another format.

@jberkel
Copy link
Collaborator

jberkel commented Sep 26, 2017

Actually, compatibility is another point I wanted to raise. The deployment target is 8.0 so this code would need to get put behind @availability?

@drewag
Copy link
Author

drewag commented Sep 26, 2017

no just the sortedKeys has limited availability. The existing implementation is fully supported on 8.0.

Yes the that is just the UTF-8 data as a hex string. It is the result of using an Expression for the insert. How do I take advantage of the JSON support you are referring to?

@drewag
Copy link
Author

drewag commented Sep 26, 2017

FYI, I now have a build that I believe will pass all of the CI. I am just waiting to see if we have a better way to represent any nested objects in the database. I am even happy to explore other options than JSON.

@jberkel
Copy link
Collaborator

jberkel commented Sep 26, 2017

I don't think stock SQLite has it enabled, but when using CocoaPods you can specify the build flags:

pod 'SQLite.swift/standalone'
pod 'sqlite3/json'

@drewag
Copy link
Author

drewag commented Sep 26, 2017

Won't that break manual installation? Either way, it looks like the JSON extension still just stores the JSON as regular text:

https://sqlite.org/json1.html
The json1 extension (currently) stores JSON as ordinary text.

So it doesn't seem like it would help in this situation at all because the code does not have to manipulate the JSON at all. However, if someone wants to insert it using Coding but then manipulate it later it seems best to store it as plain text. So unless you don't agree, I will update it to store as a normal string.

@jberkel
Copy link
Collaborator

jberkel commented Sep 26, 2017

yes, it won't help for storage but for querying. It shouldn't make a difference in SQLite if you store the data as UTF-8 hex or string. Hex strings have some overhead in pure transmission size though.

@jberkel
Copy link
Collaborator

jberkel commented Sep 26, 2017

To keep Query.swift more manageable, can you move the new coding/decoding relating code into a separate file? Other than that it needs a changelog entry and maybe a brief mention in Documentation/Index.md.

}

fileprivate class SQLiteDecoder : Decoder {
struct DecodingError : Error, CustomStringConvertible {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Couldn't the built-in DecodingError type be used? Some of the "not implemented errors" can probably just use fatalError since they indicate library misuse.

Copy link
Author

Choose a reason for hiding this comment

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

Yup, I didn't realize those existed. But I am leaving them as thrown errors instead of fatal errors because I would rather that be caught than bring down the whole program.

@@ -72,7 +73,7 @@

[Carthage][] is a simple, decentralized dependency manager for Cocoa. To
install SQLite.swift with Carthage:

## Custom Types
Copy link
Collaborator

Choose a reason for hiding this comment

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

misplaced heading?


/// Generates a list of settings for an Encodable object
fileprivate class SQLiteEncoder: Encoder {
struct EncodingError: Error, CustomStringConvertible {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same for EncodingError. Reuse of built-in?

@jberkel jberkel merged commit 1826097 into stephencelis:release Sep 28, 2017
@jberkel
Copy link
Collaborator

jberkel commented Sep 28, 2017

thanks!

@drewag
Copy link
Author

drewag commented Sep 28, 2017

You're welcome, thanks for the great library!

@groue
Copy link

groue commented Sep 28, 2017

Hello,

I don't quite get this test: https://github.com/stephencelis/SQLite.swift/pull/733/files#diff-7e8883d8ae5007060a25949445db3611R283

func test_update_encodable() throws {
    let emails = Table("emails")
    let value = TestCodable(int: 1, string: "2", bool: true, float: 3, double: 4, optional: nil, sub: nil)
    let update = try emails.update(value)
    AssertSQL(
        "UPDATE \"emails\" SET \"int\" = 1, \"string\" = '2', \"bool\" = 1, \"float\" = 3.0, \"double\" = 4.0",
        update
    )
}

Does it mean that using the update method on an instance of a Codable type updates all table rows?

If this is true, then maybe the documentation should be updated so that users understand that they shouldn't write code like below:

let players = Table("players")

// OK: insert a player
var player = Player(id: 1, name: "arthur", score: 1000)
try db.run(players.insert(player))

// Oops: does not update the expected row
player.score = 2000
try db.run(players.update(player))

@drewag
Copy link
Author

drewag commented Sep 28, 2017

@groue your example would indeed update the intended rows because it updates all rows. But I do see your point and I did have concerns about this being seen too much like an ORM.

@groue
Copy link

groue commented Sep 28, 2017

ORM

That's a quick and easy derivation from Codable, indeed. Maybe even an expectation. After all, when you encode a bunch of codable values in json or plist, every one of them has its own and unique place in the serialized format.

What would be the canonical sample code, then?

  • Hypothesis 1: don't encode primary key in the Codable type, in order to flush out all similarities with ORM:

    // Table has id, name & score columns
    let players = Table("players")
    
    // Codable type without identity
    struct PlayerAttributes: Codable {
        var name: String
        var score: Int
    }
    
    // OK: insert a player
    var player = PlayerAttributes(name: "arthur", score: 1000)
    let playerId = try db.run(players.insert(player))
    
    // OK: update the expected player
    player.score = 2000
    try db.run(players.filter(id == playerId).update(player))
  • Hypothesis 2: encode the primary key in the Codable type:

    // Table has id, name & score columns
    let players = Table("players")
    
    // Codable type with identity
    struct Player: Codable {
        var id: Int64?
        var name: String
        var score: Int
    }
    
    // OK: insert a player
    var player = Player(id: nil, name: "arthur", score: 1000)
    player.id = try db.run(players.insert(player))
    
    // OK: update the expected player
    player.score = 2000
    try db.run(players.filter(id == player.id).update(player))

Any way, it would greatly help if the documentation would clarify this topic, I think :-)

jberkel added a commit that referenced this pull request Sep 29, 2017
@newlix
Copy link

newlix commented Apr 14, 2018

Is there any reason the Int64, Int32 ... types can't be supported?

@ReDetection ReDetection mentioned this pull request Jul 25, 2018
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.

4 participants