FIRRemoteConfigValue.stringValue is not nullable

[myihsan]

Description

FIRRemoteConfigValue.stringValue is marked nullable in the header file but will never return null as the implement.

firebase-ios-sdk/FirebaseRemoteConfig/Sources/FIRConfigValue.m

Lines 43 to 46 in 60f9a33

	/// The string is a UTF-8 representation of NSData.
	- (NSString *)stringValue {
	return [[NSString alloc] initWithData:_data encoding:NSUTF8StringEncoding];
	}

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

[google-oss-bot]

I couldn't figure out how to label this issue, so I've labeled it for a human to triage. Hang tight.

[ncooke3]

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:

- - (NSString *)stringValue { 
+ - (nullable NSString *)stringValue { 

[myihsan]

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.

firebase-ios-sdk/FirebaseRemoteConfig/Sources/Public/FirebaseRemoteConfig/FIRRemoteConfig.h

Line 110 in 60f9a33

@property(nonatomic, readonly, nullable) NSString *stringValue;

I expected it to be solved like the below:

- (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 tonullable.

Before that, marking stringValue nonnull would be enough for me.

- @property(nonatomic, readonly, nullable) NSString *stringValue;
+ @property(nonatomic, readonly, nonnull) NSString *stringValue;

[ncooke3]

I think what is going on here is that Apple's documentation is misleading. Based on this discussion section and that Swift equivalent returns String?, I think - [NSString initWithData:encoding:] returns a nullable string, despite it's interface saying it doesn't.

[myihsan]

I think - [NSString initWithData:encoding:] returns a nullable string

You are right and the return value section 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:

NSLog(@"%@", [[NSString alloc] initWithData:[[NSData alloc] init] encoding:NSUTF8StringEncoding] == nil ? @"True" : @"False");
NSLog(@"%@", [[NSString alloc] initWithData:nil encoding:NSUTF8StringEncoding] == nil ? @"True" : @"False");

[ncooke3]

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?

[myihsan]

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)

[myihsan]

@ncooke3
I tried to solve this issue here, 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.

[paulb777]

While we may progress earlier, I'll mark this as Firebase 11 to make sure we consider before our next breaking change release.