Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Non-String Subscriber Attributes Crashes App #499

Closed
ralphcode opened this issue Apr 24, 2021 · 5 comments
Closed

Non-String Subscriber Attributes Crashes App #499

ralphcode opened this issue Apr 24, 2021 · 5 comments
Labels
bug status: waiting-for-reply Issues that are waiting for a reply

Comments

@ralphcode
Copy link

ralphcode commented Apr 24, 2021

Describe the bug

When setting attributes if a numeric value is provided, the value is saved to UserDefaults as an NSNumber I.e.

Purchases.setAttributes({
     cbid: projUser.id   // = 14050
});

When called a second time the storeAttributeLocallyIfNeededWithKey method loads the NSNumber value but compares it as an NSString using isEqualToString, which crashes the device.

- (void)storeAttributeLocallyIfNeededWithKey:(NSString *)key
                                       value:(nullable NSString *)value
                                   appUserID:(NSString *)appUserID {
    NSString *valueOrEmpty = value ?: @"";
    NSString *_Nullable currentValue = [self currentValueForAttributeWithKey:key appUserID:appUserID];
    if (!currentValue || ![currentValue isEqualToString:valueOrEmpty]) {      // <!-- Here
        [self storeAttributeLocallyWithKey:key value:valueOrEmpty appUserID:appUserID];
    }
}

The app then needs to be deleted, before it can ever be used again as this occurs every time the app loads (as we set this value while loading our user objects).

Screen Shot 2021-04-24 at 2 59 11 pm

If the intention is to only support string values (which is fine by us), then I'd suggest either casting all inputs as a string before saving or, throwing an error so we know this is not allowed while developing.

We are now casting everything as a string before calling setAttributes but someone else might trip over this like we did;

Purchases.setAttributes({
     cbid: projUser.id + ''   // = "14050"
});

Thanks! 👍

  1. Environment
    1. Platform: iOS
    2. SDK version: Latest
    3. OS version: 14.5
    4. Xcode version: 10.15.7 (19H2)
    5. How widespread is the issue. Percentage of devices affected: All
  2. Debug logs that reproduce the issue
  3. Steps to reproduce, with a description of expected vs. actual behavior
  4. Other information (e.g. stacktraces, related issues, suggestions how to fix, links for us to have context, eg. stackoverflow, etc.)
@ralphcode ralphcode added the bug label Apr 24, 2021
@aboedo
Copy link
Member

aboedo commented Apr 26, 2021

Hi! 👋 Thanks for opening.

Our API does specify that the input type of that method is supposed to be:
NSDictionary <NSString *, NSString *> * in Objective-C, and
[String: String] in Swift

https://github.com/RevenueCat/purchases-ios/blob/develop/Purchases/Public/RCPurchases.h#L414

And in fact, if you attempt to send a non-string in Objective-C, you get a build warning:
image

And in Swift you get a compilation error:
image

However, these are Lightweight Generics in Objective-C, and depending on how you're sending over the values, the compiler might not catch the issue.

Could you shed some light on how you came across the issue? Was this while using purchases-ios directly, or through one of our plugins? (i.e. React-Native, Flutter, Cordova, Unity, etc)
If so, we might be able to improve our APIs on those to make sure that non-string values won't be passed in

@ralphcode
Copy link
Author

Hi @aboedo

Sorry, I should have specified. The bug/crashes occurs within purchase-ios however, this is made possible through the Cordova plugin cordova-plugin-purchases (and maybe other non-typed ones).

To replicate, you can run this in JavaScript with the Cordova plugin installed;

document.addEventListener("deviceready", onDeviceReady, false);

function onDeviceReady() {
    Purchases.setDebugLogsEnabled(true);
    Purchases.setup("public_sdk_key");
    Purchases.setAttributes({
         test: 1234
    });
}

This will then crash purchase-ios per the original post.

Do you want me to move this over to the Cordova repo? It could be bulletproofed from both the JavaScript (to force a string value, or throw an error) but also dynamic type checking in Objective-C.

Cheers,

@aboedo
Copy link
Member

aboedo commented Apr 27, 2021

@Ralpharoo that makes sense.
Yeah, I think we should enforce types in both sides, for the sake of having a better crash log at least.

We do have the type documented in cordova-plugin-purchases as { [key: string]: string | null } in typescript, but that won't translate over to javascript unfortunately.

@stale
Copy link

stale bot commented May 27, 2021

This issue has been automatically marked as stale due to inactivity. It will be closed if no further activity occurs. Please reach out if you have additional information to help us investigate further!

@stale stale bot added the status: waiting-for-reply Issues that are waiting for a reply label May 27, 2021
@stale stale bot closed this as completed Jun 3, 2021
@github-actions
Copy link

github-actions bot commented Aug 3, 2021

This issue has been automatically locked due to no recent activity after it was closed. Please open a new issue for related reports.

@github-actions github-actions bot locked and limited conversation to collaborators Aug 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug status: waiting-for-reply Issues that are waiting for a reply
Projects
None yet
Development

No branches or pull requests

2 participants