SDL 0219 - Explicit returned type from NSDictionary(Store) category

[jordynmackool]

Hello SDL community,

The review of "SDL 0219 - Explicit returned type from NSDictionary(Store) category" begins now and runs through February 26, 2019. The proposal is available here:

https://github.com/smartdevicelink/sdl_evolution/blob/master/proposals/0219-ios-check-type.md

Reviews are an important part of the SDL evolution process. All reviews should be sent to the associated Github issue at:

#685

What goes into a review?

The goal of the review process is to improve the proposal under review through constructive criticism and, eventually, determine the direction of SDL. When writing your review, here are some questions you might want to answer in your review:

• Is the problem being addressed significant enough to warrant a change to SDL?
• Does this proposal fit well with the feel and direction of SDL?
• If you have used competitors with a similar feature, how do you feel that this proposal compares to those?
• How much effort did you put into your review? A glance, a quick reading, or an in-depth study?
  Please state explicitly whether you believe that the proposal should be accepted into SDL.

More information about the SDL evolution process is available at

https://github.com/smartdevicelink/sdl_evolution/blob/master/process.md

Thank you,
Jordyn Mackool

Program Manager - Livio
jordyn@livio.io

[joeljfischer]

This proposal doesn't really state how this change will solve the problem. Reading the code, the new method that all the RPC getters will use is:

- (nullable id)sdl_objectForName:(SDLName)name ofClass:(Class)classType {
    NSObject *obj = [self sdl_objectForName:name];
    if (obj == nil || [obj isKindOfClass:classType]) {
        return obj;
    } else {
        return [[classType alloc] initWithDictionary:(NSDictionary *)obj];
    }
}

But this means that if something is not of the expected class, which isn't really explained in this proposal either, then it will just be attempted to wrapped in the class object based on the expectation that it's a dictionary type. I think this may very well cause crashes. It appears in your PR that you modify that method to avoid this case:

- (nullable id)sdl_objectForName:(SDLName)name ofClass:(Class)classType {
    NSObject *obj = [self sdl_objectForName:name];
    if ([obj isKindOfClass:classType]) {
        return obj;
    } else if ([obj isKindOfClass:NSDictionary.class] && [classType instancesRespondToSelector:@selector(initWithDictionary:)]) {
        return [[classType alloc] initWithDictionary:(NSDictionary *)obj];
    } else if ([obj isKindOfClass:NSArray.class]) {
        NSArray *array = (NSArray *)obj;
        if ([array.firstObject isKindOfClass:classType]) {
            return array.firstObject;
        }
    }
    return nil;
}

I also think the nullability issue (returning nil when we're guaranteeing that the getter will return nonnull) may be significant, as it may introduce undefined conditions for developers. If they expect something will never be nil, and it is, we could cause additional crashes. This is probably pretty rare, and I'm not sure that it's a big deal, but it is certainly a concern. @kshala-ford do you have any thoughts on that?

[mvyrko]

@joeljfischer thank you your review.
I see your point, but, in my view, current situation is terrible.
In my project, I noticed at least 3 situation where

• NSDictionary instead of NSArray
• NSNull instead of NSArray
• NSNumber instead of NSString
  as far as I see it's too easy make mistake, if success tests have it.
  For example:

@interface SDLOnSystemRequest : SDLRPCNotification
....
@property (nullable, strong, nonatomic) NSString *url;
...

SDLOnSystemRequestSpec
SDLOnSystemRequest* testNotification = [[SDLOnSystemRequest alloc] init];
...
 testNotification.url = [@[@"www.google.com"] mutableCopy];
...
expect(testNotification.url).to(equal([@[@"www.google.com"] mutableCopy]));

As we see, client class gets NSArray instead of NSString.

About first part of your review, code was modified because it contains mistakes that lead to crash.

Agree that it's not ideal solution, it's compromise between huge changes and protection.
As for me I would remove NSDicationary+Store at all and use standard of mechanism of parsing objects(Decodable for example) or dynamic mapper of responses from dictionary.

About nullability, I think we together understand that nil would be in situation when category returns wrong type. In this case, I as developer prefer get nil instead of NSDictionary when I expect NSArray

[ghost]

I think we all agree to that we need to do something against illegal data types sent by head units. The question is rather how to solve the problem. Sort out illegal data and return nil solves most of the problems and works well for nullable properties it might cause other problems (and maybe lead to other kind of crashes).

As @joeljfischer mentioned, the most important piece of code from this proposal is:

- (nullable id)sdl_objectForName:(SDLName)name ofClass:(Class)classType {
    NSObject *obj = [self sdl_objectForName:name];
    if ([obj isKindOfClass:classType]) {
        return obj;
    } else if ([obj isKindOfClass:NSDictionary.class] && [classType instancesRespondToSelector:@selector(initWithDictionary:)]) {
        return [[classType alloc] initWithDictionary:(NSDictionary *)obj];
    } else if ([obj isKindOfClass:NSArray.class]) {
        NSArray *array = (NSArray *)obj;
        if ([array.firstObject isKindOfClass:classType]) {
            return array.firstObject;
        }
    }
    return nil;
}

It comes with a couple of issues:

1. Nullability isn't solved. In case of illegal data on nonnull properties we may need to have a mechanism in place returning some kind of fallback value?
2. Directly returning the dictionary isn't necessarily safe. The RPC struct subclass should not return an instancetype if the dictionary isn't valid. May be we should verify in initWithDictionary that the dict provided contains all mandatory parameters. Imagine you init an SDLSoftButtonCapabilities with
   a hard buttonCapabilities dict. @joeljfischer I'm not sure if this is required but it provides more protection.
3. Translated dicts are not stored. Therefore we may always reach the second else if. We should be reorganizing the code and store initialized dict
4. I don't understand the last else if. Why should we return the first object of an array? I couldn't see this case happening. It's rather the opposite when expecting an array but receiving a single object (Car may have not wrapped this single object into an array).

So here's a suggestion for non-array parameters:

- (nullable id)sdl_objectForName:(SDLName)name ofClass:(Class)classType {
    NSObject *obj = [self sdl_objectForName:name];

    // translate dictionaries to objects
    if ([obj isKindOfClass:NSDictionary.class] && [classType instancesRespondToSelector:@selector(initWithDictionary:)]) {
        obj = [[classType alloc] initWithDictionary:(NSDictionary *)obj];
        // update store so that the object isn't created multiple times
        [self sdl_setObject:obj forName:name];
    }

    if ([obj isKindOfClass:classType]) {
        return obj;
    }
    
    return nil;
}

I'll look at the array behavior but thought to provide feedback for this method now.

[ghost]

Okay... I had a look at the method for arrays and I believe the revised code below is kind of solving an issue where the vehicle returns an object (dict) instead of an array with objects. Additionally it adds some additional checks for types and sets the array with translated objects back to the store (so that retranslation doesn't happen)

- (nullable NSArray *)sdl_objectsForName:(SDLName)name ofClass:(Class)classType {
    NSObject *obj = [self sdl_objectForName:name];
    NSArray *array;

    if (!obj || [obj isKindOfClass:NSNull.class]) {
        return nil;
    } else if ([obj isKindOfClass:NSArray.class]) {
        array = (NSArray *)obj;
    } else {
        // in case the parameter content was an actual object instead of an array of objects
        array = @[obj];
    }

    // instant return for empty arrays.
    if (array.count == 0) {
        return array;
    }

    // translate arrays full of dictionaries to objects
    if ([array.firstObject isKindOfClass:NSDictionary.class] && [classType instancesRespondToSelector:@selector(initWithDictionary:)]) {
        // It's an array of dictionaries, make them into their class type
        NSMutableArray *newArray = [NSMutableArray arrayWithCapacity:array.count];
        for (NSDictionary *dict in array) {
            id newObject = [[classType alloc] initWithDictionary:dict];
            if (newObject) {
                [newArray addObject:newObject];
            }
        }
        array = [newArray copy];
        // update store so that the object isn't created multiple times
        [self sdl_setObject:array forName:name];
    }

    if ([array.firstObject isKindOfClass:classType]) {
        return array;
    } 

    return nil;
}

[mvyrko]

@kshala-ford as far as I see you suggest to add side effect to pure function, it's not good idea.

 // update store so that the object isn't created multiple times
        [self sdl_setObject:array forName:name];

[joeljfischer]

What about a separate method for nonnull parameters? Instead of nil in those cases, return:

• For string properties that are not supposed to be nil but are, return @"".
• For NSNumber properties return @0 (alternately, if it works, use the protocol conformance to use @NO, @0 or @0.0 as needed)
• For array properties, return @[]
• For object properties return [[Object alloc] init]

The main issue with this method is:

1. Mandatory properties that are nil unexpectedly are a major issue, and papering over them probably hurts OEM head unit testing.
2. Returning a boolean like @NO or an empty string may not match the head unit's state.

I also agree with @mvyrko that setting the object in the getter is probably not a great idea. Having the function be pure makes testing much easier.

[mvyrko]

as variant we can expand signature of method and add default value if it's needed
- (nullable id)sdl_objectForName:(SDLName)name ofClass:(Class)classType;
- (nonnull id)sdl_objectForName:(SDLName)name ofClass:(Class)classType defaultValue:(id)value;
where we can check that value type of classType

[ghost]

I also thought of doing this but dropped this idea because the app would always create fallback objects even if they are not needed. The caller method should be extended instead:

- (nonnull NSString *)myMandatoryParameter {
  NSString *param = [self sdl_objectForName:@"myMandatoryParameter"];
  if (!param) {
    return @"my mandatory value";
  }
  return param;
}

Okay... string may not be the best example but this code flow avoids creating fallback objects if they not needed.

I don't think we can agree to a specific value per type (0, empty array etc.) as they can still be illegal to the mobile API (minValue="1" for numbers, minSize="1" for arrays).

Regarding OEM testing, @joeljfischer you mentioned in another proposal to use NSAssert. We might want to use the assert abilities. For sure, we should log error message in case of illegal data.

@kshala-ford as far as I see you suggest to add side effect to pure function, it's not good idea.

I also agree with @mvyrko that setting the object in the getter is probably not a great idea. Having the function be pure makes testing much easier.

Isn't this a classic caching problem? I don't have a strong opinion to it but I thought it improves performance to apps. Not sure how compilers translate for loops like for (id object in response.buttonCapabilities).

[joeljfischer]

@kshala-ford I'm not sure what you mean by fallback values. The method I was thinking of was like:

- (nonnull id)sdl_objectForName:(SDLName)name ofClass:(Class)classType {
    NSObject *obj = [self sdl_objectForName:name];

    if (obj == nil) {
        if ([classType isMemberOfClass:[NSString class]]) { return @""; }
        if ([classType isMemberOfClass:[NSNumber class]]) { return @0; }
        if ([classType isKindOfClass:[SDLRPCStruct class]]) {
            return [[classType alloc] init];
        }
    }

    // translate dictionaries to objects
    if ([obj isKindOfClass:NSDictionary.class] && [classType instancesRespondToSelector:@selector(initWithDictionary:)]) {
        obj = [[classType alloc] initWithDictionary:(NSDictionary *)obj];
        // update store so that the object isn't created multiple times
        [self sdl_setObject:obj forName:name];
    }

    if ([obj isKindOfClass:classType]) {
        return obj;
    }
    
    return nil;
}

Extending the calling method would be a fairly massive task, in addition, there is no default value for mandatory parameters, so it doesn't seem to gain us anything.

I don't think we can agree to a specific value per type (0, empty array etc.) as they can still be illegal to the mobile API (minValue="1" for numbers, minSize="1" for arrays).

I agree, but is it better or worse than nil? And even if we gave them a "legal" value, is that worse since it's not truly a "good" value and the head unit is broken by returning an illegal value for a mandatory parameter?

Regarding OEM testing, @joeljfischer you mentioned in another proposal to use NSAssert. We might want to use the assert abilities. For sure, we should log error message in case of illegal data.

We definitely could assert and log an error, but I feel like we still don't know what we're going to return at runtime. All of our options are bad, because they can all lead to developer error or crashes in different cases. I'm not sure there is anything better than a bad answer here.

[mvyrko]

@joeljfischer Looks like you example still can return nil. I suggest to add assert for it

- (nonnull id)sdl_objectForName:(SDLName)name ofClass:(Class)classType {
    NSObject *obj = [self sdl_objectForName:name];

    if (obj == nil) {
        if ([classType isMemberOfClass:[NSString class]]) { return @""; }
        if ([classType isMemberOfClass:[NSNumber class]]) { return @0; }
        if ([classType isKindOfClass:[SDLRPCStruct class]]) {
            return [[classType alloc] init];
        }
    }

    // translate dictionaries to objects
    if ([obj isKindOfClass:NSDictionary.class] && [classType instancesRespondToSelector:@selector(initWithDictionary:)]) {
        obj = [[classType alloc] initWithDictionary:(NSDictionary *)obj];
        // update store so that the object isn't created multiple times
        [self sdl_setObject:obj forName:name];
    }

    if ([obj isKindOfClass:classType]) {
        return obj;
    }
    if([obj isKindOfClass: NSObject.class]) {
          return [[classType alloc] init];
    }
    NSAssert(nil, @"panic: undefined object");
    return nil;
}

[ghost]

@kshala-ford I'm not sure what you mean by fallback values. The method I was thinking of was like:

@joeljfischer @mvyrko I'm against this approach. The dictionary method should not return fallback values. As already mentioned there is not the value, that can always stand up as a default fallback. In the mobile API there are mandatory integer parameters, where 0 is out of range (position, majorVersion, width, rows and many more), hence can lead to a crash. The same thing can happen to empty strings if minlength is >0. Think of Enum elements as they are all string and must not be of empty string. Returning empty array also violates minsize. Last [[classType alloc] init] will produce objects with mandatory parameters set to nil. As an example AudioPassThruCapabilities has three mandatory parameters. I'm not sure what's the SDLC plan with designated initializers on RPC level...

Currently we have 644 optional and 343 mandatory paramters in our mobile API. We have only two options here:

1. Implement fallback values individually

Let's continue with the example of AudioPassThruCapabilities. The following method

- (SDLSamplingRate)samplingRate {
    return [store sdl_objectForName:SDLNameSamplingRate];
}

should change to

- (SDLSamplingRate)samplingRate {
    SDLSamplingRate *rate = [store sdl_objectForName:SDLNameSamplingRate ofClass:SDLSamplingRate.class];
    if (!rate) {
        return SDLSamplingRate16KHZ;
    }
    return rate;
}

This allows us to specify individual fallback values.

This leads to the following problem:

Let's stay with above example. How should we decide on a value? If the car cannot work with 16kHz but we tell an app it's 16kHz... The app may never be able to perform audio pass thru and the developer will not be able to understand why.

Instead we have to analyze further and understand the audio passthru usage. It's used in RegisterAppInterfaceResponse.audioPassThruCapabilities which is an array. As I proposed in my previous comment the method for arrays can handle initWithDictionary returning nil.

            id newObject = [[classType alloc] initWithDictionary:dict];
            if (newObject) {
                [newArray addObject:newObject];
            }

So AudioPassThruCapabilities initializer initWithDictionary should verify all mandatory parameters exist. If any doesn't it should log an error and return nil instead of an instance. Those nil will not be added to the array therefore the array may be empty. So the following method:

- (nullable NSArray<SDLAudioPassThruCapabilities *> *)audioPassThruCapabilities {
    return [parameters sdl_objectsForName:SDLNameAudioPassThruCapabilities ofClass:SDLAudioPassThruCapabilities.class];
}

should be changed to

- (nullable NSArray<SDLAudioPassThruCapabilities *> *)audioPassThruCapabilities {
    NSArray *value = [parameters sdl_objectsForName:SDLNameAudioPassThruCapabilities ofClass:SDLAudioPassThruCapabilities.class];
    if (value.count == 0) return nil;
    return value;

If none of the possible audio Passthru capabilities is valid we have to accept this fact and return nil. Why not empty array? Because the mobile API doesn't allow it (minsize="1").

I guess in some rare cases we could throw an Exception if there is just no fallback solution available. Let's take parkBrakeActive as an example. Do we really want to tell an app the park brake is not active (false) though we don't know it?

2. Accept the truth. Decouple mandatory from nullability and return nil.

Simple question: Do we want to handle up to 343 mandatory parameters individually? I think many of them are happy with @NO, @0 or @"" but is it still worth it?

May be it would be the best to decouple nullability from mandatory/optional flag. I'm afraid it's too late for this as it would require a breaking change and it increases code for the swift developers.

[jordynmackool]

Due the meeting time running out on 2019-02-26, this proposal will remain in review and be voted on during the next Steering Committee meeting on 2019-03-05.

[mvyrko]

@joeljfischer What do you think?

[joeljfischer]

For optional parameters, I think we're agreed that returning nil for misconfigured parameters is the right way to go. The exact code would need tests and review.

Forgive me if I'm wrong, but none of the parameters we're talking about are mandatory. I think we actually have six options for mandatory parameters:

1. Do nothing - return unknown values for misconfigured head units, and allow crashes to happen if they happen.

2. Implement fallback values per type - 0 for ints, 0.0 for floats, NO for booleans, empty strings for strings, empty arrays for arrays, init with nothing for classes.

3. Implement individual fallback values for each mandatory parameter.

4. Assert at runtime (this only affects DEBUG, not RELEASE), and so would need to be combined with another solution for runtime.

5. Throw an exception (this affects DEBUG and RELEASE) and crash the app.

6. Make a major version change and make all parameters nullable.

Here are my thoughts on these:

(1) may be the way to go for RELEASE builds if we can't agree on (2)

(2) I'm still a proponent of this solution. It prevents crashes and isn't a massive amount of work. When the value is out of range as mentioned by @kshala-ford, this is certainly not ideal and could lead to crashes, but is less likely to than (1).

(3) I'm strongly against this solution. It's not scalable and I don't think it would provide much more help than (2) would.

(4) is the barest minimum we should do. Head unit developers returning invalid objects for mandatory parameters is a massive issue that should never happen in the wild.

(5) may be too harsh, but also necessary, as I noted for (4), a misconfigured head unit returning bad data for mandatory parameters is a massive issue.

(6) can be dismissed out of hand. It's a massive increase in work and annoyance for swift developers, and a major version change.

So, my suggestion is the following:

• Assert in DEBUG
• Either return defaults per type, or leave it alone and keep the status quo.

Lastly, this change would also affect the android platform. @joeygrover @BrettyWhite @bilal-alsharifi we need your input on what to do with misconfigured mandatory parameters.

[ghost]

@joeljfischer

For optional parameters, I think we're agreed that returning nil for misconfigured parameters is the right way to go.

Agree

The exact code would need tests and review.

Can you please be more specific?

Forgive me if I'm wrong, but none of the parameters we're talking about are mandatory.

I'm not sure what parameters you mean? The whole point of this discussion is about mandatory parameters. Or do you mean the parameters of my example? audioPassThruCapabilities is an optional array, however the parameters of APT object's are mandatory. Returning empty strings for enum based parameters is a severe issue here. It is basically making nonnull enum based parameter nullable... Also using plain init can be catastrophic when returning objects with nonnull properties returning nil causing (swift) applications to crash. If you're not happy with APT capabilities as an example then have a look at one of the following arrays with minsize="1":

• OnTouchEvent with event that may be empty due to subsequent issues in TouchEvent
• TouchEvent with ts or c where both arrays should match in length. The initWithDictionary: of TouchEvent should validate both arrays
• LightControlData with lightState if the light status enum is invalid. If this is the case should we forward the notification at all?
• LightControlCapabilities with supportedLights. Strictly following (2) would return with an empty array.
• OnWaypointChange with wayPoints. How much sense does it make to return empty objects in an array? Should the notification be forwarded?
• ImageField with imageTypeSupported
• DiagnosticMessage response with messageDataResult This is very sensitive and should not be touched at all.

From your six options for mandatory parameters I don't see any reason following any of the options exclusively. They should be seen as tools to be used for certain cases. If you read my 1. Implement fallback values individually you will see that I was actually proposing (2), (3) and (5). (4) can be considered as an additional option. I believe, I didn't make it clear in the example.

I'm strongly against following only (2). We must not procrastinate this issue and must not mask, that there was a problem. It makes it way more difficult to identify the root cause when apps crash.

I grouped the mandatory parameters to types and found:

• 184 parameters of type struct or enum. 51 of them are "resultCode"
• 45 parameters of type Integer. 13 of them with minvalue > 0
• 74 parameters of type Boolean. 49 of them are success
• 9 parameters of type Float
• 31 parameters of type String

Using (2) could cause dramatic issues of any enum and any struct that has nonnull params. It makes it impossible to add exclusive behavior on critical parameters. My suggestion scales way better as you might think and allows to individually react whenever needed. I would agree to have sdl_objectForName extended to return an error or to mark that a default should be returned. Then we could add individual implementations as needed. Something like

- (nullable id)sdl_objectForName:(SDLName)name ofClass:(Class)classType error:(NSError **)error;

In case of an error the NSError object can return the fallback object but also the original data. With this we can add individual solutions whenever needed.

I suggest the more individual solution.

PS: added the search results for mandatory params
mandatory-params-grouped.txt

[joeljfischer]

Forgive me if I'm wrong, but none of the parameters we're talking about are mandatory.

I'm not sure what parameters you mean? The whole point of this discussion is about mandatory parameters. Or do you mean the parameters of my example?

I apologize for my lack of clarity, I meant the parameters that we have seen crashes on that have brought forth this discussion in the first place. If so, we don't need to do anything right now regarding mandatory parameters. Then we can fix the optional parameters and move forward. We only need to change our mandatory parameter strategy if we have bad head units in the wild providing bad values for mandatory parameters. Does that follow?

Returning empty strings for enum based parameters is a severe issue here.

I agree.

Also using plain init can be catastrophic when returning objects with nonnull properties returning nil causing (swift) applications to crash.

Well, in my example, those would return the default value as well. I agree it's not a good thing.

From your six options for mandatory parameters I don't see any reason following any of the options exclusively. They should be seen as tools to be used for certain cases. If you read my 1. Implement fallback values individually you will see that I was actually proposing (2), (3) and (5). (4) can be considered as an additional option. I believe, I didn't make it clear in the example.

I'm still against this. There are cases where providing any value just doesn't make sense, and implementing individual fallback values doesn't improve this (not to mention the massive amount of work). e.g. a touch event. What touch event would we provide? 0, 0? That could have severe consequences for the usability of the application.

I suppose I'm leaning toward doing nothing and asserting in DEBUG, or throwing an exception at runtime at this point.

Finally, if we are talking about altering the nullability or behavior of mandatory parameters, we need the solution to also fit Android, and the proposal would need to change accordingly. @joeygrover @BrettyWhite @bilal-alsharifi

[bilal-alsharifi]

In Android currently, we don't have Nullable/NotNull annotations for the the parameters that are returned from the getter methods even if that param is mandatory. However, we have a formatObject method that checks the type of class. If the passed class type doesn't match the object type, it will return null and shouldn't cause a crash at that point.

public Language getLanguage() {
	return (Language) getObject(Language.class, KEY_LANGUAGE);
}

public Object getObject(Class tClass, String key) {
	Object obj = parameters.get(key);
	return formatObject(tClass, obj);
}

protected Object formatObject(Class tClass, Object obj){
	if(obj == null){
		return null;
	} else if (tClass.isInstance(obj)) {
		return obj;
	} else if (obj instanceof String) {
		return getValueForString(tClass, (String) obj);
	} else if (obj instanceof Hashtable) {
		...
	} 
         ...
	return null;
}

[mvyrko]

@joeljfischer @kshala-ford
One of possible ways to resolve current problem it's remove NSDictionary+Store at all.
Checking of correct type move to response initializer(as Codable).
Another words if we try to create object like

@class SomeResponse: NSObject
@property (nonatomic) NSArray <String *> *items;
@end

from JSON like

{
    "items" : "value"
}

In this case, we shouldn't create SomeResponse at all. Now, we try to decide should items be nil or not, but looks like it doesn't matter due to json is incorrect for us and object can't be created.

[jordynmackool]

The Steering Committee voted to accept this proposal with revisions. The Revisions will include changing the method signature to the one that returns an error noted in this comment, so in the case of an error the NSError object could return the original data and an error message, or in the future, a fallback object for mandatory parameters.
The revisions will also need to state that optional parameters will log the error and assert in DEBUG, then return nil, and for mandatory parameters we will log the error, and assert in DEBUG, but will continue to return the bad object for now.

[jordynmackool]

@mvyrko please notify me when revisions have been made.

[theresalech]

Proposal has been updated to reflect revisions, and issue has been entered: iOS