feat: Add repeating section support for collection types#36
Conversation
Adds support for variable-length collections as repeating sections
within composite keys using the `{Prop...separator}` syntax, where
`...` signals repetition and the character after it is the separator.
For example, `[CompositeKey("LOCATION#{LocationId...#}")]` on a record
with `IReadOnlyList<Guid> LocationId` generates formatting and parsing
for keys like `"LOCATION#id1#id2#id3"` where the number of IDs is
variable.
Supported collection property types are `List<T>`,
`IReadOnlyList<T>`, and `ImmutableArray<T>`. The inner element type
supports any type the library already handles (`Guid`, `string`, enum,
`ISpanParsable<T>`). An optional format specifier is supported via
`{Prop:format...separator}`, e.g. `{LocationId:D...#}`.
Repeating sections must be the last part in their key section and are
limited to one per section. Empty collections are rejected at runtime
with a `FormatException`.
Each repeated item counts as its own "part" for partial key formatting,
with runtime clamping for out-of-bounds indices. For example, with
`"LOCATION#{LocationId...#}"` and values `[id1, id2, id3]`:
- `ToPartitionKeyString(0, false)` returns `"LOCATION"`
- `ToPartitionKeyString(1, false)` returns `"LOCATION#id1"`
- `ToPartitionKeyString(2, true)` returns `"LOCATION#id1#id2#"`
- `ToPartitionKeyString(3, false)` returns `"LOCATION#id1#id2#id3"`
This commit adds support for detecting and reporting the following
diagnostics;
- COMPOSITE0009 - Repeating property must use a collection type
- COMPOSITE0010 - Collection property must use repeating syntax
- COMPOSITE0011 - Repeating property must be the last part in its
key section
…ateStringTokenizer
…s and extract helpers
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughAdds first-class support for repeating property syntax ("...separator") across tokenization, validation, analyzer diagnostics, source-generation model and emitter, and comprehensive tests; introduces three new diagnostics (COMPOSITE0009–COMPOSITE0011) and new collection-type handling for List/IReadOnlyList/ImmutableArray. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as User Code
participant Tokenizer as TemplateStringTokenizer
participant Parser as Template Parser
participant Validator as TemplateValidation/PropertyValidation
participant Analyzer as Property/Template Analyzers
participant SourceGen as Source Generator (Parser+Emitter)
participant Generated as Generated Code
Dev->>Tokenizer: Provide template string with "{Prop...#}"
Tokenizer->>Tokenizer: Detect "..." and parse separator/format
Tokenizer-->>Parser: Emit RepeatingPropertyToken
Parser->>Validator: Validate token structure
Validator-->>Parser: Structure result (position/count)
Parser->>Parser: Resolve property symbol(s)
Parser->>Validator: Validate repeating vs non-repeating types
Validator-->>Parser: Type validation result
Parser-->>Analyzer: Report diagnostics (COMPOSITE0009/0010/0011) as needed
Parser->>SourceGen: Produce RepeatingPropertyKeyPart
SourceGen->>SourceGen: Emit split/parse/format loops with count handling
SourceGen-->>Generated: Generated Parse/TryParse/ToString implementations
Dev->>Generated: Call Parse/ToString for repeating key
Generated->>Generated: Split input, loop parts, parse inner type, build collection
Generated-->>Dev: Constructed key/outputs
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
DrBarnabus
changed the title
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #36 +/- ##
==========================================
- Coverage 91.67% 87.66% -4.02%
==========================================
Files 34 35 +1
Lines 1525 2099 +574
Branches 249 344 +95
==========================================
+ Hits 1398 1840 +442
- Misses 60 157 +97
- Partials 67 102 +35 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/CompositeKey.Analyzers.Common/Validation/TemplateValidation.cs`:
- Around line 165-200: The code uses tokens.IndexOf(repeatingToken) (in the loop
inside ValidateRepeatingPropertyPlacement) which relies on record structural
equality and returns the first matching token, misclassifying a repeated token
that appears in both partition and sort sections; change the foreach over
repeatingTokens to an index-based iteration (for i = 0; i < tokens.Count; i++ or
iterate repeatingTokens with an explicit tokenIndex captured from the current
loop index) so you compute tokenIndex from the loop index (e.g., the index of
the current token in the tokens list) rather than calling IndexOf, then
determine section and lastValueToken as before; apply the same index-based fix
in ValidateRepeatingPropertyCount where IndexOf is used at the referenced
locations to avoid the structural-equality pitfall.
In `@src/CompositeKey.SourceGeneration/SourceGenerator.Emitter.cs`:
- Around line 376-383: The fixed stackalloc buffer "stackalloc Range[128]" can
overflow for large repeating collections; replace it with a dynamically-sized
array rented from ArrayPool<Range> sized from the input length (e.g. rent
inputName.Length + 1) and use its Span for {partRangesVariableName}, then after
calling {inputName}.{method} return the rented array to the pool in a finally
block; also add an explicit check after computing {partCountVariableName} that
if {partCountVariableName} > span.Length then either throw a clear
FormatException (when {shouldThrow} is true) or return false so the failure
reports a buffer-capacity problem instead of a misleading parse error.
In `@src/CompositeKey.SourceGeneration/SourceGenerator.Parser.cs`:
- Around line 185-228: The global "repeating must be last" validation that
builds valueParts (variable valueParts and loop over keyParts) incorrectly runs
for composite keys and should only run for non-composite (single-section) keys;
change the guard so the block that computes valueParts and reports
DiagnosticDescriptors.RepeatingPropertyMustBeLastPart runs only when there is no
PrimaryDelimiterKeyPart in keyParts (i.e., if !keyParts.Any(kp => kp is
PrimaryDelimiterKeyPart)) or remove the global block entirely and rely on the
existing per-section checks that use PrimaryDelimiterKeyPart, keeping usages of
ReportDiagnostic and the RepeatingPropertyKeyPart/ValueKeyPart checks intact.
🧹 Nitpick comments (15)
src/CompositeKey.SourceGeneration.FunctionalTests/PrimaryKeys.cs (1)
1-1: Missing functional test forImmutableArray<T>.The
using System.Collections.Immutableimport is added, but no record in this file actually exercisesImmutableArray<T>as a repeating property type. Since the PR objectives explicitly listImmutableArray<T>as a supported collection type alongsideList<T>andIReadOnlyList<T>, consider adding a test record such as:[CompositeKey("{Ids...#}")] public sealed partial record ImmutableArrayRepeatingKey(ImmutableArray<Guid> Ids);This would ensure end-to-end coverage for all three supported collection types.
src/CompositeKey.SourceGeneration.UnitTests/SourceGeneratorTests.cs (3)
549-573: Consider aligning the success-test pattern with existing tests.Other success tests (e.g.
BasicPrimaryKey_ShouldSuccessfullyGenerateSourceat line 30) callCompilationHelper.RunSourceGenerator(compilation)withoutdisableDiagnosticValidation: true, relying on the helper's built-in validation. Here, you disable that validation and manually filter to> DiagnosticSeverity.Info.If these compilations are expected to succeed cleanly, dropping
disableDiagnosticValidation: truewould give you the same strict validation as every other happy-path test. If there's a reason Info-level diagnostics are emitted that would trip the default validation, it would be worth a brief comment explaining why.
575-617: Failure-test assertions are weaker than the established pattern.The existing failure tests (e.g.
NonRecordType_ShouldFailCompilation) assert severity, location, and message viaCompilationHelper.AssertDiagnostics. These two new tests only check that a diagnostic with the expected ID exists. This means a regression that changes the severity toWarningor produces a garbled message would still pass.Consider matching the stricter pattern used elsewhere — resolve the location and assert the full
DiagnosticData— so these diagnostics are validated as thoroughly as the rest.
595-617: No test coverage for COMPOSITE0011 ("repeating property must be the last part").COMPOSITE0009 and COMPOSITE0010 each have a dedicated test, but COMPOSITE0011 appears to be missing. Adding a test with a repeating section followed by another part (e.g.
[CompositeKey("TAG_{Tags...#}_{Suffix}")]) would close this gap.Would you like me to draft the test case for COMPOSITE0011, or open an issue to track it?
src/CompositeKey.SourceGeneration.FunctionalTests/CompositePrimaryKeyTests.cs (4)
500-507: Consider adding edge-case entries to the invalid primary keys data.A few notable cases are missing:
- An empty repeating section, e.g.
"eccd98c4-d484-4429-896d-8fcdd77c6327|LOCATION#"— per the PR description, empty collections should throwFormatException, so this is a valuable boundary test.- A key with the correct constant prefix but no
#separator after it, e.g."eccd98c4-d484-4429-896d-8fcdd77c6327|LOCATION".These would strengthen confidence in the parser's handling of malformed repeating sections.
Suggested additions
public static object[][] CompositeWithRepeatingSort_InvalidPrimaryKeys() => [ [""], ["a"], ["a|b"], ["not-a-guid|LOCATION#not-a-guid"], - ["eccd98c4-d484-4429-896d-8fcdd77c6327|WRONG#eccd98c4-d484-4429-896d-8fcdd77c6328"] + ["eccd98c4-d484-4429-896d-8fcdd77c6327|WRONG#eccd98c4-d484-4429-896d-8fcdd77c6328"], + ["eccd98c4-d484-4429-896d-8fcdd77c6327|LOCATION#"], + ["eccd98c4-d484-4429-896d-8fcdd77c6327|LOCATION"] ];
509-515: Similarly, the partition+sort key invalid data could cover an empty repeating section.Adding a case with an empty sort-key repeating part (e.g.
"LOCATION#") would mirror the suggestion above.Suggested addition
public static object[][] CompositeWithRepeatingSort_InvalidPartitionKeyAndSortKeys() => [ ["a", "b"], ["not-a-guid", "LOCATION#not-a-guid"], ["eccd98c4-d484-4429-896d-8fcdd77c6327", "WRONG#eccd98c4-d484-4429-896d-8fcdd77c6328"], - ["eccd98c4-d484-4429-896d-8fcdd77c6327", "LOCATION#not-a-guid"] + ["eccd98c4-d484-4429-896d-8fcdd77c6327", "LOCATION#not-a-guid"], + ["eccd98c4-d484-4429-896d-8fcdd77c6327", "LOCATION#"], + ["eccd98c4-d484-4429-896d-8fcdd77c6327", "LOCATION"] ];
326-404: Missing tests for empty-collection runtime behaviour and invalidToSortKeyStringpart indices.Two gaps compared to the existing
CompositePrimaryKeytest patterns:
Empty collection at runtime — the PR states empty collections throw
FormatException, but there is no test exercisingToString()/ToSortKeyString()on aCompositeWithRepeatingSortconstructed with an emptyLocationIdlist.Invalid
ToSortKeyStringpart index — the existingCompositePrimaryKeyregion includes explicitShouldThrowInvalidOperationExceptiontests for out-of-rangethroughPartIndexvalues (lines 123–131). The new region lacks an equivalent test forCompositeWithRepeatingSort.ToSortKeyStringwith an excessive index.
377-404: Partial-key test only exercises a 3-element collection — consider a single-element case too.The test constructs
idswith three GUIDs, so the repeating section always has multiple elements. A single-element list would verify the boundary where there is exactly one repeated part and no inter-element separator is emitted, which is a common off-by-one risk area.src/CompositeKey.Analyzers.Common/Validation/TemplateValidation.cs (1)
89-112: Consider extracting shared property-accessibility validation to reduce duplication.Lines 64–87 and 89–112 perform identical accessibility checks — one for
PropertyTemplateTokenand one forRepeatingPropertyTemplateToken. A shared helper that accepts token name, getter, and setter flags would eliminate this duplication.♻️ Sketch: extract a common helper
+ private static TemplateValidationResult ValidateTokenPropertyAccessibility( + string tokenName, + List<PropertyInfo> availableProperties) + { + var property = availableProperties.FirstOrDefault(p => p.Name == tokenName); + if (property == null) + { + return TemplateValidationResult.Failure( + DiagnosticDescriptors.PropertyMustHaveAccessibleGetterAndSetter, + tokenName); + } + + var accessibilityInfo = new PropertyValidation.PropertyAccessibilityInfo( + property.Name, property.HasGetter, property.HasSetter); + + var result = PropertyValidation.ValidatePropertyAccessibility(accessibilityInfo); + if (!result.IsSuccess) + { + return TemplateValidationResult.Failure(result.Descriptor, result.MessageArgs); + } + + return TemplateValidationResult.Success(); + }Then both loops can call
ValidateTokenPropertyAccessibility(token.Name, availableProperties).src/CompositeKey.Analyzers/Analyzers/PropertyAnalyzer.cs (2)
275-302: Consider extracting shared type-info creation logic.
CreateInnerTypeInfoduplicates most of the logic fromCreatePropertyTypeInfo(Lines 307–329), differing only in the sourceITypeSymbol. Both could delegate to a sharedCreateTypeInfo(ITypeSymbol type, Compilation compilation)helper.♻️ Suggested shared helper
+ private static PropertyValidation.PropertyTypeInfo CreateTypeInfoFromSymbol( + ITypeSymbol type, + Compilation compilation) + { + var guidType = compilation.GetTypeByMetadataName("System.Guid"); + var stringType = compilation.GetSpecialType(SpecialType.System_String); + + var isGuid = SymbolEqualityComparer.Default.Equals(type, guidType); + var isString = SymbolEqualityComparer.Default.Equals(type, stringType); + var isEnum = type.TypeKind == TypeKind.Enum; + + var interfaces = type.AllInterfaces; + var isSpanParsable = interfaces.Any(i => i.ToDisplayString().StartsWith("System.ISpanParsable", StringComparison.Ordinal)); + var isSpanFormattable = interfaces.Any(i => i.ToDisplayString().Equals("System.ISpanFormattable", StringComparison.Ordinal)); + + return new PropertyValidation.PropertyTypeInfo( + TypeName: type.ToDisplayString(), + IsGuid: isGuid, + IsString: isString, + IsEnum: isEnum, + IsSpanParsable: isSpanParsable, + IsSpanFormattable: isSpanFormattable); + }Then both
CreatePropertyTypeInfoandCreateInnerTypeInfocan delegate to it.
59-126: Accessibility validation is duplicated across both branches.The
PropertyAccessibilityInfoconstruction andValidatePropertyAccessibilitycall at Lines 65–78 is repeated nearly identically in theRepeatingPropertyTemplateTokenbranch at Lines 133–146. Consider extracting into a small helper to reduce duplication, though this is a minor nit given the current scope.src/CompositeKey.SourceGeneration/SourceGenerator.Emitter.cs (2)
892-916: Fixed-part switch cases lack a catch-all forthroughPartIndexvalues that fall into the repeating range.
WriteFixedPartCasesemits aswitchstatement with explicit cases only for fixed parts. IfthroughPartIndexequals or exceedsfixedPartCount, execution falls through the switch without matching and continues toWriteRepeatingPartHandler, which is the intended flow. This works because the switch has nodefaultarm — control simply falls past the block.This is correct but could be fragile if someone later adds a
default: throwarm. A brief inline comment in the generated code (or a code comment here) noting the intentional fall-through would improve maintainability.
935-946: Minor: Intermediate string allocation for fixed prefix in repeating dynamic format.Lines 943–945 generate code like
handler.AppendFormatted(string.Create(..., $"PREFIX#{Prop}")), which creates a temporary string for the fixed prefix before appending to the handler. For the dynamic format method this is called infrequently (once per invocation), so the impact is negligible, but it's worth noting for future optimisation if needed.src/CompositeKey.SourceGeneration/SourceGenerator.Parser.cs (1)
185-193: Redundant validation — already enforced inToPropertyKeyPart.The check at Lines 188–192 verifies that no
PropertyKeyParthasCollectionType != None. However,ToPropertyKeyPart(Line 251) already rejects collection-type properties and returnsnull, which causesParseTemplateStringIntoKeyPartsto bail out before reaching this block. AnyPropertyKeyPartthat made it into thekeyPartslist is guaranteed to haveCollectionType == None.This block is dead code and can be removed to avoid confusion.
♻️ Proposed removal
- // Validate: repeating type used without repeating syntax -> COMPOSITE0010 - foreach (var keyPart in keyParts) - { - if (keyPart is PropertyKeyPart pkp && pkp.Property.CollectionType != CollectionType.None) - { - ReportDiagnostic(DiagnosticDescriptors.RepeatingTypeMustUseRepeatingSyntax, _location, pkp.Property.Name); - return null; - } - }src/CompositeKey.Analyzers.Common.UnitTests/Validation/TemplateValidationTests.cs (1)
789-894: Consider adding a test for a repeating property as the last part in the partition key.The position tests cover the sort-key side (
CompositeKey_RepeatingPropertyLastInSortKey,CompositeKey_RepeatingPropertyNotLastInSortKey) but there's no test for a repeating property being (correctly) last in the partition key section of a composite key, e.g.:{UserId}#{Tags...,} # {PostId} ^sort ^partition has repeating lastThis would exercise the partition-specific validation path (Lines 213–219 in the Parser) and help catch issues like the global-vs-per-section validation bug noted in the Parser review.
DrBarnabus
force-pushed
the
feat/repeating-section-support
branch
from
f9c2a68 to
0580992
Compare
…tring> Accept a string expression instead of an integer index so callers can pass variable names like "ri" directly, removing the brittle string.Replace hack used to substitute loop variables.
…types Add functional tests for RepeatingEnumPrimaryKey, RepeatingIntPrimaryKey, and ImmutableArrayPrimaryKey to exercise uncovered Emitter/Parser branches. Add analyzer tests for ImmutableArray collection detection, multiple repeating property count violation, and partition key section position check.
DrBarnabus
force-pushed
the
feat/repeating-section-support
branch
from
0580992 to
dc64cad
Compare
1 participant
Adds support for variable-length collections as repeating sections
within composite keys using the
{Prop...separator}syntax, where...signals repetition and the character after it is the separator.For example,
[CompositeKey("LOCATION#{LocationId...#}")]on a recordwith
IReadOnlyList<Guid> LocationIdgenerates formatting and parsingfor keys like
"LOCATION#id1#id2#id3"where the number of IDs isvariable.
Supported collection property types are
List<T>,IReadOnlyList<T>, andImmutableArray<T>. The inner element typesupports any type the library already handles (
Guid,string, enum,ISpanParsable<T>). An optional format specifier is supported via{Prop:format...separator}, e.g.{LocationId:D...#}.Repeating sections must be the last part in their key section and are
limited to one per section. Empty collections are rejected at runtime
with a
FormatException.Each repeated item counts as its own "part" for partial key formatting,
with runtime clamping for out-of-bounds indices. For example, with
"LOCATION#{LocationId...#}"and values[id1, id2, id3]:ToPartitionKeyString(0, false)returns"LOCATION"ToPartitionKeyString(1, false)returns"LOCATION#id1"ToPartitionKeyString(2, true)returns"LOCATION#id1#id2#"ToPartitionKeyString(3, false)returns"LOCATION#id1#id2#id3"This commit adds support for detecting and reporting the following
diagnostics;
key section
Summary by CodeRabbit
New Features
Behaviour / Validation
Tests