-
Notifications
You must be signed in to change notification settings - Fork 18
Target AWSSDK.DynamoDBv2 v4 #86
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
Conversation
|
@bartelink thanks for this - I was planning v4 support for 1.0 but would you prefer to get this out asap? |
|
Well, I think it's easiest to talk about the other features and/or issues once we're working off a better baseline in V4 assuming V3 is unlikely to have any feature work beyond autogenned coverage of added endpoints? I don't personally have a pressing need for this, but assuming we do get some kind of a release out soon, I'll pretty quickly integration test under AFAICT the V4 key breaking changes result in pretty easy to find and fix NREs (see th bulk of the diffs) There's one remaining issue on the branch that I'll probably park now given the time this side |
2fe7108 to
5172f6f
Compare
|
Substantially revised the approach after getting the tests to pass (aka realized I'd gotten it all wrong!) Applies Rider 2025.3 fixes/suggestions to all affected files (can separate out a PR for that if you wish...) Not certain about how I'd package this in terms of release labelling, but I entitleed it Added minor notes regarding potential implications of taking this V4 dependency on in-flight PRs: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR updates the AWSSDK.DynamoDBv2 dependency from version 3.7 to version 4.0, implementing necessary code changes to accommodate breaking changes in the AWS SDK v4, particularly around handling of empty collections and nullable properties.
Key Changes
- Updates AWSSDK.DynamoDBv2 to v4.0.10.4 and AWSSDK.Core to v4.0.3.6, bringing in additional transitive dependencies
- Adapts code to handle v4 breaking changes where empty Lists and Dictionaries must be replaced with
nullrather than empty collections - Updates AttributeValue property access patterns to use nullable/HasValue checks for properties like NULL and BOOL that changed from boolean to nullable types
Reviewed changes
Copilot reviewed 16 out of 17 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| paket.dependencies | Updates AWSSDK.DynamoDBv2 constraint from ~> 3.7.5 to ~> 4 |
| paket.lock | Reflects updated dependencies including AWSSDK v4 packages and new transitive dependencies |
| src/FSharp.AWS.DynamoDB/Utils/DynamoUtils.fs | Updates AttributeValue property checks to use nullable patterns (NULL.HasValue, BOOL.HasValue) and adds IsNULL helper; replaces direct array indexing syntax |
| src/FSharp.AWS.DynamoDB/TableContext.fs | Initializes request objects with non-null collections, adds null checks before iterating collections, and updates AttributeWriter usage patterns |
| src/FSharp.AWS.DynamoDB/RecordKeySchema.fs | Adds null checks before iterating GlobalSecondaryIndexes and LocalSecondaryIndexes, initializes collections explicitly |
| src/FSharp.AWS.DynamoDB/Picklers/*.fs | Updates to use IsNULL helper and ensures empty collections return None (null) in v4 |
| src/FSharp.AWS.DynamoDB/Expression/*.fs | Fixes "WriteAttibute" typo to "WriteAttribute", updates AttributeWriter to return null for empty dictionaries |
| tests/FSharp.AWS.DynamoDB.Tests/*.fs | Updates test utilities to handle v4 nullable properties using GetValueOrDefault |
| FSharp.AWS.DynamoDB.sln.DotSettings | Adds Rider dictionary entries for custom terms used in the codebase |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Excuse: Rider renders things confusingly!
samritchie
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work thanks @bartelink - I appreciate your time.
Happy with -beta label; projects will all currently be configured for prerelease so -alpha wouldn’t add much. This is the main reason I’m keen to get to 1.0.
I’ll do a private nuget now & dogfood over the next day or two, then push the release.
|
@bartelink no luck with dogfooding, looks like I have a major dependency on v3 with a v4 release due 'real soon now'. Irony so thick you could cut it. I can push the nuget release if you’re able to verify with Equinox? I have to say v4 is one of the most developer hostile API changes I’ve seen - I hope it’s less painful from C# at least... |
|
Yes, some interesting decisions really - hard to see the point of going to such pains to reduce allocations in the context of something that's a) I/O bound and b) probably still allocating massively in the real world path anyway Yes, If you push the nuget I'll do an update targeting it. A local test shows 2x It's possible that the Lambda/streaming integration might throw up other things and I don't have a 5 minute way of spiking that. In terms of your porting issues, are we talking runtime or compile time? If the latter, it might be worth splitting the work into two phases by trying running V3 using the suggestion from the docs of turning that new flag to V4 mode to shake out issues relating to the handling of |
|
@bartelink package is published now. Porting issue was a MissingMethodException I think - I can’t find the logs now. I did try the I’m also seeing odd behaviour with the client interfaces ( |
Updates the AWSSDK.DynamoDBv2 dependency to V4
Handles the key breaking change in v4 of the DynamoDB SDK: empty Lists and Dictionaries are not permitted in requests, and must be replaced with
nullresolves: #84