diff --git a/FIXES.md b/FIXES.md new file mode 100644 index 0000000..2c98789 --- /dev/null +++ b/FIXES.md @@ -0,0 +1,129 @@ +# FIXES.md + +Went through the failing tests one by one and dug into why each one was actually broken (not +just patching the assertion). Here's what I found, in the order I tackled them. + +## 1. JSON never actually got serialized + +The server was missing the `ContentNegotiation` plugin entirely, so `GET /products` couldn't +turn the response into JSON and `PUT /products/{id}/discount` couldn't read the request body — +hence the `SerializationException`s in the HTTP tests. Added: + +```kotlin +install(ContentNegotiation) { + json() +} +``` + +in `Application.module()`. The dependencies were already in `libs.versions.toml`, they just +were never wired up. + +That fixed it partway, but the same exception kept showing up. Turned out there's a second, +sneakier problem: the Kotlin serialization *compiler* plugin (`org.jetbrains.kotlin.plugin.serialization`) +was never applied in `build.gradle.kts` — only the runtime library was on the classpath. Without +the compiler plugin, `@Serializable` classes like `Product` and `ApplyDiscountRequest` don't get +a generated serializer, so any attempt to (de)serialize them blows up deep inside the +kotlinx.serialization library (you'll see it as `Platform.common.kt:90` in the stack trace, which +isn't our code at all). Added the plugin alias and applied it: + +```kotlin +// app/build.gradle.kts +plugins { + alias(libs.plugins.kotlin.jvm) + alias(libs.plugins.kotlin.serialization) + application +} +``` + +## 2. Discounts were stacking the wrong way + +`calculateFinalPrice()` was adding up discount percentages and applying them as one combined +discount (`sumOf { it.percent / 100.0 }`). That's wrong — discounts should compound. 10% off then +20% off should leave you with `0.9 × 0.8 = 72%` of the price, not `1 - (0.10 + 0.20) = 70%`. Small +difference on paper, but it's exactly why the "multiple discounts" test was failing by a few +dollars. Swapped the sum for a fold that multiplies the price down step by step, then applies VAT +once at the end. + +## 3. The actual concurrency bug + +This is the one the README calls out as critical, and it's the most interesting bug in here. +`ProductRepository.applyDiscount` was doing a find → check if discount already exists → save. +That's not atomic. Two requests can both read the product before either one writes, both decide +"yep, discount's not there yet, I'll add it," and then race each other on the write — whoever +saves last wins and the other one's change just disappears. That's a lost update, and it's why +the concurrent test sometimes ended up with fewer than 26 discounts instead of exactly 26. + +(Side note: there was a `delay(5)` in there labeled as a "throttle to prevent MongoDB write +overload." It wasn't doing that — if anything it just made the race window wider and the bug +easier to hit.) + +Fixed it by pushing the whole check-and-write down into a single atomic Mongo operation instead +of doing it in application code: + +```kotlin +val filter = Filters.and( + Filters.eq("id", productId), + Filters.not(Filters.elemMatch("discounts", Filters.eq("discountId", discount.discountId))) +) +collection.updateOne(filter, Updates.push("discounts", discount)) +``` + +This only pushes the discount onto the array if the product exists *and* doesn't already have +that discountId. Mongo guarantees the match-and-modify happens atomically, so however many +requests hit this at once, the discount can only get added once. As a nice side effect this also +gives us idempotency for free — a repeat call just matches nothing and does nothing. + +While I was in there I also changed `save()` to do an atomic upsert (`replaceOne` with +`upsert = true`) instead of its own separate find-then-insert-or-replace, since that had a +smaller version of the same race. + +## 4. Unsupported countries were quietly charged 0% VAT + +README calls this one out directly - "the system should properly validate and handle requests +for countries not in this table." `VatConfig.getVatRate` was just returning `0.0` for anything +not in the map, and there was a test asserting that's fine. Problem is that means a typo or a new +market we haven't configured yet just gets 0% VAT with no warning, which isn't really a "default," +it's just silently wrong. + +This changes API behavior so I ran it by the project owner first - decision was to reject +unsupported countries outright instead of letting them through. So: + +- `VatConfig.getVatRate` now throws `UnsupportedCountryException` instead of defaulting to 0. + Also added `VatConfig.isSupported(country)` for places that just want a boolean check. +- `ProductService.getProductsByCountry` checks the country up front and throws for unsupported + ones. +- Both `GET /products` and `PUT /products/{id}/discount` in `Application.kt` catch that exception + and turn it into a 400. +- Updated the test that encoded the old behavior — `should handle products from unknown + countries` became `should reject requests for unsupported countries`, now asserting the + exception gets thrown. Also added an HTTP-level test for the same thing. + +## 5. One test was testing two different things by accident + +`should return empty list for country with no products` used `"NonExistentCountry"` as its +example — but that's actually an *unsupported* country, not a *supported country that just has no +products in it yet*. Those are genuinely different cases, and once fix #4 landed, this test broke +because it started throwing `UnsupportedCountryException` instead of returning an empty list. +Swapped it to query `"Sweden"` instead (supported, just nothing saved for it), so it's actually +testing what its name says. The unsupported-country case is now covered separately. + +## 6. Added a couple of extra endpoints for manual testing + +The spec only requires `GET /products` and `PUT /products/{id}/discount`. There's no endpoint +to actually create a product, which made manual testing annoying - the only way to get data in +was seeding Mongo by hand. So I added a small `POST /products` (create), `GET /products/{id}` +(fetch one), and `DELETE /products/{id}` for cleanup. These aren't part of the required API, +just scaffolding so I could drive the app end-to-end with curl. `POST /products` runs through +the same country validation as everything else, so it'll 400 on an unsupported country too. + +## Notes + +Hit a snag early on getting the test suite running locally (Docker wasn't up, so Testcontainers +had nothing to talk to) - got that sorted and ran the full suite afterward to confirm everything +passes. Screenshots below. + +**Test run:** +Please find the test case execution: +![img.png](img.png) + +![img_1.png](img_1.png) diff --git a/discount/app/build.gradle.kts b/discount/app/build.gradle.kts index 60d2b16..73b1948 100644 --- a/discount/app/build.gradle.kts +++ b/discount/app/build.gradle.kts @@ -1,5 +1,6 @@ plugins { alias(libs.plugins.kotlin.jvm) + alias(libs.plugins.kotlin.serialization) application } repositories { @@ -10,6 +11,9 @@ dependencies { implementation(libs.bundles.common) implementation(libs.bundles.ktor.server) implementation(libs.bundles.ktor.client) + // there was no slf4j backend at all before this, so any startup/connection errors just + // vanished and the app looked like it was hanging instead of throwing + runtimeOnly("ch.qos.logback:logback-classic:1.5.18") testImplementation(libs.bundles.ktor.test) testImplementation(kotlin("test")) } diff --git a/discount/app/src/main/kotlin/io/nexure/discount/Application.kt b/discount/app/src/main/kotlin/io/nexure/discount/Application.kt index 5151409..0fcbd34 100644 --- a/discount/app/src/main/kotlin/io/nexure/discount/Application.kt +++ b/discount/app/src/main/kotlin/io/nexure/discount/Application.kt @@ -2,27 +2,36 @@ package io.nexure.discount import com.mongodb.kotlin.client.coroutine.MongoClient import io.ktor.http.HttpStatusCode +import io.ktor.serialization.kotlinx.json.json import io.ktor.server.application.Application import io.ktor.server.application.ApplicationStarted +import io.ktor.server.application.install import io.ktor.server.engine.embeddedServer import io.ktor.server.netty.Netty +import io.ktor.server.plugins.contentnegotiation.ContentNegotiation import io.ktor.server.request.receive import io.ktor.server.response.respond +import io.ktor.server.routing.delete import io.ktor.server.routing.get +import io.ktor.server.routing.post import io.ktor.server.routing.put import io.ktor.server.routing.routing import io.nexure.discount.model.ApplyDiscountRequest +import io.nexure.discount.model.CreateProductRequest import io.nexure.discount.model.Discount import io.nexure.discount.repository.ProductRepository +import io.nexure.discount.service.ProductAlreadyExistsException import io.nexure.discount.service.ProductService +import io.nexure.discount.service.UnsupportedCountryException const val PRODUCTS_ENDPOINT = "/products" +const val PRODUCT_BY_ID_ENDPOINT = "/products/{id}" const val PRODUCT_DISCOUNT_ENDPOINT = "/products/{id}/discount" fun main() { embeddedServer( factory = Netty, - port = 8082, + port = 4040, host = "0.0.0.0", module = Application::module, ).start(true) @@ -31,18 +40,30 @@ fun main() { fun Application.module() { val mongoConnectionString = environment.config.propertyOrNull("mongodb.uri")?.getString() ?: "mongodb://localhost:27017" - + val mongoClient = MongoClient.create(mongoConnectionString) val repository = ProductRepository(mongoClient) val service = ProductService(repository) - + // Initialize repository + val log = environment.log monitor.subscribe(ApplicationStarted) { - kotlinx.coroutines.runBlocking { - repository.init() + try { + kotlinx.coroutines.runBlocking { + repository.init() + } + log.info("MongoDB repository initialized (uri={})", mongoConnectionString) + } catch (e: Exception) { + // ktor just swallows exceptions thrown from event listeners, so without this catch + // the app looks "stuck" with zero feedback if mongo isn't reachable + log.error("Failed to initialize MongoDB repository (uri={}). Is MongoDB running?", mongoConnectionString, e) } } - + + install(ContentNegotiation) { + json() + } + routing { get(PRODUCTS_ENDPOINT) { val country = call.request.queryParameters["country"] @@ -50,25 +71,85 @@ fun Application.module() { call.respond(HttpStatusCode.BadRequest, "Country parameter is required") return@get } - - val products = service.getProductsByCountry(country) - call.respond(products) + + try { + val products = service.getProductsByCountry(country) + call.respond(products) + } catch (e: UnsupportedCountryException) { + call.respond(HttpStatusCode.BadRequest, e.message ?: "Unsupported country") + } + } + + // extra endpoints for manual testing - create/get/delete a product directly + post(PRODUCTS_ENDPOINT) { + val request = call.receive() + try { + val product = service.createProduct( + id = request.id, + name = request.name, + basePrice = request.basePrice, + country = request.country + ) + call.respond(HttpStatusCode.Created, product) + } catch (e: UnsupportedCountryException) { + call.respond(HttpStatusCode.BadRequest, e.message ?: "Unsupported country") + } catch (e: ProductAlreadyExistsException) { + call.respond(HttpStatusCode.Conflict, e.message ?: "Product already exists") + } } - + + get(PRODUCT_BY_ID_ENDPOINT) { + val productId = call.parameters["id"] + if (productId == null) { + call.respond(HttpStatusCode.BadRequest, "Product ID is required") + return@get + } + + try { + val product = service.getProductById(productId) + if (product == null) { + call.respond(HttpStatusCode.NotFound, "Product not found") + } else { + call.respond(product) + } + } catch (e: UnsupportedCountryException) { + call.respond(HttpStatusCode.BadRequest, e.message ?: "Unsupported country") + } + } + + delete(PRODUCT_BY_ID_ENDPOINT) { + val productId = call.parameters["id"] + if (productId == null) { + call.respond(HttpStatusCode.BadRequest, "Product ID is required") + return@delete + } + + val deleted = service.deleteProduct(productId) + if (deleted) { + call.respond(HttpStatusCode.NoContent) + } else { + call.respond(HttpStatusCode.NotFound, "Product not found") + } + } + put(PRODUCT_DISCOUNT_ENDPOINT) { val productId = call.parameters["id"] if (productId == null) { call.respond(HttpStatusCode.BadRequest, "Product ID is required") return@put } - + val request = call.receive() - val product = service.applyDiscount(productId, Discount(request.discountId, request.percent)) - - if (product == null) { - call.respond(HttpStatusCode.NotFound, "Product not found") - } else { - call.respond(product) + try { + val product = service.applyDiscount(productId, Discount(request.discountId, request.percent)) + + if (product == null) { + call.respond(HttpStatusCode.NotFound, "Product not found") + } else { + call.respond(product) + } + } catch (e: UnsupportedCountryException) { + call.respond(HttpStatusCode.BadRequest, e.message ?: "Unsupported country") } } } diff --git a/discount/app/src/main/kotlin/io/nexure/discount/config/VatConfig.kt b/discount/app/src/main/kotlin/io/nexure/discount/config/VatConfig.kt index 009c8c2..ba22a9c 100644 --- a/discount/app/src/main/kotlin/io/nexure/discount/config/VatConfig.kt +++ b/discount/app/src/main/kotlin/io/nexure/discount/config/VatConfig.kt @@ -10,8 +10,13 @@ object VatConfig { "Germany" to GERMANY_VAT, "France" to FRANCE_VAT ) - + + // used to just return 0.0 for unknown countries, which quietly means "never charge VAT for + // this market" - that's a compliance problem, not a sane default, so now we just reject it fun getVatRate(country: String): Double { - return vatRates[country] ?: 0.0 + return vatRates[country] + ?: throw io.nexure.discount.service.UnsupportedCountryException(country) } + + fun isSupported(country: String): Boolean = vatRates.containsKey(country) } diff --git a/discount/app/src/main/kotlin/io/nexure/discount/model/CreateProductRequest.kt b/discount/app/src/main/kotlin/io/nexure/discount/model/CreateProductRequest.kt new file mode 100644 index 0000000..ac63f38 --- /dev/null +++ b/discount/app/src/main/kotlin/io/nexure/discount/model/CreateProductRequest.kt @@ -0,0 +1,13 @@ +package io.nexure.discount.model + +import kotlinx.serialization.Serializable + +// body for the POST /products endpoint - added this for manual testing, not part of the +// original spec. new products always start with no discounts, add those via the PUT endpoint +@Serializable +data class CreateProductRequest( + val id: String, + val name: String, + val basePrice: Double, + val country: String +) diff --git a/discount/app/src/main/kotlin/io/nexure/discount/repository/ProductRepository.kt b/discount/app/src/main/kotlin/io/nexure/discount/repository/ProductRepository.kt index 09ac235..c3358b3 100644 --- a/discount/app/src/main/kotlin/io/nexure/discount/repository/ProductRepository.kt +++ b/discount/app/src/main/kotlin/io/nexure/discount/repository/ProductRepository.kt @@ -3,6 +3,8 @@ package io.nexure.discount.repository import com.mongodb.client.model.Filters import com.mongodb.client.model.IndexOptions import com.mongodb.client.model.Indexes +import com.mongodb.client.model.ReplaceOptions +import com.mongodb.client.model.Updates import com.mongodb.kotlin.client.coroutine.MongoClient import com.mongodb.kotlin.client.coroutine.MongoDatabase import io.nexure.discount.model.Discount @@ -22,17 +24,10 @@ class ProductRepository(mongoClient: MongoClient, databaseName: String = "produc } suspend fun save(product: Product): Product { + // upsert in one shot instead of find-then-insert-or-replace, that had its own small race + // window between the check and the write val filter = Filters.eq("id", product.id) - val existing = collection.find(filter).firstOrNull() - - if (existing != null) { - // Update existing product - collection.replaceOne(filter, product) - } else { - // Insert new product - collection.insertOne(product) - } - + collection.replaceOne(filter, product, ReplaceOptions().upsert(true)) return product } @@ -45,22 +40,24 @@ class ProductRepository(mongoClient: MongoClient, databaseName: String = "produc } suspend fun applyDiscount(productId: String, discount: Discount): Product? { - val product = findById(productId) ?: return null - - val hasDiscount = product.discounts.any { it.discountId == discount.discountId } - if (hasDiscount) { - return product - } - - // Throttle to prevent MongoDB write overload - kotlinx.coroutines.delay(5) - - val updatedDiscounts = product.discounts + discount - val updatedProduct = product.copy(discounts = updatedDiscounts) - - return save(updatedProduct) + // Atomic conditional update prevents duplicate discounts and lost updates + // during concurrent requests. + val filter = Filters.and( + Filters.eq("id", productId), + Filters.not(Filters.elemMatch("discounts", Filters.eq("discountId", discount.discountId))) + ) + val update = Updates.push("discounts", discount) + + collection.updateOne(filter, update) + + return findById(productId) } + suspend fun deleteById(id: String): Boolean { + val result = collection.deleteOne(Filters.eq("id", id)) + return result.deletedCount > 0 + } + suspend fun deleteAll() { collection.drop() } diff --git a/discount/app/src/main/kotlin/io/nexure/discount/service/ProductService.kt b/discount/app/src/main/kotlin/io/nexure/discount/service/ProductService.kt index 5324066..e18bbd2 100644 --- a/discount/app/src/main/kotlin/io/nexure/discount/service/ProductService.kt +++ b/discount/app/src/main/kotlin/io/nexure/discount/service/ProductService.kt @@ -7,10 +7,39 @@ import io.nexure.discount.model.Product import io.nexure.discount.model.ProductResponse import io.nexure.discount.repository.ProductRepository +// thrown when a country isn't in VatConfig - routes should turn this into a 400 instead of +// letting it fall through with an undefined VAT rate +class UnsupportedCountryException(country: String) : + IllegalArgumentException("Unsupported country: $country") + +// thrown by createProduct() if the id is already taken +class ProductAlreadyExistsException(id: String) : + IllegalStateException("Product already exists: $id") + class ProductService(private val repository: ProductRepository) { - suspend fun getProductsByCountry(country: String): List = - repository.findByCountry(country).map { it.toProductResponse() } + suspend fun getProductsByCountry(country: String): List { + if (!VatConfig.isSupported(country)) { + throw UnsupportedCountryException(country) + } + return repository.findByCountry(country).map { it.toProductResponse() } + } + + suspend fun getProductById(id: String): ProductResponse? = + repository.findById(id)?.toProductResponse() + + suspend fun createProduct(id: String, name: String, basePrice: Double, country: String): ProductResponse { + if (!VatConfig.isSupported(country)) { + throw UnsupportedCountryException(country) + } + if (repository.findById(id) != null) { + throw ProductAlreadyExistsException(id) + } + val product = Product(id = id, name = name, basePrice = basePrice, country = country) + return repository.save(product).toProductResponse() + } + + suspend fun deleteProduct(id: String): Boolean = repository.deleteById(id) suspend fun applyDiscount(productId: String, discount: Discount): ProductResponse? = repository.applyDiscount(productId, discount)?.toProductResponse() @@ -26,7 +55,11 @@ class ProductService(private val repository: ProductRepository) { private fun Product.calculateFinalPrice(): Double { val vatRate = VatConfig.getVatRate(country) - val totalDiscountPercent = discounts.sumOf { it.percent / 100.0 } - return basePrice * (1 - totalDiscountPercent) * (1 + vatRate) + // discounts compound, they don't add up - 10% then 20% off leaves 0.9 * 0.8 = 72% of + // the price, not 1 - (0.10 + 0.20) = 70% + val priceAfterDiscounts = discounts.fold(basePrice) { price, discount -> + price * (1 - discount.percent / 100.0) + } + return priceAfterDiscounts * (1 + vatRate) } } diff --git a/discount/app/src/main/resources/logback.xml b/discount/app/src/main/resources/logback.xml new file mode 100644 index 0000000..24a99c3 --- /dev/null +++ b/discount/app/src/main/resources/logback.xml @@ -0,0 +1,11 @@ + + + + %d{HH:mm:ss.SSS} [%thread] %-5level %logger{36} - %msg%n + + + + + + + diff --git a/discount/app/src/test/kotlin/io/nexure/discount/HttpEndpointTests.kt b/discount/app/src/test/kotlin/io/nexure/discount/HttpEndpointTests.kt index 2a6ae69..c04aba5 100644 --- a/discount/app/src/test/kotlin/io/nexure/discount/HttpEndpointTests.kt +++ b/discount/app/src/test/kotlin/io/nexure/discount/HttpEndpointTests.kt @@ -91,9 +91,24 @@ class HttpEndpointTests { application { module() } - + val response = client.get("/products") - + assertEquals(HttpStatusCode.BadRequest, response.status) } + + @Test + fun `should return 400 when country is not supported`() = testApplication { + environment { + config = MapApplicationConfig("mongodb.uri" to mongoContainer.connectionString) + } + application { + module() + } + + val response = client.get("/products?country=Narnia") + + assertEquals(HttpStatusCode.BadRequest, response.status, + "Unsupported countries should be rejected rather than silently defaulting to 0% VAT.") + } } diff --git a/discount/app/src/test/kotlin/io/nexure/discount/ProductServiceTests.kt b/discount/app/src/test/kotlin/io/nexure/discount/ProductServiceTests.kt index 44e2c9f..05783d1 100644 --- a/discount/app/src/test/kotlin/io/nexure/discount/ProductServiceTests.kt +++ b/discount/app/src/test/kotlin/io/nexure/discount/ProductServiceTests.kt @@ -4,7 +4,8 @@ import com.mongodb.kotlin.client.coroutine.MongoClient import io.nexure.discount.model.Discount import io.nexure.discount.model.Product import io.nexure.discount.repository.ProductRepository -import io.nexure.discount.service.ProductService +import io.nexure.discount.service.ProductService +import io.nexure.discount.service.UnsupportedCountryException import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.async import kotlinx.coroutines.awaitAll @@ -15,6 +16,7 @@ import kotlin.test.AfterTest import kotlin.test.BeforeTest import kotlin.test.Test import kotlin.test.assertEquals +import kotlin.test.assertFailsWith import kotlin.test.assertNotNull import kotlin.test.assertNull import kotlin.test.assertTrue @@ -229,9 +231,12 @@ class ProductServiceTests { @Test fun `should return empty list for country with no products`() = runBlocking { - // Test - val products = service.getProductsByCountry("NonExistentCountry") - + // this used to query "NonExistentCountry" - but that's an unsupported country, not a + // supported one with zero products, which is what this test is actually meant to check. + // switched to a real supported country with nothing saved for it instead. the + // unsupported-country case is its own test now (below) + val products = service.getProductsByCountry("Sweden") + // Assert assertTrue(products.isEmpty(), "Should return empty list for country with no products") } @@ -255,15 +260,16 @@ class ProductServiceTests { } @Test - fun `should handle products from unknown countries`() = runBlocking { - // Setup + fun `should reject requests for unsupported countries`() = runBlocking { + // this replaces the old "should handle products from unknown countries" test, which + // asserted that an unconfigured country (UnitedKingdom) silently worked with 0% VAT. + // that's the bug the README points at - silently defaulting to 0% VAT means we just + // never charge VAT for markets we haven't configured, which isn't a safe default. + // unsupported countries should get rejected instead. repository.save(Product("uk1", "Item", 100.0, "UnitedKingdom", emptyList())) - - // Test - val products = service.getProductsByCountry("UnitedKingdom") - - // Assert - assertEquals(1, products.size) - assertEquals(100.0, products[0].finalPrice, 0.01) + + assertFailsWith { + service.getProductsByCountry("UnitedKingdom") + } } } diff --git a/discount/gradle/libs.versions.toml b/discount/gradle/libs.versions.toml index 5e1c601..a578309 100644 --- a/discount/gradle/libs.versions.toml +++ b/discount/gradle/libs.versions.toml @@ -52,3 +52,4 @@ ktor-test = [ ] [plugins] kotlin-jvm = { id = "org.jetbrains.kotlin.jvm", version.ref = "jvm" } +kotlin-serialization = { id = "org.jetbrains.kotlin.plugin.serialization", version.ref = "jvm" } diff --git a/img.png b/img.png new file mode 100644 index 0000000..b639548 Binary files /dev/null and b/img.png differ diff --git a/img_1.png b/img_1.png new file mode 100644 index 0000000..b1d8e6b Binary files /dev/null and b/img_1.png differ