perf: Convert AttributeValue from class to readonly struct with factory methods#70
Merged
perf: Convert AttributeValue from class to readonly struct with factory methods#70
Conversation
…t and typed factory methods
…ributeValue factory methods
…ds instead of object initializers
Contributor
📝 WalkthroughWalkthroughReplaces the legacy AttributeValue class with a readonly struct and AttributeType, adds a JSON converter and factory methods, and updates codegen, serializers/readers, builders, extensions, models, tests, and benchmarks to use the new factory-style AttributeValue API and null semantics. Changes
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ 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: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/Clients/Goa.Clients.Dynamo/Extensions/DynamoRecordExtensions.cs (1)
43-50:⚠️ Potential issue | 🟠 MajorAllow empty strings through
TryGetString.Line 49 still treats
""as missing even though the XML docs only mention missing/null, andTryGetNullableStringalready round-trips empty strings. That makes the required-string path fail on a valid stored value.💡 Suggested fix
- if (attributeValue.Type == AttributeType.Null || string.IsNullOrEmpty(attributeValue.S)) + if (attributeValue.Type == AttributeType.Null || attributeValue.S is null) return false;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Clients/Goa.Clients.Dynamo/Extensions/DynamoRecordExtensions.cs` around lines 43 - 50, The TryGetString method currently treats empty strings as missing; update it so it only treats AttributeType.Null or a null S as missing, allowing "" to be returned as a valid value (same behavior as TryGetNullableString). Specifically, in TryGetString (and related checks around attributeValue.S) remove the string.IsNullOrEmpty check and only check attributeValue.Type == AttributeType.Null or attributeValue.S == null before returning false so empty string values are preserved and returned via the out parameter.src/Clients/Goa.Clients.Dynamo.Generator/TypeHandlers/CollectionTypeHandler.cs (2)
81-84:⚠️ Potential issue | 🟠 MajorPass the active lambda variable name to
TryGeneratePrimitiveCollectionForElementfor correct code generation with nested collections.The method at line 103 hard-codes
itemin all return statements (lines 107-116), but is called from line 81 without receiving the current lambda variable name. For nested collections likeList<List<List<T>>>where T is primitive, the recursive call at line 89 generatesnestedVarName(e.g.,"nested"), butTryGeneratePrimitiveCollectionForElementstill returns code referencing the wrong variable. The generated code compiles but uses the wrong level of nesting.Add
itemVarNameparameter to the method signature and pass it from the call site:Required changes
- var nestedPrimitiveMapping = TryGeneratePrimitiveCollectionForElement(nestedElementType); + var nestedPrimitiveMapping = TryGeneratePrimitiveCollectionForElement(nestedElementType, itemVarName); - private string? TryGeneratePrimitiveCollectionForElement(ITypeSymbol elementType) + private string? TryGeneratePrimitiveCollectionForElement(ITypeSymbol elementType, string collectionVarName)Replace all hard-coded
itemreferences in the switch arms (lines 107-116) withcollectionVarName.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Clients/Goa.Clients.Dynamo.Generator/TypeHandlers/CollectionTypeHandler.cs` around lines 81 - 84, TryGeneratePrimitiveCollectionForElement is using a hard-coded "item" variable causing wrong nesting for nested collections; update its signature to accept an itemVarName (or collectionVarName) parameter, pass the current lambda variable name (e.g., nestedVarName generated in the recursive path) from the call site where TryGeneratePrimitiveCollectionForElement is invoked, and replace every hard-coded "item" reference in the switch arms inside TryGeneratePrimitiveCollectionForElement with the new parameter (collectionVarName) so generated code uses the correct lambda variable at each nesting level.
66-72:⚠️ Potential issue | 🔴 CriticalStruct elements need value-type null semantics, not reference-type checks.
IsComplexTypeincludes structs, but line 71 emitsitem != null ? ... ToDynamoRecord(item) .... ForList<MyStruct>, the null check is logically incorrect (value types can't be null). ForList<MyStruct?>, the generated code fails to compile because it passesMyStruct?directly to a method expectingMyStructwithout unwrapping. Check ifelementTypeis a nullable value type and handle unwrapping accordingly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Clients/Goa.Clients.Dynamo.Generator/TypeHandlers/CollectionTypeHandler.cs` around lines 66 - 72, IsComplexType currently treats structs like reference types and emits a null-check using itemVarName, which breaks for value types and nullable structs; update the complex-type branch in CollectionTypeHandler.cs to detect nullable value types (use Nullable.GetUnderlyingType(elementType) or elementType.IsValueType && Nullable.GetUnderlyingType(...) != null) and emit: for non-nullable value-type structs call DynamoMapper.{normalizedTypeName}.ToDynamoRecord(itemVarName) directly (no null check), and for nullable structs emit itemVarName.HasValue ? DynamoMapper.{normalizedTypeName}.ToDynamoRecord(itemVarName.Value) : AttributeValue.Null(); keep the existing reference-type null-check logic for class types and continue using NamingHelpers.NormalizeTypeName to compute the mapper name.src/Clients/Goa.Clients.Dynamo.Generator/TypeHandlers/ComplexTypeHandler.cs (1)
65-72:⚠️ Potential issue | 🟠 MajorDon't emit empty string sets for null or empty inner lists.
Line 71 calls
AttributeValue.FromStringSet(kvp.Value ?? new List<string>()), which creates an empty string set whenkvp.Valueis null or empty. DynamoDB does not allow empty Set attributes in any context, including nested in Maps—this would generate invalid items. EmitAttributeValue.Null()instead for empty or null collections.Proposed fix
- return $"model.{propertyName} != null ? AttributeValue.FromMap(model.{propertyName}.ToDictionary(kvp => kvp.Key, kvp => AttributeValue.FromStringSet(kvp.Value ?? new List<string>()))) : AttributeValue.Null()"; + return $"model.{propertyName} != null ? AttributeValue.FromMap(model.{propertyName}.ToDictionary(kvp => kvp.Key, kvp => kvp.Value != null && kvp.Value.Any() ? AttributeValue.FromStringSet(kvp.Value) : AttributeValue.Null())) : AttributeValue.Null()";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Clients/Goa.Clients.Dynamo.Generator/TypeHandlers/ComplexTypeHandler.cs` around lines 65 - 72, The code in ComplexTypeHandler.cs special-case for Dictionary<string, List<string>> currently always emits AttributeValue.FromStringSet(kvp.Value ?? new List<string>()), which produces an empty set for null/empty inner lists (invalid in DynamoDB); update the emitted expression so that for each kvp you emit AttributeValue.Null() when kvp.Value is null or kvp.Value.Count == 0, otherwise emit AttributeValue.FromStringSet(kvp.Value) — locate the block that checks valueType/ namedValueType and adjust the lambda inside ToDictionary(kvp => ..., kvp => ...) to test kvp.Value null-or-empty and return AttributeValue.Null() in that case, otherwise AttributeValue.FromStringSet(kvp.Value).
🧹 Nitpick comments (9)
tests/Clients/Goa.Clients.Dynamo.Benchmarks/Benchmarks/TransactWriteItemsAsyncBenchmarks.cs (1)
117-131: Seed operation included in benchmark timing.The
PutItemAsynccall on lines 122-131 to seed the item is included in the benchmark measurement. If the intent is to measure pureTransactWriteItemsAsyncperformance, consider moving the seed operation to an[IterationSetup]method. However, if the goal is to benchmark a realistic "seed-then-transact" workflow, the current design is appropriate.🤖 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/TransactWriteItemsAsyncBenchmarks.cs` around lines 117 - 131, The benchmark method AwsSdk_TransactWrite_MixedOps currently performs a seed PutItemAsync via _fixture.AwsSdkClient inside the measured benchmark; move that seeding logic out of the method into a dedicated [IterationSetup] or [GlobalSetup] method (or alternatively remove it if you intend to measure a pure TransactWriteItemsAsync scenario) so the PutItemAsync call is not included in the timing for AwsSdk_TransactWrite_MixedOps; locate the PutItemAsync call and transfer its request creation and await call to the setup method while keeping AwsSdk_TransactWrite_MixedOps focused only on invoking TransactWriteItemsAsync.tests/Clients/Goa.Clients.Dynamo.Benchmarks/Benchmarks/BatchWriteItemAsyncBenchmarks.cs (1)
39-39: Consider pre-allocating list capacity for more accurate benchmark measurements.Since the item counts are known constants (10 and 25), pre-allocating list capacity would eliminate internal array resizing and reduce allocation noise in the benchmark results. This applies to all six benchmark methods.
♻️ Example for 10-item benchmarks
- var requests = new List<Amazon.DynamoDBv2.Model.WriteRequest>(); + var requests = new List<Amazon.DynamoDBv2.Model.WriteRequest>(10);Similar changes for lines 63, 93, 116, 140, and 170 with their respective capacities (10 or 25).
🤖 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/BatchWriteItemAsyncBenchmarks.cs` at line 39, The benchmarks create a List<Amazon.DynamoDBv2.Model.WriteRequest> called requests without pre-allocating capacity which introduces allocation/resizing noise; change each allocation of requests in the six benchmark methods to new List<Amazon.DynamoDBv2.Model.WriteRequest>(N) using the known item counts (use 10 for the 10-item benchmarks and 25 for the 25-item benchmarks) so the list is pre-sized and allocation noise is removed.tests/Clients/Goa.Clients.Dynamo.Benchmarks/Benchmarks/TransactGetItemsAsyncBenchmarks.cs (2)
35-36: Pre-size the request lists to keep resize noise out of the benchmark.Each method knows its exact item count up front. Initializing the lists with
5or10avoids avoidable reallocations during the measured path.♻️ Suggested change
- var items = new List<Amazon.DynamoDBv2.Model.TransactGetItem>(); + var items = new List<Amazon.DynamoDBv2.Model.TransactGetItem>(5); ... - var items = new List<Goa.Clients.Dynamo.Operations.Transactions.TransactGetItem>(); + var items = new List<Goa.Clients.Dynamo.Operations.Transactions.TransactGetItem>(5); ... - var items = new List<EfficientTransactGetRequest>(); + var items = new List<EfficientTransactGetRequest>(5); ... - var items = new List<Amazon.DynamoDBv2.Model.TransactGetItem>(); + var items = new List<Amazon.DynamoDBv2.Model.TransactGetItem>(10); ... - var items = new List<Goa.Clients.Dynamo.Operations.Transactions.TransactGetItem>(); + var items = new List<Goa.Clients.Dynamo.Operations.Transactions.TransactGetItem>(10); ... - var items = new List<EfficientTransactGetRequest>(); + var items = new List<EfficientTransactGetRequest>(10);Also applies to: 60-61, 86-87, 104-105, 129-130, 155-156
🤖 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/TransactGetItemsAsyncBenchmarks.cs` around lines 35 - 36, Pre-size the request lists to avoid reallocations during benchmarks: replace new List<Amazon.DynamoDBv2.Model.TransactGetItem>() (and the other List<T> creations at the similar sections) with a capacity-initialized list (e.g., new List<Amazon.DynamoDBv2.Model.TransactGetItem>(5) for loops that run 5 iterations, or 10 where appropriate) so the local variables like items are allocated with the exact expected capacity before the for-loops in the TransactGetItemsAsyncBenchmarks methods.
23-23: Extract the seeded key literals into shared constants/helpers.
"transact-get"and theitem-{i:D4}format are repeated across setup and every benchmark path. Centralizing them will make future seed/key changes less error-prone and keep the three clients aligned.♻️ Suggested change
public class TransactGetItemsAsyncBenchmarks { + private const string PartitionKey = "transact-get"; + private static string SortKey(int i) => $"item-{i:D4}"; + private LocalStackFixture _fixture = null!; ... - _fixture.SeedItemsAsync("transact-get", 10).GetAwaiter().GetResult(); + _fixture.SeedItemsAsync(PartitionKey, 10).GetAwaiter().GetResult(); ... - ["pk"] = new AttributeValue("transact-get"), - ["sk"] = new AttributeValue($"item-{i:D4}") + ["pk"] = new AttributeValue(PartitionKey), + ["sk"] = new AttributeValue(SortKey(i)) ... - ["pk"] = GoaModels.AttributeValue.String("transact-get"), - ["sk"] = GoaModels.AttributeValue.String($"item-{i:D4}") + ["pk"] = GoaModels.AttributeValue.String(PartitionKey), + ["sk"] = GoaModels.AttributeValue.String(SortKey(i)) ... - Key = new PrimaryKey("pk", "transact-get", "sk", $"item-{i:D4}") + Key = new PrimaryKey("pk", PartitionKey, "sk", SortKey(i))Also applies to: 45-46, 70-71, 92-92, 114-115, 139-140, 161-161
🤖 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/TransactGetItemsAsyncBenchmarks.cs` at line 23, The tests repeat the literal partition key "transact-get" and the item id format "item-{i:D4}" across setup and benchmark paths; refactor by extracting these into shared constants (e.g., a public const string TransactGetPartitionKey and a public const string ItemIdFormat) in the test class or the test fixture used by TransactGetItemsAsyncBenchmarks, then update all calls to _fixture.SeedItemsAsync, path constructions and any string.Format/Interpolations that build "item-{i:D4}" to use those constants (affecting usages in TransactGetItemsAsyncBenchmarks methods and any related benchmark methods referenced in the diff). Ensure the constants are accessible to all benchmark files so all seed and path code (including calls to SeedItemsAsync and benchmark path strings) use the centralized values.tests/Clients/Goa.Clients.Dynamo.Benchmarks/Benchmarks/QueryAsyncBenchmarks.cs (2)
14-31: DuplicateBenchmarkEntityclass definition.This class appears to duplicate
tests/Clients/Goa.Clients.Dynamo.Benchmarks/Models/BenchmarkEntity.cs(per the relevant code snippets). Consider removing this local definition and using the existing model to avoid maintenance burden.♻️ Proposed fix to remove duplication
using EfficientDynamoDb.Attributes; using Goa.Clients.Dynamo.Benchmarks.Infrastructure; using Goa.Clients.Dynamo.Benchmarks.Models; using EfficientAttributeValue = EfficientDynamoDb.DocumentModel.AttributeValue; using GoaModels = Goa.Clients.Dynamo.Models; using GoaQueryRequest = Goa.Clients.Dynamo.Operations.Query.QueryRequest; using EfficientQueryRequest = EfficientDynamoDb.Operations.Query.QueryRequest; namespace Goa.Clients.Dynamo.Benchmarks.Benchmarks; -[DynamoDbTable("benchmark-table")] -public class BenchmarkEntity -{ - [DynamoDbProperty("pk", DynamoDbAttributeType.PartitionKey)] - public string Pk { get; set; } = ""; - - [DynamoDbProperty("sk", DynamoDbAttributeType.SortKey)] - public string Sk { get; set; } = ""; - - [DynamoDbProperty("data")] - public string Data { get; set; } = ""; - - [DynamoDbProperty("number")] - public int Number { get; set; } - - [DynamoDbProperty("status")] - public string Status { get; set; } = ""; -} - [MemoryDiagnoser]🤖 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 14 - 31, The BenchmarkEntity class here is a duplicate of the existing model; remove this local definition and update the file to use the shared model instead: delete the BenchmarkEntity class declaration in QueryAsyncBenchmarks.cs, add the appropriate using/import for the existing BenchmarkEntity type from the Models namespace, and ensure any code that referenced the local properties (Pk, Sk, Data, Number, Status) uses the shared BenchmarkEntity class; rebuild to verify no missing references.
160-492: Large block of commented-out code should be removed or tracked.Over 300 lines of commented-out benchmark code (10-item, 100-item, and no-results variants) clutters the file. If these are intentionally disabled for now, consider:
- Removing and re-adding when needed via version control
- Creating a tracking issue if they're planned for future implementation
🤖 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 160 - 492, The large commented-out benchmark block (methods like AwsSdk_Query_10Items, Goa_Query_10Items_DynamoRecord, Goa_Query_10Items_Typed, Efficient_Query_10Items, AwsSdk_Query_100Items, Goa_Query_100Items_DynamoRecord, Efficient_Query_100Items, AwsSdk_Query_NoResults, Goa_Query_NoResults_DynamoRecord, Efficient_Query_NoResults, Efficient_Query_NoResults_Typed, etc.) should be removed from the file or moved out of the main test class to reduce noise; either delete the commented methods (they remain recoverable in VCS), or extract them into a separate file (e.g., ArchivedBenchmarks.cs) or create a tracking issue and add a short comment referencing that issue where the block was to indicate it was intentionally removed. Ensure any references to those method names are also cleaned up so the class compiles and linter warnings are avoided.src/Clients/Goa.Clients.Dynamo/Models/AttributeValue.cs (2)
12-18: Clarify: This is not a true union layout.The comment on line 10 mentions "union layout to avoid heap allocations", but the field offsets don't overlap—this is a standard sequential layout with explicit offsets. The struct still allocates heap memory for
_referenceValue(strings, lists, dictionaries). The benefit here is avoiding a separate heap allocation for theAttributeValuecontainer itself when used in arrays/dictionaries, not eliminating heap allocations entirely.Consider updating the doc comment to accurately reflect the optimization (avoiding boxing/extra allocations for the container).
🤖 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 12 - 18, The doc comment for the AttributeValue struct incorrectly claims a "union layout to avoid heap allocations"; update that comment to accurately state that although StructLayout with explicit FieldOffset is used on AttributeValue (which contains fields like _referenceValue, _type, and _boolValue) the offsets do not overlap and reference types (e.g., string, List<>, Dictionary<>) still allocate on the heap — the real optimization is avoiding boxing/extra heap allocation for the AttributeValue container itself when placed in arrays or dictionaries, not eliminating heap allocations for reference payloads. Ensure the revised comment mentions AttributeValue, _referenceValue, and the intent to avoid container boxing/extra allocations rather than implying a true union that prevents all heap allocations.
121-122: Implicit conversion fromList<string>creates a StringSet, not a List.This implicit conversion maps
List<string>toFromStringSet, which creates a DynamoDB String Set (SS). Sets have unique-element semantics and don't preserve order. If the caller passes a list with duplicates or expects order preservation, this could cause data loss or unexpected behavior.Consider whether this implicit conversion should be removed to make the distinction explicit, or document this behavior clearly.
🤖 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 121 - 122, The implicit conversion operator public static implicit operator AttributeValue(List<string> value) => FromStringSet(value) incorrectly treats a List<string> as a DynamoDB String Set (SS) losing order/duplicates; remove this implicit operator (or change it to explicit) so callers must explicitly call FromStringSet or a new FromStringList helper, and update any usages to avoid silently converting lists to sets; reference the AttributeValue type, the FromStringSet method, and the implicit operator for List<string> when making the change.src/Clients/Goa.Clients.Dynamo/Models/DynamoRecord.cs (1)
28-31: Pre-size the copy constructor.Line 28 drops the source count, so copied maps grow incrementally while this constructor replays entries. Since nested map extraction goes through this path, preserving
record.Counthere avoids extra rehashing.💡 Suggested fix
-public DynamoRecord(Dictionary<string, AttributeValue> record) : base(StringComparer.Ordinal) +public DynamoRecord(Dictionary<string, AttributeValue> record) + : base(record is null ? 0 : record.Count, StringComparer.Ordinal) { + ArgumentNullException.ThrowIfNull(record); foreach (var item in record) - this[item.Key] = item.Value; + Add(item.Key, item.Value); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Clients/Goa.Clients.Dynamo/Models/DynamoRecord.cs` around lines 28 - 31, The constructor DynamoRecord(Dictionary<string, AttributeValue> record) currently calls base(StringComparer.Ordinal) so the internal dictionary grows incrementally; change the base call to include the source count to pre-size the map (use base(record.Count, StringComparer.Ordinal)) before copying items from record to avoid rehashing during the foreach.
🤖 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/TypeHandlers/ComplexTypeHandler.cs`:
- Around line 92-95: GenerateToAttributeValue currently emits a reference-type
null check and passes model.{propertyName} directly for all complex types;
update it to branch on propertyInfo.IsNullable and
propertyInfo.UnderlyingType.TypeKind: for non-nullable structs (TypeKind ==
Struct and IsNullable == false) emit no null check and call
DynamoMapper.{normalizedTypeName}.ToDynamoRecord(model.{propertyName}); for
nullable structs (TypeKind == Struct and IsNullable == true) emit a null check
and pass model.{propertyName}.Value into
DynamoMapper.{normalizedTypeName}.ToDynamoRecord(...); for reference types keep
the existing "model.{propertyName} != null ?
AttributeValue.FromMap(DynamoMapper.{normalizedTypeName}.ToDynamoRecord(model.{propertyName}))
: AttributeValue.Null()" pattern. Ensure you reference propertyInfo,
propertyName, GenerateToAttributeValue and normalizedTypeName when making the
change.
In
`@src/Clients/Goa.Clients.Dynamo.Generator/TypeHandlers/DateOnlyTypeHandler.cs`:
- Around line 62-65: The nullable write branch in DateOnlyTypeHandler currently
uses the CLR propertyName when adding to {recordVariable} (return string uses
"{propertyName}") which causes a mismatch if a different Dynamo attribute name
is configured; update the generated string to use the configured Dynamo
attribute name variable (the same identifier used for reads—e.g. the
attributeName/columnName symbol used elsewhere in DateOnlyTypeHandler) instead
of the CLR propertyName so the write path stores under the configured attribute
key.
In
`@src/Clients/Goa.Clients.Dynamo.Generator/TypeHandlers/PrimitiveTypeHandler.cs`:
- Around line 64-79: In PrimitiveTypeHandler (the switch arms handling
SpecialType.System_DateTime and DateTimeOffset) remove the extra backslashes
escaping the inner quotes so the generated expression emits ToString("o") (not
ToString(\"o\")). Update the string templates that build
AttributeValue.String(model.{propertyName}.ToString(\\\"o\\\")) to produce
AttributeValue.String(model.{propertyName}.ToString("o")) and make the same
change for the DateTimeOffset arm (and any other occurrences emitting
ToString(\\\"o\\\")). Ensure you only modify the string literal templates that
reference propertyName and the ToString format, leaving the nullable branches
that return null unchanged.
In `@src/Clients/Goa.Clients.Dynamo/Models/AttributeValue.cs`:
- Around line 163-168: The JSON reader path that handles the "S" attribute
currently uses reader.GetString()! and can produce a null string for inputs like
{"S": null}; update the branch that matches reader.ValueTextEquals("S"u8) to
explicitly detect a JSON null (e.g., inspect the reader token or check for null
result from reader.GetString()) after reader.Read(), and then either (a) treat
it as an invalid value and throw a descriptive exception, or (b) map it to a
distinct AttributeValue state (e.g., AttributeValue.Null or a documented
nullable string variant) instead of passing null into AttributeValue.String;
ensure you still call reader.Read() to consume the EndObject and keep the
overall reader flow intact when modifying the logic around AttributeValue.String
and reader.GetString.
- Around line 241-289: The Write method for AttributeValue (Write(Utf8JsonWriter
writer, AttributeValue value, JsonSerializerOptions options)) doesn't handle
AttributeType.Binary; add a case for AttributeType.Binary that serializes the
binary payload under the "B" key (e.g., base64-encode the
byte[]/ReadOnlyMemory<byte> stored on the AttributeValue) and write it with the
writer, and also ensure full Binary support is implemented by adding the
corresponding Binary property/field on AttributeValue, a factory/constructor for
Binary values, and the Read logic that deserializes "B" back into that property;
alternatively remove AttributeType.Binary from the enum if you prefer not to
support binary.
In
`@tests/Clients/Goa.Clients.Dynamo.Benchmarks/Benchmarks/DeleteItemAsyncBenchmarks.cs`:
- Around line 77-87: The benchmarks currently call AwsSdkClient.DeleteItemAsync
(and the corresponding Efficient/other variants) with
ReturnValues.ALL_OLD/AllOld but never insert the target key first, so the delete
never returns the old item; update the benchmark to seed the item before the
measured delete by inserting the same key (e.g., use
_fixture.AwsSdkClient.PutItemAsync or the fixture helper that inserts items) in
a per-iteration setup step immediately before calling DeleteItemAsync in
DeleteItemAsyncBenchmarks (and do the same for the other AllOld benchmark
methods referenced), ensuring the seeded item uses the same key pattern
("delret-<id>") so ReturnValues.ALL_OLD actually returns the old item.
- Around line 48-60: In Goa_DeleteItem() you're converting an error response
into a successful benchmark sample by returning !response.IsError; instead,
detect when response.IsError and fail fast: if response.IsError throw an
exception (e.g., InvalidOperationException) including the response error details
(response.Error / response.ErrorMessage / response.Body as available) so failed
Goa calls surface as benchmark failures rather than as successful samples; keep
returning true only when the response indicates success.
In
`@tests/Clients/Goa.Clients.Dynamo.Benchmarks/Benchmarks/GetItemAsyncBenchmarks.cs`:
- Around line 61-74: The compile error CS0246 is caused by the missing type
BenchmarkItem used in the typed benchmark methods (e.g., Goa_GetItem_Typed) and
other benchmark variants; to fix, either add or reference a BenchmarkItem model
in the test project (or replace the type with an existing benchmark model type),
update the using/imports if the model exists in another namespace, and ensure
DynamoItemReaderRegistry.Get<BenchmarkItem>() targets a valid registered reader;
adjust all occurrences (the typed GetItem/GetItems benchmark methods) so they
reference the resolved type or add the missing class before merging.
In
`@tests/Clients/Goa.Clients.Dynamo.Benchmarks/Benchmarks/UpdateItemAsyncBenchmarks.cs`:
- Line 218: The method currently returns response.Value.Attributes directly
which can throw if the call failed; mirror the pattern used in
Goa_UpdateItem_SingleAttribute and Goa_UpdateItem_MultipleAttributes by checking
response.IsError before accessing response.Value, and handle the error (e.g.,
throw a descriptive exception or return an empty attribute set) so you don't
access Attributes when response indicates an error; locate the response variable
and the return statement and add the same guard/handling used in the other
Goa_UpdateItem_* benchmark methods.
In `@tests/Clients/Goa.Clients.Dynamo.Tests/TypedExtensionTests.cs`:
- Around line 1-14: Add the missing typed API: declare the generic delegate
DynamoItemReader<T>, add generic overloads on IDynamoClient for
QueryAsync<T>(..., DynamoItemReader<T>) and ScanAsync<T>(...,
DynamoItemReader<T>), create generic result types QueryResult<T> and
ScanResult<T> (mirroring existing non-generic results), and implement extension
methods QueryAllAsync<T> and ScanAllAsync<T> that use the new
QueryAsync<T>/ScanAsync<T> and the paginated result types to yield typed
pages/items; update implementations to reuse the existing non-generic logic
where possible so behavior matches current Query/Scan flows.
---
Outside diff comments:
In
`@src/Clients/Goa.Clients.Dynamo.Generator/TypeHandlers/CollectionTypeHandler.cs`:
- Around line 81-84: TryGeneratePrimitiveCollectionForElement is using a
hard-coded "item" variable causing wrong nesting for nested collections; update
its signature to accept an itemVarName (or collectionVarName) parameter, pass
the current lambda variable name (e.g., nestedVarName generated in the recursive
path) from the call site where TryGeneratePrimitiveCollectionForElement is
invoked, and replace every hard-coded "item" reference in the switch arms inside
TryGeneratePrimitiveCollectionForElement with the new parameter
(collectionVarName) so generated code uses the correct lambda variable at each
nesting level.
- Around line 66-72: IsComplexType currently treats structs like reference types
and emits a null-check using itemVarName, which breaks for value types and
nullable structs; update the complex-type branch in CollectionTypeHandler.cs to
detect nullable value types (use Nullable.GetUnderlyingType(elementType) or
elementType.IsValueType && Nullable.GetUnderlyingType(...) != null) and emit:
for non-nullable value-type structs call
DynamoMapper.{normalizedTypeName}.ToDynamoRecord(itemVarName) directly (no null
check), and for nullable structs emit itemVarName.HasValue ?
DynamoMapper.{normalizedTypeName}.ToDynamoRecord(itemVarName.Value) :
AttributeValue.Null(); keep the existing reference-type null-check logic for
class types and continue using NamingHelpers.NormalizeTypeName to compute the
mapper name.
In `@src/Clients/Goa.Clients.Dynamo.Generator/TypeHandlers/ComplexTypeHandler.cs`:
- Around line 65-72: The code in ComplexTypeHandler.cs special-case for
Dictionary<string, List<string>> currently always emits
AttributeValue.FromStringSet(kvp.Value ?? new List<string>()), which produces an
empty set for null/empty inner lists (invalid in DynamoDB); update the emitted
expression so that for each kvp you emit AttributeValue.Null() when kvp.Value is
null or kvp.Value.Count == 0, otherwise emit
AttributeValue.FromStringSet(kvp.Value) — locate the block that checks
valueType/ namedValueType and adjust the lambda inside ToDictionary(kvp => ...,
kvp => ...) to test kvp.Value null-or-empty and return AttributeValue.Null() in
that case, otherwise AttributeValue.FromStringSet(kvp.Value).
In `@src/Clients/Goa.Clients.Dynamo/Extensions/DynamoRecordExtensions.cs`:
- Around line 43-50: The TryGetString method currently treats empty strings as
missing; update it so it only treats AttributeType.Null or a null S as missing,
allowing "" to be returned as a valid value (same behavior as
TryGetNullableString). Specifically, in TryGetString (and related checks around
attributeValue.S) remove the string.IsNullOrEmpty check and only check
attributeValue.Type == AttributeType.Null or attributeValue.S == null before
returning false so empty string values are preserved and returned via the out
parameter.
---
Nitpick comments:
In `@src/Clients/Goa.Clients.Dynamo/Models/AttributeValue.cs`:
- Around line 12-18: The doc comment for the AttributeValue struct incorrectly
claims a "union layout to avoid heap allocations"; update that comment to
accurately state that although StructLayout with explicit FieldOffset is used on
AttributeValue (which contains fields like _referenceValue, _type, and
_boolValue) the offsets do not overlap and reference types (e.g., string,
List<>, Dictionary<>) still allocate on the heap — the real optimization is
avoiding boxing/extra heap allocation for the AttributeValue container itself
when placed in arrays or dictionaries, not eliminating heap allocations for
reference payloads. Ensure the revised comment mentions AttributeValue,
_referenceValue, and the intent to avoid container boxing/extra allocations
rather than implying a true union that prevents all heap allocations.
- Around line 121-122: The implicit conversion operator public static implicit
operator AttributeValue(List<string> value) => FromStringSet(value) incorrectly
treats a List<string> as a DynamoDB String Set (SS) losing order/duplicates;
remove this implicit operator (or change it to explicit) so callers must
explicitly call FromStringSet or a new FromStringList helper, and update any
usages to avoid silently converting lists to sets; reference the AttributeValue
type, the FromStringSet method, and the implicit operator for List<string> when
making the change.
In `@src/Clients/Goa.Clients.Dynamo/Models/DynamoRecord.cs`:
- Around line 28-31: The constructor DynamoRecord(Dictionary<string,
AttributeValue> record) currently calls base(StringComparer.Ordinal) so the
internal dictionary grows incrementally; change the base call to include the
source count to pre-size the map (use base(record.Count,
StringComparer.Ordinal)) before copying items from record to avoid rehashing
during the foreach.
In
`@tests/Clients/Goa.Clients.Dynamo.Benchmarks/Benchmarks/BatchWriteItemAsyncBenchmarks.cs`:
- Line 39: The benchmarks create a List<Amazon.DynamoDBv2.Model.WriteRequest>
called requests without pre-allocating capacity which introduces
allocation/resizing noise; change each allocation of requests in the six
benchmark methods to new List<Amazon.DynamoDBv2.Model.WriteRequest>(N) using the
known item counts (use 10 for the 10-item benchmarks and 25 for the 25-item
benchmarks) so the list is pre-sized and allocation noise is removed.
In
`@tests/Clients/Goa.Clients.Dynamo.Benchmarks/Benchmarks/QueryAsyncBenchmarks.cs`:
- Around line 14-31: The BenchmarkEntity class here is a duplicate of the
existing model; remove this local definition and update the file to use the
shared model instead: delete the BenchmarkEntity class declaration in
QueryAsyncBenchmarks.cs, add the appropriate using/import for the existing
BenchmarkEntity type from the Models namespace, and ensure any code that
referenced the local properties (Pk, Sk, Data, Number, Status) uses the shared
BenchmarkEntity class; rebuild to verify no missing references.
- Around line 160-492: The large commented-out benchmark block (methods like
AwsSdk_Query_10Items, Goa_Query_10Items_DynamoRecord, Goa_Query_10Items_Typed,
Efficient_Query_10Items, AwsSdk_Query_100Items, Goa_Query_100Items_DynamoRecord,
Efficient_Query_100Items, AwsSdk_Query_NoResults,
Goa_Query_NoResults_DynamoRecord, Efficient_Query_NoResults,
Efficient_Query_NoResults_Typed, etc.) should be removed from the file or moved
out of the main test class to reduce noise; either delete the commented methods
(they remain recoverable in VCS), or extract them into a separate file (e.g.,
ArchivedBenchmarks.cs) or create a tracking issue and add a short comment
referencing that issue where the block was to indicate it was intentionally
removed. Ensure any references to those method names are also cleaned up so the
class compiles and linter warnings are avoided.
In
`@tests/Clients/Goa.Clients.Dynamo.Benchmarks/Benchmarks/TransactGetItemsAsyncBenchmarks.cs`:
- Around line 35-36: Pre-size the request lists to avoid reallocations during
benchmarks: replace new List<Amazon.DynamoDBv2.Model.TransactGetItem>() (and the
other List<T> creations at the similar sections) with a capacity-initialized
list (e.g., new List<Amazon.DynamoDBv2.Model.TransactGetItem>(5) for loops that
run 5 iterations, or 10 where appropriate) so the local variables like items are
allocated with the exact expected capacity before the for-loops in the
TransactGetItemsAsyncBenchmarks methods.
- Line 23: The tests repeat the literal partition key "transact-get" and the
item id format "item-{i:D4}" across setup and benchmark paths; refactor by
extracting these into shared constants (e.g., a public const string
TransactGetPartitionKey and a public const string ItemIdFormat) in the test
class or the test fixture used by TransactGetItemsAsyncBenchmarks, then update
all calls to _fixture.SeedItemsAsync, path constructions and any
string.Format/Interpolations that build "item-{i:D4}" to use those constants
(affecting usages in TransactGetItemsAsyncBenchmarks methods and any related
benchmark methods referenced in the diff). Ensure the constants are accessible
to all benchmark files so all seed and path code (including calls to
SeedItemsAsync and benchmark path strings) use the centralized values.
In
`@tests/Clients/Goa.Clients.Dynamo.Benchmarks/Benchmarks/TransactWriteItemsAsyncBenchmarks.cs`:
- Around line 117-131: The benchmark method AwsSdk_TransactWrite_MixedOps
currently performs a seed PutItemAsync via _fixture.AwsSdkClient inside the
measured benchmark; move that seeding logic out of the method into a dedicated
[IterationSetup] or [GlobalSetup] method (or alternatively remove it if you
intend to measure a pure TransactWriteItemsAsync scenario) so the PutItemAsync
call is not included in the timing for AwsSdk_TransactWrite_MixedOps; locate the
PutItemAsync call and transfer its request creation and await call to the setup
method while keeping AwsSdk_TransactWrite_MixedOps focused only on invoking
TransactWriteItemsAsync.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6d7fe385-d202-4c2d-9e43-d87c05a32582
📒 Files selected for processing (45)
src/Clients/Goa.Clients.Bedrock.Conversation.Dynamo/DynamoConversationStore.cssrc/Clients/Goa.Clients.Bedrock.Conversation.Dynamo/Internal/ContentBlockSerializer.cssrc/Clients/Goa.Clients.Dynamo.Generator/CodeGeneration/MapperGenerator.cssrc/Clients/Goa.Clients.Dynamo.Generator/TypeHandlers/CollectionTypeHandler.cssrc/Clients/Goa.Clients.Dynamo.Generator/TypeHandlers/ComplexTypeHandler.cssrc/Clients/Goa.Clients.Dynamo.Generator/TypeHandlers/DateOnlyTypeHandler.cssrc/Clients/Goa.Clients.Dynamo.Generator/TypeHandlers/DateTimeTypeHandler.cssrc/Clients/Goa.Clients.Dynamo.Generator/TypeHandlers/EnumTypeHandler.cssrc/Clients/Goa.Clients.Dynamo.Generator/TypeHandlers/PrimitiveTypeHandler.cssrc/Clients/Goa.Clients.Dynamo.Generator/TypeHandlers/TimeOnlyTypeHandler.cssrc/Clients/Goa.Clients.Dynamo.Generator/TypeHandlers/TypeHandlerRegistry.cssrc/Clients/Goa.Clients.Dynamo.Generator/TypeHandlers/UnixTimestampTypeHandler.cssrc/Clients/Goa.Clients.Dynamo.Generator/TypeHandlers/UnsupportedDictionaryHandler.cssrc/Clients/Goa.Clients.Dynamo/Extensions/DynamoRecordExtensions.cssrc/Clients/Goa.Clients.Dynamo/Models/AttributeType.cssrc/Clients/Goa.Clients.Dynamo/Models/AttributeValue.cssrc/Clients/Goa.Clients.Dynamo/Models/DynamoRecord.cssrc/Clients/Goa.Clients.Dynamo/Operations/Batch/BatchGetTableBuilder.cssrc/Clients/Goa.Clients.Dynamo/Operations/Condition.cssrc/Clients/Goa.Clients.Dynamo/Operations/UpdateItem/UpdateItemBuilder.cstests/Clients/Goa.Clients.Bedrock.Conversation.Dynamo.Tests/ContentBlockSerializerTests.cstests/Clients/Goa.Clients.Dynamo.Benchmarks/Benchmarks/BatchGetItemAsyncBenchmarks.cstests/Clients/Goa.Clients.Dynamo.Benchmarks/Benchmarks/BatchWriteItemAsyncBenchmarks.cstests/Clients/Goa.Clients.Dynamo.Benchmarks/Benchmarks/DeleteItemAsyncBenchmarks.cstests/Clients/Goa.Clients.Dynamo.Benchmarks/Benchmarks/GetItemAsyncBenchmarks.cstests/Clients/Goa.Clients.Dynamo.Benchmarks/Benchmarks/PutItemAsyncBenchmarks.cstests/Clients/Goa.Clients.Dynamo.Benchmarks/Benchmarks/QueryAsyncBenchmarks.cstests/Clients/Goa.Clients.Dynamo.Benchmarks/Benchmarks/ScanAsyncBenchmarks.cstests/Clients/Goa.Clients.Dynamo.Benchmarks/Benchmarks/TransactGetItemsAsyncBenchmarks.cstests/Clients/Goa.Clients.Dynamo.Benchmarks/Benchmarks/TransactWriteItemsAsyncBenchmarks.cstests/Clients/Goa.Clients.Dynamo.Benchmarks/Benchmarks/UpdateItemAsyncBenchmarks.cstests/Clients/Goa.Clients.Dynamo.Generator.Tests/CodeGeneration/MapperGeneratorTests.cstests/Clients/Goa.Clients.Dynamo.Generator.Tests/Integration/NullAwarenessIntegrationTests.cstests/Clients/Goa.Clients.Dynamo.Generator.Tests/TypeHandlers/CollectionTypeHandlerTests.cstests/Clients/Goa.Clients.Dynamo.Generator.Tests/TypeHandlers/ComplexTypeHandlerTests.cstests/Clients/Goa.Clients.Dynamo.Generator.Tests/TypeHandlers/DateOnlyTypeHandlerTests.cstests/Clients/Goa.Clients.Dynamo.Generator.Tests/TypeHandlers/DateTimeTypeHandlerTests.cstests/Clients/Goa.Clients.Dynamo.Generator.Tests/TypeHandlers/EnumTypeHandlerTests.cstests/Clients/Goa.Clients.Dynamo.Generator.Tests/TypeHandlers/PrimitiveTypeHandlerTests.cstests/Clients/Goa.Clients.Dynamo.Generator.Tests/TypeHandlers/StringEmptyHandlingTests.cstests/Clients/Goa.Clients.Dynamo.Generator.Tests/TypeHandlers/TimeOnlyTypeHandlerTests.cstests/Clients/Goa.Clients.Dynamo.Generator.Tests/TypeHandlers/UnixTimestampTypeHandlerTests.cstests/Clients/Goa.Clients.Dynamo.Tests/BuilderChainingTests.cstests/Clients/Goa.Clients.Dynamo.Tests/DynamoClientIntegrationTests.cstests/Clients/Goa.Clients.Dynamo.Tests/TypedExtensionTests.cs
Comment on lines
+163
to
+168
| if (reader.ValueTextEquals("S"u8)) | ||
| { | ||
| reader.Read(); | ||
| var v = reader.GetString()!; | ||
| reader.Read(); // EndObject | ||
| return AttributeValue.String(v); |
Contributor
There was a problem hiding this comment.
Potential null dereference if JSON contains null value.
reader.GetString()! uses null-forgiving operator, but if the JSON is {"S": null}, this would return null and pass it to AttributeValue.String(null). The String factory method stores this as a valid String-type AttributeValue with a null reference, which may cause issues downstream when S property is accessed expecting a non-null string.
Consider adding validation or documenting the expected behavior for null string values.
Proposed defensive check
if (reader.ValueTextEquals("S"u8))
{
reader.Read();
- var v = reader.GetString()!;
+ var v = reader.GetString();
+ if (v is null)
+ return default;
reader.Read(); // EndObject
return AttributeValue.String(v);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (reader.ValueTextEquals("S"u8)) | |
| { | |
| reader.Read(); | |
| var v = reader.GetString()!; | |
| reader.Read(); // EndObject | |
| return AttributeValue.String(v); | |
| if (reader.ValueTextEquals("S"u8)) | |
| { | |
| reader.Read(); | |
| var v = reader.GetString(); | |
| reader.Read(); // EndObject | |
| if (v is null) | |
| return default; | |
| return AttributeValue.String(v); | |
| } |
🤖 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 163 -
168, The JSON reader path that handles the "S" attribute currently uses
reader.GetString()! and can produce a null string for inputs like {"S": null};
update the branch that matches reader.ValueTextEquals("S"u8) to explicitly
detect a JSON null (e.g., inspect the reader token or check for null result from
reader.GetString()) after reader.Read(), and then either (a) treat it as an
invalid value and throw a descriptive exception, or (b) map it to a distinct
AttributeValue state (e.g., AttributeValue.Null or a documented nullable string
variant) instead of passing null into AttributeValue.String; ensure you still
call reader.Read() to consume the EndObject and keep the overall reader flow
intact when modifying the logic around AttributeValue.String and
reader.GetString.
Comment on lines
+48
to
+60
| public async Task<bool> Goa_DeleteItem() | ||
| { | ||
| var i = Interlocked.Increment(ref _counter); | ||
| var id = Interlocked.Increment(ref _counter); | ||
| var response = await _fixture.GoaClient.DeleteItemAsync(new Goa.Clients.Dynamo.Operations.DeleteItem.DeleteItemRequest | ||
| { | ||
| TableName = _fixture.TableName, | ||
| Key = new Dictionary<string, GoaModels.AttributeValue> | ||
| { | ||
| ["pk"] = new() { S = $"del-goa-{i}" }, | ||
| ["sk"] = new() { S = "item" } | ||
| ["pk"] = GoaModels.AttributeValue.String($"del-goa-{id}"), | ||
| ["sk"] = GoaModels.AttributeValue.String("item") | ||
| } | ||
| }); | ||
| return response.Value; | ||
| return !response.IsError; |
Contributor
There was a problem hiding this comment.
Don’t turn Goa failures into successful benchmark samples.
On Line 60, an error response becomes false instead of failing the benchmark. That can make a broken Goa path look artificially fast while the AWS and Efficient variants would surface the failure.
Proposed fix
var response = await _fixture.GoaClient.DeleteItemAsync(new Goa.Clients.Dynamo.Operations.DeleteItem.DeleteItemRequest
{
TableName = _fixture.TableName,
Key = new Dictionary<string, GoaModels.AttributeValue>
{
["pk"] = GoaModels.AttributeValue.String($"del-goa-{id}"),
["sk"] = GoaModels.AttributeValue.String("item")
}
});
- return !response.IsError;
+ if (response.IsError)
+ {
+ throw new System.InvalidOperationException("DeleteItemAsync returned an error.");
+ }
+
+ return true;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public async Task<bool> Goa_DeleteItem() | |
| { | |
| var i = Interlocked.Increment(ref _counter); | |
| var id = Interlocked.Increment(ref _counter); | |
| var response = await _fixture.GoaClient.DeleteItemAsync(new Goa.Clients.Dynamo.Operations.DeleteItem.DeleteItemRequest | |
| { | |
| TableName = _fixture.TableName, | |
| Key = new Dictionary<string, GoaModels.AttributeValue> | |
| { | |
| ["pk"] = new() { S = $"del-goa-{i}" }, | |
| ["sk"] = new() { S = "item" } | |
| ["pk"] = GoaModels.AttributeValue.String($"del-goa-{id}"), | |
| ["sk"] = GoaModels.AttributeValue.String("item") | |
| } | |
| }); | |
| return response.Value; | |
| return !response.IsError; | |
| public async Task<bool> Goa_DeleteItem() | |
| { | |
| var id = Interlocked.Increment(ref _counter); | |
| var response = await _fixture.GoaClient.DeleteItemAsync(new Goa.Clients.Dynamo.Operations.DeleteItem.DeleteItemRequest | |
| { | |
| TableName = _fixture.TableName, | |
| Key = new Dictionary<string, GoaModels.AttributeValue> | |
| { | |
| ["pk"] = GoaModels.AttributeValue.String($"del-goa-{id}"), | |
| ["sk"] = GoaModels.AttributeValue.String("item") | |
| } | |
| }); | |
| if (response.IsError) | |
| { | |
| throw new System.InvalidOperationException("DeleteItemAsync returned an error."); | |
| } | |
| return true; | |
| } |
🤖 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/DeleteItemAsyncBenchmarks.cs`
around lines 48 - 60, In Goa_DeleteItem() you're converting an error response
into a successful benchmark sample by returning !response.IsError; instead,
detect when response.IsError and fail fast: if response.IsError throw an
exception (e.g., InvalidOperationException) including the response error details
(response.Error / response.ErrorMessage / response.Body as available) so failed
Goa calls surface as benchmark failures rather than as successful samples; keep
returning true only when the response indicates success.
Comment on lines
+77
to
+87
| var id = Interlocked.Increment(ref _counter); | ||
| return await _fixture.AwsSdkClient.DeleteItemAsync(new DeleteItemRequest | ||
| { | ||
| TableName = _fixture.TableName, | ||
| Key = new Dictionary<string, AttributeValue> | ||
| { | ||
| ["pk"] = new AttributeValue($"delret-aws-{id}"), | ||
| ["sk"] = new AttributeValue("item") | ||
| }, | ||
| ReturnValues = ReturnValue.ALL_OLD | ||
| }); |
Contributor
There was a problem hiding this comment.
These ALL_OLD benchmarks never hit the return-values path.
Each method deletes a brand-new delret-* key, and Setup() only starts the fixture. Since nothing inserts those items first, ReturnValues.ALL_OLD/AllOld will return no old item. That means Line 104 will usually stay null, and the AWS/Efficient variants also won’t measure old-value materialization at all.
Seed the target item outside the measured delete call, e.g. in per-iteration setup for these benchmarks only.
Also applies to: 93-104, 110-116
🤖 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/DeleteItemAsyncBenchmarks.cs`
around lines 77 - 87, The benchmarks currently call AwsSdkClient.DeleteItemAsync
(and the corresponding Efficient/other variants) with
ReturnValues.ALL_OLD/AllOld but never insert the target key first, so the delete
never returns the old item; update the benchmark to seed the item before the
measured delete by inserting the same key (e.g., use
_fixture.AwsSdkClient.PutItemAsync or the fixture helper that inserts items) in
a per-iteration setup step immediately before calling DeleteItemAsync in
DeleteItemAsyncBenchmarks (and do the same for the other AllOld benchmark
methods referenced), ensuring the seeded item uses the same key pattern
("delret-<id>") so ReturnValues.ALL_OLD actually returns the old item.
| }, | ||
| ReturnValues = Goa.Clients.Dynamo.Enums.ReturnValues.ALL_NEW | ||
| }); | ||
| return response.Value.Attributes; |
Contributor
There was a problem hiding this comment.
Consider adding error handling for consistency.
Unlike Goa_UpdateItem_SingleAttribute and Goa_UpdateItem_MultipleAttributes which check response.IsError, this method directly accesses response.Value.Attributes. If the operation fails, this could throw an exception.
While unlikely in a controlled benchmark environment, adding a guard would maintain consistency with other Goa benchmark methods.
Proposed fix
- return response.Value.Attributes;
+ return response.IsError ? null : response.Value.Attributes;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| return response.Value.Attributes; | |
| return response.IsError ? null : response.Value.Attributes; |
🤖 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/UpdateItemAsyncBenchmarks.cs`
at line 218, The method currently returns response.Value.Attributes directly
which can throw if the call failed; mirror the pattern used in
Goa_UpdateItem_SingleAttribute and Goa_UpdateItem_MultipleAttributes by checking
response.IsError before accessing response.Value, and handle the error (e.g.,
throw a descriptive exception or return an empty attribute set) so you don't
access Attributes when response indicates an error; locate the response variable
and the return statement and add the same guard/handling used in the other
Goa_UpdateItem_* benchmark methods.
Comment on lines
+1
to
+14
| using ErrorOr; | ||
| using Goa.Clients.Dynamo.Exceptions; | ||
| using Goa.Clients.Dynamo.Models; | ||
| using Goa.Clients.Dynamo.Operations.Query; | ||
| using Goa.Clients.Dynamo.Operations.Scan; | ||
| using Moq; | ||
|
|
||
| namespace Goa.Clients.Dynamo.Tests; | ||
|
|
||
| public class TypedExtensionTests | ||
| { | ||
| private record TestModel(string Id, string Name); | ||
|
|
||
| private static readonly DynamoItemReader<TestModel> TestReader = (ref System.Text.Json.Utf8JsonReader _) => new TestModel("", ""); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for the DynamoItemReader delegate definition
rg -n "delegate.*DynamoItemReader" --type=csRepository: Im5tu/goa
Length of output: 35
🏁 Script executed:
#!/bin/bash
# Search for DynamoItemReader definition more broadly
rg -n "DynamoItemReader" --type=cs -B 2 -A 2 | head -50Repository: Im5tu/goa
Length of output: 5368
🏁 Script executed:
#!/bin/bash
# Also check what's in the Serialization namespace
fd -e cs -path "*Serialization*" | head -20Repository: Im5tu/goa
Length of output: 221
🏁 Script executed:
#!/bin/bash
# Look for type alias or interface definition of DynamoItemReader
rg -n "(type|interface|using).*DynamoItemReader" --type=csRepository: Im5tu/goa
Length of output: 35
🏁 Script executed:
#!/bin/bash
# Search for DynamoItemReader type/class definition
rg -n "class DynamoItemReader|record DynamoItemReader|type DynamoItemReader|DynamoItemReader.*=" --type=csRepository: Im5tu/goa
Length of output: 252
🏁 Script executed:
#!/bin/bash
# Search files in the main source directory for DynamoItemReader
fd -e cs -path "*/Goa.Clients.Dynamo/*" ! -path "*Tests*" ! -path "*Benchmarks*" | xargs rg -l "DynamoItemReader"Repository: Im5tu/goa
Length of output: 538
🏁 Script executed:
#!/bin/bash
# Check the entire Serialization namespace
fd -e cs -path "*Serialization*" ! -path "*Tests*" ! -path "*Benchmarks*"Repository: Im5tu/goa
Length of output: 221
🏁 Script executed:
#!/bin/bash
# List the main source structure
find src -name "*.cs" 2>/dev/null | grep -i dynamo | head -20Repository: Im5tu/goa
Length of output: 1645
🏁 Script executed:
#!/bin/bash
# Search all C# files (not tests/benchmarks) for DynamoItemReader
find . -name "*.cs" ! -path "*Tests*" ! -path "*Benchmarks*" -exec grep -l "DynamoItemReader" {} \;Repository: Im5tu/goa
Length of output: 35
🏁 Script executed:
#!/bin/bash
# Try to find the definition - could be a type alias
rg "DynamoItemReader" --type=cs ! -path "*Tests*" ! -path "*Benchmarks*"Repository: Im5tu/goa
Length of output: 470
🏁 Script executed:
#!/bin/bash
# Search all C# files for DynamoItemReader definition (delegate, class, record, type alias)
rg -n "DynamoItemReader" --type=cs | grep -E "(delegate|class|record|using|type)" | head -20Repository: Im5tu/goa
Length of output: 35
🏁 Script executed:
#!/bin/bash
# Look specifically for it in the source directory
find src/Clients/Goa.Clients.Dynamo -name "*.cs" -exec grep -l "DynamoItemReader" {} \;Repository: Im5tu/goa
Length of output: 35
🏁 Script executed:
#!/bin/bash
# Check what's in the main Goa.Clients.Dynamo namespace
ls -la src/Clients/Goa.Clients.Dynamo/ 2>/dev/null || echo "Directory not found"Repository: Im5tu/goa
Length of output: 1760
🏁 Script executed:
#!/bin/bash
# Check Serialization directory
ls -la src/Clients/Goa.Clients.Dynamo/Serialization/Repository: Im5tu/goa
Length of output: 236
🏁 Script executed:
#!/bin/bash
# Check DynamoExtensions.cs for DynamoItemReader
cat -n src/Clients/Goa.Clients.Dynamo/DynamoExtensions.cs | head -50Repository: Im5tu/goa
Length of output: 2791
🏁 Script executed:
#!/bin/bash
# Check IDynamoClient.cs for DynamoItemReader
cat -n src/Clients/Goa.Clients.Dynamo/IDynamoClient.csRepository: Im5tu/goa
Length of output: 6315
🏁 Script executed:
#!/bin/bash
# Check Query operation files
ls -la src/Clients/Goa.Clients.Dynamo/Operations/Query/Repository: Im5tu/goa
Length of output: 430
🏁 Script executed:
#!/bin/bash
# Check Scan operation files
ls -la src/Clients/Goa.Clients.Dynamo/Operations/Scan/Repository: Im5tu/goa
Length of output: 426
🏁 Script executed:
#!/bin/bash
# Search in query operations for DynamoItemReader
rg "DynamoItemReader" src/Clients/Goa.Clients.Dynamo/Operations/Query/ --type=cs -A 3 -B 3Repository: Im5tu/goa
Length of output: 35
🏁 Script executed:
#!/bin/bash
# Check QueryResult.cs
cat -n src/Clients/Goa.Clients.Dynamo/Operations/Query/QueryResult.csRepository: Im5tu/goa
Length of output: 1624
🏁 Script executed:
#!/bin/bash
# Check ScanResult.cs
cat -n src/Clients/Goa.Clients.Dynamo/Operations/Scan/ScanResult.csRepository: Im5tu/goa
Length of output: 1619
🏁 Script executed:
#!/bin/bash
# Search entire Operations directory for DynamoItemReader
find src/Clients/Goa.Clients.Dynamo/Operations -name "*.cs" -exec grep -l "DynamoItemReader" {} \;Repository: Im5tu/goa
Length of output: 35
🏁 Script executed:
#!/bin/bash
# Check QueryRequest.cs - might have the type definition
cat -n src/Clients/Goa.Clients.Dynamo/Operations/Query/QueryRequest.csRepository: Im5tu/goa
Length of output: 3393
🏁 Script executed:
#!/bin/bash
# Check QueryResponse.cs
cat -n src/Clients/Goa.Clients.Dynamo/Operations/Query/QueryResponse.csRepository: Im5tu/goa
Length of output: 1711
🏁 Script executed:
#!/bin/bash
# Search for "delegate" keyword in all .cs files to find DynamoItemReader
rg "delegate" src/Clients/Goa.Clients.Dynamo --type=cs -A 2 -B 2Repository: Im5tu/goa
Length of output: 564
🏁 Script executed:
#!/bin/bash
# Check csproj for any source generator or build-time generation
cat src/Clients/Goa.Clients.Dynamo/Goa.Clients.Dynamo.csprojRepository: Im5tu/goa
Length of output: 646
🏁 Script executed:
#!/bin/bash
# Check Models directory
ls -la src/Clients/Goa.Clients.Dynamo/Models/Repository: Im5tu/goa
Length of output: 588
🏁 Script executed:
#!/bin/bash
# Search everywhere (including all namespaces) for DynamoItemReader with broader pattern
rg "DynamoItemReader.*=" --type=cs -B 2 -A 2Repository: Im5tu/goa
Length of output: 603
🏁 Script executed:
#!/bin/bash
# Check DynamoJsonContext.cs
cat -n src/Clients/Goa.Clients.Dynamo/Serialization/DynamoJsonContext.csRepository: Im5tu/goa
Length of output: 3806
🏁 Script executed:
#!/bin/bash
# Search with grep more broadly - maybe it's defined as a using alias
grep -r "DynamoItemReader" src/Clients/Goa.Clients.Dynamo --include="*.cs"Repository: Im5tu/goa
Length of output: 35
🏁 Script executed:
#!/bin/bash
# Check the entire source directory for any type/delegate/alias definition
grep -rn "type.*DynamoItemReader\|delegate.*DynamoItemReader\|using.*DynamoItemReader.*=" src/Repository: Im5tu/goa
Length of output: 35
🏁 Script executed:
#!/bin/bash
# Search in the Generator project
find src/Clients/Goa.Clients.Dynamo.Generator -name "*.cs" -exec grep -l "DynamoItemReader" {} \;Repository: Im5tu/goa
Length of output: 35
🏁 Script executed:
#!/bin/bash
# Check the full test file to see if DynamoItemReader is defined there
wc -l tests/Clients/Goa.Clients.Dynamo.Tests/TypedExtensionTests.csRepository: Im5tu/goa
Length of output: 118
🏁 Script executed:
#!/bin/bash
# Check if DynamoItemReader is defined in benchmarks
cat -n tests/Clients/Goa.Clients.Dynamo.Benchmarks/Benchmarks/GetItemAsyncBenchmarks.cs | head -30Repository: Im5tu/goa
Length of output: 1256
🏁 Script executed:
#!/bin/bash
# Search for DynamoItemReaderRegistry
rg "DynamoItemReaderRegistry" --type=cs -B 5 -A 5Repository: Im5tu/goa
Length of output: 13597
🏁 Script executed:
#!/bin/bash
# Look for any global usings files
find src/Clients/Goa.Clients.Dynamo -name "GlobalUsings.cs" -o -name "*.cs" | xargs grep -l "global using"Repository: Im5tu/goa
Length of output: 37
🏁 Script executed:
#!/bin/bash
# Check full TypedExtensionTests file to see all imports
cat -n tests/Clients/Goa.Clients.Dynamo.Tests/TypedExtensionTests.cs | head -50Repository: Im5tu/goa
Length of output: 2279
🏁 Script executed:
#!/bin/bash
# Search for QueryAllAsync definition
rg "QueryAllAsync" --type=cs -B 3 -A 5Repository: Im5tu/goa
Length of output: 6538
🏁 Script executed:
#!/bin/bash
# Search for definition of DynamoItemReaderRegistry
rg "DynamoItemReaderRegistry" --type=cs -B 5 -A 10Repository: Im5tu/goa
Length of output: 19276
🏁 Script executed:
#!/bin/bash
# Look for overloads of QueryAsync with DynamoItemReader parameter
rg "QueryAsync.*DynamoItemReader" --type=cs -B 2 -A 2Repository: Im5tu/goa
Length of output: 1862
🏁 Script executed:
#!/bin/bash
# Check full DynamoExtensions.cs to see all QueryAllAsync overloads
wc -l src/Clients/Goa.Clients.Dynamo/DynamoExtensions.csRepository: Im5tu/goa
Length of output: 107
🏁 Script executed:
#!/bin/bash
# Get lines around QueryAllAsync definitions
rg -n "QueryAllAsync|QueryAsync|ScanAsync" src/Clients/Goa.Clients.Dynamo/DynamoExtensions.cs | head -30Repository: Im5tu/goa
Length of output: 945
Missing core API implementation for generic typed operations.
The test file references DynamoItemReader<T> and several generic overloads (QueryAsync<T>, ScanAsync<T>, QueryAllAsync<T>, ScanAllAsync<T>) that do not exist in the codebase. These types and methods need to be implemented:
- Define the
DynamoItemReader<T>delegate type - Add generic
QueryAsync<T>andScanAsync<T>overloads toIDynamoClientaccepting aDynamoItemReader<T>parameter - Add generic
QueryAllAsync<T>andScanAllAsync<T>extension methods to handle typed pagination - Implement
QueryResult<T>andScanResult<T>generic result types if not already present
The test expectations assume a fully typed, strongly-typed Query/Scan API that is not yet implemented. Adding a using directive will not resolve the compilation errors.
🧰 Tools
🪛 GitHub Check: build
[failure] 14-14:
The type or namespace name 'DynamoItemReader<>' could not be found (are you missing a using directive or an assembly reference?)
[failure] 14-14:
The type or namespace name 'DynamoItemReader<>' could not be found (are you missing a using directive or an assembly reference?)
🤖 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 1
- 14, Add the missing typed API: declare the generic delegate
DynamoItemReader<T>, add generic overloads on IDynamoClient for
QueryAsync<T>(..., DynamoItemReader<T>) and ScanAsync<T>(...,
DynamoItemReader<T>), create generic result types QueryResult<T> and
ScanResult<T> (mirroring existing non-generic results), and implement extension
methods QueryAllAsync<T> and ScanAllAsync<T> that use the new
QueryAsync<T>/ScanAsync<T> and the paginated result types to yield typed
pages/items; update implementations to reuse the existing non-generic logic
where possible so behavior matches current Query/Scan flows.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
tests/Clients/Goa.Clients.Dynamo.Benchmarks/Benchmarks/QueryAsyncBenchmarks.cs (3)
44-47: Drop the unusedquery-10/query-100seeding from setup.All active benchmarks in this file query
query-1; the extra seeds are only referenced by the commented-out code below. That adds 110 unnecessary writes to every benchmark job's setup.♻️ Suggested cleanup
// Seed items for query benchmarks _fixture.SeedItemsAsync("query-1", 1).GetAwaiter().GetResult(); - _fixture.SeedItemsAsync("query-10", 10).GetAwaiter().GetResult(); - _fixture.SeedItemsAsync("query-100", 100).GetAwaiter().GetResult();🤖 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 44 - 47, Remove the unnecessary seeding calls that write 10 and 100 items in the QueryAsyncBenchmarks setup: delete the two lines calling _fixture.SeedItemsAsync("query-10", 10).GetAwaiter().GetResult() and _fixture.SeedItemsAsync("query-100", 100).GetAwaiter().GetResult() inside the QueryAsyncBenchmarks setup so only the active "query-1" seed remains; this eliminates the 110 extra writes while keeping _fixture.SeedItemsAsync usage and existing setup logic intact.
136-468: Remove the retired benchmarks instead of keeping them commented out.This block is dead code now. Leaving old benchmark bodies in comments makes the file much harder to scan and guarantees they’ll drift from the current API surface.
🤖 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 136 - 468, The large block of commented-out benchmark methods (e.g., AwsSdk_Query_10Items, Goa_Query_10Items_DynamoRecord, Goa_Query_10Items_Typed, Efficient_Query_10Items, Efficient_Query_10Items_Typed and their 100-item and No Results variants) is dead code and should be removed; delete the entire commented benchmark bodies so the QueryAsyncBenchmarks class contains only active benchmarks and no outdated commented implementations, ensuring no leftover commented methods like AwsSdk_Query_100Items, Goa_Query_NoResults_Typed, Efficient_Query_NoResults, etc., remain.
13-30: Reuse the sharedBenchmarkEntitymodel instead of redefining it here.There is already a
BenchmarkEntitywith the same DynamoDB mapping intests/Clients/Goa.Clients.Dynamo.Benchmarks/Models/BenchmarkEntity.cs. Keeping two copies makes typed benchmark mappings easy to drift the next time the schema changes.🤖 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 13 - 30, Remove the duplicate BenchmarkEntity class defined in QueryAsyncBenchmarks.cs and use the shared BenchmarkEntity model instead: delete the local class declaration (the class named BenchmarkEntity with properties Pk, Sk, Data, Number, Status and DynamoDbProperty/DynamoDbTable attributes), add the appropriate using/import for the existing shared model's namespace if needed, and ensure any code in QueryAsyncBenchmarks references the shared BenchmarkEntity type rather than the removed local definition so mappings stay consistent with Models/BenchmarkEntity.
🤖 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 44-47: Remove the unnecessary seeding calls that write 10 and 100
items in the QueryAsyncBenchmarks setup: delete the two lines calling
_fixture.SeedItemsAsync("query-10", 10).GetAwaiter().GetResult() and
_fixture.SeedItemsAsync("query-100", 100).GetAwaiter().GetResult() inside the
QueryAsyncBenchmarks setup so only the active "query-1" seed remains; this
eliminates the 110 extra writes while keeping _fixture.SeedItemsAsync usage and
existing setup logic intact.
- Around line 136-468: The large block of commented-out benchmark methods (e.g.,
AwsSdk_Query_10Items, Goa_Query_10Items_DynamoRecord, Goa_Query_10Items_Typed,
Efficient_Query_10Items, Efficient_Query_10Items_Typed and their 100-item and No
Results variants) is dead code and should be removed; delete the entire
commented benchmark bodies so the QueryAsyncBenchmarks class contains only
active benchmarks and no outdated commented implementations, ensuring no
leftover commented methods like AwsSdk_Query_100Items,
Goa_Query_NoResults_Typed, Efficient_Query_NoResults, etc., remain.
- Around line 13-30: Remove the duplicate BenchmarkEntity class defined in
QueryAsyncBenchmarks.cs and use the shared BenchmarkEntity model instead: delete
the local class declaration (the class named BenchmarkEntity with properties Pk,
Sk, Data, Number, Status and DynamoDbProperty/DynamoDbTable attributes), add the
appropriate using/import for the existing shared model's namespace if needed,
and ensure any code in QueryAsyncBenchmarks references the shared
BenchmarkEntity type rather than the removed local definition so mappings stay
consistent with Models/BenchmarkEntity.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c233b1f0-68a3-43fa-bf5a-6dcfc35d8525
📒 Files selected for processing (2)
tests/Clients/Goa.Clients.Dynamo.Benchmarks/Benchmarks/QueryAsyncBenchmarks.cstests/Clients/Goa.Clients.Dynamo.Benchmarks/Benchmarks/ScanAsyncBenchmarks.cs
Strip GetItem typed benchmarks using BenchmarkItem/DynamoItemReaderRegistry and delete TypedExtensionTests using DynamoItemReader<T>/AttributeValue factories.
…gs, empty strings - ComplexTypeHandler: differentiate non-nullable structs (no null check), nullable structs (.Value unwrap), and reference types - DateOnly/DateTime/TimeOnly handlers: use GetDynamoAttributeName() for dictionary key in conditional write path - PrimitiveTypeHandler: fix over-escaped DateTime format strings - DynamoRecordExtensions: allow empty strings through TryGetString
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 (5)
src/Clients/Goa.Clients.Dynamo.Generator/TypeHandlers/DateOnlyTypeHandler.cs (1)
30-66:⚠️ Potential issue | 🟠 MajorMirror the serialized attribute name in
GenerateFromDynamoRecord.The nullable write path now uses
GetDynamoAttributeName(), but the read path still queriespropertyInfo.Name. AnyDateOnlyproperty with a configured Dynamo name will be written and read under different keys.Possible fix
- var memberName = propertyInfo.Name; + var memberName = propertyInfo.GetDynamoAttributeName(); var isNullable = propertyInfo.IsNullable; // Avoid variable name conflicts with pk/sk extraction variables - var varName = GetSafeVariableName(memberName); + var varName = GetSafeVariableName(propertyInfo.Name);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Clients/Goa.Clients.Dynamo.Generator/TypeHandlers/DateOnlyTypeHandler.cs` around lines 30 - 66, GenerateFromDynamoRecord is using propertyInfo.Name when reading the attribute but the write path uses propertyInfo.GetDynamoAttributeName(); update GenerateFromDynamoRecord to call propertyInfo.GetDynamoAttributeName() (store in a local like dynamoName) and use that value in the TryGetNullableString/TryGetString calls and in the MissingAttributeException message so reads and writes use the same Dynamo attribute key; keep existing nullability handling and pkVariable/skVariable usage unchanged.src/Clients/Goa.Clients.Dynamo.Generator/TypeHandlers/TimeOnlyTypeHandler.cs (1)
30-66:⚠️ Potential issue | 🟠 MajorMirror the serialized attribute name in
GenerateFromDynamoRecord.The nullable write path now uses
GetDynamoAttributeName(), but the read path still queriespropertyInfo.Name. AnyTimeOnlyproperty with a configured Dynamo name will be written and read under different keys.Possible fix
- var memberName = propertyInfo.Name; + var memberName = propertyInfo.GetDynamoAttributeName(); var isNullable = propertyInfo.IsNullable; // Avoid variable name conflicts with pk/sk extraction variables - var varName = GetSafeVariableName(memberName); + var varName = GetSafeVariableName(propertyInfo.Name);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Clients/Goa.Clients.Dynamo.Generator/TypeHandlers/TimeOnlyTypeHandler.cs` around lines 30 - 66, The read logic in GenerateFromDynamoRecord uses propertyInfo.Name for the attribute key while the write/conditional paths use propertyInfo.GetDynamoAttributeName(), causing mismatched keys; update GenerateFromDynamoRecord so both nullable and non-nullable branches call propertyInfo.GetDynamoAttributeName() (store it in a local like var dynamoName = propertyInfo.GetDynamoAttributeName()) and use that variable in the TryGetString/TryGetNullableString calls and the MissingAttributeException.Throw message to ensure reads use the same serialized attribute name as writes.src/Clients/Goa.Clients.Dynamo.Generator/TypeHandlers/DateTimeTypeHandler.cs (1)
40-69:⚠️ Potential issue | 🟠 MajorUse the configured Dynamo attribute name on the read path too.
This handler now writes with
GetDynamoAttributeName(), butGenerateFromDynamoRecordstill looks uppropertyInfo.Name. A renamedDateTime/DateTimeOffsetproperty will be serialized under one key and deserialized from another.Possible fix
- var memberName = propertyInfo.Name; + var memberName = propertyInfo.GetDynamoAttributeName(); var underlyingType = propertyInfo.UnderlyingType; var isNullable = propertyInfo.IsNullable; // Avoid variable name conflicts with pk/sk extraction variables - var varName = GetSafeVariableName(memberName); + var varName = GetSafeVariableName(propertyInfo.Name);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Clients/Goa.Clients.Dynamo.Generator/TypeHandlers/DateTimeTypeHandler.cs` around lines 40 - 69, The read path in GenerateFromDynamoRecord is using propertyInfo.Name (memberName) to build the TryGet... lookup keys, which will not match writes that use propertyInfo.GetDynamoAttributeName(); update GenerateFromDynamoRecord to call propertyInfo.GetDynamoAttributeName() (store as dynamoAttributeName) and use that variable in all TryGetNullableDateTime/TryGetDateTime/TryGetNullableDateTimeOffset/TryGetDateTimeOffset lookup string arguments and in the MissingAttributeException.Throw calls so deserialization uses the same Dynamo attribute key as serialization.src/Clients/Goa.Clients.Dynamo.Generator/TypeHandlers/PrimitiveTypeHandler.cs (1)
146-200:⚠️ Potential issue | 🟠 MajorDon't drop empty strings in the generated write path.
TryGetStringnow treats""as a valid value, but this condition still suppresses writing it. An empty string will round-trip as a missing attribute, and the non-nullable read path then throwsMissingAttributeException.Possible fix
- // String types (both nullable and non-nullable) need conditional assignment to skip empty strings + // String types still need conditional assignment so null strings are omitted var isString = underlyingType.SpecialType == SpecialType.System_String; @@ - var condition = underlyingType.SpecialType == SpecialType.System_String - ? $"!string.IsNullOrEmpty(model.{propertyName})" + var condition = underlyingType.SpecialType == SpecialType.System_String + ? $"model.{propertyName} != null" : $"model.{propertyName}.HasValue";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Clients/Goa.Clients.Dynamo.Generator/TypeHandlers/PrimitiveTypeHandler.cs` around lines 146 - 200, The generated write path currently suppresses empty strings because GenerateConditionalAssignment builds the string condition using !string.IsNullOrEmpty; change the string branch to only check for null (e.g., model.{propertyName} != null) so empty strings are written; update the condition variable construction in GenerateConditionalAssignment (referencing condition, attributeValue, propertyName, recordVariable, dynamoAttributeName) to use a null check for SpecialType.System_String while keeping HasValue for other nullable types.src/Clients/Goa.Clients.Dynamo/Extensions/DynamoRecordExtensions.cs (1)
256-295:⚠️ Potential issue | 🟠 MajorParse floating-point and decimal numbers with
InvariantCulture.The generator serializes
double,float, anddecimalvalues withCultureInfo.InvariantCulture, but the reader extension methods use culture-sensitiveTryParseoverloads. On systems using,as the decimal separator, persisted values like1.23will fail to parse or be interpreted incorrectly.Fix by passing
CultureInfo.InvariantCulturewith appropriateNumberStylesto allTryParsecalls:
double/float: UseNumberStyles.Float | NumberStyles.AllowThousandsdecimal: UseNumberStyles.NumberApply to:
TryGetDouble,TryGetNullableDouble,TryGetFloat,TryGetNullableFloat,TryGetDecimal,TryGetNullableDecimal, andTryGetDoubleSet.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Clients/Goa.Clients.Dynamo/Extensions/DynamoRecordExtensions.cs` around lines 256 - 295, The parsing methods currently use culture-sensitive TryParse overloads; update all TryParse calls in TryGetDouble, TryGetNullableDouble, TryGetFloat, TryGetNullableFloat, TryGetDecimal, TryGetNullableDecimal, and TryGetDoubleSet to use the overloads that accept NumberStyles and CultureInfo.InvariantCulture: for double/float use NumberStyles.Float | NumberStyles.AllowThousands with CultureInfo.InvariantCulture, and for decimal use NumberStyles.Number with CultureInfo.InvariantCulture; ensure null/empty checks remain and set out values exactly as before on successful parse.
🤖 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.Generator/TypeHandlers/DateOnlyTypeHandler.cs`:
- Around line 30-66: GenerateFromDynamoRecord is using propertyInfo.Name when
reading the attribute but the write path uses
propertyInfo.GetDynamoAttributeName(); update GenerateFromDynamoRecord to call
propertyInfo.GetDynamoAttributeName() (store in a local like dynamoName) and use
that value in the TryGetNullableString/TryGetString calls and in the
MissingAttributeException message so reads and writes use the same Dynamo
attribute key; keep existing nullability handling and pkVariable/skVariable
usage unchanged.
In
`@src/Clients/Goa.Clients.Dynamo.Generator/TypeHandlers/DateTimeTypeHandler.cs`:
- Around line 40-69: The read path in GenerateFromDynamoRecord is using
propertyInfo.Name (memberName) to build the TryGet... lookup keys, which will
not match writes that use propertyInfo.GetDynamoAttributeName(); update
GenerateFromDynamoRecord to call propertyInfo.GetDynamoAttributeName() (store as
dynamoAttributeName) and use that variable in all
TryGetNullableDateTime/TryGetDateTime/TryGetNullableDateTimeOffset/TryGetDateTimeOffset
lookup string arguments and in the MissingAttributeException.Throw calls so
deserialization uses the same Dynamo attribute key as serialization.
In
`@src/Clients/Goa.Clients.Dynamo.Generator/TypeHandlers/PrimitiveTypeHandler.cs`:
- Around line 146-200: The generated write path currently suppresses empty
strings because GenerateConditionalAssignment builds the string condition using
!string.IsNullOrEmpty; change the string branch to only check for null (e.g.,
model.{propertyName} != null) so empty strings are written; update the condition
variable construction in GenerateConditionalAssignment (referencing condition,
attributeValue, propertyName, recordVariable, dynamoAttributeName) to use a null
check for SpecialType.System_String while keeping HasValue for other nullable
types.
In
`@src/Clients/Goa.Clients.Dynamo.Generator/TypeHandlers/TimeOnlyTypeHandler.cs`:
- Around line 30-66: The read logic in GenerateFromDynamoRecord uses
propertyInfo.Name for the attribute key while the write/conditional paths use
propertyInfo.GetDynamoAttributeName(), causing mismatched keys; update
GenerateFromDynamoRecord so both nullable and non-nullable branches call
propertyInfo.GetDynamoAttributeName() (store it in a local like var dynamoName =
propertyInfo.GetDynamoAttributeName()) and use that variable in the
TryGetString/TryGetNullableString calls and the MissingAttributeException.Throw
message to ensure reads use the same serialized attribute name as writes.
In `@src/Clients/Goa.Clients.Dynamo/Extensions/DynamoRecordExtensions.cs`:
- Around line 256-295: The parsing methods currently use culture-sensitive
TryParse overloads; update all TryParse calls in TryGetDouble,
TryGetNullableDouble, TryGetFloat, TryGetNullableFloat, TryGetDecimal,
TryGetNullableDecimal, and TryGetDoubleSet to use the overloads that accept
NumberStyles and CultureInfo.InvariantCulture: for double/float use
NumberStyles.Float | NumberStyles.AllowThousands with
CultureInfo.InvariantCulture, and for decimal use NumberStyles.Number with
CultureInfo.InvariantCulture; ensure null/empty checks remain and set out values
exactly as before on successful parse.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0dfab218-5251-4340-aa8f-b4e4ebc91a3d
📒 Files selected for processing (6)
src/Clients/Goa.Clients.Dynamo.Generator/TypeHandlers/ComplexTypeHandler.cssrc/Clients/Goa.Clients.Dynamo.Generator/TypeHandlers/DateOnlyTypeHandler.cssrc/Clients/Goa.Clients.Dynamo.Generator/TypeHandlers/DateTimeTypeHandler.cssrc/Clients/Goa.Clients.Dynamo.Generator/TypeHandlers/PrimitiveTypeHandler.cssrc/Clients/Goa.Clients.Dynamo.Generator/TypeHandlers/TimeOnlyTypeHandler.cssrc/Clients/Goa.Clients.Dynamo/Extensions/DynamoRecordExtensions.cs
Resolves AttributeValue conflict — absorbs PR #68's B/BS binary support into the readonly struct union layout with factory methods (FromBinary, FromBinarySet) and updates DynamoResponseReader, AttributeValueJsonConverter to use immutable construction.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/Clients/Goa.Clients.Dynamo/Models/AttributeValue.cs (1)
187-192:⚠️ Potential issue | 🟡 MinorPotential null dereference if JSON contains
nullvalue.
reader.GetString()!uses null-forgiving operator, but if the JSON is{"S": null}, this would returnnulland pass it toAttributeValue.String(null). The same pattern exists for "N" (line 197), "B" (line 258), and string arrays (lines 220, 229, 248, 267).Consider adding defensive null checks or documenting the expected behavior.
🤖 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 187 - 192, The code uses reader.GetString()! (e.g. inside the reader.ValueTextEquals("S"u8) branch that then calls AttributeValue.String(v)) and similar calls for "N", "B" and the string-array branches without guarding against a JSON null token; add defensive null/TokenType checks after reader.Read() and before calling AttributeValue.String/Number/Binary or adding to arrays (or explicitly handle JSON null by returning an appropriate AttributeValue.Null/throwing a clear exception). Update the branches that check "S", "N", "B" and the array-handling code (the locations calling reader.GetString(), reader.GetDouble()/GetBytes(), and the loops that Add to string lists) to validate the reader.TokenType is not JsonTokenType.Null and handle that case cleanly with a descriptive exception or null-value mapping.
🧹 Nitpick comments (4)
src/Clients/Goa.Clients.Dynamo/Models/AttributeValue.cs (3)
151-168:ToValue<T>doesn't support List and Map types.The method handles primitive types but doesn't support
List<AttributeValue>orDictionary<string, AttributeValue>. Callers must accessLandMproperties directly.This may be intentional, but consider adding these for API consistency or documenting the limitation.
🤖 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 151 - 168, ToValue<T>() currently converts primitives but omits handling for collection/map types; add support for List<AttributeValue> and Dictionary<string, AttributeValue> by recognizing typeof(T) == typeof(List<AttributeValue>) and typeof(T) == typeof(Dictionary<string, AttributeValue>) (or their interfaces) and returning the L and M properties cast to T (e.g., (T?)(object?)L and (T?)(object?)M) so callers can get typed lists/maps from AttributeValue without manually accessing L and M; ensure null/AttributeType.Null handling remains consistent and update unit tests around the ToValue<T> method in the AttributeValue class.
119-144: Implicit conversions accept null without producing Null type.The implicit operators for
string,byte[],List<byte[]>, andList<string>accept null values but create AttributeValues with their respective types (String, Binary, etc.) rather thanAttributeType.Null. This meansAttributeValue av = (string)null;createsType=String, S=nullinstead ofType=Null.Consider documenting this behavior or adding null checks that return
Null()for null inputs if that's the intended semantic.🤖 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 119 - 144, The implicit conversion operators for string, byte[], List<byte[]>, and List<string> should treat null as an AttributeType.Null; update the operators (the methods declared as "public static implicit operator AttributeValue(string value)", "public static implicit operator AttributeValue(byte[] value)", "public static implicit operator AttributeValue(List<byte[]> value)", and "public static implicit operator AttributeValue(List<string> value)") to check for null and return Null() when value is null, otherwise call the existing helpers (String(...), FromBinary(...), FromBinarySet(...), FromStringSet(...)); alternatively, if you intend to keep the current behavior, add a brief XML doc comment to those operator declarations documenting that null inputs produce a typed AttributeValue with a null payload instead of AttributeType.Null.
12-18: Explicit struct layout assumes 64-bit pointer size.The
FieldOffset(8)for_typeassumes an 8-byte reference at offset 0. On 32-bit runtimes,object?is 4 bytes, so_typecould start at offset 4 without overlap. While .NET 5+ primarily targets 64-bit, if 32-bit support is ever needed, this layout would waste 4 bytes per instance.This is acceptable for current targets, but consider documenting this assumption.
🤖 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 12 - 18, The explicit FieldOffset usage in the AttributeValue struct assumes 64-bit pointers (FieldOffset(8) for _type after _referenceValue) which can be incorrect on 32-bit runtimes; update the code by adding a clear XML/doc comment on the AttributeValue struct explaining the 64-bit pointer-size assumption and why FieldOffset(8) is used, and add a runtime guard (e.g., a static constructor or Debug.Assert checking IntPtr.Size == 8) referencing AttributeValue, _referenceValue and _type so the mismatch is detected early; this documents the limitation and fails fast if ever run on a 32-bit runtime.src/Clients/Goa.Clients.Dynamo.Generator/TypeHandlers/ComplexTypeHandler.cs (1)
342-363: Dictionary string values may pass null toAttributeValue.String().Line 346 generates
AttributeValue.String(kvp.Value)without checking ifkvp.Valueis null. If a dictionary contains null values, this creates an AttributeValue withType=StringbutS=null, which serializes as{"S": null}- valid JSON but potentially unexpected DynamoDB behavior.Consider adding null handling similar to line 81's nested dictionary pattern.
Proposed fix
if (valueType.SpecialType == SpecialType.System_String) { - return $"model.{propertyName} != null ? AttributeValue.FromMap(model.{propertyName}.ToDictionary(kvp => kvp.Key, kvp => AttributeValue.String(kvp.Value))) : AttributeValue.Null()"; + return $"model.{propertyName} != null ? AttributeValue.FromMap(model.{propertyName}.ToDictionary(kvp => kvp.Key, kvp => kvp.Value != null ? AttributeValue.String(kvp.Value) : AttributeValue.Null())) : AttributeValue.Null()"; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Clients/Goa.Clients.Dynamo.Generator/TypeHandlers/ComplexTypeHandler.cs` around lines 342 - 363, In GeneratePrimitiveDictionary, handle null dictionary values before calling AttributeValue.String/Number so you don't create AttributeValue with a null S; update each ToDictionary value selector for strings, numerics, DateTime and enums to use a conditional like kvp.Value == null ? AttributeValue.Null() : AttributeValue.String(...) (or AttributeValue.Number(...) for numerics) so null entries map to AttributeValue.Null(); modify the value selector expressions inside GeneratePrimitiveDictionary (referencing propertyName and valueType) accordingly to mirror the nested-dictionary null-check pattern used elsewhere.
🤖 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 217-222: The code in DynamoResponseReader currently treats
malformed or unsupported input as valid AttributeValue by discarding the NULL
boolean (reader.GetBoolean) and returning AttributeValue.Null(), and by
returning default(AttributeValue) in other paths; instead, validate the boolean
and fail fast: in the branch that checks reader.ValueTextEquals("NULL"u8) call
reader.GetBoolean() and if it is not true throw a JsonException describing the
unexpected NULL value, ensure the reader is advanced correctly (consume
EndObject) only on valid TRUE, and replace any return default paths (returning
default AttributeValue) with throwing JsonException for unsupported/malformed
wire data so only legitimate factory-produced AttributeValue instances are
returned.
In `@src/Clients/Goa.Clients.Dynamo/Models/AttributeValue.cs`:
- Around line 51-67: The factory methods on AttributeValue (FromStringSet,
FromNumberSet, FromList, FromMap, FromBinary, FromBinarySet) currently accept
nulls and later cause NullReferenceExceptions in the Write method; add defensive
null validation in each factory (e.g., throw ArgumentNullException when the
provided value is null) so AttributeValue instances always contain non-null
payloads, or alternatively modify AttributeValue.Write to use null-conditional
checks before accessing collection/array properties—update the
constructors/factory methods in the AttributeValue class to validate input and
reference the Write method implementation to ensure consistency.
---
Duplicate comments:
In `@src/Clients/Goa.Clients.Dynamo/Models/AttributeValue.cs`:
- Around line 187-192: The code uses reader.GetString()! (e.g. inside the
reader.ValueTextEquals("S"u8) branch that then calls AttributeValue.String(v))
and similar calls for "N", "B" and the string-array branches without guarding
against a JSON null token; add defensive null/TokenType checks after
reader.Read() and before calling AttributeValue.String/Number/Binary or adding
to arrays (or explicitly handle JSON null by returning an appropriate
AttributeValue.Null/throwing a clear exception). Update the branches that check
"S", "N", "B" and the array-handling code (the locations calling
reader.GetString(), reader.GetDouble()/GetBytes(), and the loops that Add to
string lists) to validate the reader.TokenType is not JsonTokenType.Null and
handle that case cleanly with a descriptive exception or null-value mapping.
---
Nitpick comments:
In `@src/Clients/Goa.Clients.Dynamo.Generator/TypeHandlers/ComplexTypeHandler.cs`:
- Around line 342-363: In GeneratePrimitiveDictionary, handle null dictionary
values before calling AttributeValue.String/Number so you don't create
AttributeValue with a null S; update each ToDictionary value selector for
strings, numerics, DateTime and enums to use a conditional like kvp.Value ==
null ? AttributeValue.Null() : AttributeValue.String(...) (or
AttributeValue.Number(...) for numerics) so null entries map to
AttributeValue.Null(); modify the value selector expressions inside
GeneratePrimitiveDictionary (referencing propertyName and valueType) accordingly
to mirror the nested-dictionary null-check pattern used elsewhere.
In `@src/Clients/Goa.Clients.Dynamo/Models/AttributeValue.cs`:
- Around line 151-168: ToValue<T>() currently converts primitives but omits
handling for collection/map types; add support for List<AttributeValue> and
Dictionary<string, AttributeValue> by recognizing typeof(T) ==
typeof(List<AttributeValue>) and typeof(T) == typeof(Dictionary<string,
AttributeValue>) (or their interfaces) and returning the L and M properties cast
to T (e.g., (T?)(object?)L and (T?)(object?)M) so callers can get typed
lists/maps from AttributeValue without manually accessing L and M; ensure
null/AttributeType.Null handling remains consistent and update unit tests around
the ToValue<T> method in the AttributeValue class.
- Around line 119-144: The implicit conversion operators for string, byte[],
List<byte[]>, and List<string> should treat null as an AttributeType.Null;
update the operators (the methods declared as "public static implicit operator
AttributeValue(string value)", "public static implicit operator
AttributeValue(byte[] value)", "public static implicit operator
AttributeValue(List<byte[]> value)", and "public static implicit operator
AttributeValue(List<string> value)") to check for null and return Null() when
value is null, otherwise call the existing helpers (String(...),
FromBinary(...), FromBinarySet(...), FromStringSet(...)); alternatively, if you
intend to keep the current behavior, add a brief XML doc comment to those
operator declarations documenting that null inputs produce a typed
AttributeValue with a null payload instead of AttributeType.Null.
- Around line 12-18: The explicit FieldOffset usage in the AttributeValue struct
assumes 64-bit pointers (FieldOffset(8) for _type after _referenceValue) which
can be incorrect on 32-bit runtimes; update the code by adding a clear XML/doc
comment on the AttributeValue struct explaining the 64-bit pointer-size
assumption and why FieldOffset(8) is used, and add a runtime guard (e.g., a
static constructor or Debug.Assert checking IntPtr.Size == 8) referencing
AttributeValue, _referenceValue and _type so the mismatch is detected early;
this documents the limitation and fails fast if ever run on a 32-bit runtime.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cf422f47-ca58-408f-8bfc-f1a143ccdc6b
📒 Files selected for processing (9)
src/Clients/Goa.Clients.Dynamo.Generator/TypeHandlers/ComplexTypeHandler.cssrc/Clients/Goa.Clients.Dynamo.Generator/TypeHandlers/DateOnlyTypeHandler.cssrc/Clients/Goa.Clients.Dynamo.Generator/TypeHandlers/DateTimeTypeHandler.cssrc/Clients/Goa.Clients.Dynamo.Generator/TypeHandlers/PrimitiveTypeHandler.cssrc/Clients/Goa.Clients.Dynamo.Generator/TypeHandlers/TimeOnlyTypeHandler.cssrc/Clients/Goa.Clients.Dynamo/Extensions/DynamoRecordExtensions.cssrc/Clients/Goa.Clients.Dynamo/Internal/DynamoResponseReader.cssrc/Clients/Goa.Clients.Dynamo/Models/AttributeType.cssrc/Clients/Goa.Clients.Dynamo/Models/AttributeValue.cs
🚧 Files skipped from review as they are similar to previous changes (4)
- src/Clients/Goa.Clients.Dynamo.Generator/TypeHandlers/DateOnlyTypeHandler.cs
- src/Clients/Goa.Clients.Dynamo.Generator/TypeHandlers/TimeOnlyTypeHandler.cs
- src/Clients/Goa.Clients.Dynamo/Models/AttributeType.cs
- src/Clients/Goa.Clients.Dynamo.Generator/TypeHandlers/DateTimeTypeHandler.cs
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 `@src/Clients/Goa.Clients.Dynamo/Models/AttributeValue.cs`:
- Around line 177-180: The Read override in AttributeValue currently returns
default on unexpected JSON tokens, which masks malformed/unsupported payloads;
change all fallback returns (e.g. the initial check in Read(ref Utf8JsonReader
reader, Type typeToConvert, JsonSerializerOptions options) and the similar
fallback branch later around the second Read block) to throw a JsonException
with a descriptive message instead of returning default so deserialization fails
loudly for invalid input; update both places that return default to throw new
JsonException($"Invalid AttributeValue JSON: unexpected token
{reader.TokenType}") (or similar) so callers can detect bad payloads.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 91179a65-acc9-4c92-99c7-d105e00229fa
📒 Files selected for processing (1)
src/Clients/Goa.Clients.Dynamo/Models/AttributeValue.cs
Resolve conflicts: use factory methods (readonly struct) with collectionVarName parameterization from main, and update tests to use factory-style AttributeValue construction.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/Clients/Goa.Clients.Dynamo.Generator/TypeHandlers/CollectionTypeHandler.cs (1)
71-71:⚠️ Potential issue | 🔴 CriticalMirror the struct/nullability branch from
ComplexTypeHandler.Line 71 still emits
item != nullfor every complex element. That is invalid forList<MyStruct>, andList<MyStruct?>will also targetDynamoMapper.Nullableunless the element type is unwrapped before building the mapper name.#!/bin/bash # Expectation: # 1) ComplexTypeHandler already differentiates non-nullable structs, nullable structs, and reference types. # 2) CollectionTypeHandler still emits a single `item != null` branch for all complex collection elements. # 3) CollectionTypeHandlerTests do not appear to cover struct collection elements yet. sed -n '92,111p' src/Clients/Goa.Clients.Dynamo.Generator/TypeHandlers/ComplexTypeHandler.cs sed -n '64,72p' src/Clients/Goa.Clients.Dynamo.Generator/TypeHandlers/CollectionTypeHandler.cs rg -n -C2 'Struct|Nullable<' tests/Clients/Goa.Clients.Dynamo.Generator.Tests/TypeHandlers/CollectionTypeHandlerTests.cs🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Clients/Goa.Clients.Dynamo.Generator/TypeHandlers/CollectionTypeHandler.cs` at line 71, CollectionTypeHandler currently emits a single null-check branch using itemVarName and DynamoMapper.{normalizedTypeName} for all complex element types; update its logic to mirror ComplexTypeHandler by distinguishing between non-nullable structs, nullable structs and reference types: if the element is a non-nullable struct emit direct AttributeValue.FromMap(DynamoMapper.{normalizedTypeName}.ToDynamoRecord(...)) without an item != null branch; if the element is a nullable struct unwrap the Nullable<T> to get the inner type name and use DynamoMapper.Nullable{InnerTypeName} (and check HasValue/Value instead of item != null); for reference types keep the item != null ? AttributeValue.FromMap(DynamoMapper.{normalizedTypeName}.ToDynamoRecord(item)) : AttributeValue.Null() pattern. Locate and modify the string construction in CollectionTypeHandler where normalizedTypeName and itemVarName are used so the mapper name and null-check match the element's nullability, reusing the same type-inspection logic/patterns from ComplexTypeHandler.
🤖 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/TypeHandlers/CollectionTypeHandler.cs`:
- Around line 51-52: The read path is dropping NULL elements because
GenerateFromDynamoRecord filters complex collection items with Where(item =>
item.M != null); change it to preserve placeholders by removing that filter and
instead map each AttributeValue: for each item in the Dynamo list, if the
AttributeValue is Null() map to null (for List<T?>) otherwise map the item.M
using the existing element mapping logic; update the code that currently uses
Where(item => item.M != null) to a Select that returns null for
AttributeValue.Null() and the mapped instance for non-null entries so
List<MyClass?> keeps the same length/order after round trip.
In `@src/Clients/Goa.Clients.Dynamo.Generator/TypeHandlers/ComplexTypeHandler.cs`:
- Line 346: The generated map branch currently passes string values directly
into AttributeValue.String which breaks for nulls; in the return expression that
builds AttributeValue.FromMap (the line referencing propertyName and
AttributeValue.FromMap/AttributeValue.String), change the ToDictionary value
selector to produce kvp.Value != null ? AttributeValue.String(kvp.Value) :
AttributeValue.Null() so null string entries become AttributeValue.Null()
instead of an invalid string attribute.
In
`@tests/Clients/Goa.Clients.Dynamo.Generator.Tests/TypeHandlers/CollectionTypeHandlerTests.cs`:
- Around line 474-475: The two tests
GenerateToAttributeValue_NestedStringCollection_ShouldUseCorrectInnerVariableName
and
GenerateToAttributeValue_NestedIntCollection_ShouldUseCorrectInnerVariableName
still assert the old "new AttributeValue { ... }" output; update their expected
strings to match the new factory style (e.g., use
AttributeValue.FromList(...)/AttributeValue.FromNumber(...) patterns) so they
assert the generated output that contains "AttributeValue.FromList(" (also
update the analogous assertions around the other occurrences noted at lines
~503-504); change only the expected string constants in those tests to the
factory-based representation used elsewhere in this file so the assertions match
the generator output.
---
Duplicate comments:
In
`@src/Clients/Goa.Clients.Dynamo.Generator/TypeHandlers/CollectionTypeHandler.cs`:
- Line 71: CollectionTypeHandler currently emits a single null-check branch
using itemVarName and DynamoMapper.{normalizedTypeName} for all complex element
types; update its logic to mirror ComplexTypeHandler by distinguishing between
non-nullable structs, nullable structs and reference types: if the element is a
non-nullable struct emit direct
AttributeValue.FromMap(DynamoMapper.{normalizedTypeName}.ToDynamoRecord(...))
without an item != null branch; if the element is a nullable struct unwrap the
Nullable<T> to get the inner type name and use
DynamoMapper.Nullable{InnerTypeName} (and check HasValue/Value instead of item
!= null); for reference types keep the item != null ?
AttributeValue.FromMap(DynamoMapper.{normalizedTypeName}.ToDynamoRecord(item)) :
AttributeValue.Null() pattern. Locate and modify the string construction in
CollectionTypeHandler where normalizedTypeName and itemVarName are used so the
mapper name and null-check match the element's nullability, reusing the same
type-inspection logic/patterns from ComplexTypeHandler.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a9908c16-8122-4402-ba4e-01efa5d0d77e
📒 Files selected for processing (5)
src/Clients/Goa.Clients.Dynamo.Generator/TypeHandlers/CollectionTypeHandler.cssrc/Clients/Goa.Clients.Dynamo.Generator/TypeHandlers/ComplexTypeHandler.cstests/Clients/Goa.Clients.Dynamo.Generator.Tests/TypeHandlers/CollectionTypeHandlerTests.cstests/Clients/Goa.Clients.Dynamo.Generator.Tests/TypeHandlers/ComplexTypeHandlerTests.cstests/Clients/Goa.Clients.Dynamo.Tests/DynamoRecordExtensionsTests.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/Clients/Goa.Clients.Dynamo.Generator.Tests/TypeHandlers/ComplexTypeHandlerTests.cs
…ested collection test expectations Replace silent `return default` with `throw JsonException` in AttributeValueJsonConverter.Read to fail loudly on malformed input. Update two nested collection test expected strings to use factory method syntax.
Owner
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/Models/AttributeValue.cs`:
- Around line 123-126: Numeric serialization/parsing for AttributeValue
currently uses the current thread culture; update the int/long implicit
operators and any numeric parsing paths (e.g., the methods referenced at lines
158-161) to use CultureInfo.InvariantCulture so DynamoDB "N" values are
culture-invariant. Replace value.ToString() calls in the implicit operators
AttributeValue(int) and AttributeValue(long) with
value.ToString(CultureInfo.InvariantCulture), and ensure any numeric
Parse/TryParse calls that convert AttributeValue back to numbers explicitly pass
CultureInfo.InvariantCulture as well so round-tripping is deterministic.
- Around line 187-269: The Read method branches (e.g., the checks for "S", "N",
"BOOL", "NULL", "SS", "NS", "L", "M", "B", "BS" inside Read(ref reader,
typeToConvert, options)) currently assume the post-value reader.Read() lands on
JsonTokenType.EndObject and treat any boolean in "NULL" as null; instead, after
reading each wrapped value you must validate that the next token is
JsonTokenType.EndObject and throw a JsonException if not, and for the "NULL"
branch require the boolean to be true (otherwise throw) before returning
AttributeValue.Null(); apply this same EndObject validation to all branches
(AttributeValue.String, Number, Bool, FromStringSet, FromNumberSet, FromList,
FromMap, FromBinary, FromBinarySet) so malformed wrappers like {"S":"x","N":"1"}
fail fast and the reader is left in a consistent state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ec74b6d2-6986-4eac-894a-d7ee4f782806
📒 Files selected for processing (2)
src/Clients/Goa.Clients.Dynamo/Models/AttributeValue.cstests/Clients/Goa.Clients.Dynamo.Generator.Tests/TypeHandlers/CollectionTypeHandlerTests.cs
Comment on lines
+123
to
+126
| public static implicit operator AttributeValue(int value) => Number(value.ToString()); | ||
|
|
||
| /// <summary>Implicitly converts a long to an AttributeValue with N type.</summary> | ||
| public static implicit operator AttributeValue(long value) => Number(value.ToString()); |
Contributor
There was a problem hiding this comment.
Make numeric attribute round-tripping culture-invariant.
These paths still use the current culture for int/long serialization and for all numeric parsing. That makes N values locale-dependent: the same DynamoDB payload can round-trip differently, or fail entirely, off an en-US thread. Using CultureInfo.InvariantCulture here would also keep this type aligned with the invariant numeric formatting the generator now emits elsewhere in the PR.
Possible fix
-public static implicit operator AttributeValue(int value) => Number(value.ToString());
+public static implicit operator AttributeValue(int value) => Number(value.ToString(CultureInfo.InvariantCulture));
-public static implicit operator AttributeValue(long value) => Number(value.ToString());
+public static implicit operator AttributeValue(long value) => Number(value.ToString(CultureInfo.InvariantCulture));
@@
- var t when t == typeof(int) => int.TryParse(N, out var i) ? (T?)(object?)i : default,
- var t when t == typeof(long) => long.TryParse(N, out var l) ? (T?)(object?)l : default,
- var t when t == typeof(double) => double.TryParse(N, out var d) ? (T?)(object?)d : default,
- var t when t == typeof(decimal) => decimal.TryParse(N, out var m) ? (T?)(object?)m : default,
+ var t when t == typeof(int) => int.TryParse(N, NumberStyles.Integer, CultureInfo.InvariantCulture, out var i) ? (T?)(object?)i : default,
+ var t when t == typeof(long) => long.TryParse(N, NumberStyles.Integer, CultureInfo.InvariantCulture, out var l) ? (T?)(object?)l : default,
+ var t when t == typeof(double) => double.TryParse(N, NumberStyles.Float, CultureInfo.InvariantCulture, out var d) ? (T?)(object?)d : default,
+ var t when t == typeof(decimal) => decimal.TryParse(N, NumberStyles.Float, CultureInfo.InvariantCulture, out var m) ? (T?)(object?)m : default,Also applies to: 158-161
🤖 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 123 -
126, Numeric serialization/parsing for AttributeValue currently uses the current
thread culture; update the int/long implicit operators and any numeric parsing
paths (e.g., the methods referenced at lines 158-161) to use
CultureInfo.InvariantCulture so DynamoDB "N" values are culture-invariant.
Replace value.ToString() calls in the implicit operators AttributeValue(int) and
AttributeValue(long) with value.ToString(CultureInfo.InvariantCulture), and
ensure any numeric Parse/TryParse calls that convert AttributeValue back to
numbers explicitly pass CultureInfo.InvariantCulture as well so round-tripping
is deterministic.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/Clients/Goa.Clients.Dynamo.Generator/TypeHandlers/EnumTypeHandler.cs (1)
59-71:⚠️ Potential issue | 🟠 MajorUse the serialized Dynamo attribute name for nullable enums.
This path still emits the CLR property name as the map key. Models with
[SerializedName]will write enum values under the wrong attribute, andGenerateFromDynamoRecordin this handler still reads by CLR name, so those values will not round-trip correctly.Suggested fix
public string GenerateFromDynamoRecord(PropertyInfo propertyInfo, string recordVariableName, string pkVariable, string skVariable) { - var memberName = propertyInfo.Name; + var memberName = propertyInfo.GetDynamoAttributeName(); var enumType = propertyInfo.UnderlyingType.ToDisplayString(); var isNullable = propertyInfo.IsNullable; @@ public string? GenerateConditionalAssignment(PropertyInfo propertyInfo, string recordVariable) { var propertyName = propertyInfo.Name; + var dynamoAttributeName = propertyInfo.GetDynamoAttributeName(); var isNullable = propertyInfo.IsNullable; @@ - {recordVariable}[""{propertyName}""] = AttributeValue.String(model.{propertyName}.Value.ToString()); + {recordVariable}[""{dynamoAttributeName}""] = AttributeValue.String(model.{propertyName}.Value.ToString()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Clients/Goa.Clients.Dynamo.Generator/TypeHandlers/EnumTypeHandler.cs` around lines 59 - 71, The nullable-enum conditional assignment is using the CLR property name as the Dynamo key which breaks models using [SerializedName]; update GenerateConditionalAssignment to resolve the serialized attribute name (from PropertyInfo metadata/SerializedName helper used elsewhere) instead of propertyInfo.Name when writing into recordVariable, and also ensure GenerateFromDynamoRecord reads using the same serialized attribute name lookup so read/write use the same key; reference GenerateConditionalAssignment, GenerateFromDynamoRecord, propertyInfo and recordVariable to find where to replace propertyInfo.Name with the serialized-name lookup.src/Clients/Goa.Clients.Dynamo/Extensions/DynamoRecordExtensions.cs (2)
92-99:⚠️ Potential issue | 🟠 MajorReturn
falsefor non-BOOL attributes.After the
Nullbranch, this method reports success unconditionally. If the stored value has the wrong Dynamo type,attributeValue.BOOLstays null and callers gettrue/null, which hides schema mismatches as missing data.💡 Suggested fix
if (attributeValue.Type == AttributeType.Null) { value = null; return true; } - value = attributeValue.BOOL; - return true; + if (attributeValue.BOOL is not bool boolValue) + return false; + + value = boolValue; + return true;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Clients/Goa.Clients.Dynamo/Extensions/DynamoRecordExtensions.cs` around lines 92 - 99, The method currently returns true unconditionally after handling AttributeType.Null; change the logic to only return true when attributeValue.Type is AttributeType.Null (set value = null) or when the attribute is actually a BOOL (set value = attributeValue.BOOL and return true); for any other attributeValue.Type return false to signal a type mismatch. Use the existing symbols attributeValue.Type, AttributeType.Null, and attributeValue.BOOL to implement the conditional check and ensure callers receive false when the stored Dynamo type is not BOOL.
262-295:⚠️ Potential issue | 🟠 MajorParse floating-point and decimal values with invariant culture.
The write path uses
ToString(CultureInfo.InvariantCulture)to serialize numeric values, but the read path uses defaultTryParse()overloads that applyCurrentCulture. On non-invariant locales (e.g., German with,as decimal separator), invariant-formatted strings like"1.23"will fail to parse or round-trip incorrectly.Update all six methods in these ranges to use
CultureInfo.InvariantCulture:
TryGetDouble(line 265)TryGetNullableDouble(line 290)TryGetFloat(line 314)TryGetNullableFloat(line 339)TryGetDecimal(line 363)TryGetNullableDecimal(line 388)💡 Representative fix
- return !string.IsNullOrEmpty(attributeValue.N) && double.TryParse(attributeValue.N, out value); + return !string.IsNullOrEmpty(attributeValue.N) && + double.TryParse( + attributeValue.N, + System.Globalization.NumberStyles.Float | System.Globalization.NumberStyles.AllowThousands, + System.Globalization.CultureInfo.InvariantCulture, + out value);Apply the same pattern to
floatanddecimalvariants.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Clients/Goa.Clients.Dynamo/Extensions/DynamoRecordExtensions.cs` around lines 262 - 295, The parsing uses culture-sensitive TryParse overloads and fails for invariant-formatted numbers; update TryGetDouble, TryGetNullableDouble, TryGetFloat, TryGetNullableFloat, TryGetDecimal, and TryGetNullableDecimal to call the TryParse overloads that accept CultureInfo (and NumberStyles) and pass CultureInfo.InvariantCulture (for double/float use NumberStyles.Float | NumberStyles.AllowThousands; for decimal use NumberStyles.Number), and add "using System.Globalization;" if not present so invariant-formatted strings produced with ToString(CultureInfo.InvariantCulture) round-trip correctly.src/Clients/Goa.Clients.Dynamo.Generator/TypeHandlers/CollectionTypeHandler.cs (1)
67-72:⚠️ Potential issue | 🔴 CriticalEmit null guards only for reference types, not for non-nullable structs.
Line 72 generates
item != null ? ... : AttributeValue.Null()wheneverIsComplexTypeis true, butIsComplexTypereturns true forTypeKind.Struct(line 446). Non-nullable structs cannot be compared tonullin C#, soList<MyStruct>orMyStruct[]would produce uncompilable code. Conditionally emit the null guard only when the element type is a reference type or explicitly nullable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Clients/Goa.Clients.Dynamo.Generator/TypeHandlers/CollectionTypeHandler.cs` around lines 67 - 72, The current emission in CollectionTypeHandler.cs uses a null check for all complex element types which breaks for non-nullable structs; change the generation to only emit a null-guard when the element type is a reference type or an explicit Nullable<T>. Use IsComplexType to detect mapping but then inspect elementType.IsValueType and Nullable.GetUnderlyingType(elementType): if elementType is a value type and not nullable, emit AttributeValue.FromMap(DynamoMapper.{normalizedTypeName}.ToDynamoRecord({itemVarName})) with no null ternary; if elementType is Nullable<T> emit a HasValue ? ToDynamoRecord({itemVarName}.Value) : AttributeValue.Null() pattern; otherwise (reference types) keep the existing {itemVarName} != null ? ... : AttributeValue.Null() logic using NamingHelpers.NormalizeTypeName and DynamoMapper.{normalizedTypeName}.ToDynamoRecord.
♻️ Duplicate comments (7)
src/Clients/Goa.Clients.Dynamo.Generator/TypeHandlers/CollectionTypeHandler.cs (1)
51-52:⚠️ Potential issue | 🟠 MajorNULL placeholders still disappear on round-trip.
This write path now preserves null elements as
AttributeValue.Null(), butGenerateFromDynamoRecordlater filters them out with.Where(item => item.M != null).List<MyClass?>comes back shorter after a round-trip.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Clients/Goa.Clients.Dynamo.Generator/TypeHandlers/CollectionTypeHandler.cs` around lines 51 - 52, The round-trip drops null list elements because GenerateFromDynamoRecord (in CollectionTypeHandler.cs) currently filters list items with ".Where(item => item.M != null)"; remove or change that filter so AttributeValue.Null() entries are preserved, and update the mapping logic in GenerateFromDynamoRecord to detect AttributeValue.Null (or the library's null/IsNull representation) and map those elements to C# null for nullable element types (instead of filtering them out). Locate the code that consumes the list produced by AttributeValue.FromList and ensure it no longer excludes items where the AttributeValue represents NULL, mapping them back to null values for List<T?>.src/Clients/Goa.Clients.Dynamo/Internal/DynamoResponseReader.cs (1)
217-222:⚠️ Potential issue | 🟠 MajorFail fast on malformed attribute wrappers.
NULL:falseis still accepted asAttributeValue.Null(), and the fallback still returnsdefault(AttributeValue). That diverges fromsrc/Clients/Goa.Clients.Dynamo/Models/AttributeValue.cs, which now throws for both cases, and it reintroduces uninitialized union states intoDynamoRecord.💡 Suggested fix
if (reader.ValueTextEquals("NULL"u8)) { reader.Read(); - reader.GetBoolean(); + if (!reader.GetBoolean()) + throw new JsonException("DynamoDB NULL attributes must contain true."); reader.Read(); // EndObject return AttributeValue.Null(); } @@ - return default; + throw new JsonException("Invalid AttributeValue JSON: no recognized DynamoDB type property found");Also applies to: 267-271
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Clients/Goa.Clients.Dynamo/Internal/DynamoResponseReader.cs` around lines 217 - 222, The code in DynamoResponseReader that handles the "NULL" wrapper (the block in DynamoResponseReader.cs that checks reader.ValueTextEquals("NULL"u8)) accepts malformed inputs like "NULL": false and returns AttributeValue.Null() or default; change it to validate the boolean value: after confirming "NULL" key, call reader.Read() then bool isNull = reader.GetBoolean(); if isNull is true, consume the EndObject token and return AttributeValue.Null(); if isNull is false or the token is not a boolean, throw a JsonException (or a suitable parsing exception) to fail fast; remove any fallback that returns default(AttributeValue). Apply the same strict validation to the similar block around lines 267-271 so both NULL wrappers enforce boolean true only.tests/Clients/Goa.Clients.Dynamo.Benchmarks/Benchmarks/DeleteItemAsyncBenchmarks.cs (2)
74-116:⚠️ Potential issue | 🟠 MajorThese
ALL_OLDbenchmarks never hit the return-values path.Each method deletes a brand-new
delret-*key without first inserting items. SinceSetup()only starts the fixture without seeding these keys,ReturnValues.ALL_OLDwill return no old item data. The benchmarks won't measure old-value materialization.Consider seeding the target items before the measured delete call, or document that these benchmarks intentionally measure the no-op return path.
🤖 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/DeleteItemAsyncBenchmarks.cs` around lines 74 - 116, The three benchmark methods AwsSdk_DeleteItem_WithReturnValues, Goa_DeleteItem_WithReturnValues, and Efficient_DeleteItem_WithReturnValues currently delete fresh keys so ReturnValues.ALL_OLD never returns old data; fix by seeding a matching item immediately before the measured DeleteItemAsync call (or in the benchmark Setup for the fixture) using the corresponding client APIs (AwsSdkClient.PutItemAsync or equivalent, GoaClient.PutItemAsync or creating a GoaModels item, and EfficientClient.PutItemAsync or PrimaryKey-based insert) so the delete actually materializes OLD attributes, or alternatively add a clear comment in those methods documenting that the benchmark intentionally measures the no-old-value path.
56-60:⚠️ Potential issue | 🟠 MajorDon't turn Goa failures into successful benchmark samples.
Returning
!response.IsErrormasks errors. A broken Goa path would appear artificially fast while AWS SDK and EfficientDynamoDb variants would surface failures as exceptions.Proposed fix
var response = await _fixture.GoaClient.DeleteItemAsync(new Goa.Clients.Dynamo.Operations.DeleteItem.DeleteItemRequest { TableName = _fixture.TableName, Key = new Dictionary<string, GoaModels.AttributeValue> { ["pk"] = GoaModels.AttributeValue.String($"del-goa-{id}"), ["sk"] = GoaModels.AttributeValue.String("item") } }); - return !response.IsError; + if (response.IsError) + throw new InvalidOperationException("DeleteItemAsync returned an error."); + return true;🤖 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/DeleteItemAsyncBenchmarks.cs` around lines 56 - 60, The current code returns !response.IsError which hides Goa failures; instead, in the DeleteItemAsyncBenchmarks method (the code that calls and stores the variable named response), check response.IsError and throw an exception (include response error details or message) when true so failures surface as exceptions rather than being treated as successful benchmark samples; replace the inverted return with either a thrown exception on error and a true return on success, or call a response.ThrowIfError()-style helper that includes response details.src/Clients/Goa.Clients.Dynamo.Generator/TypeHandlers/ComplexTypeHandler.cs (1)
346-346:⚠️ Potential issue | 🟠 MajorGuard null string map values before calling
AttributeValue.String().This line passes dictionary values directly to
AttributeValue.String(). ForDictionary<string, string?>entries with null values, this will produce invalid attributes instead ofAttributeValue.Null(). The nested string-map branch on line 81 handles this correctly, but this path does not.Suggested fix
- return $"model.{propertyName} != null ? AttributeValue.FromMap(model.{propertyName}.ToDictionary(kvp => kvp.Key, kvp => AttributeValue.String(kvp.Value))) : AttributeValue.Null()"; + return $"model.{propertyName} != null ? AttributeValue.FromMap(model.{propertyName}.ToDictionary(kvp => kvp.Key, kvp => kvp.Value != null ? AttributeValue.String(kvp.Value) : AttributeValue.Null())) : AttributeValue.Null()";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Clients/Goa.Clients.Dynamo.Generator/TypeHandlers/ComplexTypeHandler.cs` at line 346, The generated expression that maps a string dictionary to AttributeValue currently passes kvp.Value directly to AttributeValue.String, which fails for null values; update the return in ComplexTypeHandler (the expression using model.{propertyName}.ToDictionary(kvp => kvp.Key, kvp => AttributeValue.String(kvp.Value))) so the value lambda checks for null and returns AttributeValue.Null() when kvp.Value is null, e.g. kvp => kvp.Value != null ? AttributeValue.String(kvp.Value) : AttributeValue.Null().src/Clients/Goa.Clients.Dynamo/Models/AttributeValue.cs (2)
123-126:⚠️ Potential issue | 🟠 MajorMake numeric
Nround-tripping invariant.These paths still format
int/longand parse numbers with the current thread culture. DynamoDB numeric strings need stable invariant formatting, otherwise the same payload can serialize or parse differently off anen-USthread.Possible fix
-public static implicit operator AttributeValue(int value) => Number(value.ToString()); -public static implicit operator AttributeValue(long value) => Number(value.ToString()); +public static implicit operator AttributeValue(int value) => Number(value.ToString(CultureInfo.InvariantCulture)); +public static implicit operator AttributeValue(long value) => Number(value.ToString(CultureInfo.InvariantCulture)); @@ - var t when t == typeof(int) => int.TryParse(N, out var i) ? (T?)(object?)i : default, - var t when t == typeof(long) => long.TryParse(N, out var l) ? (T?)(object?)l : default, - var t when t == typeof(double) => double.TryParse(N, out var d) ? (T?)(object?)d : default, - var t when t == typeof(decimal) => decimal.TryParse(N, out var m) ? (T?)(object?)m : default, + var t when t == typeof(int) => int.TryParse(N, NumberStyles.Integer, CultureInfo.InvariantCulture, out var i) ? (T?)(object?)i : default, + var t when t == typeof(long) => long.TryParse(N, NumberStyles.Integer, CultureInfo.InvariantCulture, out var l) ? (T?)(object?)l : default, + var t when t == typeof(double) => double.TryParse(N, NumberStyles.Float, CultureInfo.InvariantCulture, out var d) ? (T?)(object?)d : default, + var t when t == typeof(decimal) => decimal.TryParse(N, NumberStyles.Float, CultureInfo.InvariantCulture, out var m) ? (T?)(object?)m : default,Also applies to: 158-161
🤖 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 123 - 126, The implicit conversions for numeric types (the methods "public static implicit operator AttributeValue(int value)" and "public static implicit operator AttributeValue(long value)" and the similar overloads around lines 158-161) currently call ToString()/parse using the current thread culture; change them to use invariant formatting by calling value.ToString(CultureInfo.InvariantCulture) when producing the DynamoDB "N" string and use parsing methods that accept CultureInfo.InvariantCulture (and appropriate NumberStyles) when reading numeric strings; ensure the Number(...) helper and any parsing utilities are updated to use CultureInfo.InvariantCulture so numeric "N" values round-trip deterministically across cultures.
187-213:⚠️ Potential issue | 🟠 MajorReject malformed Dynamo wrappers instead of partially consuming them.
A payload like
{"S":"x","N":"1"}currently returns from the first branch with the reader still inside the object, and{"NULL": false}is accepted as a real null. Validate that each wrapper ends immediately after its value and only acceptNULL: true.Possible hardening
+ static void EnsureWrapperEnd(ref Utf8JsonReader reader, string name) + { + if (!reader.Read() || reader.TokenType != JsonTokenType.EndObject) + throw new JsonException($"Invalid AttributeValue JSON: '{name}' must be the only property in the wrapper."); + } + if (reader.ValueTextEquals("S"u8)) { reader.Read(); var v = reader.GetString()!; - reader.Read(); // EndObject + EnsureWrapperEnd(ref reader, "S"); return AttributeValue.String(v); } @@ if (reader.ValueTextEquals("NULL"u8)) { reader.Read(); - reader.GetBoolean(); - reader.Read(); // EndObject + if (!reader.GetBoolean()) + throw new JsonException("Invalid AttributeValue JSON: 'NULL' must be true."); + EnsureWrapperEnd(ref reader, "NULL"); return AttributeValue.Null(); }Also applies to: 215-269
🤖 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 187 - 213, The current JSON wrapper handling returns immediately after reading a value and can leave the reader inside the object or accept {"NULL": false}; update each wrapper branch that reads a value (the blocks that call reader.GetString/GetBoolean and then return AttributeValue.String/Number/Bool/Null) to first read the value without returning, then advance and assert the next token is the object end (e.g., verify reader.TokenType == JsonTokenType.EndObject after a reader.Read() and throw or return an error if not), and for the NULL branch only accept when the boolean is true (reject false); apply the same validation logic to the other similar branches in the same method (the code around the AttributeValue.String/Number/Bool/Null calls and the section covering lines ~215-269).
🧹 Nitpick comments (6)
tests/Clients/Goa.Clients.Dynamo.Tests/BuilderChainingTests.cs (1)
249-271: Consider updating remaining implicit conversions for consistency.Lines 253, 265, and 316 still use implicit string-to-
AttributeValueconversion (["pk"] = "value1"), while other tests in this file have been updated to use the explicit factory methodAttributeValue.String("value1"). For consistency with the rest of this file and the PR's migration to factory methods, consider updating these as well.♻️ Suggested change for consistency
- .WithItem(new Dictionary<string, AttributeValue> { ["pk"] = "value1" }) + .WithItem(new Dictionary<string, AttributeValue> { ["pk"] = AttributeValue.String("value1") })Apply similar changes to lines 265 and 316.
🤖 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 249 - 271, Update the implicit string-to-AttributeValue conversions in the PutItemBuilder tests to use the explicit factory method for consistency: in the test methods PutItemBuilder_WithReturnValuesOnConditionCheckFailure_AllOld and PutItemBuilder_WithReturnValuesOnConditionCheckFailure_None (and the other remaining test that still uses ["pk"] = "value1"), replace the dictionary value expressions that use the raw string (["pk"] = "value1") with AttributeValue.String("value1") so the builders receive an AttributeValue instance instead of relying on implicit conversion.src/Clients/Goa.Clients.Dynamo/Operations/Condition.cs (1)
270-273: Prefer the newAttributeTypeenum here.This helper is still stringly typed even though the PR introduces a dedicated enum for Dynamo attribute kinds. Keeping
stringhere still allows invalid tokens through and leaves this public API inconsistent with the rest of the change.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Clients/Goa.Clients.Dynamo/Operations/Condition.cs` around lines 270 - 273, Change the public helper Condition.AttributeType to take the dedicated AttributeType enum (not string) and use the enum value when building the condition expression and attribute value map; update the signature from AttributeType(string attributeName, string type) to AttributeType(string attributeName, AttributeType attributeType) and replace the string usage passed into AttributeValue.String(...) with the enum's string representation (e.g., attributeType.ToString()) when creating the KeyValuePair for $":{attributeName}", keeping the expression string $"attribute_type(#{attributeName}, :{attributeName})" and the attribute name placeholder KeyValuePair unchanged.tests/Clients/Goa.Clients.Dynamo.Benchmarks/Benchmarks/BatchWriteItemAsyncBenchmarks.cs (1)
86-86: Consider throwing on error instead of returning false.Returning
!response.IsErrormasks Goa failures as successful benchmark samples, potentially making a broken Goa path appear artificially fast compared to AWS SDK and EfficientDynamoDb variants that would surface errors as exceptions.Suggested improvement
var response = await _fixture.GoaClient.BatchWriteItemAsync(new GoaBatchWriteItemRequest { RequestItems = new Dictionary<string, List<BatchWriteRequestItem>> { [_fixture.TableName] = requests } }); - return !response.IsError; + if (response.IsError) + throw new InvalidOperationException("BatchWriteItemAsync returned an error."); + return true;Also applies to: 163-163
🤖 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/BatchWriteItemAsyncBenchmarks.cs` at line 86, In BatchWriteItemAsyncBenchmarks (and the other occurrence around line 163) don't mask failures by returning !response.IsError; instead detect when response.IsError is true and throw a descriptive exception (including response.Error/response.Exception or other diagnostic info) so the benchmark runtime surfaces Goa failures rather than treating them as successful samples; update the code paths that currently return !response.IsError to throw when IsError is true.tests/Clients/Goa.Clients.Dynamo.Benchmarks/Benchmarks/PutItemAsyncBenchmarks.cs (1)
62-62: Consider throwing on error instead of returning false.Same pattern as other benchmarks - returning
!response.IsErrormasks Goa failures as successful benchmark samples.Also applies to: 120-120, 205-205
🤖 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/PutItemAsyncBenchmarks.cs` at line 62, In PutItemAsyncBenchmarks replace the pattern "return !response.IsError;" with an exception throw when response.IsError is true so failures surface in benchmark runs: check response.IsError and throw a descriptive exception (e.g. throw new InvalidOperationException($"Dynamo error: {response.Error ?? response.ToString()}")) instead of returning false; apply this change to all occurrences that use response.IsError in the PutItemAsyncBenchmarks class (the three locations flagged) so failed Goa/Dynamo responses are reported as errors, not successful samples.tests/Clients/Goa.Clients.Dynamo.Benchmarks/Benchmarks/QueryAsyncBenchmarks.cs (2)
13-30: Reuse the sharedBenchmarkEntitymodel.This duplicates the mapped entity that already exists in
tests/Clients/Goa.Clients.Dynamo.Benchmarks/Models/BenchmarkEntity.cs, so the typed benchmark schema can drift in two places. Keep a single model definition and reference it here.🤖 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 13 - 30, The BenchmarkEntity class here duplicates the existing model and can drift; remove this local duplicate and import/use the shared BenchmarkEntity model instead (replace the local class declaration with a using/import and reference to BenchmarkEntity wherever used in QueryAsyncBenchmarks); ensure any attributes or property names expected by the benchmark (Pk, Sk, Data, Number, Status) match the shared model so tests compile and behavior remains identical.
44-47: Remove or restore the inactive query scenarios.
Setup()still seedsquery-10andquery-100, but every 10/100-item benchmark below is commented out. That leaves dead code plus extra fixture work in the suite with nothing exercising it.Also applies to: 136-468
🤖 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 44 - 47, The Setup() method is seeding "query-10" and "query-100" via _fixture.SeedItemsAsync("query-10", 10) and _fixture.SeedItemsAsync("query-100", 100) even though the corresponding 10- and 100-item query benchmarks are commented out (dead code across the file, see comment range 136-468); either remove the inactive seeding calls for "query-10"/"query-100" from Setup() to avoid unnecessary fixture work, or restore the commented benchmark methods to exercise those scenarios—update the code paths referring to "query-10"/"query-100" consistently (in Setup() and the benchmark methods) and delete any leftover unused seed calls or commented benchmark blocks accordingly.
🤖 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/TypeHandlers/CollectionTypeHandler.cs`:
- Around line 107-116: The code in CollectionTypeHandler.cs currently emits
AttributeValue.FromStringSet/FromNumberSet for any primitive collection (see the
branches using collectionVarName and elementType), which loses ordering and
duplicates for arrays/Lists/IEnumerable; change the generator to emit
list-valued AttributeValues for non-set CLR types (e.g., arrays, List<T>,
IEnumerable<T>) and only emit set-valued
AttributeValue.FromStringSet/FromNumberSet when the declared collection type
implements ISet<T> (or is a concrete set type like HashSet<T>/ISet<T>); update
the branches that match SpecialType.System_String, numeric types, Boolean,
DateTime, Guid, TimeSpan, DateTimeOffset and enum handling to choose
FromList/FromStringList/FromNumberList (preserve element conversions like
ToString("o") or ToString(CultureInfo.InvariantCulture)) for ordered collections
and retain the existing From...Set logic only for detected set types,
referencing collectionVarName and elementType to decide which form to emit.
In
`@src/Clients/Goa.Clients.Dynamo.Generator/TypeHandlers/TimeOnlyTypeHandler.cs`:
- Around line 52-65: The write-side now uses
propertyInfo.GetDynamoAttributeName() in GenerateConditionalAssignment but the
read-side still uses propertyInfo.Name; update GenerateFromDynamoRecord to
mirror this by looking up the Dynamo serialized key via
propertyInfo.GetDynamoAttributeName() (instead of propertyInfo.Name) when
deserializing TimeOnly properties so reads use the same dictionary key as
writes; adjust any local variables or lookup logic in GenerateFromDynamoRecord
to use that dynamoAttributeName and ensure null/HasValue handling matches the
write semantics.
In `@src/Clients/Goa.Clients.Dynamo/Models/AttributeValue.cs`:
- Around line 140-144: The implicit conversion operators on AttributeValue (the
two methods: public static implicit operator AttributeValue(List<byte[]> value)
=> FromBinarySet(value); and public static implicit operator
AttributeValue(List<string> value) => FromStringSet(value);) cause ambiguous
semantics between DynamoDB sets and lists; change these to explicit conversions
(replace implicit with explicit) or remove them so callers must call
FromBinarySet/FromStringSet (or a clearly named factory) directly; update any
callers that relied on the implicit cast to call the explicit factory
(AttributeValue.FromStringSet or AttributeValue.FromBinarySet) to make set
creation explicit and prevent silent semantics changes.
- Around line 12-23: AttributeValue’s zero state must be treated as invalid: add
a sentinel enum member (e.g. AttributeType.None = 0) and ensure all
AttributeValue constructors/creators set a non-None type; change the
AttributeValue.Type accessor (and Write() path) to throw/Assert when _type ==
AttributeType.None instead of silently returning/using it; update
DynamoResponseReader (the code paths that currently return default
AttributeValue for unknown wrappers) to throw or return an explicit failure
rather than default(AttributeValue); apply the same guard to other creation
sites referenced around the 281–337 region so zero-initialized AttributeValue
cannot flow through silently.
In
`@tests/Clients/Goa.Clients.Dynamo.Benchmarks/Benchmarks/UpdateItemAsyncBenchmarks.cs`:
- Around line 57-79: The benchmark Goa_UpdateItem_SingleAttribute currently
returns !response.IsError which treats failed Goa.UpdateItemAsync responses as
successful benchmark results; change the method to fail fast on error by
checking response.IsError and throwing an exception (or rethrowing
response.Error) instead of returning a boolean, so failed Goa requests abort the
benchmark run; apply the same change to the other Goa benchmark(s) that return
!response.IsError.
---
Outside diff comments:
In
`@src/Clients/Goa.Clients.Dynamo.Generator/TypeHandlers/CollectionTypeHandler.cs`:
- Around line 67-72: The current emission in CollectionTypeHandler.cs uses a
null check for all complex element types which breaks for non-nullable structs;
change the generation to only emit a null-guard when the element type is a
reference type or an explicit Nullable<T>. Use IsComplexType to detect mapping
but then inspect elementType.IsValueType and
Nullable.GetUnderlyingType(elementType): if elementType is a value type and not
nullable, emit
AttributeValue.FromMap(DynamoMapper.{normalizedTypeName}.ToDynamoRecord({itemVarName}))
with no null ternary; if elementType is Nullable<T> emit a HasValue ?
ToDynamoRecord({itemVarName}.Value) : AttributeValue.Null() pattern; otherwise
(reference types) keep the existing {itemVarName} != null ? ... :
AttributeValue.Null() logic using NamingHelpers.NormalizeTypeName and
DynamoMapper.{normalizedTypeName}.ToDynamoRecord.
In `@src/Clients/Goa.Clients.Dynamo.Generator/TypeHandlers/EnumTypeHandler.cs`:
- Around line 59-71: The nullable-enum conditional assignment is using the CLR
property name as the Dynamo key which breaks models using [SerializedName];
update GenerateConditionalAssignment to resolve the serialized attribute name
(from PropertyInfo metadata/SerializedName helper used elsewhere) instead of
propertyInfo.Name when writing into recordVariable, and also ensure
GenerateFromDynamoRecord reads using the same serialized attribute name lookup
so read/write use the same key; reference GenerateConditionalAssignment,
GenerateFromDynamoRecord, propertyInfo and recordVariable to find where to
replace propertyInfo.Name with the serialized-name lookup.
In `@src/Clients/Goa.Clients.Dynamo/Extensions/DynamoRecordExtensions.cs`:
- Around line 92-99: The method currently returns true unconditionally after
handling AttributeType.Null; change the logic to only return true when
attributeValue.Type is AttributeType.Null (set value = null) or when the
attribute is actually a BOOL (set value = attributeValue.BOOL and return true);
for any other attributeValue.Type return false to signal a type mismatch. Use
the existing symbols attributeValue.Type, AttributeType.Null, and
attributeValue.BOOL to implement the conditional check and ensure callers
receive false when the stored Dynamo type is not BOOL.
- Around line 262-295: The parsing uses culture-sensitive TryParse overloads and
fails for invariant-formatted numbers; update TryGetDouble,
TryGetNullableDouble, TryGetFloat, TryGetNullableFloat, TryGetDecimal, and
TryGetNullableDecimal to call the TryParse overloads that accept CultureInfo
(and NumberStyles) and pass CultureInfo.InvariantCulture (for double/float use
NumberStyles.Float | NumberStyles.AllowThousands; for decimal use
NumberStyles.Number), and add "using System.Globalization;" if not present so
invariant-formatted strings produced with ToString(CultureInfo.InvariantCulture)
round-trip correctly.
---
Duplicate comments:
In
`@src/Clients/Goa.Clients.Dynamo.Generator/TypeHandlers/CollectionTypeHandler.cs`:
- Around line 51-52: The round-trip drops null list elements because
GenerateFromDynamoRecord (in CollectionTypeHandler.cs) currently filters list
items with ".Where(item => item.M != null)"; remove or change that filter so
AttributeValue.Null() entries are preserved, and update the mapping logic in
GenerateFromDynamoRecord to detect AttributeValue.Null (or the library's
null/IsNull representation) and map those elements to C# null for nullable
element types (instead of filtering them out). Locate the code that consumes the
list produced by AttributeValue.FromList and ensure it no longer excludes items
where the AttributeValue represents NULL, mapping them back to null values for
List<T?>.
In `@src/Clients/Goa.Clients.Dynamo.Generator/TypeHandlers/ComplexTypeHandler.cs`:
- Line 346: The generated expression that maps a string dictionary to
AttributeValue currently passes kvp.Value directly to AttributeValue.String,
which fails for null values; update the return in ComplexTypeHandler (the
expression using model.{propertyName}.ToDictionary(kvp => kvp.Key, kvp =>
AttributeValue.String(kvp.Value))) so the value lambda checks for null and
returns AttributeValue.Null() when kvp.Value is null, e.g. kvp => kvp.Value !=
null ? AttributeValue.String(kvp.Value) : AttributeValue.Null().
In `@src/Clients/Goa.Clients.Dynamo/Internal/DynamoResponseReader.cs`:
- Around line 217-222: The code in DynamoResponseReader that handles the "NULL"
wrapper (the block in DynamoResponseReader.cs that checks
reader.ValueTextEquals("NULL"u8)) accepts malformed inputs like "NULL": false
and returns AttributeValue.Null() or default; change it to validate the boolean
value: after confirming "NULL" key, call reader.Read() then bool isNull =
reader.GetBoolean(); if isNull is true, consume the EndObject token and return
AttributeValue.Null(); if isNull is false or the token is not a boolean, throw a
JsonException (or a suitable parsing exception) to fail fast; remove any
fallback that returns default(AttributeValue). Apply the same strict validation
to the similar block around lines 267-271 so both NULL wrappers enforce boolean
true only.
In `@src/Clients/Goa.Clients.Dynamo/Models/AttributeValue.cs`:
- Around line 123-126: The implicit conversions for numeric types (the methods
"public static implicit operator AttributeValue(int value)" and "public static
implicit operator AttributeValue(long value)" and the similar overloads around
lines 158-161) currently call ToString()/parse using the current thread culture;
change them to use invariant formatting by calling
value.ToString(CultureInfo.InvariantCulture) when producing the DynamoDB "N"
string and use parsing methods that accept CultureInfo.InvariantCulture (and
appropriate NumberStyles) when reading numeric strings; ensure the Number(...)
helper and any parsing utilities are updated to use CultureInfo.InvariantCulture
so numeric "N" values round-trip deterministically across cultures.
- Around line 187-213: The current JSON wrapper handling returns immediately
after reading a value and can leave the reader inside the object or accept
{"NULL": false}; update each wrapper branch that reads a value (the blocks that
call reader.GetString/GetBoolean and then return
AttributeValue.String/Number/Bool/Null) to first read the value without
returning, then advance and assert the next token is the object end (e.g.,
verify reader.TokenType == JsonTokenType.EndObject after a reader.Read() and
throw or return an error if not), and for the NULL branch only accept when the
boolean is true (reject false); apply the same validation logic to the other
similar branches in the same method (the code around the
AttributeValue.String/Number/Bool/Null calls and the section covering lines
~215-269).
In
`@tests/Clients/Goa.Clients.Dynamo.Benchmarks/Benchmarks/DeleteItemAsyncBenchmarks.cs`:
- Around line 74-116: The three benchmark methods
AwsSdk_DeleteItem_WithReturnValues, Goa_DeleteItem_WithReturnValues, and
Efficient_DeleteItem_WithReturnValues currently delete fresh keys so
ReturnValues.ALL_OLD never returns old data; fix by seeding a matching item
immediately before the measured DeleteItemAsync call (or in the benchmark Setup
for the fixture) using the corresponding client APIs (AwsSdkClient.PutItemAsync
or equivalent, GoaClient.PutItemAsync or creating a GoaModels item, and
EfficientClient.PutItemAsync or PrimaryKey-based insert) so the delete actually
materializes OLD attributes, or alternatively add a clear comment in those
methods documenting that the benchmark intentionally measures the no-old-value
path.
- Around line 56-60: The current code returns !response.IsError which hides Goa
failures; instead, in the DeleteItemAsyncBenchmarks method (the code that calls
and stores the variable named response), check response.IsError and throw an
exception (include response error details or message) when true so failures
surface as exceptions rather than being treated as successful benchmark samples;
replace the inverted return with either a thrown exception on error and a true
return on success, or call a response.ThrowIfError()-style helper that includes
response details.
---
Nitpick comments:
In `@src/Clients/Goa.Clients.Dynamo/Operations/Condition.cs`:
- Around line 270-273: Change the public helper Condition.AttributeType to take
the dedicated AttributeType enum (not string) and use the enum value when
building the condition expression and attribute value map; update the signature
from AttributeType(string attributeName, string type) to AttributeType(string
attributeName, AttributeType attributeType) and replace the string usage passed
into AttributeValue.String(...) with the enum's string representation (e.g.,
attributeType.ToString()) when creating the KeyValuePair for
$":{attributeName}", keeping the expression string
$"attribute_type(#{attributeName}, :{attributeName})" and the attribute name
placeholder KeyValuePair unchanged.
In
`@tests/Clients/Goa.Clients.Dynamo.Benchmarks/Benchmarks/BatchWriteItemAsyncBenchmarks.cs`:
- Line 86: In BatchWriteItemAsyncBenchmarks (and the other occurrence around
line 163) don't mask failures by returning !response.IsError; instead detect
when response.IsError is true and throw a descriptive exception (including
response.Error/response.Exception or other diagnostic info) so the benchmark
runtime surfaces Goa failures rather than treating them as successful samples;
update the code paths that currently return !response.IsError to throw when
IsError is true.
In
`@tests/Clients/Goa.Clients.Dynamo.Benchmarks/Benchmarks/PutItemAsyncBenchmarks.cs`:
- Line 62: In PutItemAsyncBenchmarks replace the pattern "return
!response.IsError;" with an exception throw when response.IsError is true so
failures surface in benchmark runs: check response.IsError and throw a
descriptive exception (e.g. throw new InvalidOperationException($"Dynamo error:
{response.Error ?? response.ToString()}")) instead of returning false; apply
this change to all occurrences that use response.IsError in the
PutItemAsyncBenchmarks class (the three locations flagged) so failed Goa/Dynamo
responses are reported as errors, not successful samples.
In
`@tests/Clients/Goa.Clients.Dynamo.Benchmarks/Benchmarks/QueryAsyncBenchmarks.cs`:
- Around line 13-30: The BenchmarkEntity class here duplicates the existing
model and can drift; remove this local duplicate and import/use the shared
BenchmarkEntity model instead (replace the local class declaration with a
using/import and reference to BenchmarkEntity wherever used in
QueryAsyncBenchmarks); ensure any attributes or property names expected by the
benchmark (Pk, Sk, Data, Number, Status) match the shared model so tests compile
and behavior remains identical.
- Around line 44-47: The Setup() method is seeding "query-10" and "query-100"
via _fixture.SeedItemsAsync("query-10", 10) and
_fixture.SeedItemsAsync("query-100", 100) even though the corresponding 10- and
100-item query benchmarks are commented out (dead code across the file, see
comment range 136-468); either remove the inactive seeding calls for
"query-10"/"query-100" from Setup() to avoid unnecessary fixture work, or
restore the commented benchmark methods to exercise those scenarios—update the
code paths referring to "query-10"/"query-100" consistently (in Setup() and the
benchmark methods) and delete any leftover unused seed calls or commented
benchmark blocks accordingly.
In `@tests/Clients/Goa.Clients.Dynamo.Tests/BuilderChainingTests.cs`:
- Around line 249-271: Update the implicit string-to-AttributeValue conversions
in the PutItemBuilder tests to use the explicit factory method for consistency:
in the test methods
PutItemBuilder_WithReturnValuesOnConditionCheckFailure_AllOld and
PutItemBuilder_WithReturnValuesOnConditionCheckFailure_None (and the other
remaining test that still uses ["pk"] = "value1"), replace the dictionary value
expressions that use the raw string (["pk"] = "value1") with
AttributeValue.String("value1") so the builders receive an AttributeValue
instance instead of relying on implicit conversion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 749c5a40-54c8-4f41-9efc-60b0b2b988a7
📒 Files selected for processing (47)
src/Clients/Goa.Clients.Bedrock.Conversation.Dynamo/DynamoConversationStore.cssrc/Clients/Goa.Clients.Bedrock.Conversation.Dynamo/Internal/ContentBlockSerializer.cssrc/Clients/Goa.Clients.Dynamo.Generator/CodeGeneration/MapperGenerator.cssrc/Clients/Goa.Clients.Dynamo.Generator/TypeHandlers/CollectionTypeHandler.cssrc/Clients/Goa.Clients.Dynamo.Generator/TypeHandlers/ComplexTypeHandler.cssrc/Clients/Goa.Clients.Dynamo.Generator/TypeHandlers/DateOnlyTypeHandler.cssrc/Clients/Goa.Clients.Dynamo.Generator/TypeHandlers/DateTimeTypeHandler.cssrc/Clients/Goa.Clients.Dynamo.Generator/TypeHandlers/EnumTypeHandler.cssrc/Clients/Goa.Clients.Dynamo.Generator/TypeHandlers/PrimitiveTypeHandler.cssrc/Clients/Goa.Clients.Dynamo.Generator/TypeHandlers/TimeOnlyTypeHandler.cssrc/Clients/Goa.Clients.Dynamo.Generator/TypeHandlers/TypeHandlerRegistry.cssrc/Clients/Goa.Clients.Dynamo.Generator/TypeHandlers/UnixTimestampTypeHandler.cssrc/Clients/Goa.Clients.Dynamo.Generator/TypeHandlers/UnsupportedDictionaryHandler.cssrc/Clients/Goa.Clients.Dynamo/Extensions/DynamoRecordExtensions.cssrc/Clients/Goa.Clients.Dynamo/Internal/DynamoResponseReader.cssrc/Clients/Goa.Clients.Dynamo/Models/AttributeType.cssrc/Clients/Goa.Clients.Dynamo/Models/AttributeValue.cssrc/Clients/Goa.Clients.Dynamo/Models/DynamoRecord.cssrc/Clients/Goa.Clients.Dynamo/Operations/Batch/BatchGetTableBuilder.cssrc/Clients/Goa.Clients.Dynamo/Operations/Condition.cssrc/Clients/Goa.Clients.Dynamo/Operations/UpdateItem/UpdateItemBuilder.cstests/Clients/Goa.Clients.Bedrock.Conversation.Dynamo.Tests/ContentBlockSerializerTests.cstests/Clients/Goa.Clients.Dynamo.Benchmarks/Benchmarks/BatchGetItemAsyncBenchmarks.cstests/Clients/Goa.Clients.Dynamo.Benchmarks/Benchmarks/BatchWriteItemAsyncBenchmarks.cstests/Clients/Goa.Clients.Dynamo.Benchmarks/Benchmarks/DeleteItemAsyncBenchmarks.cstests/Clients/Goa.Clients.Dynamo.Benchmarks/Benchmarks/GetItemAsyncBenchmarks.cstests/Clients/Goa.Clients.Dynamo.Benchmarks/Benchmarks/PutItemAsyncBenchmarks.cstests/Clients/Goa.Clients.Dynamo.Benchmarks/Benchmarks/QueryAsyncBenchmarks.cstests/Clients/Goa.Clients.Dynamo.Benchmarks/Benchmarks/ScanAsyncBenchmarks.cstests/Clients/Goa.Clients.Dynamo.Benchmarks/Benchmarks/TransactGetItemsAsyncBenchmarks.cstests/Clients/Goa.Clients.Dynamo.Benchmarks/Benchmarks/TransactWriteItemsAsyncBenchmarks.cstests/Clients/Goa.Clients.Dynamo.Benchmarks/Benchmarks/UpdateItemAsyncBenchmarks.cstests/Clients/Goa.Clients.Dynamo.Generator.Tests/CodeGeneration/MapperGeneratorTests.cstests/Clients/Goa.Clients.Dynamo.Generator.Tests/Integration/NullAwarenessIntegrationTests.cstests/Clients/Goa.Clients.Dynamo.Generator.Tests/TypeHandlers/CollectionTypeHandlerTests.cstests/Clients/Goa.Clients.Dynamo.Generator.Tests/TypeHandlers/ComplexTypeHandlerTests.cstests/Clients/Goa.Clients.Dynamo.Generator.Tests/TypeHandlers/DateOnlyTypeHandlerTests.cstests/Clients/Goa.Clients.Dynamo.Generator.Tests/TypeHandlers/DateTimeTypeHandlerTests.cstests/Clients/Goa.Clients.Dynamo.Generator.Tests/TypeHandlers/EnumTypeHandlerTests.cstests/Clients/Goa.Clients.Dynamo.Generator.Tests/TypeHandlers/PrimitiveTypeHandlerTests.cstests/Clients/Goa.Clients.Dynamo.Generator.Tests/TypeHandlers/StringEmptyHandlingTests.cstests/Clients/Goa.Clients.Dynamo.Generator.Tests/TypeHandlers/TimeOnlyTypeHandlerTests.cstests/Clients/Goa.Clients.Dynamo.Generator.Tests/TypeHandlers/UnixTimestampTypeHandlerTests.cstests/Clients/Goa.Clients.Dynamo.Tests/BuilderChainingTests.cstests/Clients/Goa.Clients.Dynamo.Tests/DynamoClientIntegrationTests.cstests/Clients/Goa.Clients.Dynamo.Tests/DynamoRecordExtensionsTests.cstests/Clients/Goa.Clients.Dynamo.Tests/TypedExtensionTests.cs
Comment on lines
+107
to
+116
| SpecialType.System_String => $"({collectionVarName} != null && {collectionVarName}.Any() ? AttributeValue.FromStringSet({collectionVarName}.ToList()) : AttributeValue.Null())", | ||
| SpecialType.System_Byte or SpecialType.System_SByte or SpecialType.System_Int16 or SpecialType.System_UInt16 or | ||
| SpecialType.System_Int32 or SpecialType.System_UInt32 or SpecialType.System_Int64 or SpecialType.System_UInt64 or | ||
| SpecialType.System_Decimal or SpecialType.System_Single or SpecialType.System_Double => $"({collectionVarName} != null && {collectionVarName}.Any() ? new AttributeValue {{ NS = {collectionVarName}.Select(x => x.ToString(CultureInfo.InvariantCulture)).ToList() }} : new AttributeValue {{ NULL = true }})", | ||
| SpecialType.System_Boolean => $"({collectionVarName} != null && {collectionVarName}.Any() ? new AttributeValue {{ SS = {collectionVarName}.Select(x => x.ToString()).ToList() }} : new AttributeValue {{ NULL = true }})", | ||
| SpecialType.System_DateTime => $"({collectionVarName} != null && {collectionVarName}.Any() ? new AttributeValue {{ SS = {collectionVarName}.Select(x => x.ToString(\"o\")).ToList() }} : new AttributeValue {{ NULL = true }})", | ||
| _ when elementType.Name == nameof(Guid) => $"({collectionVarName} != null && {collectionVarName}.Any() ? new AttributeValue {{ SS = {collectionVarName}.Select(x => x.ToString()).ToList() }} : new AttributeValue {{ NULL = true }})", | ||
| _ when elementType.Name == nameof(TimeSpan) => $"({collectionVarName} != null && {collectionVarName}.Any() ? new AttributeValue {{ SS = {collectionVarName}.Select(x => x.ToString()).ToList() }} : new AttributeValue {{ NULL = true }})", | ||
| _ when elementType.Name == nameof(DateTimeOffset) => $"({collectionVarName} != null && {collectionVarName}.Any() ? new AttributeValue {{ SS = {collectionVarName}.Select(x => x.ToString(\"o\")).ToList() }} : new AttributeValue {{ NULL = true }})", | ||
| _ when elementType.TypeKind == TypeKind.Enum => $"({collectionVarName} != null && {collectionVarName}.Any() ? new AttributeValue {{ SS = {collectionVarName}.Select(x => x.ToString()).ToList() }} : new AttributeValue {{ NULL = true }})", | ||
| SpecialType.System_Decimal or SpecialType.System_Single or SpecialType.System_Double => $"({collectionVarName} != null && {collectionVarName}.Any() ? AttributeValue.FromNumberSet({collectionVarName}.Select(x => x.ToString(CultureInfo.InvariantCulture)).ToList()) : AttributeValue.Null())", | ||
| SpecialType.System_Boolean => $"({collectionVarName} != null && {collectionVarName}.Any() ? AttributeValue.FromStringSet({collectionVarName}.Select(x => x.ToString()).ToList()) : AttributeValue.Null())", | ||
| SpecialType.System_DateTime => $"({collectionVarName} != null && {collectionVarName}.Any() ? AttributeValue.FromStringSet({collectionVarName}.Select(x => x.ToString(\"o\")).ToList()) : AttributeValue.Null())", | ||
| _ when elementType.Name == nameof(Guid) => $"({collectionVarName} != null && {collectionVarName}.Any() ? AttributeValue.FromStringSet({collectionVarName}.Select(x => x.ToString()).ToList()) : AttributeValue.Null())", | ||
| _ when elementType.Name == nameof(TimeSpan) => $"({collectionVarName} != null && {collectionVarName}.Any() ? AttributeValue.FromStringSet({collectionVarName}.Select(x => x.ToString()).ToList()) : AttributeValue.Null())", | ||
| _ when elementType.Name == nameof(DateTimeOffset) => $"({collectionVarName} != null && {collectionVarName}.Any() ? AttributeValue.FromStringSet({collectionVarName}.Select(x => x.ToString(\"o\")).ToList()) : AttributeValue.Null())", | ||
| _ when elementType.TypeKind == TypeKind.Enum => $"({collectionVarName} != null && {collectionVarName}.Any() ? AttributeValue.FromStringSet({collectionVarName}.Select(x => x.ToString()).ToList()) : AttributeValue.Null())", |
Contributor
There was a problem hiding this comment.
Don't encode arrays and lists as DynamoDB sets.
These branches emit FromStringSet/FromNumberSet for every primitive collection even when the CLR type is an array, List<T>, or IEnumerable<T>. That drops ordering and duplicates, so non-set collections cannot round-trip.
Also applies to: 176-185
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/Clients/Goa.Clients.Dynamo.Generator/TypeHandlers/CollectionTypeHandler.cs`
around lines 107 - 116, The code in CollectionTypeHandler.cs currently emits
AttributeValue.FromStringSet/FromNumberSet for any primitive collection (see the
branches using collectionVarName and elementType), which loses ordering and
duplicates for arrays/Lists/IEnumerable; change the generator to emit
list-valued AttributeValues for non-set CLR types (e.g., arrays, List<T>,
IEnumerable<T>) and only emit set-valued
AttributeValue.FromStringSet/FromNumberSet when the declared collection type
implements ISet<T> (or is a concrete set type like HashSet<T>/ISet<T>); update
the branches that match SpecialType.System_String, numeric types, Boolean,
DateTime, Guid, TimeSpan, DateTimeOffset and enum handling to choose
FromList/FromStringList/FromNumberList (preserve element conversions like
ToString("o") or ToString(CultureInfo.InvariantCulture)) for ordered collections
and retain the existing From...Set logic only for detected set types,
referencing collectionVarName and elementType to decide which form to emit.
Comment on lines
+12
to
+23
| [StructLayout(LayoutKind.Explicit, Pack = 1)] | ||
| [JsonConverter(typeof(AttributeValueJsonConverter))] | ||
| public readonly struct AttributeValue | ||
| { | ||
| /// <summary> | ||
| /// An attribute of type String. Strings are UTF-8 encoded and can be up to 400 KB in size. | ||
| /// </summary> | ||
| public string? S { get; set; } | ||
| [FieldOffset(0)] private readonly object? _referenceValue; // string, List<>, Dictionary<> | ||
| [FieldOffset(8)] private readonly AttributeType _type; | ||
| [FieldOffset(9)] private readonly bool _boolValue; | ||
|
|
||
| /// <summary> | ||
| /// An attribute of type Number. Numbers are sent across the network to DynamoDB as strings, | ||
| /// to maximize compatibility across platforms and languages. | ||
| /// Gets the type of this attribute value. | ||
| /// </summary> | ||
| public string? N { get; set; } | ||
| public AttributeType Type => _type; |
Contributor
There was a problem hiding this comment.
Make the zero value invalid.
Now that AttributeValue is a struct, default(AttributeValue) is unavoidable. Without a sentinel like AttributeType.None = 0, zero-initialized values can still flow through accessors and Write() as if they were real attributes; src/Clients/Goa.Clients.Dynamo.Internal/DynamoResponseReader.cs still returns default for unknown wrappers, so this becomes silent corruption instead of a hard failure.
Also applies to: 281-337
🤖 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 12 -
23, AttributeValue’s zero state must be treated as invalid: add a sentinel enum
member (e.g. AttributeType.None = 0) and ensure all AttributeValue
constructors/creators set a non-None type; change the AttributeValue.Type
accessor (and Write() path) to throw/Assert when _type == AttributeType.None
instead of silently returning/using it; update DynamoResponseReader (the code
paths that currently return default AttributeValue for unknown wrappers) to
throw or return an explicit failure rather than default(AttributeValue); apply
the same guard to other creation sites referenced around the 281–337 region so
zero-initialized AttributeValue cannot flow through silently.
Comment on lines
+140
to
+144
| /// <summary>Implicitly converts a List of byte arrays to an AttributeValue with BS type.</summary> | ||
| public static implicit operator AttributeValue(List<byte[]> value) => FromBinarySet(value); | ||
|
|
||
| /// <summary>Implicitly converts a List<string> to an AttributeValue with SS type.</summary> | ||
| public static implicit operator AttributeValue(List<string> value) => FromStringSet(value); |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Keep set creation explicit.
List<string> and List<byte[]> can mean either DynamoDB sets (SS/BS) or ordered L values. Making them implicit silently picks the set representation, which changes semantics for duplicates and order-sensitive callers.
🤖 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 140 -
144, The implicit conversion operators on AttributeValue (the two methods:
public static implicit operator AttributeValue(List<byte[]> value) =>
FromBinarySet(value); and public static implicit operator
AttributeValue(List<string> value) => FromStringSet(value);) cause ambiguous
semantics between DynamoDB sets and lists; change these to explicit conversions
(replace implicit with explicit) or remove them so callers must call
FromBinarySet/FromStringSet (or a clearly named factory) directly; update any
callers that relied on the implicit cast to call the explicit factory
(AttributeValue.FromStringSet or AttributeValue.FromBinarySet) to make set
creation explicit and prevent silent semantics changes.
Comment on lines
+57
to
79
| [Benchmark, BenchmarkCategory("Update Single Attribute")] | ||
| public async Task<bool> Goa_UpdateItem_SingleAttribute() | ||
| { | ||
| var i = Interlocked.Increment(ref _counter); | ||
| var response = await _fixture.GoaClient.UpdateItemAsync(new Goa.Clients.Dynamo.Operations.UpdateItem.UpdateItemRequest | ||
| { | ||
| TableName = _fixture.TableName, | ||
| Key = new Dictionary<string, GoaModels.AttributeValue> | ||
| { | ||
| ["pk"] = new() { S = "update-bench" }, | ||
| ["sk"] = new() { S = "item-0000" } | ||
| ["pk"] = GoaModels.AttributeValue.String("update-bench"), | ||
| ["sk"] = GoaModels.AttributeValue.String("item-0000") | ||
| }, | ||
| UpdateExpression = "SET #d = :val", | ||
| ExpressionAttributeNames = new Dictionary<string, string> | ||
| { | ||
| ["#d"] = "data" | ||
| }, | ||
| UpdateExpression = "SET #d = :d", | ||
| ExpressionAttributeNames = new Dictionary<string, string> { ["#d"] = "data" }, | ||
| ExpressionAttributeValues = new Dictionary<string, GoaModels.AttributeValue> | ||
| { | ||
| [":d"] = new() { S = $"updated-{i}" } | ||
| [":val"] = GoaModels.AttributeValue.String("updated-value") | ||
| } | ||
| }); | ||
| return response.Value; | ||
| return !response.IsError; | ||
| } |
Contributor
There was a problem hiding this comment.
Let Goa update failures fail the benchmark.
Returning !response.IsError turns a failed request into a successful timed result, while the AWS and Efficient paths still fail fast. That can make a broken Goa path look artificially fast instead of failing the run.
Possible fix
- return !response.IsError;
+ if (response.IsError)
+ throw new InvalidOperationException("Goa update benchmark failed.");
+ return true;Also applies to: 127-153
🤖 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/UpdateItemAsyncBenchmarks.cs`
around lines 57 - 79, The benchmark Goa_UpdateItem_SingleAttribute currently
returns !response.IsError which treats failed Goa.UpdateItemAsync responses as
successful benchmark results; change the method to fail fast on error by
checking response.IsError and throwing an exception (or rethrowing
response.Error) instead of returning a boolean, so failed Goa requests abort the
benchmark run; apply the same change to the other Goa benchmark(s) that return
!response.IsError.
double.TryParse and decimal.TryParse in ToValue<T>() used current culture, causing locale-dependent parsing of DynamoDB numbers.
DateOnly, TimeOnly, DateTime, Enum, and Complex type handlers used propertyInfo.Name instead of propertyInfo.GetDynamoAttributeName() in GenerateFromDynamoRecord, breaking properties with custom DynamoAttribute names.
return default silently created a misleading AttributeType.String struct with null value. Now throws JsonException to surface the error instead of propagating corrupt data.
…ict values CollectionTypeHandler filtered out NULL entries via .Where(), changing collection length on round-trip. Now preserves NULLs as null for nullable element types. ComplexTypeHandler threw ArgumentNullException for Dictionary<string, string?> null values; now emits AttributeValue.Null().
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/Clients/Goa.Clients.Dynamo/Internal/DynamoResponseReader.cs (1)
217-222:⚠️ Potential issue | 🟠 MajorReject
{"NULL": false}instead of coercing it to null.
reader.GetBoolean()only validates the token shape. This still accepts malformed wire data and returnsAttributeValue.Null().Suggested fix
if (reader.ValueTextEquals("NULL"u8)) { reader.Read(); - reader.GetBoolean(); - reader.Read(); // EndObject + if (!reader.GetBoolean()) + throw new JsonException("DynamoDB NULL attributes must contain true."); + if (!reader.Read() || reader.TokenType != JsonTokenType.EndObject) + throw new JsonException("Invalid AttributeValue JSON: expected end of NULL wrapper."); return AttributeValue.Null(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Clients/Goa.Clients.Dynamo/Internal/DynamoResponseReader.cs` around lines 217 - 222, The current DynamoResponseReader branch that handles reader.ValueTextEquals("NULL"u8) calls reader.GetBoolean() but ignores its return value, allowing {"NULL": false} to be accepted and coerced to AttributeValue.Null(); update the branch in DynamoResponseReader (the block that currently calls reader.GetBoolean(), reader.Read(), and returns AttributeValue.Null()) to capture the boolean result from reader.GetBoolean(), only return AttributeValue.Null() when that result is true, and throw or return a parse error when the boolean is false or not a valid true literal so malformed {"NULL": false} inputs are rejected.
🧹 Nitpick comments (1)
src/Clients/Goa.Clients.Dynamo/Internal/DynamoResponseReader.cs (1)
198-200: Code assumes single type property per wrapper but doesn't validate.The
ReadAttributeValuemethod relies on the DynamoDB JSON format guarantee that each attribute wrapper contains exactly one type property (e.g., only"S"or only"N", not both). While AWS DynamoDB enforces this at the service level, the code doesn't validate it—each branch reads and returns immediately, expecting the nextreader.Read()to land onEndObject. If a wrapper unexpectedly contained multiple properties, the reader would be left inside the object, desynchronizing the outer parse.Consider adding defensive validation: after processing a recognized type property, consume any remaining properties before returning, or explicitly verify the next token is
EndObject.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Clients/Goa.Clients.Dynamo/Internal/DynamoResponseReader.cs` around lines 198 - 200, In ReadAttributeValue, the code currently assumes the DynamoDB attribute wrapper contains exactly one property and returns after handling a known type, which can leave the JsonReader mid-object if extra properties exist; update ReadAttributeValue to validate or fully consume the wrapper: after detecting and handling a recognized type (e.g., the branches that read v = reader.GetString() / reader.GetDouble() / reader.GetBoolean() etc.), ensure the next token is JsonToken.EndObject or loop to skip any remaining property names/values until EndObject is reached, and if an unexpected additional property is encountered throw a descriptive exception; use the existing reader variable and the ReadAttributeValue method name to locate where to add this defensive check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/Clients/Goa.Clients.Dynamo/Internal/DynamoResponseReader.cs`:
- Around line 217-222: The current DynamoResponseReader branch that handles
reader.ValueTextEquals("NULL"u8) calls reader.GetBoolean() but ignores its
return value, allowing {"NULL": false} to be accepted and coerced to
AttributeValue.Null(); update the branch in DynamoResponseReader (the block that
currently calls reader.GetBoolean(), reader.Read(), and returns
AttributeValue.Null()) to capture the boolean result from reader.GetBoolean(),
only return AttributeValue.Null() when that result is true, and throw or return
a parse error when the boolean is false or not a valid true literal so malformed
{"NULL": false} inputs are rejected.
---
Nitpick comments:
In `@src/Clients/Goa.Clients.Dynamo/Internal/DynamoResponseReader.cs`:
- Around line 198-200: In ReadAttributeValue, the code currently assumes the
DynamoDB attribute wrapper contains exactly one property and returns after
handling a known type, which can leave the JsonReader mid-object if extra
properties exist; update ReadAttributeValue to validate or fully consume the
wrapper: after detecting and handling a recognized type (e.g., the branches that
read v = reader.GetString() / reader.GetDouble() / reader.GetBoolean() etc.),
ensure the next token is JsonToken.EndObject or loop to skip any remaining
property names/values until EndObject is reached, and if an unexpected
additional property is encountered throw a descriptive exception; use the
existing reader variable and the ReadAttributeValue method name to locate where
to add this defensive check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ace8b16d-aeba-4844-a4a4-5cb1f8a51037
📒 Files selected for processing (9)
src/Clients/Goa.Clients.Dynamo.Generator/TypeHandlers/CollectionTypeHandler.cssrc/Clients/Goa.Clients.Dynamo.Generator/TypeHandlers/ComplexTypeHandler.cssrc/Clients/Goa.Clients.Dynamo.Generator/TypeHandlers/DateOnlyTypeHandler.cssrc/Clients/Goa.Clients.Dynamo.Generator/TypeHandlers/DateTimeTypeHandler.cssrc/Clients/Goa.Clients.Dynamo.Generator/TypeHandlers/EnumTypeHandler.cssrc/Clients/Goa.Clients.Dynamo.Generator/TypeHandlers/TimeOnlyTypeHandler.cssrc/Clients/Goa.Clients.Dynamo/Internal/DynamoResponseReader.cssrc/Clients/Goa.Clients.Dynamo/Models/AttributeValue.cstests/Clients/Goa.Clients.Dynamo.Generator.Tests/TypeHandlers/ComplexTypeHandlerTests.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/Clients/Goa.Clients.Dynamo.Generator.Tests/TypeHandlers/ComplexTypeHandlerTests.cs
…eader
Reject {"NULL": false} instead of silently coercing to Null. Add
ReadEndObject helper to all 10 type branches to validate the wrapper
object is fully consumed, preventing malformed extra-property payloads.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/Clients/Goa.Clients.Dynamo.Tests/DynamoResponseReaderTests.cs (1)
855-870: Consider using UTF-8 string literals for consistency.These tests use
System.Text.Encoding.UTF8.GetBytes()while all other tests in the file use theu8suffix for UTF-8 string literals. Using theu8suffix is more concise and consistent with the rest of the test file.♻️ Suggested refactor for encoding consistency
[Test] public void ReadAttributeValue_NullFalse_ShouldThrowJsonException() { - var json = System.Text.Encoding.UTF8.GetBytes(""" + var json = """ { "Items": [], "Count": 0, "ScannedCount": 0, "LastEvaluatedKey": { "empty": {"NULL": false} } } - """); + """u8.ToArray(); Assert.Throws<JsonException>(() => DynamoResponseReader.ReadQueryResponse<TestEntity>(json, ReadTestEntity)); } // === Wrapper object validation === [Test] public void ReadAttributeValue_ExtraPropertyInWrapper_ShouldThrowJsonException() { - var json = System.Text.Encoding.UTF8.GetBytes(""" + var json = """ { "Items": [], "Count": 0, "ScannedCount": 0, "LastEvaluatedKey": { "pk": {"S": "value", "Extra": "bad"} } } - """); + """u8.ToArray(); Assert.Throws<JsonException>(() => DynamoResponseReader.ReadQueryResponse<TestEntity>(json, ReadTestEntity)); }Also applies to: 874-889
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Clients/Goa.Clients.Dynamo.Tests/DynamoResponseReaderTests.cs` around lines 855 - 870, Update the test to use UTF-8 string literals instead of System.Text.Encoding.UTF8.GetBytes: replace the JSON byte array construction in ReadAttributeValue_NullFalse_ShouldThrowJsonException with a u8 string literal and pass that byte span to DynamoResponseReader.ReadQueryResponse<TestEntity> (the call sites referencing DynamoResponseReader.ReadQueryResponse<TestEntity> and the delegate ReadTestEntity should remain unchanged); apply the same change to the other test block around lines 874-889 so all tests use the u8 suffix consistently.
🤖 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.Tests/DynamoResponseReaderTests.cs`:
- Around line 855-870: Update the test to use UTF-8 string literals instead of
System.Text.Encoding.UTF8.GetBytes: replace the JSON byte array construction in
ReadAttributeValue_NullFalse_ShouldThrowJsonException with a u8 string literal
and pass that byte span to DynamoResponseReader.ReadQueryResponse<TestEntity>
(the call sites referencing DynamoResponseReader.ReadQueryResponse<TestEntity>
and the delegate ReadTestEntity should remain unchanged); apply the same change
to the other test block around lines 874-889 so all tests use the u8 suffix
consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8867220d-337a-4e3d-9ea7-96be0aa6d235
📒 Files selected for processing (2)
src/Clients/Goa.Clients.Dynamo/Internal/DynamoResponseReader.cstests/Clients/Goa.Clients.Dynamo.Tests/DynamoResponseReaderTests.cs
…ation
Mirror the DynamoResponseReader hardening: validate EndObject token
after all 10 type branches and reject {"NULL": false} instead of
silently coercing to Null.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/Clients/Goa.Clients.Dynamo/Models/AttributeValue.cs (3)
123-126:⚠️ Potential issue | 🟠 Major
int/longstill round-trip through the current culture.These four paths are still culture-sensitive, so
Nvalues can format/parse differently across thread cultures even though thedouble/decimalbranches were fixed.Suggested fix
- public static implicit operator AttributeValue(int value) => Number(value.ToString()); + public static implicit operator AttributeValue(int value) => Number(value.ToString(CultureInfo.InvariantCulture)); - public static implicit operator AttributeValue(long value) => Number(value.ToString()); + public static implicit operator AttributeValue(long value) => Number(value.ToString(CultureInfo.InvariantCulture)); @@ - var t when t == typeof(int) => int.TryParse(N, out var i) ? (T?)(object?)i : default, - var t when t == typeof(long) => long.TryParse(N, out var l) ? (T?)(object?)l : default, + var t when t == typeof(int) => int.TryParse(N, NumberStyles.Integer, CultureInfo.InvariantCulture, out var i) ? (T?)(object?)i : default, + var t when t == typeof(long) => long.TryParse(N, NumberStyles.Integer, CultureInfo.InvariantCulture, out var l) ? (T?)(object?)l : default,Also applies to: 158-159
🤖 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 123 - 126, The int and long implicit operators (public static implicit operator AttributeValue(int value) and long) are still using value.ToString() which is culture-sensitive; change both to use value.ToString(CultureInfo.InvariantCulture) (and similarly update the other two affected paths noted around the file) and add a using System.Globalization if needed so numeric N values consistently format/parse invariantly when calling Number(...).
23-23:⚠️ Potential issue | 🟠 MajorReject
default(AttributeValue)explicitly.Because this is a public struct, zero-initialized instances will exist. Right now
Typeexposes_typedirectly andWrite()accepts it without a guard, sodefault(AttributeValue)can leak out as a real attribute or{}depending on whatever enum value maps to zero.Also applies to: 294-346
🤖 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` at line 23, AttributeValue is a public struct and can be zero-initialized; change the Type getter and any consumers like Write() to explicitly detect and reject default(AttributeValue) (e.g., check a private initialized flag or that _type is set to a non-zero/valid enum) and throw an InvalidOperationException (or ArgumentException) when the struct is uninitialized instead of returning/using the raw _type. Apply the same guard to the other affected members referenced (the Write method and the related code block around the other occurrences) so no zero-initialized AttributeValue can be treated as a valid attribute.
187-199:⚠️ Potential issue | 🟡 MinorMalformed
nulltokens still escape as the wrong exception type.These branches still rely on
reader.GetString()!, so inputs like{"S":null},{"N":null},{"B":null}, or{"SS":[null]}can bubble up asArgumentNullExceptionorFormatExceptioninstead of aJsonExceptionfrom the converter. Validate the token/value first and throwJsonExceptionexplicitly.Also applies to: 217-233, 257-270
🤖 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 187 - 199, The branches that call reader.GetString()! (e.g., when handling "S" -> AttributeValue.String(...), "N" -> AttributeValue.Number(...), and similarly "B", "SS", etc.) must validate the JSON token before dereferencing; replace direct GetString()/GetBytesFromBase64() calls with a check on reader.TokenType (ensure it's JsonTokenType.String or the expected StartArray/StartObject) and if the token is JsonTokenType.Null or otherwise unexpected throw a JsonException with a clear message instead of allowing ArgumentNullException/FormatException to escape; do the same for collection branches (e.g., "SS") by validating each element token and throwing JsonException on null elements, then proceed to Read() and call ReadEndObject(ref reader) and the AttributeValue factory (AttributeValue.String, AttributeValue.Number, AttributeValue.Binary, AttributeValue.StringSet, etc.).
🤖 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/Models/AttributeValue.cs`:
- Around line 274-276: The code in AttributeValue's JSON reader currently skips
unknown wrapper members (uses reader.Read(); reader.Skip()), which allows
malformed wrapper objects like {"X":1,"S":"ok"} to be accepted; instead, detect
the unexpected property name and immediately throw a JsonException. Update the
reader branch in the AttributeValue deserialization logic (replace the
reader.Read()/reader.Skip() path) to capture the current property name from the
Utf8JsonReader and throw a new JsonException indicating an unknown wrapper
member (include the property name in the message) rather than skipping it.
---
Duplicate comments:
In `@src/Clients/Goa.Clients.Dynamo/Models/AttributeValue.cs`:
- Around line 123-126: The int and long implicit operators (public static
implicit operator AttributeValue(int value) and long) are still using
value.ToString() which is culture-sensitive; change both to use
value.ToString(CultureInfo.InvariantCulture) (and similarly update the other two
affected paths noted around the file) and add a using System.Globalization if
needed so numeric N values consistently format/parse invariantly when calling
Number(...).
- Line 23: AttributeValue is a public struct and can be zero-initialized; change
the Type getter and any consumers like Write() to explicitly detect and reject
default(AttributeValue) (e.g., check a private initialized flag or that _type is
set to a non-zero/valid enum) and throw an InvalidOperationException (or
ArgumentException) when the struct is uninitialized instead of returning/using
the raw _type. Apply the same guard to the other affected members referenced
(the Write method and the related code block around the other occurrences) so no
zero-initialized AttributeValue can be treated as a valid attribute.
- Around line 187-199: The branches that call reader.GetString()! (e.g., when
handling "S" -> AttributeValue.String(...), "N" -> AttributeValue.Number(...),
and similarly "B", "SS", etc.) must validate the JSON token before
dereferencing; replace direct GetString()/GetBytesFromBase64() calls with a
check on reader.TokenType (ensure it's JsonTokenType.String or the expected
StartArray/StartObject) and if the token is JsonTokenType.Null or otherwise
unexpected throw a JsonException with a clear message instead of allowing
ArgumentNullException/FormatException to escape; do the same for collection
branches (e.g., "SS") by validating each element token and throwing
JsonException on null elements, then proceed to Read() and call
ReadEndObject(ref reader) and the AttributeValue factory (AttributeValue.String,
AttributeValue.Number, AttributeValue.Binary, AttributeValue.StringSet, etc.).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 27198bdb-f966-4170-b61b-e4cbe9bb9426
📒 Files selected for processing (1)
src/Clients/Goa.Clients.Dynamo/Models/AttributeValue.cs
Comment on lines
+274
to
+276
| // Skip unknown properties | ||
| reader.Read(); | ||
| reader.Skip(); |
Contributor
There was a problem hiding this comment.
Unknown wrapper members should fail, not be skipped.
A payload like {"X":1,"S":"ok"} currently succeeds: Lines 274-276 skip X, then the S branch returns normally. That still accepts malformed wrappers with extra properties; this branch should throw JsonException as soon as an unrecognized member is seen.
🤖 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 274 -
276, The code in AttributeValue's JSON reader currently skips unknown wrapper
members (uses reader.Read(); reader.Skip()), which allows malformed wrapper
objects like {"X":1,"S":"ok"} to be accepted; instead, detect the unexpected
property name and immediately throw a JsonException. Update the reader branch in
the AttributeValue deserialization logic (replace the
reader.Read()/reader.Skip() path) to capture the current property name from the
Utf8JsonReader and throw a new JsonException indicating an unknown wrapper
member (include the property name in the message) rather than skipping it.
Owner
Author
|
@coderabbitai resume |
Contributor
✅ Actions performedReviews resumed. |
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
…rison Only the 1-item benchmarks were active, giving misleading perf numbers. Typed Goa benchmark stays commented until PR #69 merges.
Only 100-item benchmarks active for meaningful comparison.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/Clients/Goa.Clients.Dynamo.Benchmarks/Benchmarks/QueryAsyncBenchmarks.cs (2)
302-324: Same concern: remove or extract commented-out code.Another ~100+ lines of commented-out benchmarks for "100 Items Typed" and "No Results" categories. Same recommendation applies as above.
Also applies to: 368-468
🤖 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 302 - 324, Remove the large blocks of commented-out benchmark methods (e.g., the Goa_Query_100Items_Typed method and the other commented benchmarks around lines referenced) from QueryAsyncBenchmarks.cs; either delete them entirely or extract them into a separate archival file (e.g., Benchmarks/Archived/QueryAsyncBenchmarks.Archive.cs) so the active test class only contains runnable code. Locate references by method name Goa_Query_100Items_Typed, the use of BenchmarkItem, _fixture.GoaClient.QueryAsync and DynamoItemReaderRegistry.Get<BenchmarkItem>() to ensure you move or remove the correct commented blocks, and if you need to keep them for future work add a short TODO note with a pointer to the archive rather than leaving large commented sections inline.
56-251: Remove or extract commented-out benchmark code.There are ~200 lines of commented-out benchmark methods here covering "1 Item" and "10 Items" categories. This amount of dead code harms readability and maintainability. If these benchmarks are intended for future use, consider:
- Delete and rely on version control - the history preserves this code if needed later
- Move to a separate file - create a
QueryAsyncBenchmarks.Disabled.cspartial class if you want to keep them accessible but not compiled- Add a clear TODO - if there's a specific reason these are temporarily disabled, document it
🤖 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 56 - 251, The file contains ~200 lines of commented-out benchmark methods (e.g., AwsSdk_Query_1Item, Goa_Query_1Item_DynamoRecord, Efficient_Query_1Item, Efficient_Query_1Item_Typed, AwsSdk_Query_10Items, Goa_Query_10Items_DynamoRecord, Goa_Query_10Items_Typed, Efficient_Query_10Items, Efficient_Query_10Items_Typed) which should be removed or extracted; either delete them (rely on VCS), move them into a separate partial file like QueryAsyncBenchmarks.Disabled.cs to keep them out of compilation, or add a brief TODO comment explaining why they’re intentionally disabled—choose one approach and apply it consistently to all commented benchmark blocks.
🤖 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 302-324: Remove the large blocks of commented-out benchmark
methods (e.g., the Goa_Query_100Items_Typed method and the other commented
benchmarks around lines referenced) from QueryAsyncBenchmarks.cs; either delete
them entirely or extract them into a separate archival file (e.g.,
Benchmarks/Archived/QueryAsyncBenchmarks.Archive.cs) so the active test class
only contains runnable code. Locate references by method name
Goa_Query_100Items_Typed, the use of BenchmarkItem,
_fixture.GoaClient.QueryAsync and DynamoItemReaderRegistry.Get<BenchmarkItem>()
to ensure you move or remove the correct commented blocks, and if you need to
keep them for future work add a short TODO note with a pointer to the archive
rather than leaving large commented sections inline.
- Around line 56-251: The file contains ~200 lines of commented-out benchmark
methods (e.g., AwsSdk_Query_1Item, Goa_Query_1Item_DynamoRecord,
Efficient_Query_1Item, Efficient_Query_1Item_Typed, AwsSdk_Query_10Items,
Goa_Query_10Items_DynamoRecord, Goa_Query_10Items_Typed,
Efficient_Query_10Items, Efficient_Query_10Items_Typed) which should be removed
or extracted; either delete them (rely on VCS), move them into a separate
partial file like QueryAsyncBenchmarks.Disabled.cs to keep them out of
compilation, or add a brief TODO comment explaining why they’re intentionally
disabled—choose one approach and apply it consistently to all commented
benchmark blocks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3fad408a-3137-4434-806f-d7ee375bc024
📒 Files selected for processing (1)
tests/Clients/Goa.Clients.Dynamo.Benchmarks/Benchmarks/QueryAsyncBenchmarks.cs
Owner
Author
Baseline
PR
Note: Docker must have been having a good time whilst running the pr as everything is faster. Main thing that we were looking for is the reduction in the allocations which is good here. |
Im5tu
added a commit
that referenced
this pull request
Mar 12, 2026
AttributeValue is now a readonly struct after PR #70 merge; property setters no longer exist.
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
AttributeValuefrom a class to areadonly structwith union-style typed storageAttributeTypeenum for strongly-typed attribute classificationString(),Number(),Bool(),Null(),FromList(),FromMap(), etc.) replacing object initializers