Termination: Cannot unarchive type from non-NSData object. about synckit HOT 9 OPEN

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. :)

from synckit.

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.

from synckit.

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

from synckit.

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

from synckit.

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.

from synckit.

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)

from synckit.

@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.

from synckit.

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?

from synckit.

Sure, sounds good

from synckit.