mentrena / SyncKit

Automatic CloudKit synchronization
https://mentrena.github.io/SyncKit/
MIT License
505 stars 57 forks source link

Termination: Cannot unarchive type from non-NSData object. #177

Open nneubauer opened 2 years ago

nneubauer commented 2 years ago

Hi,

first of all, thanks for creating this library! I am just starting with it so I might have been done something wrong here. I just tried to -- for the first time -- sync my existing data to the cloud. It also started syncing but at some point I ran into

"CloudKitSynchronizer >> Initiating synchronization" 2022-04-01 14:00:18.492960+0200 Feedibus[55977:4635609] *** Terminating app due to uncaught exception 'NSInvalidArgumentException', reason: 'Cannot unarchive type from non-NSData object.'

My core data model has a few columns that are Transformable with their custom type mostly being array types (eg. [String]) and their transformer being NSSecureUnarchiveFromData. The declaration is eg. @NSManaged public var keyPhrasesOfDuplicateGroup: [String]?.

UPDATE: A bit more insight:

The attribute in question is of type [URL], has the NSSecureUnarchiveFromData transformer and the attribute value that seems to be offending is an empty array. In my CD model, the field has the signature @NSManaged public var rawImageURLs: [URL]?.

The problematic line of code is

return transformer?.transformedValue(value)

where transformer is

(NSValueTransformer?) transformer = 0x0000600002f0c6f0 {
  baseNSValueTransformer@0 = {
    baseNSObject@0 = {
      isa = NSSecureUnarchiveFromDataTransformer
    }
  }
}

and value (is shown as Any in the debugger) prints

Printing description of value:
0 elements

Are transformable types not supported? What can I do to fix it? Any help appreciated. Thanks!

Full stack trace below:

** First throw call stack:
(
    0   CoreFoundation                      0x00000001803f25e4 __exceptionPreprocess + 236
    1   libobjc.A.dylib                     0x000000018019813c objc_exception_throw + 56
    2   Foundation                          0x000000018086b42c -[NSSecureUnarchiveFromDataTransformer reverseTransformedValue:] + 0
    3   Feedibus                            0x0000000105625368 $s16SyncKit_CoreData0cD7AdapterC16transformedValue_20valueTransformerNameypSgyp_SSSgtF + 528
    4   Feedibus                            0x000000010562ed1c $s16SyncKit_CoreData0cD7AdapterC14recordToUpload3for7context12parentEntitySo8CKRecordCAA08QSSyncedL0C_So22NSManagedObjectContextCAKSgztFyyXEfU_ySS_So22NSAttributeDescriptionCtXEfU_ + 2624
    5   Feedibus                            0x000000010563bd9c $s16SyncKit_CoreData0cD7AdapterC14recordToUpload3for7context12parentEntitySo8CKRecordCAA08QSSyncedL0C_So22NSManagedObjectContextCAKSgztFyyXEfU_ySS_So22NSAttributeDescriptionCtXEfU_TA + 68
    6   Feedibus                            0x000000010562f2a8 $sSSSo22NSAttributeDescriptionCs5Error_pIgggzo_SS3key_AB5valuetsAC_pIegnzo_TR + 44
    7   Feedibus                            0x000000010563bdc8 $sSSSo22NSAttributeDescriptionCs5Error_pIgggzo_SS3key_AB5valuetsAC_pIegnzo_TRTA.107 + 32
    8   libswiftCore.dylib                  0x000000018f757b70 $sSTsE7forEachyyy7ElementQzKXEKF + 428
    9   Feedibus                            0x000000010562de6c $s16SyncKit_CoreData0cD7AdapterC14recordToUpload3for7context12parentEntitySo8CKRecordCAA08QSSyncedL0C_So22NSManagedObjectContextCAKSgztFyyXEfU_ + 1192
    10  Feedibus                            0x0000000105630994 $s16SyncKit_CoreData0cD7AdapterC14recordToUpload3for7context12parentEntitySo8CKRecordCAA08QSSyncedL0C_So22NSManagedObjectContextCAKSgztFyyXEfU_TA + 164
    11  Feedibus                            0x0000000105614714 $sIg_Ieg_TR + 20
    12  Feedibus                            0x0000000105614774 $sIeg_IyB_TR + 28
    13  CoreData                            0x000000018587a64c developerSubmittedBlockToNSManagedObjectContextPerform + 156
    14  libdispatch.dylib                   0x0000000107299b94 _dispatch_client_callout + 16
    15  libdispatch.dylib                   0x00000001072a9768 _dispatch_async_and_wait_invoke + 188
    16  libdispatch.dylib                   0x0000000107299b94 _dispatch_client_callout + 16
    17  libdispatch.dylib                   0x00000001072a8650 _dispatch_main_queue_drain + 1064
    18  libdispatch.dylib                   0x00000001072a8218 _dispatch_main_queue_callback_4CF + 40
    19  CoreFoundation                      0x0000000180360218 __CFRUNLOOP_IS_SERVICING_THE_MAIN_DISPATCH_QUEUE__ + 12
    20  CoreFoundation                      0x000000018035a69c __CFRunLoopRun + 2432
    21  CoreFoundation                      0x0000000180359804 CFRunLoopRunSpecific + 572
    22  GraphicsServices                    0x000000018c23660c GSEventRunModal + 160
    23  UIKitCore                           0x0000000184d7bd2c -[UIApplication _run] + 992
    24  UIKitCore                           0x0000000184d808c8 UIApplicationMain + 112
    25  SwiftUI                             0x00000001ba1f7564 $s7SwiftUI17KitRendererCommon33_ACC2C5639A7D76F611E170E831FCA491LLys5NeverOyXlXpFAESpySpys4Int8VGSgGXEfU_ + 160
    26  SwiftUI                             0x00000001ba1f74c0 $s7SwiftUI6runAppys5NeverOxAA0D0RzlF + 164
    27  SwiftUI                             0x00000001b9c2aadc $s7SwiftUI3AppPAAE4mainyyFZ + 80
    28  Feedibus                            0x0000000105203f28 $s8Feedibus0A3AppV5$mainyyFZ + 40
    29  Feedibus                            0x0000000105203f68 main + 12
    30  dyld                                0x0000000107035cd8 start_sim + 20
    31  ???                                 0x00000001070e5088 0x0 + 4413345928
    32  ???                                 0xcd0b800000000000 0x0 + 14775043740007399424
)
libc++abi: terminating with uncaught exception of type NSException
nneubauer commented 2 years ago

Another update. After trying to figure out what is happening, it seems just the transform/reverseTransform are in the wrong order.

I basically replaced like so:

func transformedValue(_ value: Any, valueTransformerName: String?) -> Any? {
        if let valueTransformerName = valueTransformerName {
            let transformer = ValueTransformer(forName: NSValueTransformerName(valueTransformerName))
            return transformer?.reverseTransformedValue(value) //.transformedValue(value) <- Change I made.
        } else {
            return QSCoder.shared.data(from: value)
        }
    }

    func reverseTransformedValue(_ value: Any, valueTransformerName: String?) -> Any? {
        if let valueTransformerName = valueTransformerName {
            let transformer = ValueTransformer(forName: NSValueTransformerName(valueTransformerName))
            return transformer?.transformedValue(value) //.reverseTransformedValue(value) <- Change I made.
        } else if let data = value as? Data {
            return QSCoder.shared.object(from: data)
        } else {
            return nil
        }
    }

Could you double check if this is actually a mistake in the library and if so fix it? I could also provide a PR but I think since its just two lines and trying to confidently figure out the test suite, you might be much faster. If you can't do it in due time I'll try to figure it out anyways. Cheers. :)

BlixLT commented 2 years ago

I can add my observations regarding this issue. I am trying to adjust my app’s transformable attributes to conform upcoming requirements with secure archiving/unarchiving. I made valuetransformer’s subclass and noticed such strange thing: if my value transformer is a subclass of nsvaluetransformer class, then during managedobjectcontext’s save: method - transformer’s transformedvalue: is being called. Then I made my custom transformer subclass of NSSecureUnarchiveFromDataTransformer, and then reversedTransformedValue: is being called during save: instead. A bit weird, but IMO synckit should do something similar (depending on the transformer’s class/superclass) - use your fix if entity attribute’s transformer is secureunarchivetransformer and use the current logic if it is not secureunarchiver (but a sublclass of basic nsvaluetransformer). Unless mentrena wants to force us using only secureunarchiver transformers, then I guess your fix will be enough. Then it will likely crash for those who will update synckit, but will not update their model’s transformers to secureunarchiver.

mentrena commented 2 years ago

Just been checking this and I did indeed mix up the meaning of transformed and reverseTransformed, preparing a PR now

mentrena commented 2 years ago

The fix is now available in 1.3.1. Thanks and let me know if there are still any problems with this.

BlixLT commented 2 years ago

I am not completely convinced, that it is the correct approach (ignoring check of ValueTransformer's class/subclass). From the Apple documentation : The transformer must output an NSData object from transformedValue: and must allow reverse transformations. Of course, it is quite possible that this part of documentation is out of date. Now Apple kind of forces to use NSSecureUnarchiveFromDataTransformer (or its subclass), which acts differently from the "default" ValueTransformer and from what is written in the documentation near transformable attributes. So this will definitely work for those who updated their formatters to this new recommendation. But IMO it will crash for those who used/uses ValueTransformers the way it is mentioned in the link above. Of course, as Apple forces to update value transformers, maybe it is not a bad thing that it will crash for those who hadn't done that.

BlixLT commented 2 years ago

Btw, NSSecureUnarchiveFromDataTransformer is available from iOS 12.0+, macOS 10.14+, so it is not possible to make a subclass of it for the earlier OS versions. But I guess you can make a subclass of a basic ValueTransformer and make it return Data object from reverseTransformedValue and non-data from transformedValue. (however as I mentioned in the previous message it is different from what is written in the current version of documentation near transformable attributes)

BlixLT commented 2 years ago

@mentrena there are some problems with your tests. Firstly, unlike real apps it uses in memory store. As far as I noticed, it does not call valueTransformer's methods during context's save (if store is in-memory type). Therefore although your tests correctly test SyncKit logic, but QSNamesTransformer is unusable (will not work correctly) for CoreData managedObjectModel. There seems to be some mysterious logic with valuetransformers depending of their subclasses: as I mentioned earlier, if transformer is subclass of valueTransformer class, then during context's save transformer's transformedValue is being called. If transformer is subclass of NSSecureUnarchiveFromDataTransformer, then during context's save reverseTransformedValue. It does not happen in your test testRecordsToUploadWithLimit_transformableProperty_usesValueTransformer() probably because it is in-memory store. I added some more tests in this branch temp_transformable_attribute_tests to illustrate issues: testValueTransformer_sqlite - uses your QSNamesTransformer (subclass of ValueTransformer) and during context's save transformedValue is being called. It returns not-data, so no names are being saved to store testValueTransformer_secureUnarchive_sqlite - uses mine QSNamesSecureUnarchiverTransformer (subclass of NSSecureUnarchiveFromDataTransformer) and during context's save reversedTransformedValue is being called. This method returns Data, so everything works as expected. testRecordsToUploadWithLimit_transformableProperty_usesValueTransformer_sqlite - uses your transformer, but fails with "Record property should be of data type", because your transformer does not work with coredata's managedobjectmodel testRecordsToUploadWithLimit_transformableProperty_usesValueTransformer_secureunarchive_sqlite - uses mine QSNamesSecureUnarchiverTransformer and it works with your updated value transformers logic. So, your fix will work only with transformers that are subclasses of NSSecureUnarchiveFromDataTransformer, but will not work with ValueTransformer subclass. Maybe it is ok as Apple is warning/forcing to update valueFormatters to NSSecureUnarchiveFromDataTransformer (or its subclasses). Let me know if you need more info.

mentrena commented 2 years ago

Hmm I see, it does seem that Core Data transformable types used other type -> transformedValue -> Data, but now NSSecureUnarchiveFromDataTransformer for some reason does Data -> transformedValue -> other type, this is confusing.

I'm not sure checking the type of the transformer would be good practice though, if Apple decide to change things again in a year then the logic would break again, so I'm tempted to just leverage RecordProcessingDelegate and leave it up to each program to transform the value to some CKRecordValueProtocol type. It does mean that if you're using Transformable types you'd have to write a few lines specifically to handle the case, but feels safer to me right now 🤔

What are your thoughts on that?

BlixLT commented 2 years ago

Sure, sounds good