RCNConfigExperiment is incorrectly flagging valid experiment JSON payloads as invalid

philviso-reverb commented 3 years ago

[REQUIRED] Step 1: Describe your environment

Xcode version: 12.2
Firebase SDK version: 7.0.0
Installation method: Carthage
Firebase Component: RemoteConfig

[REQUIRED] Step 2: Describe the problem

Since we upgraded from 6.18.0 to 7.0.0, we're seeing RemoteConfig log "Experiment payload could not be parsed as JSON." a bunch of times. I didn't count, but it's probably being logged once for each of our experiments. The result of this is that our A/B tests seem to have completely stopped getting updated values from the server.

Steps to reproduce:

It's easy to reproduce in RCNConfigExperiement.testLoadExperimentFromTable if you add an additional assertion to verify that the payloads were handled properly...

XCTAssertEqualObjects(_payloadsData, _configExperiment.experimentPayloads);

That assertion will fail before the fix and succeeds after the fix (see below)

Relevant Code:

I looked around in the FirebaseRemoteConfig source code and pinned down the issue (I have a PR ready to go as well).

As part of the change in this pull request (https://github.com/firebase/firebase-ios-sdk/pull/5890/files)

RCNConfigExperiment.m changed how it did JSON validation from...

NSError *error;
id experimentPayloadJSON = [NSJSONSerialization JSONObjectWithData:experiment options:kNilOptions error:&error];
if (!experimentPayloadJSON || error) {

to...

if (![NSJSONSerialization isValidJSONObject:experiment]) {

isValidJSONObject is meant to receive an NSArray or NSDictionary to test if that collection can be converted into JSON data (source). Passing it data causes it to return false every time. The previous implementation is the correct way to test if NSData can be converted to JSON.

google-oss-bot commented 3 years ago

I found a few problems with this issue:

I couldn't figure out how to label this issue, so I've labeled it for a human to triage. Hang tight.
This issue does not seem to follow the issue template. Make sure you provide all the required information.

paulb777 commented 3 years ago

@philviso-reverb Thanks for the bug report and investigation.

@christibbs Please review the referenced change from #5890

christibbs commented 3 years ago

@philviso-reverb Sorry for the delay. Just added #7269 which I assume is identical to your PR!

philviso-reverb commented 3 years ago

Thanks for the update! Yep, that's identical to what I did.

firebase / firebase-ios-sdk