Upgrade dynamo sync

[lindseydew]

What does this change?

This PR does the job of upgrading the scanamo and dynamo library ahead of a larger upgrade to Java11.

Changes:

• Amazon client from Async to Sync.

This was to maintain the type signatures as the AsyncScanamo returns a Future, and this would introduce much larger code changes. I don't think there should be operational reasons that we would need an async client in this case. It would be good to confirm this point.

• Removes the write method which calls dyanmo's put method, in favour of calling update for both create and update requests.

This is because put item https://docs.aws.amazon.com/amazondynamodb/latest/APIReference/API_PutItem.html does not return the latest version of data in the data store. However update item https://docs.aws.amazon.com/amazondynamodb/latest/APIReference/API_UpdateItem.html does.
The code is using the data returned from the database in the response back to the client.

How to test

Using the iOS Simulator

• Able to display saved articles
• Able to remove all articles
• Able to add a new article

Using the lambda console

• Ability to delete a user

[rtyley]

Absolutely beautiful, I love it!

A few small nits to fix up but overall it looks great- wonderful work!

Will get to reviewing #107 as soon as I can (probably tomorrow afternoon), but in the mean time rolling out this change and letting it bed in is probably a good idea.

[rtyley]

This is can probably be cleaned up a bit, because we've still got awsDynamo pointing to the dynamodb client for AWS SDK v1, and awsJavaSdk pointing to the client for AWS SDK v2 - do we definitely want both versions of the dynamodb client?

Probably we can get rid of the AWS SDK v1 client ("com.amazonaws" % "aws-java-sdk-dynamodb") and rename the AWS SDK v2 client ("software.amazon.awssdk" % "dynamodb") to awsDynamo?

[lindseydew]

Oh thank you that's a good spot.

[rtyley]

Removes the write method which calls dyanmo's put method, in favour of calling update for both create and update requests.
This is because put item https://docs.aws.amazon.com/amazondynamodb/latest/APIReference/API_PutItem.html does not return the latest version of data in the data store. However update item https://docs.aws.amazon.com/amazondynamodb/latest/APIReference/API_UpdateItem.html does.
The code is using the data returned from the database in the response back to the client.

Oh, yes, DynamoDB's PutItem at most lets you have the old version of the data - and that wasn't originally obvious from the signature of Scanamo's table.put() method (see scanamo/scanamo#464) and so scanamo/scanamo#486 (released with v1.0.0-M12 in 2020) made table.put() return Unit - and also added table.putAndReturn() to Scanamo's API if you really do want to ask for the old value.

This means that the old write() code you're deleting here was returning the old state of the DynamoSavedArticles. As it happens, the write() method was only called in one specific case:

mobile-save-for-later/mobile-save-for-later/src/main/scala/com/gu/sfl/lib/SavedArticlesMerger.scala

Lines 51 to 52 in fe89602

	logger.info(s"UserId: $userId. Storing articles for the first time. Version: ${deduplicatedArticles.version}. Client count: ${deduplicatedArticles.articles.length}")
	persistMergedArticles(userId, deduplicatedArticles)(savedArticlesPersistence.write)

...when articles were being saved for a user for the first time.

This makes it look like, when a user saved an article for the first time, the response returned by POSTing to the /syncedPrefs/me/savedArticles endpoint would actually list no articles - so the Live App might think it had no saved articles at all, at least until it did a fresh fetch of the endpoint again.

The code that uses this endpoint is here:

• 🤖 https://github.com/guardian/android-news-app/blob/608b3d257367e65580b248cfac1b2ea69037833b/saved-for-later/src/main/java/com/guardian/feature/sfl/syncing/SavedListSyncer.kt#L141-L153 - seems to always throw away the response of returned by updateSavedArticles(), so would actually not have been affected by this bug.
• 🍎 https://github.com/guardian/ios-live/blob/933086cdc7bf91abe973cb19eabb927b1b94795e/GLA/Frameworks/SavedForLater/Sources/SavedForLater/SavedArticlesLocalSyncManager.swift#L111-L148 - seems to take the returned response as the true state of play, so might have had a problem.

Abandoning the write() method, and just using the update() method everywhere instead, to call DynamoDB's UpdateItem for a create-or-update semantic that also returns the updated value, seems like a really good choice! 👍

[lindseydew]

Great thank you for confirming that

[lindseydew]

Absolutely beautiful, I love it!

A few small nits to fix up but overall it looks great- wonderful work!

Will get to reviewing #107 as soon as I can (probably tomorrow afternoon), but in the mean time rolling out this change and letting it bed in is probably a good idea.

Great thank you for the thorough review, and especially looking into how the clients of this service work. Much appreciated.