Skip to content

Adds various null checks required for AWS SDK v4 #88

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.

Sign up for GitHub

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

Open
njlr wants to merge 20 commits into
base: master
Choose a base branch
from
Open

Adds various null checks required for AWS SDK v4 #88

Changes from 7 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
5e055fe
BatchPutItemsAsync checks for nulls
njlr Mar 18, 2026
2b99e3d
tryGetItemAsync checks for null
njlr Mar 18, 2026
eeacc63
batchGetItemsAsync checks for null
njlr Mar 18, 2026
9608c99
queryPaginatedAsync checks for null
njlr Mar 18, 2026
1cf08fa
scanPaginatedAsync checks for null
njlr Mar 18, 2026
8751759
BatchDeleteItemsAsync checks for null
njlr Mar 18, 2026
3f76966
TransactWriteItems checks for null
njlr Mar 18, 2026
a042d1e
Merge branch 'master' of https://github.com/fsprojects/FSharp.AWS.Dyn…
njlr Mar 20, 2026
956abd7
Explicit `Some`
njlr Mar 20, 2026
fb2a1e3
Array comprehension
njlr Mar 20, 2026
fb4d91a
Refactor
njlr Mar 20, 2026
80eb473
Refactor
njlr Mar 20, 2026
2fcbfb5
Refactor
njlr Mar 20, 2026
e11260f
Array comprehension
njlr Mar 20, 2026
6f0c6ac
Refactor
njlr Mar 20, 2026
cd78f70
Format
njlr Mar 20, 2026
3c5ee5d
Refactor
njlr Mar 20, 2026
c78f8ee
Removes usages of isNull
njlr Mar 20, 2026
94e692f
refactor: remove isNull/isNotNull helpers
bartelink Mar 21, 2026
e0e4789
Merge pull request #1 from bartelink/bugfix/check-nulls-aws-sdk-v4
njlr Mar 21, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 36 additions & 23 deletions src/FSharp.AWS.DynamoDB/TableContext.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -280,7 +280,7 @@ type TableContext<'TRecord>
failwithf "GetItem request returned error %O" response.HttpStatusCode

if response.IsItemSet then
return Some response.Item
return Option.ofObj response.Item
Copy link
Member

@bartelink bartelink Mar 19, 2026

Choose a reason for hiding this comment

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

I know you're doing it for consistency but the guard guarantees it won't be null (in fact the guard will return Null if the Item.Count = 0)

Copy link
Contributor Author

@njlr njlr Mar 19, 2026

Choose a reason for hiding this comment

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

Yeah, I had to return an option either way so figured why not.
I guess Some response.Item might be marginally more efficient since it is one fewer null check?

Copy link
Member

@bartelink bartelink Mar 19, 2026

Choose a reason for hiding this comment

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

TL;DR Its more about not putting 'just in case' code that's not justified as that makes the reader have to think more (and/or if you were trying to autogen coverage of all paths, you'd be trying to generate a null with the guard returning true, which would be impossible)

It happens to be more efficient, but its more about keeping the code straightforward and not putting diffs in unless there is an actual correctness improvement - the IsItemSet absolutely guarantees a non-null value, and the general pattern should be to think about guarding all inspections of properties and then either do the work conditionally, or substitute an empty if the arrays are being processed in some way.

Copy link
Contributor Author

@njlr njlr Mar 20, 2026

Choose a reason for hiding this comment

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

Makes sense; updated

else
return None
}
Expand Down Expand Up @@ -311,11 +311,11 @@ type TableContext<'TRecord>
let! ct = Async.CancellationToken
let! response = client.BatchGetItemAsync(request, ct) |> Async.AwaitTaskCorrect
maybeReport
|> Option.iter (fun r -> r BatchGetItems (List.ofSeq response.ConsumedCapacity) response.Responses[tableName].Count)
|> Option.iter (fun r -> r BatchGetItems (if isNull response.ConsumedCapacity then [] else List.ofSeq response.ConsumedCapacity) (if isNull response.Responses then 0 else response.Responses[tableName].Count))
if response.HttpStatusCode <> HttpStatusCode.OK then
failwithf "BatchGetItem request returned error %O" response.HttpStatusCode

return response.Responses[tableName]
return if isNull response.Responses then ResizeArray() else response.Responses[tableName]
}

let queryPaginatedAsync
Expand Down Expand Up @@ -374,7 +374,9 @@ type TableContext<'TRecord>
emitMetrics ()
failwithf "Query request returned error %O" response.HttpStatusCode

downloaded.AddRange response.Items
if notNull response.Items then
downloaded.AddRange response.Items

if response.LastEvaluatedKey <> null && response.LastEvaluatedKey.Count > 0 then
lastEvaluatedKey <- Some response.LastEvaluatedKey
if limit.IsDownloadIncomplete downloaded.Count then
Expand Down Expand Up @@ -456,7 +458,9 @@ type TableContext<'TRecord>
emitMetrics ()
failwithf "Scan request returned error %O" response.HttpStatusCode

downloaded.AddRange response.Items
if notNull response.Items then
downloaded.AddRange response.Items

consumedCapacity.Add response.ConsumedCapacity
if response.LastEvaluatedKey <> null && response.LastEvaluatedKey.Count > 0 then
lastEvaluatedKey <- Some response.LastEvaluatedKey
Expand Down Expand Up @@ -578,15 +582,18 @@ type TableContext<'TRecord>
let! ct = Async.CancellationToken
let! response = client.BatchWriteItemAsync(pbr, ct) |> Async.AwaitTaskCorrect
let unprocessed =
match response.UnprocessedItems.TryGetValue tableName with
| true, reqs ->
reqs
|> Seq.choose (fun r -> r.PutRequest |> Option.ofObj)
|> Seq.map _.Item
|> Seq.toArray
| false, _ -> [||]
if isNull response.UnprocessedItems then
Copy link
Member

@bartelink bartelink Mar 19, 2026

Choose a reason for hiding this comment

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

perhaps clearer if we switch to array comprehensions:

        let unprocessed =
            [| if response.UnprocessedItems <> null then
                   match response.UnprocessedItems.TryGetValue tableName with
                   | true, reqs ->
                       for r in reqs do
                           if r.PutRequest <> null then
                               yield r.PutRequest.Item
                   | false, _ -> () |]

NOTE in the above I did not use isNull / isNotNull
These should normally only be used when you're checking a reference to an F# type that is marked as not allowing null (the default) (to circumvent the compiler error that would arise if you did)

Copy link
Contributor Author

@njlr njlr Mar 20, 2026

Choose a reason for hiding this comment

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

I like the comprehensions too; updated

[||]
else
match response.UnprocessedItems.TryGetValue tableName with
| true, reqs ->
reqs
|> Seq.choose (fun r -> r.PutRequest |> Option.ofObj)
|> Seq.map _.Item
|> Seq.toArray
| false, _ -> [||]
maybeReport
|> Option.iter (fun r -> r BatchWriteItems (Seq.toList response.ConsumedCapacity) (items.Length - unprocessed.Length))
|> Option.iter (fun r -> r BatchWriteItems (if isNull response.ConsumedCapacity then [] else Seq.toList response.ConsumedCapacity) (items.Length - unprocessed.Length))
Copy link
Member

@bartelink bartelink Mar 19, 2026

Choose a reason for hiding this comment

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

these get pretty illegible and reformatting might split the lines.

        |> Option.iter (fun r ->
            let cc = if response.ConsumedCapacity = null then [] else Seq.toList response.ConsumedCapacity
            r BatchWriteItems cc (items.Length - unprocessed.Length))

Copy link
Contributor Author

@njlr njlr Mar 19, 2026

Choose a reason for hiding this comment

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

I will wait till you have made all comments then fix these in one go.
Can also apply Fantomas if it doesn't blow up the diff?

Copy link
Collaborator

@samritchie samritchie Mar 19, 2026

Choose a reason for hiding this comment

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

@njlr we did a big reformat with fantomas last year so it should only update the changes

Copy link
Contributor Author

@njlr njlr Mar 20, 2026

Choose a reason for hiding this comment

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

these get pretty illegible and reformatting might split the lines.

Much nicer; updated

@njlr we did a big reformat with fantomas last year so it should only update the changes

Great, I have applied Fantomas

if response.HttpStatusCode <> HttpStatusCode.OK then
failwithf "BatchWriteItem put request returned error %O" response.HttpStatusCode

Expand Down Expand Up @@ -841,15 +848,18 @@ type TableContext<'TRecord>
let! ct = Async.CancellationToken
let! response = client.BatchWriteItemAsync(request, ct) |> Async.AwaitTaskCorrect
let unprocessed =
match response.UnprocessedItems.TryGetValue tableName with
| true, reqs ->
reqs
|> Seq.choose (fun r -> r.DeleteRequest |> Option.ofObj)
|> Seq.map _.Key
|> Seq.toArray
| false, _ -> [||]
if isNull response.UnprocessedItems then
[||]
else
match response.UnprocessedItems.TryGetValue tableName with
| true, reqs ->
reqs
|> Seq.choose (fun r -> r.DeleteRequest |> Option.ofObj)
|> Seq.map _.Key
|> Seq.toArray
| false, _ -> [||]
maybeReport
|> Option.iter (fun r -> r BatchWriteItems (Seq.toList response.ConsumedCapacity) (keys.Length - unprocessed.Length))
|> Option.iter (fun r -> r BatchWriteItems (if isNull response.ConsumedCapacity then [] else Seq.toList response.ConsumedCapacity) (keys.Length - unprocessed.Length))
if response.HttpStatusCode <> HttpStatusCode.OK then
failwithf "BatchWriteItem deletion request returned error %O" response.HttpStatusCode

Expand Down Expand Up @@ -1412,7 +1422,7 @@ and Transaction(client: IAmazonDynamoDB, ?metricsCollector: RequestMetrics -> un
let writer = AttributeWriter()
req.UpdateExpression <- updater.UpdateOps.Write writer
precondition |> Option.iter (fun cond -> req.ConditionExpression <- cond.Conditional.Write writer)

req.ExpressionAttributeNames <- writer.Names
req.ExpressionAttributeValues <- writer.Values
transactionItems.Add(TransactWriteItem(Update = req))
Expand Down Expand Up @@ -1455,7 +1465,10 @@ and Transaction(client: IAmazonDynamoDB, ?metricsCollector: RequestMetrics -> un
let! response = client.TransactWriteItemsAsync(req, ct) |> Async.AwaitTaskCorrect
maybeReport
|> Option.iter (fun r ->
response.ConsumedCapacity
(if isNull response.ConsumedCapacity then
Copy link
Member

@bartelink bartelink Mar 19, 2026

Choose a reason for hiding this comment

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

            let cc = if response.ConsumedCapacity = null then [] else Seq.toList response.ConsumedCapacity
            for tableName, consumedCapacity in cc |> Seq.groupBy _.TableName do
                r tableName Operation.TransactWriteItems (Seq.toList consumedCapacity) (Seq.length transactionItems))

ResizeArray()
else
response.ConsumedCapacity)
|> Seq.groupBy _.TableName
|> Seq.iter (fun (tableName, consumedCapacity) ->
r tableName Operation.TransactWriteItems (Seq.toList consumedCapacity) (Seq.length transactionItems)))
Expand Down