Cross Cutting Performance Improvements#73
Merged
Merged
Conversation
Adds the sealed keyword to ~89 classes across Bedrock, Dynamo, and Core libraries that are not abstract, not inherited, and not partial. This enables the JIT to devirtualize and inline method calls on these types.
Add LogScope2 struct for the fixed StatusCode/ReasonPhrase fields, replacing the Dictionary allocation on every AWS call. Variable x-amz headers use a lazy Dictionary with capacity hint of 4.
Add capacity arguments to Dictionary/List constructors where the size is known or predictable: SNS query params, SQS batch chunking, DynamoDB conversation store items, and content block serialization.
Move the identical ProcessAwsErrorHeaders method from JsonAwsServiceClient, XmlAwsServiceClient, and SnsServiceClient into the shared AwsServiceClient base class to eliminate duplication.
Move ReadResponseBytesAsync and PooledBuffer from JsonAwsServiceClient to the base AwsServiceClient class. Update XmlAwsServiceClient and SnsServiceClient to use pooled byte buffers instead of ReadAsStringAsync, avoiding intermediate StreamReader allocations.
Add ReadOnlySpan<byte> overload to ApiErrorReader and use it directly from ProcessJsonResponseAsync to skip the string->bytes round-trip. The string overload now uses stackalloc for payloads <= 1KB and ArrayPool for larger ones.
Remove redundant [MethodImpl(NoInlining)] — the JIT already avoids inlining methods annotated with [DoesNotReturn].
Eliminates 4 heap allocations + 2 AsyncLocal writes per request when logging is disabled by wrapping both BeginScope calls with Logger.IsEnabled(Configuration.LogLevel).
…eClient Follows the existing pattern in JsonAwsServiceClient to avoid allocating a new MediaTypeHeaderValue instance per request.
Avoids int.ToString() allocation per response for the most common HTTP status codes by returning cached string literals from a switch expression.
Replaces magic string literals with named constants for log scope keys, improving deduplication and readability.
Replaces inline header name strings with named constants for deduplication and readability.
Deduplicates header name strings between HeaderRef.Create calls and IsAwsManagedHeader by introducing named constants.
Contributor
📝 WalkthroughWalkthroughSealed many public types; replaced LINQ allocations with explicit loops and capacity-aware collections; added a typed Throw helper; refactored error/response reading toward byte/span buffering; introduced allocation-aware header and logging helpers; centralised Dynamo response readers. Public APIs largely unchanged. Changes
✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
|
![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: 1
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/Clients/Goa.Clients.Core/JsonAwsServiceClient.cs (1)
97-110:⚠️ Potential issue | 🟠 MajorKeep the raw error body when structured parsing fails.
If
ReadApiErrorreturnsnull, this branch throws awayerrorPayloadand returns only a generic message. Plain-text, XML, or malformed upstream errors then become opaque even though the body was already buffered.🛠️ Suggested fix
else { - error = new ApiError("Failed to send request and unable to deserialize payload") { StatusCode = response.StatusCode }; + error = new ApiError("Failed to send request and unable to deserialize payload") + { + Payload = errorPayload, + StatusCode = response.StatusCode + }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Clients/Goa.Clients.Core/JsonAwsServiceClient.cs` around lines 97 - 110, When ApiErrorReader.ReadApiError returns null the code currently drops the buffered body and returns a generic ApiError; preserve the raw body by setting the Payload on the new ApiError. In the error-null branch update the creation to include Payload = errorPayload and StatusCode = response.StatusCode (using the existing errorBuffer/errorPayload variables and the ApiError type), so both parsed and unparsed responses keep the raw body for diagnostics.src/Clients/Goa.Clients.Dynamo/Operations/GetItem/GetItemBuilder.cs (1)
65-74:⚠️ Potential issue | 🟠 MajorFix the
IEnumerable<string>overload to work with non-collection sequences.The current guard
if (attributes is ICollection<string> { Count: > 0 })rejects LINQ results, iterators, and otherIEnumerable<string>implementations that don't implementICollection<string>. These fall through to_request.ProjectionExpression = null, causing DynamoDB to fetch full items instead of applying the requested projection. The method signature accepts the broadIEnumerable<string>interface, so this is a silent behavioral regression.♻️ Proposed fix
public GetItemBuilder WithProjection(IEnumerable<string> attributes) { - if (attributes is ICollection<string> { Count: > 0 }) - { - _request.ProjectionExpression = string.Join(", ", attributes); - } - else + if (attributes is null) { _request.ProjectionExpression = null; + return this; } + var projection = string.Join(", ", attributes); + _request.ProjectionExpression = projection.Length > 0 ? projection : null; + return this; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Clients/Goa.Clients.Dynamo/Operations/GetItem/GetItemBuilder.cs` around lines 65 - 74, GetItemBuilder.WithProjection currently only accepts ICollection<string> and therefore ignores non-collection IEnumerables (LINQ, iterators). Fix by accounting for any non-null, non-empty sequence: materialize attributes into a list (e.g., call ToList()) and then check list.Count > 0; if so set _request.ProjectionExpression = string.Join(", ", list) otherwise set it to null. Ensure you handle a null attributes input and include the ToList/Any usage so LINQ/iterators are respected in WithProjection.
🟠 Major comments (23)
src/Clients/Goa.Clients.Dynamo/Operations/Transactions/TransactUpdateItem.cs-10-10 (1)
10-10:⚠️ Potential issue | 🟠 Major
sealedon a public model is a breaking API changeLine 10 changes
TransactUpdateItemto non-inheritable. Any consumer subclassing this public type will now fail to compile. If API stability is a goal, keep this type unsealed (or explicitly treat this PR/release as breaking and document it clearly).Suggested compatibility-preserving change
-public sealed class TransactUpdateItem +public class TransactUpdateItem🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Clients/Goa.Clients.Dynamo/Operations/Transactions/TransactUpdateItem.cs` at line 10, The change made the public model class TransactUpdateItem sealed, which is a breaking API change for consumers who may subclass it; revert this by removing the sealed modifier from the TransactUpdateItem class declaration so it remains inheritable (or if the intent is to make a breaking change, ensure the change is documented in the release notes and the package version is bumped appropriately).src/Clients/Goa.Clients.Dynamo/Operations/Transactions/TransactGetRequest.cs-9-9 (1)
9-9:⚠️ Potential issue | 🟠 MajorSealing this public type introduces a breaking API change.
At Line 9, changing
TransactGetRequesttosealedbreaks consumers that inherit from this class, which conflicts with the PR’s API-stability goal unless this is an intentional versioned break.Suggested compatibility-preserving change
-public sealed class TransactGetRequest +public class TransactGetRequestIf sealing is intentional, please ship it with explicit breaking-change notes and a major-version release boundary.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Clients/Goa.Clients.Dynamo/Operations/Transactions/TransactGetRequest.cs` at line 9, The class declaration for TransactGetRequest was changed to "sealed", which is a breaking change for consumers that may inherit from TransactGetRequest; revert the sealing by removing the sealed modifier from the TransactGetRequest declaration (restore it to a non-sealed public class) or, if sealing is intentional, add explicit documentation and release notes indicating a breaking change and bump the major version—update the TransactGetRequest declaration accordingly to match the chosen approach.src/Functions/Goa.Functions.Core/Logging/JsonLogger.cs-127-127 (1)
127-127:⚠️ Potential issue | 🟠 MajorScope payload can overwrite the log category field.
At Line 127,
FieldStatecurrently resolves to"category", so non-keyvalue scope data can replace_categoryNamein the output. Use a dedicated key (e.g.,"state"or"scope") for scope objects.Suggested fix
- private static readonly string FieldState = "category"; + private static readonly string FieldState = "state";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Functions/Goa.Functions.Core/Logging/JsonLogger.cs` at line 127, The current assignment state[FieldState] = scope in JsonLogger.cs can overwrite the log category because FieldState resolves to "category"; change the key used for scope payloads to a dedicated name (e.g., "state" or "scope") so scope objects cannot replace _categoryName. Concretely, add/introduce a distinct constant (for example ScopeField = "state") in the JsonLogger class and replace the usage state[FieldState] = scope with state[ScopeField] = scope (or directly state["state"] = scope) so only the new key is written and category remains untouched.src/Clients/Goa.Clients.Dynamo/DynamoServiceClient.cs-24-24 (1)
24-24:⚠️ Potential issue | 🟠 MajorSealing
DynamoServiceClientintroduces a public API break.At Line 24, changing a previously inheritable public class to
sealedcan break downstream consumers that subclassed this client. If inheritance was part of expected extensibility, keep it unsealed; otherwise, treat this as a documented breaking change (release notes/versioning).Compatibility-preserving diff
-public sealed class DynamoServiceClient : JsonAwsServiceClient<DynamoServiceClientConfiguration>, IDynamoClient +public class DynamoServiceClient : JsonAwsServiceClient<DynamoServiceClientConfiguration>, IDynamoClient🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Clients/Goa.Clients.Dynamo/DynamoServiceClient.cs` at line 24, The change made DynamoServiceClient sealed, which breaks consumers that subclass it; revert the sealing by removing the sealed modifier from the public class declaration (i.e., restore "public class DynamoServiceClient : JsonAwsServiceClient<DynamoServiceClientConfiguration>, IDynamoClient") so subclasses can continue to inherit, or if sealing was intentional, document the breaking change in release notes and bump the major version; locate the class declaration for DynamoServiceClient to apply this change.src/Clients/Goa.Clients.Dynamo/Operations/Transactions/TransactGetBuilder.cs-9-9 (1)
9-9:⚠️ Potential issue | 🟠 MajorSealing this public type introduces a breaking API change.
Line 9 changes
TransactGetBuilderfrom extensible to non-extensible. Any consumers inheriting from this type will break at compile/runtime upgrade boundaries. If this is intentional, treat it as a breaking-change release (major version + release note); otherwise keep it unsealed or introduce a supported abstraction for extension/testing.Possible non-breaking adjustment
-public sealed class TransactGetBuilder +public class TransactGetBuilder🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Clients/Goa.Clients.Dynamo/Operations/Transactions/TransactGetBuilder.cs` at line 9, The change made TransactGetBuilder into a sealed public class which is a breaking API (prevents consumers from inheriting); revert the sealing by removing the sealed modifier on TransactGetBuilder or, if sealing is intentional, provide a non-breaking alternative such as introducing a public abstract base class or interface (e.g., ITransactGetBuilder or an abstract TransactGetBuilderBase) and migrate extensible members there so existing inheritors can target the new abstraction; update constructors/members on TransactGetBuilder (or the new base) to support extension as needed.src/Clients/Goa.Clients.Dynamo/Extensions/DynamoRecordExtensions.cs-544-549 (1)
544-549:⚠️ Potential issue | 🟠 MajorFail fast on invalid set entries instead of returning partial success.
At Line 546, Line 576, Line 606, Line 638, and Line 1061, failed parses are silently skipped, but each method still returns
true. That can hide malformed data and return truncated sets as “valid.” These methods should returnfalsewhen any element in the set is invalid.💡 Proposed fix (strict parse semantics)
- var result = new List<int>(); - foreach (var x in attributeValue.NS) - if (int.TryParse(x, out var val)) - result.Add(val); - value = result; - return true; + var result = new List<int>(attributeValue.NS.Count); + foreach (var x in attributeValue.NS) + { + if (!int.TryParse(x, out var val)) + return false; + result.Add(val); + } + value = result; + return true;Apply the same fail-fast pattern to:
TryGetLongSet(Line 574-579)TryGetDoubleSet(Line 604-609)TryGetEnumSet<T>(Line 635-642)TryGetDateTimeSet(Line 1059-1064)Also applies to: 574-579, 604-609, 635-642, 1059-1064
🤖 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 544 - 549, The set-parsing methods (TryGetIntSet, TryGetLongSet, TryGetDoubleSet, TryGetEnumSet<T>, TryGetDateTimeSet) currently skip invalid elements and still return true; change them to fail-fast: when iterating attributeValue.*S, if any element fails to parse/convert (e.g., int.TryParse, long.TryParse, double.TryParse, Enum.TryParse<T>, DateTime.TryParse), immediately set value to null (or leave it unset) and return false instead of continuing and returning true; otherwise populate the result list and return true. Ensure you reference and update the implementations named TryGetIntSet, TryGetLongSet, TryGetDoubleSet, TryGetEnumSet<T>, and TryGetDateTimeSet so partial/truncated sets are not treated as successful.src/Clients/Goa.Clients.Dynamo/SerializedNameAttribute.cs-8-8 (1)
8-8:⚠️ Potential issue | 🟠 MajorSealing this public attribute is a breaking API change.
Line 8 changes a previously extensible public type into a closed one. Any downstream package deriving from
SerializedNameAttributewill stop compiling, and already-built consumers can fail to load against the new assembly. Unless this release is intentionally taking a major-version break, I’d keep this attribute unsealed.Suggested fix
-public sealed class SerializedNameAttribute : Attribute +public class SerializedNameAttribute : Attribute🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Clients/Goa.Clients.Dynamo/SerializedNameAttribute.cs` at line 8, The change made SerializedNameAttribute a sealed public class which is a breaking API for consumers that inherit from it; revert this by removing the sealed modifier so SerializedNameAttribute remains extensible (i.e., change the declaration of SerializedNameAttribute to be a non-sealed public class) to preserve backward compatibility for downstream derived types.src/Clients/Goa.Clients.Bedrock/Operations/Converse/ConverseBuilder.cs-11-11 (1)
11-11:⚠️ Potential issue | 🟠 MajorSealing
ConverseBuilderis a public breaking change.Line 11 changes a public type from inheritable to sealed. Any consumer subclassing
ConverseBuilderwill fail to compile after upgrade. If this release is not intentionally breaking, keep this type unsealed (or ship with explicit major-version/migration guidance).Proposed compatibility fix
-public sealed class ConverseBuilder(string modelId) +public class ConverseBuilder(string modelId)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Clients/Goa.Clients.Bedrock/Operations/Converse/ConverseBuilder.cs` at line 11, The change made ConverseBuilder into a sealed type which is a public breaking change; revert this by removing the sealed modifier from the ConverseBuilder declaration (restore it to public class ConverseBuilder(string modelId)) so consumers can inherit, then rebuild and run tests to ensure no other API changes were introduced; if sealing was intentional, instead add clear breaking-change documentation and bump major version.src/Goa.Core/LoggerExtensions.cs-72-76 (1)
72-76:⚠️ Potential issue | 🟠 MajorException should be passed to LoggerMessage methods for proper structured logging.
The new partial methods only accept
exceptionMessagestring, but the standard pattern (seePublishFailed,InvokeSynchronousFailed, etc. in other files) passes theExceptionobject directly. This ensures:
- Full exception details (including inner exceptions) are captured by the logging infrastructure
- Logging sinks that have special exception handling can process it correctly
- Exception appears in the standard
Exceptionproperty expected by log aggregators🔧 Proposed fix
- [LoggerMessage(EventId = 0, Level = LogLevel.Critical, Message = "{ExceptionMessage}")] - private static partial void LogExceptionCritical(this ILogger logger, string? exceptionMessage); + [LoggerMessage(EventId = 0, Level = LogLevel.Critical, Message = "{ExceptionMessage}")] + private static partial void LogExceptionCritical(this ILogger logger, Exception exception, string? exceptionMessage); - [LoggerMessage(EventId = 0, Level = LogLevel.Error, Message = "{ExceptionMessage}")] - private static partial void LogExceptionError(this ILogger logger, string? exceptionMessage); + [LoggerMessage(EventId = 0, Level = LogLevel.Error, Message = "{ExceptionMessage}")] + private static partial void LogExceptionError(this ILogger logger, Exception exception, string? exceptionMessage);Then update the call sites:
if (critical) - logger.LogExceptionCritical(exception.Message); + logger.LogExceptionCritical(exception, exception.Message); else - logger.LogExceptionError(exception.Message); + logger.LogExceptionError(exception, exception.Message);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Goa.Core/LoggerExtensions.cs` around lines 72 - 76, The LoggerMessage partials LogExceptionCritical and LogExceptionError currently take only a string exceptionMessage; change their signatures to accept an Exception (e.g., Exception exception) as the exception parameter (keeping the message template or adding an exception placeholder if needed) so the Logging infrastructure receives the Exception object; then update all call sites that invoked LogExceptionCritical/LogExceptionError to pass the original Exception instance instead of just exceptionMessage (mirror the pattern used by PublishFailed/InvokeSynchronousFailed elsewhere) to ensure full exception details and proper structured logging.src/Clients/Goa.Clients.Bedrock.Conversation/Entities/ConversationMetadata.cs-6-6 (1)
6-6:⚠️ Potential issue | 🟠 MajorSealing this public model introduces a breaking API change.
Line 6 changes
ConversationMetadatafrom an inheritable class to a sealed one. While sealing a DTO is a best practice for DTOs to prevent subclassing anti-patterns, this is a breaking change for any downstream consumer that subclasses it. Confirm this change is intentional and either:
- Document it as a major-version breaking change with migration guidance, or
- Revert if unintended.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Clients/Goa.Clients.Bedrock.Conversation/Entities/ConversationMetadata.cs` at line 6, The change seals the public model ConversationMetadata which introduces a breaking API change for consumers that subclass it; either revert the sealing (remove the sealed modifier from ConversationMetadata) to restore binary compatibility, or if sealing is intentional, document the breaking change in the release notes/changelog and bump the package major version and add migration guidance explaining callers must stop subclassing ConversationMetadata and instead use composition or copy the properties into a new type.src/Clients/Goa.Clients.Dynamo/Operations/Transactions/TransactGetResult.cs-9-9 (1)
9-9:⚠️ Potential issue | 🟠 MajorSource-breaking API change: sealing prevents inheritance.
At Line 9, changing
TransactGetResulttosealedbreaks code that inherits from this type. While no inheritance exists in the codebase today, this is a documented source-breaking change. If intentional (the entire Operations module uses sealed classes for performance), it should be accompanied by a major version bump and release notes; otherwise keep it unsealed or document the compatibility guarantee.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Clients/Goa.Clients.Dynamo/Operations/Transactions/TransactGetResult.cs` at line 9, You changed the class declaration for TransactGetResult to sealed which introduces a source-breaking API change by preventing consumer inheritance; revert the change by removing the sealed modifier from the TransactGetResult declaration (restore it to a public class) unless this is an intentional, public breaking change — in which case update release notes and bump the major version and document that the Operations module uses sealed classes for performance.src/Clients/Goa.Clients.Dynamo/Operations/Condition.cs-9-9 (1)
9-9:⚠️ Potential issue | 🟠 MajorSealing
Conditionis a breaking public API change.Any downstream code that subclasses
Conditionor relies on proxying/mocking it will now fail to compile or load against this version. If this perf trade-off is intentional, it should ship as a major-version change; otherwise keep the type unsealed.🛠️ Minimal compatibility-preserving change
-public sealed class Condition +public class Condition🤖 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` at line 9, The class Condition was made sealed which is a breaking public API change; revert this by removing the sealed modifier from the Condition class declaration so it remains inheritable (or if sealing is intentional, document as a major-version change). Locate the public sealed class Condition declaration and change it back to public class Condition to preserve downstream subclassing, mocking and proxying behavior.src/Clients/Goa.Clients.Bedrock.Conversation/Chat/ChatSession.cs-321-331 (1)
321-331:⚠️ Potential issue | 🟠 MajorAvoid leaking mutable internal message content via history snapshots.
At Line 327,
Content = m.Contentexposes the session’s internal list reference. Consumers can mutate chat history indirectly by castingIReadOnlyList<ContentBlock>back toList<ContentBlock>.Proposed fix
- foreach (var m in _messages) + foreach (var m in _messages) { history.Add(new ChatMessage { Role = m.Role, - Content = m.Content, + Content = new List<ContentBlock>(m.Content), Timestamp = DateTimeOffset.UtcNow, // In-memory messages don't have timestamps TokenUsage = null }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Clients/Goa.Clients.Bedrock.Conversation/Chat/ChatSession.cs` around lines 321 - 331, The history snapshot is exposing internal mutable message content by assigning Content = m.Content; instead create a deep copy of each message's Content when building history so callers can't mutate the session's internal objects. In the ChatSession snapshot loop that constructs new ChatMessage instances (iterating _messages), replace the direct reference assignment with a clone/copy of the content (e.g. use ContentBlock.Clone() or a ContentBlock copy constructor or construct a new ContentBlock populating all fields from m.Content) so the new ChatMessage has its own independent Content instance; keep Timestamp = DateTimeOffset.UtcNow and TokenUsage = null as before.src/Clients/Goa.Clients.Bedrock/Models/GuardrailConfiguration.cs-6-6 (1)
6-6:⚠️ Potential issue | 🟠 MajorSealing this public model introduces a breaking API change.
Line 6 prevents downstream inheritance, which is a compile-time break for consumers extending this model. If intentional, this should be treated/versioned as a breaking change and called out in release notes.
#!/bin/bash set -euo pipefail # Verify whether internal code derives from newly sealed public models. rg -nP --type=cs \ 'class\s+\w+\s*:\s*(GuardrailConfiguration|ContentBlock|ImageBlock|ImageSource|S3Location|ToolConfiguration|ToolChoice|AutoToolChoice|AnyToolChoice|SpecificToolChoice|InferenceConfiguration|Message|CachePoint)\b'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Clients/Goa.Clients.Bedrock/Models/GuardrailConfiguration.cs` at line 6, The public model GuardrailConfiguration was marked "sealed", causing a breaking change for consumers that inherit from it; to fix, remove the sealed modifier from the class declaration (restore "public class GuardrailConfiguration") so downstream inheritance works, run the repository-wide search for the related public models (ContentBlock, ImageBlock, ImageSource, S3Location, ToolConfiguration, ToolChoice, AutoToolChoice, AnyToolChoice, SpecificToolChoice, InferenceConfiguration, Message, CachePoint) to ensure none were accidentally sealed, update any unit/integration tests that expect sealing, and if sealing was intentional instead increment the library major version and add an explicit breaking-change note to the release notes documenting the change.src/Clients/Goa.Clients.Dynamo/Operations/Scan/ScanRequest.cs-10-10 (1)
10-10:⚠️ Potential issue | 🟠 MajorSealing a public type returned from a public builder API is a source-breaking change.
ScanBuilder.Build()returnsScanRequestdirectly (Lines 168-171), so making it sealed at Line 10 prevents external code from extending it. If any downstream code was subclassingScanRequest, this breaks their builds. Either revert thesealedmodifier or address this in a major version release with migration guidance.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Clients/Goa.Clients.Dynamo/Operations/Scan/ScanRequest.cs` at line 10, The ScanRequest type was made sealed which is a source-breaking change because ScanBuilder.Build() returns ScanRequest and callers may subclass it; revert the sealed modifier on the ScanRequest class so it remains inheritable, or if you intended to prevent subclassing, change the API contract (e.g., return an interface or document as a major-version breaking change) and update ScanBuilder.Build() accordingly; locate the ScanRequest declaration (currently "public sealed class ScanRequest") and remove the sealed keyword so it reads "public class ScanRequest", ensuring compatibility with existing subclasses created by callers of ScanBuilder.Build().src/Clients/Goa.Clients.Dynamo/Operations/Transactions/TransactWriteOperation.cs-9-9 (1)
9-9:⚠️ Potential issue | 🟠 MajorSealing this public operation contract is a breaking change.
The class was previously unsealed (see commit f7f3b81 initial version) and is now sealed. This prevents external consumers from deriving from
TransactWriteOperation, breaking any downstream code that extends this type. Unless the next package release is explicitly major, this should remain unsealed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Clients/Goa.Clients.Dynamo/Operations/Transactions/TransactWriteOperation.cs` at line 9, The class TransactWriteOperation was made sealed which is a breaking change for external consumers; revert this by removing the sealed modifier so TransactWriteOperation remains an inheritable public class (restore its original declaration), ensuring any unit tests or constructors referencing TransactWriteOperation still compile and the public contract is unchanged.src/Clients/Goa.Clients.Dynamo/Models/ItemCollectionMetrics.cs-8-8 (1)
8-8:⚠️ Potential issue | 🟠 MajorSealing this public model type is a breaking compatibility change.
Line 8 adds the
sealedkeyword toItemCollectionMetrics, which was previously an unsealed public class. This prevents downstream consumers from deriving from it, breaking any code that extends this response model. For a client library, this requires an explicit major version bump and migration guidance.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Clients/Goa.Clients.Dynamo/Models/ItemCollectionMetrics.cs` at line 8, The change adds the sealed modifier to the public model class ItemCollectionMetrics which breaks downstream inheritance; revert this by removing the sealed keyword from the ItemCollectionMetrics declaration so the class remains inheritable, or if sealing was intentional, instead keep it non-sealed in code and document the breaking change plus perform a major version bump and migration notes referencing ItemCollectionMetrics.src/Clients/Goa.Clients.Dynamo/Operations/Query/QueryBuilder.cs-152-152 (1)
152-152:⚠️ Potential issue | 🟠 Major
WithProjection(IEnumerable<string>)behavior regressed for iterator inputs.Line 152 now requires
ICollection<string>, so non-collection enumerables with values are treated as empty and projection is not applied.💡 Proposed fix
public QueryBuilder WithProjection(IEnumerable<string> attributes) { - if (attributes is ICollection<string> { Count: > 0 }) + if (attributes is null) + { + _request.ProjectionExpression = null; + return this; + } + + var materialized = attributes as ICollection<string> ?? new List<string>(attributes); + if (materialized.Count > 0) { - _request.ProjectionExpression = string.Join(", ", attributes); + _request.ProjectionExpression = string.Join(", ", materialized); return WithSelectionMode(Select.SPECIFIC_ATTRIBUTES); } _request.ProjectionExpression = null; return this; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Clients/Goa.Clients.Dynamo/Operations/Query/QueryBuilder.cs` at line 152, The check in QueryBuilder's WithProjection(IEnumerable<string>) now only treats inputs that implement ICollection<string> as non-empty, regressing support for iterator-based enumerables; update the conditional that inspects the attributes variable (used in WithProjection) to detect non-empty sequences for any IEnumerable<string> (e.g., use attributes != null && attributes.Any() or check enumerator.MoveNext() / materialize to a list) so iterator inputs with values correctly apply the projection.src/Clients/Goa.Clients.Dynamo/Operations/Scan/ScanBuilder.cs-106-106 (1)
106-106:⚠️ Potential issue | 🟠 Major
WithProjection(IEnumerable<string>)now drops valid non-collection enumerables.Line 106 only accepts
ICollection<string>, so iterators/generators with values now fall through and clear projection instead of applying it.💡 Proposed fix
public ScanBuilder WithProjection(IEnumerable<string> attributes) { - if (attributes is ICollection<string> { Count: > 0 }) + if (attributes is null) + { + _request.ProjectionExpression = null; + return this; + } + + var materialized = attributes as ICollection<string> ?? new List<string>(attributes); + if (materialized.Count > 0) { - _request.ProjectionExpression = string.Join(", ", attributes); + _request.ProjectionExpression = string.Join(", ", materialized); return WithSelectionMode(Select.SPECIFIC_ATTRIBUTES); } _request.ProjectionExpression = null; return this; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Clients/Goa.Clients.Dynamo/Operations/Scan/ScanBuilder.cs` at line 106, The WithProjection(IEnumerable<string>) implementation in ScanBuilder.cs currently only recognizes inputs that implement ICollection<string> and thus ignores valid non-collection iterables (e.g., generators), clearing the projection unintentionally; change the guard to detect any non-empty IEnumerable<string> (for example by checking attributes != null && attributes.Any() or by casting to IEnumerable<string> and using Any()) and then apply the projection using that sequence so generators/iterators are handled correctly by WithProjection.src/Clients/Goa.Clients.Lambda/LambdaServiceClient.cs-83-86 (1)
83-86:⚠️ Potential issue | 🟠 MajorRe-throw caller cancellations instead of logging them as Lambda failures.
Each broad
catch (Exception)also catchesOperationCanceledExceptionfromSendAsync, so a caller-requested cancellation is turned into a generic failure result. That breaks cooperative cancellation and can confuse retry/timeout handling.🛠️ Suggested fix
- catch (Exception ex) + catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested) + { + throw; + } + catch (Exception ex) { Logger.InvokeSynchronousFailed(ex, request.FunctionName); return Error.Failure("Lambda.InvokeSynchronous.Failed", $"Failed to invoke Lambda function synchronously {request.FunctionName}"); } @@ - catch (Exception ex) + catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested) + { + throw; + } + catch (Exception ex) { Logger.InvokeAsynchronousFailed(ex, request.FunctionName); return Error.Failure("Lambda.InvokeAsynchronous.Failed", $"Failed to invoke Lambda function asynchronously {request.FunctionName}"); } @@ - catch (Exception ex) + catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested) + { + throw; + } + catch (Exception ex) { Logger.InvokeDryRunFailed(ex, request.FunctionName); return Error.Failure("Lambda.InvokeDryRun.Failed", $"Failed to dry run Lambda function {request.FunctionName}"); }Also applies to: 131-134, 179-182
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Clients/Goa.Clients.Lambda/LambdaServiceClient.cs` around lines 83 - 86, The catch blocks in LambdaServiceClient (the ones that call Logger.InvokeSynchronousFailed and return Error.Failure) are catching OperationCanceledException and turning caller cancellations into generic failures; update each such catch to re-throw if the exception is an OperationCanceledException or TaskCanceledException (e.g., if (ex is OperationCanceledException || ex is TaskCanceledException) throw;) and only log/return Error.Failure for other exceptions; make this change in the catch blocks that call Logger.InvokeSynchronousFailed (and the similar Logger/return patterns at the other locations mentioned).src/Clients/Goa.Clients.Core/Http/ApiResponse.cs-20-23 (1)
20-23:⚠️ Potential issue | 🟠 MajorThis breaks the public
ApiResponse<T>contract.
ApiResponse<T>is public, and switching it from header metadata to aContentTypestring changes both the shape and semantics of the API. Existing consumers reading the old response metadata or calling the previous two-argument constructor will hit source/binary breaks, which conflicts with the PR’s API-stability goal.Also applies to: 39-47
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Clients/Goa.Clients.Core/Http/ApiResponse.cs` around lines 20 - 23, The change replaced the previous response metadata shape with a string ContentType, breaking the public ApiResponse<T> contract; restore the original public API by reintroducing the original headers/metadata property and the two-argument constructor overload used by consumers while optionally keeping ContentType as a convenience (derive it from the headers). Specifically, revert or add back the original headers collection/property (the one used before the change) and the constructor signature on ApiResponse<T>, and implement ContentType as a nullable computed property that reads the content-type header from that headers collection so existing callers and binary consumers continue to work.src/Clients/Goa.Clients.Core/XmlAwsServiceClient.cs-153-155 (1)
153-155:⚠️ Potential issue | 🟠 MajorDon't log the full XML error payload here.
This warning path emits the entire response body whenever XML error parsing fails. That can leak customer data and create very large warning entries on upstream HTML/XML error pages.
Safer logging option
- Logger.LogWarning(ex, "Failed to deserialize XML error response: {Content}", content); + Logger.LogWarning( + ex, + "Failed to deserialize XML error response. PayloadLength={PayloadLength}", + content.Length);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Clients/Goa.Clients.Core/XmlAwsServiceClient.cs` around lines 153 - 155, The current catch block in XmlAwsServiceClient that calls Logger.LogWarning(ex, "Failed to deserialize XML error response: {Content}", content) logs the entire XML payload which can leak sensitive data and produce huge logs; change this to avoid emitting the full payload by replacing the logged {Content} with a safe summary such as a length/size and a short truncated snippet or a deterministic hash of content (e.g., contentLength and contentHash or first N chars only), and update the LogWarning invocation to include those safe fields instead of the raw content (reference the Logger.LogWarning call, the ex exception variable, and the content variable in the deserialization error handling code).src/Clients/Goa.Clients.Core/AwsServiceClient.cs-309-317 (1)
309-317:⚠️ Potential issue | 🟠 MajorReject truncated bodies when
Content-Lengthis known.Breaking on
read == 0and returningtotalReadtreats an early EOF as success. That turns a transport truncation into a partial payload for downstream parsers.Possible fix
while (totalRead < contentLength) { var read = await stream.ReadAsync(knownBuffer.AsMemory(totalRead, contentLength - totalRead), cancellationToken); - if (read == 0) break; + if (read == 0) + Throw.InvalidOperation($"Response ended after {totalRead} of {contentLength} bytes."); totalRead += read; } return new PooledBuffer(knownBuffer, totalRead);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Clients/Goa.Clients.Core/AwsServiceClient.cs` around lines 309 - 317, The current read loop in AwsServiceClient (inside the method that reads response.Content.ReadAsStreamAsync and returns new PooledBuffer(knownBuffer, totalRead)) treats an early EOF (read == 0) as success and returns a truncated PooledBuffer; change the logic to detect incomplete reads when contentLength is known and throw an exception (e.g., IOException or HttpRequestException) instead of returning a partial buffer. Specifically, in the loop that uses knownBuffer, totalRead and contentLength, if read == 0 before totalRead == contentLength or after the loop if totalRead != contentLength, raise an error to reject truncated bodies rather than returning new PooledBuffer(knownBuffer, totalRead).
🟡 Minor comments (5)
src/Clients/Goa.Clients.Dynamo/GlobalSecondaryIndexAttribute.cs-8-8 (1)
8-8:⚠️ Potential issue | 🟡 MinorDocumentation contradicts sealed keyword; clarify whether class should be inheritable.
Line 8: The XML documentation states "This attribute is inherited, allowing base classes to define common GSI patterns," but the
sealedkeyword prevents inheriting from this class. TheInherited = trueinAttributeUsagecontrols whether the attribute itself propagates to derived classes—not whether the attribute class itself can be subclassed. Update the documentation to clarify this distinction, or removesealedif class inheritance is intended.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Clients/Goa.Clients.Dynamo/GlobalSecondaryIndexAttribute.cs` at line 8, The XML doc for GlobalSecondaryIndexAttribute conflicts with the sealed modifier: decide whether the attribute class itself should be inheritable or only its usage should be inherited; if you want consumers to derive new attribute types remove the sealed keyword from the GlobalSecondaryIndexAttribute declaration so the class can be subclassed, otherwise update the documentation to state that only the attribute usage propagates to derived target classes (controlled by AttributeUsage(Inherited = true)) and keep the sealed keyword; reference GlobalSecondaryIndexAttribute, the sealed modifier, and the AttributeUsage(Inherited = true) setting when making the change.src/Clients/Goa.Clients.Lambda/LambdaServiceClient.cs-43-49 (1)
43-49:⚠️ Potential issue | 🟡 MinorValidate unsupported
LogTypevalues before entering thetry.The new switch now detects out-of-range enum values, but the surrounding
catchimmediately converts that local validation problem intoLambda.InvokeSynchronous.Failed. That makes bad caller input look like a service fault.🛠️ Suggested fix
+ if (request.LogType is not LogType.None and not LogType.Tail) + return Error.Validation("InvokeRequest.LogType", $"Unsupported log type: {request.LogType}"); + try { var headers = new Dictionary<string, string>(); // Add log type header - if (request.LogType != LogType.None) - headers["X-Amz-Log-Type"] = request.LogType switch - { - LogType.None => nameof(LogType.None), - LogType.Tail => nameof(LogType.Tail), - _ => throw new ArgumentOutOfRangeException(nameof(request.LogType), request.LogType, null) - }; + if (request.LogType == LogType.Tail) + headers["X-Amz-Log-Type"] = nameof(LogType.Tail);Also applies to: 83-86
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Clients/Goa.Clients.Lambda/LambdaServiceClient.cs` around lines 43 - 49, The switch on request.LogType that can throw ArgumentOutOfRangeException must be performed before entering the try that wraps the invoke so caller input errors are not converted into a service-failure catch; move the validation out of the try in LambdaServiceClient and explicitly validate request.LogType (e.g., if (!Enum.IsDefined(typeof(LogType), request.LogType)) throw new ArgumentOutOfRangeException(nameof(request.LogType), request.LogType, null)) before you set headers["X-Amz-Log-Type"], and then set headers using a non-throwing map (or nameof) inside the try; apply the same change to the other occurrence that assigns headers["X-Amz-Log-Type"] so both input validations happen outside the try/catch.src/Clients/Goa.Clients.Core/JsonAwsServiceClient.cs-155-156 (1)
155-156:⚠️ Potential issue | 🟡 Minor
IsJsonSerializednow rejects valid scalar JSON.Checking only
{and[means payloads like"foo",123,true,false, andnullare treated as “not serialized” and get encoded again. Any caller passing a valid scalar JSON document as astringrequest body will send different bytes than it provided.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Clients/Goa.Clients.Core/JsonAwsServiceClient.cs` around lines 155 - 156, IsJsonSerialized currently only checks for '{' or '[' and thus misclassifies valid scalar JSON (e.g. "foo", 123, true, null) as non-serialized; update IsJsonSerialized in JsonAwsServiceClient.cs to attempt to parse the trimmed input as JSON (for example with System.Text.Json.JsonDocument.Parse or Utf8JsonReader.TryParse) and return true if parsing succeeds (and false on parse failure) so both objects/arrays and scalar JSON values are recognised as already-serialized; ensure you handle exceptions (or use TryParse API) and keep the trimmed span logic for whitespace handling.src/Functions/Goa.Functions.Core/Bootstrapping/LambdaRuntimeClient.cs-48-60 (1)
48-60:⚠️ Potential issue | 🟡 MinorSubtle behavioral change: empty header values now pass validation.
The refactor replaces
FirstOrDefault()with a manual loop to avoid LINQ allocations. However, there's a nuanced difference:
- Before: If the header exists but contains no values,
FirstOrDefault()returnsnull.- After: If the header exists but contains no values,
requestIdremainsstring.Empty.For
requestIdspecifically, Line 63 only checks== null, so an empty string would now pass this validation, whereas previously it would fail. ThedeadlineMsandfunctionArnchecks useIsNullOrWhiteSpaceand are unaffected.In practice, Lambda runtime headers are unlikely to be empty, but consider aligning
requestIdvalidation with the others:Suggested fix
- if (requestId == null) + if (string.IsNullOrWhiteSpace(requestId))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Functions/Goa.Functions.Core/Bootstrapping/LambdaRuntimeClient.cs` around lines 48 - 60, The manual header-extraction loop for requestId in LambdaRuntimeClient.cs leaves requestId as string.Empty when a header exists but has no values, which bypasses the later null check; update the code to align with deadlineMs/functionArn handling by treating empty string as missing: either set requestId to null when no values are found (e.g., after the foreach, if requestId == string.Empty then requestId = null) or change the validation that checks requestId to use string.IsNullOrWhiteSpace; reference the requestId variable and the LambdaRuntimeClient header-extraction block when applying the change.src/Clients/Goa.Clients.Dynamo.Generator/CodeGeneration/JsonMapperGenerator.cs-117-118 (1)
117-118:⚠️ Potential issue | 🟡 MinorHandle
nullin the abstract write fallback.
defaultalso matchesnull, so this generated branch can throwNullReferenceExceptionbefore the intendedInvalidOperationException. Emit a null-safe type name, or guardmodelearlier, so the generated mapper fails deterministically.Possible fix
- builder.AppendLine($"Throw.InvalidOperation($\"Unknown concrete type: {{model.GetType().FullName}} for abstract type {type.FullName}\");"); + builder.AppendLine("var actualType = model?.GetType().FullName ?? \"null\";"); + builder.AppendLine($"Throw.InvalidOperation($\"Unknown concrete type: {{actualType}} for abstract type {type.FullName}\");");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Clients/Goa.Clients.Dynamo.Generator/CodeGeneration/JsonMapperGenerator.cs` around lines 117 - 118, The generated fallback branch calls model.GetType().FullName which can throw if model is null; update the code that emits the fallback (the builder.AppendLine lines that produce the InvalidOperation fallback for unknown concrete types) to be null-safe: either emit an earlier guard that handles model == null (e.g., write a deterministic null-handling branch and return) or change the produced message to use a null-safe type name expression (e.g., emit a conditional that prints "null" when model is null instead of calling GetType()). Ensure the change is made where the generator appends the fallback lines so the generated mapper no longer throws NullReferenceException.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b57dafce-8220-4e31-8afe-29c580d94ceb
📒 Files selected for processing (135)
src/Clients/Goa.Clients.Bedrock.Conversation.Dynamo/DynamoConversationStore.cssrc/Clients/Goa.Clients.Bedrock.Conversation.Dynamo/DynamoConversationStoreConfiguration.cssrc/Clients/Goa.Clients.Bedrock.Conversation.Dynamo/Internal/ContentBlockSerializer.cssrc/Clients/Goa.Clients.Bedrock.Conversation/Chat/ChatSession.cssrc/Clients/Goa.Clients.Bedrock.Conversation/Entities/Conversation.cssrc/Clients/Goa.Clients.Bedrock.Conversation/Entities/ConversationListResult.cssrc/Clients/Goa.Clients.Bedrock.Conversation/Entities/ConversationMessage.cssrc/Clients/Goa.Clients.Bedrock.Conversation/Entities/ConversationMetadata.cssrc/Clients/Goa.Clients.Bedrock.Conversation/Entities/ConversationWithMessages.cssrc/Clients/Goa.Clients.Bedrock.Conversation/Internal/XmlTagParser.cssrc/Clients/Goa.Clients.Bedrock/BedrockServiceClient.cssrc/Clients/Goa.Clients.Bedrock/BedrockServiceClientConfiguration.cssrc/Clients/Goa.Clients.Bedrock/Errors/ErrorExtensions.cssrc/Clients/Goa.Clients.Bedrock/Mcp/McpToolDefinition.cssrc/Clients/Goa.Clients.Bedrock/Models/CachePoint.cssrc/Clients/Goa.Clients.Bedrock/Models/ContentBlock.cssrc/Clients/Goa.Clients.Bedrock/Models/ConverseMetrics.cssrc/Clients/Goa.Clients.Bedrock/Models/DocumentBlock.cssrc/Clients/Goa.Clients.Bedrock/Models/GuardrailConfiguration.cssrc/Clients/Goa.Clients.Bedrock/Models/ImageBlock.cssrc/Clients/Goa.Clients.Bedrock/Models/InferenceConfiguration.cssrc/Clients/Goa.Clients.Bedrock/Models/Message.cssrc/Clients/Goa.Clients.Bedrock/Models/OutputConfig.cssrc/Clients/Goa.Clients.Bedrock/Models/PerformanceConfiguration.cssrc/Clients/Goa.Clients.Bedrock/Models/TokenUsage.cssrc/Clients/Goa.Clients.Bedrock/Models/Tool.cssrc/Clients/Goa.Clients.Bedrock/Models/ToolConfiguration.cssrc/Clients/Goa.Clients.Bedrock/Models/ToolResultBlock.cssrc/Clients/Goa.Clients.Bedrock/Models/ToolUseBlock.cssrc/Clients/Goa.Clients.Bedrock/Operations/Converse/ConverseBuilder.cssrc/Clients/Goa.Clients.Bedrock/Operations/Converse/ConverseRequest.cssrc/Clients/Goa.Clients.Bedrock/Operations/Converse/ConverseResponse.cssrc/Clients/Goa.Clients.Bedrock/Serialization/ConversationRoleConverter.cssrc/Clients/Goa.Clients.Bedrock/Serialization/GuardrailTextQualifierConverter.cssrc/Clients/Goa.Clients.Bedrock/Serialization/LatencyModeConverter.cssrc/Clients/Goa.Clients.Bedrock/Serialization/ServiceTierConverter.cssrc/Clients/Goa.Clients.Bedrock/Serialization/StopReasonConverter.cssrc/Clients/Goa.Clients.Core/AwsServiceClient.cssrc/Clients/Goa.Clients.Core/Http/ApiErrorReader.cssrc/Clients/Goa.Clients.Core/Http/ApiResponse.cssrc/Clients/Goa.Clients.Core/Http/RequestSigner.cssrc/Clients/Goa.Clients.Core/Http/ResponseHeaders.cssrc/Clients/Goa.Clients.Core/JsonAwsServiceClient.cssrc/Clients/Goa.Clients.Core/Logging/LogScope.cssrc/Clients/Goa.Clients.Core/Throw.cssrc/Clients/Goa.Clients.Core/XmlApiError.cssrc/Clients/Goa.Clients.Core/XmlAwsServiceClient.cssrc/Clients/Goa.Clients.Dynamo.Generator/CodeGeneration/JsonMapperGenerator.cssrc/Clients/Goa.Clients.Dynamo.Generator/CodeGeneration/MapperGenerator.cssrc/Clients/Goa.Clients.Dynamo/DynamoExtensions.cssrc/Clients/Goa.Clients.Dynamo/DynamoModelAttribute.cssrc/Clients/Goa.Clients.Dynamo/DynamoServiceClient.cssrc/Clients/Goa.Clients.Dynamo/DynamoServiceClientConfiguration.cssrc/Clients/Goa.Clients.Dynamo/Errors/ErrorExtensions.cssrc/Clients/Goa.Clients.Dynamo/Exceptions/DynamoPaginationException.cssrc/Clients/Goa.Clients.Dynamo/Exceptions/MissingAttributeException.cssrc/Clients/Goa.Clients.Dynamo/ExtensionAttribute.cssrc/Clients/Goa.Clients.Dynamo/Extensions/DynamoRecordExtensions.cssrc/Clients/Goa.Clients.Dynamo/GlobalSecondaryIndexAttribute.cssrc/Clients/Goa.Clients.Dynamo/IgnoreAttribute.cssrc/Clients/Goa.Clients.Dynamo/Internal/DynamoResponseReader.cssrc/Clients/Goa.Clients.Dynamo/Internal/DynamoResponseReaderRegistry.cssrc/Clients/Goa.Clients.Dynamo/Models/CapacityDetail.cssrc/Clients/Goa.Clients.Dynamo/Models/ConsumedCapacity.cssrc/Clients/Goa.Clients.Dynamo/Models/DynamoRecord.cssrc/Clients/Goa.Clients.Dynamo/Models/ItemCollectionMetrics.cssrc/Clients/Goa.Clients.Dynamo/Operations/Batch/BatchGetItemBuilder.cssrc/Clients/Goa.Clients.Dynamo/Operations/Batch/BatchGetItemRequest.cssrc/Clients/Goa.Clients.Dynamo/Operations/Batch/BatchGetItemResponse.cssrc/Clients/Goa.Clients.Dynamo/Operations/Batch/BatchGetRequestItem.cssrc/Clients/Goa.Clients.Dynamo/Operations/Batch/BatchGetTableBuilder.cssrc/Clients/Goa.Clients.Dynamo/Operations/Batch/BatchWriteItemBuilder.cssrc/Clients/Goa.Clients.Dynamo/Operations/Batch/BatchWriteItemRequest.cssrc/Clients/Goa.Clients.Dynamo/Operations/Batch/BatchWriteItemResponse.cssrc/Clients/Goa.Clients.Dynamo/Operations/Batch/BatchWriteRequestItem.cssrc/Clients/Goa.Clients.Dynamo/Operations/Batch/BatchWriteResult.cssrc/Clients/Goa.Clients.Dynamo/Operations/Batch/BatchWriteTableBuilder.cssrc/Clients/Goa.Clients.Dynamo/Operations/Batch/DeleteRequest.cssrc/Clients/Goa.Clients.Dynamo/Operations/Batch/PutRequest.cssrc/Clients/Goa.Clients.Dynamo/Operations/Condition.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/GetItem/GetItemBuilder.cssrc/Clients/Goa.Clients.Dynamo/Operations/GetItem/GetItemRequest.cssrc/Clients/Goa.Clients.Dynamo/Operations/GetItem/GetItemResponse.cssrc/Clients/Goa.Clients.Dynamo/Operations/PutItem/PutItemBuilder.cssrc/Clients/Goa.Clients.Dynamo/Operations/PutItem/PutItemRequest.cssrc/Clients/Goa.Clients.Dynamo/Operations/PutItem/PutItemResponse.cssrc/Clients/Goa.Clients.Dynamo/Operations/Query/QueryBuilder.cssrc/Clients/Goa.Clients.Dynamo/Operations/Query/QueryRequest.cssrc/Clients/Goa.Clients.Dynamo/Operations/Query/QueryResponse.cssrc/Clients/Goa.Clients.Dynamo/Operations/Query/QueryResult.cssrc/Clients/Goa.Clients.Dynamo/Operations/Scan/ScanBuilder.cssrc/Clients/Goa.Clients.Dynamo/Operations/Scan/ScanRequest.cssrc/Clients/Goa.Clients.Dynamo/Operations/Scan/ScanResponse.cssrc/Clients/Goa.Clients.Dynamo/Operations/Scan/ScanResult.cssrc/Clients/Goa.Clients.Dynamo/Operations/Transactions/TransactConditionCheckItem.cssrc/Clients/Goa.Clients.Dynamo/Operations/Transactions/TransactDeleteItem.cssrc/Clients/Goa.Clients.Dynamo/Operations/Transactions/TransactGetBuilder.cssrc/Clients/Goa.Clients.Dynamo/Operations/Transactions/TransactGetItem.cssrc/Clients/Goa.Clients.Dynamo/Operations/Transactions/TransactGetItemRequest.cssrc/Clients/Goa.Clients.Dynamo/Operations/Transactions/TransactGetItemResponse.cssrc/Clients/Goa.Clients.Dynamo/Operations/Transactions/TransactGetRequest.cssrc/Clients/Goa.Clients.Dynamo/Operations/Transactions/TransactGetResult.cssrc/Clients/Goa.Clients.Dynamo/Operations/Transactions/TransactPutItem.cssrc/Clients/Goa.Clients.Dynamo/Operations/Transactions/TransactUpdateItem.cssrc/Clients/Goa.Clients.Dynamo/Operations/Transactions/TransactWriteBuilder.cssrc/Clients/Goa.Clients.Dynamo/Operations/Transactions/TransactWriteItem.cssrc/Clients/Goa.Clients.Dynamo/Operations/Transactions/TransactWriteItemResponse.cssrc/Clients/Goa.Clients.Dynamo/Operations/Transactions/TransactWriteOperation.cssrc/Clients/Goa.Clients.Dynamo/Operations/Transactions/TransactWriteRequest.cssrc/Clients/Goa.Clients.Dynamo/Operations/UpdateItem/UpdateItemBuilder.cssrc/Clients/Goa.Clients.Dynamo/Operations/UpdateItem/UpdateItemRequest.cssrc/Clients/Goa.Clients.Dynamo/Operations/UpdateItem/UpdateItemResponse.cssrc/Clients/Goa.Clients.Dynamo/SerializedNameAttribute.cssrc/Clients/Goa.Clients.Dynamo/UnixTimestampAttribute.cssrc/Clients/Goa.Clients.EventBridge/EventBridgeServiceClient.cssrc/Clients/Goa.Clients.EventBridge/Log.cssrc/Clients/Goa.Clients.Lambda/LambdaServiceClient.cssrc/Clients/Goa.Clients.Lambda/Log.cssrc/Clients/Goa.Clients.Lambda/Operations/Invoke/InvokeResponse.cssrc/Clients/Goa.Clients.Sns/Log.cssrc/Clients/Goa.Clients.Sns/SnsServiceClient.cssrc/Clients/Goa.Clients.Sqs/Log.cssrc/Clients/Goa.Clients.Sqs/SqsExtensions.cssrc/Clients/Goa.Clients.Sqs/SqsServiceClient.cssrc/Functions/Goa.Functions.ApiGateway/Features/V1/LambdaHttpRequestFeatureV1.cssrc/Functions/Goa.Functions.ApiGateway/Features/V2/LambdaHttpAuthenticationFeatureV2.cssrc/Functions/Goa.Functions.Core/Bootstrapping/LambdaRuntimeClient.cssrc/Functions/Goa.Functions.Core/Logging/JsonLogger.cssrc/Goa.Core/CollectionExtensions.cssrc/Goa.Core/LoggerExtensions.cstests/Clients/Goa.Clients.Dynamo.Benchmarks/Benchmarks/QueryAsyncBenchmarks.cstests/Clients/Goa.Clients.Dynamo.Generator.Tests/CodeGeneration/MapperGeneratorTests.cs
💤 Files with no reviewable changes (1)
- src/Clients/Goa.Clients.Core/Http/ResponseHeaders.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.Generator/DynamoMapperIncrementalGenerator.cs`:
- Around line 221-226: The code currently unwraps only one collection layer
using IsCollectionType and sets propertyType = elementType, which misses nested
collections like List<List<MyType>>; update the logic in
DynamoMapperIncrementalGenerator.cs (the block using IsCollectionType and the
later system-type skip) to loop/recursively unwrap: repeatedly call
IsCollectionType(out var elementType) and set propertyType = elementType until
IsCollectionType returns false, so nested collection element types are fully
resolved before the subsequent system-type check and collection handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fdfa949e-9dc1-4214-8f0b-547652d3fae3
📒 Files selected for processing (2)
src/Clients/Goa.Clients.Dynamo.Generator/DynamoMapperIncrementalGenerator.cssrc/Clients/Goa.Clients.Dynamo/Operations/Condition.cs
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/Clients/Goa.Clients.Dynamo/Internal/PropertyNameCache.cs (1)
14-19: Consider defensive null-coalescing inGetOrAddto guard against accidental default-initializationThe theoretical risk is real: in C#,
default(PropertyNameCache)bypasses constructors and leaves_cachenull, which would crash at line 27 inGetOrAdd. However, the codebase never actually uses this pattern—all five instantiation sites explicitly callnew PropertyNameCache()ornew PropertyNameCache(int), and there are no uninitialized struct fields. The code is currently safe.To prevent future regressions and improve defensive coding, consider lazy-initializing
_cacheinGetOrAdd:Optional defensive pattern
public string GetOrAdd(ref Utf8JsonReader reader) { + _cache ??= new Dictionary<ulong, string>(15); var key = ComputeKey(reader.ValueSpan); if (_cache.TryGetValue(key, out var cached) && reader.ValueTextEquals(cached)) return cached;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Clients/Goa.Clients.Dynamo/Internal/PropertyNameCache.cs` around lines 14 - 19, GetOrAdd may dereference the private field _cache which can be null if PropertyNameCache is default-initialized; update GetOrAdd to defensively lazy-initialize _cache using null-coalescing (assign a new ConcurrentDictionary or appropriate collection into _cache if it's null) before using it. Locate the GetOrAdd method in the PropertyNameCache class and add a thread-safe null check/assignment (e.g., use Interlocked.CompareExchange or double-check pattern) so the method works even when the constructor was bypassed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/Clients/Goa.Clients.Dynamo/Internal/PropertyNameCache.cs`:
- Around line 14-19: GetOrAdd may dereference the private field _cache which can
be null if PropertyNameCache is default-initialized; update GetOrAdd to
defensively lazy-initialize _cache using null-coalescing (assign a new
ConcurrentDictionary or appropriate collection into _cache if it's null) before
using it. Locate the GetOrAdd method in the PropertyNameCache class and add a
thread-safe null check/assignment (e.g., use Interlocked.CompareExchange or
double-check pattern) so the method works even when the constructor was
bypassed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4843e30b-5595-46b2-a06d-c2ce6ca81119
📒 Files selected for processing (1)
src/Clients/Goa.Clients.Dynamo/Internal/PropertyNameCache.cs
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.
No description provided.