From f8e30ac94692fec56cb338909693de301a071aa9 Mon Sep 17 00:00:00 2001 From: Youssef Shoaib Date: Mon, 7 Aug 2023 13:36:20 +0100 Subject: [PATCH 1/2] Use Arrow's Raise for validation --- build.gradle | 2 + src/main/java/com/gildedrose/domain/ID.kt | 5 ++ .../com/gildedrose/domain/NonBlankString.kt | 7 ++ .../com/gildedrose/domain/NonNegativeInt.kt | 7 ++ .../java/com/gildedrose/domain/Quality.kt | 6 ++ src/main/java/com/gildedrose/routes.kt | 78 +++++++++++-------- 6 files changed, 74 insertions(+), 31 deletions(-) diff --git a/build.gradle b/build.gradle index d9f0c035..dc4723c2 100644 --- a/build.gradle +++ b/build.gradle @@ -43,6 +43,8 @@ dependencies { implementation "org.jooq:jooq:3.17.6" + implementation "io.arrow-kt:arrow-core:1.2.0" + testImplementation "org.junit.jupiter:junit-jupiter-api:$junitVersion" testImplementation "org.junit.jupiter:junit-jupiter-params:$junitVersion" testImplementation "org.junit.jupiter:junit-jupiter-engine:$junitVersion" diff --git a/src/main/java/com/gildedrose/domain/ID.kt b/src/main/java/com/gildedrose/domain/ID.kt index 3ad48b8b..db420397 100644 --- a/src/main/java/com/gildedrose/domain/ID.kt +++ b/src/main/java/com/gildedrose/domain/ID.kt @@ -1,11 +1,16 @@ package com.gildedrose.domain +import arrow.core.raise.Raise + @JvmInline value class ID<@Suppress("unused") T>(val value: NonBlankString) { companion object { operator fun invoke(value: String): ID? = NonBlankString(value)?.let { ID(it) } + + context(Raise) + operator fun invoke(value: String): ID = ID(NonBlankString(value)) } override fun toString() = value.toString() diff --git a/src/main/java/com/gildedrose/domain/NonBlankString.kt b/src/main/java/com/gildedrose/domain/NonBlankString.kt index c1878a3c..4823b526 100644 --- a/src/main/java/com/gildedrose/domain/NonBlankString.kt +++ b/src/main/java/com/gildedrose/domain/NonBlankString.kt @@ -1,5 +1,7 @@ package com.gildedrose.domain +import arrow.core.raise.Raise + @JvmInline value class NonBlankString private constructor(val value: String) : CharSequence by value { @@ -7,6 +9,11 @@ private constructor(val value: String) : CharSequence by value { operator fun invoke(value: String): NonBlankString? = if (value.isNotBlank()) NonBlankString(value) else null + + context(Raise) + operator fun invoke(value: String): NonBlankString = + if (value.isNotBlank()) NonBlankString(value) + else raise("String cannot be blank") } init { diff --git a/src/main/java/com/gildedrose/domain/NonNegativeInt.kt b/src/main/java/com/gildedrose/domain/NonNegativeInt.kt index a53d2491..f7b9a214 100644 --- a/src/main/java/com/gildedrose/domain/NonNegativeInt.kt +++ b/src/main/java/com/gildedrose/domain/NonNegativeInt.kt @@ -1,5 +1,7 @@ package com.gildedrose.domain +import arrow.core.raise.Raise + @JvmInline value class NonNegativeInt private constructor(val value: Int) { @@ -7,6 +9,11 @@ private constructor(val value: Int) { operator fun invoke(value: Int): NonNegativeInt? = if (value >= 0) NonNegativeInt(value) else null + + context(Raise) + operator fun invoke(value: Int): NonNegativeInt = + if (value >= 0) NonNegativeInt(value) + else raise("Integer cannot be negative") } init { diff --git a/src/main/java/com/gildedrose/domain/Quality.kt b/src/main/java/com/gildedrose/domain/Quality.kt index 819c0b49..5a84b0b3 100644 --- a/src/main/java/com/gildedrose/domain/Quality.kt +++ b/src/main/java/com/gildedrose/domain/Quality.kt @@ -1,5 +1,7 @@ package com.gildedrose.domain +import arrow.core.raise.Raise + @JvmInline value class Quality( private val value: NonNegativeInt @@ -11,6 +13,10 @@ value class Quality( operator fun invoke(value: Int): Quality? = NonNegativeInt(value)?.let { Quality(it) } + + context(Raise) + operator fun invoke(value: Int): Quality = + Quality(NonNegativeInt(value)) } override fun toString() = value.toString() diff --git a/src/main/java/com/gildedrose/routes.kt b/src/main/java/com/gildedrose/routes.kt index 013ed773..1f429f92 100644 --- a/src/main/java/com/gildedrose/routes.kt +++ b/src/main/java/com/gildedrose/routes.kt @@ -1,23 +1,25 @@ package com.gildedrose +import arrow.core.raise.* import com.gildedrose.domain.* import com.gildedrose.foundation.AnalyticsEvent +import com.gildedrose.foundation.magic import com.gildedrose.foundation.runIO import com.gildedrose.http.ResponseErrors import com.gildedrose.http.ResponseErrors.withError import com.gildedrose.http.catchAll import com.gildedrose.http.reportHttpTransactions import com.gildedrose.rendering.render +import java.time.Duration +import java.time.LocalDate +import java.time.format.DateTimeParseException import org.http4k.core.* +import org.http4k.core.body.Form import org.http4k.core.body.form import org.http4k.filter.ServerFilters import org.http4k.lens.* -import org.http4k.lens.ParamMeta.IntegerParam import org.http4k.routing.bind import org.http4k.routing.routes -import java.time.Duration -import java.time.LocalDate - val App.routes: HttpHandler get() = ServerFilters.RequestTracing() @@ -33,39 +35,53 @@ val App.routes: HttpHandler ) ) -internal fun App.addHandler(request: Request): Response { - val idLens = FormField.nonBlankString().map { ID(it) }.required("new-itemId") - val nameLens = FormField.nonBlankString().required("new-itemName") - val sellByLens = FormField.nullOnEmptyLocalDate().optional("new-itemSellBy") - val qualityLens = FormField.nonNegativeInt().map { Quality(it) }.required("new-itemQuality") - val formBody = Body.webForm(Validator.Feedback, idLens, nameLens, sellByLens, qualityLens).toLens() - val form: WebForm = formBody(request) - if (form.errors.isNotEmpty()) - return Response(Status.BAD_REQUEST).withError(NewItemFailedEvent(form.errors.toString())) - - val item = Item(idLens(form), nameLens(form), sellByLens(form), qualityLens(form)) - runIO { - addItem(newItem = item) - } - return Response(Status.SEE_OTHER).header("Location", "/") +internal fun App.addHandler(request: Request): Response = recover({ + val form = request.form() + val item = Item( + form.required("new-itemId") { ID(it) }, + form.required("new-itemName") { NonBlankString(it) }, + form.optional("new-itemSellBy") { it?.ifEmpty { null }?.toLocalDate() }, + form.required("new-itemQuality") { Quality(it.toIntSafe()) }) + runIO { addItem(newItem = item) } + Response(Status.SEE_OTHER).header("Location", "/") +}) { error -> + Response(Status.BAD_REQUEST).withError(NewItemFailedEvent(error)) } data class NewItemFailedEvent(val message: String) : AnalyticsEvent -fun FormField.nonNegativeInt() = - mapWithNewMeta( - BiDiMapping( - { NonNegativeInt(it.toInt()) ?: throw IllegalArgumentException("Integer cannot be negative") }, - NonNegativeInt::toString - ), IntegerParam - ) +context(Raise) +fun Form.required(name: String): String = + findSingle(name) ?: raise("formData '$name' is required") + +fun Form.optional(name: String): String? = findSingle(name) + +// The following 2 functions take care of including the invalid field name in the error message. +context(Raise) +fun Form.required(name: String, transform: context(Raise) (String) -> T): T { + val value = required(name) + return withError({ e -> "formData '$name': $e" }) { + transform(magic(), value) + } +} + +context(Raise) +fun Form.optional(name: String, transform: context(Raise) (String?) -> T): T { + val value = optional(name) + return withError({ e -> "formData '$name': $e" }) { + transform(magic(), value) + } +} -fun FormField.nullOnEmptyLocalDate() = string().map { if (it.isEmpty()) null else LocalDate.parse(it) } +// In a Raise world, these would already be defined instead of their exception-throwing counterparts +context(Raise) +fun String.toLocalDate(): LocalDate = + // This catch function is reified! So it only catches DateTimeParseException + catch({ LocalDate.parse(this) }) { raise("Invalid date format") } -fun FormField.nonBlankString(): BiDiLensSpec = - map(BiDiMapping({ s: String -> - NonBlankString(s) ?: throw IllegalArgumentException("String cannot be blank") - }, { it.toString() })) +context(Raise) +fun String.toIntSafe(): Int = + catch({ toInt() }) { raise("Invalid number format") } private fun App.listHandler( @Suppress("UNUSED_PARAMETER") request: Request From 8d6e33d4df508ad6024e51f3434e7052bfd978c9 Mon Sep 17 00:00:00 2001 From: Youssef Shoaib Date: Mon, 7 Aug 2023 13:38:25 +0100 Subject: [PATCH 2/2] Accumulate validation errors --- src/main/java/com/gildedrose/routes.kt | 21 +++++++++++--------- src/test/java/com/gildedrose/AddItemTests.kt | 2 +- 2 files changed, 13 insertions(+), 10 deletions(-) diff --git a/src/main/java/com/gildedrose/routes.kt b/src/main/java/com/gildedrose/routes.kt index 1f429f92..e4990bc1 100644 --- a/src/main/java/com/gildedrose/routes.kt +++ b/src/main/java/com/gildedrose/routes.kt @@ -36,18 +36,21 @@ val App.routes: HttpHandler ) internal fun App.addHandler(request: Request): Response = recover({ - val form = request.form() - val item = Item( - form.required("new-itemId") { ID(it) }, - form.required("new-itemName") { NonBlankString(it) }, - form.optional("new-itemSellBy") { it?.ifEmpty { null }?.toLocalDate() }, - form.required("new-itemQuality") { Quality(it.toIntSafe()) }) - runIO { addItem(newItem = item) } + with(request.form()) { + val item = zipOrAccumulate( + { required("new-itemId") { ID(it) }}, + { required("new-itemName") { NonBlankString(it) } }, + { optional("new-itemSellBy") { it?.ifEmpty { null }?.toLocalDate() } }, + { required("new-itemQuality") { Quality(it.toIntSafe()) } }, + ::Item) + runIO { addItem(newItem = item) } + } Response(Status.SEE_OTHER).header("Location", "/") -}) { error -> - Response(Status.BAD_REQUEST).withError(NewItemFailedEvent(error)) +}) { errorList -> + Response(Status.BAD_REQUEST).withError(NewItemFailedEvent(errorList.toList().toString())) } + data class NewItemFailedEvent(val message: String) : AnalyticsEvent context(Raise) diff --git a/src/test/java/com/gildedrose/AddItemTests.kt b/src/test/java/com/gildedrose/AddItemTests.kt index ae968330..0c408b46 100644 --- a/src/test/java/com/gildedrose/AddItemTests.kt +++ b/src/test/java/com/gildedrose/AddItemTests.kt @@ -186,7 +186,7 @@ class AddItemTests { app.addHandler(postWithTwoMissingFields), hasStatus(Status.BAD_REQUEST) and hasAttachedError( - NewItemFailedEvent("[formData 'new-itemId' is required, formData 'new-itemQuality' must be integer]") + NewItemFailedEvent("[formData 'new-itemId' is required, formData 'new-itemQuality': Invalid number format]") ) ) }