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

Already on GitHub? Sign in to your account

Fix a crash when receving something different than a NSDictionary #197

Merged
merged 6 commits into from Dec 12, 2013

Conversation

Projects
None yet
4 participants
Contributor

dcordero commented Dec 9, 2013

Fix a crash when receving something different than a NSDictionary to build the model, which can easily happen if the json structure changes

David Cordero Fix a crash when receving something different than a NSDictionary to
build the model, which can easily happen if the json structure changes
7c0f041
Contributor

dcordero commented Dec 9, 2013

It can happen in runtime with no warning, if you were getting a piece of the json to model with Mantle and the format of the json changes, you could finally send a NSString to Mantle and it will crash

Owner

robb commented Dec 9, 2013

👍 Can you throw in a test for good measure?

FYI: In 2.0, we'll have ways to bubble up the error (see #153).

Contributor

dcordero commented Dec 9, 2013

Test added.

I am looking forward the release of the 2.0 version which contains a lot of great new things.

Member

dcaunt commented Dec 9, 2013

Can this return an error in 1.x as well as 2.0? Otherwise I'm going to get a nil model it won't be immediately clear why.

Contributor

dcordero commented Dec 9, 2013

I just gave it the same behaviour it has if you send a nil pointer to Mantle.

I don't want to modify so much the behaviour of the method in the current version since the error management is being solved in the 2.0 version.

Owner

robb commented Dec 9, 2013

yeah, not sure there is a way in 1.x to bubble up the errors :-/

Member

dcaunt commented Dec 9, 2013

Ah yes, it won't actually bubble up. 🐢

2.0 is going to be awesome.

Owner

robb commented Dec 9, 2013

@dcordero could it be that you forget to pull upstream master after #160 replaced your #144 ? Specifically, this commit doesn't seem to include your 9e7cbce which made changes to how nils are handled.

Contributor

dcordero commented Dec 9, 2013

Yes, definitely I forgot it 🐙 I will merge from master to get these changes

@robb robb and 1 other commented on an outdated diff Dec 9, 2013

Mantle/MTLJSONAdapter.m
@@ -72,8 +72,7 @@ - (id)initWithJSONDictionary:(NSDictionary *)JSONDictionary modelClass:(Class)mo
NSParameterAssert([modelClass isSubclassOfClass:MTLModel.class]);
NSParameterAssert([modelClass conformsToProtocol:@protocol(MTLJSONSerializing)]);
-
- if (JSONDictionary == nil) {
+ if (JSONDictionary == nil || (![JSONDictionary isKindOfClass:NSDictionary.class])) {
if (error != NULL) {
NSDictionary *userInfo = @{
NSLocalizedDescriptionKey: NSLocalizedString(@"Missing JSON dictionary", @""),
@robb

robb Dec 9, 2013

Owner

This error description doesn't really fit the wrong-class case. Can you return a different error instead?

@dcordero

dcordero Dec 9, 2013

Contributor

I can not find something better than InvalidJSONDictionary which is already used. Any suggestion for this new error? MTLJSONAdapterErrorWrongClassDictionary ?

@robb

robb Dec 9, 2013

Owner

MTLJSONAdapterErrorInvalidJSONDictionary works for me as an error code, just the description feels kinda off (would be nice to include the value that was supplied, imo)

@dcordero

dcordero Dec 9, 2013

Contributor

That has sense. Fixed 💎

@jspahrsummers jspahrsummers commented on an outdated diff Dec 9, 2013

Mantle/MTLJSONAdapter.m
@@ -72,12 +72,11 @@ - (id)initWithJSONDictionary:(NSDictionary *)JSONDictionary modelClass:(Class)mo
NSParameterAssert([modelClass isSubclassOfClass:MTLModel.class]);
NSParameterAssert([modelClass conformsToProtocol:@protocol(MTLJSONSerializing)]);
-
- if (JSONDictionary == nil) {
+ if (JSONDictionary == nil || (![JSONDictionary isKindOfClass:NSDictionary.class])) {
@jspahrsummers

jspahrsummers Dec 9, 2013

Owner

No parentheses are needed here.

@jspahrsummers jspahrsummers commented on an outdated diff Dec 9, 2013

MantleTests/MTLJSONAdapterSpec.m
@@ -82,6 +82,16 @@
expect(error.code).to.equal(MTLJSONAdapterErrorInvalidJSONDictionary);
});
+it(@"should return nil and an error with a wrong data type as dictionary", ^{
+ NSError *error = nil;
+ id wrongDictionary = [NSString new];
@jspahrsummers

jspahrsummers Dec 9, 2013

Owner

Why not @""?

@ghost ghost assigned jspahrsummers and robb Dec 9, 2013

Owner

jspahrsummers commented Dec 9, 2013

Thanks for the PR! This looks pretty good to me, just a couple minor comments.

@jspahrsummers jspahrsummers was assigned Dec 9, 2013

@robb robb added a commit that referenced this pull request Dec 12, 2013

@robb robb Merge pull request #197 from dcordero/fixCrashWhenBuildingTheModel
Fix a crash when receving something different than a NSDictionary
7a47437

@robb robb merged commit 7a47437 into Mantle:master Dec 12, 2013

@dcordero dcordero deleted the unknown repository branch Dec 12, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment