Repository: firebase/firebase-ios-sdk (language: cpp)

Issue:
FIRRemoteConfigValue.stringValue is not nullable
### Description

FIRRemoteConfigValue.stringValue is marked `nullable` in the header file but will never return `null` as the implement.
https://github.com/firebase/firebase-ios-sdk/blob/60f9a33e7084482df67b48e16f315f4f7a6f5da9/FirebaseRemoteConfig/Sources/FIRConfigValue.m#L43-L46
It would be thankful to implement it as `nullable`, before that marking it `nonnull` would be enough.

### Firebase SDK Version

9.6.0

### Xcode Version

14.1

### Installation Method

CocoaPods

### Firebase Product(s)

Remote Config

### Targeted Platforms

iOS


Discussion / hints:
I couldn't figure out how to label this issue, so I've labeled it for a human to triage. Hang tight.
Hi @myihsan,

Thanks for opening an issue. Just so I understand the issue correctly, is the issue that the implementation's return type does not have the `nullable` attribute?

So something like the below diff would resolve things:
```diff
- - (NSString *)stringValue { 
+ - (nullable NSString *)stringValue { 
```
Hi @ncooke3,

Thanks for the confirmation.
If I understand the implementation correctly, adding the `nullable` attribute will not solve the issue since it's already `nullable` in the header file.
https://github.com/firebase/firebase-ios-sdk/blob/60f9a33e7084482df67b48e16f315f4f7a6f5da9/FirebaseRemoteConfig/Sources/Public/FirebaseRemoteConfig/FIRRemoteConfig.h#L110


I expected it to be solved like the below:
```objc
- (NSString *)stringValue {
  // if `_data` is nil or empty return nil.
  if (!_data) {
    return nil;
  }
  return [[NSString alloc] initWithData:_data encoding:NSUTF8StringEncoding];
}
```

But it will affect `numberValue` and `boolValue`, so it may need some discussion before changing them to`nullable`.

Before that, marking `stringValue` `nonnull` would be enough for me.

```diff
- @property(nonatomic, readonly, nullable) NSString *stringValue;
+ @property(nonatomic, readonly, nonnull) NSString *stringValue;
```
I think what is going on here is that Apple's documentation is misleading. Based on [this discussion section](https://developer.apple.com/documentation/foundation/nsstring/1416374-initwithdata#:~:text=Returns%20nil%20if%20the%20initialization%20fails%20for%20some%20reason%20(for%20example%20if%20data%20does%20not%20represent%20valid%20data%20for%20encoding).) and that [Swift equivalent returns `String?`](https://developer.apple.com/documentation/foundation/nsstring/1416374-init), I think `- [NSString initWithData:encoding:]` returns a nullable string, despite it's interface saying it doesn't. 
>  I think - [NSString initWithData:encoding:] returns a nullable string

You are right and [the return value section](https://developer.apple.com/documentation/foundation/nsstring/1416374-initwithdata#return_value) mentioned that.

> Returns nil if the initialization fails for some reason (for example if data does not represent valid data for encoding).

However, it will not return nil in the cases below:
- `[[NSString alloc] initWithData:[[NSData alloc] init] encoding:NSUTF8StringEncoding]`
- `[[NSString alloc] initWithData:nil encoding:NSUTF8StringEncoding]`

You can verify this behavior by:

```objc
NSLog(@"%@", [[NSString alloc] initWithData:[[NSData alloc] init] encoding:NSUTF8StringEncoding] == nil ? @"True" : @"False");
NSLog(@"%@", [[NSString alloc] initWithData:nil encoding:NSUTF8StringEncoding] == nil ? @"True" : @"False");
```
Thanks @myihsan, that's interesting. Sorry, so what exactly is the issue with the current behavior? Is it that you expected `- [FIRRemoteConfigValue stringValue]` to return `nil` when `_data` is `nil`?
Yes or no since I am unsure when `_data` became `nil`.
I expected `- [FIRRemoteConfigValue stringValue]` to return `nil` when no related config is set.

I want to distinguish the situations below:
- nil (there is no related config set)
- empty (there is a related config, but the value is empty)
@ncooke3 
I tried to solve this issue [here](https://github.com/firebase/firebase-ios-sdk/compare/master...myihsan:fix/FIRRemoteConfigValue-stringValue-nullable), but I realised that this will at least make dataValue `nullable` and this is an API change.
Should I open another issue focusing on distinguishing the config is set as empty or not set at all.
While we may progress earlier, I'll mark this as Firebase 11 to make sure we consider before our next breaking change release.

Fix patch:
diff --git a/FirebaseRemoteConfig/CHANGELOG.md b/FirebaseRemoteConfig/CHANGELOG.md
index 2a3024918c8..ff8a8a72050 100644
--- a/FirebaseRemoteConfig/CHANGELOG.md
+++ b/FirebaseRemoteConfig/CHANGELOG.md
@@ -1,4 +1,7 @@
-# 10.25.0
+# 11.0.0
+- [fixed] RemoteConfigValue stringValue is now `nonnull`. This may break some builds. (#10870)
+
+  # 10.25.0
 - [fixed] Fixed bug preventing Remote Config from working with a custom sqlite3
   dependency (#10884).
 
diff --git a/FirebaseRemoteConfig/Sources/FIRConfigValue.m b/FirebaseRemoteConfig/Sources/FIRConfigValue.m
index 8ab2d30d77a..e68d9c1312d 100644
--- a/FirebaseRemoteConfig/Sources/FIRConfigValue.m
+++ b/FirebaseRemoteConfig/Sources/FIRConfigValue.m
@@ -41,8 +41,8 @@ - (instancetype)init {
 }
 
 /// The string is a UTF-8 representation of NSData.
-- (NSString *)stringValue {
-  return [[NSString alloc] initWithData:_data encoding:NSUTF8StringEncoding];
+- (nonnull NSString *)stringValue {
+  return [[NSString alloc] initWithData:_data encoding:NSUTF8StringEncoding] ?: @"";
 }
 
 /// Number representation of a UTF-8 string.
diff --git a/FirebaseRemoteConfig/Sources/Public/FirebaseRemoteConfig/FIRRemoteConfig.h b/FirebaseRemoteConfig/Sources/Public/FirebaseRemoteConfig/FIRRemoteConfig.h
index 29cd12a5143..0a45617545f 100644
--- a/FirebaseRemoteConfig/Sources/Public/FirebaseRemoteConfig/FIRRemoteConfig.h
+++ b/FirebaseRemoteConfig/Sources/Public/FirebaseRemoteConfig/FIRRemoteConfig.h
@@ -138,7 +138,7 @@ typedef void (^FIRRemoteConfigFetchAndActivateCompletion)(
 NS_SWIFT_NAME(RemoteConfigValue)
 @interface FIRRemoteConfigValue : NSObject <NSCopying>
 /// Gets the value as a string.
-@property(nonatomic, readonly, nullable) NSString *stringValue;
+@property(nonatomic, readonly, nonnull) NSString *stringValue;
 /// Gets the value as a number value.
 @property(nonatomic, readonly, nonnull) NSNumber *numberValue;
 /// Gets the value as a NSData object.
diff --git a/FirebaseRemoteConfig/Swift/FirebaseRemoteConfigValueDecoderHelper.swift b/FirebaseRemoteConfig/Swift/FirebaseRemoteConfigValueDecoderHelper.swift
index d0cea378995..f2d56542760 100644
--- a/FirebaseRemoteConfig/Swift/FirebaseRemoteConfigValueDecoderHelper.swift
+++ b/FirebaseRemoteConfig/Swift/FirebaseRemoteConfigValueDecoderHelper.swift
@@ -35,7 +35,7 @@ struct FirebaseRemoteConfigValueDecoderHelper: FirebaseRemoteConfigValueDecoding
   }
 
   func stringValue() -> String {
-    return value.stringValue ?? ""
+    return value.stringValue
   }
 
   func dataValue() -> Data {
diff --git a/FirebaseRemoteConfigSwift/Tests/SwiftAPI/APITests.swift b/FirebaseRemoteConfigSwift/Tests/SwiftAPI/APITests.swift
index 7bf81c97a37..6665dd91258 100644
--- a/FirebaseRemoteConfigSwift/Tests/SwiftAPI/APITests.swift
+++ b/FirebaseRemoteConfigSwift/Tests/SwiftAPI/APITests.swift
@@ -188,12 +188,7 @@ class APITests: APITestBase {
 
     config.fetchAndActivate { status, error in
       XCTAssertNil(error, "Fetch & Activate Error \(error!)")
-
-      if let configValue = self.config.configValue(forKey: Constants.jedi).stringValue {
-        XCTAssertEqual(configValue, Constants.obiwan)
-      } else {
-        XCTFail("Could not unwrap config value for key: \(Constants.jedi)")
-      }
+      XCTAssertEqual(self.config.configValue(forKey: Constants.jedi).stringValue, Constants.obiwan)
       expectation.fulfill()
     }
     waitForExpectations()
@@ -204,13 +199,7 @@ class APITests: APITestBase {
     let expectation2 = self.expectation(description: #function + "2")
     config.fetchAndActivate { status, error in
       XCTAssertNil(error, "Fetch & Activate Error \(error!)")
-
-      if let configValue = self.config.configValue(forKey: Constants.jedi).stringValue {
-        XCTAssertEqual(configValue, Constants.yoda)
-      } else {
-        XCTFail("Could not unwrap config value for key: \(Constants.jedi)")
-      }
-
+      XCTAssertEqual(self.config.configValue(forKey: Constants.jedi).stringValue, Constants.yoda)
       expectation2.fulfill()
     }
     waitForExpectations()
@@ -239,13 +228,10 @@ class APITests: APITestBase {
 
     config.fetchAndActivate { status, error in
       XCTAssertNil(error, "Fetch & Activate Error \(error!)")
-
-      if let configValue = self.config.configValue(forKey: Constants.sith).stringValue {
-        XCTAssertEqual(configValue, Constants.darthSidious)
-      } else {
-        XCTFail("Could not unwrap config value for key: \(Constants.sith)")
-      }
-
+      XCTAssertEqual(
+        self.config.configValue(forKey: Constants.sith).stringValue,
+        Constants.darthSidious
+      )
       expectation2.fulfill()
     }
     waitForExpectations()
@@ -258,12 +244,7 @@ class APITests: APITestBase {
 
     config.fetchAndActivate { status, error in
       XCTAssertNil(error, "Fetch & Activate Error \(error!)")
-
-      if let configValue = self.config.configValue(forKey: Constants.jedi).stringValue {
-        XCTAssertEqual(configValue, Constants.obiwan)
-      } else {
-        XCTFail("Could not unwrap config value for key: \(Constants.jedi)")
-      }
+      XCTAssertEqual(self.config.configValue(forKey: Constants.jedi).stringValue, Constants.obiwan)
       expectation.fulfill()
     }
     waitForExpectations()


Test patch: