caiyue1993 / IceCream

Sync Realm Database with CloudKit
MIT License
1.94k stars 245 forks source link

Add primary value for CKRecord name assertions #191

Closed Binlogo closed 4 years ago

Binlogo commented 4 years ago

Add assertions for issue #190

I've considered checking validation and transform to a valid name if need.

if let primaryValueString = self[primaryKeyProperty.name] as? String {
    var recordName = primaryValueString
    if primaryValueString.allSatisfy({ $0.isASCII }) {
        if let data = primaryValueString.data(using: .ascii, allowLossyConversion: true),
            let string = String(data: data, encoding: .ascii) {
            recordName = string
        }
    }
    if primaryValueString.starts(with: "_") {
        recordName = "ice" + recordName
    }
    return CKRecord.ID(recordName: primaryValueString, zoneID: Self.zoneID)
}

But I think it's better for user to decide how to handle this issue. By the way, just make assertions won't have any other side effect or migration issue.

caiyue1993 commented 4 years ago

Hi @Binlogo, the changes look great to me. I have the same idea with you that I prefer making no transforms as well. Neat!

dbmrq commented 3 years ago

I have an app on the App Store running with IceCream for a while now, and my primary keys already had non ASCII characters. Now I tried upgrading to IceCream's latest version and it crashes when trying to sync because primary keys can't contain ASCII characters anymore. However, they're already on iCloud with those keys! So this change will basically make it unusable to previous users.