MTLJSONAdapter modelOfClass: fails if array is present #257

Closed
foulkesjohn opened this Issue Mar 6, 2014 · 9 comments

Projects

None yet

3 participants

@foulkesjohn

I've just upgraded to 1.4 and my tests are failing when I try the following:

NSError *error = nil;
Result *result = [MTLJSONAdapter modelOfClass:[Result class] fromJSONDictionary:resultDict error:&error];

It was previously importing my images correctly, with the prefixed url's. From what I can see the check for if (![value isKindOfClass:NSDictionary.class]) on line 112 in MTLJSONAdapter makes this fail.

I can't figure out if my setup is wrong or the latest change is a bug?

The json looks something like this:

[
  {
    "detail" : {
      "name" : "Test",
      "pictures" : [
        {
          "original" : "original.jpg",
          "mobile" : "mobile.jpg"
        }
      ]
    }
  }
]

My model looks like this:

@interface Result : MTLModel <MTLJSONSerializing>

@property (nonatomic, strong) NSString *name;
@property (nonatomic, strong) NSArray *pictures;

@end

@implementation Result

+ (NSDictionary *)JSONKeyPathsByPropertyKey
{
  return @{
    @"name": @"detail.name",
    @"pictures": @"detail.pictures.mobile",
  };
}

+ (NSValueTransformer *)picturesJSONTransformer
{
  NSString *scheme = [[NSURL URLWithString:kBaseUrl] scheme];
  return [MTLValueTransformer transformerWithBlock:^id(NSArray *images) {
    return [images prefixedWithScheme:scheme];
  }];
}
@robb robb added the bug label Mar 6, 2014
@robb
Mantle member

Ah, dang. Looks like a quasi-regression introduced by #230/#235.

I believe what happened here is that your implementation relied on the fact that -valueForKey: when called on an NSArray returns an array containing what ever its elements returned when invoked with the same method. (It effectively maps -valueForKey: over its elements)

For a quick fix, try something like:

+ (NSDictionary *)JSONKeyPathsByPropertyKey
{
  return @{
    @"name": @"detail.name",
    @"pictures": @"detail.pictures",
  };
}

+ (NSValueTransformer *)picturesJSONTransformer
{
  NSString *scheme = [[NSURL URLWithString:kBaseUrl] scheme];
  return [MTLValueTransformer transformerWithBlock:^id(NSArray *pictures) {
    NSArray *mobile = [pictures valueForKey:@"mobile"];
    return [mobile prefixedWithScheme:scheme];
  }];
}

/cc @dcordero @jspahrsummers

@robb
Mantle member

It's not hard to restore the old behavior for a quick 1.4.1 but I wonder if it should have been correct in the first place. That hinges on what "JSON key paths" are and if they follow the all semantics of valueForKey:. E.g. should this work:

+ (NSDictionary *)JSONKeyPathsByPropertyKey {
    return @{
        @"name": @"name",
        @"length": @"name.length",
        @"array": @"array",
        @"allFoos": @"array.foo"
    };
}

I can see two ways to handle this:

  • Restore the old behavior of allowing -valueForKey: on every member of the JSON dictionary by removing the dictionary check. It seems that this approach is leaking the KVO behavior of the deserialized collection classes, though.
  • Making it more strict so that every key in the JSON keypath but the last one needs to hit a NSDictionary on which we invoke -objectForKey: rather than -valueForKey:.
@robb
Mantle member

After some thought, I'd say let's revert the dictionary change for 1.4.1 and use -objectForKey: in 2.0. Yay or nay @MantleFramework ?

@dcordero

I think the error is not in related with the custom transformer but in the fact the json that @foulkesjohn is using has an JSON Array as the root element.

So what we get in MTLJSONAdapter initWithJSONDictionary:(NSDictionary *)JSONDictionary... is an NSArray instead of an NSDictionary.

And we are checking that if it is a JSONDictionary at the beginning of that method.

So in my opinion we should modify the condition to support both NSArray and JSONDictionaries, but also change the name and the type of this parameter since both Dictionaries and Arrays are supported by JSON.

Changing the name and the type is a change probably TOO big for Mantle v1. But we can modify here the condition to support both.

I don't thing revert the change is a great solution since this condition was added to avoid crashes in the library.

@robb
Mantle member

I think the error is not in related with the custom transformer but in the fact the json that @foulkesjohn is using has an JSON Array as the root element.

I was under the impression that this happens in the context of mtl_JSONArrayTransformerWithModelClass:, otherwise I'm not sure how it would've worked in 1.3.x.

I don't thing revert the change is a great solution since this condition was added to avoid crashes in the library.

It did, however, introduce new crashes. I'm not saying we should revert all of #230, just the check that elements halfway through the keypath need to be NSDictionary instances which wasn't the case when we still used -valueForKeyPath:.

@foulkesjohn

The JSON I provided is just a rough version of what my actual JSON looks like. I am actually using mtl_JSONArrayTransformerWithModelClass and passing in an array of dictionaries

@dcordero

What I mean is that I am not sure if it is a bug in Mantle or not. Since the dot syntax can be used for navigating through a Dictionary but now we have an Array in the middle, and in an Array we can have duplicated element. Example:

{
    "key1": "value1",
    "key2": [
        {
            "key3": "value3",
            "key4": "value4"
        },
        {
            "key3": "value5",
            "key4": "value6"
        }
    ]
}

What is the value that Mantle should get for the path "key1.key2.key3" ? value3 or value5? both have the same path.

in your example "pictures" is an Array and it could have more jsons with different values for the key "mobile"

@robb
Mantle member

It's actually well defined by -[NSArray valueForKey:] that key1.key2.key3 in your example would return the equivalent of @[ @"value3", @"value5" ] and @foulkesjohn's implementation relied on the fact that we were using -valueForKeyPath: internally. That's the regression here, it's a pretty subtle one.

@robb
Mantle member

FWIW, https://github.com/MantleFramework/Mantle/tree/keypath-regression contains a test for the regression.

@robb robb added the JSON label Mar 6, 2014
This was referenced Mar 9, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment