Implicit validation #251

Merged
merged 9 commits into from Mar 14, 2014

Projects

None yet

2 participants

@robb
Member
robb commented Mar 3, 2014

As per #242

This validates properties assigned by MTLJSONAdapter by injecting an implicit NSValueTransformer that fails if its expected class is not met and passes the value through otherwise.

It's somewhat of an abuse of NSValueTransformers, depending how you look at it, but it only requires very minimal additions to MTLJSONAdapter and could easily by adopted by other (external) adapters.

TODO

  • Add a test for id properties, I think these are currently checked against NSValue
  • Document the changed behavior of -transformerForModelPropertiesOfClass: and -transformerForModelPropertiesOfObjCType:

Fixes #193

@robb robb added this to the 2.0 milestone Mar 4, 2014
@robb robb added the enhancement label Mar 4, 2014
@jspahrsummers jspahrsummers commented on an outdated diff Mar 13, 2014
...SValueTransformer+MTLPredefinedTransformerAdditions.h
@@ -63,4 +63,13 @@
// transformations, and from objects to keys for reverse transformations.
+ (NSValueTransformer<MTLTransformerErrorHandling> *)mtl_valueMappingTransformerWithDictionary:(NSDictionary *)dictionary;
+// A value transformer that errors if the transformed value are not of the given
+// class.
+//
+// class - The expected class.
@jspahrsummers
jspahrsummers Mar 13, 2014 Member

Can you document that this must not be nil?

@jspahrsummers jspahrsummers commented on an outdated diff Mar 13, 2014
...SValueTransformer+MTLPredefinedTransformerAdditions.m
@@ -306,4 +306,26 @@ + (void)load {
}];
}
++ (NSValueTransformer<MTLTransformerErrorHandling> *)mtl_validatingTransformerForClass:(Class)class {
+ NSParameterAssert(class != nil);
+
+ return [MTLValueTransformer transformerUsingForwardBlock:^id(id value, BOOL *success, NSError **error) {
@jspahrsummers
jspahrsummers Mar 13, 2014 Member

Style: spaces around return type

@jspahrsummers jspahrsummers commented on an outdated diff Mar 13, 2014
...SValueTransformer+MTLPredefinedTransformerAdditions.m
@@ -306,4 +306,26 @@ + (void)load {
}];
}
++ (NSValueTransformer<MTLTransformerErrorHandling> *)mtl_validatingTransformerForClass:(Class)class {
+ NSParameterAssert(class != nil);
+
+ return [MTLValueTransformer transformerUsingForwardBlock:^id(id value, BOOL *success, NSError **error) {
+ if (value != nil && ![value isKindOfClass:class]) {
+ if (error != NULL) {
+ NSDictionary *userInfo = @{
+ NSLocalizedDescriptionKey: NSLocalizedString(@"Value did not match expected type", @""),
+ NSLocalizedFailureReasonErrorKey: [NSString stringWithFormat:NSLocalizedString(@"Expected %1$@ be of class %2$@", @""), value, class],
@jspahrsummers
Member

👍

@robb
Member
robb commented Mar 13, 2014

What do you think of this approach? By making the validating transformer fallback an implementation detail of -valueTransformersForModelClass:, subclasses don't have to concern themselves with that when overriding -transformerForModelPropertiesOfClass: and -transformerForModelPropertiesOfObjCType:

@jspahrsummers
Member

Hmm… the first thing that occurs to me is that it'd be nice to compose the validation with an existing transformer (e.g., to verify that the output of the URL transformer going into an NSURL property), but maybe that's overengineering it.

I'm also not sure how you'd opt-out in a design like that. What do you think?

@robb
Member
robb commented Mar 13, 2014

Hmm… the first thing that occurs to me is that it'd be nice to compose the validation with an existing transformer (e.g., to verify that the output of the URL transformer going into an NSURL property), but maybe that's overengineering it.

I'd expect value transformers to make sure to return the advertised type and subclasses of MTLModel to make sure they assign the correct transformers to properties. The use-case here seems to be mostly JSON APIs returning NSNumber when you'd expect NSArrays, if there is no transformer that would error out.

I'm also not sure how you'd opt-out in a design like that. What do you think?

Yeah, after writing it, I felt like I'd rather have subclasses opt in by copying those couple of lines in their JSON adapter subclass than us pushing it down their throat. I don't really like the idea of overriding e.g. +wantsPropertyValidation to return NO but I guess it's a compromise?

@robb robb added the JSON label Mar 13, 2014
@jspahrsummers jspahrsummers and 1 other commented on an outdated diff Mar 14, 2014
MantleTests/MTLJSONAdapterSpec.m
@@ -204,6 +204,33 @@
expect(error.userInfo[MTLTransformerErrorHandlingInputValueErrorKey]).to.equal(@666);
});
+it(@"should fail to deserialize if the JSON types don't match the properties", ^{
+ NSDictionary *values = @{
+ @"flag": @"Potentially"
+ };
+
+ NSError *error = nil;
+ MTLTestModel *model = [MTLJSONAdapter modelOfClass:MTLBoolModel.class fromJSONDictionary:values error:&error];
+ expect(model).to.beNil();
+
+ expect(error.domain).to.equal(MTLTransformerErrorHandlingErrorDomain);
+ expect(error.code).to.equal(MTLTransformerErrorHandlingErrorInvalidInput);
+ expect(error.userInfo[MTLTransformerErrorHandlingInputValueErrorKey]).to.equal(@"Potentially");
+});
+
+fit(@"should acceppt any object for id properties", ^{
@jspahrsummers
jspahrsummers Mar 14, 2014 Member

This should be unfocused. Also, *accept

@robb
robb Mar 14, 2014 Member

Those are the worst, maybe we could catch this in CI somehow?

@jspahrsummers
Member

I actually like this exactly how it is. It's easy to opt-out of validation by providing a custom identity transformer, but that shouldn't be necessary in ≥ 80% of cases.

@jspahrsummers jspahrsummers self-assigned this Mar 14, 2014
@robb
Member
robb commented Mar 14, 2014

🔢

@robb
Member
robb commented on bf0a5e7 Mar 14, 2014

maybe this can be handled in mtl_copyPropertyAttributes/ext_copyPropertyAttributes?

Member

What do you mean?

Member
robb replied Mar 14, 2014

that if property_getAttributes() doesn't give us an attributes->type, it defaults to id if we have a attributes->objectClass

Member

Assumably there is a type, and it's just more specific than we're expecting? Can you find out what it is when a class is specified?

Member
robb replied Mar 14, 2014

it seems to encode the class along with it, e.g.

(lldb) print (char *)attributes->type
(char *) $1 = 0x000000010041b310 "@"NSString""
(lldb) po propertyClass
NSString

so yeah, never mind

Member

I typically handle this with *type == *(@encode(id)).

Member
robb replied Mar 14, 2014

Should I update the above?

Member

Yeah, I think that'd be a better check.

@robb
Member
robb commented Mar 14, 2014

⚡️

@jspahrsummers
Member

💥

@jspahrsummers jspahrsummers merged commit d34a138 into 2.0-development Mar 14, 2014

1 check passed

default Build #1131175 succeeded in 41s
Details
@jspahrsummers jspahrsummers deleted the implicit-validation branch Mar 14, 2014
@jspahrsummers jspahrsummers added a commit that referenced this pull request Mar 16, 2015
@jspahrsummers jspahrsummers CHANGELOG for #251 58c66ee
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment