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

BUG DECODING KEYS FROM FIRESTORE. #12729

Open
jesus-mg-ios opened this issue Apr 9, 2024 · 16 comments
Open

BUG DECODING KEYS FROM FIRESTORE. #12729

jesus-mg-ios opened this issue Apr 9, 2024 · 16 comments

Comments

@jesus-mg-ios
Copy link

Description

The firestore decoder keep in mind _JSONStringDictionaryDecodableMarker.Type. So for maps, if we have something like this:

{ "name": {"KEY": VALUE } }

and we decode it with a codable structure with a property of type [String: String] it works.

The Behavior using [EnumStringRawValue: String] does fail into:

typeMismatch : 2 elements
    - .0 : Swift.Array<Any>
    ▿ .1 : Context
      ▿ codingPath : 1 element
        - 0 : CodingKeys(stringValue: "KEY", intValue: nil)
      - debugDescription : "Expected to decode Array<Any> but found a dictionary instead."
      - underlyingError : nil

Because in this case key is always represented as a String

extension Dictionary : _JSONStringDictionaryDecodableMarker where Key == String, Value: Decodable {
  static var elementType: Decodable.Type { return Value.self }
} 

The expected behavior is to decode a map with an enum string raw representable if it can fit on the string raw representable cases.

Reproducing the issue

Add a document with a map in Firestore.

Try to decode it with document.data() passing as an argument your struct or class metatype. Inside of the struct or class metatype define a map with StringRawValueEnum as key.

Firebase SDK Version

10.23.0

Xcode Version

15.2

Installation Method

Swift Package Manager

Firebase Product(s)

Firestore

Targeted Platforms

iOS

Relevant Log Output

No response

If using Swift Package Manager, the project's Package.resolved

Expand Package.resolved snippet
Replace this line with the contents of your Package.resolved.

If using CocoaPods, the project's Podfile.lock

Expand Podfile.lock snippet
Replace this line with the contents of your Podfile.lock!
@google-oss-bot
Copy link

I couldn't figure out how to label this issue, so I've labeled it for a human to triage. Hang tight.

@jesus-mg-ios
Copy link
Author

jesus-mg-ios commented Apr 9, 2024

Workaround: Create a custom init(from decoder: Decoder), decode it with the container using [String:String] and then reduce Into your [StringRawValueEnum:String]

@tom-andersen
Copy link
Contributor

Hi @jesus-mg-ios!

I tried to reproduce your problem with no luck.

How is your EnumStringRawValue defined?

I constructed a local test with:

enum CharacterAlignment: Codable {
    case good
    case neutral
    case evil
  }

And it seems to work for me.

Could you provide a little more code explaining the problem?

@jesus-mg-ios
Copy link
Author

Thanks @tom-andersen the example should look like this:

struct YourCustomStructToDecode: Codable {
    var property1: String?
    var property2: [CharacterAlignment: Bool]?
}

And the CharacterAlignment must be String as RawRepresentable Value

enum CharacterAlignment: String, Codable  {
    case good
    case neutral
    case evil
  }
 

@tom-andersen
Copy link
Contributor

tom-andersen commented Apr 10, 2024

I wrote the following test, and it passes in my enironment:

  func testEnumMap() throws {
    enum CharacterAlignment: String, Codable {
        case good
        case neutral
        case evil
      }
    struct Model: Codable {
      var name: String
      var alignmentLaugh: [CharacterAlignment: String]
    }
    let model = Model(
      name: "name",
      alignmentLaugh: [
        CharacterAlignment.good: "Hohoho",
        CharacterAlignment.neutral: "Hahaha",
        CharacterAlignment.evil: "Hehehe"
      ]
    )

    let docToWrite = documentRef()

    for flavor in allFlavors {
      try setData(from: model, forDocument: docToWrite, withFlavor: flavor)

      let data = try readDocument(forRef: docToWrite).data(as: Model.self)

      XCTAssertEqual(data.alignmentLaugh, [
        CharacterAlignment.good: "Hohoho",
        CharacterAlignment.neutral: "Hahaha",
        CharacterAlignment.evil: "Hehehe"
      ], "Failed with flavor \(flavor)")
    }

    disableNetwork()
    defer {
      enableNetwork()
    }

    try docToWrite.setData(from: model)
    let data = try readDocument(forRef: docToWrite).data(as: Model.self)
    XCTAssertEqual(data.alignmentLaugh, [
      CharacterAlignment.good: "Hohoho",
      CharacterAlignment.neutral: "Hahaha",
      CharacterAlignment.evil: "Hehehe"
    ], "Failed with flavor  offline docRef")
  }

What are you doing different?

@jesus-mg-ios
Copy link
Author

jesus-mg-ios commented Apr 10, 2024

The only thing that I'm doing differently is getting data from the snapshot listener, also not using a local simulator (that I don't know if it's your case) and using optional properties of the model (but as far as I see, it doesn't depend on using decodeIfPresent or not). The model is created from the firebase admin/console with firebase database (not realtime database), not from local, so maybe it is something to consider, but the structure decode works with the workaround, so, it's a factor that shouldn't affect it.

@tom-andersen
Copy link
Contributor

Do you mind sharing a repro?

@mortenbekditlevsen
Copy link
Contributor

It sounds to me like you're getting a default behavior of Codable that is a bit unfortunate.
Try with a JSONDecoder and see if you don't get the exact same behavior.
A fix/workaround was proposed and implemented with Swift Evolution proposal SE-0320 by me.

https://github.com/apple/swift-evolution/blob/main/proposals/0320-codingkeyrepresentable.md

You need to conform your key type to CodingKeyRepresentable. That should do it.

Please let me know if it helps!

@jesus-mg-ios
Copy link
Author

jesus-mg-ios commented Apr 14, 2024

Thanks for sharing this valuable information @mortenbekditlevsen. I propose to extend the current implementation of Firebase decoder/encoder. In firebase a Map only can do it by using keys as string, because is a json representation constraint. So we could manage it using something like this:

extension Dictionary : _JSONStringDictionaryDecodableMarker where Key == RawRepresentable, Value: Decodable, Key.RawValue == String {
  static var elementType: Decodable.Type { return Value.self }
} 

Thoughts @tom-andersen

@mortenbekditlevsen
Copy link
Contributor

My two cents: the change would affect any user importing Firebase. And even though the existing behavior seems like a bug (and I suspect an oversight in the Codable system from the start), then we can't definitely say that no people are relying on the behavior. The change would be behavior breaking for them.

So as annoying as the issue is, it's just one of those things that we probably can't ever fix...

@jesus-mg-ios
Copy link
Author

Why would it be a breaking change? @mortenbekditlevsen In this case, I think that would depend on the code position of decoding or encoding, adding a step more for people who do fail into failing decoding/coding error

@mortenbekditlevsen
Copy link
Contributor

If people are relying on the fact that any non-String key in a Dictionary by default will encode to and decode from an array of pairs of keys and values, then importing Firebase will change that behavior. And not just for encodings to and from Firestore or the Real-time database, but for all other Codable purposes too.

@jesus-mg-ios
Copy link
Author

I disagree because the people who start to decode from an array of keys and values, will not affected by the change, nor people using Firestore or real-time databases. As you said "will encode to and decode from an array of pairs of keys and values", so -> they will not encode to and decode from a dictionary, defining their data to decode as an Array, different case managed.

Thoughts @tom-andersen

@mortenbekditlevsen
Copy link
Contributor

I'll try explaining myself a little better.
The reason that @tom-andersen is not seeing the issue is that it encodes and decodes just fine. It works as designed - and it works similarly to how JSONEncoder and JSONDecoder works.

It just doesn't encode to the format you expect (and I totally get why you expect that).

I modified Tom's test to a small playground example:

import Foundation

enum CharacterAlignment: String, Codable {
    case good
    case neutral
    case evil
}
struct Model: Codable {
    var name: String
    var alignmentLaugh: [CharacterAlignment: String]
}
let model = Model(
    name: "name",
    alignmentLaugh: [
        CharacterAlignment.good: "Hohoho",
        CharacterAlignment.neutral: "Hahaha",
        CharacterAlignment.evil: "Hehehe"
    ]
    )


let encoder = JSONEncoder()

let data = try! encoder.encode(model)
print(String(data: data, encoding: .utf8)!) // (1)

let decoder = JSONDecoder()

let other = try! decoder.decode(Model.self, from: data)
print(other)

The print statement at (1) gives:
"{"name":"name","alignmentLaugh":["evil","Hehehe","good","Hohoho","neutral","Hahaha"]}\n"

So: a list of alternating keys and values - and not what you expect (you expect the more natural map representation). But it is still a valid serialization, and it still decodes completely fine.

So if you apply your suggested change (similarly to at some point starting to conform your CharacterAlignment to CodingKeyRepresentable), then any previously stored data will fail to decode, which can be seen when you perform the change 'manually':

import Foundation

enum CharacterAlignment: String, Codable, CodingKeyRepresentable {
    case good
    case neutral
    case evil
}

struct Model: Codable {
    var name: String
    var alignmentLaugh: [CharacterAlignment: String]
}
let model = Model(
    name: "name",
    alignmentLaugh: [
        CharacterAlignment.good: "Hohoho",
        CharacterAlignment.neutral: "Hahaha",
        CharacterAlignment.evil: "Hehehe"
    ]
    )

let encoded = #"{"name":"name","alignmentLaugh":["evil","Hehehe","good","Hohoho","neutral","Hahaha"]}\n"#

let decoder = JSONDecoder()

do {
    let other = try decoder.decode(Model.self, from: encoded.data(using: .utf8)!)
    print(other)
} catch {
    print(error)
}

In this example, I have now made the key support encoding to and decoding from a map. I try decoding the previously encoded data which now fails.

That is what I mean by a behavior breaking change.
You could have many people storing a lot of data using one serialization strategy, and then after updating to a new version of firebase (that includes your suggested change), all of that data would suddenly fail to decode.

Another note is that the current behavior aligns with the behavior of JSONEncoder and JSONDecoder.
So still: while the original issue was likely due to something being forgotten in an early implementation of the Codable system, the language supported workaround is conforming your keys to CodingKeyRepresentable, so that you as a user can decide when to opt in to the 'improved' coding behavior...

@jesus-mg-ios
Copy link
Author

jesus-mg-ios commented Apr 15, 2024

Okay, now I get you. Your case said that someone could be trying to decode an array of things (that is prone to error because it must be generated with even elements) to a dictionary, and it's fine because it decodes and codes well. The thing that I didn't get is why the decode fails on my side, using the Firestore decoder, when data is populated from the admin console. Even if I change to [String: String] instead of [CharacterAlignment: String], it works like a charm.

The key point is. Why does the Custom Firebase Decoder say "Expected to decode Array but found a dictionary instead." when I'm creating a a model with a dictionary.

@mortenbekditlevsen
Copy link
Contributor

The behavior is inherited from Apple's JSONDecoder.

The reason is historical, but in the lines of:
You can use anything as Dictionary keys as long as it can be hashed.
But if it's not a String then we can't encode it to (and ditto decode from) a 'keyed container' (which requires keys to be Strings or Ints I believe).

Thus, the strategy was: if the key is not a String, we encode keys and values alternately into an unkeyedContainer (array/list).

This is why you see that it expects the (encoded) data to be in the shape of an array. When decoding.

At the time they missed the case: but what if your key still encodes as a String. Specifically: what if it's RawRepresentable as a String.

And that's what SE-0320 tries to deal with. But it's too late to make a global fix. It must be done case by case, which is a real shame.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants