👌 IMPROVE: make repository robust in treating network failures

[violoncelloCH]

closes #360

[octogradiste]

Again massive work! Thank you so much! Consider creating a wrapper function for the supabase data source. This would avoid some duplication and make it cleaner :). Ottherwise, looks really good to me!

[octogradiste]

As you are using the try catch block as an expression you can drop the return key word (the last statement is automatically returned). You can also do this in all try catch blocks below.

[violoncelloCH]

Thanks for you feedback.
Taking a close look on this, I realize that no, this is not the case. The return is intentional here to exit the function.
If you check the line above the try (currently 49), there is a variable assignment done. Based on that variable assignment, further actions are taken. However in case of the exception the result from line 53 needs to be returned directly as otherwise it would be written back to the local database unnecessarily.

[octogradiste]

You could write a function which wraps the supabase call and reuse it everywhere. It could look like this:

/**
 * Wrapper for Supabase requests that retries the request if it fails due to a
 * [HttpRequestException]. If the request fails after [maxRetriesCount] retries, it throws the
 * exception. In case of an [NoSuchElementException], it returns null.
 *
 * @param maxRetriesCount: the number of times the request will be retried
 * @param request: the request to be executed
 */
private suspend fun <T> supabaseRequestWrapper(
    maxRetriesCount: UInt,
    request: suspend () -> T,
): T? {
    return try {
        request()
    } catch (e: NoSuchElementException) {
        null
    } catch (e: HttpRequestException) {
        if (maxRetriesCount == 0u) throw e
        delay(RETRY_DELAY_MILLI)
        return supabaseRequestWrapper(maxRetriesCount - 1u, request)
    }
}

And you could use it like this:

override suspend fun getAssociation(
    associationId: String,
    maxRetriesCount: UInt
): Association? = supabaseRequestWrapper(maxRetriesCount) {
    supabase
        .from("associations")
        .select(Columns.raw(QUERY_ASSOCIATION)) {
            filter { eq("association_id", associationId) }
        }
        .decodeSingle<AssociationSupabase>()
        .toAssociation()
}

Note that you would need to also write a wrapper fur function which return a list.

[violoncelloCH]

Thanks! Yes absolutely. I've been thinking about generalizing these. Actually there are quite a few things which are similar for each type of methods (single getters, getAlls, deletes, etc). However there are still some subtle differences for some of them and I'm unsure up to which point generalization makes sense. Even for the wrapper you're suggesting, there are actually some functions for which I check for different Exceptions.
I'll look into this and see if I can build something reasonable without breaking the tests :D

[violoncelloCH]

Okay, so I heavily reworked this. Based on your suggestion I did a wrapper function for the retry and exception handling in the SupabaseDataSource. In a similar way, i reworked the code from the RepositoryImpl to remove redundancies as much as possible. (The changes are in the last two commits)

[octogradiste]

Thanks! The code already looks much cleaner. Just some minor changes to make it even more clean ;)

[octogradiste]

You could use the normal wrapper and use ?: emptyList() so that it returns an empty list when the operation fails.

[violoncelloCH]

ohh yes, good Idea! I was too tired yesterday evening to come up with a cleaner solution 😅
thanks!

[octogradiste]

Instead of using a generic type DataObject, I'd suggest you use a sealed class DataModel for all our data model, this would make it typesafe and avoid the else branch.

[violoncelloCH]

I tried creating a supertype for the dataclasses, but somehow dataclasses can't inherit in Kotlin. I was not aware that I could use sealed classes for this. Indeed it is the best solution for this! Thank you very much!

[octogradiste]

Thank you so much! Now the code is really clean!

[sonarqubecloud]

Quality Gate failed

Failed conditions
64.85% Line Coverage on New Code (required ≥ 80.0%)

See analysis details on SonarCloud