Conversation
📝 WalkthroughWalkthroughAdds support for returning attribute values on conditional check failures and reporting item-collection metrics. Introduces new enum and model types, extends request/response shapes and transaction items, adds builder fluent APIs, tightens condition-expression merging, updates JSON source-gen, and adds tests. Changes
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)tests/Clients/Goa.Clients.Dynamo.Tests/BuilderChainingTests.cs (6)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (4)
✏️ Tip: You can disable this entire section by setting Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@src/Clients/Goa.Clients.Dynamo/Operations/DeleteItem/DeleteItemRequest.cs`:
- Around line 52-56: DynamoJsonContext is missing an explicit JsonSerializable
registration for the enum ReturnValuesOnConditionCheckFailure and there is no
test ensuring WithReturnValuesOnConditionCheckFailure() gets serialized; add
[JsonSerializable(typeof(ReturnValuesOnConditionCheckFailure))] to the
DynamoJsonContext attribute list (alongside ReturnValues,
ReturnConsumedCapacity, ReturnItemCollectionMetrics) so the enum is explicitly
wired into the source-generated JSON context, then add a unit test that uses the
DeleteItem request builder (call
WithReturnValuesOnConditionCheckFailure(ReturnValuesOnConditionCheckFailure.ALL_OLD)
or NONE) and asserts the produced serialized request includes the correct
ReturnValuesOnConditionCheckFailure value sent to DynamoDB (mirroring existing
builder tests for ReturnValues).
In `@src/Clients/Goa.Clients.Dynamo/Operations/PutItem/PutItemRequest.cs`:
- Around line 51-55: DynamoJsonContext is missing a JsonSerializable entry for
the new enum type ReturnValuesOnConditionCheckFailure; add a JsonSerializable
attribute for that enum to DynamoJsonContext (i.e., add
[JsonSerializable(typeof(ReturnValuesOnConditionCheckFailure))] alongside the
existing entries for ReturnValues, ReturnConsumedCapacity, and
ReturnItemCollectionMetrics) so the enum is explicitly registered for the
source-generated JSON context and AOT compilation.
In `@src/Clients/Goa.Clients.Dynamo/Operations/Query/QueryBuilder.cs`:
- Around line 66-81: The XML doc for QueryBuilder.WithIndex is incorrect: length
validation uses ArgumentOutOfRangeException while invalid-pattern uses
ArgumentException; update the <exception> tags for WithIndex to document
ArgumentOutOfRangeException for length checks (when indexName.Length < 3 or >
255) and keep/document ArgumentException for pattern validation (when
IndexNamePattern().IsMatch fails), ensuring the summary matches the actual
thrown exceptions.
In `@src/Clients/Goa.Clients.Dynamo/Operations/UpdateItem/UpdateItemBuilder.cs`:
- Around line 803-820: UpdateItemBuilder.WithCondition currently overwrites
prior conditions and doesn't guard against empty condition.Expression; change it
to match DeleteItemBuilder's multi-condition behavior: if condition.Expression
is null or empty, return without modifying; if _request.ConditionExpression is
null or empty set it to condition.Expression, otherwise set it to "(" +
_request.ConditionExpression + ") AND (" + condition.Expression + ")"; keep the
existing ExpressionAttributeNames/ExpressionAttributeValues merge logic (using
_request.ExpressionAttributeNames ??= ... and Merge) so attribute maps still
combine across calls. Also apply the same empty-expression guard pattern to
PutItemBuilder.WithCondition to ensure consistent behavior across builders.
- Around line 844-853: Add unit tests mirroring the existing
UpdateItemBuilder_WithCondition_* patterns (see ConditionExpressionValuesTests)
that verify UpdateItemBuilder.WithReturnValuesOnConditionCheckFailure(...) and
UpdateItemBuilder.WithReturnValues(...) correctly propagate values into the
built request; instantiate an UpdateItemBuilder, call the method under test with
each relevant enum value, call Build() (or however the request is produced in
tests), and assert the resulting request's ReturnValuesOnConditionCheckFailure
and ReturnValues properties equal the values passed.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
src/Clients/Goa.Clients.Dynamo/Enums/ReturnValuesOnConditionCheckFailure.cssrc/Clients/Goa.Clients.Dynamo/Models/ItemCollectionMetrics.cssrc/Clients/Goa.Clients.Dynamo/Operations/Batch/BatchWriteItemResponse.cssrc/Clients/Goa.Clients.Dynamo/Operations/DeleteItem/DeleteItemBuilder.cssrc/Clients/Goa.Clients.Dynamo/Operations/DeleteItem/DeleteItemRequest.cssrc/Clients/Goa.Clients.Dynamo/Operations/DeleteItem/DeleteItemResponse.cssrc/Clients/Goa.Clients.Dynamo/Operations/PutItem/PutItemBuilder.cssrc/Clients/Goa.Clients.Dynamo/Operations/PutItem/PutItemRequest.cssrc/Clients/Goa.Clients.Dynamo/Operations/Query/QueryBuilder.cssrc/Clients/Goa.Clients.Dynamo/Operations/Scan/ScanBuilder.cssrc/Clients/Goa.Clients.Dynamo/Operations/Transactions/TransactConditionCheckItem.cssrc/Clients/Goa.Clients.Dynamo/Operations/Transactions/TransactDeleteItem.cssrc/Clients/Goa.Clients.Dynamo/Operations/Transactions/TransactGetBuilder.cssrc/Clients/Goa.Clients.Dynamo/Operations/Transactions/TransactGetRequest.cssrc/Clients/Goa.Clients.Dynamo/Operations/Transactions/TransactPutItem.cssrc/Clients/Goa.Clients.Dynamo/Operations/Transactions/TransactUpdateItem.cssrc/Clients/Goa.Clients.Dynamo/Operations/UpdateItem/UpdateItemBuilder.cssrc/Clients/Goa.Clients.Dynamo/Operations/UpdateItem/UpdateItemRequest.cssrc/Clients/Goa.Clients.Dynamo/Operations/UpdateItem/UpdateItemResponse.cstests/Clients/Goa.Clients.Dynamo.Tests/ConditionExpressionValuesTests.cs
🧰 Additional context used
🧬 Code graph analysis (4)
src/Clients/Goa.Clients.Dynamo/Operations/DeleteItem/DeleteItemResponse.cs (1)
src/Clients/Goa.Clients.Dynamo/Models/ItemCollectionMetrics.cs (1)
ItemCollectionMetrics(6-18)
src/Clients/Goa.Clients.Dynamo/Operations/UpdateItem/UpdateItemResponse.cs (1)
src/Clients/Goa.Clients.Dynamo/Models/ItemCollectionMetrics.cs (1)
ItemCollectionMetrics(6-18)
src/Clients/Goa.Clients.Dynamo/Operations/Batch/BatchWriteItemResponse.cs (1)
src/Clients/Goa.Clients.Dynamo/Models/ItemCollectionMetrics.cs (1)
ItemCollectionMetrics(6-18)
tests/Clients/Goa.Clients.Dynamo.Tests/ConditionExpressionValuesTests.cs (5)
src/Clients/Goa.Clients.Dynamo/Operations/PutItem/PutItemBuilder.cs (7)
PutItemBuilder(11-107)PutItemBuilder(24-28)PutItemBuilder(36-40)PutItemBuilder(47-64)PutItemBuilder(71-75)PutItemBuilder(82-86)PutItemBuilder(93-97)src/Clients/Goa.Clients.Dynamo/Operations/DeleteItem/DeleteItemBuilder.cs (7)
DeleteItemBuilder(11-120)DeleteItemBuilder(25-29)DeleteItemBuilder(36-60)DeleteItemBuilder(68-77)DeleteItemBuilder(84-88)DeleteItemBuilder(95-99)DeleteItemBuilder(106-110)src/Clients/Goa.Clients.Dynamo/Operations/UpdateItem/UpdateItemBuilder.cs (16)
UpdateItemBuilder(13-940)UpdateItemBuilder(33-37)UpdateItemBuilder(47-57)UpdateItemBuilder(64-76)UpdateItemBuilder(84-97)UpdateItemBuilder(106-126)UpdateItemBuilder(134-147)UpdateItemBuilder(156-169)UpdateItemBuilder(177-188)UpdateItemBuilder(198-198)UpdateItemBuilder(204-204)UpdateItemBuilder(210-210)UpdateItemBuilder(216-216)UpdateItemBuilder(222-222)UpdateItemBuilder(228-228)UpdateItemBuilder(234-234)src/Clients/Goa.Clients.Dynamo/Operations/Query/QueryBuilder.cs (13)
QueryBuilder(12-234)QueryBuilder(24-48)QueryBuilder(55-59)QueryBuilder(67-85)QueryBuilder(96-105)QueryBuilder(111-115)QueryBuilder(121-125)QueryBuilder(132-142)QueryBuilder(149-159)QueryBuilder(166-170)QueryBuilder(177-181)QueryBuilder(188-192)QueryBuilder(200-224)src/Clients/Goa.Clients.Dynamo/Operations/Scan/ScanBuilder.cs (11)
ScanBuilder(11-173)ScanBuilder(23-47)ScanBuilder(54-58)ScanBuilder(65-69)ScanBuilder(76-80)ScanBuilder(87-97)ScanBuilder(104-114)ScanBuilder(121-125)ScanBuilder(132-136)ScanBuilder(144-152)ScanBuilder(159-163)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (23)
src/Clients/Goa.Clients.Dynamo/Operations/Scan/ScanBuilder.cs (1)
34-44: Good: conditional merge avoids mutating request with empty expression maps.
Condition.ExpressionNamesandCondition.ExpressionValuesare always initialized as empty dictionaries, so calling.Countis safe. The pattern correctly checks before merging to keep the request clean.src/Clients/Goa.Clients.Dynamo/Operations/Transactions/TransactPutItem.cs (1)
1-2: Mapping and serialization are correctly configured.TransactPutItem is registered in DynamoJsonContext with
[JsonSerializable]and the enum serialization is handled byUseStringEnumConverter = trueat the context level, so ReturnValuesOnConditionCheckFailure values will be emitted as strings.Minor note: This file has a UTF-8 BOM (
)—consider normalizing encoding across the repo if inconsistent elsewhere.src/Clients/Goa.Clients.Dynamo/Operations/Transactions/TransactDeleteItem.cs (1)
1-2: TransactDeleteItem correctly integrated. The using statements (lines 1-2) are properly added, theReturnValuesOnConditionCheckFailureproperty is present at line 40 with defaultNONE, and the class is wired into the transact payload viaTransactWriteItem.Delete→TransactWriteRequest.TransactItems. TheDynamoJsonContextconfiguration usesUseStringEnumConverter = true(line 22), ensuring enum values serialize as strings.src/Clients/Goa.Clients.Dynamo/Enums/ReturnValuesOnConditionCheckFailure.cs (1)
1-17: Enum will serialize to Dynamo string values by default; no custom converter needed.
AWS SDK .NET serializes enums to DynamoDB strings automatically. Your enum members (NONE,ALL_OLD) will emit as string tokens matching DynamoDB's expected values. However, verify in integration tests that the request/response mappers correctly integrate this enum with DynamoDB operations.src/Clients/Goa.Clients.Dynamo/Operations/Transactions/TransactConditionCheckItem.cs (1)
1-1: Looks good — but ensure request serialization/mapping matches DynamoDB’s expected values forReturnValuesOnConditionCheckFailure.If this library translates to AWS SDK models, please confirm
NONE/ALL_OLDare emitted exactly as DynamoDB expects (and that the property is included in the transaction condition-check payload only when supported).Also applies to: 35-40
src/Clients/Goa.Clients.Dynamo/Operations/Transactions/TransactUpdateItem.cs (1)
1-2: LGTM — same note: verify enum serialization/mapping forReturnValuesOnConditionCheckFailure.Also applies to: 40-45
src/Clients/Goa.Clients.Dynamo/Operations/PutItem/PutItemBuilder.cs (2)
47-64: Good: avoid creating/merging empty ExpressionAttribute dictionaries.*Please sanity-check
Goa.Core.Merge(...)behavior on key collisions (overwrite vs exception) since this method is now the only place that merges condition maps into the request.
88-97: Nice fluent addition forReturnValuesOnConditionCheckFailure.src/Clients/Goa.Clients.Dynamo/Models/ItemCollectionMetrics.cs (1)
1-18: LGTM — model matches DynamoDB’s “item collection metrics” shape.Just confirm your serializer’s naming policy keeps
SizeEstimateRangeGBaligned with the upstream payload field name you’re consuming/producing.src/Clients/Goa.Clients.Dynamo/Operations/Query/QueryBuilder.cs (3)
35-46: Good: only merge ExpressionAttribute when condition provides entries.*Also applies to: 211-222
95-104: Good guardrail:limitmust be >= 1.
12-12:[GeneratedRegex]is fully compatible with the project configuration. The project targets net9.0 and net10.0 withLangVersionset tolatest, which exceeds the requirements for the[GeneratedRegex]attribute and partial method support (net7.0+ and C# 11+).src/Clients/Goa.Clients.Dynamo/Operations/DeleteItem/DeleteItemResponse.cs (1)
20-23: Verify compilation and response mapping forItemCollectionMetrics.
Same note as UpdateItemResponse: addusing System.Collections.Generic;if implicit/global usings aren’t enabled, and confirm deserialization maps the DynamoDB field correctly.Proposed minimal compile-safety tweak (if implicit/global usings are NOT enabled)
+using System.Collections.Generic; using Goa.Clients.Dynamo.Models;src/Clients/Goa.Clients.Dynamo/Operations/UpdateItem/UpdateItemRequest.cs (2)
34-34: No issue found. All callers use??=coalescing assignment before accessingExpressionAttributeValues.The nullable change is safe. Every access in UpdateItemBuilder, QueryBuilder, ScanBuilder, PutItemBuilder, and DeleteItemBuilder follows the pattern of
_request.ExpressionAttributeValues ??= new(...)before calling.Count,.Merge(), or indexing. No unsafe read patterns exist, and System.Text.Json serialization handles null properties correctly by omitting them (standard DynamoDB API behavior).Likely an incorrect or invalid review comment.
56-60: ReturnValuesOnConditionCheckFailure serialization verified as correct.Enum values (NONE, ALL_OLD) and property field name match DynamoDB's API contract. The serialization context uses
UseStringEnumConverter = truewithPropertyNamingPolicy = JsonKnownNamingPolicy.Unspecified, ensuring enums serialize as exact uppercase strings and the property name remains unchanged in the request.src/Clients/Goa.Clients.Dynamo/Operations/UpdateItem/UpdateItemResponse.cs (1)
20-23: No changes needed—ImplicitUsings is enabled project-wide.The project's root
Directory.Build.propshas<ImplicitUsings>enable</ImplicitUsings>, soSystem.Collections.Genericis available implicitly. TheItemCollectionMetricsclass is properly defined in the Models folder and follows AWS DynamoDB naming conventions. No explicit using statement is required, and the property will compile correctly as-is.Likely an incorrect or invalid review comment.
src/Clients/Goa.Clients.Dynamo/Operations/Transactions/TransactGetRequest.cs (2)
15-18: Property addition looks consistent with the builder and AWS shape.
1-1: This comment is incorrect. The enum serialization is properly configured.DynamoJsonContextexplicitly setsUseStringEnumConverter = truein its[JsonSourceGenerationOptions]attribute (line 22), which ensures all enums in this context—includingReturnConsumedCapacity—serialize to string values ("NONE", "INDEXES", "TOTAL") as expected by DynamoDB, not numeric values.Likely an incorrect or invalid review comment.
src/Clients/Goa.Clients.Dynamo/Operations/Batch/BatchWriteItemResponse.cs (1)
26-30:ItemCollectionMetricsresponse surface addition looks correct and consistent with the model.src/Clients/Goa.Clients.Dynamo/Operations/DeleteItem/DeleteItemBuilder.cs (2)
47-57: Good fix: don’t allocate/merge empty ExpressionAttributeNames/Values maps.
101-110: No action needed. TheReturnValuesOnConditionCheckFailureenum is already properly configured with string serialization (viaUseStringEnumConverter = truein DynamoJsonContext.cs) and scoped to only the two values supported by AWS DynamoDB: NONE and ALL_OLD. The strongly-typed parameter in the builder method provides compile-time type safety, preventing invalid values from ever being passed.Likely an incorrect or invalid review comment.
tests/Clients/Goa.Clients.Dynamo.Tests/ConditionExpressionValuesTests.cs (1)
11-15: Nice: tests explicitly lock in the “null, not empty dictionary” behavior to prevent the DynamoDB ValidationException regression.src/Clients/Goa.Clients.Dynamo/Operations/Transactions/TransactGetBuilder.cs (1)
1-1:WithReturnConsumedCapacityis a clean fluent addition with complete end-to-end serialization coverage.The
ReturnConsumedCapacityproperty is properly defined inTransactGetRequest, correctly set by the builder method, registered as[JsonSerializable], and ultimately passed toDynamoServiceClient.SendAsync. The implementation follows the established pattern across other builders in the codebase (TransactWriteBuilder, ScanBuilder, QueryBuilder, etc.).
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| /// <summary> | ||
| /// Specifies how to return attribute values when a conditional check fails. | ||
| /// Use ALL_OLD to return all attributes of the item as they appeared before the operation. | ||
| /// </summary> | ||
| public ReturnValuesOnConditionCheckFailure ReturnValuesOnConditionCheckFailure { get; set; } = ReturnValuesOnConditionCheckFailure.NONE; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
git ls-files | grep -E "(Delete|Dynamo)" | head -20Repository: Im5tu/goa
Length of output: 1599
🏁 Script executed:
find . -name "*DeleteItem*" -o -name "*ReturnValuesOnConditionCheckFailure*" | head -20Repository: Im5tu/goa
Length of output: 493
🏁 Script executed:
fd -t f "DeleteItemRequest" | head -10Repository: Im5tu/goa
Length of output: 126
🏁 Script executed:
fd -t f "ReturnValuesOnConditionCheckFailure" | head -10Repository: Im5tu/goa
Length of output: 128
🏁 Script executed:
cat -n src/Clients/Goa.Clients.Dynamo/Enums/ReturnValuesOnConditionCheckFailure.csRepository: Im5tu/goa
Length of output: 595
🏁 Script executed:
cat -n src/Clients/Goa.Clients.Dynamo/Operations/DeleteItem/DeleteItemRequest.csRepository: Im5tu/goa
Length of output: 2759
🏁 Script executed:
cat -n src/Clients/Goa.Clients.Dynamo/Operations/DeleteItem/DeleteItemBuilder.csRepository: Im5tu/goa
Length of output: 5725
🏁 Script executed:
cat -n src/Clients/Goa.Clients.Dynamo/Operations/DeleteItem/DeleteItemResponse.csRepository: Im5tu/goa
Length of output: 1002
🏁 Script executed:
find . -path "*/DeleteItem*" -name "*.cs" | xargs ls -laRepository: Im5tu/goa
Length of output: 428
🏁 Script executed:
find . -name "*Serializer*" -o -name "*WireModel*" -o -name "*Wire*" | grep -i dynamoRepository: Im5tu/goa
Length of output: 35
🏁 Script executed:
fd -t f "DeleteItem" --exec grep -l "ReturnValuesOnConditionCheckFailure" {} \;Repository: Im5tu/goa
Length of output: 283
🏁 Script executed:
fd -t f "Test" | grep -i deleteitemRepository: Im5tu/goa
Length of output: 35
🏁 Script executed:
git ls-files | grep -i test | head -20Repository: Im5tu/goa
Length of output: 1737
🏁 Script executed:
find . -name "*.Tests.cs" -o -name "*Tests" -type d | head -10Repository: Im5tu/goa
Length of output: 379
🏁 Script executed:
rg "ReturnValuesOnConditionCheckFailure" --type csRepository: Im5tu/goa
Length of output: 2823
🏁 Script executed:
cat -n src/Clients/Goa.Clients.Dynamo/Operations/Transactions/TransactDeleteItem.csRepository: Im5tu/goa
Length of output: 1827
🏁 Script executed:
find . -path "*/DeleteItem*" -o -name "*Serializer*" | xargs rg -l "DeleteItem" 2>/dev/null | head -10Repository: Im5tu/goa
Length of output: 510
🏁 Script executed:
fd -t f "Dynamo.*Test" tests/Clients/Goa.Clients.Dynamo.Tests/Repository: Im5tu/goa
Length of output: 254
🏁 Script executed:
ls -la tests/Clients/Goa.Clients.Dynamo.Tests/Repository: Im5tu/goa
Length of output: 630
🏁 Script executed:
find tests/Clients/Goa.Clients.Dynamo.Tests -name "*.cs" | grep -i deleteRepository: Im5tu/goa
Length of output: 35
🏁 Script executed:
rg "DeleteItem" tests/Clients/Goa.Clients.Dynamo.Tests/ -C 3Repository: Im5tu/goa
Length of output: 10190
🏁 Script executed:
rg "ReturnValuesOnConditionCheckFailure" tests/ -C 2Repository: Im5tu/goa
Length of output: 35
🏁 Script executed:
cat -n tests/Clients/Goa.Clients.Dynamo.Tests/DynamoClientIntegrationTests.cs | head -100Repository: Im5tu/goa
Length of output: 3900
🏁 Script executed:
find . -name "*.cs" -path "*/Dynamo/*" | xargs rg -l "ToJson|Serialize|Wire" | head -5Repository: Im5tu/goa
Length of output: 315
🏁 Script executed:
find . -name "*.cs" -path "*/Dynamo/*" | xargs rg -l "JsonSerializableAttribute|JsonConverter|JsonPropertyName" | head -10Repository: Im5tu/goa
Length of output: 785
🏁 Script executed:
find . -name "*Client*.cs" -path "*/Dynamo/*" | head -10Repository: Im5tu/goa
Length of output: 35
🏁 Script executed:
fd -t f "DynamoClient" | grep -v Test | head -5Repository: Im5tu/goa
Length of output: 100
🏁 Script executed:
cat -n $(fd -t f "DynamoClient" | grep -v Test | head -1)Repository: Im5tu/goa
Length of output: 6315
🏁 Script executed:
find . -name "DynamoClient.cs" -o -name "*DynamoClient*.cs" | grep -v Test | grep -v InterfaceRepository: Im5tu/goa
Length of output: 102
🏁 Script executed:
rg "class DynamoClient" src/Clients/Goa.Clients.Dynamo/Repository: Im5tu/goa
Length of output: 35
🏁 Script executed:
find src/Clients/Goa.Clients.Dynamo -name "*.cs" -type f | xargs grep -l "DeleteItemRequest" | grep -v Operations | head -5Repository: Im5tu/goa
Length of output: 220
🏁 Script executed:
cat -n src/Clients/Goa.Clients.Dynamo/DynamoServiceClient.cs | head -150Repository: Im5tu/goa
Length of output: 7640
🏁 Script executed:
cat -n src/Clients/Goa.Clients.Dynamo/Serialization/DynamoJsonContext.csRepository: Im5tu/goa
Length of output: 3678
🏁 Script executed:
rg "ReturnValuesOnConditionCheckFailure" src/Clients/Goa.Clients.Dynamo/Serialization/Repository: Im5tu/goa
Length of output: 35
🏁 Script executed:
rg "UseStringEnumConverter\|JsonConverter\|ReturnValuesOnConditionCheckFailure" src/Clients/Goa.Clients.Dynamo/Serialization/ -A 2 -B 2Repository: Im5tu/goa
Length of output: 35
🏁 Script executed:
grep -n "ReturnValuesOnConditionCheckFailure" src/Clients/Goa.Clients.Dynamo/Serialization/DynamoJsonContext.csRepository: Im5tu/goa
Length of output: 35
🏁 Script executed:
find . -name "*.cs" -path "*/Serialization/*" -o -name "*Converter*.cs" | xargs grep -l "ReturnValuesOnConditionCheckFailure" 2>/dev/nullRepository: Im5tu/goa
Length of output: 37
🌐 Web query:
AWS DynamoDB DeleteItem API ReturnValuesOnConditionCheckFailure parameter valid values enum
💡 Result:
Valid values: ALL_OLD and NONE. [1][2]
Sources:
- AWS DeleteItem API reference (ReturnValuesOnConditionCheckFailure). [1]
- AWS SDK Java enum ReturnValuesOnConditionCheckFailure (ALL_OLD, NONE). [2]
🏁 Script executed:
cat -n src/Clients/Goa.Clients.Dynamo/Serialization/DynamoJsonContext.cs | grep -E "JsonSerializable.*Delete|JsonSerializable.*Return"Repository: Im5tu/goa
Length of output: 431
🏁 Script executed:
rg "class DeleteItemRequest" src/Clients/Goa.Clients.Dynamo/Operations/DeleteItem/DeleteItemRequest.cs -A 10 | head -20Repository: Im5tu/goa
Length of output: 463
🏁 Script executed:
find . -name "*.cs" -path "*/Serialization/*" | xargs cat | grep -i "returnvalues"Repository: Im5tu/goa
Length of output: 93
🏁 Script executed:
python3 << 'EOF'
# Check if ReturnValuesOnConditionCheckFailure would be serialized as part of DeleteItemRequest
# when UseStringEnumConverter is set globally
import json
# The question is: does the enum get serialized when it's a property of a JsonSerializable class?
# With UseStringEnumConverter = true globally, nested enums should serialize as strings
# Looking at the context:
# - DeleteItemRequest is [JsonSerializable] (line 32)
# - UseStringEnumConverter = true (line 22)
# - ReturnValuesOnConditionCheckFailure is a property of DeleteItemRequest
# - The enum is NOT explicitly in [JsonSerializable] attributes
# This means the enum MIGHT serialize correctly due to global setting, but it's not explicit
print("ANALYSIS:")
print("1. DeleteItemRequest IS in [JsonSerializable] list (line 32)")
print("2. UseStringEnumConverter = true is set globally (line 22)")
print("3. ReturnValuesOnConditionCheckFailure property is part of DeleteItemRequest")
print("")
print("POTENTIAL ISSUE:")
print("- The enum is NOT explicitly listed in [JsonSerializable] attributes")
print("- While global UseStringEnumConverter may handle it, best practice is explicit")
print("- Missing from serialization context like other enums (ReturnValues, ReturnConsumedCapacity, etc.)")
EOFRepository: Im5tu/goa
Length of output: 139
🏁 Script executed:
# Let's verify AWS parameter field name
web_search_query="AWS DynamoDB DeleteItem API request parameter field name ReturnValuesOnConditionCheckFailure JSON"
echo "Searching for field name mapping in AWS docs..."Repository: Im5tu/goa
Length of output: 100
🏁 Script executed:
rg "WithReturnValuesOnConditionCheckFailure\|ReturnValuesOnConditionCheckFailure\s*=" tests/ -C 2Repository: Im5tu/goa
Length of output: 35
Add ReturnValuesOnConditionCheckFailure to the serialization context and add test coverage.
While the enum values (NONE, ALL_OLD) correctly match AWS DynamoDB's DeleteItem API contract and the property is properly integrated into the request/builder, two issues need addressing:
-
The enum should be explicitly registered in
DynamoJsonContextas[JsonSerializable(typeof(ReturnValuesOnConditionCheckFailure))](lines 19-62) for consistency with similar enums likeReturnValues,ReturnConsumedCapacity, andReturnItemCollectionMetrics, even though the globalUseStringEnumConvertersetting may handle it implicitly. -
Add a test case for
WithReturnValuesOnConditionCheckFailure()to verify the property is correctly serialized and sent to DynamoDB, similar to existing builder tests.
🤖 Prompt for AI Agents
In `@src/Clients/Goa.Clients.Dynamo/Operations/DeleteItem/DeleteItemRequest.cs`
around lines 52 - 56, DynamoJsonContext is missing an explicit JsonSerializable
registration for the enum ReturnValuesOnConditionCheckFailure and there is no
test ensuring WithReturnValuesOnConditionCheckFailure() gets serialized; add
[JsonSerializable(typeof(ReturnValuesOnConditionCheckFailure))] to the
DynamoJsonContext attribute list (alongside ReturnValues,
ReturnConsumedCapacity, ReturnItemCollectionMetrics) so the enum is explicitly
wired into the source-generated JSON context, then add a unit test that uses the
DeleteItem request builder (call
WithReturnValuesOnConditionCheckFailure(ReturnValuesOnConditionCheckFailure.ALL_OLD)
or NONE) and asserts the produced serialized request includes the correct
ReturnValuesOnConditionCheckFailure value sent to DynamoDB (mirroring existing
builder tests for ReturnValues).
src/Clients/Goa.Clients.Dynamo/Operations/Query/QueryBuilder.cs
Outdated
Show resolved
Hide resolved
| public UpdateItemBuilder WithCondition(Condition condition) | ||
| { | ||
| _request.ConditionExpression = condition.Expression; | ||
| _request.ExpressionAttributeNames ??= new(StringComparer.OrdinalIgnoreCase); | ||
| _request.ExpressionAttributeValues ??= new(StringComparer.OrdinalIgnoreCase); | ||
| _request.ExpressionAttributeNames.Merge(condition.ExpressionNames); | ||
| _request.ExpressionAttributeValues.Merge(condition.ExpressionValues); | ||
|
|
||
| if (condition.ExpressionNames.Count > 0) | ||
| { | ||
| _request.ExpressionAttributeNames ??= new(StringComparer.OrdinalIgnoreCase); | ||
| _request.ExpressionAttributeNames.Merge(condition.ExpressionNames); | ||
| } | ||
|
|
||
| if (condition.ExpressionValues.Count > 0) | ||
| { | ||
| _request.ExpressionAttributeValues ??= new(StringComparer.OrdinalIgnoreCase); | ||
| _request.ExpressionAttributeValues.Merge(condition.ExpressionValues); | ||
| } | ||
|
|
||
| return this; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find src -name "*.cs" | head -20Repository: Im5tu/goa
Length of output: 1260
🏁 Script executed:
rg -n --type=cs 'class Condition\b|record Condition\b|struct Condition\b' src/Repository: Im5tu/goa
Length of output: 132
🏁 Script executed:
rg -n --type=cs 'WithCondition\(|WithReturnValuesOnConditionCheckFailure\(' src/ -A 2Repository: Im5tu/goa
Length of output: 2536
🏁 Script executed:
cat -n src/Clients/Goa.Clients.Dynamo/Operations/Condition.csRepository: Im5tu/goa
Length of output: 20578
🏁 Script executed:
head -50 src/Clients/Goa.Clients.Dynamo/Operations/DeleteItem/DeleteItemBuilder.csRepository: Im5tu/goa
Length of output: 1907
🏁 Script executed:
rg -n 'WithCondition|WithReturnValuesOnConditionCheckFailure' tests/ -A 5 -B 2 | head -100Repository: Im5tu/goa
Length of output: 10834
🏁 Script executed:
rg -n 'UpdateItemBuilder.*WithCondition' tests/ -A 10 -B 2 | head -80Repository: Im5tu/goa
Length of output: 4755
🏁 Script executed:
rg -n '\.WithCondition\(.*\).*\.WithCondition\(' src/Clients/Goa.Clients.Dynamo --type=cs -A 2 -B 2Repository: Im5tu/goa
Length of output: 35
🏁 Script executed:
sed -n '800,860p' src/Clients/Goa.Clients.Dynamo/Operations/UpdateItem/UpdateItemBuilder.csRepository: Im5tu/goa
Length of output: 2469
🏁 Script executed:
rg -n 'WithReturnValuesOnConditionCheckFailure' tests/ -B 5 -A 10 | head -60Repository: Im5tu/goa
Length of output: 35
🏁 Script executed:
sed -n '36,55p' src/Clients/Goa.Clients.Dynamo/Operations/DeleteItem/DeleteItemBuilder.csRepository: Im5tu/goa
Length of output: 770
🏁 Script executed:
sed -n '47,66p' src/Clients/Goa.Clients.Dynamo/Operations/PutItem/PutItemBuilder.csRepository: Im5tu/goa
Length of output: 709
🏁 Script executed:
rg -n 'WithCondition.*WithCondition' tests/ -C 3Repository: Im5tu/goa
Length of output: 35
Inconsistent handling of multiple conditions: DeleteItemBuilder properly supports chaining multiple WithCondition() calls by combining expressions with AND, but UpdateItemBuilder and PutItemBuilder silently overwrite the previous condition expression. Align all builders to use the same multi-condition pattern as DeleteItemBuilder, or document that only a single condition is supported.
The dictionary merging optimization is correct—guarding against empty collections avoids allocating and serializing unnecessary attributes. However, consider adding a defensive check for empty condition.Expression to match DeleteItemBuilder's guard and prevent invalid ConditionExpressions from being sent.
🤖 Prompt for AI Agents
In `@src/Clients/Goa.Clients.Dynamo/Operations/UpdateItem/UpdateItemBuilder.cs`
around lines 803 - 820, UpdateItemBuilder.WithCondition currently overwrites
prior conditions and doesn't guard against empty condition.Expression; change it
to match DeleteItemBuilder's multi-condition behavior: if condition.Expression
is null or empty, return without modifying; if _request.ConditionExpression is
null or empty set it to condition.Expression, otherwise set it to "(" +
_request.ConditionExpression + ") AND (" + condition.Expression + ")"; keep the
existing ExpressionAttributeNames/ExpressionAttributeValues merge logic (using
_request.ExpressionAttributeNames ??= ... and Merge) so attribute maps still
combine across calls. Also apply the same empty-expression guard pattern to
PutItemBuilder.WithCondition to ensure consistent behavior across builders.
| /// <summary> | ||
| /// Specifies how to return attribute values when a conditional check fails. | ||
| /// </summary> | ||
| /// <param name="returnValuesOnConditionCheckFailure">The return values on condition check failure setting.</param> | ||
| /// <returns>The UpdateItemBuilder instance for method chaining.</returns> | ||
| public UpdateItemBuilder WithReturnValuesOnConditionCheckFailure(ReturnValuesOnConditionCheckFailure returnValuesOnConditionCheckFailure) | ||
| { | ||
| _request.ReturnValuesOnConditionCheckFailure = returnValuesOnConditionCheckFailure; | ||
| return this; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check for existing coverage / add similar coverage if missing.
rg -n --type=cs -C2 'WithReturnValuesOnConditionCheckFailure|ReturnValuesOnConditionCheckFailure' tests src/Clients/Goa.Clients.Dynamo.Tests || trueRepository: Im5tu/goa
Length of output: 129
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find test directories
find . -type d -name "*Tests" -o -type d -name "*test*" | head -20
# Also list the structure around the source file
ls -la src/Clients/Goa.Clients.Dynamo/
</script>Repository: Im5tu/goa
Length of output: 2180
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for UpdateItemBuilder tests
rg -n --type=cs 'WithReturnValuesOnConditionCheckFailure|ReturnValuesOnConditionCheckFailure' tests/Clients/Goa.Clients.Dynamo.Tests || true
echo "---"
# Also search for UpdateItemBuilder test files
fd -t f UpdateItemBuilder tests/Clients/Goa.Clients.Dynamo.Tests/ || trueRepository: Im5tu/goa
Length of output: 56
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for UpdateItem test files
find tests/Clients/Goa.Clients.Dynamo.Tests -type f -name "*UpdateItem*" || true
echo "---"
# Search for any UpdateItem references in tests
rg -l 'UpdateItem' tests/Clients/Goa.Clients.Dynamo.Tests --type=cs || true
echo "---"
# List test directory structure
find tests/Clients/Goa.Clients.Dynamo.Tests/Operations -type f 2>/dev/null | head -20 || trueRepository: Im5tu/goa
Length of output: 204
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for ReturnValuesOnConditionCheckFailure in test files
rg -n 'ReturnValuesOnConditionCheckFailure' tests/Clients/Goa.Clients.Dynamo.Tests/ --type=cs -C3 || true
echo "---"
# Check the content of DynamoClientIntegrationTests.cs to see what UpdateItem tests exist
grep -n 'UpdateItem' tests/Clients/Goa.Clients.Dynamo.Tests/DynamoClientIntegrationTests.cs | head -20 || trueRepository: Im5tu/goa
Length of output: 323
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if UpdateItemBuilder is used in tests
rg -n 'UpdateItemBuilder' tests/Clients/Goa.Clients.Dynamo.Tests/ --type=cs || true
echo "---"
# Look at the actual test that uses UpdateItem
rg -n -A 30 'UpdateItemAsync_ShouldModifyItem_WhenItemExists' tests/Clients/Goa.Clients.Dynamo.Tests/DynamoClientIntegrationTests.cs || trueRepository: Im5tu/goa
Length of output: 2365
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look at the UpdateItemBuilder tests section
rg -n -A 100 '#region UpdateItemBuilder Tests' tests/Clients/Goa.Clients.Dynamo.Tests/ConditionExpressionValuesTests.cs | head -150 || true
echo "---"
# Check for any use of ReturnValuesOnConditionCheckFailure in the entire test file
rg -n 'ReturnValuesOnConditionCheckFailure' tests/Clients/Goa.Clients.Dynamo.Tests/ConditionExpressionValuesTests.cs || echo "Not found in ConditionExpressionValuesTests.cs"Repository: Im5tu/goa
Length of output: 4443
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for other WithReturnValues methods in UpdateItemBuilder
rg -n 'WithReturnValues' src/Clients/Goa.Clients.Dynamo/Operations/UpdateItem/UpdateItemBuilder.cs --type=cs || true
echo "---"
# Check if any ReturnValues methods are tested
rg -n 'WithReturnValues|ReturnValues' tests/Clients/Goa.Clients.Dynamo.Tests/ --type=cs || trueRepository: Im5tu/goa
Length of output: 525
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if WithReturnValues (the non-condition version) is tested
rg -n 'WithReturnValues' tests/Clients/Goa.Clients.Dynamo.Tests/ --type=cs -B2 -A5 || true
echo "---"
# Look at how ReturnValues.ALL_NEW is used in the test
rg -n -B10 -A10 'ReturnValues = ReturnValues.ALL_NEW' tests/Clients/Goa.Clients.Dynamo.Tests/DynamoClientIntegrationTests.cs || trueRepository: Im5tu/goa
Length of output: 919
Add test coverage for the WithReturnValuesOnConditionCheckFailure method.
The builder method is straightforward and follows the fluent API pattern correctly, but it lacks test coverage. Following the pattern established by other builder tests (e.g., UpdateItemBuilder_WithCondition_* tests in ConditionExpressionValuesTests.cs), add a test that validates the method properly flows the value into the built request. Also note that the related WithReturnValues method (line 838) similarly lacks test coverage.
🤖 Prompt for AI Agents
In `@src/Clients/Goa.Clients.Dynamo/Operations/UpdateItem/UpdateItemBuilder.cs`
around lines 844 - 853, Add unit tests mirroring the existing
UpdateItemBuilder_WithCondition_* patterns (see ConditionExpressionValuesTests)
that verify UpdateItemBuilder.WithReturnValuesOnConditionCheckFailure(...) and
UpdateItemBuilder.WithReturnValues(...) correctly propagate values into the
built request; instantiate an UpdateItemBuilder, call the method under test with
each relevant enum value, call Build() (or however the request is produced in
tests), and assert the resulting request's ReturnValuesOnConditionCheckFailure
and ReturnValues properties equal the values passed.
1 participant
Summary by CodeRabbit
New Features
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.