oharaandrew314 / dynamodb-kotlin-module

Kotlin Module for the dynamodb-enhanced SDk
Apache License 2.0
26 stars 3 forks source link

Save fails without exception and scan runs forever when a mapping error occurs #9

Closed KarolineKarkosch closed 8 months ago

KarolineKarkosch commented 1 year ago

When a mapping error occurs (e.g. when a required field is null as in this example) the current behaviour is not as expected:

updateItem

scan

The only method behaving as expected (throwing an exception) is the query-method.

Here is an example Code detailing the problem (make sure to use a test dynamodb since the test deletes all other entities before running)

import io.andrewohara.dynamokt.DataClassTableSchema
import io.andrewohara.dynamokt.DynamoKtPartitionKey
import io.kotest.matchers.shouldBe
import io.kotest.matchers.shouldNotBe
import kotlinx.coroutines.future.await
import org.junit.jupiter.api.Test
import org.springframework.beans.factory.annotation.Autowired
import org.springframework.boot.test.context.SpringBootTest
import org.springframework.test.context.ActiveProfiles
import software.amazon.awssdk.enhanced.dynamodb.DynamoDbEnhancedAsyncClient
import software.amazon.awssdk.enhanced.dynamodb.Key
import software.amazon.awssdk.enhanced.dynamodb.mapper.annotations.DynamoDbBean
import software.amazon.awssdk.enhanced.dynamodb.model.GetItemEnhancedRequest

const val TABLE_NAME = "INSERT_NAME_OF_YOUR_TABLE_HERE"

@SpringBootTest
class ScanAllExample(
    @Autowired private val enhancedClient: DynamoDbEnhancedAsyncClient,
) {

    @Test
    fun shouldSaveEntity() = runBlocking {
        val testClient = enhancedClient.table(TABLE_NAME, DataClassTableSchema(TestEntityV1::class))
        enhancedClient.table(TABLE_NAME, DataClassTableSchema(TestEntityV2::class))
        //delete item if already exists
        testClient.deleteItem(Key.builder().partitionValue("test-id").build())
        //save item with id 'test-id'
        testClient.updateItem(TestEntityV1(id = "test-id", name = null)).await()
        //get an item with 'test-id'
        val item = testClient.getItem(
            GetItemEnhancedRequest.builder()
                .consistentRead(true)
                .key(Key.builder().partitionValue("test-id").build())
                .build(),
        ).await()
        //item was not found in db (actually it was never stored - even though no exception was thrown)
        item shouldNotBe null
    }

    @Test
    fun shouldBehaveCorrectlyWithScanAll() = runBlocking {
        val legacyClient = enhancedClient.table(TABLE_NAME, DataClassTableSchema(TestEntityV1::class))
        legacyClient.scan().subscribe { list ->
            list.items().forEach {
                println("Deleting ${it.id}")
                legacyClient.deleteItem(Key.builder().partitionValue(it.id).build())
            }
        }.get()
        val valid = legacyClient.updateItem(TestEntityV1(id = "valid", name = "valid name")).await()
        val invalid = legacyClient.updateItem(TestEntityV1(id = "invalid", name = null)).await()
        valid shouldNotBe null
        invalid shouldNotBe null

        legacyClient.scan().subscribe { items ->
            items.items().forEach {
                println("Legacy Client found ${it.id} in database")
            }
        }.get()

        val newClient = enhancedClient.table(TABLE_NAME, DataClassTableSchema(TestEntityV2::class))

        val setOfFound = mutableSetOf<String>()
        println("Scanning repository")
        newClient.scan().subscribe { items ->
            items.items().forEach {
                println("New Client Found ${it.id} in database")
                setOfFound.add(it.id)
            }
        }.get()

        //the code never reaches this point

        setOfFound.size shouldBe 2
        setOfFound.contains("valid") shouldBe true
        setOfFound.contains("invalid") shouldBe true
    }
}

@DynamoDbBean()
data class TestEntityV2(
    @DynamoKtPartitionKey
    var id: String,
    val name: String,
    val otherField: String = "abc"
)

@DynamoDbBean()
data class TestEntityV1(
    @DynamoKtPartitionKey
    var id: String,
    val name: String?,
    val otherField: String = "abc"
)
oharaandrew314 commented 1 year ago

Thanks for the test cases. I'll look into a solution. Are you running these tests against real dynamo db, a fake, or local?

KarolineKarkosch commented 1 year ago

Thanks for the quick response! The tests ran against a real dynamoDB

oharaandrew314 commented 1 year ago

I'm away for several more days, so if the need is urgent, a PR is welcome!

oharaandrew314 commented 1 year ago

@KarolineKarkosch Unfortunately, I wasn't able to reproduce your issue. After adapting your tests, both pass against a real and dockerized Dynamo DB. I can't run the tests against fake dynamo, since its update expression parser isn't good enough.

Here's my adapted version of your tests in another branch. Does anything seem wrong with it? https://github.com/oharaandrew314/dynamodb-kotlin-module/blob/gh9/src/test/kotlin/io/andrewohara/dynamokt/GH9Test.kt

KarolineKarkosch commented 1 year ago

Hi @oharaandrew314

thanks for your response. I noticed that you are using DynamoDbEnhancedClient instead of DynamoDbEnhancedAsyncClient in your tests.

My assumption is that the error is caused by the asynchronous functions which may handle errors differently.

Would you mind testing it with DynamoDbEnhancedAsyncClient in your examples?

I did try to reproduce the error without the Kotlin module but since null-Fields don't cause an error in Java, missing fields don't cause an error at all so this scenario can't happen there.

oharaandrew314 commented 1 year ago

Yes, after switching to the async client, I can reproduce the scan operation running forever. You may be right that a different kind of error is expected.

KarolineKarkosch commented 1 year ago

@oharaandrew314 were you able to look into a solution yet?

oharaandrew314 commented 1 year ago

I haven't made a breakthrough yet. It's clear that some exceptions manage to bubble out of the scan operation, but I'm not sure how that's done. I've been waiting for PRs for fake dynamo to be released so I can test more easily.

oharaandrew314 commented 1 year ago

@KarolineKarkosch I've released a fix in version 0.2.4. Please let me know if it works for you!

oharaandrew314 commented 1 year ago

By the way, http4k-connect-amazon-dynamodb-fake now supports the official async SDK via an adapter from http4k-aws. It's much faster than any test container or local dynamo. Here's an example.

KarolineKarkosch commented 1 year ago

Thanks a lot for the fix! Unfortunately we are currently unable to use it because we are still on Kotlin 1.6.0 and as far as I can tell the newer versions of this library requires Kotlin 1.9.0

Would it be easy for you to provide a Kotlin 1.6.0-compatible version? Otherwise we will need to wait until our service is updated before being able to test the fix :)

oharaandrew314 commented 1 year ago

I should be able to lower the minimum stdlib.

On Thu, Sept 7, 2023, 3:52 a.m. KarolineKarkosch @.***> wrote:

Thanks a lot for the fix! Unfortunately we are currently unable to use it because we are still on Kotlin 1.6.0 and as far as I can tell the newer versions of this library requires Kotlin 1.9.0

Would it be easy for you to provide a Kotlin 1.6.0-compatible version? Otherwise we will need to wait until our service is updated before being able to test the fix :)

— Reply to this email directly, view it on GitHub https://github.com/oharaandrew314/dynamodb-kotlin-module/issues/9#issuecomment-1709650974, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAG2JSCHVUXPCPUJDEVU6ADXZF4LNANCNFSM6AAAAAA3RGQM6Y . You are receiving this because you were mentioned.Message ID: @.***>

oharaandrew314 commented 1 year ago

I've downgraded the minimum stdlib to 1.4. You should be able to use it now.

KarolineKarkosch commented 1 year ago

@oharaandrew314 thanks for your effort! Unfortunately we are still getting the same error with version 0.3.0:

Kotlin: Module was compiled with an incompatible version of Kotlin. The binary version of its metadata is 1.9.0, expected version is 1.6.0.

Our assumption is that you might need to reduce the version in this line: https://github.com/oharaandrew314/dynamodb-kotlin-module/blob/8fd23bf1e0d5a358d3c3ae54eb361bc3036f523c/build.gradle.kts#L2

oharaandrew314 commented 1 year ago

It seems I cannot do that. If I downgrade the Kotlin version, then the project can't compile against its own dependencies.

I'm afraid you'll just have to communicate to your team the importance of keeping your dependencies up to date.

PoisonedYouth commented 8 months ago

I just checked the updateItem - function and if I use the test provided above I still don't get an exception in case I try to update an item that does not exist. The scan - function works fine and throws an exception in case of a defective item.

Both are tested with the DynamoDbEnhancedAsyncClient version using the newly published 0.8.0 version of dynamodb-kotlin-module.

Can you please verify this?

oharaandrew314 commented 8 months ago

This issue was fixed. The part I couldn't do was downgrade the Kotlin stdlib version for OP.

On Thu, Feb 29, 2024, 3:01 a.m. Matthias Schenk @.***> wrote:

I just checked the updateItem - function and if I use the test provided above I still don't get an exception in case I try to update an item that does not exist. The scan - function works fine and throws an exception in case of a defective item.

Both are tested with the DynamoDbEnhancedAsyncClient version using the newly published 0.8.0 version of dynamodb-kotlin-module.

Can you please verify this?

— Reply to this email directly, view it on GitHub https://github.com/oharaandrew314/dynamodb-kotlin-module/issues/9#issuecomment-1970604480, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAG2JSBRWWORO6KHQTNMIR3YV3P7LAVCNFSM6AAAAAA3RGQM62VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNZQGYYDINBYGA . You are receiving this because you modified the open/close state.Message ID: @.***>

PoisonedYouth commented 8 months ago

That means the behavior of the updateItem - function (not throwing an exception when calling with a non-existent item) is the expected one?

oharaandrew314 commented 8 months ago

I think I misunderstood your observation. I'll look into it.

oharaandrew314 commented 8 months ago

@PoisonedYouth I just looked into this again. Based on the docs, if the item doesn't exist, then updateItem adds it. So the behavior I'm observing in this test seems correct.

PoisonedYouth commented 8 months ago

You are right. It seams that it works as expected.