fix(dynamo-gen): filter [UnixTimestamp] to DateTime/DateTimeOffset properties#74
Merged
Conversation
…operties [UnixTimestamp] applied to an incompatible property type (e.g. a long TTL) is now filtered out during attribute processing rather than flowing into code generation. Covers nullable types by unwrapping the underlying type.
…tions Verify that the presence of a complex collection property does not break [UnixTimestamp] conversion (FromUnixTimeSeconds/ToUnixTimeSeconds) or [Ignore] handling on sibling properties.
Contributor
📝 WalkthroughWalkthroughThis PR introduces semantic validation to the attribute handler registry, restricting ChangesAttribute Applicability Filtering for UnixTimestamp
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/Clients/Goa.Clients.Dynamo.Generator/Attributes/AttributeHandlerRegistry.cs`:
- Line 58: The current check in AttributeHandlerRegistry that returns type.Name
is nameof(DateTime) or nameof(DateTimeOffset) is brittle; update it to use the
symbol's SpecialType for DateTime (e.g., type.SpecialType ==
SpecialType.System_DateTime) and handle DateTimeOffset via a
fully-qualified-symbol check (e.g., compare type.ToDisplayString() or the
INamedTypeSymbol.ContainingNamespace + Name to "System.DateTimeOffset" or use
compilation.GetTypeByMetadataName("System.DateTimeOffset")). Modify the method
that contains the current return (the type-name comparison) to use SpecialType
for System_DateTime and a qualified-name lookup for DateTimeOffset so custom
types named "DateTime" do not match erroneously.
- Around line 54-56: The current nullable unwrap and type checks misclassify
generics and rely on name strings; in AttributeHandlerRegistry (around the
handling of property.Type) only unwrap when the symbol is a Nullable<T> by
checking that the INamedTypeSymbol.OriginalDefinition.SpecialType ==
SpecialType.System_Nullable_T and TypeArguments.Length == 1, then set type =
TypeArguments[0]; replace any name-based checks (type.Name) with robust identity
checks using type.SpecialType (e.g., SpecialType.System_DateTime,
SpecialType.System_DateTimeOffset) or metadata/type symbol equality to detect
DateTime/DateTimeOffset; apply the same stronger check in
UnixTimestampTypeHandler (the class/method that currently uses name-based
comparison) so both places use Nullable detection via
OriginalDefinition.SpecialType and SpecialType-based or metadata identity checks
for date/time types.
In
`@tests/Clients/Goa.Clients.Dynamo.Generator.Tests/Attributes/AttributeHandlerRegistryTests.cs`:
- Around line 235-297: Add tests that verify nullable DateTime/DateTimeOffset
are unwrapped before compatibility checks: create two new tests mirroring
ProcessAttributes_UnixTimestampOnDateTimeOffsetProperty_ShouldBeIncluded but use
MockSymbolFactory.PrimitiveTypes.NullableDateTime and
MockSymbolFactory.PrimitiveTypes.NullableDateTimeOffset for the property types,
register a mock IAttributeHandler that returns a UnixTimestampAttributeInfo,
call AttributeHandlerRegistry.ProcessAttributes on the property symbol, and
assert the UnixTimestampAttributeInfo is preserved (Count == 1 and attributes[0]
is UnixTimestampAttributeInfo); this ensures
AttributeHandlerRegistry.ProcessAttributes correctly handles nullable variants
when checking UnixTimestampAttributeInfo applicability.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1191ad4b-1519-4b83-bb56-090df4b6911a
📒 Files selected for processing (3)
src/Clients/Goa.Clients.Dynamo.Generator/Attributes/AttributeHandlerRegistry.cstests/Clients/Goa.Clients.Dynamo.Generator.Tests/Attributes/AttributeHandlerRegistryTests.cstests/Clients/Goa.Clients.Dynamo.Generator.Tests/CodeGeneration/JsonMapperGeneratorTests.cs
Comment on lines
+54
to
+56
| var type = property.Type; | ||
| if (type is INamedTypeSymbol { IsGenericType: true } nullable) | ||
| type = nullable.TypeArguments[0]; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Search for nullable type handling patterns in the codebase
rg -nP --type=cs -C3 'Nullable.*SpecialType|OriginalDefinition.*Nullable|NullableAnnotation' -g '!**/bin/**' -g '!**/obj/**'Repository: Im5tu/goa
Length of output: 11271
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/Clients/Goa.Clients.Dynamo.Generator/Attributes/AttributeHandlerRegistry.cs"
# Show the relevant block around the reported lines
nl -ba "$FILE" | sed -n '1,140p' | sed -n '40,90p'
# Also locate the UnixTimestampAttributeInfo block (in case line numbers shifted)
rg -n "UnixTimestampAttributeInfo|hasUnixTimestamp|DateTimeOffset|DateTime" "$FILE"Repository: Im5tu/goa
Length of output: 96
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/Clients/Goa.Clients.Dynamo.Generator/Attributes/AttributeHandlerRegistry.cs"
# Print with line numbers using awk (nl isn't available)
awk '{print FNR "\t" $0}' "$FILE" | sed -n '40,90p'
# Locate UnixTimestamp-related logic
rg -n "UnixTimestampAttributeInfo|hasUnixTimestamp|DateTimeOffset|DateTime" "$FILE"Repository: Im5tu/goa
Length of output: 2050
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "OriginalDefinition\.SpecialType\s*==\s*SpecialType\.System_Nullable_T|System_Nullable_T" src/Clients/Goa.Clients.Dynamo.Generator -S
FILE="src/Clients/Goa.Clients.Dynamo.Generator/Models/PropertyInfo.cs"
awk '{print FNR "\t" $0}' "$FILE" | sed -n '1,120p'
FILE2="src/Clients/Goa.Clients.Dynamo.Generator/Attributes/AttributeHandlerRegistry.cs"
awk '{print FNR "\t" $0}' "$FILE2" | sed -n '1,140p'Repository: Im5tu/goa
Length of output: 6903
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find UnixTimestamp attribute info + attribute definition
rg -n "UnixTimestampAttributeInfo|UnixTimestampAttribute|UnixTimestamp" src/Clients/Goa.Clients.Dynamo.Generator -S
# Check for tests referencing UnixTimestamp applicability
rg -n "UnixTimestamp" tests/Clients/Goa.Clients.Dynamo.Generator.Tests -S || true
# Inspect AttributeHandlerRegistry usage around UnixTimestamp (if any)
rg -n "IsAttributeApplicable\\(|AttributeHandlerRegistry" src/Clients/Goa.Clients.Dynamo.Generator -SRepository: Im5tu/goa
Length of output: 38291
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect UnixTimestampTypeHandler.CanHandle logic
FILE="src/Clients/Goa.Clients.Dynamo.Generator/TypeHandlers/UnixTimestampTypeHandler.cs"
awk '{print FNR "\t" $0}' "$FILE" | sed -n '1,220p'
# Inspect AttributeHandlerRegistry tests for UnixTimestamp filtering cases
TESTFILE="tests/Clients/Goa.Clients.Dynamo.Generator.Tests/Attributes/AttributeHandlerRegistryTests.cs"
awk '{print FNR "\t" $0}' "$TESTFILE" | sed -n '200,340p'
# Inspect mock primitive types (DateTime/DateTimeOffset) if it uses name-based matching
MOCKFILE="tests/Clients/Goa.Clients.Dynamo.Generator.Tests/Helpers/MockSymbolFactory.cs"
awk '{print FNR "\t" $0}' "$MOCKFILE" | sed -n '1,130p'Repository: Im5tu/goa
Length of output: 18990
Fix UnixTimestamp attribute type validation (only unwrap Nullable and use stronger type identity)
- src/Clients/Goa.Clients.Dynamo.Generator/Attributes/AttributeHandlerRegistry.cs:55-56 unwraps any generic
INamedTypeSymbolviaTypeArguments[0], soList<DateTime>-like cases can be misclassified; unwrap only whenOriginalDefinition.SpecialType == SpecialType.System_Nullable_T(andTypeArguments.Length == 1, likePropertyInfo.UnderlyingType). - src/Clients/Goa.Clients.Dynamo.Generator/Attributes/AttributeHandlerRegistry.cs:58 uses name-based
type.Namematching; switch totype.SpecialType(e.g.,System_DateTime/System_DateTimeOffset) or metadata identity to prevent unrelated types namedDateTime/DateTimeOffset. - Also update src/Clients/Goa.Clients.Dynamo.Generator/TypeHandlers/UnixTimestampTypeHandler.cs:18, which repeats the same name-based check, otherwise the overall validation remains bypassable.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/Clients/Goa.Clients.Dynamo.Generator/Attributes/AttributeHandlerRegistry.cs`
around lines 54 - 56, The current nullable unwrap and type checks misclassify
generics and rely on name strings; in AttributeHandlerRegistry (around the
handling of property.Type) only unwrap when the symbol is a Nullable<T> by
checking that the INamedTypeSymbol.OriginalDefinition.SpecialType ==
SpecialType.System_Nullable_T and TypeArguments.Length == 1, then set type =
TypeArguments[0]; replace any name-based checks (type.Name) with robust identity
checks using type.SpecialType (e.g., SpecialType.System_DateTime,
SpecialType.System_DateTimeOffset) or metadata/type symbol equality to detect
DateTime/DateTimeOffset; apply the same stronger check in
UnixTimestampTypeHandler (the class/method that currently uses name-based
comparison) so both places use Nullable detection via
OriginalDefinition.SpecialType and SpecialType-based or metadata identity checks
for date/time types.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/Clients/Goa.Clients.Dynamo.Generator/Attributes/AttributeHandlerRegistry.cs (1)
80-81: 💤 Low valueConsider using
ToDisplayString()on the type symbol directly.The current implementation correctly checks both name and namespace, addressing the past review concern. However, the past review suggested using
type.ToDisplayString() == "System.DateTimeOffset", which is slightly more concise and eliminates the theoretical edge case where a user definesnamespace System { struct DateTimeOffset {} }.Both approaches work correctly in practice. The suggested alternative is marginally cleaner:
return type.ToDisplayString() == "System.DateTimeOffset";🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Clients/Goa.Clients.Dynamo.Generator/Attributes/AttributeHandlerRegistry.cs` around lines 80 - 81, Replace the separate type name and namespace checks with a single direct call to ToDisplayString() on the type symbol. In the return statement that validates the DateTimeOffset type, change the condition from checking type.Name == nameof(DateTimeOffset) && type.ContainingNamespace?.ToDisplayString() == "System" to simply type.ToDisplayString() == "System.DateTimeOffset". This is more concise and eliminates the edge case where a user could define a custom DateTimeOffset in the System namespace.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/Clients/Goa.Clients.Dynamo.Generator/Attributes/AttributeHandlerRegistry.cs`:
- Around line 70-82: The UnixTimestampTypeHandler class uses brittle name-only
comparisons at line 18 to check for DateTime and DateTimeOffset types. Replace
the current comparison logic (underlyingType.Name == nameof(DateTime) ||
underlyingType.Name == nameof(DateTimeOffset)) with the same robust symbol-based
approach used in the IsDateTimeType method: check if the type's SpecialType is
System_DateTime, and for DateTimeOffset, verify both the type name and the
fully-qualified namespace using ToDisplayString() to ensure proper type
identification.
---
Nitpick comments:
In
`@src/Clients/Goa.Clients.Dynamo.Generator/Attributes/AttributeHandlerRegistry.cs`:
- Around line 80-81: Replace the separate type name and namespace checks with a
single direct call to ToDisplayString() on the type symbol. In the return
statement that validates the DateTimeOffset type, change the condition from
checking type.Name == nameof(DateTimeOffset) &&
type.ContainingNamespace?.ToDisplayString() == "System" to simply
type.ToDisplayString() == "System.DateTimeOffset". This is more concise and
eliminates the edge case where a user could define a custom DateTimeOffset in
the System namespace.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0c159f18-0af9-4f65-90b2-351881329aea
📒 Files selected for processing (2)
src/Clients/Goa.Clients.Dynamo.Generator/Attributes/AttributeHandlerRegistry.cstests/Clients/Goa.Clients.Dynamo.Generator.Tests/Attributes/AttributeHandlerRegistryTests.cs
Comment on lines
+70
to
+82
| /// <summary> | ||
| /// Determines whether the given type is System.DateTime or System.DateTimeOffset | ||
| /// using robust symbol identity rather than brittle name comparisons. | ||
| /// </summary> | ||
| private static bool IsDateTimeType(ITypeSymbol type) | ||
| { | ||
| if (type.SpecialType == SpecialType.System_DateTime) | ||
| return true; | ||
|
|
||
| // DateTimeOffset has no SpecialType, so identify it by its fully-qualified name. | ||
| return type.Name == nameof(DateTimeOffset) | ||
| && type.ContainingNamespace?.ToDisplayString() == "System"; | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify UnixTimestampTypeHandler uses consistent type checking
# Search for the CanHandle method or type-checking logic in UnixTimestampTypeHandler
rg -n -A10 "class UnixTimestampTypeHandler|CanHandle|IsDateTimeType" src/Clients/Goa.Clients.Dynamo.Generator/TypeHandlers/UnixTimestampTypeHandler.csRepository: Im5tu/goa
Length of output: 692
Update UnixTimestampTypeHandler to use robust symbol-based type checking.
Line 18 currently uses brittle name-only comparisons (underlyingType.Name == nameof(DateTime) || underlyingType.Name == nameof(DateTimeOffset)). Update it to match the robust type identification in AttributeHandlerRegistry.IsDateTimeType(): use SpecialType.System_DateTime for DateTime and namespace-qualified name checking for DateTimeOffset.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/Clients/Goa.Clients.Dynamo.Generator/Attributes/AttributeHandlerRegistry.cs`
around lines 70 - 82, The UnixTimestampTypeHandler class uses brittle name-only
comparisons at line 18 to check for DateTime and DateTimeOffset types. Replace
the current comparison logic (underlyingType.Name == nameof(DateTime) ||
underlyingType.Name == nameof(DateTimeOffset)) with the same robust symbol-based
approach used in the IsDateTimeType method: check if the type's SpecialType is
System_DateTime, and for DateTimeOffset, verify both the type name and the
fully-qualified namespace using ToDisplayString() to ensure proper type
identification.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
[UnixTimestamp]applied to an incompatible property type (e.g. alongTTL) is now filtered out during attribute processing inAttributeHandlerRegistry, rather than flowing into code generation. Nullable types are unwrapped to their underlying type before the check.JsonMapperGeneratorregression tests verifying that the presence of a complex collection property does not break[UnixTimestamp]conversion (FromUnixTimeSeconds/ToUnixTimeSeconds) or[Ignore]handling on sibling properties.Testing
dotnet testonGoa.Clients.Dynamo.Generator.Tests— 356/356 pass.🤖 Generated with Claude Code