Store the raw JSON payload from the getRelays API call

[rablador]

Instead of storing the parsed relays to disk we should store the raw json response. This will help us avoid old structure being read when doing migration to new object properties in our relay related data.

---

This change is 

[linear]

IOS-814 Store the raw JSON payload from the `getRelays` API call

[pinkisemils]

Reviewable status: 0 of 13 files reviewed, 1 unresolved discussion (waiting on @rablador)

---

ios/MullvadREST/Relay/StoredRelays.swift line 25 at r1 (raw file):

    public var relays: REST.ServerRelaysResponse {
        get throws {
            try REST.Coding.makeJSONDecoder().decode(REST.ServerRelaysResponse.self, from: rawData)

How often do we get it? Could we make this lazy as to not decode the whole relay list all of the time?

[pinkisemils]

Reviewable status: 0 of 13 files reviewed, 2 unresolved discussions (waiting on @rablador)

---

ios/MullvadVPN/RelayCacheTracker/RelayCacheTracker.swift line 122 at r1 (raw file):

        )

        let now = Date()

As per the discussion earlier, I do not believe updatedAt should be changed to the current timestamp. Instead, we shouldn't change it at all or change it such that it forces a relay list update.

The reason we have the hotfix is because there will be cases where people cannot reliably reach our API - but we should do our best to fetch a new relay list if we know that the data on disk is incomplete.

[pinkisemils]

Reviewable status: 0 of 13 files reviewed, 2 unresolved discussions (waiting on @rablador)

---

ios/MullvadREST/Relay/StoredRelays.swift line 25 at r1 (raw file):

Previously, pinkisemils (Emīls Piņķis) wrote…

How often do we get it? Could we make this lazy as to not decode the whole relay list all of the time?

Also, could we not store the REST.ServerRelaysResponse as is here? The fact that this is a fallible getter gets on my nerves and inflates the diff size here.

[pinkisemils]

Reviewable status: 0 of 13 files reviewed, 2 unresolved discussions (waiting on @rablador)

---

ios/MullvadREST/Relay/StoredRelays.swift line 25 at r1 (raw file):

Previously, pinkisemils (Emīls Piņķis) wrote…

Also, could we not store the REST.ServerRelaysResponse as is here? The fact that this is a fallible getter gets on my nerves and inflates the diff size here.

The above could be achieved by deserializing the relay list from the raw data and never serializing it to disk in the first place.

[buggmagnet]

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

---

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

    public func read() throws -> StoredRelays {
        do {
            return try fileCache.read()

The first time we launch the app with this patch, reading from the cache will fail because a CachedRelays type was stored, but we're trying to decode a StoredRelays type.

In this case, we want to be able to create a StoredRelays directly from a CachedRelays.
We probably want something along those lines

    public init(cachedRelays: CachedRelays) throws {
        self.etag = cachedRelays.etag
        self.rawData = try JSONEncoder().encode(cachedRelays.relays)
        self.updatedAt = cachedRelays.updatedAt
    }

[rablador]

Reviewable status: 3 of 13 files reviewed, 3 unresolved discussions (waiting on @pinkisemils)

---

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

Previously, buggmagnet wrote…

The first time we launch the app with this patch, reading from the cache will fail because a CachedRelays type was stored, but we're trying to decode a StoredRelays type.

In this case, we want to be able to create a StoredRelays directly from a CachedRelays.
We probably want something along those lines

    public init(cachedRelays: CachedRelays) throws {
        self.etag = cachedRelays.etag
        self.rawData = try JSONEncoder().encode(cachedRelays.relays)
        self.updatedAt = cachedRelays.updatedAt
    }

Yes, with fallback on the bundled relays. As also discussed with @pinkisemils, we should do something like the above.

---

ios/MullvadREST/Relay/StoredRelays.swift line 25 at r1 (raw file):

Previously, pinkisemils (Emīls Piņķis) wrote…

The above could be achieved by deserializing the relay list from the raw data and never serializing it to disk in the first place.

I'm not sure I follow. So we shouldn't store the raw data after all...?

---

ios/MullvadVPN/RelayCacheTracker/RelayCacheTracker.swift line 122 at r1 (raw file):

Previously, pinkisemils (Emīls Piņķis) wrote…

As per the discussion earlier, I do not believe updatedAt should be changed to the current timestamp. Instead, we shouldn't change it at all or change it such that it forces a relay list update.

The reason we have the hotfix is because there will be cases where people cannot reliably reach our API - but we should do our best to fetch a new relay list if we know that the data on disk is incomplete.

If we force a new fetch here there would be no need for the hotfix either, since the idea behind the fix is to graft some new properties into old structure without the need for network calls. I think leaving it makes more sense.

[pinkisemils]

Reviewable status: 3 of 13 files reviewed, 3 unresolved discussions (waiting on @rablador)

---

ios/MullvadREST/Relay/StoredRelays.swift line 25 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

I'm not sure I follow. So we shouldn't store the raw data after all...?

We should store rawData, but fail the deserialization if we cannot deserialize rawData into ServerRelyResponse during deserializaiton. We can chat about this in person.

---

ios/MullvadVPN/RelayCacheTracker/RelayCacheTracker.swift line 122 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

If we force a new fetch here there would be no need for the hotfix either, since the idea behind the fix is to graft some new properties into old structure without the need for network calls. I think leaving it makes more sense.

We can force the fetch, but we can never rely on the fetch working. Even if we don't force a new fetch, we shouldn't update the timestamp every time the grafting takes place.

[rablador]

Reviewable status: 3 of 13 files reviewed, 3 unresolved discussions (waiting on @pinkisemils)

---

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

Previously, rablador (Jon Petersson) wrote…

Yes, with fallback on the bundled relays. As also discussed with @pinkisemils, we should do something like the above.

Done.

---

ios/MullvadVPN/RelayCacheTracker/RelayCacheTracker.swift line 122 at r1 (raw file):

Previously, pinkisemils (Emīls Piņķis) wrote…

We can force the fetch, but we can never rely on the fetch working. Even if we don't force a new fetch, we shouldn't update the timestamp every time the grafting takes place.

As discussed offline, let's keep the old date.

[rablador]

Reviewable status: 3 of 13 files reviewed, 3 unresolved discussions (waiting on @pinkisemils)

---

ios/MullvadVPN/RelayCacheTracker/RelayCacheTracker.swift line 122 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

As discussed offline, let's keep the old date.

Done.

[buggmagnet]

Reviewed 7 of 13 files at r1, 4 of 4 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @pinkisemils)

---

ios/MullvadREST/Relay/StoredRelays.swift line 25 at r1 (raw file):

Previously, pinkisemils (Emīls Piņķis) wrote…

We should store rawData, but fail the deserialization if we cannot deserialize rawData into ServerRelyResponse during deserializaiton. We can chat about this in person.

We discussed this offline, and came to the conclusion taht we can make the init failable, create this as a let property and do the JSON parsing once when we instantiate this to avoid decoding from raw JSON every time.

[rablador]

Reviewable status: 9 of 13 files reviewed, 3 unresolved discussions (waiting on @buggmagnet and @pinkisemils)

---

ios/MullvadREST/Relay/StoredRelays.swift line 25 at r1 (raw file):

Previously, buggmagnet wrote…

We discussed this offline, and came to the conclusion taht we can make the init failable, create this as a let property and do the JSON parsing once when we instantiate this to avoid decoding from raw JSON every time.

Done.

[pinkisemils]

Reviewed 7 of 7 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @buggmagnet)

[pinkisemils]

Reviewed 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @buggmagnet)

[pinkisemils]

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

[github-actions]

🚨 End to end tests failed. Please check the failed workflow run.