DynamoDB `Table` misrepresents dependency on `IAmazonDynamoDB`

[rlyczynski]

I think the Table class misrepresents it's dependency on IAmazonDynamoDB when it really depends on an AmazonDynamoDBClient. The constructor takes an IAmazonDynamoDB but then attempts to cast it to a AmazonDynamoDBClient just a few lines later.

https://github.com/aws/aws-sdk-net/blob/41ca5761addd6f1d3dbfcc2752d8569cf1ef93f0/sdk/src/Services/DynamoDBv2/Custom/DocumentModel/Table.cs#L369-L383

This hinders the ability to unit test code that relies on the Table class by passing in a mocked version of IAmazonDynamoDB and also makes it very difficult to do something like use a decorator instead of an AmazonDynamoDBClient specifically.

At very least I think the constructor should be clear that it depends on a AmazonDynamoDBClient, but ideally the Table class would really only depended on an IAmazonDynamoDB.

Steps to Reproduce

Use the Table class with an object that implements IAmazonDynamoDB but is not a AmazonDynamoDBClient.

Table.LoadTable((IAmazonDynamoDB)notAnAmazonDynamoDBClient, "tablename")

This will cause the cast to an AmazonDynamoDBClient to fail and set DDBClient to null and eventually result in a NullReferenceException.

[ashishdhingra]

Hi @rlyczynski,

Good morning.

If you refer the code at https://github.com/aws/aws-sdk-net/blob/41ca5761addd6f1d3dbfcc2752d8569cf1ef93f0/sdk/src/Services/DynamoDBv2/Custom/DocumentModel/Table.cs#L56, for platforms PCL || UNITY || NETSTANDARD, the following internal property is declared:

internal AmazonDynamoDBClient DDBClient { get; private set; }

The code at line https://github.com/aws/aws-sdk-net/blob/41ca5761addd6f1d3dbfcc2752d8569cf1ef93f0/sdk/src/Services/DynamoDBv2/Custom/DocumentModel/Table.cs#L378 in Table constructor simple casts to IAmazonDynamoDb to AmazonDynamoDBClient. I'm not sure the design consideration around this, but cast appears to be correct given that there is an internal property of that type.

Hope this provides some guidance.

Thanks, Ashish

[github-actions[bot]]

This issue has not recieved a response in 2 weeks. If you want to keep this issue open, please just leave a comment below and auto-close will be canceled.

[Kralizek]

@ashishdhingra

I think this issue is pointing out how accepting a parameter via an interface and then casting it it to a concrete type is a bad practice and malevolent communication. Especially if the constructor throws if the parameter is not of the concrete type.

It gives the false idea one could create a table injecting a fake client (like a mock) and be able to assert the behavior of the class.

Unfortunately, the DynamoDB library is not designed with testing from user side in mind, like this and other issues have highlighted over time.

[ashishdhingra]

@Kralizek Thanks for the input. I understand the concern and will have a developer look at it.

[ashishdhingra]

Hi @rlyczynski @Kralizek The object is being cast to a AmazonDynamoDBClient to handle NetStandard specific features. The implementation could be handled otherwise to support your request. With that being said, this request will be handled as a feature request to be taken in future sprints, depending on the developers' capacity.

[william-li-ry]

Just wondering is there any progress with this issue? I followed the issue from #1801 to here and seems this problem is still not resolved. So what's the current strategy to mock an IAmazonDynamoDB instance? Thanks.

[xero-timothy]

I'm not affiliated in any way with AWS. But I have to say @rlyczynski, your tone when reporting this issue is unnecessarily aggressive. How about being a good human in the future?

[rlyczynski]

I'm not affiliated in any way with AWS. But I have to say @rlyczynski, your tone when reporting this issue is unnecessarily aggressive. How about being a good human in the future?

Fair. Was funnier in my head at the time. Definitely come off as a dick as I reread this a year later. My b.

[ericadams]

@ashishdhingra this is affecting my team's ability to fulfill their test coverage obligations, and I believe it's been misclassified. It's a #bug and not a #feature-request, given that it's pretty clearly a violation of the public contract declared by IAmazonDynamoDB. After 18months open, do you have a timeline for closing this?

[ashishdhingra]

@ericadams Let me discuss this with the team and post any updates here.

[normj]

This is a left over artifact of the Document and DataModel high level abstractions being originally written for .NET Framework and the predominate API used where the sync APIs. These abstractions have some old corners in them that still rely on sync APIs even for .NET Core which the SDK public interface only supports async APIs.

We are trying to prioritize some rework/redesign in these abstractions to remove this old cruft and modernize them. When we do that top of the list is to make sure everything is async only and get rid of this casting.

[Kralizek]

Great to hear @normj

Would it be possible to add in the feature list for the revamp a convenient mocking experience?

Currently, whenever you go higher than the raw client you get stopped by unmockable types.

[sl-omideidivandi]

i had a look at repo and it looks me that there is not enough Unit tests in DynamodbV2 , the integration tests covers Query Async and etc but no Unit test that we can inspire by :(

[vukovinski]

Horrible

[ashovlin]

For background, when we moved to versions of .NET that only have an async HTTP client, we removed the sync APIs from the low-level IAmazonDynamoDB.

However we did not remove the sync APIs from the high-level DynamoDB clients, which still rely on the sync low-level APIs. That's why we cast to AmazonDynamoDBClient from the high-level code path, to still access the internal sync APIs that are on AmazonDynamoDBClient but not on IAmazonDynamoDB.

In PR #3388 in the V4 branch, we now delay the cast until right before we need to access the internal, low-level sync APIs.

• For users still calling the sync high-level APIs in .NET/Standard/Core, you can avoid this by moving to the async equivalents where possible, and then a mocked client should work.
• 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.

That's what we have planned currently for V4. In the long term we could explore removing the sync APIs from the high-level library, or else adding back a sync HTTP client support in .NET 5+.

[96malhar]

Hello, we have been working on the V4 of the AWS SDK for .NET. We have added the ability to mock various DynamoDB operations by adding interfaces in both the Data Model mode and Document Model mode in DynamoDB.

Refer to this PR - https://github.com/aws/aws-sdk-net/pull/3450

The IDynamoDBContext.GetTargetTable methods now return an ITable interface which is implemented the by the Table class. The interface can be mocked to simulate table operations in unit tests.

This feature is available in the AWSSDK.DynamoDBv2 Version 4.0.0-preview.2

[github-actions[bot]]

Comments on closed issues are hard for our team to see. If you need more assistance, please either tag a team member or open a new issue that references this one. If you wish to keep having a conversation with other community members under this issue feel free to do so.