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

Number in JSON does NOT get Converted to NSString Property #193

Closed
fabb opened this Issue Nov 27, 2013 · 25 comments

Comments

Projects
None yet
6 participants

fabb commented Nov 27, 2013

I have a model with an NSString property. The JSON I receive has a number in place instead. When I convert the JSON to a Mantle model, I do not get an error, but the property is filled with an NSNumber.

This is VERY dangerous a runtime.
Until now I expected Mantle doing the conversion from NSNumber to NSString and the other way around out of the box - which would make sense for robustness.

Problematic example:

@interface MantleTestModel : MTLModel <MTLJSONSerializing>
@property (nonatomic, strong) NSString *stringProperty;
@end

@implementation MantleTestModel
+ (NSDictionary *)JSONKeyPathsByPropertyKey {
    return @{@"stringProperty": @"stringProperty"};
}
@end


@implementation MantleTest

- (void)doTest {
    NSDictionary *json = @{@"stringProperty":@666};
    NSError *error = nil;
    MantleTestModel *mantleTestModel = [MTLJSONAdapter modelOfClass:MantleTestModel.class fromJSONDictionary:json error:&error];
    NSParameterAssert(error == nil);

    // this raises
    NSParameterAssert([mantleTestModel.stringProperty isKindOfClass:[NSString class]]);
}

@end

fabb commented Nov 27, 2013

Tried with 1.3.1.

fabb commented Nov 27, 2013

There is no error returned by the adapter on model conversion.

I can think of several reasonable ways to handle mismatching types out of
the box:

  • return an error
  • set the according property nil
  • do auto conversion

Neither of this is happening right now.

Owner

robb commented Nov 27, 2013

Thanks for raising your concern! There is work on automatic transformers (see #147), though I am not sure if this kind of transformation will be supported out of the box (if not, it's going to be easy to add though).

Until then, since Mantle uses key value coding, you can make your model resilient against improper types by implementing Key-Value Validation or a transformer

and the other way around out of the box

MTLJSONAdapter has no way of knowing out of the box what to translate to, so even if Mantle would automatically convert your number into a string, the other way isn't really possible without a transformer.

Member

dcaunt commented Nov 27, 2013

You could validate types using property introspection and Key-Value validation.

It looks like there may be some issues with this approach so you'll have to do some research.

Owner

robb commented Dec 4, 2013

Closing this one. Feel free to reopen if you need help with implementing key value validation or if you have any more questions 🍫

@robb robb closed this Dec 4, 2013

fabb commented Dec 5, 2013

I'd also be happy if an error was thrown. Tons of sleeping bugs because of
this one.

Am Mittwoch, 4. Dezember 2013 schrieb Robert Böhnke :

Closed #193 github#193.


Reply to this email directly or view it on GitHubhttps://github.com/github/Mantle/issues/193
.

@robb robb reopened this Dec 5, 2013

Owner

robb commented Dec 5, 2013

What we'd need to do is implement -[MTLModel validateValue:forKeyPath:error:] and inspect the property for the given key and compare the classes; if they don't match, we throw an error.

I am not sure, however, if this will reliably work for every viable member in +propertyKeys. As I learned in #196, subclasses may have to extend +propertyKeys and thus may be KVO compliant for a key that does not have a matching property.

While it's not very convenient, I believe that subclasses of MTLModel are in a much better position to decide how and if a given value key pair should be validated. I can imagine this being the reason that NSObject doesn't do such validation, either.

Owner

jspahrsummers commented Dec 5, 2013

We could inspect a property's class and throw an exception if a set value doesn't match. I don't think it has anything to do with +propertyKeys, since the key would be passed in, and we could reflect upon the class using that.

False negatives might be an issue, but it could always be disabled by using a more general type (like id) instead.

Owner

robb commented Dec 5, 2013

I thought we could only get the classes for properties, everything else being id. I.e. can we access the class in this case?

@interface XYModel : MTLModel <SomeProtocol>

@end

@implementation

+ (NSSet *)propertyKeys {
    return [[super propertyKeys] setByAddingObject:@"someObject"];
}

- (XYObject)someObject {
  return _someObject;
}

- (void)setSomeObject:(XYObject *)object {
  _someObject = object;
}

@end
Owner

jspahrsummers commented Dec 5, 2013

@robb I think you have #196 on the mind. :P

This is just about properties on a class.

Owner

robb commented Dec 5, 2013

Well, kinda.

As I learned in #196, there are scenarios in which an MTLModels +propertyKeys contains a KVO compliant value for which no property exists and for which we cannot determine the class at runtime, right?

Owner

jspahrsummers commented Dec 5, 2013

If we can't determine the class, we don't have to check it. This would just be a safety net, not something to depend upon.

Owner

robb commented Dec 5, 2013

If we can't determine the class, we don't have to check it. This would just be a safety net, not something to depend upon.

Sure, just wanted to point that one out.

I can go either way on this, it's fairly straight forward to implement but I don't mind MTLModel being just as safe as most other objective-c objects in this regard (which is not a lot).

Owner

jspahrsummers commented Dec 6, 2013

I've certainly stumbled upon this once or twice. Anything we can do to reduce debugging time seems like a win to me. I think false negatives are less likely than catching real issues.

Owner

robb commented Dec 6, 2013

👌

What kind of false negatives do you imagine?

Owner

jspahrsummers commented Dec 6, 2013

Any kind of duck typing could result in a false negative—e.g., using an object that doesn't inherit from NSString in the position of an NSString property.

Not that it's good code, but Objective-C does allow it.

Owner

robb commented Dec 6, 2013

yeah, that's the only thing I could come up with, too.

In that case, the caller passing in the quacking dragon couldn't disable the type check, it would be the responsibility of the model object to allow that by changing the property to id or by overriding -validateValue:forKeyPath:error: again (which kinda defeats the purpose of duck typing…).

Don't think that's a big deal though.

duck-icon

This was referenced Feb 15, 2014

@robb robb added the JSON label Mar 6, 2014

For now, we have a transformer that makes a property explicitly a string

#import "NSValueTransformer+NSString.h"
#import <Mantle/Mantle.h>

NSString * const TMDStringValueTransformerName = @"TMDStringValueTransformerName";

@implementation NSValueTransformer (NSString)

+ (void)load {
    @autoreleasepool {
        MTLValueTransformer *stringValueTransformer = [MTLValueTransformer reversibleTransformerWithForwardBlock:^(id value) {
            if ([value isKindOfClass:[NSNumber class]]) {
                return [(NSNumber*)value stringValue];
            } else {
                return (NSString*)value;
            }
        } reverseBlock:^(NSString *value) {
            return value;
        }];

        [NSValueTransformer setValueTransformer:stringValueTransformer forName:TMDStringValueTransformerName];
    }
}
@end

fabb commented Mar 20, 2014

Great idea and thanks for sharing, BUT wouldn't it be better to convert the value to NSString in the else block instead of just casting it?

Owner

jspahrsummers commented Mar 20, 2014

Thanks to #251, Mantle 2.0 will automatically validate the type of property being set, so this shouldn't be an issue anymore.

Is there any estimate on when will 2.0 be completed?

Owner

jspahrsummers commented Dec 18, 2014

When it's ready.

Not looking for a hard date or anything, but a general idea of how far along it is right now would be appreciated, as this issue was originally posted in Nov 2013 and the last comment in March.

Owner

jspahrsummers commented Dec 18, 2014

Do not make plans predicated on Mantle 2.0, because it's uncertain when it'll be released. 1.x is the latest stable release series.

If and when Mantle 2.0 is released, there will be plenty of documentation available to help with migration.

No problem, I've already implemented the workaround suggested above.
I'd be glad to contribute to 2.0 if needed.

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