Relay selector should use overridden ip addresses for relays

[rablador]

The relay selector should use the overridden IP address for relays which have overrides. The correct IP address should be shown in the connection details in the UI.

---

This change is 

[linear]

IOS-452 Relay selector should use overridden IP addresses for relays

[buggmagnet]

Reviewed 10 of 11 files at r1, all commit messages.
Reviewable status: 10 of 11 files reviewed, 3 unresolved discussions (waiting on @rablador)

---

ios/MullvadREST/ApiHandlers/ServerRelaysResponse.swift line 38 at r1 (raw file):

        public let includeInCountry: Bool

        public func copyWith(ipv4AddrIn: IPv4Address?) -> Self {

nit
Technically we're not doing a copy of self so I'd rather we call this either override (which functions well thematically speaking IMHO)

Alternatively, I wonder if we could make sure of the consuming operator to make sure we don't reuse this instance of the relay ?

---

ios/MullvadREST/Relay/RelayCache.swift line 70 at r1 (raw file):

    }

    private func apply(

It would be cleaner to have a wrapper class that consumes the RelayCache rather than patching it in.

That way, we don't change its implementation, and at runtime, we can do the equivalent of that.
On top of that we don't need to modify existing unit tests, just add tests for the new class and it's all good.

struct CachedRelays {}

protocol RelayCacheProtocol {
    func read() throws -> CachedRelays
    func write(record: CachedRelays) throws
}

struct IPOverrideWrapper: RelayCacheProtocol {
    let relayCache: RelayCacheProtocol

    func read() throws -> CachedRelays {
        let relays = try relayCache.read()
        // Override the IPs here
        return relays
    }
    
    func write(record: CachedRelays) throws {
        try relayCache.write(record: record)
    }
}

struct RelayCache: RelayCacheProtocol {
    func read() throws -> CachedRelays { CachedRelays() }
    func write(record: CachedRelays) throws { }
}


func relayCache(shouldOverrideIP: Bool) -> RelayCacheProtocol {
    let relayCache = RelayCache()
    if shouldOverrideIP {
        return IPOverrideWrapper(relayCache: relayCache)
    }
    return relayCache
}

---

ios/MullvadREST/Relay/RelayCache.swift line 100 at r1 (raw file):

    private func apply<T: AnyRelay>(overrides: [IPOverride], to relay: T) -> T {
        if let override = overrides.first(where: { host in

nit
I was thinking : we could rewrite it like this

        overrides.first(where: { $0.hostname == relay.hostname })
            .flatMap { relay.copyWith(ipv4AddrIn: $0.ipv4Address, ipv6AddrIn: $0.ipv6Address) }
            ?? relay

But now I'm wondering about readability 🤔

[rablador]

Reviewable status: 3 of 30 files reviewed, 3 unresolved discussions (waiting on @buggmagnet)

---

ios/MullvadREST/ApiHandlers/ServerRelaysResponse.swift line 38 at r1 (raw file):

Previously, buggmagnet wrote…

nit
Technically we're not doing a copy of self so I'd rather we call this either override (which functions well thematically speaking IMHO)

Alternatively, I wonder if we could make sure of the consuming operator to make sure we don't reuse this instance of the relay ?

You mean consuming at the call site?

---

ios/MullvadREST/Relay/RelayCache.swift line 70 at r1 (raw file):

Previously, buggmagnet wrote…

It would be cleaner to have a wrapper class that consumes the RelayCache rather than patching it in.

That way, we don't change its implementation, and at runtime, we can do the equivalent of that.
On top of that we don't need to modify existing unit tests, just add tests for the new class and it's all good.

struct CachedRelays {}

protocol RelayCacheProtocol {
    func read() throws -> CachedRelays
    func write(record: CachedRelays) throws
}

struct IPOverrideWrapper: RelayCacheProtocol {
    let relayCache: RelayCacheProtocol

    func read() throws -> CachedRelays {
        let relays = try relayCache.read()
        // Override the IPs here
        return relays
    }
    
    func write(record: CachedRelays) throws {
        try relayCache.write(record: record)
    }
}

struct RelayCache: RelayCacheProtocol {
    func read() throws -> CachedRelays { CachedRelays() }
    func write(record: CachedRelays) throws { }
}


func relayCache(shouldOverrideIP: Bool) -> RelayCacheProtocol {
    let relayCache = RelayCache()
    if shouldOverrideIP {
        return IPOverrideWrapper(relayCache: relayCache)
    }
    return relayCache
}

Done.

---

ios/MullvadREST/Relay/RelayCache.swift line 100 at r1 (raw file):

Previously, buggmagnet wrote…

nit
I was thinking : we could rewrite it like this

        overrides.first(where: { $0.hostname == relay.hostname })
            .flatMap { relay.copyWith(ipv4AddrIn: $0.ipv4Address, ipv6AddrIn: $0.ipv6Address) }
            ?? relay

But now I'm wondering about readability 🤔

I did something similar based on your idea.

[buggmagnet]

Reviewed 14 of 27 files at r2, 23 of 23 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @rablador)

---

ios/MullvadSettings/IPOverrideRepository.swift line 59 at r3 (raw file):

    public func deleteAll() {
        do {
            try readWriteLock.withLock {

nit
I wonder if it wouldn't have been easier using a dispatch queue instead 🤔

---

ios/MullvadVPN/Coordinators/Settings/IPOverride/IPOverrideViewController.swift line 244 at r3 (raw file):

    func documentPicker(_ controller: UIDocumentPickerViewController, didPickDocumentsAt urls: [URL]) {
        if let url = urls.first {
            url.securelyScoped { [weak self] scopedUrl in

nit
We can avoid the optional binding and make it a one liner with a map operation

url.securelyScoped { scopedURL in scopedURL.map { interactor.import(url: $0) } }

[rablador]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @buggmagnet)

---

ios/MullvadVPN/Coordinators/Settings/IPOverride/IPOverrideViewController.swift line 244 at r3 (raw file):

Previously, buggmagnet wrote…

nit
We can avoid the optional binding and make it a one liner with a map operation

url.securelyScoped { scopedURL in scopedURL.map { interactor.import(url: $0) } }

A little too compressed I think. I did something in-between.

[buggmagnet]

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved