feat: Add typed DynamoDB query, write, and batch APIs with auto-pagination#69
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds typed generic DynamoDB operations with automatic JSON serialization support. Introduces code generators for optimized JSON mappers and reader registration, adds item registries for runtime reader/writer delegates, and annotates operation models with explicit JSON property names for consistent serialization. Changes
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
Im5tu
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/Clients/Goa.Clients.Dynamo.Generator/DynamoMapperIncrementalGenerator.cs (1)
114-131:⚠️ Potential issue | 🔴 CriticalType discovery is still too shallow for the new JSON mapper.
Wiring
JsonMapperGeneratorin here assumesAnalyzeTypesfinds every nested type it may reference, butCollectReferencedTypescurrently bails out onSystem.Collections.*before unwrapping collection/dictionary members and only recurses into class symbols. AList<Address>,Dictionary<string, Address>, orAddressStructproperty can therefore emitDynamoJsonMapper.Address...calls without ever generating that mapper.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Clients/Goa.Clients.Dynamo.Generator/DynamoMapperIncrementalGenerator.cs` around lines 114 - 131, AnalyzeTypes' CollectReferencedTypes currently stops when encountering System.Collections.* and only recurses into class symbols, which causes JsonMapperGenerator to reference unmapped nested element types (e.g., List<Address>, Dictionary<string,Address>, AddressStruct). Modify CollectReferencedTypes so that when encountering collection or dictionary types it unwraps and visits their element/key/value type symbols instead of bailing out, and ensure recursion covers value types/structs (not just classes). Keep the JsonMapperGenerator usage (JsonMapperGenerator.GenerateCode) but make sure AnalyzeTypes/CollectReferencedTypes collects those referenced element types before GenerateCode is invoked.src/Clients/Goa.Clients.Dynamo/Serialization/DynamoJsonContext.cs (1)
1-33:⚠️ Potential issue | 🔴 Critical
[JsonGenerator]is unresolved in this project.CI is already failing with CS0234/CS0246 here, which means this target does not currently reference the assembly that provides
Goa.Json.GeneratorandJsonGeneratorAttribute. Please add the required package/analyzer reference, or keep the existing generation mechanism until that dependency is wired in.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Clients/Goa.Clients.Dynamo/Serialization/DynamoJsonContext.cs` around lines 1 - 33, The build is failing because the JsonGenerator attribute/namespace used on DynamoJsonContext (the [JsonGenerator(typeof(...))] annotations) comes from Goa.Json.Generator which is not referenced; either add the package/analyzer that supplies Goa.Json.Generator/JsonGeneratorAttribute to this project (update the project file to reference the NuGet or analyzer package that exposes the generator) or remove/replace the source generation attributes on the DynamoJsonContext class and keep the previous serialization implementation until the generator dependency is wired in.
🧹 Nitpick comments (6)
src/Clients/Goa.Clients.Dynamo/Operations/PutItem/PutItemResponse.cs (1)
27-28: Consider using theItemCollectionMetricsmodel type.The property type
Dictionary<string, List<Dictionary<string, AttributeValue>>>doesn't leverage the existingItemCollectionMetricsclass (seen insrc/Clients/Goa.Clients.Dynamo/Models/ItemCollectionMetrics.cs), which has properly typedItemCollectionKeyandSizeEstimateRangeGBproperties. Using the model class would provide better type safety and consistency.💡 Suggested type change
- [JsonPropertyName("ItemCollectionMetrics")] - public Dictionary<string, List<Dictionary<string, AttributeValue>>>? ItemCollectionMetrics { get; set; } + [JsonPropertyName("ItemCollectionMetrics")] + public Dictionary<string, List<ItemCollectionMetrics>>? ItemCollectionMetrics { get; set; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Clients/Goa.Clients.Dynamo/Operations/PutItem/PutItemResponse.cs` around lines 27 - 28, The ItemCollectionMetrics property in PutItemResponse.cs currently uses Dictionary<string, List<Dictionary<string, AttributeValue>>>, replace it with the strongly-typed model using Dictionary<string, List<ItemCollectionMetrics>> (the ItemCollectionMetrics class from Models/ItemCollectionMetrics.cs) to improve type safety; update any using/imports to reference the model's namespace and adjust deserialization expectations or tests that rely on the old nested Dictionary shape accordingly (look for the ItemCollectionMetrics property and the ItemCollectionMetrics class to make the change).src/Clients/Goa.Clients.Dynamo/Operations/Transactions/TransactWriteItemResponse.cs (1)
20-21: Consider using a strongly-typed collection forItemCollectionMetrics.The property is typed as
Dictionary<string, object>?which loses type safety. Based on theItemCollectionMetricsmodel insrc/Clients/Goa.Clients.Dynamo/Models/ItemCollectionMetrics.cs, DynamoDB returns this as a map of table names to lists ofItemCollectionMetricsobjects.♻️ Suggested change for type safety
- [JsonPropertyName("ItemCollectionMetrics")] - public Dictionary<string, object>? ItemCollectionMetrics { get; set; } + [JsonPropertyName("ItemCollectionMetrics")] + public Dictionary<string, List<ItemCollectionMetrics>>? ItemCollectionMetrics { get; set; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Clients/Goa.Clients.Dynamo/Operations/Transactions/TransactWriteItemResponse.cs` around lines 20 - 21, The ItemCollectionMetrics property in TransactWriteItemResponse (currently declared as Dictionary<string, object>?) is losing type safety; change its type to a strongly-typed map matching the model (Dictionary<string, List<ItemCollectionMetrics>>?) and import/qualify the ItemCollectionMetrics model from src/Clients/Goa.Clients.Dynamo.Models; update any deserialization/usage sites that expect object values to use List<ItemCollectionMetrics> instead and keep the nullable marker consistent with other response properties.src/Clients/Goa.Clients.Dynamo/DynamoItemReaderRegistry.cs (1)
21-27: Consider thread safety for concurrent access.The
Cache<T>.Readerfield is accessed without synchronization. While this is likely fine if registration happens exclusively at startup (before any reads), concurrent calls toRegisterandGetduring initialization could cause issues. If multi-threaded initialization is a concern, consider usingVolatile.Read/Volatile.WriteorInterlocked.CompareExchangefor the field access.💡 Optional: Add volatile semantics if needed
If thread-safety during initialization is a concern:
private static class Cache<T> { - public static DynamoItemReader<T>? Reader; + private static DynamoItemReader<T>? _reader; + public static DynamoItemReader<T>? Reader + { + get => Volatile.Read(ref _reader); + set => Volatile.Write(ref _reader, value); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Clients/Goa.Clients.Dynamo/DynamoItemReaderRegistry.cs` around lines 21 - 27, The Cache<T>.Reader field is accessed without any synchronization which can race with concurrent registration; make reads/writes to Cache<T>.Reader thread-safe by using volatile semantics or atomic operations—e.g., mark the storage as volatile or replace unsynchronized reads in DynamoItemReader<T>.Get and the registration path (e.g., Register or the method that assigns Cache<T>.Reader) with Volatile.Read/Volatile.Write or Interlocked.CompareExchange so that readers always see a fully-published reference and concurrent Register calls cannot produce torn/old values.docs/DynamoDbTypedApi.md (1)
439-445: Performance claim should be verifiable.The documentation states "26x fewer allocations" which is a specific benchmark claim. Consider linking to benchmark results or noting the conditions under which this was measured to help users understand the context.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/DynamoDbTypedApi.md` around lines 439 - 445, Update the "26x fewer allocations" performance claim in the DynamoDbTypedApi.md table by either (a) adding a citation link to the benchmark results that produced that number, or (b) replacing the raw number with a qualified statement that lists the measurement conditions (hardware, dataset size, runtime, GC settings, and version) and a link to the full benchmark; ensure the table cell for "Allocations" (the row comparing Untyped vs Typed) includes the reference or the parenthetical note so readers can verify the claim.tests/Clients/Goa.Clients.Dynamo.Tests/TypedExtensionTests.cs (1)
36-41: Tighten the pagination mocks to assert the actual continuation token.These setups only distinguish
ExclusiveStartKey == nullvs!= null, so the tests still pass if the extensions forward the wrong key between pages. For the typed overloads, matchingTestReaderexplicitly would also prove the caller-supplied reader is propagated.Also applies to: 69-74, 98-103, 131-136, 163-168
🤖 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 36 - 41, Mocks for pagination are too weak: replace the broad It.Is checks in the QueryAsync setups with predicates that assert the exact ExclusiveStartKey value passed between pages (e.g., compare the specific key field/value in QueryRequest.ExclusiveStartKey for first vs second page) and, for the typed overloads, also match the reader argument exactly (the TestReader delegate) so the setup only matches when the caller forwards the correct continuation token and the provided reader; apply the same tightening to the other similar setups referenced (the other QueryAsync/TestReader usages).src/Clients/Goa.Clients.Dynamo/IDynamoClient.cs (1)
114-117:PutItemAsync<T>narrows the existing PutItem surface quite a bit.Unlike the other typed overloads, this one drops
PutItemRequestentirely in favor oftableName+item. That means callers can't use conditions, return values, consumed-capacity options, etc. without falling back to the untyped API. I'd keep the typed serializer, but hang it off a request-shaped overload or typed builder before this public shape hardens.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Clients/Goa.Clients.Dynamo/IDynamoClient.cs` around lines 114 - 117, The PutItemAsync<T> signature currently removes PutItemRequest and thus prevents callers from specifying conditions, ReturnValues, ConsumedCapacity, etc.; restore a request-shaped overload that accepts a PutItemRequest (or a typed PutItemRequest<T>/builder) along with the DynamoItemWriter<T> so callers can set optional parameters while keeping zero-copy serialization. Specifically, add an overload such as Task<ErrorOr<PutItemResponse>> PutItemAsync<T>(PutItemRequest request, DynamoItemWriter<T> itemWriter, CancellationToken cancellationToken = default) (or introduce a typed/builder variant that produces a PutItemRequest) and keep the existing tableName+item convenience overload delegating to the request overload so no surface capabilities are lost.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/DynamoDbOptimisations.md`:
- Around line 174-187: The documentation is out of sync: update all occurrences
of the old types and API names to match the new interface surface — replace
DynamoJsonReader<T> and DynamoJsonWriter<T> with DynamoItemReader<T> and
DynamoItemWriter<T>, and update the examples and feature list to include the
now-exposed generic PutItemAsync<T> and the new typed QueryAsync<T> signature;
ensure any example method signatures, parameter names, and descriptions
reference QueryAsync<T>, DynamoItemReader<T>, DynamoItemWriter<T>, and
PutItemAsync<T> so readers and examples match the actual IDynamoClient API.
- Around line 7-11: The fenced ASCII diagrams starting with "HTTP Response
bytes" (the block showing "-> STJ deserializes to QueryResponse {
List<DynamoRecord> } (Pass 1)" and "-> DynamoMapper converts DynamoRecord to
Entity (Pass 2)") should include a language tag (use "text") on the opening
triple-backtick to satisfy markdownlint MD040; update that fenced block and the
similar block at the later location (lines showing the same ASCII flow around
48-57) by changing ``` to ```text so both code fences are explicitly tagged.
In `@docs/DynamoDbTypedApi.md`:
- Around line 300-316: The TransactGetItems example incorrectly sets each
TransactGetItem.Get to a TransactGetRequest; update the inner objects so the Get
property of TransactGetItem is constructed as a TransactGetItemRequest (not
TransactGetRequest) inside the TransactGetRequest's TransactItems list (replace
the two instances where new TransactGetRequest is used for the Get property with
new TransactGetItemRequest, keeping TableName and Key as-is).
In
`@src/Clients/Goa.Clients.Dynamo.Generator/CodeGeneration/JsonMapperGenerator.cs`:
- Around line 36-53: The generated mappers are referenced without their
namespace causing missing symbol errors; update GenerateCode/GenerateTypeMapper
so all lookups qualify DynamoJsonMapper with the target namespace (e.g.,
reference {targetNamespace}.DynamoJsonMapper.{TypeName} instead of
DynamoJsonMapper.{TypeName}), and apply the same namespace-qualification in the
collection/dictionary helper emitters (the blocks that emit mapper lookups
around the collection/dictionary handling mentioned at lines ~97-109, ~284-293,
~494-535); ensure the emitted identifier uses the computed targetNamespace
(falling back to "Generated" when empty) consistently wherever a mapper is
referenced.
- Around line 185-209: The code incorrectly treats empty strings as null by
using string.IsNullOrEmpty in both nullable and non-nullable string branches;
update the checks in JsonMapperGenerator.cs so only null becomes {"NULL":true}
and empty strings are preserved: for SpecialType.System_String use a null-only
check (e.g., {accessExpr} != null or !({accessExpr} is null)) instead of
string.IsNullOrEmpty when calling EmitPrimitiveTypeWrapper; ensure both branches
(the isNullable block and the non-nullable string runtime check) are changed and
keep EmitPrimitiveTypeWrapper, accessExpr, isNullable and hasUnixTimestamp usage
intact.
- Around line 659-665: The generated code currently calls Utf8Parser.TryParse
(via GetUtf8NumericParseExpression) and then unconditionally uses the
<property.Name>Parsed variable, which can be left default if parsing fails;
update the code emitted by JsonMapperGenerator.cs so every TryParse result is
checked (if true then assign propAccess = <property.Name>Parsed; else throw or
return a parse error/JsonException). Locate the blocks where
GetUtf8NumericParseExpression is used (the utf8Expr handling around
property.NameParsed and assignments to propAccess, as well as similar usages for
set/list elements and dictionary values) and wrap the assignment in an if-check
on the TryParse boolean, handling the failure path consistently (throwing a
descriptive exception or returning an error) instead of assuming the parsed
value is valid.
In
`@src/Clients/Goa.Clients.Dynamo.Generator/CodeGeneration/ReaderRegistrationGenerator.cs`:
- Around line 24-45: The registration code uses unqualified DynamoJsonMapper
references which fail when a registrable type lives in a different namespace
than the generated DynamoReaderRegistration; update the generation loop in
ReaderRegistrationGenerator (where registrableTypes are iterated and
NamingHelpers.NormalizeTypeName is used) to produce fully-qualified mapper
references: if type.Namespace != targetNamespace emit something like
global::{type.Namespace}.DynamoJsonMapper.{normalizedName} (otherwise keep
DynamoJsonMapper.{normalizedName}), and use that qualified identifier in the
DynamoItemReaderRegistry.Register and DynamoItemWriterRegistry.Register calls so
cross-namespace models reference the correct mapper class.
In `@src/Clients/Goa.Clients.Dynamo/DynamoExtensions.cs`:
- Around line 231-235: The iterator is silently swallowing failed pages by
returning early on result.IsError; change this to surface the failure instead of
yielding end-of-sequence: when client.QueryAsync returns a result with IsError,
throw the contained error (or propagate it as ErrorOr<T> if you prefer to change
the iterator signature) rather than calling yield break. Update the code paths
that check result.IsError (the instances around client.QueryAsync /
result.IsError and yield break and the analogous blocks at the other noted
spots: the checks at the same pattern near lines 263-267 and 335-339) to either
throw result.Error (or return ErrorOr<T>.FromError(...)) so callers receive the
failure instead of a truncated result set.
In `@src/Clients/Goa.Clients.Dynamo/DynamoServiceClient.cs`:
- Around line 351-368: The typed BatchGetItemAsync<T> path currently discards
"UnprocessedKeys" because ReadBatchGetItemResponse<T> skips that payload; update
the typed reader and result plumbing so UnprocessedKeys are parsed and returned
in BatchGetResult<T> instead of being skipped: modify
DynamoResponseReader.ReadBatchGetItemResponse (and any helper methods it uses)
to read and populate an UnprocessedKeys collection on BatchGetResult<T>, ensure
BatchGetItemAsync<T> returns that populated BatchGetResult<T>, and verify
BatchGetAllAsync<T> consumes UnprocessedKeys for retries.
In `@src/Clients/Goa.Clients.Dynamo/Operations/Batch/BatchGetResult`{T}.cs:
- Around line 18-23: BatchGetResult<T>.HasUnprocessedKeys is lying because
DynamoResponseReader.ReadBatchGetItemResponse<T> never populates
UnprocessedKeys; update ReadBatchGetItemResponse<T> to parse the
"UnprocessedKeys" field from the Dynamo response and assign a Dictionary<string,
BatchGetRequestItem> to BatchGetResult<T>.UnprocessedKeys (or set it to null if
absent). Locate the ReadBatchGetItemResponse<T> method and when reading the
top-level response object, detect and deserialize the "UnprocessedKeys" token
into the same shape as BatchGetRequestItem (reusing existing deserialization
helpers for attribute maps and keys), then set result.UnprocessedKeys =
parsedDict so HasUnprocessedKeys accurately reflects DynamoDB retry metadata.
In `@src/Clients/Goa.Clients.Dynamo/Operations/Batch/BatchWriteResult.cs`:
- Around line 14-15: Delete the unused BatchWriteResult class and its file:
remove the class BatchWriteResult (with properties UnprocessedPutItems,
UnprocessedDeleteKeys, ConsumedCapacityUnits and their JsonPropertyName
attributes) since it has no references and duplicates Dynamo's
BatchWriteItemResponse; after deletion, run a solution-wide search to confirm no
remaining references to BatchWriteResult and run the build/tests to verify
nothing else depends on it.
In `@src/Clients/Goa.Clients.Dynamo/Operations/DeleteItem/DeleteItemResponse.cs`:
- Around line 20-21: The DeleteItemResponse currently exposes
ConsumedCapacityUnits (double?) which does not match DynamoDB's wire format;
replace or augment it with a ConsumedCapacity object property named
ConsumedCapacity (matching the other response classes) so the JSON deserializer
can bind the DynamoDB response; if you need to preserve the scalar, add a
ConsumedCapacityUnits getter that derives its value from the new
ConsumedCapacity property (and mark the old property [Obsolete] if kept) and
ensure the property uses [JsonPropertyName("ConsumedCapacity")] and the type
matches the existing ConsumedCapacity model used across response classes.
In `@src/Clients/Goa.Clients.Dynamo/Operations/GetItem/GetItemResponse.cs`:
- Around line 20-21: The GetItemResponse class contains a bogus JSON-mapped
property ConsumedCapacityUnits that never appears in DynamoDB responses; remove
that property or make it a derived value that returns
ConsumedCapacity?.CapacityUnits instead. Locate the ConsumedCapacityUnits
property and either delete it, or change it to a non-serialized computed
property (no JsonPropertyName) that returns ConsumedCapacity?.CapacityUnits
(ensure ConsumedCapacity and its CapacityUnits member are used), and update any
callers to use the derived value.
In `@src/Clients/Goa.Clients.Dynamo/Operations/PutItem/PutItemRequest.cs`:
- Line 1: The four enum types ReturnValues, ReturnConsumedCapacity,
ReturnItemCollectionMetrics, and ReturnValuesOnConditionCheckFailure are missing
JSON string converters; add the attribute
[JsonConverter(typeof(JsonStringEnumConverter))] immediately above each enum
declaration (ensure System.Text.Json.Serialization is imported) so enums
serialize as their string names (e.g., "NONE", "ALL_OLD") rather than integers.
In `@src/Clients/Goa.Clients.Dynamo/Operations/Query/QueryResult.cs`:
- Around line 43-44: The non-generic QueryResult class incorrectly models
DynamoDB's ConsumedCapacity as a double property ConsumedCapacityUnits
(JsonPropertyName "ConsumedCapacityUnits"), which mismatches the generic
QueryResult<T> that uses a full ConsumedCapacity object and will fail
deserialization; change QueryResult to expose a ConsumedCapacity property of the
same type used in QueryResult<T> (and set its JsonPropertyName to
"ConsumedCapacity"), and if you need backward compatibility optionally keep the
existing ConsumedCapacityUnits as a deprecated passthrough that returns
ConsumedCapacity?.CapacityUnits (or similar) so callers still work while full
capacity details are preserved.
In `@src/Clients/Goa.Clients.Dynamo/Operations/Scan/ScanResult.cs`:
- Around line 43-44: The non-generic ScanResult class defines a scalar property
ConsumedCapacityUnits with JsonPropertyName("ConsumedCapacityUnits"), but
DynamoDB returns a ConsumedCapacity object and the generic ScanResult<T> uses
ConsumedCapacity with JsonPropertyName("ConsumedCapacity"); update the
non-generic ScanResult by replacing the scalar ConsumedCapacityUnits property
with a ConsumedCapacity property (matching the type used in ScanResult<T>) and
annotate it with JsonPropertyName("ConsumedCapacity") so deserialization matches
the DynamoDB wire format and mirrors ScanResult<T>.
---
Outside diff comments:
In
`@src/Clients/Goa.Clients.Dynamo.Generator/DynamoMapperIncrementalGenerator.cs`:
- Around line 114-131: AnalyzeTypes' CollectReferencedTypes currently stops when
encountering System.Collections.* and only recurses into class symbols, which
causes JsonMapperGenerator to reference unmapped nested element types (e.g.,
List<Address>, Dictionary<string,Address>, AddressStruct). Modify
CollectReferencedTypes so that when encountering collection or dictionary types
it unwraps and visits their element/key/value type symbols instead of bailing
out, and ensure recursion covers value types/structs (not just classes). Keep
the JsonMapperGenerator usage (JsonMapperGenerator.GenerateCode) but make sure
AnalyzeTypes/CollectReferencedTypes collects those referenced element types
before GenerateCode is invoked.
In `@src/Clients/Goa.Clients.Dynamo/Serialization/DynamoJsonContext.cs`:
- Around line 1-33: The build is failing because the JsonGenerator
attribute/namespace used on DynamoJsonContext (the [JsonGenerator(typeof(...))]
annotations) comes from Goa.Json.Generator which is not referenced; either add
the package/analyzer that supplies Goa.Json.Generator/JsonGeneratorAttribute to
this project (update the project file to reference the NuGet or analyzer package
that exposes the generator) or remove/replace the source generation attributes
on the DynamoJsonContext class and keep the previous serialization
implementation until the generator dependency is wired in.
---
Nitpick comments:
In `@docs/DynamoDbTypedApi.md`:
- Around line 439-445: Update the "26x fewer allocations" performance claim in
the DynamoDbTypedApi.md table by either (a) adding a citation link to the
benchmark results that produced that number, or (b) replacing the raw number
with a qualified statement that lists the measurement conditions (hardware,
dataset size, runtime, GC settings, and version) and a link to the full
benchmark; ensure the table cell for "Allocations" (the row comparing Untyped vs
Typed) includes the reference or the parenthetical note so readers can verify
the claim.
In `@src/Clients/Goa.Clients.Dynamo/DynamoItemReaderRegistry.cs`:
- Around line 21-27: The Cache<T>.Reader field is accessed without any
synchronization which can race with concurrent registration; make reads/writes
to Cache<T>.Reader thread-safe by using volatile semantics or atomic
operations—e.g., mark the storage as volatile or replace unsynchronized reads in
DynamoItemReader<T>.Get and the registration path (e.g., Register or the method
that assigns Cache<T>.Reader) with Volatile.Read/Volatile.Write or
Interlocked.CompareExchange so that readers always see a fully-published
reference and concurrent Register calls cannot produce torn/old values.
In `@src/Clients/Goa.Clients.Dynamo/IDynamoClient.cs`:
- Around line 114-117: The PutItemAsync<T> signature currently removes
PutItemRequest and thus prevents callers from specifying conditions,
ReturnValues, ConsumedCapacity, etc.; restore a request-shaped overload that
accepts a PutItemRequest (or a typed PutItemRequest<T>/builder) along with the
DynamoItemWriter<T> so callers can set optional parameters while keeping
zero-copy serialization. Specifically, add an overload such as
Task<ErrorOr<PutItemResponse>> PutItemAsync<T>(PutItemRequest request,
DynamoItemWriter<T> itemWriter, CancellationToken cancellationToken = default)
(or introduce a typed/builder variant that produces a PutItemRequest) and keep
the existing tableName+item convenience overload delegating to the request
overload so no surface capabilities are lost.
In `@src/Clients/Goa.Clients.Dynamo/Operations/PutItem/PutItemResponse.cs`:
- Around line 27-28: The ItemCollectionMetrics property in PutItemResponse.cs
currently uses Dictionary<string, List<Dictionary<string, AttributeValue>>>,
replace it with the strongly-typed model using Dictionary<string,
List<ItemCollectionMetrics>> (the ItemCollectionMetrics class from
Models/ItemCollectionMetrics.cs) to improve type safety; update any
using/imports to reference the model's namespace and adjust deserialization
expectations or tests that rely on the old nested Dictionary shape accordingly
(look for the ItemCollectionMetrics property and the ItemCollectionMetrics class
to make the change).
In
`@src/Clients/Goa.Clients.Dynamo/Operations/Transactions/TransactWriteItemResponse.cs`:
- Around line 20-21: The ItemCollectionMetrics property in
TransactWriteItemResponse (currently declared as Dictionary<string, object>?) is
losing type safety; change its type to a strongly-typed map matching the model
(Dictionary<string, List<ItemCollectionMetrics>>?) and import/qualify the
ItemCollectionMetrics model from src/Clients/Goa.Clients.Dynamo.Models; update
any deserialization/usage sites that expect object values to use
List<ItemCollectionMetrics> instead and keep the nullable marker consistent with
other response properties.
In `@tests/Clients/Goa.Clients.Dynamo.Tests/TypedExtensionTests.cs`:
- Around line 36-41: Mocks for pagination are too weak: replace the broad It.Is
checks in the QueryAsync setups with predicates that assert the exact
ExclusiveStartKey value passed between pages (e.g., compare the specific key
field/value in QueryRequest.ExclusiveStartKey for first vs second page) and, for
the typed overloads, also match the reader argument exactly (the TestReader
delegate) so the setup only matches when the caller forwards the correct
continuation token and the provided reader; apply the same tightening to the
other similar setups referenced (the other QueryAsync/TestReader usages).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0d365bad-9f71-4c7b-842a-37f9e85db79b
📒 Files selected for processing (62)
docs/DynamoDbOptimisations.mddocs/DynamoDbTypedApi.mdsrc/Clients/Goa.Clients.Dynamo.Generator/CodeGeneration/JsonMapperGenerator.cssrc/Clients/Goa.Clients.Dynamo.Generator/CodeGeneration/ReaderRegistrationGenerator.cssrc/Clients/Goa.Clients.Dynamo.Generator/DynamoMapperIncrementalGenerator.cssrc/Clients/Goa.Clients.Dynamo/DynamoExtensions.cssrc/Clients/Goa.Clients.Dynamo/DynamoItemReader.cssrc/Clients/Goa.Clients.Dynamo/DynamoItemReaderRegistry.cssrc/Clients/Goa.Clients.Dynamo/DynamoItemWriter.cssrc/Clients/Goa.Clients.Dynamo/DynamoItemWriterRegistry.cssrc/Clients/Goa.Clients.Dynamo/DynamoServiceClient.cssrc/Clients/Goa.Clients.Dynamo/Goa.Clients.Dynamo.csprojsrc/Clients/Goa.Clients.Dynamo/IDynamoClient.cssrc/Clients/Goa.Clients.Dynamo/Internal/DynamoResponseReader.cssrc/Clients/Goa.Clients.Dynamo/Models/AttributeValue.cssrc/Clients/Goa.Clients.Dynamo/Models/CapacityDetail.cssrc/Clients/Goa.Clients.Dynamo/Models/ConsumedCapacity.cssrc/Clients/Goa.Clients.Dynamo/Models/ItemCollectionMetrics.cssrc/Clients/Goa.Clients.Dynamo/Operations/Batch/BatchGetItemRequest.cssrc/Clients/Goa.Clients.Dynamo/Operations/Batch/BatchGetItemResponse.cssrc/Clients/Goa.Clients.Dynamo/Operations/Batch/BatchGetRequestItem.cssrc/Clients/Goa.Clients.Dynamo/Operations/Batch/BatchGetResult.cssrc/Clients/Goa.Clients.Dynamo/Operations/Batch/BatchGetResult{T}.cssrc/Clients/Goa.Clients.Dynamo/Operations/Batch/BatchWriteItemRequest.cssrc/Clients/Goa.Clients.Dynamo/Operations/Batch/BatchWriteItemResponse.cssrc/Clients/Goa.Clients.Dynamo/Operations/Batch/BatchWriteRequestItem.cssrc/Clients/Goa.Clients.Dynamo/Operations/Batch/BatchWriteResult.cssrc/Clients/Goa.Clients.Dynamo/Operations/Batch/DeleteRequest.cssrc/Clients/Goa.Clients.Dynamo/Operations/Batch/PutRequest.cssrc/Clients/Goa.Clients.Dynamo/Operations/DeleteItem/DeleteItemRequest.cssrc/Clients/Goa.Clients.Dynamo/Operations/DeleteItem/DeleteItemResponse.cssrc/Clients/Goa.Clients.Dynamo/Operations/GetItem/GetItemRequest.cssrc/Clients/Goa.Clients.Dynamo/Operations/GetItem/GetItemResponse.cssrc/Clients/Goa.Clients.Dynamo/Operations/PutItem/PutItemRequest.cssrc/Clients/Goa.Clients.Dynamo/Operations/PutItem/PutItemResponse.cssrc/Clients/Goa.Clients.Dynamo/Operations/Query/QueryRequest.cssrc/Clients/Goa.Clients.Dynamo/Operations/Query/QueryResponse.cssrc/Clients/Goa.Clients.Dynamo/Operations/Query/QueryResult.cssrc/Clients/Goa.Clients.Dynamo/Operations/Scan/ScanRequest.cssrc/Clients/Goa.Clients.Dynamo/Operations/Scan/ScanResponse.cssrc/Clients/Goa.Clients.Dynamo/Operations/Scan/ScanResult.cssrc/Clients/Goa.Clients.Dynamo/Operations/Transactions/TransactConditionCheckItem.cssrc/Clients/Goa.Clients.Dynamo/Operations/Transactions/TransactDeleteItem.cssrc/Clients/Goa.Clients.Dynamo/Operations/Transactions/TransactGetItem.cssrc/Clients/Goa.Clients.Dynamo/Operations/Transactions/TransactGetItemRequest.cssrc/Clients/Goa.Clients.Dynamo/Operations/Transactions/TransactGetItemResponse.cssrc/Clients/Goa.Clients.Dynamo/Operations/Transactions/TransactGetRequest.cssrc/Clients/Goa.Clients.Dynamo/Operations/Transactions/TransactGetResult.cssrc/Clients/Goa.Clients.Dynamo/Operations/Transactions/TransactGetResult{T}.cssrc/Clients/Goa.Clients.Dynamo/Operations/Transactions/TransactPutItem.cssrc/Clients/Goa.Clients.Dynamo/Operations/Transactions/TransactUpdateItem.cssrc/Clients/Goa.Clients.Dynamo/Operations/Transactions/TransactWriteItem.cssrc/Clients/Goa.Clients.Dynamo/Operations/Transactions/TransactWriteItemResponse.cssrc/Clients/Goa.Clients.Dynamo/Operations/Transactions/TransactWriteOperation.cssrc/Clients/Goa.Clients.Dynamo/Operations/Transactions/TransactWriteRequest.cssrc/Clients/Goa.Clients.Dynamo/Operations/UpdateItem/UpdateItemRequest.cssrc/Clients/Goa.Clients.Dynamo/Operations/UpdateItem/UpdateItemResponse.cssrc/Clients/Goa.Clients.Dynamo/Serialization/DynamoJsonContext.cstests/Clients/Goa.Clients.Dynamo.Tests/BuilderChainingTests.cstests/Clients/Goa.Clients.Dynamo.Tests/DynamoItemWriterRegistryTests.cstests/Clients/Goa.Clients.Dynamo.Tests/DynamoResponseReaderTests.cstests/Clients/Goa.Clients.Dynamo.Tests/TypedExtensionTests.cs
💤 Files with no reviewable changes (1)
- src/Clients/Goa.Clients.Dynamo/Operations/Batch/BatchGetResult.cs
Comment on lines
+7
to
+11
| ``` | ||
| HTTP Response bytes | ||
| -> STJ deserializes to QueryResponse { List<DynamoRecord> } (Pass 1) | ||
| -> DynamoMapper converts DynamoRecord to Entity (Pass 2) | ||
| ``` |
Contributor
There was a problem hiding this comment.
Add a language tag to these fenced blocks.
Both snippets are bare fences right now, which triggers markdownlint MD040 on this new doc. text is enough for these ASCII flow diagrams.
Also applies to: 48-57
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 7-7: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/DynamoDbOptimisations.md` around lines 7 - 11, The fenced ASCII diagrams
starting with "HTTP Response bytes" (the block showing "-> STJ deserializes to
QueryResponse { List<DynamoRecord> } (Pass 1)" and "-> DynamoMapper converts
DynamoRecord to Entity (Pass 2)") should include a language tag (use "text") on
the opening triple-backtick to satisfy markdownlint MD040; update that fenced
block and the similar block at the later location (lines showing the same ASCII
flow around 48-57) by changing ``` to ```text so both code fences are explicitly
tagged.
Comment on lines
+36
to
+53
| foreach (var ns in typesByNamespace) | ||
| { | ||
| var targetNamespace = string.IsNullOrEmpty(ns.Key) ? "Generated" : ns.Key; | ||
| builder.AppendLine(); | ||
| builder.AppendLine($"namespace {targetNamespace}"); | ||
| builder.OpenBrace(); | ||
| { | ||
| builder.OpenBraceWithLine("public static class DynamoJsonMapper"); | ||
|
|
||
| foreach (var type in ns) | ||
| { | ||
| builder.AppendLine($"// {type.FullName}"); | ||
| GenerateTypeMapper(builder, type, context); | ||
| } | ||
|
|
||
| builder.CloseBrace(); | ||
| } | ||
| builder.CloseBrace(); |
Contributor
There was a problem hiding this comment.
Namespace-qualify generated mapper lookups.
GenerateCode emits one DynamoJsonMapper per namespace, but these call sites always emit DynamoJsonMapper.{Type} as if the nested type or concrete subtype lived in the current namespace. A model with a cross-namespace complex member or subtype will resolve to a mapper that does not exist and the generated code will not compile. The same unqualified lookup pattern is repeated in the collection/dictionary helpers below.
Also applies to: 97-109, 284-293, 494-535
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/Clients/Goa.Clients.Dynamo.Generator/CodeGeneration/JsonMapperGenerator.cs`
around lines 36 - 53, The generated mappers are referenced without their
namespace causing missing symbol errors; update GenerateCode/GenerateTypeMapper
so all lookups qualify DynamoJsonMapper with the target namespace (e.g.,
reference {targetNamespace}.DynamoJsonMapper.{TypeName} instead of
DynamoJsonMapper.{TypeName}), and apply the same namespace-qualification in the
collection/dictionary helper emitters (the blocks that emit mapper lookups
around the collection/dictionary handling mentioned at lines ~97-109, ~284-293,
~494-535); ensure the emitted identifier uses the computed targetNamespace
(falling back to "Generated" when empty) consistently wherever a mapper is
referenced.
Comment on lines
+20
to
21
| [JsonPropertyName("ConsumedCapacityUnits")] | ||
| public double? ConsumedCapacityUnits { get; set; } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
DynamoDB GetItem response JSON format ConsumedCapacity
💡 Result:
In a DynamoDB GetItem response, ConsumedCapacity is only present if you set ReturnConsumedCapacity in the request (TOTAL or INDEXES).[1]
Response JSON shape (relevant part)
{
"ConsumedCapacity": {
"CapacityUnits": 1,
"ReadCapacityUnits": 1,
"WriteCapacityUnits": 0,
"TableName": "MyTable",
"Table": {
"CapacityUnits": 1,
"ReadCapacityUnits": 1,
"WriteCapacityUnits": 0
},
"LocalSecondaryIndexes": {
"MyLSI": { "CapacityUnits": 1, "ReadCapacityUnits": 1, "WriteCapacityUnits": 0 }
},
"GlobalSecondaryIndexes": {
"MyGSI": { "CapacityUnits": 1, "ReadCapacityUnits": 1, "WriteCapacityUnits": 0 }
}
},
"Item": { }
}What the fields mean
CapacityUnits: total capacity units consumed by the operation.[2]ReadCapacityUnits/WriteCapacityUnits: total read/write capacity units consumed.[2]TableName: table name (or table ARN if you provided an ARN in the request).[2]Table: per-table consumption breakdown.[2]LocalSecondaryIndexes/GlobalSecondaryIndexes: per-index breakdown (included whenReturnConsumedCapacity=INDEXES; forGetItem, these are often absent becauseGetItemdoesn’t read from indexes).[1]
GetItem sample response with ReturnConsumedCapacity: "TOTAL" shows the minimal form typically returned:
{
"ConsumedCapacity": { "CapacityUnits": 1, "TableName": "Thread" },
"Item": { ... }
}
```[1]
Sources: [1] [2]
---
🏁 Script executed:
```shell
cat -n "src/Clients/Goa.Clients.Dynamo/Operations/GetItem/GetItemResponse.cs" | head -30Repository: Im5tu/goa
Length of output: 1077
Remove or derive ConsumedCapacityUnits from the correct API structure.
The DynamoDB GetItem API returns ConsumedCapacity as an object (with nested CapacityUnits), not a top-level ConsumedCapacityUnits field. The property on line 20 will never deserialize from actual DynamoDB responses since the JSON field "ConsumedCapacityUnits" does not exist in the API.
Either remove this property or compute it from ConsumedCapacity?.CapacityUnits instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Clients/Goa.Clients.Dynamo/Operations/GetItem/GetItemResponse.cs` around
lines 20 - 21, The GetItemResponse class contains a bogus JSON-mapped property
ConsumedCapacityUnits that never appears in DynamoDB responses; remove that
property or make it a derived value that returns ConsumedCapacity?.CapacityUnits
instead. Locate the ConsumedCapacityUnits property and either delete it, or
change it to a non-serialized computed property (no JsonPropertyName) that
returns ConsumedCapacity?.CapacityUnits (ensure ConsumedCapacity and its
CapacityUnits member are used), and update any callers to use the derived value.
| @@ -1,3 +1,4 @@ | |||
| using System.Text.Json.Serialization; | |||
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check if enum types have JSON string serialization configured
# Search for JsonStringEnumConverter usage in serializer options
rg -n "JsonStringEnumConverter" --type cs
# Check enum definitions for JsonConverter attributes
for enum_name in "ReturnValues" "ReturnConsumedCapacity" "ReturnItemCollectionMetrics" "ReturnValuesOnConditionCheckFailure"; do
echo "=== Checking enum: $enum_name ==="
ast-grep --pattern "enum $enum_name { $$$ }"
doneRepository: Im5tu/goa
Length of output: 363
🏁 Script executed:
#!/bin/bash
# Find where these enums are actually defined
echo "=== Searching for enum definitions in Dynamo project ==="
rg -n "enum (ReturnValues|ReturnConsumedCapacity|ReturnItemCollectionMetrics|ReturnValuesOnConditionCheckFailure)" --type cs
# Also search for references to these enums in PutItemRequest.cs
echo ""
echo "=== Checking PutItemRequest.cs full content ==="
find . -name "PutItemRequest.cs" -path "*Dynamo*" -type f
# Search for JsonSerializerOptions configuration
echo ""
echo "=== Searching for JsonSerializerOptions configuration ==="
rg -n "JsonSerializerOptions|AddJsonOptions" --type cs | head -20Repository: Im5tu/goa
Length of output: 4500
🏁 Script executed:
#!/bin/bash
# Check the contents of the enum files
for enum_file in \
"src/Clients/Goa.Clients.Dynamo/Enums/ReturnValues.cs" \
"src/Clients/Goa.Clients.Dynamo/Enums/ReturnConsumedCapacity.cs" \
"src/Clients/Goa.Clients.Dynamo/Enums/ReturnItemCollectionMetrics.cs" \
"src/Clients/Goa.Clients.Dynamo/Enums/ReturnValuesOnConditionCheckFailure.cs"; do
echo "=== $enum_file ==="
cat "$enum_file"
echo ""
doneRepository: Im5tu/goa
Length of output: 2663
🏁 Script executed:
#!/bin/bash
# Search for global JsonSerializerOptions configuration in Dynamo project
echo "=== Searching for JsonSerializerOptions in Dynamo project ==="
find src/Clients/Goa.Clients.Dynamo -type f -name "*.cs" | xargs rg -l "JsonSerializerOptions" 2>/dev/null
echo ""
echo "=== Searching for service configuration/extension methods ==="
find src/Clients/Goa.Clients.Dynamo -type f -name "*.cs" | xargs rg -l "AddJsonOptions|AddJson|ConfigureJson" 2>/dev/null | head -20
echo ""
echo "=== Checking PutItemRequest.cs for the full context ==="
wc -l src/Clients/Goa.Clients.Dynamo/Operations/PutItem/PutItemRequest.cs
head -80 src/Clients/Goa.Clients.Dynamo/Operations/PutItem/PutItemRequest.csRepository: Im5tu/goa
Length of output: 2960
Add [JsonConverter(typeof(JsonStringEnumConverter))] to all enum definitions.
The JsonPropertyName attributes are good and follow the existing pattern. However, the four enum types used in this file lack JsonConverter attributes and will serialize as integers by default, breaking DynamoDB API calls which expect string values like "NONE", "ALL_OLD", etc.
Update these enum files:
src/Clients/Goa.Clients.Dynamo/Enums/ReturnValues.cssrc/Clients/Goa.Clients.Dynamo/Enums/ReturnConsumedCapacity.cssrc/Clients/Goa.Clients.Dynamo/Enums/ReturnItemCollectionMetrics.cssrc/Clients/Goa.Clients.Dynamo/Enums/ReturnValuesOnConditionCheckFailure.cs
Add the attribute before each enum definition: [JsonConverter(typeof(JsonStringEnumConverter))]
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Clients/Goa.Clients.Dynamo/Operations/PutItem/PutItemRequest.cs` at line
1, The four enum types ReturnValues, ReturnConsumedCapacity,
ReturnItemCollectionMetrics, and ReturnValuesOnConditionCheckFailure are missing
JSON string converters; add the attribute
[JsonConverter(typeof(JsonStringEnumConverter))] immediately above each enum
declaration (ensure System.Text.Json.Serialization is imported) so enums
serialize as their string names (e.g., "NONE", "ALL_OLD") rather than integers.
- Replace DynamoJsonContext with standard JsonSerializable attributes - Add ResolveJsonTypeInfo override to DynamoServiceClient - Fix JsonMapperGenerator to handle records with primary constructors and init-only properties - Add DynamoConverter attribute support to generator PropertyInfo model - Update BuilderChainingTests to use STJ serialization
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (4)
src/Clients/Goa.Clients.Dynamo.Generator/CodeGeneration/JsonMapperGenerator.cs (3)
801-807:⚠️ Potential issue | 🔴 CriticalCheck
Utf8Parser.TryParsebefore using the parsed value.These branches discard the boolean return and then assign/add the
*Parsedvariable unconditionally. On malformed numeric payloads, the generated code silently uses default zero values instead of failing fast.Also applies to: 925-930, 1030-1035, 1114-1119
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Clients/Goa.Clients.Dynamo.Generator/CodeGeneration/JsonMapperGenerator.cs` around lines 801 - 807, The generated branches call GetUtf8NumericParseExpression and then use the resulting "{property.Name}Parsed" value without checking the boolean result of Utf8Parser.TryParse, causing malformed numeric payloads to silently fall back to default zero; update the code paths (the block using utf8Expr in JsonMapperGenerator.cs and the similar blocks around the other occurrences) so that after emitting the Utf8Parser.TryParse expression you test its boolean result before assigning/adding the parsed variable (e.g., only set propAccess = {property.Name}Parsed or add to collection when TryParse returned true), and if TryParse returns false emit appropriate failure handling (throw/return error) consistent with surrounding mapper error handling.
193-217:⚠️ Potential issue | 🟠 MajorPreserve empty strings instead of serializing them as
NULL.Both string branches use
string.IsNullOrEmpty, so""is written as{"NULL":true}and no longer round-trips as a present attribute. These checks need to be null-only.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Clients/Goa.Clients.Dynamo.Generator/CodeGeneration/JsonMapperGenerator.cs` around lines 193 - 217, The current string handling in JsonMapperGenerator.cs mistakenly treats empty strings as NULL by using string.IsNullOrEmpty in both the nullable and non-nullable branches; replace those string.IsNullOrEmpty checks with a null-only check (e.g. {accessExpr} != null) in both places that guard EmitPrimitiveTypeWrapper (the branch where isNullable is true and the non-nullable string special-case) so empty strings are preserved and only actual nulls are serialized as {"NULL": true}.
105-108:⚠️ Potential issue | 🔴 CriticalNamespace-qualify generated mapper calls.
These emissions always reference the current namespace's
DynamoJsonMapper. If a subtype or nested member type lives in another namespace, the generated call resolves to a mapper that does not exist and the file will not compile.Also applies to: 299-300, 367-368, 457-458, 541-542, 851-852, 994-995, 1102-1103
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Clients/Goa.Clients.Dynamo.Generator/CodeGeneration/JsonMapperGenerator.cs` around lines 105 - 108, The generated calls to DynamoJsonMapper are not namespace-qualified, causing unresolved references when the target mapper lives in a different namespace; update the emission in JsonMapperGenerator (e.g., where NamingHelpers.NormalizeTypeName(concreteType.Name) and concreteType.FullName are used) to prefix the mapper with the concrete type's declaring namespace (or the resolved mapper namespace) and the global:: qualifier (e.g., global::{MapperNamespace}.DynamoJsonMapper.{normalizedTypeName}.WriteToJson(...)); apply the same change to all other emission sites (mentions at the other line ranges) so the generated code always references the fully-qualified DynamoJsonMapper class.src/Clients/Goa.Clients.Dynamo/DynamoServiceClient.cs (1)
320-337:⚠️ Potential issue | 🟠 MajorTyped batch get still returns partial results as complete.
src/Clients/Goa.Clients.Dynamo/Internal/DynamoResponseReader.csinReadBatchGetItemResponse<T>still skips"UnprocessedKeys", so this overload drops the retry payload and makes partial batch responses look complete. That also leaves any auto-retry helper with nothing to continue from.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Clients/Goa.Clients.Dynamo/DynamoServiceClient.cs` around lines 320 - 337, The typed BatchGet still treats partial results as complete because DynamoResponseReader.ReadBatchGetItemResponse<T> ignores the "UnprocessedKeys" JSON property; update ReadBatchGetItemResponse<T> to parse and include "UnprocessedKeys" into the returned BatchGetResult<T> (the same structure used by BatchGetItemAsync -> DynamoResponseReader.ReadBatchGetItemResponse(buffer.Span, itemReader)), so the BatchGetResult<T> contains both the retrieved Items and the UnprocessedKeys payload for retry helpers to use; ensure you deserialize the same key structure used by the untyped implementation and populate the corresponding property on BatchGetResult<T> rather than discarding it.
🧹 Nitpick comments (1)
tests/Clients/Goa.Clients.Dynamo.Tests/BuilderChainingTests.cs (1)
339-349: Consider using the simpler string-returning overload.The implementation is correct, but
JsonSerializer.Serialize<T>(T, JsonTypeInfo<T>)returns a string directly, avoiding the manual stream/writer setup.♻️ Proposed simplification
private static string SerializeWithGeneratedWriter<T>(T value) { var typeInfo = DynamoJsonContext.Default.GetTypeInfo(typeof(T)) as System.Text.Json.Serialization.Metadata.JsonTypeInfo<T> ?? throw new System.InvalidOperationException($"Cannot find type {typeof(T).Name} in serialization context"); - using var buffer = new System.IO.MemoryStream(); - using var jsonWriter = new System.Text.Json.Utf8JsonWriter(buffer); - System.Text.Json.JsonSerializer.Serialize(jsonWriter, value, typeInfo); - jsonWriter.Flush(); - return System.Text.Encoding.UTF8.GetString(buffer.ToArray()); + return JsonSerializer.Serialize(value, typeInfo); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Clients/Goa.Clients.Dynamo.Tests/BuilderChainingTests.cs` around lines 339 - 349, The SerializeWithGeneratedWriter<T> method manually creates a MemoryStream and Utf8JsonWriter to serialize using a JsonTypeInfo; replace that with the simpler string-returning overload System.Text.Json.JsonSerializer.Serialize<T>(T value, System.Text.Json.Serialization.Metadata.JsonTypeInfo<T> jsonTypeInfo) by calling JsonSerializer.Serialize(value, typeInfo) inside SerializeWithGeneratedWriter<T> (keeping the same typeInfo retrieval and the same exception behavior).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/Clients/Goa.Clients.Dynamo.Generator/CodeGeneration/JsonMapperGenerator.cs`:
- Around line 579-581: The readableProperties filter currently excludes
properties for JSON read when IsSupportedForJsonRead(p) is false, which causes
converter-backed properties to be skipped; update the predicate used to build
readableProperties (the var readableProperties in JsonMapperGenerator.cs) so
that properties with p.ConverterTypeName != null or
_typeHandlerRegistry.CanHandle(p) are allowed regardless of
IsSupportedForJsonRead(p) -- i.e., change the Where clause to short-circuit:
(p.ConverterTypeName != null || _typeHandlerRegistry.CanHandle(p) ||
IsSupportedForJsonRead(p)) and apply the same change to the other
readableProperties query referenced in the file so both read-path filters treat
converter-backed properties identically.
- Around line 1160-1175: FindBestConstructor currently picks a parameterized
ctor using constructors.FirstOrDefault(...) which is non-deterministic because
type.Symbol.Constructors has undefined order; update FindBestConstructor to
choose deterministically — either (A) sort the constructors collection by a
stable criterion (e.g., descending Parameters.Length, then by parameter type
names or names) and then pick the first parameterized constructor, or (B) detect
a C# 11+ primary constructor by inspecting DeclaringSyntaxReferences for the
type (look for a TypeDeclarationSyntax/RecordDeclarationSyntax with a
ParameterList) and prefer that constructor (falling back to the sorted list) so
selection in FindBestConstructor is stable and correctly prefers true primary
constructors.
In `@src/Clients/Goa.Clients.Dynamo/DynamoServiceClient.cs`:
- Around line 367-377: The code deserializes an error into the local variable
error, enriches it (sets Payload and StatusCode and calls
ProcessAwsErrorHeaders) but then ignores those changes by returning a newly
built Error.Failure; instead, preserve and return the enriched error
information: after calling DeserializeJsonError and ProcessAwsErrorHeaders
(functions referenced), map the enriched error.Type/Code using
MapErrorCodeToDynamo and create/return an Error.Failure that includes the
already-enriched error's Payload, StatusCode and any processed header fields (or
convert/return the enriched error object directly if compatible) so callers
receive the additional diagnostics.
- Around line 268-285: The generic GetItemAsync<T> method currently uses T? and
returns default(T) for missing items which collapses missing vs zero-value for
value types; update the method signature GetItemAsync to either add a
reference-type constraint (where T : class) so T? behaves as nullable reference,
or change the return contract to an explicit optional wrapper (e.g.,
ErrorOr<Optional<T>> or Nullable<T> equivalent) and propagate that through
DynamoResponseReader.ReadGetItemResponse and the default-return site
(default(T)) so callers can distinguish missing items from zero-valued value
types; ensure all call sites and the ReadGetItemResponse API are updated
consistently.
---
Duplicate comments:
In
`@src/Clients/Goa.Clients.Dynamo.Generator/CodeGeneration/JsonMapperGenerator.cs`:
- Around line 801-807: The generated branches call GetUtf8NumericParseExpression
and then use the resulting "{property.Name}Parsed" value without checking the
boolean result of Utf8Parser.TryParse, causing malformed numeric payloads to
silently fall back to default zero; update the code paths (the block using
utf8Expr in JsonMapperGenerator.cs and the similar blocks around the other
occurrences) so that after emitting the Utf8Parser.TryParse expression you test
its boolean result before assigning/adding the parsed variable (e.g., only set
propAccess = {property.Name}Parsed or add to collection when TryParse returned
true), and if TryParse returns false emit appropriate failure handling
(throw/return error) consistent with surrounding mapper error handling.
- Around line 193-217: The current string handling in JsonMapperGenerator.cs
mistakenly treats empty strings as NULL by using string.IsNullOrEmpty in both
the nullable and non-nullable branches; replace those string.IsNullOrEmpty
checks with a null-only check (e.g. {accessExpr} != null) in both places that
guard EmitPrimitiveTypeWrapper (the branch where isNullable is true and the
non-nullable string special-case) so empty strings are preserved and only actual
nulls are serialized as {"NULL": true}.
- Around line 105-108: The generated calls to DynamoJsonMapper are not
namespace-qualified, causing unresolved references when the target mapper lives
in a different namespace; update the emission in JsonMapperGenerator (e.g.,
where NamingHelpers.NormalizeTypeName(concreteType.Name) and
concreteType.FullName are used) to prefix the mapper with the concrete type's
declaring namespace (or the resolved mapper namespace) and the global::
qualifier (e.g.,
global::{MapperNamespace}.DynamoJsonMapper.{normalizedTypeName}.WriteToJson(...));
apply the same change to all other emission sites (mentions at the other line
ranges) so the generated code always references the fully-qualified
DynamoJsonMapper class.
In `@src/Clients/Goa.Clients.Dynamo/DynamoServiceClient.cs`:
- Around line 320-337: The typed BatchGet still treats partial results as
complete because DynamoResponseReader.ReadBatchGetItemResponse<T> ignores the
"UnprocessedKeys" JSON property; update ReadBatchGetItemResponse<T> to parse and
include "UnprocessedKeys" into the returned BatchGetResult<T> (the same
structure used by BatchGetItemAsync ->
DynamoResponseReader.ReadBatchGetItemResponse(buffer.Span, itemReader)), so the
BatchGetResult<T> contains both the retrieved Items and the UnprocessedKeys
payload for retry helpers to use; ensure you deserialize the same key structure
used by the untyped implementation and populate the corresponding property on
BatchGetResult<T> rather than discarding it.
---
Nitpick comments:
In `@tests/Clients/Goa.Clients.Dynamo.Tests/BuilderChainingTests.cs`:
- Around line 339-349: The SerializeWithGeneratedWriter<T> method manually
creates a MemoryStream and Utf8JsonWriter to serialize using a JsonTypeInfo;
replace that with the simpler string-returning overload
System.Text.Json.JsonSerializer.Serialize<T>(T value,
System.Text.Json.Serialization.Metadata.JsonTypeInfo<T> jsonTypeInfo) by calling
JsonSerializer.Serialize(value, typeInfo) inside SerializeWithGeneratedWriter<T>
(keeping the same typeInfo retrieval and the same exception behavior).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e5c5c0e3-1eee-4909-a411-d9d873601d11
📒 Files selected for processing (6)
src/Clients/Goa.Clients.Dynamo.Generator/Attributes/DynamoConverterAttributeHandler.cssrc/Clients/Goa.Clients.Dynamo.Generator/CodeGeneration/JsonMapperGenerator.cssrc/Clients/Goa.Clients.Dynamo.Generator/Models/DynamoConverterAttributeInfo.cssrc/Clients/Goa.Clients.Dynamo.Generator/Models/PropertyInfo.cssrc/Clients/Goa.Clients.Dynamo/DynamoServiceClient.cstests/Clients/Goa.Clients.Dynamo.Tests/BuilderChainingTests.cs
Comment on lines
+40
to
+43
| public void ValidateAttribute(AttributeInfo attributeInfo, ISymbol symbol, Action<Diagnostic> reportDiagnostic) | ||
| { | ||
| // No additional validation needed at this stage | ||
| } |
Contributor
There was a problem hiding this comment.
Validate converter shape before codegen.
JsonMapperGenerator later emits new {converter}.Write(...) and new {converter}.Read(...). With ValidateAttribute empty, a converter missing the expected API or a public parameterless ctor only shows up as broken generated code instead of a diagnostic on the annotated symbol.
Comment on lines
+579
to
+581
| var readableProperties = allProperties | ||
| .Where(p => (p.ConverterTypeName != null || _typeHandlerRegistry.CanHandle(p)) && !p.IsIgnored(IgnoreDirection.WhenReading) && p.Symbol?.SetMethod != null && IsSupportedForJsonRead(p)) | ||
| .ToList(); |
Contributor
There was a problem hiding this comment.
Don't filter converter-backed properties out of the read path.
Lines 723-728 already delegate [DynamoConverter] properties to the custom reader, but these readableProperties filters still require IsSupportedForJsonRead(p). That makes converter-backed unsupported shapes write successfully and then get skipped during deserialization, leaving the property at its default value.
Representative fix
- .Where(p => (p.ConverterTypeName != null || _typeHandlerRegistry.CanHandle(p)) && !p.IsIgnored(IgnoreDirection.WhenReading) && p.Symbol?.SetMethod != null && IsSupportedForJsonRead(p))
+ .Where(p => (p.ConverterTypeName != null || (_typeHandlerRegistry.CanHandle(p) && IsSupportedForJsonRead(p)))
+ && !p.IsIgnored(IgnoreDirection.WhenReading)
+ && p.Symbol?.SetMethod != null)Apply the same condition in both readableProperties queries.
Also applies to: 628-632
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/Clients/Goa.Clients.Dynamo.Generator/CodeGeneration/JsonMapperGenerator.cs`
around lines 579 - 581, The readableProperties filter currently excludes
properties for JSON read when IsSupportedForJsonRead(p) is false, which causes
converter-backed properties to be skipped; update the predicate used to build
readableProperties (the var readableProperties in JsonMapperGenerator.cs) so
that properties with p.ConverterTypeName != null or
_typeHandlerRegistry.CanHandle(p) are allowed regardless of
IsSupportedForJsonRead(p) -- i.e., change the Where clause to short-circuit:
(p.ConverterTypeName != null || _typeHandlerRegistry.CanHandle(p) ||
IsSupportedForJsonRead(p)) and apply the same change to the other
readableProperties query referenced in the file so both read-path filters treat
converter-backed properties identically.
Comment on lines
+1160
to
+1175
| private static IMethodSymbol? FindBestConstructor(DynamoTypeInfo type) | ||
| { | ||
| var constructors = type.Symbol.Constructors | ||
| .Where(c => c.DeclaredAccessibility == Accessibility.Public) | ||
| // Exclude the synthesized record copy constructor (single parameter of the same type) | ||
| .Where(c => !(c.Parameters.Length == 1 && | ||
| SymbolEqualityComparer.Default.Equals(c.Parameters[0].Type, type.Symbol))) | ||
| .ToList(); | ||
|
|
||
| // Prefer constructor with parameters (primary constructor) | ||
| var paramConstructor = constructors.FirstOrDefault(c => c.Parameters.Any()); | ||
| if (paramConstructor != null) | ||
| return paramConstructor; | ||
|
|
||
| // Fallback to parameterless constructor | ||
| return constructors.FirstOrDefault(c => !c.Parameters.Any()); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /tmp && find . -name "JsonMapperGenerator.cs" 2>/dev/null | head -5Repository: Im5tu/goa
Length of output: 35
🏁 Script executed:
find . -name "JsonMapperGenerator.cs" -type fRepository: Im5tu/goa
Length of output: 133
🏁 Script executed:
find . -name "*.cs" -path "*/JsonMapperGenerator*" -type fRepository: Im5tu/goa
Length of output: 133
🏁 Script executed:
find . -name "*JsonMapperGenerator*" -type fRepository: Im5tu/goa
Length of output: 133
🏁 Script executed:
find . -type f -name "*.cs" | grep -i "mapper" | head -20Repository: Im5tu/goa
Length of output: 471
🏁 Script executed:
git ls-files | grep -i "jsonmapper"Repository: Im5tu/goa
Length of output: 131
🏁 Script executed:
git ls-files | grep -i "dynamo.*generator"Repository: Im5tu/goa
Length of output: 6221
🏁 Script executed:
git ls-files src/Clients/Goa.Clients.Dynamo.Generator/ 2>/dev/null | head -20Repository: Im5tu/goa
Length of output: 1626
🏁 Script executed:
git ls-files | grep "JsonMapperGenerator"Repository: Im5tu/goa
Length of output: 131
🏁 Script executed:
rg -l "FindBestConstructor" --type csRepository: Im5tu/goa
Length of output: 206
🏁 Script executed:
ls -la src/Clients/Goa.Clients.Dynamo.Generator/CodeGeneration/ 2>/dev/null || echo "Path not found"Repository: Im5tu/goa
Length of output: 806
🏁 Script executed:
find . -type d -name "Dynamo*" 2>/dev/nullRepository: Im5tu/goa
Length of output: 85
🏁 Script executed:
fd "JsonMapperGenerator" . 2>/dev/nullRepository: Im5tu/goa
Length of output: 133
🏁 Script executed:
rg "FindBestConstructor" -l 2>/dev/nullRepository: Im5tu/goa
Length of output: 206
🏁 Script executed:
git ls-files | head -50Repository: Im5tu/goa
Length of output: 2519
🏁 Script executed:
git ls-files src/ 2>/dev/null | head -30Repository: Im5tu/goa
Length of output: 2202
🏁 Script executed:
ls -la src/ 2>/dev/null || ls -la . | head -20Repository: Im5tu/goa
Length of output: 478
🏁 Script executed:
find . -maxdepth 3 -type d -name "Goa*" 2>/dev/nullRepository: Im5tu/goa
Length of output: 1834
🏁 Script executed:
rg "FindBestConstructor" --max-count 10 2>/dev/nullRepository: Im5tu/goa
Length of output: 609
🏁 Script executed:
rg "private static IMethodSymbol\? FindBestConstructor" 2>/dev/nullRepository: Im5tu/goa
Length of output: 206
🏁 Script executed:
rg "FindBestConstructor" -A 5 2>/dev/null | head -50Repository: Im5tu/goa
Length of output: 2962
🏁 Script executed:
rg "Exclude the synthesized record copy constructor" 2>/dev/nullRepository: Im5tu/goa
Length of output: 230
🏁 Script executed:
rg "FirstOrDefault.*Parameters\.Any" 2>/dev/nullRepository: Im5tu/goa
Length of output: 670
🏁 Script executed:
rg "DynamoTypeInfo" -l 2>/dev/null | head -10Repository: Im5tu/goa
Length of output: 838
🏁 Script executed:
rg "class DynamoTypeInfo" 2>/dev/nullRepository: Im5tu/goa
Length of output: 146
🏁 Script executed:
git log --oneline --all | head -20Repository: Im5tu/goa
Length of output: 1384
🏁 Script executed:
git statusRepository: Im5tu/goa
Length of output: 118
🏁 Script executed:
rg "primary constructor" -i 2>/dev/nullRepository: Im5tu/goa
Length of output: 199
🏁 Script executed:
rg "Constructors" -A 3 2>/dev/null | head -50Repository: Im5tu/goa
Length of output: 2734
🏁 Script executed:
find . -type f -name "*.cs" | xargs grep -l "JsonMapper" 2>/dev/null | head -10Repository: Im5tu/goa
Length of output: 371
🏁 Script executed:
find . -type f -name "*.cs" -exec grep -l "FindBestConstructor" {} \;Repository: Im5tu/goa
Length of output: 210
🏁 Script executed:
git ls-files | grep -E "\.cs$" | wc -lRepository: Im5tu/goa
Length of output: 56
🏁 Script executed:
git ls-files | grep -E "\.cs$" | head -30Repository: Im5tu/goa
Length of output: 2155
🏁 Script executed:
rg "synthesized record copy constructor" 2>/dev/nullRepository: Im5tu/goa
Length of output: 230
🏁 Script executed:
rg "synthesized.*record" -i 2>/dev/nullRepository: Im5tu/goa
Length of output: 230
🏁 Script executed:
git ls-files "*.cs" | head -20Repository: Im5tu/goa
Length of output: 1533
🏁 Script executed:
git ls-files | head -50Repository: Im5tu/goa
Length of output: 2519
🏁 Script executed:
git rev-parse --show-toplevelRepository: Im5tu/goa
Length of output: 71
🏁 Script executed:
git ls-files --cached | head -30Repository: Im5tu/goa
Length of output: 1066
🏁 Script executed:
rg "Exclude the synthesized record" 2>/dev/nullRepository: Im5tu/goa
Length of output: 230
🏁 Script executed:
rg "SymbolEqualityComparer\.Default\.Equals" 2>/dev/nullRepository: Im5tu/goa
Length of output: 232
🏁 Script executed:
rg "Prefer constructor with parameters" 2>/dev/nullRepository: Im5tu/goa
Length of output: 320
🏁 Script executed:
rg "Fallback to parameterless constructor" 2>/dev/nullRepository: Im5tu/goa
Length of output: 304
🏁 Script executed:
rg "\.Constructors" | head -20Repository: Im5tu/goa
Length of output: 474
🏁 Script executed:
rg "Roslyn" -i 2>/dev/null | head -10Repository: Im5tu/goa
Length of output: 681
🏁 Script executed:
rg "IMethodSymbol" 2>/dev/null | head -10Repository: Im5tu/goa
Length of output: 1708
🏁 Script executed:
sed -n '1160,1180p' src/Clients/Goa.Clients.Dynamo.Generator/CodeGeneration/JsonMapperGenerator.csRepository: Im5tu/goa
Length of output: 1023
🏁 Script executed:
wc -l src/Clients/Goa.Clients.Dynamo.Generator/CodeGeneration/JsonMapperGenerator.csRepository: Im5tu/goa
Length of output: 136
🏁 Script executed:
grep -n "private static IMethodSymbol? FindBestConstructor" src/Clients/Goa.Clients.Dynamo.Generator/CodeGeneration/JsonMapperGenerator.csRepository: Im5tu/goa
Length of output: 132
🏁 Script executed:
sed -n '1154,1180p' src/Clients/Goa.Clients.Dynamo.Generator/CodeGeneration/JsonMapperGenerator.csRepository: Im5tu/goa
Length of output: 1189
🏁 Script executed:
grep -n "private IMethodSymbol? FindBestConstructor" src/Clients/Goa.Clients.Dynamo.Generator/CodeGeneration/MapperGenerator.csRepository: Im5tu/goa
Length of output: 124
🏁 Script executed:
sed -n '478,510p' src/Clients/Goa.Clients.Dynamo.Generator/CodeGeneration/MapperGenerator.csRepository: Im5tu/goa
Length of output: 1475
🏁 Script executed:
grep -rn "FindBestConstructor" tests/Repository: Im5tu/goa
Length of output: 35
🏁 Script executed:
grep -rn "multiple.*constructor" tests/ -i | head -20Repository: Im5tu/goa
Length of output: 35
🏁 Script executed:
grep -A 20 "parameterized" tests/Clients/Goa.Clients.Dynamo.Generator.Tests/CodeGeneration/MapperGeneratorTests.cs | head -40Repository: Im5tu/goa
Length of output: 35
🏁 Script executed:
sed -n '1,50p' src/Clients/Goa.Clients.Dynamo.Generator/Models/DynamoTypeInfo.csRepository: Im5tu/goa
Length of output: 994
🏁 Script executed:
grep -rn "primary" src/Clients/Goa.Clients.Dynamo.Generator/ -iRepository: Im5tu/goa
Length of output: 1435
🏁 Script executed:
grep -rn "IMethodSymbol" src/Clients/Goa.Clients.Dynamo.Generator/CodeGeneration/ | head -20Repository: Im5tu/goa
Length of output: 992
🏁 Script executed:
rg "CreateConstructorSymbol" tests/ -A 5 -B 2 | head -60Repository: Im5tu/goa
Length of output: 992
🏁 Script executed:
rg "constructor" tests/Clients/Goa.Clients.Dynamo.Generator.Tests/CodeGeneration/MapperGeneratorTests.cs -i -A 3 -B 1 | head -80Repository: Im5tu/goa
Length of output: 35
🏁 Script executed:
sed -n '1100,1190p' src/Clients/Goa.Clients.Dynamo.Generator/CodeGeneration/JsonMapperGenerator.csRepository: Im5tu/goa
Length of output: 4705
🌐 Web query:
Roslyn IMethodSymbol Constructors ordering deterministic
💡 Result:
Roslyn does not document any guaranteed ordering for constructor lists like INamedTypeSymbol.InstanceConstructors / .Constructors (they’re just ImmutableArray<IMethodSymbol>). So you should treat the sequence as unspecified and not rely on it being declaration-order or stable across runs/versions.
Even though Roslyn’s compiler output is designed to be deterministic given the same inputs, the Roslyn team has explicitly called out that ordering wasn’t historically guaranteed and has had multiple “make ordering deterministic” work items/bugs across APIs/collections of symbols/locations. That’s a strong signal you should not depend on incidental ordering from symbol enumeration APIs. [1], [2]
What to do instead (deterministic ordering):
- If the constructors are from source: sort by
constructor.Locations(primary) and the source span (SourceSpan.Start) /DeclaringSyntaxReferencesposition. - If from metadata: sort by a stable key you define (e.g.,
(MethodKind, MetadataName, Parameters.Length, parameter types’ fully-qualified names)), since “declaration order” may not exist in a meaningful way.
Bottom line: If you need deterministic ordering, sort them yourself with an explicit comparer. [1], [2]
References:
[1] Roslyn determinism umbrella issue tracking non-deterministic orderings across APIs/collections: https://lightrun.com/answers/dotnet-roslyn-umbrella-compilers-should-be-deterministic-same-inputs-generate-same-outputs
[2] Roslyn deterministic builds discussion noting ordering wasn’t guaranteed and required explicit work/tests: https://blog.paranoidcoding.org/2016/04/05/deterministic-builds-in-roslyn.html
🌐 Web query:
C# Roslyn primary constructor detection IMethodSymbol
💡 Result:
To detect whether an IMethodSymbol represents a C# 12 primary constructor, you generally can’t rely on MethodKind alone (it’s still just Constructor). Instead, distinguish it by what syntax node declares it.
Robust check (works for source symbols)
static bool IsPrimaryConstructor(IMethodSymbol ctor, CancellationToken ct = default)
{
if (ctor.MethodKind != MethodKind.Constructor)
return false; // still the right first gate [1]
// Primary constructor "declaration" is the *type declaration* (class/struct) with a ParameterList,
// not a ConstructorDeclarationSyntax.
var syntax = ctor.DeclaringSyntaxReferences.FirstOrDefault()?.GetSyntax(ct);
return syntax is TypeDeclarationSyntax { ParameterList: not null };
}Notes
- On the syntax side, a primary constructor is represented as the type’s
ParameterListSyntax(e.g.,ClassDeclarationSyntax.ParameterList). [2] - Roslyn itself has internal helpers like
INamedTypeSymbol.TryGetPrimaryConstructor(...)used by analyzers, but it’s not part of the public API surface you can call directly. [3]
Sources: MethodKind Constructor enum value [1]; primary ctor appears as ParameterList on the type declaration [2]; Roslyn analyzer usage of TryGetPrimaryConstructor [3].
[1] https://learn.microsoft.com/en-us/dotnet/api/microsoft.codeanalysis.methodkind?view=roslyn-dotnet-4.14.0
[2] https://stackoverflow.com/questions/78397407/c-sharp-detect-primary-constructor-with-roslyn
[3] https://source.dot.net/Microsoft.CodeAnalysis.CSharp.Features/src/Analyzers/CSharp/Analyzers/UsePrimaryConstructor/CSharpUsePrimaryConstructorDiagnosticAnalyzer.cs.html
🌐 Web query:
Roslyn symbol.Constructors order First() selection
💡 Result:
INamedTypeSymbol.Constructors (and InstanceConstructors / StaticConstructors) returns an ImmutableArray<IMethodSymbol>, but you should not rely on its order—so symbol.Constructors.First() is not a stable way to pick “the” constructor (order can vary across compilations / source vs metadata, etc.). (sourceroslyn.io)
If you need a specific constructor, filter explicitly, e.g.:
- Parameterless instance ctor:
var ctor = type.InstanceConstructors.Single(c => c.Parameters.Length == 0);(sourceroslyn.io)
- Pick by signature (parameter types/count), accessibility, attributes, etc. (e.g.,
Where(...)+OrderBy(...)on your own criteria). - If multiple match, decide deterministically (e.g., sort by
Parameters.Length, then parameter type display name, etc.) rather than usingFirst(). (goalkicker.com)
Citations:
- 1: https://sourceroslyn.io/microsoft.codeanalysis/Symbols/INamedTypeSymbol.cs.html
- 2: https://sourceroslyn.io/microsoft.codeanalysis/Symbols/INamedTypeSymbol.cs.html
- 3: https://goalkicker.com/CSharpBook/CSharpProfessionalTipsSecrets.pdf?utm_source=openai
🏁 Script executed:
grep -rn "multiple.*constructor" tests/ -iRepository: Im5tu/goa
Length of output: 35
🏁 Script executed:
grep -rn "ctor.*ctor\|constructor.*constructor" tests/Clients/Goa.Clients.Dynamo.Generator.Tests/ | head -10Repository: Im5tu/goa
Length of output: 2146
🏁 Script executed:
sed -n '245,275p' src/Clients/Goa.Clients.Dynamo.Generator/CodeGeneration/JsonMapperGenerator.csRepository: Im5tu/goa
Length of output: 1626
🏁 Script executed:
sed -n '600,650p' src/Clients/Goa.Clients.Dynamo.Generator/CodeGeneration/JsonMapperGenerator.csRepository: Im5tu/goa
Length of output: 2317
🏁 Script executed:
grep -rn "multiple\|ambiguous\|overload" src/Clients/Goa.Clients.Dynamo.Generator/ -i | grep -i constructorRepository: Im5tu/goa
Length of output: 35
🏁 Script executed:
cat src/Clients/Goa.Clients.Dynamo.Generator/README.md 2>/dev/null | head -100Repository: Im5tu/goa
Length of output: 3254
Constructor selection via FirstOrDefault() is non-deterministic when multiple parameterized constructors exist.
The code uses constructors.FirstOrDefault(c => c.Parameters.Any()) to select a parameterized constructor, but Roslyn does not guarantee the ordering of the Constructors collection. Per Roslyn documentation, this collection order is undefined and can vary across compilations or versions.
This means if a type has multiple public parameterized constructors (e.g., new Foo(int x) and new Foo(string name)), the selected constructor depends on Roslyn's internal ordering, causing non-deterministic code generation.
Additionally, the comment mentions "primary constructor" but the code doesn't actually detect C# 11+ primary constructors via syntax inspection—it just picks whichever parameterized constructor happens to come first from the unordered collection.
To fix: Sort constructors explicitly by a stable criterion (e.g., parameter count), or use DeclaringSyntaxReferences with TypeDeclarationSyntax.ParameterList to detect and prefer the actual primary constructor.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/Clients/Goa.Clients.Dynamo.Generator/CodeGeneration/JsonMapperGenerator.cs`
around lines 1160 - 1175, FindBestConstructor currently picks a parameterized
ctor using constructors.FirstOrDefault(...) which is non-deterministic because
type.Symbol.Constructors has undefined order; update FindBestConstructor to
choose deterministically — either (A) sort the constructors collection by a
stable criterion (e.g., descending Parameters.Length, then by parameter type
names or names) and then pick the first parameterized constructor, or (B) detect
a C# 11+ primary constructor by inspecting DeclaringSyntaxReferences for the
type (look for a TypeDeclarationSyntax/RecordDeclarationSyntax with a
ParameterList) and prefer that constructor (falling back to the sorted list) so
selection in FindBestConstructor is stable and correctly prefers true primary
constructors.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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`:
- Around line 83-109: The test QueryAllAsync_T_StopsOnError_MidPagination
currently asserts silent truncation; if the production QueryAllAsync<T> behavior
was changed to throw DynamoPaginationException on mid-pagination errors, update
this test (QueryAllAsync_T_StopsOnError_MidPagination) to expect that exception:
replace the current call and result assertions with an
Assert.ThrowsAsync<DynamoPaginationException> around
mock.Object.QueryAllAsync("TestTable", TestReader, _ => { }).ToListAsync(),
keeping the same mock setup for firstPage and secondPage so the failure occurs
during pagination.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 84394d2c-ad5e-40e0-8cb7-9f155588d30e
📒 Files selected for processing (2)
src/Clients/Goa.Clients.Dynamo/DynamoExtensions.cstests/Clients/Goa.Clients.Dynamo.Tests/TypedExtensionTests.cs
Empty strings are valid DynamoDB attribute values and should not be treated as NULL. Use != null instead of IsNullOrEmpty for null checks in generated writer code.
Typed QueryAllAsync<T>, ScanAllAsync<T>, and BatchGetAllAsync<T> now throw DynamoPaginationException on mid-pagination errors, consistent with the non-generic pagination methods.
Include Payload and StatusCode in Error.Failure metadata so callers can inspect AWS error details for diagnostics.
Replace reader.Skip() with proper parsing of UnprocessedKeys so typed BatchGetAllAsync<T> can retry unprocessed keys correctly.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
src/Clients/Goa.Clients.Dynamo.Generator/CodeGeneration/JsonMapperGenerator.cs (4)
798-804:⚠️ Potential issue | 🔴 CriticalCheck
Utf8Parser.TryParsebefore assigning parsed values.These branches still discard the boolean result and always use the
*Parsedtemp. A malformed Dynamo number therefore silently becomes0/default instead of failing the read.What does `System.Buffers.Text.Utf8Parser.TryParse` guarantee about the out value when the method returns false?Also applies to: 922-927, 1027-1032, 1111-1116
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Clients/Goa.Clients.Dynamo.Generator/CodeGeneration/JsonMapperGenerator.cs` around lines 798 - 804, The current generated branches created by GetUtf8NumericParseExpression use Utf8Parser.TryParse but ignore its boolean result and always assign the temp like {property.Name}Parsed into {propAccess}; change each of those branches (locations around lines using GetUtf8NumericParseExpression, e.g., the block that appends "{utf8Expr};" and then "{propAccess} = {property.Name}Parsed;") to capture and test the boolean return value from Utf8Parser.TryParse and only assign {property.Name}Parsed to {propAccess} when TryParse returns true; on false, handle the parse failure consistently (e.g., throw a read/parse exception or set a failure flag and bail out) so malformed Dynamo numeric values do not silently become default zeros.
1157-1172:⚠️ Potential issue | 🟠 MajorMake constructor selection deterministic.
constructors.FirstOrDefault(c => c.Parameters.Any())still depends on Roslyn's constructor enumeration order, so a type with multiple public parameterized constructors can generate different read paths across compilations. Pick a ctor with an explicit ordering rule, or detect the actual primary constructor before falling back.Does Roslyn guarantee the order of `INamedTypeSymbol.Constructors`, and how can a C# primary constructor be detected from `IMethodSymbol`?
576-578:⚠️ Potential issue | 🟠 MajorDon't filter converter-backed members out before the read branch.
Both
readablePropertiespredicates still requireIsSupportedForJsonRead(p), so a property withConverterTypeNamecan write through the custom converter but never reaches the custom read path at Lines 720-725 when its underlying shape is otherwise unsupported. Short-circuit the filter for converter-backed properties.Also applies to: 625-629
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Clients/Goa.Clients.Dynamo.Generator/CodeGeneration/JsonMapperGenerator.cs` around lines 576 - 578, The filter that builds readableProperties is incorrectly excluding converter-backed members by applying IsSupportedForJsonRead(p) even when p.ConverterTypeName != null; change the predicate so that if p.ConverterTypeName != null the IsSupportedForJsonRead check is skipped (i.e., short-circuit to include converter-backed properties), while keeping the existing checks for _typeHandlerRegistry.CanHandle(p), !p.IsIgnored(IgnoreDirection.WhenReading) and p.Symbol?.SetMethod; make the analogous change in the other similar filter block (the one around lines 625-629) so both read-branch code paths can reach the custom converter read logic (refer to the read branch handling at lines ~720-725).
97-109:⚠️ Potential issue | 🔴 CriticalNamespace-qualify generated mapper calls.
These sites still emit
DynamoJsonMapper.{Type}in the current namespace. As soon as a subtype or complex member lives in another namespace, the generated file references a mapper that does not exist and stops compiling. Build the mapper reference from the target symbol's namespace instead of assuming the emitter's current namespace.Also applies to: 294-298, 364-365, 454-455, 538-539, 845-849, 989-992, 1097-1100
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Clients/Goa.Clients.Dynamo.Generator/CodeGeneration/JsonMapperGenerator.cs` around lines 97 - 109, The generated mapper calls (e.g., in GenerateAbstractWriteDispatch) currently emit DynamoJsonMapper.{Type} without the target type's namespace, causing missing references when the concrete subtype lives in another namespace; update the generator to build a fully-qualified mapper reference using the concrete type's namespace + the DynamoJsonMapper class and the normalized type name (use the concrete type symbol's namespace/containing namespace together with NamingHelpers.NormalizeTypeName(concreteType.Name)) wherever you emit DynamoJsonMapper.{...} (including the other similar emit sites listed), so the code emits something like <TargetNamespace>.DynamoJsonMapper.{NormalizedType} instead of assuming the current file namespace.src/Clients/Goa.Clients.Dynamo/DynamoServiceClient.cs (1)
268-285:⚠️ Potential issue | 🟠 Major
GetItemAsync<T>still collapses "missing" intodefault(T).For an unconstrained generic,
T?is only a nullability annotation. WhenTis a value type, the empty-body fallback here and theReadGetItemResponsefallback both produce the same zero value as a real item, so callers cannot distinguish "not found" from "found default". ConstrainTto reference types or return an explicit optional wrapper.In C#, what does `T?` mean for an unconstrained generic return type, and does it become `Nullable<T>` for value types?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Clients/Goa.Clients.Dynamo/DynamoServiceClient.cs` around lines 268 - 285, GetItemAsync<T> collapses "not found" into default(T) for value types; fix by constraining T to reference types: change the method signature to Add "where T : class" on GetItemAsync<T> and ensure the early-empty response returns null (not default(T)), and that DynamoResponseReader.ReadGetItemResponse returns T? for reference types; update any callers to handle nullable reference returns. This avoids conflating a missing item with a value-type default; alternatively, if you prefer value-type support, change the return to an explicit optional wrapper (e.g., Optional<T> or ErrorOr<Maybe<T>>) and adjust ReadGetItemResponse and caller handling accordingly.
🧹 Nitpick comments (1)
tests/Clients/Goa.Clients.Dynamo.Tests/TypedExtensionTests.cs (1)
17-106: Add coverage for typedBatchGetAllAsync<T>retries.This file exercises typed query/scan pagination, but the new typed batch path in
src/Clients/Goa.Clients.Dynamo/DynamoExtensions.csLines 328-357 is still untested. A regression inUnprocessedKeysround-tripping would currently slip through.🤖 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 17 - 106, Add tests to TypedExtensionTests.cs that exercise the typed BatchGetAllAsync<T> retry path in DynamoExtensions (method BatchGetAllAsync<T>) by mocking IDynamoClient.BatchGetAsync: set up a first response (ErrorOr<BatchGetResult<TestModel>>) containing Items with a non-empty UnprocessedKeys dictionary and a second response returning the remaining Items with UnprocessedKeys empty; call mock.Object.BatchGetAllAsync("TestTable", TestReader, keys, _ => { }).ToListAsync() and assert both items are returned in order. Also add a test for error mid-retry where the second BatchGetAsync returns Error.Failure and assert BatchGetAllAsync<T> throws DynamoPaginationException. Use symbols BatchGetAllAsync<T>, IDynamoClient.BatchGetAsync, BatchGetResult<TestModel>, UnprocessedKeys, TestReader, and DynamoPaginationException to locate the code to change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Clients/Goa.Clients.Dynamo/Internal/DynamoResponseReader.cs`:
- Around line 172-175: The switch in DynamoResponseReader.cs currently skips the
"B" and "BS" wire types which causes LastEvaluatedKey and UnprocessedKeys to
contain empty AttributeValue entries; update the handling for the "B" and "BS"
cases inside the reader method (the switch that currently matches "B"/"BS") to
fully parse binary values (and binary sets) into the AttributeValue
representation used by the rest of the code, or if parsing is not possible,
throw a descriptive exception instead of calling reader.Skip(); ensure this
change produces valid AttributeValue objects consumed by the typed
pagination/retry logic in DynamoExtensions.cs so binary-key tables are not
resent with blank keys.
---
Duplicate comments:
In
`@src/Clients/Goa.Clients.Dynamo.Generator/CodeGeneration/JsonMapperGenerator.cs`:
- Around line 798-804: The current generated branches created by
GetUtf8NumericParseExpression use Utf8Parser.TryParse but ignore its boolean
result and always assign the temp like {property.Name}Parsed into {propAccess};
change each of those branches (locations around lines using
GetUtf8NumericParseExpression, e.g., the block that appends "{utf8Expr};" and
then "{propAccess} = {property.Name}Parsed;") to capture and test the boolean
return value from Utf8Parser.TryParse and only assign {property.Name}Parsed to
{propAccess} when TryParse returns true; on false, handle the parse failure
consistently (e.g., throw a read/parse exception or set a failure flag and bail
out) so malformed Dynamo numeric values do not silently become default zeros.
- Around line 576-578: The filter that builds readableProperties is incorrectly
excluding converter-backed members by applying IsSupportedForJsonRead(p) even
when p.ConverterTypeName != null; change the predicate so that if
p.ConverterTypeName != null the IsSupportedForJsonRead check is skipped (i.e.,
short-circuit to include converter-backed properties), while keeping the
existing checks for _typeHandlerRegistry.CanHandle(p),
!p.IsIgnored(IgnoreDirection.WhenReading) and p.Symbol?.SetMethod; make the
analogous change in the other similar filter block (the one around lines
625-629) so both read-branch code paths can reach the custom converter read
logic (refer to the read branch handling at lines ~720-725).
- Around line 97-109: The generated mapper calls (e.g., in
GenerateAbstractWriteDispatch) currently emit DynamoJsonMapper.{Type} without
the target type's namespace, causing missing references when the concrete
subtype lives in another namespace; update the generator to build a
fully-qualified mapper reference using the concrete type's namespace + the
DynamoJsonMapper class and the normalized type name (use the concrete type
symbol's namespace/containing namespace together with
NamingHelpers.NormalizeTypeName(concreteType.Name)) wherever you emit
DynamoJsonMapper.{...} (including the other similar emit sites listed), so the
code emits something like <TargetNamespace>.DynamoJsonMapper.{NormalizedType}
instead of assuming the current file namespace.
In `@src/Clients/Goa.Clients.Dynamo/DynamoServiceClient.cs`:
- Around line 268-285: GetItemAsync<T> collapses "not found" into default(T) for
value types; fix by constraining T to reference types: change the method
signature to Add "where T : class" on GetItemAsync<T> and ensure the early-empty
response returns null (not default(T)), and that
DynamoResponseReader.ReadGetItemResponse returns T? for reference types; update
any callers to handle nullable reference returns. This avoids conflating a
missing item with a value-type default; alternatively, if you prefer value-type
support, change the return to an explicit optional wrapper (e.g., Optional<T> or
ErrorOr<Maybe<T>>) and adjust ReadGetItemResponse and caller handling
accordingly.
---
Nitpick comments:
In `@tests/Clients/Goa.Clients.Dynamo.Tests/TypedExtensionTests.cs`:
- Around line 17-106: Add tests to TypedExtensionTests.cs that exercise the
typed BatchGetAllAsync<T> retry path in DynamoExtensions (method
BatchGetAllAsync<T>) by mocking IDynamoClient.BatchGetAsync: set up a first
response (ErrorOr<BatchGetResult<TestModel>>) containing Items with a non-empty
UnprocessedKeys dictionary and a second response returning the remaining Items
with UnprocessedKeys empty; call mock.Object.BatchGetAllAsync("TestTable",
TestReader, keys, _ => { }).ToListAsync() and assert both items are returned in
order. Also add a test for error mid-retry where the second BatchGetAsync
returns Error.Failure and assert BatchGetAllAsync<T> throws
DynamoPaginationException. Use symbols BatchGetAllAsync<T>,
IDynamoClient.BatchGetAsync, BatchGetResult<TestModel>, UnprocessedKeys,
TestReader, and DynamoPaginationException to locate the code to change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fbc8205d-16f8-47c5-9904-215de8582858
📒 Files selected for processing (6)
docs/DynamoDbTypedApi.mdsrc/Clients/Goa.Clients.Dynamo.Generator/CodeGeneration/JsonMapperGenerator.cssrc/Clients/Goa.Clients.Dynamo/DynamoExtensions.cssrc/Clients/Goa.Clients.Dynamo/DynamoServiceClient.cssrc/Clients/Goa.Clients.Dynamo/Internal/DynamoResponseReader.cstests/Clients/Goa.Clients.Dynamo.Tests/TypedExtensionTests.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/DynamoDbTypedApi.md
Resolves conflicts from PR #68 merge. Removes duplicate X-Amz-Target headers from all typed methods (the RequestSigner already adds it from HttpOptions.Target).
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/Operations/Query/QueryResult.cs (1)
25-29:⚠️ Potential issue | 🟠 MajorBind
Countfrom the response instead of recomputing it fromItems.
QuerysupportsSelect = COUNT, where DynamoDB returns the number of matching items rather than the items themselves. In that case,Countcan be non-zero whileItemsis empty or omitted, so both wrappers will report0here. Expose the wire field directly and, if useful, add a separateReturnedItemCount => Items.Count. (docs.aws.amazon.com)Suggested shape
- public int Count => Items.Count; + [JsonPropertyName("Count")] + public int Count { get; set; } + + [JsonIgnore] + public int ReturnedItemCount => Items.Count;- public int Count => Items.Count; + [JsonPropertyName("Count")] + public int Count { get; set; } + + [JsonIgnore] + public int ReturnedItemCount => Items.Count;Amazon DynamoDB Query API Select COUNT response Count ItemsAlso applies to: 62-63
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Clients/Goa.Clients.Dynamo/Operations/Query/QueryResult.cs` around lines 25 - 29, The Count property currently computes Items.Count, which is wrong for DynamoDB Query responses that use Select=COUNT; instead map/bind the wire response Count value directly (expose the response Count field as the Count property in QueryResult) and add a new ReturnedItemCount (or ReturnedItemsCount) property that returns Items.Count for callers who need the number of returned item objects; update the QueryResult.Count getter to return the deserialized response Count and add the new ReturnedItemCount => Items.Count, leaving ScannedCount unchanged.
♻️ Duplicate comments (3)
src/Clients/Goa.Clients.Dynamo/DynamoServiceClient.cs (2)
361-375:⚠️ Potential issue | 🟠 MajorReturn the enriched
Errorinstead of rebuilding it.After
ProcessAwsErrorHeadersruns, Lines 368-375 create a freshError.Failureand only copy payload/status metadata. Any metadata or categorization added during deserialization/header processing is dropped before it reaches callers.Suggested fix
- var errorType = error?.Type ?? error?.Code ?? "Unknown"; - var metadata = new Dictionary<string, object>(); - if (error?.Payload is not null) metadata["Payload"] = error.Payload; - if (error?.StatusCode is not null) metadata["StatusCode"] = error.StatusCode; - return Error.Failure( - code: MapErrorCodeToDynamo(errorType), - description: error?.Message ?? "An error occurred while processing the request.", - metadata: metadata); + if (error is not null) + { + var errorType = error.Type ?? error.Code ?? "Unknown"; + return error with { Code = MapErrorCodeToDynamo(errorType) }; + } + + return Error.Failure( + code: MapErrorCodeToDynamo("Unknown"), + description: "An error occurred while processing the request.");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Clients/Goa.Clients.Dynamo/DynamoServiceClient.cs` around lines 361 - 375, The code currently deserializes into error via DeserializeJsonError, then updates it with Payload/StatusCode and ProcessAwsErrorHeaders, but discards that enriched Error by creating a new Error.Failure; instead return the enriched error instance. After calling ProcessAwsErrorHeaders(error), set or override only the failure code/description fields if needed (use MapErrorCodeToDynamo(error.Type ?? error.Code ?? "Unknown") for the code and fallback description) but preserve error.Metadata and other fields produced by DeserializeJsonError/ProcessAwsErrorHeaders; then return that enriched Error (do not reconstruct a new Error.Failure that loses metadata).
266-282:⚠️ Potential issue | 🟠 MajorConstrain
There or return an explicit optional wrapper.
T?on an unconstrained generic is only a nullability annotation. Line 280 still returnsdefault(T), so a missing item is indistinguishable from a legitimate zero-value whenTis a struct. Either addwhere T : classand keep the nullable-reference contract, or switch this API plusDynamoResponseReader.ReadGetItemResponse<T>to an explicit optional result.Possible minimal direction
-public async Task<ErrorOr<T?>> GetItemAsync<T>(GetItemRequest request, DynamoItemReader<T> itemReader, CancellationToken cancellationToken = default) +public async Task<ErrorOr<T?>> GetItemAsync<T>(GetItemRequest request, DynamoItemReader<T> itemReader, CancellationToken cancellationToken = default) + where T : class🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Clients/Goa.Clients.Dynamo/DynamoServiceClient.cs` around lines 266 - 282, The generic result is currently nullable only by annotation so returning default(T) makes missing-item vs zero-value ambiguous for value types; change the API to constrain T to reference types (add “where T : class” to the GetItemAsync<T> signature and any other methods that call it) and keep ErrorOr<T?> as the nullable-reference contract, and update the call site to DynamoResponseReader.ReadGetItemResponse<T> to match the constrained type; alternatively, if you need value-type support, change the return to an explicit optional wrapper (e.g., ErrorOr<Optional<T>>) and adjust DynamoResponseReader.ReadGetItemResponse and default(T) handling accordingly.src/Clients/Goa.Clients.Dynamo/Operations/Query/QueryResult.cs (1)
43-47:⚠️ Potential issue | 🟠 MajorMap the non-generic consumed-capacity field to DynamoDB’s actual response shape.
DynamoDB
QueryreturnsConsumedCapacityas an object, withCapacityUnitsnested inside it; there is no top-levelConsumedCapacityUnitsresponse member. As written, the non-generic wrapper will deserialize this as the default0and drop the table/index breakdowns the API can return. (docs.aws.amazon.com)Suggested fix
- [JsonPropertyName("ConsumedCapacityUnits")] - public double ConsumedCapacityUnits { get; set; } + [JsonPropertyName("ConsumedCapacity")] + public ConsumedCapacity? ConsumedCapacity { get; set; } + + [JsonIgnore] + public double? ConsumedCapacityUnits => ConsumedCapacity?.CapacityUnits;Amazon DynamoDB Query API response ConsumedCapacity object CapacityUnits🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Clients/Goa.Clients.Dynamo/Operations/Query/QueryResult.cs` around lines 43 - 47, The ConsumedCapacityUnits property on QueryResult should be replaced with a property that maps to DynamoDB's ConsumedCapacity object so deserialization preserves CapacityUnits and table/index breakdowns; update the QueryResult class to remove or deprecate the double ConsumedCapacityUnits and add a public property like ConsumedCapacity of a new or existing ConsumedCapacity type (containing CapacityUnits, TableName, Table, LocalSecondaryIndexes, GlobalSecondaryIndexes, etc.), then adjust any consumers of QueryResult. Locate the QueryResult class and the ConsumedCapacityUnits member to implement the new ConsumedCapacity model and update tests/call sites accordingly.
🧹 Nitpick comments (2)
src/Clients/Goa.Clients.Dynamo/Models/AttributeValue.cs (1)
46-51: Consider addingJsonPropertyNametoBandBSfor consistency.All other DynamoDB attribute type properties (
S,N,BOOL,SS,NS,L,M,NULL) have explicitJsonPropertyNameattributes, butBandBSare missing them. While the property names already match DynamoDB's wire format, adding explicit attributes would be more consistent and resilient to any future changes in JSON serialization settings (e.g., naming policies).♻️ Proposed fix for consistency
/// <summary> /// An attribute of type Binary. DynamoDB sends binary data as a base64-encoded string. /// </summary> + [JsonPropertyName("B")] public byte[]? B { get; set; } /// <summary> /// An attribute of type Binary Set. A set of binary values, each base64-encoded. /// </summary> + [JsonPropertyName("BS")] public List<byte[]>? BS { get; set; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Clients/Goa.Clients.Dynamo/Models/AttributeValue.cs` around lines 46 - 51, The properties B and BS in AttributeValue are missing explicit JsonPropertyName attributes; add [JsonPropertyName("B")] above the byte[]? B property and [JsonPropertyName("BS")] above the List<byte[]>? BS property (ensuring System.Text.Json.Serialization is imported if not already) so their JSON names are explicit and consistent with the other DynamoDB attributes (S, N, BOOL, SS, NS, L, M, NULL).src/Clients/Goa.Clients.Dynamo/DynamoServiceClient.cs (1)
288-299:PutItemAsync<T>still pays for an extra body copy.Lines 288-297 buffer JSON into
ArrayBufferWriter<byte>and then immediately callToArray(). That makes this typed path allocate/copy once more than the other typed methods, which cuts against the perf goal of this PR. Worth switching this to the pooled request-buffer plumbing already available insrc/Clients/Goa.Clients.Core/Http/PooledBufferWriter.cs, or anotherCreateRequestMessagepath that can consume the written buffer directly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Clients/Goa.Clients.Dynamo/DynamoServiceClient.cs` around lines 288 - 299, The PutItemAsync<T> path is doing an unnecessary extra copy by writing JSON into System.Buffers.ArrayBufferWriter<byte> and then calling ToArray() before CreateRequestMessage; replace the ArrayBufferWriter usage with the project's pooled buffer plumbing (use the PooledBufferWriter from src/Clients/Goa.Clients.Core/Http/PooledBufferWriter.cs) or call a CreateRequestMessage overload that can consume the writer's buffer directly so you can pass the underlying Memory/IBuffer without ToArray(); update the code in PutItemAsync<T> to construct the Utf8JsonWriter over the pooled writer, flush and then hand the pooled buffer into CreateRequestMessage, disposing/returning the pooled writer as appropriate to avoid the extra allocation.
🤖 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/Operations/Query/QueryResult.cs`:
- Around line 25-29: The Count property currently computes Items.Count, which is
wrong for DynamoDB Query responses that use Select=COUNT; instead map/bind the
wire response Count value directly (expose the response Count field as the Count
property in QueryResult) and add a new ReturnedItemCount (or ReturnedItemsCount)
property that returns Items.Count for callers who need the number of returned
item objects; update the QueryResult.Count getter to return the deserialized
response Count and add the new ReturnedItemCount => Items.Count, leaving
ScannedCount unchanged.
---
Duplicate comments:
In `@src/Clients/Goa.Clients.Dynamo/DynamoServiceClient.cs`:
- Around line 361-375: The code currently deserializes into error via
DeserializeJsonError, then updates it with Payload/StatusCode and
ProcessAwsErrorHeaders, but discards that enriched Error by creating a new
Error.Failure; instead return the enriched error instance. After calling
ProcessAwsErrorHeaders(error), set or override only the failure code/description
fields if needed (use MapErrorCodeToDynamo(error.Type ?? error.Code ??
"Unknown") for the code and fallback description) but preserve error.Metadata
and other fields produced by DeserializeJsonError/ProcessAwsErrorHeaders; then
return that enriched Error (do not reconstruct a new Error.Failure that loses
metadata).
- Around line 266-282: The generic result is currently nullable only by
annotation so returning default(T) makes missing-item vs zero-value ambiguous
for value types; change the API to constrain T to reference types (add “where T
: class” to the GetItemAsync<T> signature and any other methods that call it)
and keep ErrorOr<T?> as the nullable-reference contract, and update the call
site to DynamoResponseReader.ReadGetItemResponse<T> to match the constrained
type; alternatively, if you need value-type support, change the return to an
explicit optional wrapper (e.g., ErrorOr<Optional<T>>) and adjust
DynamoResponseReader.ReadGetItemResponse and default(T) handling accordingly.
In `@src/Clients/Goa.Clients.Dynamo/Operations/Query/QueryResult.cs`:
- Around line 43-47: The ConsumedCapacityUnits property on QueryResult should be
replaced with a property that maps to DynamoDB's ConsumedCapacity object so
deserialization preserves CapacityUnits and table/index breakdowns; update the
QueryResult class to remove or deprecate the double ConsumedCapacityUnits and
add a public property like ConsumedCapacity of a new or existing
ConsumedCapacity type (containing CapacityUnits, TableName, Table,
LocalSecondaryIndexes, GlobalSecondaryIndexes, etc.), then adjust any consumers
of QueryResult. Locate the QueryResult class and the ConsumedCapacityUnits
member to implement the new ConsumedCapacity model and update tests/call sites
accordingly.
---
Nitpick comments:
In `@src/Clients/Goa.Clients.Dynamo/DynamoServiceClient.cs`:
- Around line 288-299: The PutItemAsync<T> path is doing an unnecessary extra
copy by writing JSON into System.Buffers.ArrayBufferWriter<byte> and then
calling ToArray() before CreateRequestMessage; replace the ArrayBufferWriter
usage with the project's pooled buffer plumbing (use the PooledBufferWriter from
src/Clients/Goa.Clients.Core/Http/PooledBufferWriter.cs) or call a
CreateRequestMessage overload that can consume the writer's buffer directly so
you can pass the underlying Memory/IBuffer without ToArray(); update the code in
PutItemAsync<T> to construct the Utf8JsonWriter over the pooled writer, flush
and then hand the pooled buffer into CreateRequestMessage, disposing/returning
the pooled writer as appropriate to avoid the extra allocation.
In `@src/Clients/Goa.Clients.Dynamo/Models/AttributeValue.cs`:
- Around line 46-51: The properties B and BS in AttributeValue are missing
explicit JsonPropertyName attributes; add [JsonPropertyName("B")] above the
byte[]? B property and [JsonPropertyName("BS")] above the List<byte[]>? BS
property (ensuring System.Text.Json.Serialization is imported if not already) so
their JSON names are explicit and consistent with the other DynamoDB attributes
(S, N, BOOL, SS, NS, L, M, NULL).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0f89761e-7f49-4658-a492-b7b91346cbda
📒 Files selected for processing (6)
src/Clients/Goa.Clients.Dynamo/DynamoServiceClient.cssrc/Clients/Goa.Clients.Dynamo/Goa.Clients.Dynamo.csprojsrc/Clients/Goa.Clients.Dynamo/Models/AttributeValue.cssrc/Clients/Goa.Clients.Dynamo/Models/ConsumedCapacity.cssrc/Clients/Goa.Clients.Dynamo/Operations/Query/QueryResult.cssrc/Clients/Goa.Clients.Dynamo/Operations/Scan/ScanResult.cs
🚧 Files skipped from review as they are similar to previous changes (3)
- src/Clients/Goa.Clients.Dynamo/Goa.Clients.Dynamo.csproj
- src/Clients/Goa.Clients.Dynamo/Models/ConsumedCapacity.cs
- src/Clients/Goa.Clients.Dynamo/Operations/Scan/ScanResult.cs
Owner
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (6)
src/Clients/Goa.Clients.Dynamo/Operations/DeleteItem/DeleteItemResponse.cs (1)
20-21:⚠️ Potential issue | 🟠 Major
ConsumedCapacityUnitsstill does not match the DeleteItem payload.This remains wired to a scalar
"ConsumedCapacityUnits"field, but DeleteItem returns"ConsumedCapacity"as an object. Real responses still will not deserialize into this property.#!/bin/bash set -euo pipefail echo "Check the AWS DeleteItem API reference for response members:" curl -Lfs https://docs.aws.amazon.com/amazondynamodb/latest/APIReference/API_DeleteItem.html \ | tr -d '\r' \ | grep -nE 'ConsumedCapacity|ConsumedCapacityUnits' \ | head -n 20 echo echo "Compare with response models in this repository:" fd '.*Response\.cs$' src/Clients/Goa.Clients.Dynamo | xargs rg -n --no-heading '\bConsumedCapacity\b|\bConsumedCapacityUnits\b'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Clients/Goa.Clients.Dynamo/Operations/DeleteItem/DeleteItemResponse.cs` around lines 20 - 21, The DeleteItemResponse currently expects a scalar ConsumedCapacityUnits (property ConsumedCapacityUnits) but AWS returns a nested ConsumedCapacity object; update the model so DeleteItemResponse exposes a ConsumedCapacity property of an appropriate type instead of ConsumedCapacityUnits: add a ConsumedCapacity class/record (e.g., ConsumedCapacity { public double? CapacityUnits { get; set; } } or similar matching the AWS payload field names) and change DeleteItemResponse to use public ConsumedCapacity ConsumedCapacity { get; set; } with JsonPropertyName("ConsumedCapacity"), then remove or deprecate the scalar ConsumedCapacityUnits property so responses will deserialize correctly.docs/DynamoDbTypedApi.md (1)
299-307:⚠️ Potential issue | 🟠 MajorInstantiate
TransactGetRequestat the top level.Only the inner
Getpayload should beTransactGetItemRequest. The outer object passed toTransactGetItemsAsyncneeds to be the transaction request type, otherwise this sample won't compile.Proposed fix
var result = await client.TransactGetItemsAsync( - new TransactGetItemRequest + new TransactGetRequest { TransactItems = new List<TransactGetItem> { new() { Get = new TransactGetItemRequest { TableName = "Users", Key = key1 } }, new() { Get = new TransactGetItemRequest { TableName = "Users", Key = key2 } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/DynamoDbTypedApi.md` around lines 299 - 307, The sample incorrectly passes a TransactGetItem (or wrong request type) as the top-level argument to TransactGetItemsAsync; instead create a TransactGetRequest (top-level transaction object) and set its TransactItems to a List<TransactGetItem>, where each TransactGetItem has its Get property set to a TransactGetItemRequest with TableName and Key. Update the code that calls TransactGetItemsAsync so the top-level object is TransactGetRequest and only the inner Get values are TransactGetItemRequest (referencing TransactGetItemsAsync, TransactGetRequest, TransactGetItem, and TransactGetItemRequest).docs/DynamoDbOptimisations.md (2)
161-161:⚠️ Potential issue | 🟡 MinorUpdate this page to the current typed API names and feature status.
The doc still refers to
DynamoJsonReader<T>/DynamoJsonWriter<T>and lists typedPutItemAsync<T>,ScanAsync<T>, andGetItemAsync<T>as future work, but the current surface already usesDynamoItemReader<T>/DynamoItemWriter<T>and ships those overloads.Also applies to: 183-186, 323-324, 351-352, 369-370
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/DynamoDbOptimisations.md` at line 161, Replace outdated API names and feature-status text: update every occurrence of DynamoJsonReader<T> and DynamoJsonWriter<T> to DynamoItemReader<T> and DynamoItemWriter<T>, and change any statements that list typed overloads PutItemAsync<T>, ScanAsync<T>, and GetItemAsync<T> as "future work" so they reflect that these typed overloads are already implemented and available; ensure related explanatory text and examples use the current typed API names (e.g., references inside code samples, headings, and callouts) and apply the same edits to the other occurrences noted in the comment.
7-11:⚠️ Potential issue | 🟡 MinorAdd
textlanguage tags to these fenced diagrams.These bare fences still trip markdownlint MD040 in this doc.
Also applies to: 48-57
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/DynamoDbOptimisations.md` around lines 7 - 11, The fenced diagram blocks like the one starting with "HTTP Response bytes" (and the similar block around "STJ deserializes to QueryResponse { List<DynamoRecord> }" further down) need explicit language tags to satisfy markdownlint MD040; change their opening fences from ``` to ```text for both occurrences (the block at "HTTP Response bytes -> STJ deserializes..." and the block in the 48-57 region) so the diagrams are fenced as ```text ... ``` .src/Clients/Goa.Clients.Dynamo/DynamoServiceClient.cs (2)
361-375:⚠️ Potential issue | 🟠 MajorPreserve the enriched error instead of rebuilding it.
Lines 364-365 enrich the deserialized error, but Lines 368-375 create a fresh
Error.Failureand only copyPayload/StatusCode. AnythingProcessAwsErrorHeadersadded is lost for the typed path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Clients/Goa.Clients.Dynamo/DynamoServiceClient.cs` around lines 361 - 375, The deserialized error returned by DeserializeJsonError is enriched (mutated) and further processed by ProcessAwsErrorHeaders, but the code then constructs a new Error.Failure losing any additional fields added earlier; instead reuse the enriched `error` instance when creating the failure: build the metadata from the enriched `error` (including everything ProcessAwsErrorHeaders might have added) and create the Error.Failure using `error`'s properties (Type/Code/Message/Payload/StatusCode) and MapErrorCodeToDynamo(error.Type ?? error.Code ?? "Unknown") so no processed header data is dropped; update the block around DeserializeJsonError / ProcessAwsErrorHeaders to return a failure based on the enriched `error` rather than reconstructing it.
266-282:⚠️ Potential issue | 🟠 MajorConstrain
Tor make “item not found” explicit.
T?on an unconstrained generic is only a nullability annotation, so Line 280 still returnsdefault(T)for a missing item. That makes a missing value-type item indistinguishable from a real0/false/default struct.In C#, for an unconstrained generic return type, what does `T?` mean? Does it become `Nullable<T>` for value types, or is it only a nullability annotation?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Clients/Goa.Clients.Dynamo/DynamoServiceClient.cs` around lines 266 - 282, The method GetItemAsync<T> currently returns T? for an unconstrained T, which makes “not found” indistinguishable from a default value for value types; fix by constraining T to reference types (add "where T : class" to the GetItemAsync<T> signature) so that the empty-response branch (the buffer.Length == 0 return default(T)) returns null explicitly for “item not found”; update any callers/tests if they relied on value-type returns or change callers to use a separate nullable/value wrapper if value types must be supported.
🧹 Nitpick comments (5)
src/Clients/Goa.Clients.Dynamo/Operations/Scan/ScanResult.cs (1)
54-75: Consider addingJsonPropertyNameattributes to genericScanResult<T>for consistency.The non-generic
ScanResultnow has explicitJsonPropertyNameattributes on all serialized properties, but the genericScanResult<T>lacks them. If this class is used for JSON deserialization (which seems likely given the typed API additions in this PR), consider adding matching attributes:[JsonPropertyName("Items")] public List<T> Items { get; set; } = new(); [JsonPropertyName("LastEvaluatedKey")] public Dictionary<string, AttributeValue>? LastEvaluatedKey { get; set; } [JsonPropertyName("ScannedCount")] public int ScannedCount { get; set; } [JsonPropertyName("ConsumedCapacity")] public ConsumedCapacity? ConsumedCapacity { get; set; }This ensures consistent deserialization behavior regardless of
JsonSerializerOptions.PropertyNamingPolicysettings.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Clients/Goa.Clients.Dynamo/Operations/Scan/ScanResult.cs` around lines 54 - 75, Add JsonPropertyName attributes on the generic ScanResult<T> properties to match the non-generic ScanResult: annotate Items, LastEvaluatedKey, ScannedCount and ConsumedCapacity with [JsonPropertyName("...")] using the exact JSON names ("Items", "LastEvaluatedKey", "ScannedCount", "ConsumedCapacity") so JSON serialization/deserialization of ScanResult<T> (class ScanResult<T>, properties Items, LastEvaluatedKey, ScannedCount, ConsumedCapacity) behaves consistently regardless of JsonSerializerOptions.PropertyNamingPolicy.src/Clients/Goa.Clients.Dynamo/Operations/Transactions/TransactWriteItemResponse.cs (1)
20-21:ItemCollectionMetricstype is less specific than other response classes.This property uses
Dictionary<string, object>?whileUpdateItemResponse.ItemCollectionMetricsusesDictionary<string, List<ItemCollectionMetrics>>?. The loose typing here loses type safety and may cause deserialization issues.♻️ Suggested type alignment
- [JsonPropertyName("ItemCollectionMetrics")] - public Dictionary<string, object>? ItemCollectionMetrics { get; set; } + [JsonPropertyName("ItemCollectionMetrics")] + public Dictionary<string, List<ItemCollectionMetrics>>? ItemCollectionMetrics { get; set; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Clients/Goa.Clients.Dynamo/Operations/Transactions/TransactWriteItemResponse.cs` around lines 20 - 21, The ItemCollectionMetrics property on TransactWriteItemResponse is too loosely typed (Dictionary<string, object>?) causing loss of type safety and potential deserialization problems; update the property type to match other response classes by using Dictionary<string, List<ItemCollectionMetrics>>? (or the existing ItemCollectionMetrics model type used by UpdateItemResponse) and adjust any related using/imports and serializers to ensure correct deserialization for the ItemCollectionMetrics property.src/Clients/Goa.Clients.Dynamo/DynamoItemWriterRegistry.cs (1)
24-27: Consider marking theWriterfield asvolatile.The
Cache<T>.Writerfield is accessed across threads (registration at startup, retrieval at runtime). Withoutvolatile, there's a theoretical visibility issue where a thread callingGet<T>()might read a stalenulleven after another thread completedRegister<T>().If registration is guaranteed to complete before any
Get<T>()calls (e.g., via module initializers), this may be acceptable. Otherwise, considervolatilefor correctness.♻️ Suggested fix
private static class Cache<T> { - public static DynamoItemWriter<T>? Writer; + public static volatile DynamoItemWriter<T>? Writer; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Clients/Goa.Clients.Dynamo/DynamoItemWriterRegistry.cs` around lines 24 - 27, Mark the Cache<T>.Writer field as volatile to ensure writes in Register<T>() are immediately visible to other threads reading it in Get<T>(); update the declaration of Cache<T>.Writer to use the C# volatile keyword for the static field (preserving its nullable type), so the runtime guarantees visibility across threads and avoids stale null reads when registration and retrieval happen concurrently.src/Clients/Goa.Clients.Dynamo/DynamoItemReaderRegistry.cs (1)
24-27: Consider marking theReaderfield asvolatile.Same consideration as
DynamoItemWriterRegistry: withoutvolatile, there's a theoretical visibility issue for cross-thread access.♻️ Suggested fix
private static class Cache<T> { - public static DynamoItemReader<T>? Reader; + public static volatile DynamoItemReader<T>? Reader; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Clients/Goa.Clients.Dynamo/DynamoItemReaderRegistry.cs` around lines 24 - 27, Mark the static cache field as volatile to ensure cross-thread visibility: update the nested generic Cache<T> by changing the declaration of the static field DynamoItemReader<T>? Reader to be volatile (i.e., declare Reader as volatile) so assignments to Cache<T>.Reader are immediately visible to other threads; update any similar patterns if present to match the same memory-visibility semantics used in DynamoItemWriterRegistry.src/Clients/Goa.Clients.Dynamo/Models/ConsumedCapacity.cs (1)
37-42: MissingJsonPropertyNameattributes on index properties.
GlobalSecondaryIndexesandLocalSecondaryIndexeslackJsonPropertyNameattributes while all other properties in this class have them. For consistency and resilience against naming policies, consider adding them.♻️ Suggested addition
/// <summary> /// The capacity consumed by the global secondary indexes affected by the operation. /// </summary> + [JsonPropertyName("GlobalSecondaryIndexes")] public Dictionary<string, CapacityDetail>? GlobalSecondaryIndexes { get; set; } /// <summary> /// The capacity consumed by the local secondary indexes affected by the operation. /// </summary> + [JsonPropertyName("LocalSecondaryIndexes")] public Dictionary<string, CapacityDetail>? LocalSecondaryIndexes { get; set; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Clients/Goa.Clients.Dynamo/Models/ConsumedCapacity.cs` around lines 37 - 42, ConsumedCapacity has inconsistent JSON attributes: add JsonPropertyName attributes to the GlobalSecondaryIndexes and LocalSecondaryIndexes properties so they match the rest of the class and are resilient to naming policies; in the ConsumedCapacity class annotate the GlobalSecondaryIndexes property with JsonPropertyName("GlobalSecondaryIndexes") and the LocalSecondaryIndexes property with JsonPropertyName("LocalSecondaryIndexes") (ensure the System.Text.Json.Serialization attribute namespace is available in the file).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Clients/Goa.Clients.Dynamo/DynamoExtensions.cs`:
- Around line 350-356: The retry loop currently resubmits
result.Value.UnprocessedKeys immediately; modify the loop that checks
result.Value.HasUnprocessedKeys to perform exponential backoff with jitter
before retrying: introduce an attempt counter (e.g., attempts or retryCount) in
the method containing the RequestItems/UnprocessedKeys logic, compute a delay
like min(baseDelay * 2^attempts, maxDelay) with randomized jitter, await
Task.Delay(theDelay) before setting request.RequestItems =
result.Value.UnprocessedKeys and retrying, and reset or cap attempts when
UnprocessedKeys becomes empty or a maxRetries limit is reached to avoid infinite
loops; ensure this change is applied around the code that references
request.RequestItems, result.Value.UnprocessedKeys, and
result.Value.HasUnprocessedKeys.
In `@src/Clients/Goa.Clients.Dynamo/Operations/DeleteItem/DeleteItemResponse.cs`:
- Around line 26-27: The ItemCollectionMetrics property on DeleteItemResponse is
typed incorrectly as Dictionary<string, List<ItemCollectionMetrics>>?; change it
to use the existing ItemCollectionMetrics model type (ItemCollectionMetrics?) so
it matches the single-object shape returned by DeleteItem and can deserialize
correctly; update the property declaration for ItemCollectionMetrics in the
DeleteItemResponse class to be of type ItemCollectionMetrics? while keeping the
[JsonPropertyName("ItemCollectionMetrics")] attribute.
In `@src/Clients/Goa.Clients.Dynamo/Operations/PutItem/PutItemResponse.cs`:
- Around line 27-28: The ItemCollectionMetrics property on PutItemResponse is
typed as Dictionary<string, List<Dictionary<string, AttributeValue>>>? but
PutItem returns a single ItemCollectionMetrics object; change the property type
on the PutItemResponse class (property name ItemCollectionMetrics) to the
project's ItemCollectionMetrics model type (ItemCollectionMetrics?), update any
using/import if necessary so it references the ItemCollectionMetrics model
class, keep the JsonPropertyName("ItemCollectionMetrics") attribute, and
run/build tests to ensure the response binds correctly.
In `@src/Clients/Goa.Clients.Dynamo/Operations/UpdateItem/UpdateItemResponse.cs`:
- Around line 20-21: Update the UpdateItemResponse class to accept DynamoDB's
actual capacity payload by adding a ConsumedCapacity property: add a nullable
ConsumedCapacity? property named ConsumedCapacity decorated with
[JsonPropertyName("ConsumedCapacity")] (matching the pattern used in
GetItemResponse) so responses deserialize correctly; remove or keep the existing
double? ConsumedCapacityUnits property only if you need backward compatibility,
but ensure ConsumedCapacity is present and correctly typed.
---
Duplicate comments:
In `@docs/DynamoDbOptimisations.md`:
- Line 161: Replace outdated API names and feature-status text: update every
occurrence of DynamoJsonReader<T> and DynamoJsonWriter<T> to DynamoItemReader<T>
and DynamoItemWriter<T>, and change any statements that list typed overloads
PutItemAsync<T>, ScanAsync<T>, and GetItemAsync<T> as "future work" so they
reflect that these typed overloads are already implemented and available; ensure
related explanatory text and examples use the current typed API names (e.g.,
references inside code samples, headings, and callouts) and apply the same edits
to the other occurrences noted in the comment.
- Around line 7-11: The fenced diagram blocks like the one starting with "HTTP
Response bytes" (and the similar block around "STJ deserializes to QueryResponse
{ List<DynamoRecord> }" further down) need explicit language tags to satisfy
markdownlint MD040; change their opening fences from ``` to ```text for both
occurrences (the block at "HTTP Response bytes -> STJ deserializes..." and the
block in the 48-57 region) so the diagrams are fenced as ```text ... ``` .
In `@docs/DynamoDbTypedApi.md`:
- Around line 299-307: The sample incorrectly passes a TransactGetItem (or wrong
request type) as the top-level argument to TransactGetItemsAsync; instead create
a TransactGetRequest (top-level transaction object) and set its TransactItems to
a List<TransactGetItem>, where each TransactGetItem has its Get property set to
a TransactGetItemRequest with TableName and Key. Update the code that calls
TransactGetItemsAsync so the top-level object is TransactGetRequest and only the
inner Get values are TransactGetItemRequest (referencing TransactGetItemsAsync,
TransactGetRequest, TransactGetItem, and TransactGetItemRequest).
In `@src/Clients/Goa.Clients.Dynamo/DynamoServiceClient.cs`:
- Around line 361-375: The deserialized error returned by DeserializeJsonError
is enriched (mutated) and further processed by ProcessAwsErrorHeaders, but the
code then constructs a new Error.Failure losing any additional fields added
earlier; instead reuse the enriched `error` instance when creating the failure:
build the metadata from the enriched `error` (including everything
ProcessAwsErrorHeaders might have added) and create the Error.Failure using
`error`'s properties (Type/Code/Message/Payload/StatusCode) and
MapErrorCodeToDynamo(error.Type ?? error.Code ?? "Unknown") so no processed
header data is dropped; update the block around DeserializeJsonError /
ProcessAwsErrorHeaders to return a failure based on the enriched `error` rather
than reconstructing it.
- Around line 266-282: The method GetItemAsync<T> currently returns T? for an
unconstrained T, which makes “not found” indistinguishable from a default value
for value types; fix by constraining T to reference types (add "where T : class"
to the GetItemAsync<T> signature) so that the empty-response branch (the
buffer.Length == 0 return default(T)) returns null explicitly for “item not
found”; update any callers/tests if they relied on value-type returns or change
callers to use a separate nullable/value wrapper if value types must be
supported.
In `@src/Clients/Goa.Clients.Dynamo/Operations/DeleteItem/DeleteItemResponse.cs`:
- Around line 20-21: The DeleteItemResponse currently expects a scalar
ConsumedCapacityUnits (property ConsumedCapacityUnits) but AWS returns a nested
ConsumedCapacity object; update the model so DeleteItemResponse exposes a
ConsumedCapacity property of an appropriate type instead of
ConsumedCapacityUnits: add a ConsumedCapacity class/record (e.g.,
ConsumedCapacity { public double? CapacityUnits { get; set; } } or similar
matching the AWS payload field names) and change DeleteItemResponse to use
public ConsumedCapacity ConsumedCapacity { get; set; } with
JsonPropertyName("ConsumedCapacity"), then remove or deprecate the scalar
ConsumedCapacityUnits property so responses will deserialize correctly.
---
Nitpick comments:
In `@src/Clients/Goa.Clients.Dynamo/DynamoItemReaderRegistry.cs`:
- Around line 24-27: Mark the static cache field as volatile to ensure
cross-thread visibility: update the nested generic Cache<T> by changing the
declaration of the static field DynamoItemReader<T>? Reader to be volatile
(i.e., declare Reader as volatile) so assignments to Cache<T>.Reader are
immediately visible to other threads; update any similar patterns if present to
match the same memory-visibility semantics used in DynamoItemWriterRegistry.
In `@src/Clients/Goa.Clients.Dynamo/DynamoItemWriterRegistry.cs`:
- Around line 24-27: Mark the Cache<T>.Writer field as volatile to ensure writes
in Register<T>() are immediately visible to other threads reading it in
Get<T>(); update the declaration of Cache<T>.Writer to use the C# volatile
keyword for the static field (preserving its nullable type), so the runtime
guarantees visibility across threads and avoids stale null reads when
registration and retrieval happen concurrently.
In `@src/Clients/Goa.Clients.Dynamo/Models/ConsumedCapacity.cs`:
- Around line 37-42: ConsumedCapacity has inconsistent JSON attributes: add
JsonPropertyName attributes to the GlobalSecondaryIndexes and
LocalSecondaryIndexes properties so they match the rest of the class and are
resilient to naming policies; in the ConsumedCapacity class annotate the
GlobalSecondaryIndexes property with JsonPropertyName("GlobalSecondaryIndexes")
and the LocalSecondaryIndexes property with
JsonPropertyName("LocalSecondaryIndexes") (ensure the
System.Text.Json.Serialization attribute namespace is available in the file).
In `@src/Clients/Goa.Clients.Dynamo/Operations/Scan/ScanResult.cs`:
- Around line 54-75: Add JsonPropertyName attributes on the generic
ScanResult<T> properties to match the non-generic ScanResult: annotate Items,
LastEvaluatedKey, ScannedCount and ConsumedCapacity with
[JsonPropertyName("...")] using the exact JSON names ("Items",
"LastEvaluatedKey", "ScannedCount", "ConsumedCapacity") so JSON
serialization/deserialization of ScanResult<T> (class ScanResult<T>, properties
Items, LastEvaluatedKey, ScannedCount, ConsumedCapacity) behaves consistently
regardless of JsonSerializerOptions.PropertyNamingPolicy.
In
`@src/Clients/Goa.Clients.Dynamo/Operations/Transactions/TransactWriteItemResponse.cs`:
- Around line 20-21: The ItemCollectionMetrics property on
TransactWriteItemResponse is too loosely typed (Dictionary<string, object>?)
causing loss of type safety and potential deserialization problems; update the
property type to match other response classes by using Dictionary<string,
List<ItemCollectionMetrics>>? (or the existing ItemCollectionMetrics model type
used by UpdateItemResponse) and adjust any related using/imports and serializers
to ensure correct deserialization for the ItemCollectionMetrics property.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cf4397c7-6fb9-4624-943b-d47c3d8650a8
📒 Files selected for processing (58)
docs/DynamoDbOptimisations.mddocs/DynamoDbTypedApi.mdsrc/Clients/Goa.Clients.Dynamo.Generator/Attributes/DynamoConverterAttributeHandler.cssrc/Clients/Goa.Clients.Dynamo.Generator/CodeGeneration/JsonMapperGenerator.cssrc/Clients/Goa.Clients.Dynamo.Generator/CodeGeneration/ReaderRegistrationGenerator.cssrc/Clients/Goa.Clients.Dynamo.Generator/DynamoMapperIncrementalGenerator.cssrc/Clients/Goa.Clients.Dynamo.Generator/Models/DynamoConverterAttributeInfo.cssrc/Clients/Goa.Clients.Dynamo.Generator/Models/PropertyInfo.cssrc/Clients/Goa.Clients.Dynamo/DynamoExtensions.cssrc/Clients/Goa.Clients.Dynamo/DynamoItemReaderRegistry.cssrc/Clients/Goa.Clients.Dynamo/DynamoItemWriterRegistry.cssrc/Clients/Goa.Clients.Dynamo/DynamoServiceClient.cssrc/Clients/Goa.Clients.Dynamo/Goa.Clients.Dynamo.csprojsrc/Clients/Goa.Clients.Dynamo/IDynamoClient.cssrc/Clients/Goa.Clients.Dynamo/Models/AttributeValue.cssrc/Clients/Goa.Clients.Dynamo/Models/CapacityDetail.cssrc/Clients/Goa.Clients.Dynamo/Models/ConsumedCapacity.cssrc/Clients/Goa.Clients.Dynamo/Models/ItemCollectionMetrics.cssrc/Clients/Goa.Clients.Dynamo/Operations/Batch/BatchGetItemRequest.cssrc/Clients/Goa.Clients.Dynamo/Operations/Batch/BatchGetItemResponse.cssrc/Clients/Goa.Clients.Dynamo/Operations/Batch/BatchGetRequestItem.cssrc/Clients/Goa.Clients.Dynamo/Operations/Batch/BatchGetResult.cssrc/Clients/Goa.Clients.Dynamo/Operations/Batch/BatchWriteItemRequest.cssrc/Clients/Goa.Clients.Dynamo/Operations/Batch/BatchWriteItemResponse.cssrc/Clients/Goa.Clients.Dynamo/Operations/Batch/BatchWriteRequestItem.cssrc/Clients/Goa.Clients.Dynamo/Operations/Batch/BatchWriteResult.cssrc/Clients/Goa.Clients.Dynamo/Operations/Batch/DeleteRequest.cssrc/Clients/Goa.Clients.Dynamo/Operations/Batch/PutRequest.cssrc/Clients/Goa.Clients.Dynamo/Operations/DeleteItem/DeleteItemRequest.cssrc/Clients/Goa.Clients.Dynamo/Operations/DeleteItem/DeleteItemResponse.cssrc/Clients/Goa.Clients.Dynamo/Operations/GetItem/GetItemRequest.cssrc/Clients/Goa.Clients.Dynamo/Operations/GetItem/GetItemResponse.cssrc/Clients/Goa.Clients.Dynamo/Operations/PutItem/PutItemRequest.cssrc/Clients/Goa.Clients.Dynamo/Operations/PutItem/PutItemResponse.cssrc/Clients/Goa.Clients.Dynamo/Operations/Query/QueryRequest.cssrc/Clients/Goa.Clients.Dynamo/Operations/Query/QueryResponse.cssrc/Clients/Goa.Clients.Dynamo/Operations/Query/QueryResult.cssrc/Clients/Goa.Clients.Dynamo/Operations/Scan/ScanRequest.cssrc/Clients/Goa.Clients.Dynamo/Operations/Scan/ScanResponse.cssrc/Clients/Goa.Clients.Dynamo/Operations/Scan/ScanResult.cssrc/Clients/Goa.Clients.Dynamo/Operations/Transactions/TransactConditionCheckItem.cssrc/Clients/Goa.Clients.Dynamo/Operations/Transactions/TransactDeleteItem.cssrc/Clients/Goa.Clients.Dynamo/Operations/Transactions/TransactGetItem.cssrc/Clients/Goa.Clients.Dynamo/Operations/Transactions/TransactGetItemRequest.cssrc/Clients/Goa.Clients.Dynamo/Operations/Transactions/TransactGetItemResponse.cssrc/Clients/Goa.Clients.Dynamo/Operations/Transactions/TransactGetRequest.cssrc/Clients/Goa.Clients.Dynamo/Operations/Transactions/TransactGetResult.cssrc/Clients/Goa.Clients.Dynamo/Operations/Transactions/TransactPutItem.cssrc/Clients/Goa.Clients.Dynamo/Operations/Transactions/TransactUpdateItem.cssrc/Clients/Goa.Clients.Dynamo/Operations/Transactions/TransactWriteItem.cssrc/Clients/Goa.Clients.Dynamo/Operations/Transactions/TransactWriteItemResponse.cssrc/Clients/Goa.Clients.Dynamo/Operations/Transactions/TransactWriteOperation.cssrc/Clients/Goa.Clients.Dynamo/Operations/Transactions/TransactWriteRequest.cssrc/Clients/Goa.Clients.Dynamo/Operations/UpdateItem/UpdateItemRequest.cssrc/Clients/Goa.Clients.Dynamo/Operations/UpdateItem/UpdateItemResponse.cstests/Clients/Goa.Clients.Dynamo.Tests/BuilderChainingTests.cstests/Clients/Goa.Clients.Dynamo.Tests/DynamoItemWriterRegistryTests.cstests/Clients/Goa.Clients.Dynamo.Tests/TypedExtensionTests.cs
💤 Files with no reviewable changes (1)
- src/Clients/Goa.Clients.Dynamo/Operations/Batch/BatchGetResult.cs
These documentation files are no longer needed.
The write path emits {"NULL":true} for runtime-null non-nullable strings,
but the read path would call reader.GetString() on a boolean token. Now
checks for NULL descriptor and assigns string.Empty as default.
The error was enriched with Payload/StatusCode then immediately rebuilt from scratch extracting those same fields. Simplified to build metadata directly from the available variables.
PutItemResponse.ItemCollectionMetrics used raw dictionary instead of the ItemCollectionMetrics model. QueryResult and ScanResult used ConsumedCapacityUnits (double) which never matched the DynamoDB wire format; aligned to ConsumedCapacity object type.
Both BatchGetAllAsync overloads immediately retried unprocessed keys with no delay. Now uses exponential backoff (100ms base, 25.6s cap) with randomized jitter and a max of 10 retry attempts.
Single-item ops (Delete/Put/Update) use ItemCollectionMetrics? and ConsumedCapacity?. TransactWriteItemResponse uses Dictionary<string, List<ItemCollectionMetrics>>? for batch shape.
Without where T : class, default(T) for value types is indistinguishable from a valid result, making "not found" undetectable.
Without volatile, JIT/CPU could cache stale null reads of Reader/Writer fields when registration and retrieval happen on different threads.
…ence ConsumedCapacity lacked attributes on GlobalSecondaryIndexes and LocalSecondaryIndexes. QueryResult<T> and ScanResult<T> lacked attributes on all properties, making them naming-policy dependent.
Owner
Author
|
@coderabbitai resume |
Contributor
✅ Actions performedReviews resumed. |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/Clients/Goa.Clients.Dynamo/DynamoItemReaderRegistry.cs (1)
24-27: Addvolatilemodifier for thread-safety consistency.The
DynamoItemWriterRegistry.Cache<T>.Writerfield usesvolatileto ensure visibility across threads, butReaderhere does not. In multi-threaded scenarios (e.g., ASP.NET Core startup with parallel module initialization), a thread callingGet<T>might see a stalenulleven after another thread calledRegister<T>.Proposed fix
private static class Cache<T> { - public static DynamoItemReader<T>? Reader; + public static volatile DynamoItemReader<T>? Reader; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Clients/Goa.Clients.Dynamo/DynamoItemReaderRegistry.cs` around lines 24 - 27, The Reader field on the nested generic class Cache<T> is missing the volatile modifier causing potential visibility issues; update the declaration of DynamoItemReaderRegistry.Cache<T>.Reader to be public static volatile DynamoItemReader<T>? Reader so it matches the Writer field and ensures cross-thread visibility when Register<T> and Get<T> are called concurrently.src/Clients/Goa.Clients.Dynamo/DynamoExtensions.cs (1)
294-335: Consider extracting shared BatchGet retry logic to avoid drift.
BatchGetAllAsyncandBatchGetAllAsync<T>now duplicate nearly identical retry/pagination code paths. A shared internal helper would reduce maintenance risk when retry policy changes.Also applies to: 340-383
🤖 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 294 - 335, BatchGetAllAsync and BatchGetAllAsync<T> duplicate the same retry/pagination loop; extract that logic into a single internal helper (e.g., ExecuteBatchGetWithRetriesAsync or ProcessBatchGetWithRetries) that accepts the built request (or a BatchGetItemBuilder/BatchGetItemRequest) plus the Dynamo client and CancellationToken, contains the loop that calls client.BatchGetItemAsync, throws DynamoPaginationException on result.IsError, yields KeyValuePair<string, DynamoRecord> for each returned item, handles UnprocessedKeys, enforces maxAttempts and exponential backoff with jitter, and returns an IAsyncEnumerable or a reusable result structure; then have both BatchGetAllAsync and BatchGetAllAsync<T> call this helper instead of duplicating the loop so any future retry policy change (maxAttempts, backoff calculation, error handling) is updated in one place.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/Clients/Goa.Clients.Dynamo/DynamoExtensions.cs`:
- Around line 294-335: BatchGetAllAsync and BatchGetAllAsync<T> duplicate the
same retry/pagination loop; extract that logic into a single internal helper
(e.g., ExecuteBatchGetWithRetriesAsync or ProcessBatchGetWithRetries) that
accepts the built request (or a BatchGetItemBuilder/BatchGetItemRequest) plus
the Dynamo client and CancellationToken, contains the loop that calls
client.BatchGetItemAsync, throws DynamoPaginationException on result.IsError,
yields KeyValuePair<string, DynamoRecord> for each returned item, handles
UnprocessedKeys, enforces maxAttempts and exponential backoff with jitter, and
returns an IAsyncEnumerable or a reusable result structure; then have both
BatchGetAllAsync and BatchGetAllAsync<T> call this helper instead of duplicating
the loop so any future retry policy change (maxAttempts, backoff calculation,
error handling) is updated in one place.
In `@src/Clients/Goa.Clients.Dynamo/DynamoItemReaderRegistry.cs`:
- Around line 24-27: The Reader field on the nested generic class Cache<T> is
missing the volatile modifier causing potential visibility issues; update the
declaration of DynamoItemReaderRegistry.Cache<T>.Reader to be public static
volatile DynamoItemReader<T>? Reader so it matches the Writer field and ensures
cross-thread visibility when Register<T> and Get<T> are called concurrently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: da526bdd-a494-435b-a9ec-655c38d40716
📒 Files selected for processing (13)
src/Clients/Goa.Clients.Dynamo.Generator/CodeGeneration/JsonMapperGenerator.cssrc/Clients/Goa.Clients.Dynamo/DynamoExtensions.cssrc/Clients/Goa.Clients.Dynamo/DynamoItemReaderRegistry.cssrc/Clients/Goa.Clients.Dynamo/DynamoItemWriterRegistry.cssrc/Clients/Goa.Clients.Dynamo/DynamoServiceClient.cssrc/Clients/Goa.Clients.Dynamo/IDynamoClient.cssrc/Clients/Goa.Clients.Dynamo/Models/ConsumedCapacity.cssrc/Clients/Goa.Clients.Dynamo/Operations/DeleteItem/DeleteItemResponse.cssrc/Clients/Goa.Clients.Dynamo/Operations/PutItem/PutItemResponse.cssrc/Clients/Goa.Clients.Dynamo/Operations/Query/QueryResult.cssrc/Clients/Goa.Clients.Dynamo/Operations/Scan/ScanResult.cssrc/Clients/Goa.Clients.Dynamo/Operations/Transactions/TransactWriteItemResponse.cssrc/Clients/Goa.Clients.Dynamo/Operations/UpdateItem/UpdateItemResponse.cs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Clients/Goa.Clients.Dynamo/Models/ConsumedCapacity.cs
- src/Clients/Goa.Clients.Dynamo/DynamoItemWriterRegistry.cs
Im5tu
added a commit
that referenced
this pull request
Mar 12, 2026
…rison Only the 1-item benchmarks were active, giving misleading perf numbers. Typed Goa benchmark stays commented until PR #69 merges.
# Conflicts: # src/Clients/Goa.Clients.Dynamo/Models/AttributeValue.cs
AttributeValue is now a readonly struct after PR #70 merge; property setters no longer exist.
Add BenchmarkItem model with DynamoModel source generator and uncomment the Goa_Query_100Items_Typed benchmark for apples-to-apples comparison against existing benchmark methods.
…utes Single class now carries both EfficientDynamoDb and Goa attributes, eliminating the separate BenchmarkItem model file.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/Clients/Goa.Clients.Dynamo.Benchmarks/Benchmarks/QueryAsyncBenchmarks.cs (1)
195-205: Hoist the reader lookup out of the pagination loop.
DynamoItemReaderRegistry.Get<BenchmarkEntity>()is constant for the whole benchmark run. Resolving it per page adds registry-access overhead to the measurement, so the typed benchmark is no longer measuring just query/pagination cost.♻️ Suggested cleanup
public async Task<int> Goa_Query_100Items_Typed() { var count = 0; Dictionary<string, GoaModels.AttributeValue>? lastKey = null; + var reader = DynamoItemReaderRegistry.Get<BenchmarkEntity>(); do { var result = await _fixture.GoaClient.QueryAsync<BenchmarkEntity>(new GoaQueryRequest { TableName = _fixture.TableName, @@ }, Limit = 25, ExclusiveStartKey = lastKey - }, DynamoItemReaderRegistry.Get<BenchmarkEntity>()); + }, reader); count += result.Value.Items.Count; lastKey = result.Value.HasMoreResults ? result.Value.LastEvaluatedKey : null; } while (lastKey != null); return count; }Also applies to: 304-321, 425-434
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Clients/Goa.Clients.Dynamo.Benchmarks/Benchmarks/QueryAsyncBenchmarks.cs` around lines 195 - 205, The benchmark is calling DynamoItemReaderRegistry.Get<BenchmarkEntity>() inside the pagination loop which adds registry lookup overhead to each page; hoist that call out of the loop by resolving var reader = DynamoItemReaderRegistry.Get<BenchmarkEntity>() once before the pagination begins and then pass that reader variable into the QueryAsync<BenchmarkEntity>(...) calls (the same reader should replace the inline call used where ExclusiveStartKey/lastKey is used); apply the same change for the other loops referenced (around lines 304-321 and 425-434) so the registry lookup is done once per benchmark run instead of per page.tests/Clients/Goa.Clients.Dynamo.Tests/TypedExtensionTests.cs (1)
14-15: Tighten the typed paginator matchers.These setups accept any reader delegate and treat any non-null
ExclusiveStartKeyas “page 2”. That means the tests still pass if the typed overload stops forwardingTestReader, or if it uses the wrong continuation key. I’d assert the expected"pk"fromlastKeyand match/captureTestReaderexplicitly so the typed pagination contract is actually covered.Example tightening
- mock.Setup(c => c.QueryAsync(It.Is<QueryRequest>(r => r.ExclusiveStartKey != null), It.IsAny<DynamoItemReader<TestModel>>(), It.IsAny<CancellationToken>())) + mock.Setup(c => c.QueryAsync( + It.Is<QueryRequest>(r => + r.ExclusiveStartKey is not null && + r.ExclusiveStartKey.TryGetValue("pk", out var pk) && + pk.S == "lastPk"), + It.Is<DynamoItemReader<TestModel>>(reader => reader == TestReader), + It.IsAny<CancellationToken>())) .ReturnsAsync(secondPage);Apply the same tightening to the
ScanAsyncand error-path setups.Also applies to: 37-40, 70-73, 99-102
🤖 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 14 - 15, The typed paginator tests are too permissive: the setups accept any reader delegate and treat any non-null ExclusiveStartKey as page 2, so they don't verify that the typed overload forwards the exact DynamoItemReader<TestModel> TestReader or that the continuation key contains the expected "pk". Update the mock setups for the typed pagination and ScanAsync/error-path variants to explicitly match and capture the TestReader delegate (ensure the mock matcher compares the delegate identity) and tighten the ExclusiveStartKey matcher to assert the expected lastKey["pk"] value instead of just non-null; then add assertions that the captured reader equals TestReader and that the continuation key's "pk" equals the expected string so the typed pagination contract is actually exercised.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@tests/Clients/Goa.Clients.Dynamo.Benchmarks/Benchmarks/QueryAsyncBenchmarks.cs`:
- Around line 195-205: The benchmark is calling
DynamoItemReaderRegistry.Get<BenchmarkEntity>() inside the pagination loop which
adds registry lookup overhead to each page; hoist that call out of the loop by
resolving var reader = DynamoItemReaderRegistry.Get<BenchmarkEntity>() once
before the pagination begins and then pass that reader variable into the
QueryAsync<BenchmarkEntity>(...) calls (the same reader should replace the
inline call used where ExclusiveStartKey/lastKey is used); apply the same change
for the other loops referenced (around lines 304-321 and 425-434) so the
registry lookup is done once per benchmark run instead of per page.
In `@tests/Clients/Goa.Clients.Dynamo.Tests/TypedExtensionTests.cs`:
- Around line 14-15: The typed paginator tests are too permissive: the setups
accept any reader delegate and treat any non-null ExclusiveStartKey as page 2,
so they don't verify that the typed overload forwards the exact
DynamoItemReader<TestModel> TestReader or that the continuation key contains the
expected "pk". Update the mock setups for the typed pagination and
ScanAsync/error-path variants to explicitly match and capture the TestReader
delegate (ensure the mock matcher compares the delegate identity) and tighten
the ExclusiveStartKey matcher to assert the expected lastKey["pk"] value instead
of just non-null; then add assertions that the captured reader equals TestReader
and that the continuation key's "pk" equals the expected string so the typed
pagination contract is actually exercised.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9faaf721-0637-4f92-8e08-b421cc827d89
📒 Files selected for processing (4)
tests/Clients/Goa.Clients.Dynamo.Benchmarks/Benchmarks/QueryAsyncBenchmarks.cstests/Clients/Goa.Clients.Dynamo.Benchmarks/Goa.Clients.Dynamo.Benchmarks.csprojtests/Clients/Goa.Clients.Dynamo.Tests/BuilderChainingTests.cstests/Clients/Goa.Clients.Dynamo.Tests/TypedExtensionTests.cs
Owner
Author
Baseline
PR
|
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
QueryAsync<T>,ScanAsync<T>,GetItemAsync<T>,PutItemAsync<T>,BatchGetItemAsync<T>,TransactGetItemsAsync<T>DynamoItemReader<T>andDynamoItemWriter<T>abstractions for typed item mappingQueryAllAsync<T>,ScanAllAsync<T>,BatchGetAllAsync<T>