-
Notifications
You must be signed in to change notification settings - Fork 141
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
Fix in case DynamoDBMapper is already provided #105
Conversation
Fix in case dynamoDbMapper is already provided,w e dont need to check for dynamodDbMapperCofig or the assert
Looking good. But as a burned kind def. want to add a unit test before merging. Will do that in the next days |
Thanks for the PR! |
@derjust ya np .. was in a hurry couldn't add the test case using this custom implementation right now for my existing project .. will be great if you could merge the fix once approved in the 4.5.x branch as well . |
Merged in branch |
HI @derjust one more thing I guess needs to be changed is getOverriddenTableName whcih will give NPE if dynamoDbconfig is not provided . Can we make the method below Public rather than introducing the new method we have for the mapper as config could be required getOverriddenTableName |
absolutely doable. Any chance to get access to your (maybe simplified) complete scenario to add it as a knot test?
|
Hi in my case I am adding a DynamoDbTemplate with a configured encryption transformer as below .. Currently using a custom DynamodBTemplate class . return new DynamoDBTemplate(amazonDynamoDB, this.dynamoDBMapperConfig(), new DynamoDBMapper(amazonDynamoDB,
this.dynamoDBMapperConfig(), new AttributeEncryptor(new DirectKmsMaterialProvider(awsKMS, kmsKey)))); |
Created #108 to go on with the discussion there/have an issue ID to relate to |
Fix in case dynamoDbMapper is already provided,w e dont need to check for dynamodDbMapperCofig or the assert
references issue #91