Attribute validation when using merge method

[FranckLetellier]

When using mergeObjectForEntityName:..., I see that updated entities are not validated if the value isnil. This happens at line 285 inGRTJSONSerialization.m`:

    if (merge && value == nil) {
        return YES;
    }

Now the problem is that when merging, I sometime have new object to insert. But if those new objects have an empty field, they are not validated. So if I have a non-optional attribut in a newly inserted object, and the value is nil, no error is created.

Could there be a way, when using the mergemethod, so that attributes in objects to insert are using the validation process, even if nil?

[AliSoftware]

To go a bit further, shouldn't mergeObjectForEntityName:… and insertObjectForEntityName:… call a common implementation to fill the (new or existing) object with the provided JSON? The code seems pretty common to me for both tasks, right?

This way the behavior of both methods would be the same (except the fact that one is merging and the other is always inserting), and especially the validation case @zerto is talking about would be called in both cases.

Something like the following structure maybe:

+ (id)insertObjectForEntityName:(NSString *)entityName
             fromJSONDictionary:(NSDictionary *)JSONDictionary
         inManagedObjectContext:(NSManagedObjectContext *)context
                          error:(NSError **)error
{
   NSManagedObject* newObject = [NSEntityDescription insertNewObjectForEntityForName:entityName inManagedObjectContext:context];
   return [self fillObject:newObject withJSON:JSONDictionary error:error];
}

+ (id)mergeObjectForEntityName:(NSString *)entityName
            fromJSONDictionary:(NSDictionary *)JSONDictionary
        inManagedObjectContext:(NSManagedObjectContext *)context
                         error:(NSError **)error
{
   ...
   NSManagedObject *existingObject = …
   if (!existingObject) {
       existingObject = [NSEntityDescription insertNewObjectForEntityForName:entityName inManagedObjectContext:context];
   }
   return [self fillObject:existingObject withJSON:JSONDictionary error:error];
}

+(void)fillObject:(NSManagedObject*)obj
         withJSON:(NSDictionary*)JSONDictionary
            error:(NSError**)error
{
    ... // Parsing the JSON and filling the NSManagedObject's attributes with its values
}

[gonzalezreal]

@zerto, @AliSoftware
Hi guys! I just came back from a short holiday. I hope I am not too late for the discussion 😄.

I see your point, but if value is nil is because it is not present in the dictionary, otherwise it will be [NSNull null].

In the merge case, we don't want to nil attributes that have been set before unless they are explicitly null in the JSON.

Consider we insert the following JSON:

{
    "id": "1699",
    "name": "Batman",
    "real_name": "Guille Gonzalez",
    "powers": [
        {
            "id": "4",
            "name": "Agility"
        },
        {
            "id": "9",
            "name": "Insanely Rich"
        }
    ],
    "publisher": {
        "id": "10",
        "name": "DC Comics"
    }
}

And later we want to fix the real name value by merging this:

{
    "id": "1699",
    "real_name": "Bruce Wayne"
}

As the code iterates over the attributes (and relationships) when it reaches the name attribute, the corresponding key path in the dictionary will be nil, that is, not present. Which means, only when merging, that we should not touch that attribute. The same thing happens with the rest of the attributes and relationships.

If we want to nil out some attribute or relationship when merging, we should be explicit about it. The following JSON will nil out the powers relationship and perform the validation as well:

{
    "id": "1699",
    "real_name": "Bruce Wayne",
    "powers": null
}

Maybe this is a matter of personal taste, but for me this is the desired behaviour and something I was missing when using Mantle with Core Data. I also wanted to make two separate code paths for insertion and merge for clarity and performance reasons.

I hope shed some light on the merge process. Let me know if you have any other questions.

[FranckLetellier]

Thanks for the answers, you were not too late ^^. I see what you mean, and I agree with it.
However, my point was for newly inserted objects using the merge method. Let's say we open the app for the first time, so we can safely use the insert method:

[
 {
    "id": "1699",
    "name": "Batman",
    "real_name": "Guille Gonzalez"
 },
 {
    "id": "1700",
    "name": "Superman",
    "real_name": "Franck Letellier"
 }
]

So here everything is check and validated (let's say id, name and real_name are mandatory). Now, later during the day, the database has been updated, and the user fetch this JSON:

[
{
    "id": "1701",
    "name": "Spiderman"
 },
 {
    "id": "1699",
    "real_name": "Bruce Wayne"
 },
 {
    "id": "1700",
    "real_name": "Clark Kent"
 }
]

Here, we surely want to use the merge method, because Batman and Superman have been updated. We also have a new object in the database, Spiderman. However, something went wrong (data loss, bad server implementation...), and the real_name for Spiderman is not present. This object will not be checked.

Then, when we save the context, an error will arise, because real_name is mandatory.

I know this is a worst case scenario, and it will probably never appear (the property real_name would probably be explicitly null if there was an error, or maybe we could set a default value to avoid such case) but still.

_

[gonzalezreal]

Oh! I see. That use case should be easy to implement. I'll find some time tomorrow and submit a pull request. Your feedback will be welcome.

[gonzalezreal]

I think I've been a bit optimistic. This is not as easy to implement as I thought it would be, and would require a major refactoring. The reason is how relationship serialization is implemented.

It will take some time to fix it.

[AliSoftware]

Thanks for your reply.

As @zerto was explaining, I believe we understood the problem with nil values when merging, but there is still a behavior difference in the validation.

For example, I totally agree that, when merging, you have to be careful not to override attributes for which the key is inexistant (nil) in the JSON. But I believe that this behavior of not affecting the attribute when the key is not present (only affecting it if it has a value, even if this value is [NSNull null] but not when nil) can also be applied when importing as well (I see no drawback in that).

So in either case (merge or import) I believe you may have the same algorithm and the same behavior, including not affecting the attribute when the JSON key is absent (valueForKey returns nil), and including validation of the entity for both merge and import cases — the only difference being reusing the existing object when merging if it exists, while using a brand new entity every time in import

I believe this approach could, IMHO, simplify your code and its maintenance.

[gonzalezreal]

👍 I will look into it.

[gonzalezreal]

Better late than never. Issue was fixed while 'swiftening' Groot 😄

[gonzalezreal]

Hi there,
The fix is already available in branch 1.0 in case you want to try it.

[AliSoftware]

👍 thx for the heads up