GRDB.swift icon indicating copy to clipboard operation
GRDB.swift copied to clipboard

RecordEncoder fails to encode optional AttributedString fields as JSON – "unkeyed encoding is not supported"

Open sipefree opened this issue 11 months ago • 2 comments

What did you do?

AttributedString structs are encodable using an @CodableConfiguration property wrapper provided with the subset of attributes that is allowed to be encoded into JSON.

I would expect GRDB to be able to handle encoding these values as JSON columns, but a subtlety in how CodableWithConfiguration types makes that impossible with the current implementation, but only when the field is optional.

Try this sample code:

import Foundation
import GRDB

let dbQueue = try DatabaseQueue()

var migrator = DatabaseMigrator()
migrator.registerMigration("createPosts") { db in
    try db.create(table: "post") { t in
        t.autoIncrementedPrimaryKey("id")
        t.column("title", .text).notNull()
        t.column("date", .date).notNull()
        t.column("text", .jsonText)
    }
}

try migrator.migrate(dbQueue)

struct Post: PersistableRecord, FetchableRecord, Hashable, Codable {
    var id: Int
    var title: String
    var date: Date

    @CodableConfiguration(from: \.foundation)
    var text: AttributedString? = nil
}

let post = Post(
    id: 0,
    title: "My first post.",
    date: Date(),
    text: try? AttributedString(markdown: "This is my **first** post!")
)

let jsonEncoder = JSONEncoder()
jsonEncoder.outputFormatting = .prettyPrinted
let postJSON = String(data: try jsonEncoder.encode(post), encoding: .utf8)!
print(postJSON)


try dbQueue.inTransaction { db in
    try post.insert(db) // <-- crash here
    return .commit
}

What did you expect to happen?

Expected the Post record to be inserted to the database.

What happened instead?

The program crashes by hitting one of these fatalErrors:

EncodableRecord+Encodable.swift:33

    func unkeyedContainer() -> UnkeyedEncodingContainer {
        fatalError("unkeyed encoding is not supported")
    }

or

EncodableRecord+Encodable.swift:48

    func singleValueContainer() -> SingleValueEncodingContainer {
        // [...]
        fatalError("single value encoding is not supported")
    }

This is because the AttributedString's encoding implementation will either open a singleValueContainer(), or an unkeyedContainer() depending on its internal representation, and this is getting called on the top level RecordEncoder rather than on an encoding container for a JSON value.

Note that this only happens when the AttributedString field is optional.

It appears to be related to how CodableWithConfiguration works when the field is optional:

From: https://github.com/apple/swift-corelibs-foundation/blob/778dde90ff7cf63f05634c3df6b0788c89249770/Sources/Foundation/AttributedString/AttributedStringCodable.swift#L116

public extension KeyedEncodingContainer {
    mutating func encode<T, C>(_ wrapper: CodableConfiguration<T?, C>, forKey key: Self.Key) throws {
        switch wrapper.wrappedValue {
        case .some(let val):
            try val.encode(to: self.superEncoder(forKey: key), configuration: C.encodingConfiguration)
            break
        default: break
        }
    }
}

This code is calling self.superEncoder(forKey: key), but the RecordEncoder.KeyedEncoder always returns recordEncoder for that method, when it should probably return something wrapping a JSON encoder. I have no idea why it's asking for a superEncoder, but this seems to be the way they've implemented it.

Environment

GRDB flavor(s): GRDB GRDB version: 6.25.0 Installation method: SPM Xcode version: Xcode 15 Swift version: 5.10 Platform(s) running GRDB: macOS macOS version running Xcode: 14.3.1

Demo Project

GRDBAttrStringTestPkg.zip

sipefree avatar Mar 06 '24 18:03 sipefree

Hello @sipefree

Thanks for the report! The Codable implementation of GRDB is indeed incomplete... I agree that we should finish the job and provide support for AttributedString 👍

Thanks also for your investigations :)

I'm currently far away from any computer, so please wait a few days before a more detailed feedback!

groue avatar Mar 07 '24 14:03 groue

I have an AttributedString+DatabaseValueConvertible extension in my current GRDB project where I handle all the JSON coding. In my own case, I have a SQLite check constraint to ensure the field is JSON (json_valid(column) = 1) which can fail for the standard encoder because an AttributedString without attributes encodes to a plain string, not JSON.

This is likely a non-issue for most but could be worth noting in documentation.

(I handle it by checking if there’s only one run and if that one run has no attributes, in which case I custom encode the plain text as [\"\(plainText)\", {}] which satisfied SQLite and decodes with the standard decoder as expected.)

Jason-Abbott avatar Mar 07 '24 17:03 Jason-Abbott

Hello @sipefree,

I would prefer to address this issue by exercising the fix on a type that correctly uses super encoders. AttributedString sounded like a perfect candidate, but when I try to reproduce the issue, I can not (Xcode 15.3, macOS 14.3.1):

struct Post: PersistableRecord, Encodable {
    let text: AttributedString
}

let dbQueue = try DatabaseQueue()
try dbQueue.inDatabase { db in
    try db.create(table: "post") { t in
        t.column("text", .jsonText)
    }

    let post = try Post(text: AttributedString(markdown: "This is my **first** post!"))
    try post.insert(db)

    try print(String.fetchOne(db, sql: "SELECT text FROM post")!)
    // Prints:
    // [
    //   "This is my ",
    //   {
    //     "NSPresentationIntent": {
    //       "components": [
    //         {
    //           "identity": 1,
    //           "kind": [
    //             "paragraph"
    //           ]
    //         }
    //       ]
    //     }
    //   },
    //   "first",
    //   {
    //     "NSInlinePresentationIntent": 2,
    //     "NSPresentationIntent": {
    //       "components": [
    //         {
    //           "identity": 1,
    //           "kind": [
    //             "paragraph"
    //           ]
    //         }
    //       ]
    //     }
    //   },
    //   " post!",
    //   {
    //     "NSPresentationIntent": {
    //       "components": [
    //         {
    //           "identity": 1,
    //           "kind": [
    //             "paragraph"
    //           ]
    //         }
    //       ]
    //     }
    //   }
    // ]
}

Do you have some more information about the exact context that triggers the call to superEncoder?

cc @itaiferber: Dear Itai, if you know where I can find a canonical Decodable+Encodable type that uses super decoders/encoders, I could work without AttributedString. (I'd prefer an "integration test" with an externally-defined Codable type rather than a "unit test" with my ad-hoc codable type that may misuse Codable). Do you have any hint?

groue avatar Mar 17 '24 11:03 groue

Dear Itai, if you know where I can find a canonical Decodable+Encodable type that uses super decoders/encoders

Great question! To be honest, I'm not aware of a canonical example of this, since all of the stdlib types that conform to Codable are value types; the only type in Foundation that uses super-encoders/-decoders is indeed AttributedString, but I'm actually not familiar enough with CodableWithConfiguration to add more than what @sipefree has already brought up.

However, there are at least some unit tests in both swift-foundation and swift-corelibs-foundation that exercise these codepaths, and so may help you.

my ad-hoc codable type that may misuse Codable

That being said, it shouldn't be difficult to write a simple type to exercise this:

// Gets compiler-synthesized `Codable` impl
class Sup: Codable {
    var x: Int
    init(_ x: Int) { self.x = x }
}

// We have to override init(from:) and encode(to:) to avoid inheriting them
class Sub: Sup {
    var y: Int
    init(_ x: Int, _ y: Int) { self.y = y; super.init(x) }

    private enum CodingKeys: String, CodingKey {
        case y
    }

    required init(from decoder: any Decoder) throws {
        let container = try decoder.container(keyedBy: CodingKeys.self)
        y = try container.decode(Int.self, forKey: .y)

        // This will effectively fetch a nested container for key "super", then return a `Decoder` wrapping it
        let superDecoder = try container.superDecoder()
        try super.init(from: superDecoder)
    }

    override func encode(to encoder: any Encoder) throws {
        var container = encoder.container(keyedBy: CodingKeys.self)
        try container.encode(y, forKey: .y)

        // This will effectively create a nested container for key "super", then return an `Encoder` wrapping it
        let superEncoder = container.superEncoder()
        try super.encode(to: superEncoder)
    }
}

You can see what this looks like round-tripping:

import Foundation

func roundTrip<T: Codable>(_ value: T) {
    let data = try! JSONEncoder().encode(value)
    let decoded = try! JSONDecoder().decode(T.self, from: data)
    print(value, "=>", String(data: data, encoding: .utf8)!, "=>", decoded)
}

roundTrip(Sup(42)) // main.Sup => {"x":42} => main.Sup
roundTrip(Sub(42, 500)) // main.Sub => {"y":500,"super":{"x":42}} => main.Sub

Happy to chat through the specific details of how this all works, but some references in the meantime:

itaiferber avatar Mar 17 '24 22:03 itaiferber

Thank you @itaiferber, this sample code will greatly help me 👍

@sipefree, I find it weird that you crash with "unkeyed encoding is not supported" or "single value encoding is not supported" in RecordEncoder.

This is different from the "Not implemented" fatal errors, that GRDB indeed raises when some feature is not implemented, i.e. missing (this is currently the case for nested containers). Actually I just noticed that there is already some support for super encoders 😅

Here, "not supported" implies a programmer error.

Indeed those errors are raised from RecordEncoder, whose role is to produce a database row, i.e. a list of (column, value) pairs. It is normal that types that use an unkeyed container, or a single-value container can not be encoded as a database row. There is no way to turn an array of values, or a single value, into a database row.

Since, as mentioned above, I can not reproduce your reported crash, I'm wondering whether your app has extended AttributedString with a protocol dedicated to records (that can feed a list of (column, value) pairs). If so, this is not correct. Attributed strings are values. If they need to conform to a GRDB protocol, it is DatabaseValueConvertible:

// INCORRECT
extension AttributedString: PersistableRecord { ... }

// CORRECT
extension AttributedString: DatabaseValueConvertible { ... }

// DatabaseValueConvertible enables requests like the one below:
let string = AttributedString(markdown: "This is my **first** post!")
let posts = try Post
    .filter(Column("text") == string) // Requires DatabaseValueConvertible
    .fetchAll(db)

If this is not the case, could you please provide an updated sample code that actually fails (or a precise description of your setup), or a stack trace of the crash? That would greatly help understanding what's wrong.

groue avatar Mar 18 '24 17:03 groue

Hello @sipefree. I'm closing the issue due to lack of feedback, and because I suspect the problem is in the application code, as explained in the previous comment.

Feel free to reopen if you have new information!

groue avatar Mar 25 '24 09:03 groue