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

Configure DynamoDb TableSchema as a bean #957

Merged
merged 13 commits into from
Nov 29, 2023
Merged

Configure DynamoDb TableSchema as a bean #957

merged 13 commits into from
Nov 29, 2023

Conversation

maciejwalkowiak
Copy link
Contributor

Fixes #674

@github-actions github-actions bot added component: dynamodb DynamoDB integration related issue type: documentation Documentation or Samples related issue labels Nov 15, 2023
Copy link
Contributor

@tomazfernandes tomazfernandes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not my turf, but left a few comments 🙂

private final List<TableSchema<?>> tableSchemas;
private final Map<String, TableSchema> tableSchemaCache = new ConcurrentHashMap<>();

DefaultDynamoDbTableResolver() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious - what is this constructor for, with package private visibility?

this.tableSchemas = tableSchemas;
}

public <T> TableSchema<T> resolveTableSchema(Class<T> clazz) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see this method was added to the interface, maybe add @Override?

public <T> TableSchema<T> resolveTableSchema(Class<T> clazz) {
String tableName = tableNameResolver.resolve(clazz);
return tableSchemaCache.computeIfAbsent(tableName,
entityClassName -> tableSchemas.stream().filter(it -> it.itemType().rawClass().equals(clazz))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're doing lookups by matching Class, maybe a better way to store this would be in a Map<Class, TableSchema>? We'd iterate it only once and have O(1) lookups afterwards.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, and it actually opened my mind for simpler design.

@maciejwalkowiak
Copy link
Contributor Author

thanks @tomazfernandes! now i think it is much better and no breaking changes. @MatejNedic can you take a look?

Copy link
Member

@MatejNedic MatejNedic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! Only 1 change and we can merge

@@ -115,17 +117,23 @@ public DynamoDbEnhancedClient dynamoDbEnhancedClient(DynamoDbClient dynamoDbClie
return DynamoDbEnhancedClient.builder().dynamoDbClient(dynamoDbClient).build();
}

@ConditionalOnMissingBean(DynamoDbTableNameResolver.class)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
@ConditionalOnMissingBean(DynamoDbTableNameResolver.class)
@ConditionalOnMissingBean(DynamoDbTableSchemaResolver.class)

Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 6 Code Smells

96.0% 96.0% Coverage
0.0% 0.0% Duplication

@maciejwalkowiak maciejwalkowiak merged commit bea0e01 into main Nov 29, 2023
6 checks passed
@maciejwalkowiak maciejwalkowiak deleted the GH-674-2 branch November 29, 2023 05:27
@maciejwalkowiak maciejwalkowiak added this to the 3.1.0 milestone Nov 29, 2023
maciejwalkowiak added a commit that referenced this pull request Dec 10, 2023
Fixes #674

Co-authored-by: Gladîș Vladlen <gladis.vladlen+github@gmail.com>
maciejwalkowiak added a commit that referenced this pull request Dec 10, 2023
Fixes #674

Co-authored-by: Gladîș Vladlen <gladis.vladlen+github@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: dynamodb DynamoDB integration related issue type: documentation Documentation or Samples related issue
Projects
None yet
4 participants