Make Parsing Methods Type Safe

[ankit-thanekar007]

PrebidSDK Expects Passthrough as an Array on auction response. But it's possible for it to come in as a dictionary from Prebid Server.

Consider this as an example

    NSMutableDictionary *jsonDictionary = [NSMutableDictionary new];
    
    jsonDictionary[@"passthrough"] = @{@"Test1" : @"Passthrough", @"Test2" : @"Crash"};
    
    NSArray<PBMJsonDictionary*> * const passthroughDics = jsonDictionary[@"passthrough"];

    if (passthroughDics) {
        for(PBMJsonDictionary *nextDic in passthroughDics) {
            if ([nextDic isKindOfClass:[NSString class]]) {
                NSLog(@"is String");
            }else if ([nextDic isKindOfClass:[NSArray class]]) {
                NSLog(@"is Array");
            }else if ([nextDic isKindOfClass:[PBMJsonDictionary class]]) {
                NSLog(@"is PBMJsonDictionary");
            }
        }
    }

Here jsonDictionary[@"passthrough"]; is coming in as a dictionary.

When this line executes
NSArray<PBMJsonDictionary*> * const passthroughDics = jsonDictionary[@"passthrough"];
it sets passthroughDics as a NSDictionary

Due to which nextDic in for(PBMJsonDictionary *nextDic in passthroughDics) { is set as NSString

The next line in PrebidSDK is

PBMORTBExtPrebidPassthrough * const nextPassthrough = [[PBMORTBExtPrebidPassthrough alloc] initWithJsonDictionary:nextDic];

which calls init for PBMORTBExtPrebidPassthrough where it executes this line _type = jsonDictionary[@"type"];

This line tries to subscript type inside a NSString which leads to this crash

-[__NSCFString objectForKeyedSubscript:]: unrecognized selector sent to instance 0x300dad3b0

Approach --> We make sure to check for the expected class before accessing or initializing the objects.

[ankit-thanekar007]

@jsligh @YuriyVelichkoPI

[YuriyVelichkoPI]

Hi @ankit-thanekar007!

I've revisited everything related to passthrough, and your change request looks highly relevant. But I think that it can be implemented in a bug-fix way. I mean that according to the PBS docs the passthrough should be an object but not the array.

So, the current implementation looks like a bug. I don't remember why we decided to use an array; maybe it was because of a lack of understanding of the design.

Likely, I see that the current utilization of the passthrough is only in the POC stage. On iOS, it works under the #if DEBUG and is not fully supported.

So, I'd ask you to reimplement the patch by removing the support of the array and relying only on the object.

If to talk about the current functionality, you can change it to support the following config:

{
  "passthrough": {
    "prebidmobilesdk": {
      "sdkconfiguration": {
        "cftbanner": 42,
        "cftprerender": 4242
      },
      "adconfiguration": {
        "maxvideoduration": 11,
        "ismuted": true,
        "closebuttonarea": 0.1,
        "closebuttonposition": "topleft"
      }
    }
  }
}

Or you can ping me, and I'll do it on your branch.

We shouldn't break the current behavior since the feature has not been released. However, it is important to log a respective error message if the passthrough has an unsupported format.

Please let me know what you think about this approach and if you have the bandwidth to implement it. Will you be able to provide the same fixes for Android?

[ankit-thanekar007]

Hi @ankit-thanekar007!

I've revisited everything related to passthrough, and your change request looks highly relevant. But I think that it can be implemented in a bug-fix way. I mean that according to the PBS docs the passthrough should be an object but not the array.

So, the current implementation looks like a bug. I don't remember why we decided to use an array; maybe it was because of a lack of understanding of the design.

Likely, I see that the current utilization of the passthrough is only in the POC stage. On iOS, it works under the #if DEBUG and is not fully supported.

So, I'd ask you to reimplement the patch by removing the support of the array and relying only on the object.

If to talk about the current functionality, you can change it to support the following config:

{
  "passthrough": {
    "prebidmobilesdk": {
      "sdkconfiguration": {
        "cftbanner": 42,
        "cftprerender": 4242
      },
      "adconfiguration": {
        "maxvideoduration": 11,
        "ismuted": true,
        "closebuttonarea": 0.1,
        "closebuttonposition": "topleft"
      }
    }
  }
}

Or you can ping me, and I'll do it on your branch.

We shouldn't break the current behavior since the feature has not been released. However, it is important to log a respective error message if the passthrough has an unsupported format.

Please let me know what you think about this approach and if you have the bandwidth to implement it. Will you be able to provide the same fixes for Android?

Hi @YuriyVelichkoPI,

This is also present in PrebidMobile/PrebidMobileRendering/Prebid/PBMCore/ORTB/Prebid/PBMORTBBidResponseExtPrebid.m and PrebidMobile/PrebidMobileRendering/Prebid/PBMCore/ORTB/Prebid/PBMORTBBidExtPrebid.m, which are part of Prod.

The issue occurred when the response included the passthrough field, causing a crash due to the mismatch between the expected JSON dictionary and an array of JSON dictionaries. In this PR, I’m converting the received dictionary into an array of dictionaries to address the issue without affecting the expected SDK behavior.

If this is indeed a bug, we may need another PR to address it more formally as a bug fix. This PR, however, serves as a patch to address the current behavior.

[YuriyVelichkoPI]

LGTM

[YuriyVelichkoPI]

@ankit-thanekar007 is this issue present on Android?

[ankit-thanekar007]

@ankit-thanekar007 is this issue present on Android?

Thanks @YuriyVelichkoPI !
Android doesn't crash. Though I'm not sure if the Dictionary vs Array bug is present there.