Repository navigation
fix(remoteconfig): keep data and source when copying a config value - #16825
Conversation
`FIRRemoteConfigValue` publicly conforms to `NSCopying`, but `-copyWithZone:` sent `initWithData:` to the new instance. The class only implements `initWithData:source:`, so copying any value raised an unrecognized-selector exception instead of returning a copy. Copy with the designated initializer instead, so the copy keeps both the backing data and the value source. Fixes #16804
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 crash and data loss issue when copying a FIRRemoteConfigValue by ensuring that the copy operation preserves both the value's data and its source. A unit test was added to verify this behavior. The review feedback suggests renaming the local variable copy in the new unit test to copiedValue to avoid potential confusion or shadowing with the Objective-C copy selector.
Use copiedValue instead of copy for the local, so that it doesn't read like the copy selector, as suggested in review.
Problem
FIRRemoteConfigValue(RemoteConfigValuein Swift) publicly conforms toNSCopying, but-copyWithZone:sentinitWithData:to the new instance. The class only implementsinitWithData:source:, so[value copy]raised an exception instead of returning a copy:The bug has been there since Remote Config was open-sourced. The SDK never copies values itself,
so only app code that copies a value hits it (
copy, acopyproperty, or otherNSCopyingAPIs).
Change
-copyWithZone:now uses the designated initializer,initWithData:_data source:_source. Thecopy keeps both the backing data and the value source.
testCopyPreservesDataAndSourcetoRCNConfigValueTest.m. It uses aDefaultsource sothe test also catches a copy that drops the source (it would read back as
Remote= 0).Testing
testCopyPreservesDataAndSourcefails:XCTAssertNoThrowcatches theunrecognized-selector
NSInvalidArgumentException.xcodebuild -scheme RemoteConfigUnit test -destination 'platform=macOS': 130 tests, 0 failures.Fixes #16804