fix: Throw DynamoPaginationException on pagination errors#71
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughError-path handling in DynamoDB paginated iterators now throws a new DynamoPaginationException instead of terminating the async stream. A new exception class was added, and tests were introduced to validate multi-page enumeration for QueryAllAsync and ScanAllAsync using Changes
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Clients/Goa.Clients.Dynamo/DynamoExtensions.cs (1)
1-12:⚠️ Potential issue | 🔴 CriticalCritical: DynamoItemReader type is undefined and required methods are missing from IDynamoClient.
The code in DynamoExtensions.cs calls
client.QueryAsync(),client.ScanAsync(), andclient.BatchGetItemAsync()with aDynamoItemReader<T>parameter, but:
DynamoItemReader<T>is not defined anywhere in the codebaseIDynamoClientinterface has no overloads acceptingDynamoItemReader<T>QueryResult<T>andScanResult<T>generic types don't exist (only non-genericQueryResultandScanResultexist)You need to:
- Define the
DynamoItemReader<T>delegate (likelypublic delegate T DynamoItemReader<T>(ref System.Text.Json.Utf8JsonReader reader);)- Add generic overloads to
IDynamoClientfor typed operations- Create generic
QueryResult<T>andScanResult<T>types or update method signatures🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Clients/Goa.Clients.Dynamo/DynamoExtensions.cs` around lines 1 - 12, The extension methods use a missing generic reader and typed results; define a delegate public delegate T DynamoItemReader<T>(ref System.Text.Json.Utf8JsonReader reader) and then add generic overloads to IDynamoClient for QueryAsync<T>, ScanAsync<T>, and BatchGetItemAsync<T> that accept the DynamoItemReader<T> and return or populate strongly-typed result types; either introduce QueryResult<T> and ScanResult<T> (wrapping existing non-generic QueryResult/ScanResult with an Items: IReadOnlyList<T>) or change the extension signatures to map non-generic results into typed lists using the reader—update the calls in DynamoExtensions.QueryAsync, ScanAsync and Batch-related methods to use the new generic overloads and result types.
🧹 Nitpick comments (3)
tests/Clients/Goa.Clients.Dynamo.Tests/TypedExtensionTests.cs (2)
12-14: TestReader is a no-op stub - consider adding a clarifying comment.The
TestReaderdelegate ignores its input and returns an emptyTestModel. This works because the mock returns pre-builtQueryResult<TestModel>/ScanResult<TestModel>objects, bypassing the actual reader. A brief comment would clarify this is intentional for the mock setup.📝 Suggested clarification
- private static readonly DynamoItemReader<TestModel> TestReader = (ref System.Text.Json.Utf8JsonReader _) => new TestModel("", ""); + // Stub reader - actual deserialization is bypassed by mock returning pre-built results + private static readonly DynamoItemReader<TestModel> TestReader = (ref System.Text.Json.Utf8JsonReader _) => new TestModel("", "");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Clients/Goa.Clients.Dynamo.Tests/TypedExtensionTests.cs` around lines 12 - 14, The TestReader delegate currently ignores its Utf8JsonReader input and returns an empty TestModel; add a brief inline comment above the TestReader declaration clarifying this is an intentional no-op used because mocks supply pre-built QueryResult<TestModel> and ScanResult<TestModel> objects (so the reader is never exercised in these tests), to prevent confusion when reading TestReader and TestModel.
82-106: Good error handling test, but coverage could be expanded.This test validates that
DynamoPaginationExceptionis thrown on error. Consider adding similar tests for:
BatchGetAllAsyncandBatchGetAllAsync<T>(different retry logic with unprocessed keys)- Optionally, error cases for
ScanAllAsyncvariants to ensure consistencyWould you like me to generate test cases for
BatchGetAllAsyncerror handling scenarios?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Clients/Goa.Clients.Dynamo.Tests/TypedExtensionTests.cs` around lines 82 - 106, Add parallel tests that mirror QueryAllAsync_T_StopsOnError_MidPagination for the batch-get paths: create new tests for BatchGetAllAsync and BatchGetAllAsync<T> that use Mock<IDynamoClient> to simulate a successful first response with some UnprocessedKeys (or a non-empty RequestItems map) and then return an ErrorOr failure on the subsequent BatchGet call; call the client methods BatchGetAllAsync (and the generic BatchGetAllAsync<T>) with the same TestModel/TestReader and assert the same exception behavior (throwing the expected pagination/retry-related exception), and optionally add a similar ScanAllAsync error test to ensure consistent error propagation across ScanAllAsync, QueryAllAsync, and BatchGetAllAsync.src/Clients/Goa.Clients.Dynamo/DynamoExtensions.cs (1)
221-251: XML documentation is incomplete for new generic overloads.The new generic methods (
QueryAllAsync<T>,ScanAllAsync<T>,BatchGetAllAsync<T>) have minimal XML documentation compared to their non-generic counterparts. Consider adding<param>and<returns>tags for consistency with the existing methods.📝 Example documentation for QueryAllAsync<T>
/// <summary> /// Executes a typed DynamoDB Query operation with automatic pagination using an async iterator. /// </summary> +/// <typeparam name="T">The type to deserialize items into.</typeparam> +/// <param name="client">The DynamoDB client instance.</param> +/// <param name="tableName">The name of the table to query.</param> +/// <param name="itemReader">The reader delegate to deserialize items.</param> +/// <param name="builder">Action to configure the query using QueryBuilder.</param> +/// <param name="cancellationToken">Token to monitor for cancellation requests.</param> +/// <returns>An async enumerable that yields typed items from all pages of the query result.</returns> public static async IAsyncEnumerable<T> QueryAllAsync<T>(...)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Clients/Goa.Clients.Dynamo/DynamoExtensions.cs` around lines 221 - 251, The XML documentation for the generic paging overload QueryAllAsync<T> is incomplete; add XML doc tags consistent with the non-generic overloads: include a <typeparam name="T"> describing the item type, <param name="client">, <param name="tableName">, <param name="itemReader">, <param name="builder">, and <param name="cancellationToken"> describing each parameter, and a <returns> describing the IAsyncEnumerable<T> of paginated items (and mention that it may throw DynamoPaginationException). Update the same style for the other generic methods ScanAllAsync<T> and BatchGetAllAsync<T> to match existing documentation conventions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/Clients/Goa.Clients.Dynamo/DynamoExtensions.cs`:
- Around line 1-12: The extension methods use a missing generic reader and typed
results; define a delegate public delegate T DynamoItemReader<T>(ref
System.Text.Json.Utf8JsonReader reader) and then add generic overloads to
IDynamoClient for QueryAsync<T>, ScanAsync<T>, and BatchGetItemAsync<T> that
accept the DynamoItemReader<T> and return or populate strongly-typed result
types; either introduce QueryResult<T> and ScanResult<T> (wrapping existing
non-generic QueryResult/ScanResult with an Items: IReadOnlyList<T>) or change
the extension signatures to map non-generic results into typed lists using the
reader—update the calls in DynamoExtensions.QueryAsync, ScanAsync and
Batch-related methods to use the new generic overloads and result types.
---
Nitpick comments:
In `@src/Clients/Goa.Clients.Dynamo/DynamoExtensions.cs`:
- Around line 221-251: The XML documentation for the generic paging overload
QueryAllAsync<T> is incomplete; add XML doc tags consistent with the non-generic
overloads: include a <typeparam name="T"> describing the item type, <param
name="client">, <param name="tableName">, <param name="itemReader">, <param
name="builder">, and <param name="cancellationToken"> describing each parameter,
and a <returns> describing the IAsyncEnumerable<T> of paginated items (and
mention that it may throw DynamoPaginationException). Update the same style for
the other generic methods ScanAllAsync<T> and BatchGetAllAsync<T> to match
existing documentation conventions.
In `@tests/Clients/Goa.Clients.Dynamo.Tests/TypedExtensionTests.cs`:
- Around line 12-14: The TestReader delegate currently ignores its
Utf8JsonReader input and returns an empty TestModel; add a brief inline comment
above the TestReader declaration clarifying this is an intentional no-op used
because mocks supply pre-built QueryResult<TestModel> and ScanResult<TestModel>
objects (so the reader is never exercised in these tests), to prevent confusion
when reading TestReader and TestModel.
- Around line 82-106: Add parallel tests that mirror
QueryAllAsync_T_StopsOnError_MidPagination for the batch-get paths: create new
tests for BatchGetAllAsync and BatchGetAllAsync<T> that use Mock<IDynamoClient>
to simulate a successful first response with some UnprocessedKeys (or a
non-empty RequestItems map) and then return an ErrorOr failure on the subsequent
BatchGet call; call the client methods BatchGetAllAsync (and the generic
BatchGetAllAsync<T>) with the same TestModel/TestReader and assert the same
exception behavior (throwing the expected pagination/retry-related exception),
and optionally add a similar ScanAllAsync error test to ensure consistent error
propagation across ScanAllAsync, QueryAllAsync, and BatchGetAllAsync.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: eeb4acdb-17b2-4075-9055-c797d45e00a3
📒 Files selected for processing (3)
src/Clients/Goa.Clients.Dynamo/DynamoExtensions.cssrc/Clients/Goa.Clients.Dynamo/Exceptions/DynamoPaginationException.cstests/Clients/Goa.Clients.Dynamo.Tests/TypedExtensionTests.cs
Strip QueryAllAsync<T>, ScanAllAsync<T>, and BatchGetAllAsync<T> overloads and their tests — these belong to the typed API branch.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/Clients/Goa.Clients.Dynamo.Tests/TypedExtensionTests.cs (1)
10-75: Consider adding error scenario tests.The PR objectives mention "typed extension test coverage for pagination error scenarios," but the tests only cover successful pagination. Consider adding tests that verify
DynamoPaginationExceptionis thrown when the client returns an error mid-pagination. Additionally,BatchGetAllAsyncpagination is not covered.Example error scenario test
[Test] public async Task QueryAllAsync_ThrowsDynamoPaginationException_OnError() { var mock = new Mock<IDynamoClient>(); var error = Error.Failure("TestError", "Simulated pagination failure"); mock.Setup(c => c.QueryAsync(It.IsAny<QueryRequest>(), It.IsAny<CancellationToken>())) .ReturnsAsync(error); var enumerable = mock.Object.QueryAllAsync("TestTable", _ => { }); await Assert.ThrowsAsync<DynamoPaginationException>(async () => await enumerable.ToListAsync()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Clients/Goa.Clients.Dynamo.Tests/TypedExtensionTests.cs` around lines 10 - 75, Add tests that cover pagination error paths: for QueryAllAsync, ScanAllAsync and BatchGetAllAsync, mock IDynamoClient to return a successful first page (with LastEvaluatedKey) then return an Error (Error.Failure) on the subsequent QueryAsync/ScanAsync/BatchGetAsync call and assert that enumerating the IAsyncEnumerable throws DynamoPaginationException; also add a test where the first call immediately returns an Error and assert the same exception. Reference QueryAllAsync, ScanAllAsync, BatchGetAllAsync, IDynamoClient, QueryAsync, ScanAsync, BatchGetAsync, and DynamoPaginationException when locating where to add these tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/Clients/Goa.Clients.Dynamo.Tests/TypedExtensionTests.cs`:
- Line 1: Remove the unused using for Goa.Clients.Dynamo.Exceptions in
TypedExtensionTests.cs (the imported symbol DynamoPaginationException is not
referenced); either delete the using directive or add a test that asserts
expected behavior using DynamoPaginationException (e.g., catching/Asserting it)
so the import is justified—look for the top-of-file using block and update the
file accordingly.
---
Nitpick comments:
In `@tests/Clients/Goa.Clients.Dynamo.Tests/TypedExtensionTests.cs`:
- Around line 10-75: Add tests that cover pagination error paths: for
QueryAllAsync, ScanAllAsync and BatchGetAllAsync, mock IDynamoClient to return a
successful first page (with LastEvaluatedKey) then return an Error
(Error.Failure) on the subsequent QueryAsync/ScanAsync/BatchGetAsync call and
assert that enumerating the IAsyncEnumerable throws DynamoPaginationException;
also add a test where the first call immediately returns an Error and assert the
same exception. Reference QueryAllAsync, ScanAllAsync, BatchGetAllAsync,
IDynamoClient, QueryAsync, ScanAsync, BatchGetAsync, and
DynamoPaginationException when locating where to add these tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 22fecb20-f97e-4a2e-8ff4-703d9bb2bf5c
📒 Files selected for processing (2)
src/Clients/Goa.Clients.Dynamo/DynamoExtensions.cstests/Clients/Goa.Clients.Dynamo.Tests/TypedExtensionTests.cs
| @@ -0,0 +1,76 @@ | |||
| using Goa.Clients.Dynamo.Exceptions; | |||
Contributor
There was a problem hiding this comment.
Unused import.
Goa.Clients.Dynamo.Exceptions is imported but DynamoPaginationException is never used in these tests.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/Clients/Goa.Clients.Dynamo.Tests/TypedExtensionTests.cs` at line 1,
Remove the unused using for Goa.Clients.Dynamo.Exceptions in
TypedExtensionTests.cs (the imported symbol DynamoPaginationException is not
referenced); either delete the using directive or add a test that asserts
expected behavior using DynamoPaginationException (e.g., catching/Asserting it)
so the import is justified—look for the top-of-file using block and update the
file accordingly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Bug Fixes
Tests