Throw exception when projectID is missing from FIROptions #7725
Conversation
Coverage ReportAffected SDKs
Test Logs
|
|
FIS, an RC dependency, should already be checking the FIROptions, so it's not clear what this is changing. Also, this might be considered a breaking change, so assigning to @ryanwilson to approve from an API perspective. |
There was a problem hiding this comment.
LGTM.
Note, the change could be considered as a breaking, but most like is not. RC has a dependency on FIS which already throws on the missing project ID, so the exception won't be new for existing RC clients.
Please wait for @ryanwilson for the final approval on the API change.
Looking at the internal bug though, it seems like they're getting through even with the dependency on FIS so I'm concerned this is going to break someone. Do we know if that's an older version sending the data, maybe pre-FIS? @karenyz |
Note, FIS started throwing the exception on the missing project ID since last major version bump. |
|
I was hoping to fix-forward so we would stop sending invalid URLs moving forward, but since FIS is already throwing an exception it seems like this change doesn't make much of a difference (maybe makes it clearer for RC users?) @ryanwilson yeah the malformed requests are from older versions, so this change wouldn't affect those requests |
| @@ -38,6 +38,8 @@ - (FIRRemoteConfig *)remoteConfigForNamespace:(NSString *)remoteConfigNamespace | |||
| errorPropertyName = @"googleAppID"; | |||
| } else if (options.GCMSenderID.length == 0) { | |||
| errorPropertyName = @"GCMSenderID"; | |||
| } else if (options.projectID.length == 0) { | |||
There was a problem hiding this comment.
Since projectID is static configuration, validating it during Component initialization is intuitive, so I think checking here is optimal, and the only change we need to make.
| serverURLStr = [serverURLStr stringByAppendingString:_options.projectID]; | ||
| } else { | ||
| FIRLogError(kFIRLoggerRemoteConfig, @"I-RCN000070", | ||
| @"Missing `projectID` from `FirebaseOptions`, please ensure the configured " |
There was a problem hiding this comment.
I've recommended just asserting project ID as a required input in FIRRemoteConfigComponent.m. Given that, and since project ID is the only thing we're checking here, simplifying this class by removing this logging makes sense.
| @@ -167,6 +167,35 @@ - (void)testThrowsWithNilGCMSenderID { | |||
| XCTAssertThrows([component remoteConfigForNamespace:@"some_namespace"]); | |||
| } | |||
|
|
|||
| - (void)testThrowsWithEmptyProjectID { | |||
There was a problem hiding this comment.
The fact we're only asserting FIRRemoteConfigComponentTest.m throws supports the idea that validating inputs there is sufficient.
|
LGTM from an API standpoint since this is a no-op |
If
projectIDis missing the fetch request can't succeed, so we can fail the request before sending it to the server. This is causing some downstream issues (tracked in b/167621250)