Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(yki): allow saving yki suoritukset if some data has errors #599

Closed
wants to merge 2 commits into from

Conversation

saku-koodari
Copy link
Contributor

@saku-koodari saku-koodari commented Dec 18, 2024

Halutaanko edes tätä

Pihvi:

  • Tallentaa ainoastaan validit rivit tietokantaan.
  • Virheelliset rivit lokitetaan
  • Jos halutaan tuoda sama data uudestaan, ajatuksena on nukettaa vanha data.
    • Tällöin koodi taas tallentaa ainoastaan validit rivit.

Olennainen koodi.
CsvParser.kt / convertCsvToResult.kt:

fun <T> convertCsvtToResults(csvString: String): Iterable<Result<T>> {
  val csvMapper = getCsvMapper<T>()
  val iterator = csvMapper
      .readerFor(T::class.java)
      .with(getSchema<T>(csvMapper))
      .readValues<T?>(csvString)

  val data = mutableListOf<Result<T>>()
    while (iterator.hasNext()) {
      // iterator.nextValue palauttaa virheen tai heittää virheen.
     // Se wrapataan runCatching avulla Result:ksi.
      data.add(runCatching { iterator.nextValue() })
    }

  return data
}

Sen lisäksi on tulosten käsittelyä varten oma virheenkäsittely

// CsvParser.kt 
// Extension metodi Iterable<Result<T>:lle 
// eli samalle tyypille joka generoidaan yllä.
fun <T> Iterable<Result<T>>.foldWithErrors(
    continueOnError: Boolean,
    action: (Iterable<CsvExportError>) -> Unit,
): List<T> {
    // Tallennetaan virheet errors - muuttujaan
    val errors = mutableListOf<CsvExportError>()

    // data lista
    val data = mutableListOf<T>()
    
    // käydään läpi koko iterable.
    this.forEachIndexed { index, result ->
        result

            // Jos Result.failure, 
            // niin lisätän sellaiset rivit listalta errors - listaan
            .onFailure { e ->
                when (e) {
                    is InvalidFormatException -> 
                      errors.add(
                        InvalidFormatCsvExportError(index, e)
                      )
                    else -> 
                      errors.add(SimpleCsvExportError(index, e))
                }
            }

             // Lisätään kaikki onnisuneet rivit data - listaan
            .onSuccess { row -> data.add(row) }

        // Sen jälkeen, jos on virheitä, tehdään virheen käsittely
        if (errors.isNotEmpty()) {
             // custom-toiminto, minkä voi itse määrittää
             // Tätä käytetään siihen,
             // ettei lokitusta tarvitse määrittää tässä metodissa.
             action(errors)
            
            // Heitetään virhe jos continueOnError == false
            // Muussa tapauksessa mennään eteenpäin, 
            // eli palautetaan lopuksi data.
            if (!continueOnError) {
                throw RuntimeException(
                    "Unable to convert string to csv, because " +
                    "the string had ${errors.count()} error(s).",
                )
            }
        }
    }

    return data
}

Ja näitä käytetään lopuksi
YkiService.kt´ - sisällä (sekä importYkiSuoritukset, että importYkiArvioijat), esim:

// YkiService.kt / importYkiSuoritukset
val arvioijat =
  parser
    .convertCsvtToResults<SolkiArvioijaResponse>(
      response.body ?: throw Error.EmptyArvioijatResponse(),
    ).foldWithErrors(importSuorituksetContinueOnError) {
      event.addErrors(it)
    }

@saku-koodari saku-koodari self-assigned this Dec 18, 2024
@tintintti
Copy link
Contributor

Tässä tarvis sit jotenkin huomioida se, miten meidän on mahdollista hakea rajapinnasta suorituksia. Haku toimii suorituksen lastModified-kentän perusteella, joka ei todennäköisesti muutu rajapinnassa, jos esim suorittajan tai järjestäjän tietoja muutetaan. Eli jos tallennetaan vain osa haetuista suorituksista, niin pitäisi jotenkin varmistaa että pystytään hakemaan myöhemmin välistä jääneitä suorituksia jos niiden tietoja korjataan.

@wolverian
Copy link
Contributor

Tässä tarvis sit jotenkin huomioida se, miten meidän on mahdollista hakea rajapinnasta suorituksia. Haku toimii suorituksen lastModified-kentän perusteella, joka ei todennäköisesti muutu rajapinnassa, jos esim suorittajan tai järjestäjän tietoja muutetaan. Eli jos tallennetaan vain osa haetuista suorituksista, niin pitäisi jotenkin varmistaa että pystytään hakemaan myöhemmin välistä jääneitä suorituksia jos niiden tietoja korjataan.

Vaihtoehto on tallentaa kantaan kaikki haetut rivit, myös virheelliset, ja sanoa että tämä on se data joka saatiin, ja että on lähdejärjestelmän vastuulla tehdä korjaukset ja varmistaa, että rajapinnasta tulee korjatut versiot.

@saku-koodari
Copy link
Contributor Author

saku-koodari commented Dec 19, 2024

Vaihtoehto on tallentaa kantaan kaikki haetut rivit, myös virheelliset, ja sanoa että tämä on se data joka saatiin, ja että on lähdejärjestelmän vastuulla tehdä korjaukset ja varmistaa, että rajapinnasta tulee korjatut versiot.

Sit se johtaa meillä siihen, että ei voida luottaa, että yksittäinen data-rivi olisi laadukasta (ilman, että tarkastellaan sitä). Juttelin joskus toisen tiimin kanssa asiasta ja niiden suositus on pitää data mahdollisimman laadukkaana. Tässäkin projektissa me nähdään, että meidän järjestelmän sisältämä data elää pidempää kuin yksittäinen järjestelmä.

Edit: Huomasin pienen aivopierun mun logiikassa. Tehdään tähän muutos

@saku-koodari saku-koodari force-pushed the feat/yki-continue-on-error branch 2 times, most recently from 4db6b95 to 7d877b2 Compare December 20, 2024 10:37
@saku-koodari saku-koodari force-pushed the feat/yki-continue-on-error branch from 7d877b2 to 0af50fc Compare December 23, 2024 08:49
@wolverian
Copy link
Contributor

@saku-koodari Suljen tämän kunnes palataan taas aiheeseen.

@wolverian wolverian closed this Jan 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants