Codable records: JSON encoding and decoding of codable record properties #397

gusrota · 2018-08-06T14:43:55Z

Hi Gwendal

Thanks for all of your recent help. I work with @valexa and we have been working on making the changes which include all of the feedback you have given including using the do { try natural decoding } catch { try JSON decoding } pattern. Please find this in the new pull request. Below I shall include sone extra details of this including answers to some questions you had asked.

Why do you read data here, and save a string below?

This code is now in a do { try natural decoding } catch { try JSON decoding }. Data is read to then be used by JSONDecoder. Data is also read as dataString which is used for a very simple / (hopefully) fast way of determining if data does contain possible JSON and if we should spend more recourses into trying to decode it.

Why isn't this code also present in decode(_: forKey:)? It looks like an oversight. Caring about your use case (decoding an optional Array) is legit, but I'd like that the library remains as consistent and welcoming as it is today. This means thinking about other people as well, including those who, for example, decode a non-optional Array.

Yes you are correct, this code should also be here, have now added. I have added tests for decoding non optional Array.

What about the JSONSerialization.isValidJSONObject performance hit? Is it really useful?
I'm not sure JSONSerialization.isValidJSONObject([dataString]) really checks what you expect it to check. To me it looks like it always return true.

Good point thanks! Have changed this to perform a very simple test if data may contain JSON. If it looks like it may then it tries to decode.

That if prevents types from providing customized decoding of JSON data. I'd like to read an exploration of the potential harm, if any.

As our nested model to JSON code is now at the end of the queue I believe this wont prevent others providing custom decoding of JSON, please say if you feel this is not the case.

Other points:

We have added tests for Persistable Record and Fetchable Record, I have tried to add them into the correct places but happy for you to move / suggest other location for them to reside.
Both PersistableRecord+Encoding and FetchableRecord+Decodable include our code in the do { try natural decoding } catch { try JSON decoding } pattern you suggested. But for PersistableRecord+Encoding I had to add ThrowingKeyedContainer and ThrowingUnkeyedContainer class to make KeyedContainer and UnkeyedContainer throwing so that the try value.encode(to: PersistableRecordEncoder(codingPath: [key], encode: encode)) could be cached. As before fatalError("unkeyed encoding is not supported”) was used.
inFetchableRecord+Decodable if JSONDecoder fails it's error is tricked down to FetchableRecord init. How do you feel about this or would you prefer for it to be cached just after the JSONDecoder code?
I am happy to add to README this nested model as JSON if you wish, though would like your advice on where about to place this. Or let me know if you prefer a dedicated guide?

One more thing…

This pull request is submitted against the develop branch.
Inline documentation has been updated.
README.md or another dedicated guide has been updated.
Changes are tested, for all flavors of GRDB, GRDBCipher, and GRDBCustomSQLite.
Updated CHANGELOG.md

groue · 2018-08-06T15:41:14Z

That's lovely @gusrota! @valexa is in good company :-)

Please give me a couple of days to have a look.

groue · 2018-08-06T15:49:42Z

Meanwhile, maybe you can have a look at the reason why some Travis tests are failing?

groue · 2018-08-06T16:40:40Z

I didn't look precisely at the pull request, but I'm sure we'll eventually merge it: be confident we will proceed to JSON decoding.

So please let me have a suggestion on String encoding/Data decoding. You wrote:

Why do you read data here, and save a string below?

[...] Data is read to then be used by JSONDecoder. Data is also read as dataString which is used for a very simple / (hopefully) fast way of determining if data does contain possible JSON and if we should spend more recourses into trying to decode it.

This is a clever use of SQLite weak typing, which easily exposes strings as blobs, and blobs as strings. I support it 100%. Yes, JSON strings are easier to spot when debugging. Yes, why turn a String into some Data when we can load Data right away?

There is a little caveat that we have to fix, though.

This caveat is in GRDB implementation of the Row type. Row have several flavors, and, to be short, two main ones:

Rows that come straight from SQLite

Those invoke as many low-level C SQLite functions as they can:

try dbQueue.read { db in
    let rows = try Row.fetchCursor(db, "SELECT * FROM player")
    while let row = try rows.next() {
        let data1: Data? = row["name"]           // invokes sqlite3_column_blob
        let data = row.dataNoCopy(named: "name") // invokes sqlite3_column_blob
    }
}

Those are the same low-level rows that are used in records queries:

struct Player: FetchableRecord {
    var data: Data?
    
    init(row: Row) {
        self.data = row.dataNoCopy(named: "name") // invokes sqlite3_column_blob
    }
}

let players: [Player] = try dbQueue.read { db in
    try Player.fetchAll(db, "SELECT * FROM player")
}

With Decodable as well:

struct Player: Decodable, FetchableRecord {
    var data: Data?
}

let players: [Player] = try dbQueue.read { db in
    try Player.fetchAll(db, "SELECT * FROM player") // invokes sqlite3_column_blob
}

Rows that are detached from SQLite

Those can't invoke any low-level C SQLite function because they don't have access to the SQLite connection:

// Those rows have been copied and detached from SQLite:
let rows: [Row] = try dbQueue.read { db in
    try Row.fetchAll(db, "SELECT * FROM player")
}

// This row has never seen SQLite in its life:
let row: Row = ["name": "Arthur"]

Detached rows can not rely on SQLite weak typing, because SQLite is not there. They basically are a plain container of DatabaseValue.

This means that eventual conversions between values have to be performed by GRDB itself. You can witness one below:

// GRDB conversions, not SQLite conversions:
let row = ["value": 1]
let int: Int = row["value"]         // 1, sure
let double: Double = row["value"]   // 1.0, no problem

Those conversions between numeric types above are what makes GRDB able to deal with numeric columns that may contain a mix of integers and doubles (SQLite is sometimes weird).

But GRDB does not yet provide automatic conversion from String to Data, and Data to String. You'll get fatal errors instead:

try dbQueue.read { db in
    // Compare cursor of low-level rows:
    let cursor = try Row.fetchCursor(db, "SELECT * FROM player")
    while let row = try cursor.next() {
        let data1: Data? = row["name"]           // invokes sqlite3_column_blob
        let data = row.dataNoCopy(named: "name") // invokes sqlite3_column_blob
    }
    
    // ... with an array of detached rows:
    let array = try Row.fetchAll(db, "SELECT * FROM player")
    for row in array {
        let data1: Data? = row["name"]           // fatalError: can not convert "Arthur" to Data
        let data = row.dataNoCopy(named: "name") // fatalError: can not convert "Arthur" to Data
    }
}

You may wonder what is the purpose of those detached rows, since you never met them. Well, I agree that those are not frequent at all in application code. But you will find them:

in GRDB tests
in Record Comparison
in tools like FetchedRecordsController or RxGRDB, that optimize some operations by only decoding rows into records on demand.

For you pull request to be a really good GRDB citizen, and work transparently with Record comparison, FetchedRecordsController, and RxGRDB, it has to be able to handle both raw rows, and detached rows.

This means making GRDB a little more convenient, and have it transparently converts strings to blobs, and blobs to strings, just like SQLite does.

Those conversions have to happen in String.fromDatabaseValue and Data.fromDatabaseValue.

Thanks for reading, and having the patience to learn more about this lib 😅

groue · 2018-08-07T12:57:28Z

.travis.yml

    - stage: Test Installation
      osx_image: xcode9.4
      install:
        - gem install cocoapods # Since Travis is not always on latest version
      env:
-        - TID=CocoaPods Lint GRDB.swift (Xcode 9.4)


Oops, it looks like you are no longer developing against the development branch. You are about to revert #393 :-)

valexa · 2018-08-07T14:45:48Z

Hi, we looked into the travis issues and fixed them all, our branch is now on sync with your develop branch as well so it should all be ready for merge, we plan to keep doing future contributions so the blob/string conversion is an interesting perspective.

groue · 2018-08-07T15:18:55Z

That's great, @valexa. Yes, we'll definitively need the blob/string conversion, I'm positive about this. And tests that check decoding from detached rows (edit: see sample code below). I'll start a full review shortly.

groue

That's a great PR. Will you please look at the suggestions?

groue · 2018-08-07T15:19:57Z

GRDB/Record/FetchableRecord+Decodable.swift

-                return try T(from: RowDecoder(row: row, codingPath: codingPath + [key]))
+                do {
+                    let natural = try T(from: RowDecoder(row: row, codingPath: codingPath + [key]))
+                    return natural


I think return try T(from...) is enough. That "natural" word was useful for communication between us, but has no real purpose in the code.

A comment like the following will help making the code more clear:

do { // This decoding will fail for types that need a keyed container, // because we're decoding a database value here (string, int, double, data, null). // Support for keyed containers is provided below (JSON decoding). return try T(from...)

groue · 2018-08-07T15:26:05Z

GRDB/Record/FetchableRecord+Decodable.swift

+                    let natural = try T(from: RowDecoder(row: row, codingPath: codingPath + [key]))
+                    return natural
+                } catch {
+                    if let data = row.dataNoCopy(named: key.stringValue), let dataString = String(data: data, encoding: .utf8), dataString.hasPrefix("[{") || dataString.hasPrefix("{"), dataString.hasSuffix("}]") || dataString.hasSuffix("}") {


Checking for the format is useless: if data contains something that is not JSON, JSONDecoder will throw anyway. And by not checking the format, we'll also avoid turning Data into String. This is the whole point of SQLite weak typing (store string, load data) 😄

groue · 2018-08-07T15:26:16Z

GRDB/Record/FetchableRecord+Decodable.swift

+                        // If data looks like JSON data then decode it into model (nested model as JSON)
+                        return try JSONDecoder().decode(type.self, from: data)
+                    } else {
+                        fatalError("\(error)")


throw error is enough, unless I'm mistaken.

groue · 2018-08-07T15:29:23Z

GRDB/Record/PersistableRecord+Encodable.swift

+                let encodeError = error
+                do {
+                    let json = try JSONEncoder().encode(value)
+                    guard let modelAsString = String(data: json, encoding: .utf8) else {


I think String(data: json, encoding: .utf8)! (force unwrap) is OK here: JSONEncoder is not supposed to output Data that can't be turned into a String (you can add this as a comment). JsonStringError is purposeless, and can be removed.

groue · 2018-08-07T15:30:58Z

GRDB/Record/PersistableRecord+Encodable.swift

+                    }
+                    return encode(modelAsString, key.stringValue)
+                } catch {
+                    fatalError("Encode error: \(encodeError), tried to encode to Json, got error: \(error)")


We should throw, and not fatalError here.

I understand that we hesitate between rethrowing encodeError or error. What do you think?

groue · 2018-08-07T15:32:19Z

GRDB/Record/PersistableRecord+Encodable.swift

@@ -180,7 +196,7 @@ private struct PersistableRecordEncoder : Encoder {
    /// - precondition: May not be called after a prior `self.container(keyedBy:)` call.
    /// - precondition: May not be called after a value has been encoded through a previous `self.singleValueContainer()` call.
    func unkeyedContainer() -> UnkeyedEncodingContainer {
-        fatalError("unkeyed encoding is not supported")
+        return ThrowingUnkeyedContainer(error: EncodingError.invalidValue(encode, EncodingError.Context(codingPath: [], debugDescription: "unkeyed encoding is not supported")))


Same as container(keyedBy:) above: we should throw an error instead.

groue · 2018-08-07T15:36:13Z

Tests/GRDBTests/FetchableRecordDecodableTests.swift

+    func testOptionalNestedStruct() throws {
+        struct NestedStruct : PersistableRecord, FetchableRecord, Codable {
+            let firstName = "Bob"
+            let lastName = "Dylan"


I'm glad a little music is entering the tests 👍

BTW... When I read let firstName = "Bob", I wonder how NestedStruct could hold different values. This makes the XCTAssertEqual(nestedModel.firstName, "Bob") test less obvious.

I'm not saying there is a bug here. I'm saying the test would be clearer if NestedStruct was declared as:

struct NestedStruct : PersistableRecord, FetchableRecord, Codable { var firstName: String var lastName: String }

and the insertion below written as:

let value = StructWithNestedType(nested: NestedStruct(firstName: "Bob", lastName: "Dylan"))

This comment applies to other testing structs with let properties.

Another point: NestedStruct does not need to adopt PersistableRecord and FetchableRecord. Same for all other nested structs.

groue · 2018-08-07T15:39:36Z

Tests/GRDBTests/FetchableRecordDecodableTests.swift

+
+            // Check the nested model contains the expected values of first and last name
+            XCTAssertEqual(nestedModel.firstName, "Bob")
+            XCTAssertEqual(nestedModel.lastName, "Dylan")


That's a good test. Checking for detached rows can be done with something like:

let row: Row = ["nested": """ { "firstName": "Bob", "lastName": "Dylan" } """] let model = StructWithNestedType(row: row) XCTAssertEqual(model.nestedModel.firstName, "Bob") XCTAssertEqual(model.nestedModel.lastName, "Dylan")

Thats interesting, where if at all would we test for detached rows ? It is still not clear to us what the implications of a "detached" row are.

"Raw rows" and "detached rows" are two different types of rows, which don't access values in the same way. Both share the same Swift type Row. But they don't have the same implementation, and their behavior is different in some corner cases. Raw rows load values straight from SQLite, using SQLite C apis. Detached rows don't have access to SQLite, and contains regular Swift values.

Since they don't access values in the same way, both code paths have to be tested. This is especially true in our case, because we store JSON strings, but load JSON data. We rely on automatic conversion from string to data.

This conversion is done by SQLite when a raw row is involved. It works, as you can see.

But this conversion is not done by SQLite for detached rows: it has to be done by GRDB itself. I know very well that this is not done today: the test above will fail if you run it (try, you will see - if the test pass, then there is something wrong in our code).

To make the test pass with detached rows, we need GRDB to provide the same automatic conversion as SQLite from string to data (and from data to string because I don't like half-baked features). This has to be done in the conversion methods I gave above: String.fromDatabaseValue and Data.fromDatabaseValue.

groue · 2018-08-07T15:41:54Z

GRDB/Record/PersistableRecord+Encodable.swift

+                // If value.encode does not work e.g. "unkeyed encoding is not supported" then see if model can be stored as JSON
+                let encodeError = error
+                do {
+                    let json = try JSONEncoder().encode(value)


Please set encoder's outputFormatting to .sortedKeys. This will guarantee stable JSON output, and will ease record comparison with databaseEquals and similar methods.

I'd also like that we think about which dateEncodingStrategy we should use. I don't quite like that we use the default format, which is not portable at all to other platforms (intervalSinceReferenceDate is specific to Foundation as far as I know). I'm thinking about people that share databases with Android: I don't want the JSON produced by GRDB to be painful for them to decode.

We were thinking of the same thing but .sortedKeys is iOS 11 only

Then we'll sort keys on iOS11 only.

And make this point clear in the documentation (that record comparison may give odd results for records stored before iOS 11)

Precisely speaking: iOS 11.0+, macOS 10.13+, watchOS 4.0+

groue · 2018-08-07T15:51:22Z

GRDB/Record/FetchableRecord+Decodable.swift

+                do {
+                    let natural = try T(from: RowDecoder(row: row, codingPath: codingPath + [key]))
+                    return natural
+                } catch {


I wonder if we could catch precisely the error emitted when the decoded type asks for a keyed container, and doesn't find any. This will prevent us from switching to JSON decoding for types that throw an error when they fail decoding from the SingleValueDecodingContainer.

well its the "unkeyed decoding is not supported" error so yes we can add a check for that

groue · 2018-08-07T16:21:25Z

Now that the first review is done, I can answer your other comments:

That if prevents types from providing customized decoding of JSON data. I'd like to read an exploration of the potential harm, if any.

As our nested model to JSON code is now at the end of the queue I believe this wont prevent others providing custom decoding of JSON, please say if you feel this is not the case.

I have a single concern so far, which is in this comment: I wonder if we could catch precisely the error emitted when the decoded type asks for a keyed container, and doesn't find any.

Other points:

We have added tests for Persistable Record and Fetchable Record, I have tried to add them into the correct places but happy for you to move / suggest other location for them to reside.

That's very good.

Both PersistableRecord+Encoding and FetchableRecord+Decodable include our code in the do { try natural decoding } catch { try JSON decoding } pattern you suggested. But for PersistableRecord+Encoding I had to add ThrowingKeyedContainer and ThrowingUnkeyedContainer class to make KeyedContainer and UnkeyedContainer throwing so that the try value.encode(to: PersistableRecordEncoder(codingPath: [key], encode: encode)) could be cached. As before fatalError("unkeyed encoding is not supported”) was used.

I hope my suggestion in the review (replace the fatal error with a regular error instead of building a throwing container) will also trigger our fallback to JSON.

in FetchableRecord+Decodable if JSONDecoder fails it's error is tricked down to FetchableRecord init. How do you feel about this or would you prefer for it to be cached just after the JSONDecoder code?

I'm not sure I understand. Would you please spot me the lines you are talking about?

I am happy to add to README this nested model as JSON if you wish, though would like your advice on where about to place this. Or let me know if you prefer a dedicated guide?

I'm very happy that you consider writing doc. This is the best way to check that a feature has any meaning :-)

What do you think about a sub-chapter of Codable Records? We'd need to put "JSON" somewhere in its title, so that people know what we are talking about. And there is this sentence which we'll have to update:

GRDB ships with support for nested codable records, but this is a more complex topic. See Associations for more information.

Cheers, @gusrota, @valexa!

valexa · 2018-08-08T14:51:14Z

I think a sub chapter might be too much, I think just changing your current example to something like

        // Declare a plain Codable struct or class...
        struct Player: Codable {
            let name: String
            let score: Int
            let location: PlayerLocation
            let medals: [PlayerMedals]
        }

        struct PlayerLocation : Codable {
            let latitude: Double
            let longitude: Double
        }

        struct PlayerMedals : Codable {
            let name: String
            let type: String
        }

to show that nested Codable types are now supported, I would state that JSON serialization is used to store them into the database as a implementation detail but outside of that I believe that we did everything in such a way that minimal understanding of the implementation is needed.

groue · 2018-08-08T14:58:59Z

This is a good idea.

Yet we'll have to talk about record comparison troubles when we can't sort JSON keys: it can surprise users in a bad way, and some will prefer to write their own JSON serialization in order to avoid this caveat. I don't like bad surprises when I use a library, and so do many people.

We can skip talking about the format of dates, though, because users will discover it when they eventually care about it. But we will care about it right now, because we won't be able to change the JSON format easily in future releases for the sake of backward compatibility.

See the comments in my initial review for more information.

valexa

Done

groue · 2018-08-10T03:25:37Z

BTW folks, I'm currently working on a project that will happily use JSON encoding. I'll be the second user of your PR :-)

valexa · 2018-08-13T09:48:59Z

Hi Gwendal, are you ok to merge this and do you have any idea when swift 4.2 support will me merged as well ? thanks

groue · 2018-08-13T10:27:25Z

@valexa I didn't know you were waiting for a second review, because you didn't ask for it. OK, I'll do it.

groue

Thanks @valexa, that's better. Here are a few other points to address. And I don't know any longer if you plan to write documentation or not. Writing doc is not mandatory, but the initial @gusrota's comment was mentioning it, and you have to be clear if I should wait for it or not.

The more this PR will be clean and complete, the faster it will be merged. All that you don't do, somebody else will have to do it, and this will postpone the merge. It's as simple as that. See the five checkboxes in the initial comment.

groue · 2018-08-13T10:28:31Z

GRDB/Record/FetchableRecord+Decodable.swift

+                    let value = try T(from: RowDecoder(row: row, codingPath: codingPath + [key]))
+                    if let _ = value as? Codable {
+                        // Support for nested ( Codable )
+                        return try JSONDecoder().decode(type.self, from: row.dataNoCopy(named: key.stringValue)!)


I don't understand the reason for if let _ = value as? Codable. The comment Support for nested ( Codable ) doesn't help me understand.

we are decoding twice here, which looks like a big waste of time.

why do we check for Codable, since we are only decoding? Isn't Decodable enough?

how on Earth this condition could be false, since we are decoding a Decodable type?

All this code is pretty obscure to me. What do I miss?

I am not sure this is the best but the way the current code is basically considering the Encodable compliance a such 4 tests hit this code the following way :
if let _ = value as? Codable {
//testOptionalNestedStruct
} else {
//testDecodableRawRepresentableProperty
//testNonTrivialSingleValueDecodableProperty
//testTrivialSingleValueDecodableProperty
}

Thanks for the details. Please provide an answer to other questions as well.

I want you to understand, @valexa, that the if let _ = value as? Codable test has to go.

That's because it would have GRDB behave differently if I start decoding this type:

struct MyStruct: Decodable { ... }

And then I decide that MyStruct should be Encodable as well:

struct MyStruct: Codable { ... }

In the first version of MyStruct, if let _ = value as? Codable doesn't pass. In the second version, if let _ = value as? Codable passes. Decoding will change. Just because I added a conformance to MyStruct, totally unrelated to decoding.

This is an important bug that has to be fixed.

I've downloaded your code, and run the tests. I see the problem you are facing. Please hold on until I find the proper way out.

All right, the problem is that we use a RowDecoder (dedicated to decoding a row) when we decode a column value. That decoder happily builds keyed and unkeyed containers. But we don't want to build keyed and unkeyed containers when we decode a column value! It is quite the contrary: we want a decoding error, so that we can switch to JSON. Conclusion: RowDecoder is not the decoder we need here. We need another decoder instead. I'll take care of fixing this one. Please make sure I can push to your branch.

groue · 2018-08-13T10:34:26Z

GRDB/Record/PersistableRecord+Encodable.swift

-            try value.encode(to: PersistableRecordEncoder(codingPath: [key], encode: encode))
+            do {
+                 try value.encode(to: PersistableRecordEncoder(codingPath: [key], encode: encode))
+            } catch {


Why do we carefully check for the error type before we switch to JSON decoding, but not when we switch to JSON encoding? I'd like some consistency, and the same level of care to be displayed in both places.

This was a oversight, fixed now that you pointed it out.

You mean in 6262d10? It still needs a little extra effort: just like we did for decoding, we have to check for a particular encoding error before switching to JSON.

Updated in d81475a

groue · 2018-08-13T10:40:49Z

GRDB/Record/PersistableRecord+Encodable.swift

+    func encode<T>(_ value: T, forKey key: KeyType) throws where T : Encodable { throw errorMessage }
+
+    func nestedContainer<NestedKey>(keyedBy keyType: NestedKey.Type, forKey key: KeyType) -> KeyedEncodingContainer<NestedKey> where NestedKey : CodingKey {
+        fatalError("Not implemented")


I think we should return another throwing container here. Imagine a codable type that tries to access a nested container before it stores its properties and triggers one of our throwing encode methods above. Program will crash instead of switching to JSON encoding. This type should never fatalError, so that we can always switch to JSON. That's the whole point :-)

Thanks @groue good point :-), commit c52b59f now returns another throwing container.

groue · 2018-08-13T10:41:00Z

GRDB/Record/PersistableRecord+Encodable.swift

+    func encodeNil() throws { throw errorMessage }
+
+    func nestedContainer<NestedKey>(keyedBy keyType: NestedKey.Type) -> KeyedEncodingContainer<NestedKey> where NestedKey : CodingKey {
+        fatalError("Not implemented")


Same comment as above: there should be no fatalError in this type.

commit c52b59f also returns another throwing container here

groue · 2018-08-13T10:43:38Z

Tests/GRDBTests/FetchableRecordDecodableTests.swift

+
+// MARK: - Custom nested Decodable types - nested saved as JSON
+
+extension FetchableRecordDecodableTests {


I still don't see any test for detached rows. Context and guidance.

Hi, we are having issues to even begin imagining how to test for detached rows, are there any existing non Codable detached rows tests we could look at for guidance ?

Start by clicking my "context and guIdance" link: it starts with a sample code.

@gusrota, @valexa, it's totally OK if you stop working on this PR. You don't have to "obey" my requests. But please be totally clear about our common expectations.

@groue please be reassured that both I and @valexa very much wish to continue on this PR and finish on the open points to so that it is ready to be merged into master branch.

@groue have just committed changes to add test for detached rows. Thank you for the example code and the description you gave to help explain detached rows. I have added JSON checking in Data.fromDatabaseValue to allow the reading of Data that is of JSON content. I did not need to add any additions to String.fromDatabaseValue as seems to work fine.

I have just committed changes to add test for detached rows. Thank you for the example code and the description you gave to help explain detached rows.

Great

I have added JSON checking in Data.fromDatabaseValue to allow the reading of Data that is of JSON content.

Something was not understood at all, here. See this comment for context.

I did not need to add any additions to String.fromDatabaseValue as seems to work fine.

I understand, you want to remain focused on your use case and your convenience. But please also understand that this is not enough. I manage this open source library not only for you. If GRDB had been built for my personal use cases only, you would have never heard of it. I repeat: If we support String to Data conversion, we have to support Data to String conversion. It is a matter of consistency. And it was explicitly requested in this comment.

This should be done now in 76f5adf

groue · 2018-08-13T10:56:47Z

@valexa:

do you have any idea when swift 4.2 support will me merged as well ? thanks

Support for Swift 4.2 and Xcode 10 will ship when Xcode 10 ships. Meanwhile, check existing issues about Swift 4.2 and Xcode 10.

… the documentation

groue · 2018-08-14T10:56:15Z

GRDB/Core/Support/Foundation/Data.swift

@@ -25,6 +25,17 @@ extension Data : DatabaseValueConvertible, StatementColumnConvertible {
    /// a Blob.
    public static func fromDatabaseValue(_ dbValue: DatabaseValue) -> Data? {
        guard case .blob(let data) = dbValue.storage else {
+            // Check to see if data is String, if so then pass is through JSONSerialization
+            // to confirm if contains JSON, if so then this is a nested type stored as JSON,
+            // so return string as Data so that it can be decoded


I'm sorry @gusrota, but we have a deep misunderstanding. I'm sure it's my fault. JSON has nothing to do here. Conversion from String to Data should always succeed:

switch dbValue.storage { case .blob(let data): return data case .string(let string): return string.data(using: .utf8) default: return nil }

I'm also concerned by the fact that you don't hesitate decoding JSON data multiple times without any concern for performance.

When we try

public static func fromDatabaseValue(_ dbValue: DatabaseValue) -> Data? { switch dbValue.storage { case .blob(let data): return data case .string(let string): return string.data(using: .utf8) default: return nil } }

we get 7 tests failing

should the tests be fixed or should we handle strings in some other way so the tests pass ?

Yes, that test needs to be fixed 😄 (not by reversing the nil/not nil test, but by checking the converted value, of course)

groue · 2018-08-15T09:36:02Z

All right, I take over. Many thanks to both of you!!!

I'll shortly merge your PR in the development branch, when it is ready. It won't go to master until the next GRDB release, which I plan to ship after Xcode 10 is out of beta. It won't be long until Apple ships iOS 12, so you won't have to wait a long time. Meanwhile, please keep on using your rotacloud branch.

@valexa, @gusrota, does it sound OK for you?

because Data belongs to Foundation, not to the Standard Library. Remove `import Foundation` from StandardLibrary.swift

…odable

…tible

groue · 2018-08-15T15:21:13Z

Complete exploration of the new String <-> Data conversion is completed. The feedback for a few conversion errors has been enhanced on the way.

Data: .base64 Date: .millisecondsSince1970 NonConformingFloat: .throw

groue · 2018-08-15T17:38:31Z

I think we're done, folks!

groue · 2018-09-06T04:42:06Z

Shipped in v3.3.0-beta1 :-)

groue added the enhancement label Aug 6, 2018

groue reviewed Aug 7, 2018

View reviewed changes

Added nested Codable support by flattening into/from JSON

feeedcb

valexa deleted the rotacloud branch August 7, 2018 14:32

gusrota changed the base branch from development to master August 7, 2018 14:34

gusrota changed the base branch from master to development August 7, 2018 14:34

groue requested changes Aug 7, 2018

View reviewed changes

Vlad Alexa added 2 commits August 8, 2018 11:41

implemented requested changes

74aae3d

fixed checking for nested codable

7def60c

valexa approved these changes Aug 9, 2018

View reviewed changes

sorted encoder outputFormatting for ios11+ etcl

2826472

groue requested changes Aug 13, 2018

View reviewed changes

Vlad Alexa and others added 4 commits August 14, 2018 09:25

added catch

6262d10

extended Codable documentation and added test for the example used in…

7f16e35

… the documentation

Detached rows string to data and tests

8c06e7b

Added tests for arrays

a08974f

groue reviewed Aug 14, 2018

View reviewed changes

groue added 2 commits August 15, 2018 12:05

Restore GRDB/Core/Support/Foundation/Data.swift

1c66ad4

because Data belongs to Foundation, not to the Standard Library. Remove `import Foundation` from StandardLibrary.swift

PersistableRecord+Encodable: make it as robust as FetchableRecord+Dec…

820673b

…odable

groue changed the title ~~Save and fetch a nested model stored as JSON in database cell Updated~~ JSON encoding and decoding of nested codable models Aug 15, 2018

groue and others added 10 commits August 15, 2018 13:41

Cleanup, documentation, and preserve coding paths

358225b

5f41c57 addition

b2234b4

Refactor value conversion tests

2d6140a

Test decoding of non-UTF8 data

de4f57c

Better feedback for conversion errors from StatementColumnConvertible

0b77d0d

Tests for failed value conversions

e8dacb3

Even better feedback for conversion errors from StatementColumnConver…

b47f586

…tible

Oh men of little faith!

eba21d3

More tests for Data -> String conversion: play with JPEG :-)

4d343e8

Restore SPM: add missing Foundation import

d00421a

groue added 6 commits August 15, 2018 17:33

Safer default implementation for RowImpl.copiedRow(_:)

659db4f

JSON encoding: choose strategies

2eb0955

Data: .base64 Date: .millisecondsSince1970 NonConformingFloat: .throw

Tests for JSON date strategy

9bb4ff8

Tests for JSON data strategy

02db2a8

Documentation for JSON support

dfb9421

Fix typo

2b8c838

groue changed the title ~~JSON encoding and decoding of nested codable models~~ JSON encoding and decoding of codable record properties Aug 15, 2018

CHANGELOG.md

44b998c

groue approved these changes Aug 15, 2018

View reviewed changes

Welcome @valexa and @gusrota to the thank you notice

7ea15dc

groue merged commit 30bc15a into groue:development Aug 15, 2018

groue mentioned this pull request Aug 18, 2018

Codable records: customize the userInfo context dictionary, and the format of JSON columns #399

Merged

groue changed the title ~~JSON encoding and decoding of codable record properties~~ Codable records: JSON encoding and decoding of codable record properties Aug 19, 2018


		// MARK: - Custom nested Decodable types - nested saved as JSON

		extension FetchableRecordDecodableTests {

Codable records: JSON encoding and decoding of codable record properties #397

Codable records: JSON encoding and decoding of codable record properties #397

Conversation

gusrota commented Aug 6, 2018 • edited by groue Loading

One more thing…

groue commented Aug 6, 2018

groue commented Aug 6, 2018

groue commented Aug 6, 2018 • edited Loading

Choose a reason for hiding this comment

valexa commented Aug 7, 2018

groue commented Aug 7, 2018 • edited Loading

groue left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

groue Aug 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

groue Aug 8, 2018 • edited Loading

Choose a reason for hiding this comment

groue Aug 7, 2018 • edited Loading

Choose a reason for hiding this comment

groue Aug 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

groue commented Aug 7, 2018 • edited Loading

valexa commented Aug 8, 2018 • edited Loading

groue commented Aug 8, 2018

valexa left a comment

Choose a reason for hiding this comment

groue commented Aug 10, 2018

valexa commented Aug 13, 2018 • edited Loading

groue commented Aug 13, 2018

groue left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

groue Aug 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

groue Aug 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valexa Aug 13, 2018 • edited Loading

Choose a reason for hiding this comment

groue Aug 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

groue commented Aug 13, 2018

groue Aug 14, 2018 • edited Loading

Choose a reason for hiding this comment

valexa Aug 14, 2018 • edited Loading

Choose a reason for hiding this comment

gusrota commented Aug 6, 2018 •

edited by groue

Loading

groue commented Aug 6, 2018 •

edited

Loading

groue commented Aug 7, 2018 •

edited

Loading

groue Aug 7, 2018 •

edited

Loading

groue Aug 8, 2018 •

edited

Loading

groue Aug 7, 2018 •

edited

Loading

groue Aug 7, 2018 •

edited

Loading

groue commented Aug 7, 2018 •

edited

Loading

valexa commented Aug 8, 2018 •

edited

Loading

valexa commented Aug 13, 2018 •

edited

Loading

groue left a comment •

edited

Loading

groue Aug 14, 2018 •

edited

Loading

groue Aug 13, 2018 •

edited

Loading

valexa Aug 13, 2018 •

edited

Loading

groue Aug 13, 2018 •

edited

Loading

groue Aug 14, 2018 •

edited

Loading

valexa Aug 14, 2018 •

edited

Loading