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

DatabaseValueConvertible support for Encodable / Decodable (?) #1096

Closed
freak4pc opened this issue Nov 14, 2021 · 7 comments
Closed

DatabaseValueConvertible support for Encodable / Decodable (?) #1096

freak4pc opened this issue Nov 14, 2021 · 7 comments
Labels
needs revisiting This issue was closed but it has unfinished business question

Comments

@freak4pc
Copy link
Contributor

freak4pc commented Nov 14, 2021

What did you do?

In some model:

public func encode(to container: inout PersistenceContainer) {
    container[someColumn] = [1, 2, 3]
}

What did you expect to happen?

I assumed that you can assign Codable things as if they were DatabaseValueConvertibles (at least that's how it seems theoretically in DatabaseValueConvertible+Decodable.swift and DatabaseValueConvertible+Encodable.swift), but this doesn't seem to be the cast, unless I'm missing something.

What happened instead?

Cannot assign value of type '[Int]' to subscript of type 'DatabaseValueConvertible?'

Environment

GRDB flavor(s): GRDB
GRDB version: 5.12.0
Installation method: CocoaPods
Xcode version: 13.1
Swift version: 5.5
Platform(s) running GRDB: iOS
macOS version running Xcode: Monterey

Demo Project

If needed let me know, seems unneeded in this question context

@groue
Copy link
Owner

groue commented Nov 14, 2021

Hello @freak4pc,

Collections, arrays, sets, etc, are not DatabaseValueConvertible, the protocol for types that can be turned into a database value. Optionals of DatabaseValueConvertible types are not DatabaseValueConvertible either. And DatabaseValueConvertible is not a sub-protocol of Codable.

Those are very old decisions that I can't fully remember now, I admit. It may even be time to revisit some of them, or at least check what would break if they were reverted.

But if you consider that Codable Record types have options that control the JSON encoding of their columns, you'll see that in the line container[someColumn] = [1, 2, 3], the record is not involved, and can't apply his customizations. There's not much room for encoding [1, 2, 3] in various ways, but there are other funny types like Date, and generally plenty of JSON encoding options.

This alone is a good reason for not letting [Int] decide how to encode itself. This is the job of a record type. And since you opt in for an explicit encode(to:) method, you have to perform your encoding of [1, 2, 3]. That's what the GRDB apis want you to do today.

Is there a specific reason you write an explicit encode(to:) method?

@groue groue added the question label Nov 14, 2021
@groue
Copy link
Owner

groue commented Nov 14, 2021

Those are very old decisions that I can't fully remember now, I admit.

Now I remember. At the root was ccgus/fmdb#180. With FMDB, you can write:

NSArray *ids = @[@1, @2, @3];
[db executeQuery:@"SELECT * FROM table WHERE id IN (?)" withArgumentsInArray:@[ids]]

The result was garbage, because this would not run SELECT * FROM table WHERE id IN (1, 2, 3) as expected. Instead, the array was encoded as some string garbage ([NSArray description]) into the one and single ? SQLite placeholder.

I learned the hard way that SQLite values that feed ? are only scalars (ints, doubles, strings, blobs, and NULL). There's no support for collections in SQLite.

Based on this experience, I did not want GRDB users to feel my pain. So this code has never compiled:

// Compiler error: [Int] is not DatabaseValueConvertible
let ids = [1, 2, 3]
let players = try Player.fetchAll(db, sql: "SELECT * FROM player WHERE id IN (?)", arguments: [ids])

And I don't want that it starts compiling: [Int] is quite unlikely to conform DatabaseValueConvertible any time soon.

Since then, I did a lot to enhance support for collections, though. For example you can run:

let ids = [1, 2, 3]
let request: SQLRequest<Player> = "SELECT * FROM player WHERE id IN \(ids)"
let request = Player.filter(ids.contains(Column("id")))
let players = try Player.fetchAll(db, ids: ids)

So maybe a layer of convenience could be added to PersistenceContainer, so that it accepts any Encodable type (with some default JSON encoding).

But before we start looking into this, I need to know why. Hence my question:

Is there a specific reason you write an explicit encode(to:) method?

@freak4pc
Copy link
Contributor Author

freak4pc commented Nov 14, 2021

@groue Thanks for the prompt response! I think I bumped into that issue with FMDB, but that's the troubles of a dynamic language. I think it would be "easier" to avoid it in a statically typed language such as Swift.

Per your question, of "why":

  1. I usually prefer opting in for "explicitness over magic" - this is just a personal preference. I don't like the thought of GRDB inferring which table properties to use for each field and which serialization, etc.
  2. Even if I would be OK with that, sometimes you simply can't. Sometimes your Codable conformance is about a server contract, but you want to serialize the data differently for persistence, etc (but only for one field out of 20, for example).

Those are the cases I have in mind. We start having a few helper methods to encode into BLOB and decode from BLOB but that's a bit more involved (probably best option if GRDB doesn't have something like it).

@groue
Copy link
Owner

groue commented Nov 15, 2021

@groue Thanks for the prompt response! I think I bumped into that issue with FMDB, but that's the troubles of a dynamic language. I think it would be "easier" to avoid it in a statically typed language such as Swift.

I agree.

Per your question, of "why" [...]

OK, I get it. That's quite legit. Thanks 👍

In the end, my understanding is that a convenience method or subscript on PersistenceContainer that accepts any Encodable value would be handy.

@freak4pc
Copy link
Contributor Author

That's what we did for now :)
I'll close this for now but appreciate your thoughts on the situation. If you ever end up doing something like this in the framework that'd be great :)

@groue
Copy link
Owner

groue commented Nov 15, 2021

Thank you very much @freak4pc for this issue. 👍

To be honest, I think that this will only be handled in GRDB 6, where I'll finally allow errors to be thrown from row initializers (a long overdue) - and I have strong hopes that by then the language will have throwing subscripts.

I add flag "needs revisiting", as an issue that contains ideas that should not be forgotten.

@groue groue added the needs revisiting This issue was closed but it has unfinished business label Nov 15, 2021
@freak4pc
Copy link
Contributor Author

Looking forward! Thanks :)

FYI, The language already has throwing subscripts in Swift 5.5 :) at least for getters

subscript(..) { 
    get throws { 
        throw ...
    } 
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs revisiting This issue was closed but it has unfinished business question
Projects
None yet
Development

No branches or pull requests

2 participants