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

Fix crash on invalid key path #230

Merged
merged 5 commits into from Feb 10, 2014
Merged

Fix crash on invalid key path #230

merged 5 commits into from Feb 10, 2014

Conversation

dcordero
Copy link
Contributor

@dcordero dcordero commented Feb 4, 2014

Fixed a crash when an invalid key path is indicated for a key in the JSONKeyPathsByPropertyKey method

@jspahrsummers
Copy link
Member

It's a little hard to tell what this is fixing. From the test, am I correct in understanding that it fixes a crash when a nested key path is specified in +JSONKeyPathsByPropertyKey, but the JSON contains a primitive instead of an object at some part of that path?

If so, we should fix this by stepping through the JSON dictionary manually, not catching exceptions. Exceptions in Objective-C are generally used to indicate unrecoverable errors, so catching them is rarely safe. (Mantle does so as a convenience for release code, and the value of even that has been debated.)

@dcordero
Copy link
Contributor Author

dcordero commented Feb 5, 2014

Yep it fix exactly that case. What exactly you mean with "stepping through the JSON dictionary manually" ? I mean, given the test case:

NSDictionary *values = @{
    @"username": @"foo",
    @"nested": @"bar",
    @"count": @"0"
    };

with a + JSONKeyPathsByPropertyKey like this:

+ (NSDictionary *)JSONKeyPathsByPropertyKey {
    return @{
        @"name": @"username",
        @"nestedName": @"nested.name",
        @"weakModel": NSNull.null,
    };
}

should we fill *error with a new error type an return nil from -(id)initWithJSONDictionary better than an Exception? or step over that key and continue with no error?

@jspahrsummers
Copy link
Member

I mean that we shouldn't be executing code which will cause NSDictionary to throw an exception.

Instead, we should follow a generalized version of these steps:

  1. Break apart nested.name into two components: nested and name
  2. Retrieve nested from the dictionary. If it's not a dictionary itself, error out.
  3. Retrieve name from the nested dictionary.

@dcordero
Copy link
Contributor Author

dcordero commented Feb 6, 2014

Modified fix to avoid sending an exception an returning an error instead

@jspahrsummers jspahrsummers self-assigned this Feb 10, 2014
jspahrsummers added a commit that referenced this pull request Feb 10, 2014
@jspahrsummers jspahrsummers merged commit cf3bb6e into Mantle:master Feb 10, 2014
@jspahrsummers
Copy link
Member

Thank you! I made some minor formatting changes in df03aa9, but otherwise this looks great! 🌟

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants