fix: recognize IncludeBase property maps during configuration validation - #130
Merged
Merged
Conversation
AssertConfigurationIsValid only inspected a type map's own PropertyMaps, so members mapped through a base map via IncludeBase() were reported as unmapped even though they resolve correctly at map time. Walk the BaseMapTypePair chain when looking up a destination member's property map. Fixes #129 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
📊 Benchmark Results
🔵 Flat Mapping — 10-property object
🟡 Flattening — 2 nested objects → 8 flat properties
🟣 Deep Mapping — 2 nested address objects
🟢 Complex Mapping — nested object + collection
🟠 Collection — 100-item
|
| Method | Mean | Error | StdDev | Min | Median | Max | Ratio | RatioSD | Rank | Gen0 | Gen1 | Allocated | Alloc Ratio |
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Manual | 1.751 μs | 0.7000 μs | 0.0384 μs | 1.716 μs | 1.745 μs | 1.792 μs | 1.00 | 0.03 | 1 | 0.5283 | 0.0172 | 8.65 KB | 1.00 |
| EggMapper | 1.782 μs | 0.6541 μs | 0.0359 μs | 1.744 μs | 1.787 μs | 1.815 μs | 1.02 | 0.03 | 1 | 0.5283 | 0.0172 | 8.65 KB | 1.00 |
| AutoMapper | 2.180 μs | 1.4394 μs | 0.0789 μs | 2.100 μs | 2.181 μs | 2.258 μs | 1.25 | 0.05 | 1 | 0.6065 | 0.0191 | 9.95 KB | 1.15 |
| Mapster | 1.690 μs | 1.0428 μs | 0.0572 μs | 1.643 μs | 1.673 μs | 1.753 μs | 0.97 | 0.03 | 1 | 0.5283 | 0.0172 | 8.65 KB | 1.00 |
| MapperlyMap | 2.054 μs | 1.1332 μs | 0.0621 μs | 1.988 μs | 2.061 μs | 2.111 μs | 1.17 | 0.04 | 1 | 0.5264 | 0.0153 | 8.65 KB | 1.00 |
| AgileMapper | 2.231 μs | 0.4289 μs | 0.0235 μs | 2.204 μs | 2.238 μs | 2.249 μs | 1.27 | 0.03 | 1 | 0.5417 | 0.0153 | 8.91 KB | 1.03 |
🟠 Collection — 100-item List<T>
| Method | Mean | Error | StdDev | Min | Median | Max | Ratio | RatioSD | Rank | Gen0 | Gen1 | Allocated | Alloc Ratio |
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Manual | 4.782 μs | 2.2360 μs | 0.1226 μs | 4.660 μs | 4.780 μs | 4.906 μs | 1.00 | 0.03 | 1 | 1.6708 | 0.0916 | 27.4 KB | 1.00 |
| EggMapper | 5.247 μs | 1.1912 μs | 0.0653 μs | 5.190 μs | 5.233 μs | 5.318 μs | 1.10 | 0.03 | 1 | 1.6708 | 0.0916 | 27.4 KB | 1.00 |
| AutoMapper | 5.924 μs | 2.6555 μs | 0.1456 μs | 5.840 μs | 5.840 μs | 6.092 μs | 1.24 | 0.04 | 1 | 1.7548 | 0.1068 | 28.7 KB | 1.05 |
| Mapster | 5.330 μs | 1.4253 μs | 0.0781 μs | 5.258 μs | 5.318 μs | 5.413 μs | 1.12 | 0.03 | 1 | 1.6708 | 0.0916 | 27.4 KB | 1.00 |
| MapperlyMap | 4.739 μs | 1.3098 μs | 0.0718 μs | 4.656 μs | 4.780 μs | 4.782 μs | 0.99 | 0.03 | 1 | 1.6785 | 0.0992 | 27.42 KB | 1.00 |
| AgileMapper | 4.553 μs | 0.9051 μs | 0.0496 μs | 4.498 μs | 4.567 μs | 4.595 μs | 0.95 | 0.02 | 1 | 1.0223 | 0.0610 | 16.72 KB | 0.61 |
🟠 Collection — 100-item List<T>
| Method | Mean | Error | StdDev | Min | Median | Max | Ratio | RatioSD | Rank | Gen0 | Gen1 | Allocated | Alloc Ratio |
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Manual | 17.92 μs | 2.854 μs | 0.156 μs | 17.75 μs | 17.97 μs | 18.05 μs | 1.00 | 0.01 | 1 | 5.2490 | 1.3123 | 85.99 KB | 1.00 |
| EggMapper | 18.21 μs | 6.715 μs | 0.368 μs | 17.83 μs | 18.25 μs | 18.56 μs | 1.02 | 0.02 | 1 | 5.2490 | 1.3123 | 85.99 KB | 1.00 |
| AutoMapper | 20.08 μs | 25.560 μs | 1.401 μs | 18.73 μs | 19.99 μs | 21.53 μs | 1.12 | 0.07 | 1 | 5.7678 | 1.4343 | 94.34 KB | 1.10 |
| Mapster | 18.89 μs | 13.350 μs | 0.732 μs | 18.05 μs | 19.31 μs | 19.32 μs | 1.05 | 0.04 | 1 | 5.2490 | 1.3123 | 85.99 KB | 1.00 |
| MapperlyMap | 17.63 μs | 0.955 μs | 0.052 μs | 17.57 μs | 17.64 μs | 17.67 μs | 0.98 | 0.01 | 1 | 5.2490 | 1.2817 | 86.02 KB | 1.00 |
| AgileMapper | 19.35 μs | 4.972 μs | 0.273 μs | 19.07 μs | 19.35 μs | 19.62 μs | 1.08 | 0.02 | 1 | 5.2795 | 1.3123 | 86.25 KB | 1.00 |
⚪ Startup / Configuration time
| Method | Mean | Error | StdDev | Min | Median | Max | Ratio | RatioSD | Rank | Gen0 | Gen1 | Allocated | Alloc Ratio |
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| EggMapperStartup | 1,101.170 μs | 655.4533 μs | 35.9276 μs | 1,064.900 μs | 1,101.865 μs | 1,136.745 μs | 1.001 | 0.04 | 3 | 3.9063 | 1.9531 | 94.76 KB | 1.00 |
| AutoMapperStartup | 199.143 μs | 122.0991 μs | 6.6927 μs | 193.718 μs | 197.088 μs | 206.622 μs | 0.181 | 0.01 | 2 | 5.8594 | 0.9766 | 104.12 KB | 1.10 |
| MapsterStartup | 2.243 μs | 0.7563 μs | 0.0415 μs | 2.197 μs | 2.253 μs | 2.279 μs | 0.002 | 0.00 | 1 | 0.7019 | 0.0267 | 11.51 KB | 0.12 |
EggMapper.Benchmarks.ColdStartBenchmark-report-github
| Method | Mean | Error | StdDev | Min | Median | Max | Ratio | RatioSD | Rank | Gen0 | Gen1 | Allocated | Alloc Ratio |
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| EggMapper | 1.147 ms | 0.7232 ms | 0.0396 ms | 1.121 ms | 1.127 ms | 1.192 ms | 1.00 | 0.04 | 1 | 5.8594 | 3.9063 | 96.65 KB | 1.00 |
| AutoMapper | 3.734 ms | 9.0190 ms | 0.4944 ms | 3.193 ms | 3.846 ms | 4.162 ms | 3.26 | 0.39 | 3 | 15.6250 | 7.8125 | 310.33 KB | 3.21 |
| Mapster | 2.228 ms | 4.5004 ms | 0.2467 ms | 2.086 ms | 2.086 ms | 2.513 ms | 1.94 | 0.20 | 2 | 42.9688 | 19.5313 | 757.43 KB | 7.84 |
📝 Notes
- Each benchmark class is decorated with
[MemoryDiagnoser]and[RankColumn]. - The global config (see
src/EggMapper.Benchmarks/Program.cs) addsMin,Median, andMaxcolumns. - Manual is the hand-written baseline (ratio = 1.00). A ratio < 1 means faster than manual.
- Benchmarks run on GitHub-hosted runners — absolute times may vary between runs; focus on Ratio for comparisons.
- To reproduce locally:
cd src/EggMapper.Benchmarks dotnet run --configuration Release -- --filter '*'
The initial validation fix exposed two related gaps found during review: - BuildFlexibleDelegate only merged PropertyMaps from the immediate BaseMapTypePair, so a multi-level IncludeBase chain silently dropped properties defined more than one hop up at map time, even though validation (which already walked the full chain) reported success. Now walks the whole chain, most-derived first. - ProjectionBuilder.BuildExpression never consulted BaseMapTypePair at all, so ProjectTo/LINQ projections dropped IncludeBase-only members entirely. Now walks the same chain as the runtime mapper. Also added cycle guards to all three BaseMapTypePair chain walks (validation, flexible delegate, projection) so a misconfigured/circular IncludeBase pairing fails fast instead of hanging. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The previous commit fixed IncludeBase transitivity but did so by re-walking BaseMapTypePair live in three separate places (BuildFlexibleDelegate, ProjectionBuilder, AssertConfigurationIsValid), which review flagged as both a correctness risk (copy-pasted cycle guards drifting independently) and a violation of this project's compile-once architecture (ProjectionBuilder re-walked the chain, with a fresh HashSet allocation, for every destination property on every ProjectTo() call). More importantly, review caught a real regression: consolidating the walk into BuildFlexibleDelegate's own pass reversed action execution order (own-level before base-level, instead of base-level before own-level), which silently breaks a derived-level Condition() that reads a destination member populated by a base-level map. Fix: precompute each TypeMap's effective, ancestry-resolved property maps once in MapperConfiguration.ResolveIncludeBaseChains(), called right after ResolveIncludeAllDerived() during configuration build. The result (TypeMap.EffectivePropertyMaps) preserves base-before-derived execution order, resolves name conflicts by nearest-declaration-wins, and detects a circular IncludeBase() pairing with a clear InvalidOperationException instead of the possibility of an infinite walk. BuildFlexibleDelegate, ProjectionBuilder, and AssertConfigurationIsValid now all just read this precomputed list. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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
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
AssertConfigurationIsValidonly looked at a type map's ownPropertyMaps, so a destination member mapped by a base map viaIncludeBase()was flagged as unmapped even though it resolves correctly at map time.BuildFlexibleDelegateonly merged the immediate base'sPropertyMaps, dropping properties from a multi-levelIncludeBasechain at map time even when validation reported success.ProjectionBuilder.BuildExpressionnever consultedBaseMapTypePairat all, soProjectTo/LINQ projections droppedIncludeBase-only members entirely.BuildFlexibleDelegatebut reversed action execution order (own-level before base-level instead of base-level before own-level) — this silently breaks a derived-levelCondition()that reads a destination member populated by a base-level map. Caught by review, not by the original tests.ProjectionBuilder, a real per-call perf cost (freshHashSetallocation per destination property perProjectTo()call) — inconsistent with this project's "compile once at configuration time" architecture.TypeMap's effective, ancestry-resolved property maps are now precomputed once inMapperConfiguration.ResolveIncludeBaseChains()during configuration build (right afterResolveIncludeAllDerived()), preserving base-before-derived execution order and resolving name conflicts by nearest-declaration-wins. A circularIncludeBase()pairing now throws a clearInvalidOperationExceptionat configuration time instead of risking an infinite walk.BuildFlexibleDelegate,ProjectionBuilder, andAssertConfigurationIsValidall just read this precomputed list — no runtime re-walking, no duplicated logic.Fixes #129
Test plan
AssertConfigurationIsValid_passes_for_member_mapped_via_IncludeBase/_multi_level_IncludeBase— the issue's repro, plus a 3-level chain.IncludeBase_mapped_member_resolves_correctly_at_map_timeandIncludeBase_applies_ForMember_transitively_across_multiple_levels— runtimeMap()correctness at 1 and 3 levels.IncludeBase_middle_level_Ignore_suppresses_mapping_from_more_distant_ancestor— precedence when a middle level ignores a member an ancestor maps.IncludeBase_base_level_mapping_runs_before_derived_condition_that_depends_on_it— regression test for the ordering bug caught during review.IncludeBase_circular_reference_throws_at_configuration_time— circularIncludeBase()fails fast with a clear message.ProjectTo_MemberMappedViaIncludeBase_MapsCorrectly/_MultiLevelIncludeBase_MapsCorrectly—ProjectTo/LINQ projection at 1 and 3 levels.dotnet test --configuration Release— full suite green (417 unit tests, 9 analyzer tests, 15 generator tests, 11 class-mapper tests, across net8.0/net9.0/net10.0).