pjfanning commented on code in PR #349:
URL:
https://github.com/apache/pekko-persistence-dynamodb/pull/349#discussion_r3747928980
##########
src/main/scala/org/apache/pekko/persistence/dynamodb/journal/DynamoDBRecovery.scala:
##########
@@ -270,7 +270,7 @@ trait DynamoDBRecovery extends AsyncReplayMessages {
def dynamoSummingPager(queryReq: QueryRequest, acc: Seq[Item]):
Future[Seq[Item]] = {
dynamo.query(queryReq).flatMap { result =>
val currentPageItems = result.items.asScala.toSeq
- if (!result.hasLastEvaluatedKey || result.lastEvaluatedKey.isEmpty)
+ if (!result.hasLastEvaluatedKey)
Review Comment:
Claude AI suggested this but I'm willing to review it.
Claude Reasoning:
In SDK v2, QueryResponse.lastEvaluatedKey() returns null when there are no
more pages — it never returns an empty map. The hasLastEvaluatedKey() method is
a null-safe check: it returns true if and only if lastEvaluatedKey is non-null.
So the original code:
if (!result.hasLastEvaluatedKey || result.lastEvaluatedKey.isEmpty)
has a redundant second clause. When hasLastEvaluatedKey is false, the ||
short-circuits and lastEvaluatedKey.isEmpty is never evaluated. When
hasLastEvaluatedKey is true, lastEvaluatedKey is guaranteed non-null and
non-empty (DynamoDB always returns at least one key entry), so .isEmpty is
always false.
The || short-circuit also prevents a potential NPE — if hasLastEvaluatedKey
were false and the second clause were evaluated, calling .isEmpty on null would
throw.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
