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

JSONRequiredError while Decoding Generic Records using singleValueContainer #1572

Closed
sroebert opened this issue Jul 12, 2024 · 7 comments
Closed

Comments

@sroebert
Copy link
Contributor

sroebert commented Jul 12, 2024

What did you do?

After #1565 was fixed, I put back my code to use the singleValueContainer. Unfortunately now I get an error when decoding a row in the same way.

struct Wrapper<Model: Codable>: Codable {
    var model: Model
    var otherValue: String

    init(from decoder: any Decoder) throws {
        let container = try decoder.container(keyedBy: CodingKeys.self)
        otherValue = try container.decode(String.self, forKey: . otherValue)
        
        let singleValueContainer = try decoder.singleValueContainer()
        model = try singleValueContainer.decode(Model.self)
    }
}

For now the workaround is to simply do:

model = try Model(from: decoder)

What did you expect to happen?

I expected decoding to work correctly.

What happened instead?

Trying to decode a generic FetchableRecord using singleValueContainer results in a JSONRequiredError.

Looking at the code in GRDB, the issue happens in _RowDecoder in FetchableRecord+Decodable.swift.

Here it returns a ColumnDecoder, apparently because there is a case where someone is trying to "Decoding an array of scalars". Instead it should most likely return a specific version of SingleValueDecodingContainer that would use a _RowDecoder when trying to decode a Decodable, similar to what happens when encoding.

Environment

GRDB flavor(s): GRDB
GRDB version: 6.28.0
Installation method: SPM
Xcode version: 16 beta 3
Swift version: 6
Platform(s) running GRDB: All
macOS version running Xcode: 14.5

Demo Project

@groue
Copy link
Owner

groue commented Jul 12, 2024

Hello @sroebert,

I understand. Do you think it would be possible for you to validate this wrapper model with plain JSON encoding?

I mean: is Wrapper, as it is, able to encode and decode a flat JSON object that contains all properties of Model, plus otherValue?

@sroebert
Copy link
Contributor Author

Hey, yes it is working normally with JSON. I have used this technique on several projects before, so it was quite common for me to do this.

I can have a look at making a PR for fixing this, doesn't seem too difficult. I was just wondering if you have an example of the case that is mentioned in the comments Decoding an array of scalars?

@groue
Copy link
Owner

groue commented Jul 12, 2024

Hey, yes it is working normally with JSON. I have used this technique on several projects before, so it was quite common for me to do this.

👍

I can have a look at making a PR for fixing this, doesn't seem too difficult.

That would be appreciated 😊

I was just wondering if you have an example of the case that is mentioned in the comments Decoding an array of scalars?

Yes. When I throw an error instead of returning a ColumnDecoder, one test fails: AssociationPrefetchingCodableRecordTests.testIncludingAllHasManyScalar. It is the last use case in the doc of including(all:)

@groue
Copy link
Owner

groue commented Jul 12, 2024

Instead it should most likely return a specific version of SingleValueDecodingContainer that would use a _RowDecoder when trying to decode a Decodable, similar to what happens when encoding.

Your acute look is very welcome as well. Please don't hesitate spotting the flaws in the current implementation 🙏 I just wish we would not introduce any breaking change (decoding more values is great, but breaking user code that currently works would not be nice).

@sroebert
Copy link
Contributor Author

Yes totally understand and agree, that's why I was asking about the existing comment on the scalars. But if there is an existing test that fails, that will make it a lot easier for me to figure out what to do. I will have a look at creating a PR.

@groue
Copy link
Owner

groue commented Jul 12, 2024

Thanks @sroebert. CONTRIBUTING will help you getting started.

EDIT: you can actually directly open Package.swift. This is enough for the topic we're discussing.

@groue
Copy link
Owner

groue commented Jul 22, 2024

Fixed by #1574, resolved in 6.29.0

@groue groue closed this as completed Jul 22, 2024
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

No branches or pull requests

2 participants