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

Throw an error if mappings don't match property keys #294

Merged
merged 7 commits into from Apr 24, 2014
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions Mantle/MTLJSONAdapter.h
Expand Up @@ -63,6 +63,10 @@ extern const NSInteger MTLJSONAdapterErrorNoClassFound;
// The provided JSONDictionary is not valid.
extern const NSInteger MTLJSONAdapterErrorInvalidJSONDictionary;

// The model's implementation of +JSONKeyPathsByPropertyKey included a key which
// does not actually exist in +propertyKeys.
extern const NSInteger MTLJSONAdapterErrorInvalidJSONMapping;

// Converts a MTLModel object to and from a JSON dictionary.
@interface MTLJSONAdapter : NSObject

Expand Down
20 changes: 19 additions & 1 deletion Mantle/MTLJSONAdapter.m
Expand Up @@ -13,6 +13,7 @@
NSString * const MTLJSONAdapterErrorDomain = @"MTLJSONAdapterErrorDomain";
const NSInteger MTLJSONAdapterErrorNoClassFound = 2;
const NSInteger MTLJSONAdapterErrorInvalidJSONDictionary = 3;
const NSInteger MTLJSONAdapterErrorInvalidJSONMapping = 4;

// An exception was thrown and caught.
const NSInteger MTLJSONAdapterErrorExceptionThrown = 1;
Expand Down Expand Up @@ -102,7 +103,24 @@ - (id)initWithJSONDictionary:(NSDictionary *)JSONDictionary modelClass:(Class)mo

NSMutableDictionary *dictionaryValue = [[NSMutableDictionary alloc] initWithCapacity:JSONDictionary.count];

for (NSString *propertyKey in [self.modelClass propertyKeys]) {
NSSet *propertyKeys = [self.modelClass propertyKeys];

for (NSString *JSONKeyPath in self.JSONKeyPathsByPropertyKey.allKeys) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The allKeys is expensive and redundant here. Dictionaries are already fast enumerated by key.

if ([propertyKeys containsObject:JSONKeyPath]) continue;

if (error != NULL) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,
Just want to add my 2 cents on this pull request which was merged.

IMO It shouldn't throw error in case of umapped JSON Key. I've multiple case where server respond lot of field in JSON response and i don't want to get them all in my object, so no reason to throw an error.

At least it should be configurable, one should be able to choose to be "strict" or not on mapping.

More than that, the pull requested merged seems to have error in it, as it silently fails.

Seems due to line 150 in "antle/MTLJSONAdapter.h" where code is
if (error != NULL) {

Should be
if (error == NULL) {

As there is no current error, it show nothing.

Thanks in advance for fixing the issue.

Regards,

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This only affects the property mapping in your model, so if you say map the property foo to the JSON key bar, this will only make sure that your class actually has a property foo. If your server's response actually contains bar or a different set of keys entirely is a different matter.

The if (error != NULL) is needed to make sure that we're not dereferencing a NULL pointer when assigning the NSError later.

Also check out #325 where we're changing this to an exception.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,
Not sure about the explanation on property mapping. While doing debug step
by step because i didn't have anymore my json mapped to object i reach the
case where server response have a field in JSON "bar", which i didn't use
in my Model on my app, and the debug goes to line 150.

And about the error, let read the block 150 - 159
If no error present, we do nothing except returning 'nil', so the actual
error is not described and is silent.

Thx

On Tue, May 13, 2014 at 5:05 PM, Robert Böhnke notifications@github.comwrote:

In Mantle/MTLJSONAdapter.m:

@@ -102,7 +103,24 @@ - (id)initWithJSONDictionary:(NSDictionary *)JSONDictionary modelClass:(Class)mo

NSMutableDictionary *dictionaryValue = [[NSMutableDictionary alloc] initWithCapacity:JSONDictionary.count];
  • for (NSString *propertyKey in [self.modelClass propertyKeys]) {
  • NSSet *propertyKeys = [self.modelClass propertyKeys];
  • for (NSString *JSONKeyPath in self.JSONKeyPathsByPropertyKey) {
  •   if ([propertyKeys containsObject:JSONKeyPath]) continue;
    
  •   if (error != NULL) {
    

This only affects the property mapping in your model, so if you say map
the property foo to the JSON key bar, this will only make sure that your
class actually has a property foo. If your server's response actually
contains bar or a different set of keys entirely is a different matter.

The if (error != NULL) is needed to make sure that we're not
dereferencing a NULL pointer when assigning the NSError later.

Also check out #325 #325 where
we're changing this to an exception.


Reply to this email directly or view it on GitHubhttps://github.com//pull/294/files#r12588116
.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about the explanation on property mapping. While doing debug step by step because i didn't have anymore my json mapped to object i reach the case where server response have a field in JSON "bar", which i didn't use in my Model on my app, and the debug goes to line 150.

Odd, do you have a test case you could put into a gist?

The error variable is an argument to the method of type NSError **. If the user passes in NULL, we wouldn't know where to store the reference to the error object we're creating. Again, this will change anyway.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok thx for the error parameter, didn't get that point, thx for your
highlight.
I need to check why my error object didn't goes through.

I'll try to make a test case.

Thx for you help

On Tue, May 13, 2014 at 5:59 PM, Robert Böhnke notifications@github.comwrote:

In Mantle/MTLJSONAdapter.m:

@@ -102,7 +103,24 @@ - (id)initWithJSONDictionary:(NSDictionary *)JSONDictionary modelClass:(Class)mo

NSMutableDictionary *dictionaryValue = [[NSMutableDictionary alloc] initWithCapacity:JSONDictionary.count];
  • for (NSString *propertyKey in [self.modelClass propertyKeys]) {
  • NSSet *propertyKeys = [self.modelClass propertyKeys];
  • for (NSString *JSONKeyPath in self.JSONKeyPathsByPropertyKey) {
  •   if ([propertyKeys containsObject:JSONKeyPath]) continue;
    
  •   if (error != NULL) {
    

Not sure about the explanation on property mapping. While doing debug
step by step because i didn't have anymore my json mapped to object i reach
the case where server response have a field in JSON "bar", which i didn't
use in my Model on my app, and the debug goes to line 150.

Odd, do you have a test case you could put into a gist?

The error variable is an argument to the method of type NSError **. If
the user passes in NULL, we wouldn't know where to store the reference to
the error object we're creating. Again, this will change anyway.


Reply to this email directly or view it on GitHubhttps://github.com//pull/294/files#r12591508
.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok founded why error didn't get there.
Come from NSValueTransformer+MTLPredefinedTransformerAdditions.m
line 57

return [MTLJSONAdapter modelOfClass:modelClass
fromJSONDictionary:JSONDictionary
error:NULL];

it's hardcoded to null.

On Tue, May 13, 2014 at 6:12 PM, Vincent YSMAL vincent.ysmal@gmail.comwrote:

Ok thx for the error parameter, didn't get that point, thx for your
highlight.
I need to check why my error object didn't goes through.

I'll try to make a test case.

Thx for you help

On Tue, May 13, 2014 at 5:59 PM, Robert Böhnke notifications@github.comwrote:

In Mantle/MTLJSONAdapter.m:

@@ -102,7 +103,24 @@ - (id)initWithJSONDictionary:(NSDictionary *)JSONDictionary modelClass:(Class)mo

NSMutableDictionary *dictionaryValue = [[NSMutableDictionary alloc] initWithCapacity:JSONDictionary.count];

  • for (NSString *propertyKey in [self.modelClass propertyKeys]) {
  • NSSet *propertyKeys = [self.modelClass propertyKeys];
  • for (NSString *JSONKeyPath in self.JSONKeyPathsByPropertyKey) {
  •  if ([propertyKeys containsObject:JSONKeyPath]) continue;
    
  •  if (error != NULL) {
    

Not sure about the explanation on property mapping. While doing debug
step by step because i didn't have anymore my json mapped to object i reach
the case where server response have a field in JSON "bar", which i didn't
use in my Model on my app, and the debug goes to line 150.

Odd, do you have a test case you could put into a gist?

The error variable is an argument to the method of type NSError **. If
the user passes in NULL, we wouldn't know where to store the reference
to the error object we're creating. Again, this will change anyway.


Reply to this email directly or view it on GitHubhttps://github.com//pull/294/files#r12591508
.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok after deeper look, you were right, sorry for this misunderstanding.
I've issue in mapping declaration one field was declared in mapped but not
in model ...

Anyway there is an issue due to error hardcoded to null and avoid having
the error reported

NSValueTransformer+MTLPredefinedTransformerAdditions.m
line 57

return [MTLJSONAdapter modelOfClass:modelClass
fromJSONDictionary:JSONDictionary error:NULL];

On Tue, May 13, 2014 at 6:16 PM, Vincent YSMAL vincent.ysmal@gmail.comwrote:

Ok founded why error didn't get there.
Come from NSValueTransformer+MTLPredefinedTransformerAdditions.m
line 57

return [MTLJSONAdapter modelOfClass:modelClass fromJSONDictionary:JSONDictionary
error:NULL];

it's hardcoded to null.

On Tue, May 13, 2014 at 6:12 PM, Vincent YSMAL vincent.ysmal@gmail.comwrote:

Ok thx for the error parameter, didn't get that point, thx for your
highlight.
I need to check why my error object didn't goes through.

I'll try to make a test case.

Thx for you help

On Tue, May 13, 2014 at 5:59 PM, Robert Böhnke notifications@github.comwrote:

In Mantle/MTLJSONAdapter.m:

@@ -102,7 +103,24 @@ - (id)initWithJSONDictionary:(NSDictionary *)JSONDictionary modelClass:(Class)mo

NSMutableDictionary *dictionaryValue = [[NSMutableDictionary alloc] initWithCapacity:JSONDictionary.count];

  • for (NSString *propertyKey in [self.modelClass propertyKeys]) {
  • NSSet *propertyKeys = [self.modelClass propertyKeys];
    +
  • for (NSString *JSONKeyPath in self.JSONKeyPathsByPropertyKey) {
  • if ([propertyKeys containsObject:JSONKeyPath]) continue;
    
  • if (error != NULL) {
    

Not sure about the explanation on property mapping. While doing debug
step by step because i didn't have anymore my json mapped to object i reach
the case where server response have a field in JSON "bar", which i didn't
use in my Model on my app, and the debug goes to line 150.

Odd, do you have a test case you could put into a gist?

The error variable is an argument to the method of type NSError **. If
the user passes in NULL, we wouldn't know where to store the reference
to the error object we're creating. Again, this will change anyway.


Reply to this email directly or view it on GitHubhttps://github.com//pull/294/files#r12591508
.

NSDictionary *userInfo = @{
NSLocalizedDescriptionKey: NSLocalizedString(@"Invalid JSON mapping", nil),
NSLocalizedFailureReasonErrorKey: [NSString stringWithFormat:NSLocalizedString(@"%1$@ could not be parsed because its JSON mapping contains illegal property keys.", nil), modelClass]
};

*error = [NSError errorWithDomain:MTLJSONAdapterErrorDomain code:MTLJSONAdapterErrorInvalidJSONMapping userInfo:userInfo];
}

return nil;
}

for (NSString *propertyKey in propertyKeys) {
NSString *JSONKeyPath = [self JSONKeyPathForPropertyKey:propertyKey];
if (JSONKeyPath == nil) continue;

Expand Down
4 changes: 4 additions & 0 deletions Mantle/MTLManagedObjectAdapter.h
Expand Up @@ -150,6 +150,10 @@ extern const NSInteger MTLManagedObjectAdapterErrorUniqueFetchRequestFailed;
// NSArray or NSSet of MTLModel<MTLManagedObjectSerializing> instances.
extern const NSInteger MTLManagedObjectAdapterErrorUnsupportedRelationshipClass;

// The model's implementation of +managedObjectKeysByPropertyKey included a key
// which does not actually exist in +propertyKeys.
extern const NSInteger MTLManagedObjectAdapterErrorInvalidManagedObjectMapping;

// Converts a MTLModel object to and from an NSManagedObject.
@interface MTLManagedObjectAdapter : NSObject

Expand Down
18 changes: 18 additions & 0 deletions Mantle/MTLManagedObjectAdapter.m
Expand Up @@ -19,6 +19,7 @@
const NSInteger MTLManagedObjectAdapterErrorUnsupportedManagedObjectPropertyType = 5;
const NSInteger MTLManagedObjectAdapterErrorUnsupportedRelationshipClass = 6;
const NSInteger MTLManagedObjectAdapterErrorUniqueFetchRequestFailed = 7;
const NSInteger MTLManagedObjectAdapterErrorInvalidManagedObjectMapping = 8;

// Performs the given block in the context's queue, if it has one.
static id performInContext(NSManagedObjectContext *context, id (^block)(void)) {
Expand Down Expand Up @@ -250,6 +251,23 @@ - (id)modelFromManagedObject:(NSManagedObject *)managedObject processedObjects:(
}

+ (id)modelOfClass:(Class)modelClass fromManagedObject:(NSManagedObject *)managedObject error:(NSError **)error {
NSSet *propertyKeys = [modelClass propertyKeys];

for (NSString *mappedPropertyKey in [modelClass managedObjectKeysByPropertyKey].allKeys) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above.

if ([propertyKeys containsObject:mappedPropertyKey]) continue;

if (error != NULL) {
NSDictionary *userInfo = @{
NSLocalizedDescriptionKey: NSLocalizedString(@"Invalid entity attribute mapping", nil),
NSLocalizedFailureReasonErrorKey: [NSString stringWithFormat:NSLocalizedString(@"%1$@ could not be parsed because its entity attribute mapping contains illegal property keys.", nil), modelClass]
};

*error = [NSError errorWithDomain:MTLManagedObjectAdapterErrorDomain code:MTLManagedObjectAdapterErrorInvalidManagedObjectMapping userInfo:userInfo];
}

return nil;
}

CFMutableDictionaryRef processedObjects = CFDictionaryCreateMutable(NULL, 0, &kCFTypeDictionaryKeyCallBacks, &kCFTypeDictionaryValueCallBacks);
if (processedObjects == NULL) return nil;

Expand Down
3 changes: 3 additions & 0 deletions MantleTests/MTLCoreDataTestModels.h
Expand Up @@ -54,3 +54,6 @@

@end

// Maps a non-existant property "name" to the "string" attribute.
@interface MTLIllegalManagedObjectMappingModel : MTLModel <MTLManagedObjectSerializing>
@end
14 changes: 14 additions & 0 deletions MantleTests/MTLCoreDataTestModels.m
Expand Up @@ -105,3 +105,17 @@ + (NSString *)managedObjectEntityName {
}

@end

@implementation MTLIllegalManagedObjectMappingModel

+ (NSString *)managedObjectEntityName {
return @"Parent";
}

+ (NSDictionary *)managedObjectKeysByPropertyKey {
return @{
@"name": @"username"
};
}

@end
13 changes: 13 additions & 0 deletions MantleTests/MTLJSONAdapterSpec.m
Expand Up @@ -88,6 +88,19 @@
expect(error.code).to.equal(MTLJSONAdapterErrorInvalidJSONDictionary);
});

it(@"should return nil and error with an illegal JSON mapping", ^{
NSDictionary *values = @{
@"username": @"foo"
};

NSError *error = nil;
MTLIllegalJSONMappingModel *model = [MTLJSONAdapter modelOfClass:MTLIllegalJSONMappingModel.class fromJSONDictionary:values error:&error];
expect(model).beNil();
expect(error).notTo.beNil();
expect(error.domain).to.equal(MTLJSONAdapterErrorDomain);
expect(error.code).to.equal(MTLJSONAdapterErrorInvalidJSONMapping);
});

it(@"should support key paths across arrays", ^{
NSDictionary *values = @{
@"users": @[
Expand Down
14 changes: 14 additions & 0 deletions MantleTests/MTLManagedObjectAdapterSpec.m
Expand Up @@ -226,6 +226,20 @@
expect(error).notTo.beNil();
});

it(@"should return nil and error with an illegal JSON mapping", ^{
MTLParent *parent = [MTLParent insertInManagedObjectContext:context];
expect(parent).notTo.beNil();

parent.string = @"foobar";

NSError *error = nil;
MTLIllegalManagedObjectMappingModel *model = [MTLManagedObjectAdapter modelOfClass:MTLIllegalManagedObjectMappingModel.class fromManagedObject:parent error:&error];
expect(model).beNil();
expect(error).notTo.beNil();
expect(error.domain).to.equal(MTLManagedObjectAdapterErrorDomain);
expect(error.code).to.equal(MTLManagedObjectAdapterErrorInvalidManagedObjectMapping);
});

it(@"should return an error if model doesn't validate for insert", ^{
MTLParentIncorrectTestModel *parentModel = [MTLParentIncorrectTestModel modelWithDictionary:@{} error:NULL];

Expand Down
4 changes: 4 additions & 0 deletions MantleTests/MTLTestModel.h
Expand Up @@ -62,3 +62,7 @@ extern const NSInteger MTLTestModelNameMissing;
// Returns a default name of 'foobar' when validateName:error: is invoked
@interface MTLSelfValidatingModel : MTLValidationModel
@end

// Maps a non-existant property "name" to the "username" key in JSON.
@interface MTLIllegalJSONMappingModel : MTLModel <MTLJSONSerializing>
@end
10 changes: 10 additions & 0 deletions MantleTests/MTLTestModel.m
Expand Up @@ -172,3 +172,13 @@ - (BOOL)validateName:(NSString **)name error:(NSError **)error {
}

@end

@implementation MTLIllegalJSONMappingModel

+ (NSDictionary *)JSONKeyPathsByPropertyKey {
return @{
@"name": @"username"
};
}

@end