Skip to content

Conversation

@pettyalex
Copy link

@pettyalex pettyalex commented Aug 21, 2020

My team got bit by #47, and I started fixing it myself before realizing that develop already has a fix, it's just not deployed to maven central yet.

I'd like to get a better lay of the land and add unit tests for this case too, but this jumped out at me as an initial simplification. In case it wasn't clear from the original bug report, the root issue was that when a table name prefix was configured in the DynamoDBMapperConfig through tableNameOverride, that prefix (or any table name overriding) was not being applied for queries that used secondary indices. The table name override was working for queries by the main hash index, or scans.

This is Kotlin code, but here's a slightly modified example of my config that was causing the issue:

@Configuration
@EnableDynamoDBRepositories("contoso.app.repositories.dynamodb")
class DynamoDBConfig {

    // Prefix tables with the environment that we're in
    @Value("\${contoso.app.dynamodb.envprefix}")
    lateinit var envPrefix: String

    @Bean
    @Primary // This must take precedence over default Bean https://github.com/derjust/spring-data-dynamodb/issues/233
    fun dynamoDBMapperConfig(): DynamoDBMapperConfig {
        val builder = DynamoDBMapperConfig.Builder()
        builder.tableNameOverride = DynamoDBMapperConfig.TableNameOverride.withTableNamePrefix(envPrefix)
        return builder.build()
    }

    @Bean
    fun amazonDynamoDB(): AmazonDynamoDB {
        return AmazonDynamoDBClientBuilder.standard()
            .withRegion(Regions.US_EAST_1).build()
    }
}

We, and other users, are using table name prefixing to handle passing environment-specific prefixes in, so we have dev-entity, stg-entity, so on for a table that holds entities.

  • Combine the two copy-pasted constructors into one. Just use a null check and a default instead.

@boostchicken boostchicken merged commit dbde40b into boostchicken:develop Aug 27, 2020
@boostchicken
Copy link
Owner

I will be cutting a release this coming weekend probably. Out of town right now. Merged into develop though!

@ahardewig
Copy link

@boostchicken Are there any plans to make a new release with this fix? I ran into this issue yesterday too

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