Cache fails to update when updating an NSManagedObject for a value that was fetched form the database as Nil

[tslater]

What happens is that allProperties on the CMDIncrementalStoreNode is set to only those properties that have values after a fetch because of these lines:

id value = [self valueForProperty:property inStatement:statement atIndex:idx forEntity:entityDescription];     
       if (value) {
                [dictionary setObject:value forKey:obj];
                [allProperties addObject:property];
        }

We tried changing them to be:

id value = [self valueForProperty:property inStatement:statement atIndex:idx forEntity:entityDescription];     
       if (value) {
                [dictionary setObject:value forKey:obj];
        }
         [allProperties addObject:property];

but then we got an error saying something like "you cannot start a transaction within a transaction"

[tslater]

Also I don't know if this is related, but I am getting another error that might be related when I use the original caching code. The error is an unrecognized selector sent to instance where isTemporaryID is being called on an NSManagedObject rather than the objectID.

I've seen a couple of helps online:
http://www.cocoabuilder.com/archive/cocoa/292000-istemporaryid-unrecognized-selector.html
"NSManagedObject doesn't respond to -isTemporaryID. That's an
NSManagedObjectID instance method. This means that someone's sending
-isTemporaryID to the wrong object. Most commonly this is a memory
management bug—an instance of NSManagedObjectID has been deallocated
prematurely, and an instance of NSManagedObject has taken its address
in RAM. Code with a pointer to what it thinks is still an
NSManagedObjectID now sends -isTemporaryID to the NSManagedObject."

and

http://www.cocoabuilder.com/archive/cocoa/292877-istemporaryid-unrecognized-selector-how-do-debug-this.html
"Brilliant! Found the problem and fixed it. The issue was that I was not registering the inverse of a relationship in a cache node for my atomic store. I was able to track it down by going to the maintainInverseRelationship:forProperty:oldDestination:newDestination: method call in the stack, and finding the entity in register 12."

This 2nd thing makes me wonder if it is related to this cache issue and could be fixed with the same fix. I just don't understand the deal about inverse relations. Do you cache the inverse relation with the primary? I'm new to this caching stuff. Any insight would be great!

[tslater]

UPDATE:

I removed the caching code by commenting out

    {
        NSIncrementalStoreNode *node = [nodeCache objectForKey:objectID];
//        if (node) { return node; }
    }

and the isTemporaryID goes away.

[gavin-black]

Hi @tslater, thanks for finding this bug and a workaround. Did commenting out the caching section also fix the "you cannot start a transaction within a transaction" issue as well? Unfortunately the caching code wasn't written by myself so I'd need to do a bit more digging to fully understand the issue myself. I do wonder if a temporary fix might by to allow an option to turn off caching be set in "makeStoreWithOptions".

[tslater]

I actually think I've got a solution. I'm still on my own hacked branch so it might take me a while to send up a PR because i still need to sort through it. There was even more work to do but I feel that the fix is ideal. I'll give you more details soon.

[gavin-black]

@tslater, excellent. Really appreciate you keeping on top of it. I'll be on the lookout for a pull request and make sure to get it through quickly.

[sigmundfridge]

After much debugging I too have found this bug. I have a really basic sample app if anyone needs something to test with.

  //        if (node) { return node; }

does solve the issue but I'm not sure of the other consequences.

I'm assuming that as nodeCache is a dictionary (containing dictionaries) it ignores the nil value when the object is first added to the cache and so doesn't contain the key relating to that value. Without a corresponding key the value cannot be updated in the cache and subsequent calls to the cache do not return a value. Navigating back down the navigation stack to a view controller where the database isn't accessed appears to clear the cache, and when it is recreated you see the updated value.

That's my guess anyway. I'll have another look on Monday.

[sigmundfridge]

The sample project is here....

https://github.com/sigmundfridge/encrypted-core-data-nil-cache/tree/master/TestMRImage

It was a pretty quick project used to isolate the bug. You can add an Item using the plus sign. If you initially also add a photo then the imageUuid property of the Item is set and everything works as expected. If you save the Item without an image and then try to add an image you will see that the imageUUID is never set. However, if you navigate down to the start menu, and then go back to the list the Item will show the correct image.

[sigmundfridge]

This is the line that ignores adding a value to the cached node unless it is not nil

        if (value) {
            [dictionary setObject:value forKey:obj];
            [allProperties addObject:property];
        }

[sigmundfridge]

In the documentation it says

For attributes: an immutable value (an instance of a value class such as NSNumber, NSString, NSData). Missing attribute keys will assume a nil value.

so I believe this is being handled correctly when creating the node. However, for CMDIncrementalStoreNode

- (void)updateWithChangedValues:(NSDictionary *)changedValues

changedValues correctly passes the updated value (that was previously nil), but the method loops over self.allProperties, which does not contain the property to update.

It may be better to loop over changedValues first, and then add any unchanged properties to the dictionary.

[sigmundfridge]

I'm new to NSIncrementalNode so I'm a bit wary of making too many changes. However, this could work

- (void)updateWithChangedValues:(NSDictionary *)changedValues
{
  NSMutableDictionary *allValues = [NSMutableDictionary dictionaryWithDictionary:changedValues];
  for (NSPropertyDescription * key in self.allProperties) {
     id newValue = [allValues objectForKey:key.name];
     if(!newValue){
          [allValues setObject:[self valueForPropertyDescription:key] forKey:key.name];
      }
  }
 [self updateWithValues:allValues version:self.version+1];
}

However, it doesn't add the previously nil value to self.allProperties.

[sigmundfridge]

OK. I'd be interested in any responses on this, but using the method above, and replacing

 if (value) {
            [dictionary setObject:value forKey:obj];
            [allProperties addObject:property];
        }

with

        if (value) {
            [dictionary setObject:value forKey:obj];
        }
        [allProperties addObject:property];

seems to be a fix. I'll send a PR.

[sigmundfridge]

Ok. Not quite a fix.

if (value) {
      [dictionary setObject:value forKey:obj];
 }
  [allProperties addObject:property];

Causes a 'cannot start a transaction within a transaction' error

[tslater]

Ok. I'm back. So I have my own version of ECD that is now so far gone from what is in this repo that it is probably not possible to merge them together again ;-(.

Anyway, I feel like my fixes below are super close. This fixes all of the caching behavioral problems and a reproducible crash with the aforementioned unrecognized selector sent to instance where ``isTemporaryID` is being called on an NSManagedObject rather than the objectID. The only problem is that I still see that same error. This time, however, I cannot find a case to reproduce it consistently, as it happens much less frequently and seems to be a different, but related bug. Obviously since there is still a crash there is still an issue. This does fix the 'cannot start a transaction within a transaction' error that was caused by the above fix.

            NSPropertyDescription *property = [properties objectForKey:obj];
            id value = [self valueForProperty:property inStatement:statement atIndex:idx forEntity:entityDescription];
            if (value) {
                [dictionary setObject:value forKey:obj];
            }
            [allProperties addObject:property];

- (void)updateWithChangedValues:(NSDictionary *)changedValues
{
    NSMutableDictionary * updateValues = [NSMutableDictionary dictionaryWithCapacity:self.allProperties.count];
    for (NSPropertyDescription * key in self.allProperties) {
        id newValue = [changedValues objectForKey:key.name];
        id value = newValue ?: [self valueForPropertyDescription:key];
        if (value && ![value isEqual: [NSNull null]]) {
            [updateValues setObject:value forKey:key.name];
        }
    }
    [self updateWithValues:updateValues version:self.version+1];
}

        // return if nothing needs updating
        if ([keys count] == 0) {
#if USE_MANUAL_NODE_CACHE
            [node updateWithChangedValues:cacheChanges];
#endif
            return;
        }

        // prepare statement
        NSString *string = [NSString stringWithFormat:
                            @"UPDATE %@ SET %@ WHERE __objectID=?;",
                            [self tableNameForEntity:entity],
                            [columns componentsJoinedByString:@", "]];
        sqlite3_stmt *statement = [self preparedStatementForQuery:string];

        // bind values
        [keys enumerateObjectsUsingBlock:^(id obj, NSUInteger idx, BOOL *stop) {
            id value = [changedAttributes objectForKey:obj];
            id property = [properties objectForKey:obj];
#if USE_MANUAL_NODE_CACHE
            if (value && ![value isKindOfClass:[NSNull class]]) {
                [cacheChanges setObject:value forKey:obj];
            }
            else {
                [cacheChanges setObject: [NSNull null] forKey: obj];
            }
#endif
            [self
             bindProperty:property
             withValue:value
             forKey:obj
             toStatement:statement
             atIndex:(idx + 1)];
        }];

[sigmundfridge]

        id value = [self valueForProperty:property inStatement:statement atIndex:idx forEntity:entityDescription];

The code as it stands does not have entityDescription. Other than that it all seems to work for me (but then I never found the 'unrecognized selector sent to instance' error.

[gavin-black]

Hi @tslater and @sigmundfridge,

Seems like you both have a good grasp of the issue, and are very close to a fix. Is there any piece of code I should merge in at the moment, or do you think there still needs to be some more work done? You're beyond where I can really help out too much, so let me know what I can do to support getting things resolved. As always, thanks for contributing and helping keep ECD working.

[tslater]

I just keep putting off the inevitable--that I need to reconcile the public repo with what I have been doing. I have branched so far from this repo and made sooo many changes that I don't know if it's worth it anymore. If I figure out that I'm no longer going to try and merge with this repo I can try and clone down a fresh copy and make a PR with these changes. It will likely be a while though because my higher priority is to see if I can go back the current repo and merge this fix among many others...or if the current repo has some of my fixes already in it. I would do this as part of my reconciliation if it can be saved!

[tslater]

Also @sigmundfridge sorry about the entityDescription thing...again my code has diverged a lot...

[sigmundfridge]

I'll put those changes into a fresh repo and do a PR.
Give me a few hours.....

Sent from my iPad

On 10 Oct 2014, at 06:59, "tslater" notifications@github.com wrote:

Also @sigmundfridge sorry about the entityDescription thing...again my code has diverged a lot...

—
Reply to this email directly or view it on GitHub.