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

Add nongeneric batchwrite to dynamodbcontext #2506

Conversation

ericjpeters
Copy link

As an expansion to #2503, this provide non-generic put's in batch.

Description

Allows for a batch put without needing a strongly typed T.

Motivation and Context

See #2503 for more detail, but makes for easy integration with EF's for batch deleting where the items are archived into DynamoDB.

Testing

Sorry, I still can't seem to get the tests to build. I have tested locally by integrating into an EF application.

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

@ashishdhingra ashishdhingra added pr/needs-review This PR needs a review from a Member. dynamodb labels Jan 27, 2023
@muhammad-othman muhammad-othman self-requested a review June 8, 2023 17:13
@peterrsongg peterrsongg self-requested a review June 9, 2023 15:55
@@ -200,6 +200,23 @@ internal BatchWrite(DynamoDBContext context, DynamoDBFlatConfig config)
DocumentBatch = table.CreateBatchWrite();
}

internal BatchWrite(DynamoDBContext context, Type valuesType, DynamoDBFlatConfig config)
: base(context, config)
{
Copy link
Member

Choose a reason for hiding this comment

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

Since the other constructor contains almost the same code, can you update it to call this one? maybe something like this

        internal BatchWrite(DynamoDBContext context, DynamoDBFlatConfig config)
            : this(context, typeof(T), config)
        {
        }

@@ -226,6 +226,30 @@ public BatchWrite<T> CreateBatchWrite<T>(DynamoDBOperationConfig operationConfig
return new BatchWrite<T>(this, config);
}

/// <summary>
Copy link
Member

Choose a reason for hiding this comment

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

Can you update the comments on BatchWrite<object> methods to include details about the object typeparam and how it is different from the generic version.

@@ -162,6 +162,15 @@ public partial interface IDynamoDBContext : IDisposable
/// <param name="operationConfig">Config object which can be used to override that table used.</param>
/// <returns>Empty strongly-typed BatchWrite object</returns>
BatchWrite<T> CreateBatchWrite<T>(DynamoDBOperationConfig operationConfig = null);

Copy link
Member

Choose a reason for hiding this comment

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

Can you add this method to BCL35 too, similar to the methods at lines 146 & 155

@ericjpeters
Copy link
Author

@muhammad-othman -- I believe I have addressed your PR comments. Let me know if there is any additional cleanup needed.

Thanks!

@muhammad-othman
Copy link
Member

@ericjpeters Changes look good. I'll send the PR through our full dry run build and assuming that doesn't have any issues I'll approve it.

@muhammad-othman
Copy link
Member

PR is queued up to go out with next non-service update release. Those types of releases usually happen about once every other week.

@normj
Copy link
Member

normj commented Jun 21, 2023

The PR was rebased, merged and released as part of version 3.7.104 of the DynamoDB package. Thanks for the PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dynamodb pr/needs-review This PR needs a review from a Member.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants