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

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

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?

@joeljfischer

@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

@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];

@mvyrko

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:

Mandatory properties that are nil unexpectedly are a major issue, and papering over them probably hurts OEM head unit testing.
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.

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

@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.

@joeljfischer

@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;
}

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.

@joeljfischer

@joeljfischer What do you think?

@joeygrover

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:

Do nothing - return unknown values for misconfigured head units, and allow crashes to happen if they happen.
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.
Implement individual fallback values for each mandatory parameter.
Assert at runtime (this only affects DEBUG, not RELEASE), and so would need to be combined with another solution for runtime.
Throw an exception (this affects DEBUG and RELEASE) and crash the app.
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.

@joeygrover

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

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;
}

@joeljfischer

@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.

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.

@mvyrko

@mvyrko please notify me when revisions have been made.

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

Navigation Menu

Repository navigation

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

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

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

Description

Activity

joeljfischer commented on Feb 22, 2019

mvyrko commented on Feb 22, 2019

mvyrko commented on Feb 25, 2019

joeljfischer commented on Feb 25, 2019

mvyrko commented on Feb 25, 2019

joeljfischer commented on Feb 25, 2019

mvyrko commented on Feb 26, 2019

jordynmackool commented on Feb 27, 2019

mvyrko commented on Mar 1, 2019

joeljfischer commented on Mar 1, 2019

joeljfischer commented on Mar 5, 2019

bilal-alsharifi commented on Mar 5, 2019

mvyrko commented on Mar 6, 2019

jordynmackool commented on Mar 6, 2019

jordynmackool commented on Mar 6, 2019

theresalech commented on Mar 19, 2019

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions