Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Keypath regression #266

Merged
merged 3 commits into from

3 participants

@robb
Owner

This is an alternative approach to fixing the key path regression introduced in Mantle 1.4.

The previous implementation introduced in #230 would assume every element along the key path apart from the last one to be an instance of NSDictionary. However, the documentation did not explicitly require this and #257 and #259 show how users would rely on the old behavior when mapping over NSArrays in the JSON response.

Since there doesn't seem to be a good way of checking reliably if an arbitrary object is Key-Value-Coding-Compliant for a given key, this effectively restores the pre-#230 behavior but wraps the call to -valueForKeyPath: in a try catch block, bubbling up the exception through the error parameter.

In 2.0, we may want to consider requiring all objects along the JSON key path to be NSDictionary instances.

Closes #265
Fixes #257 #259

@robb
Owner

/cc @joshaber If you have a moment, I'd appreciate your input on this. Would :sparkling_heart: to get a 1.4.1 out with the regression fixed.

@bobliusf

I am definitely in the opinion of not supporting key path. it breaks the concept of induction. i can see why people may think it is easier to be able to use key path but it allows people to hack the structure navigation and create further issues. to me, it is best to let people handle path navigation through standard induction and value transformer.

We should also ensure sufficient test case converges before codes are allowed to be merged in. It will help to ensure quality and reduce the risk of these type of regressions.

Love the mantle concept by the way! It is clean and elegant. My fear is that key path support sends the project to the opposite direction.

@robb
Owner

I am definitely in the opinion of not supporting key path.

Are you saying you don't want to have any support for nested keys or that every element along foo.bar.baz should be a dictionary?

Either way, I don't think that 1.4 was the right version for changing the behavior and I'd rather restore it for now and do it properly in 2.0.

@bobliusf

@robb in my opinion yes. but i would love to hear what others think. i think it is sufficient to just use + (NSValueTransformer *)XXXXJSONTransformer. to access nested models, child models should be created or the server json be updated. I understand it is not always possible to change server json, but i would rather do a little model nesting then allowing key path traversal. the feature just seems to be prone to abuse. however, i do understand there are pros and cons involved. generally i control the whole stack from backend to mobile so i have the tendency of wanting to keep things simple and exact.

MantleTests/MTLJSONAdapterSpec.m
@@ -88,6 +88,29 @@
expect(error.code).to.equal(MTLJSONAdapterErrorInvalidJSONDictionary);
});
+it(@"should support key paths across", ^{
@jspahrsummers Owner

Across?

@robb Owner
robb added a note

… arrays.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jspahrsummers jspahrsummers self-assigned this
@jspahrsummers

:+1: in general.

it breaks the concept of induction.

I don't really know what this means.

Key paths are simple enough to support, and don't introduce any structural violations to me. JSON is an external data format, it does not need to correspond semantically to the actual model you create locally.

@robb
Owner

Updated the spec description.

@jspahrsummers jspahrsummers merged commit bfdc96a into master

1 check passed

Details default Build #1120491 succeeded in 42s
@jspahrsummers jspahrsummers deleted the keypath-regression branch
@robb
Owner

Are you tagging 1.4.1., too?

@jspahrsummers

Sure, tagged. Feel free to add more detailed notes if you want.

@robb
Owner

FWIW, leaving the key path traversal to transformers would require adding compositional operators so that you could

NSValueTransformer *keyPath = [NSValueTransformer mtl_keyPathTransformer:@"foo.bar"];
NSValueTransformer *parseDate = [[XYDateParsingTransformer alloc] init];

[keyPath concat:parseDate];

where you'd currently only need parseDate.

@bobliusf

cool sounds good. just a minor thing: In the json adaptor, the error message is based on the old error. i can try to send a pr if i have enough time this week.

NSDictionary *userInfo = @{
NSLocalizedDescriptionKey: NSLocalizedString(@"Invalid JSON dictionary", nil),

Also, I am not a compiler expert, but would wrapping the exception over the whole forloop more performant than wrapping it for each property? sometimes we have pretty big json list with quite a few properties.

@jspahrsummers

Also, I am not a compiler expert, but would wrapping the exception over the whole forloop more performant than wrapping it for each property?

@try doesn't cost anything unless an exception is actually thrown. (This is one of the benefits of the modern Objective-C runtime.)

@bobliusf

awesome. thanks for the info!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 6, 2014
  1. @robb
Commits on Mar 9, 2014
  1. @robb

    Catch key path exceptions

    robb authored
Commits on Mar 11, 2014
  1. @robb

    Fix spec description

    robb authored
This page is out of date. Refresh to see the latest.
View
29 Mantle/MTLJSONAdapter.m
@@ -106,24 +106,21 @@ - (id)initWithJSONDictionary:(NSDictionary *)JSONDictionary modelClass:(Class)mo
NSString *JSONKeyPath = [self JSONKeyPathForPropertyKey:propertyKey];
if (JSONKeyPath == nil) continue;
- id value = JSONDictionary;
- NSArray *JSONKeyPathComponents = [JSONKeyPath componentsSeparatedByString:@"."];
- for (NSString *itemJSONKeyPathComponent in JSONKeyPathComponents) {
- if (![value isKindOfClass:NSDictionary.class]) {
- if (error != NULL) {
- NSDictionary *userInfo = @{
- NSLocalizedDescriptionKey: NSLocalizedString(@"Invalid JSON dictionary", @""),
- NSLocalizedFailureReasonErrorKey: [NSString stringWithFormat:NSLocalizedString(@"%@ could not be parsed because an invalid JSON dictionary was provided for key path \"%@\"", @""), modelClass, JSONKeyPath],
- };
-
- *error = [NSError errorWithDomain:MTLJSONAdapterErrorDomain code:MTLJSONAdapterErrorInvalidJSONDictionary userInfo:userInfo];
- }
-
- return nil;
+ id value;
+ @try {
+ value = [JSONDictionary valueForKeyPath:JSONKeyPath];
+ } @catch (NSException *ex) {
+ if (error != NULL) {
+ NSDictionary *userInfo = @{
+ NSLocalizedDescriptionKey: NSLocalizedString(@"Invalid JSON dictionary", nil),
+ NSLocalizedFailureReasonErrorKey: [NSString stringWithFormat:NSLocalizedString(@"%1$@ could not be parsed because an invalid JSON dictionary was provided for key path \"%2$@\"", nil), modelClass, JSONKeyPath],
+ MTLJSONAdapterThrownExceptionErrorKey: ex
+ };
+
+ *error = [NSError errorWithDomain:MTLJSONAdapterErrorDomain code:MTLJSONAdapterErrorInvalidJSONDictionary userInfo:userInfo];
}
- value = [value valueForKey:itemJSONKeyPathComponent];
- if (value == nil || value == NSNull.null) break;
+ return nil;
}
if (value == nil) continue;
View
23 MantleTests/MTLJSONAdapterSpec.m
@@ -88,6 +88,29 @@
expect(error.code).to.equal(MTLJSONAdapterErrorInvalidJSONDictionary);
});
+it(@"should support key paths across arrays", ^{
+ NSDictionary *values = @{
+ @"users": @[
+ @{
+ @"name": @"foo"
+ },
+ @{
+ @"name": @"bar"
+ },
+ @{
+ @"name": @"baz"
+ }
+ ]
+ };
+
+ NSError *error = nil;
+ MTLArrayTestModel *model = [MTLJSONAdapter modelOfClass:MTLArrayTestModel.class fromJSONDictionary:values error:&error];
+ expect(model).notTo.beNil();
+ expect(error).to.beNil();
+
+ expect(model.names).to.equal((@[ @"foo", @"bar", @"baz" ]));
+});
+
it(@"should initialize without returning any error when using a JSON dictionary which Null.null as value",^{
NSDictionary *values = @{
@"username": @"foo",
View
7 MantleTests/MTLTestModel.h
@@ -41,6 +41,13 @@ extern const NSInteger MTLTestModelNameMissing;
@end
+@interface MTLArrayTestModel : MTLModel <MTLJSONSerializing>
+
+// This property is associated with a "users.username" key in JSON.
+@property (nonatomic, copy) NSString *names;
+
+@end
+
// Parses MTLTestModel objects from JSON instead.
@interface MTLSubstitutingTestModel : MTLModel <MTLJSONSerializing>
@end
View
10 MantleTests/MTLTestModel.m
@@ -120,6 +120,16 @@ - (void)mergeCountFromModel:(MTLTestModel *)model {
@end
+@implementation MTLArrayTestModel
+
++ (NSDictionary *)JSONKeyPathsByPropertyKey {
+ return @{
+ @"names": @"users.name"
+ };
+}
+
+@end
+
@implementation MTLSubstitutingTestModel
+ (NSDictionary *)JSONKeyPathsByPropertyKey {
Something went wrong with that request. Please try again.