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

Merged
merged 7 commits into from Apr 24, 2014

Projects

None yet

3 participants

Owner
robb commented Mar 31, 2014

Fixes #293

  • JSON
  • Core Data
Owner
robb commented Mar 31, 2014

The same should probably be done for managed object mappings.

@robb robb changed the title from Throw an error if JSON mappings to match property keys to [WIP] Throw an error if mappings to match property keys Mar 31, 2014
Owner
robb commented Mar 31, 2014

@jspahrsummers Since this was silently ignored before, putting this straight into master could theoretically break some builds downstream. Let me know if I should rather target 2.0-development

Owner

Let's do this on master, since the previous behavior was basically silent failure.

@jspahrsummers jspahrsummers commented on an outdated diff Mar 31, 2014
Mantle/MTLJSONAdapter.h
@@ -63,6 +63,9 @@
// The provided JSONDictionary is not valid.
extern const NSInteger MTLJSONAdapterErrorInvalidJSONDictionary;
+// The model's JSON mapping is incompatible with its property keys.
jspahrsummers
jspahrsummers Mar 31, 2014 Owner

Can you be more specific with this (and the error message)? Maybe something like:

The model's implementation of +JSONKeyPathsByPropertyKey included a key which does not actually exist in +propertyKeys.

@robb robb changed the title from [WIP] Throw an error if mappings to match property keys to Throw an error if mappings to match property keys Apr 16, 2014
Owner
robb commented Apr 16, 2014

Not sure if CI is down but this is ready for review

@jspahrsummers jspahrsummers self-assigned this Apr 17, 2014
Owner

This looks good, but I worry about the performance impact of +setWithArray: and -isSubsetOfSet:, since these are in such a critical and frequent code path.

Maybe we should instead enumerate the array, and check containment within the set one-by-one?

Owner
robb commented Apr 18, 2014

Maybe we should instead enumerate the array, and check containment within the set one-by-one?

Sounds reasonable

@robb robb changed the title from Throw an error if mappings to match property keys to Throw an error if mappings don't match property keys Apr 22, 2014
Owner
robb commented Apr 22, 2014

🈂️

@jspahrsummers jspahrsummers commented on an outdated diff Apr 23, 2014
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.allKeys) {
jspahrsummers
jspahrsummers Apr 23, 2014 Owner

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

@jspahrsummers jspahrsummers commented on an outdated diff Apr 23, 2014
Mantle/MTLManagedObjectAdapter.m
@@ -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) {
Owner

🍔

Owner
robb commented Apr 24, 2014

🍟

@robb robb referenced this pull request Apr 24, 2014
Merged

Longer lived JSON adapters #278

Owner

🍕

@jspahrsummers jspahrsummers merged commit 9f5f0aa into master Apr 24, 2014
@jspahrsummers jspahrsummers deleted the mismatched-keypaths branch Apr 24, 2014
@robb robb referenced this pull request in octokit/octokit.objc May 9, 2014
Closed

[WIP] Mantle 2.0 #184

@yaltar yaltar commented on the diff May 13, 2014
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) {
yaltar
yaltar May 13, 2014

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,

robb
robb May 13, 2014 Owner

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.

yaltar
yaltar May 13, 2014

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/Mantle/Mantle/pull/294/files#r12588116
.

robb
robb May 13, 2014 Owner

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.

yaltar
yaltar May 13, 2014

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/Mantle/Mantle/pull/294/files#r12591508
.

yaltar
yaltar May 13, 2014

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/Mantle/Mantle/pull/294/files#r12591508
.

yaltar
yaltar May 13, 2014

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/Mantle/Mantle/pull/294/files#r12591508
.

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