Repository navigation
Conversation
`Firestore.Decoder.decode(_:from:in:)` stored the document reference in the decoder's public `userInfo` and never removed it. When an app reused a decoder, a later call with `in: nil`, or a call to `decode(_:from:)`, populated `@DocumentID` properties with the ID of the previously decoded document instead of failing like a new decoder does. Every decode with a reference also mutated the caller's `userInfo`. The reference is now added to a per-call copy of `userInfo`, so the decoder's own `userInfo` is never modified and decoding without a reference behaves the same for new and reused decoders. Fixes #16789
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. |
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request fixes a bug in Firestore.Decoder where a reused decoder incorrectly persisted and reused a DocumentReference from a previous decoding call to populate @DocumentID properties in subsequent calls. This is resolved by scoping the userInfo modification to the individual decode call. Unit tests have been added to verify this behavior. The reviewer suggested using the parameter t instead of T.self in the decode call on line 93 for consistency with other overloads.
…_:from:in:) Decode the metatype that the caller passes in, like decode(_:from:) does, instead of T.self, as suggested in review. The only internal caller passes T.self, so its behavior doesn't change.
Problem
Firestore.Decoder.decode(_:from:in:)saved theDocumentReferencein the decoder's publicuserInfoand never removed it.@DocumentIDreads the reference fromuserInfo. So whenan app reuses one
Firestore.Decoderinstance, a later call within: nil, or a call todecode(_:from:), filled@DocumentIDwith the ID of the previously decoded document. It didnot fail the way a new decoder does. A side effect was that every referenced decode also
modified the caller's
userInfo.This regressed in Firebase 10 (#9465). The 9.x decoder kept the reference scoped to the call.
Change
decode(_:from:in:)now adds the reference to a local copy ofuserInfoand passes thatcopy to a private
decode(_:from:userInfo:)helper.decode(_:from:)uses the same helperwith the decoder's own
userInfo.userInfois no longer modified. User-provided entries are stillpassed through.
decoders: decoding a type with
@DocumentIDthrowsFirestoreDecodingError.decodingIsNotSupported, as a new decoder already did.# Unreleased.Testing
New Swift Testing suite in the SPM unit target:
Firestore/Swift/Tests/Unit/Codable/FirestoreDecoderTests.swift.reusedDecoderDoesNotReusePreviousDocumentReference: decodes with a reference, thendecodes on the same decoder with
in: niland withdecode(_:from:).decodingWithDocumentReferenceDoesNotModifyUserInfo: checks that customuserInfoispassed through and left unchanged.
Model(name: "second", docId: "first")forin: niland fordecode(_:from:), anddecoder.userInfo.countwas 2.FirebaseFirestoreTestsscheme on macOS passes(8 tests in 3 suites).
Firestore/Swift/Tests/Codabletests into the SPM test target with anFSTTestDocRefshimand ran them in Swift 5 mode. All 43 of them pass with the fix.
Fixes #16789