Sub-objects get reinitialized after parent objects during transfer of data from CloudKit

[tbaggett]

I have found the root cause of some of my problems with property values not being their expected values when retrieving data saved in CloudKit using EVCloudKitDao. I'm not sure if this can be easily or at all fixed though. Please review my observations on this and provide your thoughts.

Consider a class structure like the following:

public class A : EVCloudKitDataObject {
    public var b = B()

    public required init() {
        super.init()
        b.intProp = 9
    }
}

public class B: EVCloudKitDataObject {
    public required init() {
        super.init()
        intProp = 5
    }

    public var intProp = 0 {
        didSet {
            EVLog("intProp set to \(intProp)")
        }
    }

    // NOT storing the intProp property value in CloudKit is key to this issue
    override public func propertyMapping() -> [(String?, String?)] {
        return [("intProp", nil)]
    }
}

If you set breakpoints in Class A's assignment of 9 to the b.intProp property and the EVLog("intProp set to...") statements, you will see the following behavior in action.

If an instance of class A is saved to CloudKit using EVCloudKitDao, then retrieved, the value of the "b" variable in the class A instance will be 5, not 9 as expected. This is due to a second instance of class B being directly instantiated by EVCloudKitDao and assigned to the class A instance's "b" property after it has already been instantiated from an instance of Class A. This also means sub-objects are instantiated multiple times, one per level of however deep they are nested in the root EVCloudKitDataObject being saved.

Is it possible to assign stored values to the already-instantiated sub-objects instead of instantiating new instances? If not, I believe this means I will have to remove any objects that I was expecting EVCloudKitDao to ignore using the propertyMapping override.

[evermeer]

I think I have to debug this issue to see what's really going on. It should be in the code where the object structure is created based on a dictionary. I think the moment it's at a sub object and want to parse the sub dictionary to that it creates a new object without checking if there is already a default object set. If that's the case, then I think I should be able to test if there is already an object and then use that instead of a new one. I will have a look at it this weekend.

[tbaggett]

Yes, that is exactly what is happening. In my example code, if I set a breakpoint in class B's init method, I first see it called from class A's initializer, which was called from swiftClassFromString when the parent class A was initialized. If I proceed, class B's init breakpoint is hit again, except this time it is being called directly from swiftClassFromString.

This code always creates a new instance of the object:

public class func swiftClassFromString(className: String) -> NSObject? {
    var result: NSObject? = nil
    if className == "NSObject" {
        return NSObject()
    }
    if let anyobjectype : AnyObject.Type = swiftClassTypeFromString(className) {
        if let nsobjectype : NSObject.Type = anyobjectype as? NSObject.Type {
            //** INSTANTIATION ALWAYS OCCURS HERE
            let nsobject: NSObject = nsobjectype.init()
            //**
            result = nsobject
        }
    }
    return result
}

After always being called by:

private class func dictToObject<T where T:NSObject>(type:String, original:T? ,dict:NSDictionary) -> T? {
    //** INSTANTIATION ALWAYS REQUESTED HERE
    if var returnObject:NSObject = swiftClassFromString(type) {
    //**
        if let evResult = returnObject as? EVObject {
            returnObject = evResult.getSpecificType(dict)
        }
        returnObject = setPropertiesfromDictionary(dict, anyObject: returnObject)
        return returnObject as? T
    }
    return nil
}

The fix will need to occur further upstream, where it is known that a sub-object is being restored and to check for it having already been initialized. Here's the call stack I see at the point of the sub-object being reinitialized:

swiftClassFromString(className: String) -> NSObject?
    dictToObject<T where T:NSObject>(type:String, original:T? ,dict:NSDictionary) -> T?
        dictionaryAndArrayConversion(fieldType:String?, original:NSObject?, var dictValue: AnyObject?) -> AnyObject?
            setPropertiesfromDictionary<T where T:NSObject>(dictionary:NSDictionary, anyObject: T) -> T
                fromDictionary(dictionary:NSDictionary, anyobjectTypeString: String) -> NSObject?
                    fromCKRecord(record: CKRecord!) -> EVCloudKitDataObject?

[tbaggett]

Did you get a chance to look at this over the weekend? If so, do you still feel this will be a straightforward fix?

[evermeer]

I was working on a version this weekend, but it kept crashing. I'm now thinking of an alternate way to fix this. I will let you know tomorow.

[tbaggett]

Okay great, thanks for the update. I appreciate the continued effort to resolve this. It seems like we're very close to a working solution!

[tbaggett]

FYI, I made a temporary modification to EVReflection that makes this work as expected for me. Please let me know if there are problems with my approach.

I modified the setPropertiesFromDictionary and dictToObject methods in EVReflection.swift:

/**
 Set object properties from a dictionary

 :parameter: dictionary The dictionary that will be converted to an object
 :parameter: anyObject The object where the properties will be set

 :returns: The object that is created from the dictionary
 */
public class func setPropertiesfromDictionary<T where T:NSObject>(dictionary:NSDictionary, anyObject: T) -> T {
    var (keyMapping, properties, types) = getKeyMapping(anyObject, dictionary: dictionary)
    for (k, v) in dictionary {
        var skipKey = false
        if let evObject = anyObject as? EVObject {
            if let mapping = evObject.propertyMapping().filter({$0.0 == k as? String}).first {
                if mapping.1 == nil {
                    skipKey = true
                }
            }
        }
        if !skipKey {
            let key = k as! String
            let mapping = keyMapping[key]
            var original:NSObject? = nil
            if mapping != nil {
                original = properties[mapping!] as? NSObject         
            } else {
                // Assign the already-instantiated object if one exists when property is mapped to nil
                original = anyObject.valueForKey(key) as? NSObject
            }
            if let dictValue = dictionaryAndArrayConversion(types[mapping ?? key], original: original, dictValue: v) {
                if (dictValue as! NSObject) != original {
                    if let mappedKey:String = keyMapping[key] {
                        setObjectValue(anyObject, key: mappedKey, value: dictValue, typeInObject: types[mappedKey])
                    } else {
                        setObjectValue(anyObject, key: key, value: dictValue, typeInObject: types[key])
                    }
                }
            }
        }
    }
    return anyObject
}

/**
 Set sub object properties from a dictionary

 :parameter: type The object type that will be created
 :parameter: original The original value in the object which is used to create a return object
 :parameter: dict The dictionary that will be converted to an object

 :returns: The object that is created from the dictionary
 */
private class func dictToObject<T where T:NSObject>(type:String, original:T? ,dict:NSDictionary) -> T? {
    var returnObject:NSObject? = original as? NSObject
    if returnObject == nil {
        returnObject = swiftClassFromString(type)
    }
    if let evResult = returnObject as? EVObject {
        returnObject = evResult.getSpecificType(dict)
    }
    if returnObject != nil {
        returnObject = setPropertiesfromDictionary(dict, anyObject: returnObject!)
        return returnObject as? T
    }
    return nil
}

The only issue these changes introduced, and I'm not sure it really is an issue, is having to implement an override of the NSObject's valueForUndefinedKey method. I had already implemented an override of the setValue:forUndefinedKey method for those properties, so it made sense that this would be needed as well.

Hope this helps!

[tbaggett]

I have continued to debug issues in my app and have run across another issue that is similar to this one. It looks to me as though a class instance is replaced with another instance whenever its state is saved. This occurs in the EVCloudData::upsertObject method, the pertinent portion of which I have copied below:

                if existingItem != nil  {
                    EVLog("Update object for filter \(filter)")

                    // MY COMMENT HERE: Class instance isn't being updated. It is being replaced!
                    data[filter]!.removeAtIndex(itemID!)
                    data[filter]!.insert(item, atIndex: itemID!)


                    NSOperationQueue.mainQueue().addOperationWithBlock {
                        (self.updateHandlers[filter]!)(item: item, dataIndex:itemID!)
                        (self.dataChangedHandlers[filter]!)()
                        self.postDataUpdatedNotification(filter, item: item, dataIndex: itemID!)
                    }
                    dataIsUpdated(filter)

In my case, I have a connection that retrieves all user-created documents (but not their content, which is stored separately) in my app. I also have a separate Optional property in a view model that references the currently opened user-created document. I assign the selected document to this property when the user opens one of their documents. The problem is, that referenced instance doesn't get updated after the first change when the above code replaces the referenced instance with another one.

Is there no practical way to update the existing object's properties instead of always replacing it with a new instance? I can create a separate issue for this if needed. Thanks.

[evermeer]

For the original issue: I did make some progress, but still 12 tests are failing. I will keep on working on it. I want to fix the issue.

As for your data object being replaced: I think it's a design issue. The connect method is meant to mirror a CloudKit data collection into an array of objects. These objects where meant to be data objects without additional functionality. In your case you are adding functionality to them.

But it would not be a big issue for me to just update the original object. I already have reflection functions that will do that. So instead of the remove and insert I could call a EVReflection function to update all properties. Is that what you meant? This is a separate issue. I will have a look at it after i finish this issue.

[evermeer]

It's sometimes so easy to overlook a stupid mistake...
All unit tests now pass.
Later this evening I will add a unittest for your scenario and see if this is now solved.

[evermeer]

I had to find out if this solved your issue. I could not wait. So I added a unit test for your scenario and it seems to be working. I just pushed a new version of EVReflection (2.16.1) to GitHub.

[evermeer]

And for your other issue it could be a quick fix by replacing the 2 lines at row 450 in EVCloudData.swift.

Change this:

        data[filter]!.removeAtIndex(itemID!)
        data[filter]!.insert(item, atIndex: itemID!)

to:

      EVReflection.setPropertiesfromDictionary(item.toDictionary(), anyObject: data[filter]![itemID!])

I will have a look at this tomorrow to see if it breaks something.

[tbaggett]

Thank you so much for diving into this and hopefully resolving it. I will test the update and your suggested fix for the object updating and get back to you later today.

[tbaggett]

I'm starting to test it now. The first difference I see is that with your changes for the sub-objects, properties that were handled by my temporary fix now require special handling in the setValue:forUndefinedKey method.

I understand that my temporary fix may have broken other things that I wasn't testing or using. Its no big deal for me to add the handling of those properties to setValue:forUndefinedKey. But I wanted to let you know about it in case you wanted to look into it.

Here's one of the warnings I now see. Note that the property being referred to isn't any of the unsupported types. All the CloudBool sub-object is adding to the AudioOptions class is a public Int property called "state".

2016-02-03 11:07:56.038 Marquees[2813:1982793] WARNING: The class 'AudioOptions' is not key value coding-compliant for the key 'muteAllAudio' 
There is no support for optional type, array of optionals or enum properties.
As a workaround you can implement the function 'setValue forUndefinedKey' for this. See the unit tests for more information

Here's an excerpt of the AudioOptions class that declares the CloudBool instance property:

public class AudioOptions: EVCloudKitDataObject {
    // ...

    // Option to mute all audio sources at this scope
    public let muteAllAudio = CloudBool()

    // ...
}

And here is the current CloudBool class definition:

public enum CloudBoolState: Int {
    case Unassigned = -1, False = 0, True = 1
}

public class CloudBool: EVCloudKitDataObject {

    public var state: Int = CloudBoolState.Unassigned.rawValue

    public convenience init(state: CloudBoolState) {
        self.init()

        assignState(state, raiseEvent: false)
    }

    public convenience init(state: Bool) {
        self.init()

        assignBool(state, raiseEvent: false)
    }

    public convenience init(state: Bool?) {
        self.init()

        assignOptionalBool(state, raiseEvent: false)
    }

    public convenience init(rawValue: Int) {
        self.init()

        if let state = CloudBoolState(rawValue: rawValue) {
            assignState(state, raiseEvent: false)
        }
    }

    public func assignState(state: CloudBoolState, raiseEvent: Bool = true) {
        if self.state != state.rawValue {
            self.state = state.rawValue
            if raiseEvent {
                didChange.raise()
            }
        }
    }

    public func assignBool(value: Bool, raiseEvent: Bool = true) {
        let newState = value ? CloudBoolState.True.rawValue : CloudBoolState.False.rawValue
        if state != newState {
            state = newState
            if raiseEvent {
                didChange.raise()
            }
        }
    }

    public func assignOptionalBool(value: Bool?, raiseEvent: Bool = true) {
        var newState = state
        if let val = value {
            if val {
                newState = CloudBoolState.True.rawValue
            } else {
                newState = CloudBoolState.False.rawValue
            }
        } else {
            newState = CloudBoolState.Unassigned.rawValue
        }

        if state != newState {
            state = newState
            if raiseEvent {
                didChange.raise()
            }
        }
    }

    public func assignRawValue(rawValue: Int, raiseEvent: Bool = true) {
        if let _ = CloudBoolState(rawValue: rawValue) {
            if state != rawValue {
                state = rawValue
                if raiseEvent {
                    didChange.raise()
                }
            }
        }
    }

    public func asBool() -> Bool {
        switch state {
        case CloudBoolState.True.rawValue:
            return true
        default:
            return false
        }
    }

    public func asOptionalBool() -> Bool? {
        switch state {
        case CloudBoolState.False.rawValue:
            return false
        case CloudBoolState.True.rawValue:
            return true
        default:
            return nil
        }
    }
}

[tbaggett]

This issue may be caused by multiple levels of sub-objects. The structure of this record has been in place for a while now, before I created this ticket, but didn't cause the setValue:forUndefinedKey to be called for the CloudBool state properties

Here is the CKRecord structure for the class this is happening in.

Record Type:
ApplicationOptions

Fields:
audio__muteAllAudio__state = Int(64)
audio__muteBackgroundMusic__state = Int(64)
audio__muteNarration__state = Int(64)
audio__muteSoundFX__state = Int(64)
audio__repeatBackgroundMusic__state = Int(64)
audio__restartBackgroundMusic__state = Int(64)
audio__selectedBackgroundMusicPath = String
autoPlayPerformance__referenceItemId = String
playback__endlessPlayback__state = Int(64)
video__defaultColorScheme__system__referenceItemId = String
video__defaultColorScheme__user__referenceItemId = String
video__defaultTransitionOptions__duration = Double
video__defaultTransitionOptions__transition__referenceItemId = String

Out of the above fields, it is giving the undefinedKey warning for:

audio__muteAllAudio__state
audio__muteBackgroundMusic__state
audio__muteSoundFX__state
video__defaultTransitionOptions__transition__referenceItemId
video__defaultColorScheme__system__referenceItemId
video__defaultColorScheme__user__referenceItemId

[tbaggett]

Good news on the EVCloudData upsertObject change! It seems to be working great for me so far. Hopefully that change won't break any of your other tests.

Also, after reverting your sub-object changes and putting my temporary fixes back in, that area is also working as expected now. Hopefully it won't be difficult to track down the new issues I ran into, or I can make the needed setValue:forUndefinedKey additions.

[tbaggett]

So I have a question regarding your recent commit that included the EVCloudData upsertObject change. I've only viewed the changes here on GitHub, but it looks like you added this code:

EVReflection.setPropertiesfromDictionary(item.toDictionary(), anyObject: data[filter]![itemID!])

after this code:

    data[filter]!.removeAtIndex(itemID!)
    data[filter]!.insert(item, atIndex: itemID!)

instead of replacing the removeAtIndex/insert combo with the setPropertiesFromDictionary call. Is that correct? Your original suggestion, and what I implemented and tested, was replacing the combo with the setPropertiesFromDicitionary call. Did you find that it needed to be done this way instead?

[evermeer]

Oops sorry, last minute change that should not have been committed. I corrected the code...
I have still one issue to do and then I will also create a new cocoapod of it