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 key is null in mtl_valueMappingTransformer #212

Merged
merged 1 commit into from Jan 16, 2014

Conversation

Projects
None yet
4 participants
Contributor

grraffe commented Jan 7, 2014

I found a issue that occurs when i get nil key in mtl_valueMappingTransformerWithDictionary and fixed it.

@jspahrsummers jspahrsummers was assigned Jan 7, 2014

@jspahrsummers jspahrsummers commented on an outdated diff Jan 7, 2014

...SValueTransformer+MTLPredefinedTransformerAdditions.m
@@ -121,6 +121,9 @@ + (NSValueTransformer *)mtl_valueMappingTransformerWithDictionary:(NSDictionary
NSParameterAssert(dictionary.count == [[NSSet setWithArray:dictionary.allValues] count]);
return [MTLValueTransformer reversibleTransformerWithForwardBlock:^(id<NSCopying> key) {
+ if (!key) {
+ return dictionary[[NSNull null]] ?: nil;
+ }
return dictionary[key];
@jspahrsummers

jspahrsummers Jan 7, 2014

Owner

This can be simplified into:

return dictionary[key ?: NSNull.null];
Owner

jspahrsummers commented Jan 7, 2014

Thanks! Can you make the same change to the reverse case?

Owner

robb commented Jan 7, 2014

What's the crash that you got? Doesn't dictionary[nil] simply return nil?

Contributor

grraffe commented Jan 8, 2014

Oh, sorry. I got that crash a few months ago, and maybe I had a wrong memory.

Contributor

grraffe commented Jan 8, 2014

I got it. I get nothing on mtl_JSONDictionaryTransformerWithModelClass if I have nil key, because validation fails. So if i set a value for [NSNull null], I could be more flexible for any situation. It is not a crash but i think it's an enhancement. Can i reopen PR?

Owner

robb commented Jan 8, 2014

There is another PR for adding a default value to mtl_valueMappingTransformerWithDictionary #189 – I think specifying the default value out of bounds is preferable over giving the NSNull.null yet another meaning.

@Dalog: do you think @blueless should take over?

Hi [NSNull null] is another option for default value. But for my opinion explicit default value is better.
Also we can compose 2 approaches: if no default value setted then look at [NSNull null] key.

I think this compromise can be used without any major downsides.

Owner

robb commented Jan 8, 2014

I think this compromise can be used without any major downsides.

I believe we should keep matters simple and only support explicit defaults.

Owner

jspahrsummers commented Jan 8, 2014

Is it actually well-defined that passing nil into a dictionary subscript will return nil? I don't see that in the documentation.

Owner

robb commented Jan 8, 2014

It's not explicitly state anywhere I could find.

Owner

jspahrsummers commented Jan 16, 2014

In that case, I think this still makes sense, even if #189 gets merged as well.

@jspahrsummers jspahrsummers added a commit that referenced this pull request Jan 16, 2014

@jspahrsummers jspahrsummers Merge pull request #212 from StyleShare/master
Fix a crash when key is null in mtl_valueMappingTransformer
0e7dde3

@jspahrsummers jspahrsummers merged commit 0e7dde3 into Mantle:master Jan 16, 2014

Owner

robb commented Jan 16, 2014

I guess we should document that mapping NSNull.null to something now works as a default value, at least until #189 is merged

Owner

jspahrsummers commented Jan 16, 2014

It's not really a default value—it just corresponds to a nil key. NSNull being a boxed version of nil is very common (e.g., in KVO).

Owner

robb commented Jan 16, 2014

ah, nevermind, had that ?: flipped in my head the whole time

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