use throwing var methods for realm references to other objects

[tomholub]

@tomholub ability to throw from var variable was added to the Swift recently:

var activeUser: UserRealmObject {
    get throws {
        guard let user = user else { throw AppErr... }
        return user
    }
  }

This looks good. Probably for another issue/PR?

The advantage is that an exception can be captured and handled (in the general error handling somewhere in VC, wherever user action was initiated) while a forced unwrap cannot.

Originally posted by @tomholub in #1030 (comment)

[ivan-ushakov]

@tomholub
Now we use optional reference in Realm objects for linked objects, as documentation says. What we need to do in this issue?

[tomholub]

I'm thinking, when converting from RealmObject to a general model, throw if these cross-referenced objects are missing.

Then general models would not have optional objects, if we meant the objects to be required.

For example, a private key is meaningless if it doesn't belong to a particular user. So a key with a nil user on it makes no sense: should throw when we convert that from PrivateKeyRealmObject to PrivateKey (or similar - didn't check the actual names)

[ivan-ushakov]

We have KeyInfoRealmObject but when we create it, we provide user object like this:

convenience init(_ keyDetails: KeyDetails, passphrase: String?, source: KeySource, user: UserRealmObject) throws

and in KeyInfo type we don't have user property. All other places are same. We don't construct objects with optional links (the only exception is UserRealmObject with optional SessionRealmObject).

[tomholub]

Yes, but look at this:

extension KeyInfoRealmObject {
    /// associated user email
    var account: String? {
        user?.email
    }
}

user should never be nil, therefore there should be guard and throw, and it should return a String that's not optional. Usages of .account which is undesirably optional:

Same problem here:

extension ClientConfigurationRealmObject: CachedRealmObject {
    var identifier: String { userEmail ?? "" }

    var activeUser: UserRealmObject? { user }
}

actually there are two problems above:

1. activeUser is badly named, should be just "user". There is nothing active about it. And it should not be nilable, so guard and throw?
2. identifier should not be defaulted to empty string!!! If I saw this in newly added code, I'd be chopping heads :-) I must have overlooked it earlier. What exactly is an empty string for identifier? Need to guard and throw.

And so on

[ivan-ushakov]

@tomholub
In one case we could have nil active user:

extension RecipientRealmObject: CachedRealmObject {
    // Contacts can be shared between accounts
    // https://github.com/FlowCrypt/flowcrypt-ios/issues/269
    var activeUser: UserRealmObject? { nil }

    var identifier: String { email }
}

How to deal with this case?

[ivan-ushakov]

activeUser is badly named, should be just "user". There is nothing active about it. And it should not be nilable, so guard and throw?

This will overlap with user property in object. We have different semantic here: in object we could have optional user link because Realm requires it, but in cache protocol we should have user.

protocol CachedRealmObject: Object {
    associatedtype Identifier: Equatable
    var identifier: Identifier { get }
    var activeUser: UserRealmObject { get }
}

[tomholub]

@tomholub In one case we could have nil active user:

extension RecipientRealmObject: CachedRealmObject {
    // Contacts can be shared between accounts
    // https://github.com/FlowCrypt/flowcrypt-ios/issues/269
    var activeUser: UserRealmObject? { nil }

    var identifier: String { email }
}

How to deal with this case?

There is no reason for RecipientRealmObject to inherit of CachedRealmObject. It should be separated in any case.

Then that will also solve this problem.

[tomholub]

activeUser is badly named, should be just "user". There is nothing active about it. And it should not be nilable, so guard and throw?

This will overlap with user property in object. We have different semantic here: in object we could have optional user link because Realm requires it, but in cache protocol we should have user.

protocol CachedRealmObject: Object {
    associatedtype Identifier: Equatable
    var identifier: Identifier { get }
    var activeUser: UserRealmObject { get }
}

Then we can call it owner maybe?

[ivan-ushakov]

@Kharchevskyi

Could you please explain what was the idea of EncryptedCacheService? Is it the same as EncryptedStorage?

For example if we want to remove CachedRealmObject conformance from RecipientRealmObject, we can not use this:

struct LocalContactsProvider {
    private let localContactsCache: EncryptedCacheService<RecipientRealmObject>
    let core: Core

    init(
        encryptedStorage: EncryptedStorageType = EncryptedStorage(),
        core: Core = .shared
    ) {
        self.localContactsCache = EncryptedCacheService<RecipientRealmObject>(encryptedStorage: encryptedStorage)
        self.core = core
    }
}

[ivan-ushakov]

@tomholub

I think it makes sense to wait for #1117.

[tomholub]

yes.. sorry about another ridiculous PR

[Kharchevskyi]

@Kharchevskyi

Could you please explain what was the idea of EncryptedCacheService? Is it the same as EncryptedStorage?

For example if we want to remove CachedRealmObject conformance from RecipientRealmObject, we can not use this:

struct LocalContactsProvider {
    private let localContactsCache: EncryptedCacheService<RecipientRealmObject>
    let core: Core

    init(
        encryptedStorage: EncryptedStorageType = EncryptedStorage(),
        core: Core = .shared
    ) {
        self.localContactsCache = EncryptedCacheService<RecipientRealmObject>(encryptedStorage: encryptedStorage)
        self.core = core
    }
}

Initially idea was to have some service responsible for caching with injected storage, so Realm is not exposed all over the app. For convenience you are dealing with models, not realm models, so on client side of this service there is only object which conforms to CachedRealmObject (was different naming). So realm is not exposed to all over the app

Initially, I wanted to make it more generic with Object and storage for the object. But now I'm not sure it's already needed.
So, you can get rid of it, if it's not needed anymore.

[Kharchevskyi]

also from usage perspective you don't need to deal with realm interface to get, remove or save objects

guard let objectToDelete = realm
            .objects(T.self)
            .first(where: { $0.identifier == identifier })
        else { return }

        try? realm.write {
           realm.delete(objectToDelete)
        }

[tomholub]

just a note, this

       try? realm.write {
           realm.delete(objectToDelete)
        }

the ? should be avoided, in all similar situations we should throw and let the exception bubble upwards the chain all the way to whichever Task originated the call, where the error will be handled like any other Error.