Remove v1 AWS dynamodb client and awscala dynamo lib #500
Conversation
c73141d to
b1aec53
Compare
There was a problem hiding this comment.
Great work! Nice that we have the existing tests for the serialisation. It'd be great to populate a database using the old source code, and then switch onto this branch and check that the existing data is still usable. Most likely you've already done this!
I do have one comment on the fiddly insert function, but overall this looks like a great change, thank you.
| val keySchema = table.keySchema().asScala | ||
| val partitionKeyName = | ||
| keySchema.find(_.keyType() == "HASH").get.attributeName() | ||
| val sortKeyName = keySchema.find(_.keyType() == "RANGE").get.attributeName() | ||
| val (hash_project, range_date, attrs) = auditLogAttrs(auditLog) |
There was a problem hiding this comment.
This section is a mix of hard-coded and "derived from reality". Take thepartitionKeyName for example. We ask the database what this is, but for the actual value we hard code it to the account field of the auditLog record in this auditLogAttrs call. As well as adding a lot of visual noise and incidental complexity to the insert function, it means our assumptions about which fields are used for what purpose are placed both here and in the database structure itself instead of being centralised in the source code.
I'd be tempted to ask whether we can instead change the auditLogAttrs function so that it returns more information. We can move some of this logic into that single source of truth for the field names / purposes.
As a suggestion to expand on a possible implementation (but see what you think is best!), could auditLogAttrs return a case class containing all the information about the table structure instead of just the values we previously needed in a tuple returning ((String, Long, List[(String, Any)])).
Here's an example of what I mean, but anything that takes your fancy!
case class AuditLogDbAttrs(
// the DB partition key
accountAttrName: String,
account: String,
// the DB range key
accessTimeName: String.
accessTime: Long,
// all the other database fields
attrs: List[(String, Any)]
)With this datastructure coming back from auditLogsAttrs (which does have all this information), does this insert function get much simpler?
There are a few tests that would need updating, but I'm encouraged to see that these would arguably also be a bit clearer with the above structure in place.
See what you think?
There was a problem hiding this comment.
I agree that there are various places where the code is generic and also very specific. I should have put it in the PR description but this was the minimal like-for-like changes to move the code from awscala to the v2 client. Once this in and working I'll take your suggestions in a separate refactoring PR.
| @@ -30,7 +30,7 @@ class AppComponents(context: ApplicationLoader.Context) | |||
| val requiredGoogleGroups = Set(Config.twoFAGroup(configuration)) | |||
| val dynamodDB = | |||
| if (context.environment.mode == play.api.Mode.Prod) | |||
| DynamoDB.at(Regions.getCurrentRegion) | |||
| DynamoDbClient.builder().region(EU_WEST_1).build() | |||
There was a problem hiding this comment.
I think it's fine to hard-code this 👍
|
Searching and updating the Prod audit page is still working. |
What is the purpose of this change?
To remove the deprecated AWS v1 Dynamo DB client and replace it with v2.
This is very difficult to do without also removing the awscala Dynamo DB library, which depends on the AWS v1 client.
What is the value of this change and how do we measure success?
Code is up to date and easier to patch in future.
Should still be able to: