Skip to content
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

fix: Delay the cast from IAmazonDynamoDB to AmazonDynamoDBClient in the DocumentModel #3388

Merged
merged 4 commits into from
Aug 8, 2024

Conversation

ashovlin
Copy link
Member

@ashovlin ashovlin commented Jul 16, 2024

Description

Before: When initializing the document model Table object with an IAmazonDynamoDB, .NET/Standard/Core, we cast it right away to an AmazonDynamoDBClient.

  • Because we didn't have an synchronous HTTP client, we removed the public sync apis from the low-level service client.
  • However we never removed the synchronous public Table APIs, which still rely on the internal sync low-level APIs internally.
  • So to work around this, we kept the sync low-level APIs as internal on AmazonDynamoDBClient. This is why we need the cast, since the sync low-level APIs are no longer part of the public IAmazonDynamoDB.

Now: We delay the cast until right before we need to access the internal, low-level sync APIs.

  • For users still calling the sync APIs in .NET/Standard/Core, you can avoid this by moving to the async equivalents where possible.
  • If you're still relying on the sync APIs but aren't providing an actual AmazonDynamoDBClient, you'll now see a clearer InvalidOperationException instead of the NullReferenceException from the failed cast.

Future:

  1. We can deprecate the Table sync-over-async APIs entirely. This would remove the need for this cast.
  2. Since .NET 5 added back a sync HTTP client, we've discussed internally re-exposing sync low-level APIs where possible.

Motivation and Context

DOTNET-6755 and #1589 (most of the way, until we do on of the Future ideas from above)

This allows Table consumers to mock the IAmazonDynamoDB from .NET/Standard/Core as long as you stick to async public APIs.

Testing

 dotnet test .\sdk\src\Services\DynamoDBv2\DynamoDBv2.sln
dotnet test .\sdk\AWSSDK.CoreAndCustomUnitTests.NetStandard.sln

PENDING - .NET Standard integ tests for DDB

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My code follows the code style of this project
  • My change requires a change to the documentation
  • I have updated the documentation accordingly
  • I have read the README document
  • I have added tests to cover my changes
  • All new and existing tests passed

License

  • I confirm that this pull request can be released under the Apache 2 license

Copy link
Member

@normj normj left a comment

Choose a reason for hiding this comment

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

A couple minor nits but change looks good.

var serviceResponse = client.BatchGetItem(request);
#if AWS_ASYNC_API
// Cast the IAmazonDynamoDB to the concrete client instead, so we can access the internal sync-over-async methods
var internalClient = client as AmazonDynamoDBClient;
Copy link
Member

Choose a reason for hiding this comment

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

nit: You can move the casting logic outside of the loop and only do it once.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in aa1e327

{
do
{
var result = client.BatchWriteItem(request);
#if AWS_ASYNC_API
// Cast the IAmazonDynamoDB to the concrete client instead, so we can access the internal sync-over-async methods
Copy link
Member

Choose a reason for hiding this comment

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

nit: Move the casting logic above the loop.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in aa1e327

@ashovlin
Copy link
Member Author

ashovlin commented Aug 2, 2024

Realized that I was using too wide of a condition AWS_ASYNC_API, which was casting unnecessarily for .NET Framework 4.7.2 even though it still has sync APIs we can use. Switched to NETSTANDARD in aa1e327

This also made some .NET Framework tests invalid, so I removed them.

…mazon.DynamoDBv2.DocumentModel to only when it's necessary for sync-over-async code paths. This allows mocking for async callers.
@ashovlin
Copy link
Member Author

ashovlin commented Aug 8, 2024

Dry-run e8acaff2-7604-4ade-a913-8ea4cc934cfe passed after adding 2f8a80e

@ashovlin ashovlin merged commit 6a79434 into v4-development Aug 8, 2024
1 check passed
@ashovlin ashovlin deleted the shovlia/v4-ddb-1589 branch August 8, 2024 14:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants