Rotacloud

[valexa]

Hi, we added what seems to be working support for nested Codable types in models eg:

public struct AccountModel : FetchableRecord, MutablePersistableRecord, Codable {
let id: Int
let name: String?
let users: [UserModel]?
}

to save as a JSON string in the database.

[groue]

Hello @valexa,

I admit I don't know what to do from this pull request.

On one side, it is very interesting, and it opens opportunities.

On the other side, I wonder if it could conflict, or not, with future development of associations, where we'd also like to support code like the following:

// Future Plan for GRDB associations
struct AccountInfo: Codable {
    var account: Account
    var users: [User]
}

let request = Account.including(Account.users)
let infos = AccountInfos.fetchAll(db, request) // [AccountInfo]

Look at the users Array above. It looks similar to the users Array in your sample code, doesn't it? But it is not the same. Instead of being loaded from a JSON blob column in the accounts table, it is loaded through a foreign key from the account table to the user table (when the database schema is designed this way).

I don't want to open a door that would close this one.

Your PR thus requires further analysis. The question is: who will perform this analysis? Me, another GRDB contributor, or you?

I suggest you do the first round. To help you, I'll copy below the message that has been presented to you when you opened the pull request, and that you didn't read:

<!-- Please describe your pull request here. -->

### One more thing…
<!--
When relevant, please check as many checkboxes in this list as possible.

Documentation makes sure that you are not the only one who knows about the changes. Tests make sure your changes will be supported in the future.

Write documentation and tests if you can, because if nobody else has the time to do it, the pull request will simply be rejected.

Ask for help and guidance when needed.
-->

- [ ] 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

This PR will remain open a few days: please develop your idea! I'll answer any question, if you have any.

[groue]

On top of my request for further information above, I have several questions here:

• Why do you read data here, and save a string below?
• Why isn't this code also present in decode<T>(_: 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.
• 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.
• That if prevents types from providing customized decoding of JSON data. I'd like to read an exploration of the potential harm, if any.

[valexa]

Hi Gwendal and thanks for the prompt response, the insights you brought to the table were just what we were hoping for, me and my colleague are not that familiar with your library to know for sure all the implications so your feedback is most welcome, we will do more work on it then submit a new pull request that should also have tests for the functionality we propose.

In regards for your future plan of associations would it not be possible to have both with the default being either one and having a setting to choose if you want json or associations ?

[groue]

Thanks for your answer, @valexa, it is comforting.

In regards for your future plan of associations would it not be possible to have both with the default being either one and having a setting to choose if you want json or associations ?

After I wrote my initial message, I realized that decoding of arrays of associated records will require a very different decoding process: it is unlikely that the two features overlap in a wrong way. After all, decoding a column in a row has nothing to do with associations. Please don't worry about this topic any longer.

Here are a few hints for your exploration:

• what about having Array of a Decodable type conditionally adopt DatabaseValueConvertible? This would address the topic of arrays in the "regular GRDB way", by using DatabaseValueConvertible in order to provide customized row value decoding. This may require Swift 4.2 is order to check for DatabaseValueConvertible at runtime - I'm not sure.
• when the solution above doesn't work, what about letting the decodable type attempt to decode itself, and fallback on JSON deserialization on error (and fail as well if data is not JSON)?

[valexa]

Thank you, we tried adopting DatabaseValueConvertible but it did not work out because of swifts implicit enum initializers being private amongst other things.
As for the second approach it is not clear where exactly you mean to attempt to decode itself, at this point it feels to us a bit too much like stumbling in the dark within the inner workings of the library and we are not comfortable changing too much without breaking something.
Is there a time when you think you could take a look yourself at the core thing we are trying to do which is just handling a Codable with a Codable nested inside it by serializing the JSON as a string ?

[groue]

Is there a time when you think you could take a look yourself at the core thing we are trying to do which is just handling a Codable with a Codable nested inside it by serializing the JSON as a string ?

I don't know when, but I agree that this is a natural feature request.

I'm currently on three topics on GRDB, which you can see in currently open PR and issues:

• Enhancing diagnostic when row decoding fails (like, when you decode a non-optional value from NULL). Particularly, when a Decodable type does not quite match the content of the db, you get a cryptic error: you don't quite know which column of which table is wrong. I want to fix that, because if I did lose time fixing such a bug, others do, too.
• Activate the modern full-text engine FTS5 by default on recent Apple systems, and the new SQLCipher (GRDB currently requires a custom build of SQLite in order to activate FTS5).
• Prepare the release of Xcode 10 and Swift 4.2.

All maintenance tasks, as you see.

Thank you, we tried adopting DatabaseValueConvertible but it did not work out because of swifts implicit enum initializers being private amongst other things.

Too bad 😭

As for the second approach it is not clear where exactly you mean to attempt to decode itself, at this point it feels to us a bit too much like stumbling in the dark within the inner workings of the library and we are not comfortable changing too much without breaking something.

I had in mind something like do { try natural decoding } catch { try JSON decoding }.