diff --git a/.github/workflows/sonar.yml b/.github/workflows/sonar.yml index 7ec58f75..0e1eb3f1 100644 --- a/.github/workflows/sonar.yml +++ b/.github/workflows/sonar.yml @@ -69,6 +69,9 @@ jobs: # *.verified.* are Verify snapshot oracles — generated test fixtures, not production source. Excluding them # keeps Sonar from analysing the emitted HTML/JS/Markdown as if it were hand-written code (and from counting # the near-identical snapshots as duplication). + # The Benchmarks project is a measurement harness, never shipped and never unit-tested: excluding it from + # COVERAGE (not from analysis) keeps the new-code coverage gate about the library, while its code still + # gets the SonarAnalyzer pass. - name: Begin analysis env: SONAR_TOKEN: ${{ secrets.SONAR_TOKEN }} @@ -80,6 +83,7 @@ jobs: /d:sonar.host.url="https://sonarcloud.io" /d:sonar.cs.opencover.reportsPaths="artifacts/coverage/**/coverage.opencover.xml" /d:sonar.exclusions="**/*.verified.*" + /d:sonar.coverage.exclusions="FirstClassErrors.RequestBinder.Benchmarks/**" - name: Build # Disable the CI warning ratchet (Directory.Build.props) for the analysis build only. The scanner diff --git a/Directory.Packages.props b/Directory.Packages.props index f1a268bc..116653e7 100644 --- a/Directory.Packages.props +++ b/Directory.Packages.props @@ -25,6 +25,7 @@ + diff --git a/FirstClassErrors.RequestBinder.Benchmarks/BinderBenchmarks.cs b/FirstClassErrors.RequestBinder.Benchmarks/BinderBenchmarks.cs new file mode 100644 index 00000000..5047b7da --- /dev/null +++ b/FirstClassErrors.RequestBinder.Benchmarks/BinderBenchmarks.cs @@ -0,0 +1,288 @@ +// ReSharper disable All +#region Usings declarations + +using System.Linq.Expressions; + +using BenchmarkDotNet.Attributes; +using BenchmarkDotNet.Engines; + +using FirstClassErrors; + +#endregion + +namespace FirstClassErrors.RequestBinder.Benchmarks; + +/// +/// Measures the per-request cost of the binder's happy path (the concern of issue #151): time and allocated +/// bytes when every argument is valid, against a hand-written baseline, plus the failure path and two micro +/// benchmarks isolating the irreducible call-site cost of expression-tree selectors. +/// +/// +/// Run with dotnet run -c Release --project FirstClassErrors.RequestBinder.Benchmarks -- --filter '*'. +/// Allocated bytes are exact (MemoryDiagnoser); timings in a container are indicative, comparisons are what matter. +/// +[MemoryDiagnoser] +[SimpleJob(RunStrategy.Throughput, launchCount: 1, warmupCount: 3, iterationCount: 7)] +public class BinderBenchmarks { + + private BookingRequest _fullRequest = null!; + private FiveScalarsDto _fiveScalars = null!; + private FiveScalarsDto _fiveScalarsOneMissing = null!; + private OneScalarDto _oneScalar = null!; + private OneNullableIntDto _oneNullableInt = null!; + private ListOnlyDto _listOfTen = null!; + private StayDto _stay = null!; + private string _routeBookingId = null!; + + // Hoisted selectors: the SAME public API, but the Expression<> instances are allocated once — isolates the + // call-site expression-tree allocation from the binder's internal work. + private static readonly Expression> FirstSelector = d => d.First; + private static readonly Expression> SecondSelector = d => d.Second; + private static readonly Expression> ThirdSelector = d => d.Third; + private static readonly Expression> FourthSelector = d => d.Fourth; + private static readonly Expression> FifthSelector = d => d.Fifth; + + [GlobalSetup] + public void Setup() { + _fullRequest = new BookingRequest { + GuestEmail = "guest@example.org", + Reference = "BK-2026-000123", + Currency = "EUR", + Nights = 3, + MaxNights = 10, + Stay = new StayDto { CheckIn = "2026-08-01", CheckOut = "2026-08-04" }, + Tags = new List { "beach", "family", "late-checkout" }, + RoomNumbers = new List { 101, 102, 210 }, + Guests = new List { + new GuestDto { FirstName = "Ada", Email = "ada@example.org" }, + new GuestDto { FirstName = "Blaise", Email = null }, + }, + }; + _fiveScalars = new FiveScalarsDto { + First = "guest@example.org", + Second = "BK-2026-000123", + Third = "EUR", + Fourth = "beach", + Fifth = "2026-08-01", + }; + _fiveScalarsOneMissing = new FiveScalarsDto { + First = "guest@example.org", + Second = null, // the missing required argument + Third = "EUR", + Fourth = "beach", + Fifth = "2026-08-01", + }; + _oneScalar = new OneScalarDto { First = "guest@example.org" }; + _oneNullableInt = new OneNullableIntDto { Count = 3 }; + _listOfTen = new ListOnlyDto { Items = new List { "a1", "b2", "c3", "d4", "e5", "f6", "g7", "h8", "i9", "j10" } }; + _stay = new StayDto { CheckIn = "2026-08-01", CheckOut = "2026-08-04" }; + _routeBookingId = "bk_0123456789"; + } + + // ----------------------------------------------------------------------------------------------------------------- + // Full realistic request — the canonical BookingBinder shape (9 bound inputs, nested binder, three lists). + // ----------------------------------------------------------------------------------------------------------------- + + [Benchmark] + public Outcome FullBooking_HappyPath() { + RequestBinder binder = Bind.Request(BenchmarkErrors.CommandInvalid); + PropertySource body = binder.PropertiesOf(_fullRequest); + + RequiredField email = body.SimpleProperty(r => r.GuestEmail).AsRequired(EmailAddress.Parse); + RequiredField reference = body.SimpleProperty(r => r.Reference).AsRequired(); + RequiredField currency = body.SimpleProperty(r => r.Currency).AsOptional(Currency.Parse, "EUR"); + RequiredField nights = body.SimpleProperty(r => r.Nights).AsRequired(NightCount.From); + OptionalValueField maxNights = body.SimpleProperty(r => r.MaxNights).AsOptionalValue(NightCount.From); + RequiredField stay = body.ComplexProperty(r => r.Stay).FailWith(BenchmarkErrors.StayInvalid).AsRequired(BindStay); + RequiredField> tags = body.ListOfSimpleProperties(r => r.Tags).AsOptional(Tag.Parse); + RequiredField> rooms = body.ListOfSimpleProperties(r => r.RoomNumbers).AsRequired(RoomNumber.From); + RequiredField> guests = body.ListOfComplexProperties(r => r.Guests).FailWith(BenchmarkErrors.GuestInvalid).AsRequired(BindGuest); + + return binder.New(s => new PlaceBookingCommand( + s.Get(email), + s.Get(reference), + s.Get(currency), + s.Get(nights), + s.Get(maxNights), + s.Get(stay), + s.Get(tags), + s.Get(rooms), + s.Get(guests))); + } + + // ----------------------------------------------------------------------------------------------------------------- + // Scalar scaling — 5 properties vs 1 property gives the marginal per-property cost. + // ----------------------------------------------------------------------------------------------------------------- + + [Benchmark] + public Outcome Scalars5_HappyPath() { + RequestBinder binder = Bind.Request(BenchmarkErrors.CommandInvalid); + PropertySource body = binder.PropertiesOf(_fiveScalars); + + RequiredField first = body.SimpleProperty(d => d.First).AsRequired(EmailAddress.Parse); + RequiredField second = body.SimpleProperty(d => d.Second).AsRequired(); + RequiredField third = body.SimpleProperty(d => d.Third).AsRequired(Currency.Parse); + RequiredField fourth = body.SimpleProperty(d => d.Fourth).AsRequired(Tag.Parse); + RequiredField fifth = body.SimpleProperty(d => d.Fifth).AsRequired(BookingDate.Parse); + + return binder.New(s => new FiveScalarsCommand(s.Get(first), s.Get(second), s.Get(third), s.Get(fourth), s.Get(fifth))); + } + + [Benchmark] + public Outcome Scalars5_HoistedSelectors_HappyPath() { + RequestBinder binder = Bind.Request(BenchmarkErrors.CommandInvalid); + PropertySource body = binder.PropertiesOf(_fiveScalars); + + RequiredField first = body.SimpleProperty(FirstSelector).AsRequired(EmailAddress.Parse); + RequiredField second = body.SimpleProperty(SecondSelector).AsRequired(); + RequiredField third = body.SimpleProperty(ThirdSelector).AsRequired(Currency.Parse); + RequiredField fourth = body.SimpleProperty(FourthSelector).AsRequired(Tag.Parse); + RequiredField fifth = body.SimpleProperty(FifthSelector).AsRequired(BookingDate.Parse); + + return binder.New(s => new FiveScalarsCommand(s.Get(first), s.Get(second), s.Get(third), s.Get(fourth), s.Get(fifth))); + } + + [Benchmark] + public Outcome Scalar1_String_HappyPath() { + RequestBinder binder = Bind.Request(BenchmarkErrors.CommandInvalid); + PropertySource body = binder.PropertiesOf(_oneScalar); + + RequiredField first = body.SimpleProperty(d => d.First).AsRequired(EmailAddress.Parse); + + return binder.New(s => s.Get(first)); + } + + [Benchmark] + public Outcome Scalar1_NullableInt_HappyPath() { + RequestBinder binder = Bind.Request(BenchmarkErrors.CommandInvalid); + PropertySource body = binder.PropertiesOf(_oneNullableInt); + + RequiredField count = body.SimpleProperty(d => d.Count).AsRequired(NightCount.From); + + return binder.New(s => s.Get(count)); + } + + // ----------------------------------------------------------------------------------------------------------------- + // Lists and nesting. + // ----------------------------------------------------------------------------------------------------------------- + + [Benchmark] + public Outcome> ListOfStrings10_HappyPath() { + RequestBinder binder = Bind.Request(BenchmarkErrors.CommandInvalid); + PropertySource body = binder.PropertiesOf(_listOfTen); + + RequiredField> items = body.ListOfSimpleProperties(d => d.Items).AsRequired(Tag.Parse); + + return binder.New(s => s.Get(items)); + } + + [Benchmark] + public Outcome Nested_Stay_HappyPath() { + RequestBinder binder = Bind.Request(BenchmarkErrors.CommandInvalid); + + return BindStay(binder, _stay); + } + + // ----------------------------------------------------------------------------------------------------------------- + // Out-of-DTO argument — no expression tree, no reflection: the floor the DTO-property path could approach. + // ----------------------------------------------------------------------------------------------------------------- + + [Benchmark] + public Outcome OutOfDtoArgument_HappyPath() { + RequestBinder binder = Bind.Request(BenchmarkErrors.CommandInvalid); + + RequiredField id = binder.Argument("bookingId").FromRoute(_routeBookingId).AsRequired(); + + return binder.New(s => s.Get(id)); + } + + // ----------------------------------------------------------------------------------------------------------------- + // Failure path — must not regress when the happy path gets cheaper. + // ----------------------------------------------------------------------------------------------------------------- + + [Benchmark] + public Outcome Scalars5_OneMissing_FailurePath() { + RequestBinder binder = Bind.Request(BenchmarkErrors.CommandInvalid); + PropertySource body = binder.PropertiesOf(_fiveScalarsOneMissing); + + RequiredField first = body.SimpleProperty(d => d.First).AsRequired(EmailAddress.Parse); + RequiredField second = body.SimpleProperty(d => d.Second).AsRequired(); + RequiredField third = body.SimpleProperty(d => d.Third).AsRequired(Currency.Parse); + RequiredField fourth = body.SimpleProperty(d => d.Fourth).AsRequired(Tag.Parse); + RequiredField fifth = body.SimpleProperty(d => d.Fifth).AsRequired(BookingDate.Parse); + + return binder.New(s => new FiveScalarsCommand(s.Get(first), s.Get(second), s.Get(third), s.Get(fourth), s.Get(fifth))); + } + + // ----------------------------------------------------------------------------------------------------------------- + // Hand-written baseline — the absolute floor: null checks and direct construction, no binder. + // ----------------------------------------------------------------------------------------------------------------- + + [Benchmark(Baseline = true)] + public Outcome Manual_Scalars5_HappyPath() { + FiveScalarsDto dto = _fiveScalars; + + if (dto.First is null || dto.Second is null || dto.Third is null || dto.Fourth is null || dto.Fifth is null) { + return Outcome.Failure(BenchmarkErrors.CommandInvalid(new PrimaryPortInnerErrors())); + } + + Outcome first = EmailAddress.Parse(dto.First); + Outcome third = Currency.Parse(dto.Third); + Outcome fourth = Tag.Parse(dto.Fourth); + Outcome fifth = BookingDate.Parse(dto.Fifth); + + if (first.IsFailure || third.IsFailure || fourth.IsFailure || fifth.IsFailure) { + return Outcome.Failure(BenchmarkErrors.CommandInvalid(new PrimaryPortInnerErrors())); + } + + return Outcome.Success(new FiveScalarsCommand( + first.GetResultOrThrow(), + dto.Second, + third.GetResultOrThrow(), + fourth.GetResultOrThrow(), + fifth.GetResultOrThrow())); + } + + // ----------------------------------------------------------------------------------------------------------------- + // Micro — the irreducible call-site cost of an expression-tree selector vs a compiler-cached delegate. + // This is the part of the per-property cost that NO internal optimization can remove: it quantifies exactly + // what an API alternative (delegate + name) would buy, for the v1 API-freeze decision. + // ----------------------------------------------------------------------------------------------------------------- + + [Benchmark] + public Expression> Micro_ExpressionTreeSelector_Allocation() { + Expression> selector = d => d.First; + + return selector; + } + + [Benchmark] + public Func Micro_CachedDelegateSelector_Allocation() { + Func selector = static d => d.First; + + return selector; + } + + // ----------------------------------------------------------------------------------------------------------------- + // Nested binders shared by the full-booking scenario. + // ----------------------------------------------------------------------------------------------------------------- + + private static Outcome BindStay(RequestBinder binder, StayDto dto) { + PropertySource stay = binder.PropertiesOf(dto); + + RequiredField checkIn = stay.SimpleProperty(s => s.CheckIn).AsRequired(BookingDate.Parse); + RequiredField checkOut = stay.SimpleProperty(s => s.CheckOut).AsRequired(BookingDate.Parse); + + return binder.New(s => new Stay(s.Get(checkIn), s.Get(checkOut))); + } + + private static Outcome BindGuest(RequestBinder binder, GuestDto dto) { + PropertySource guest = binder.PropertiesOf(dto); + + RequiredField firstName = guest.SimpleProperty(g => g.FirstName).AsRequired(); + OptionalReferenceField email = guest.SimpleProperty(g => g.Email).AsOptionalReference(EmailAddress.Parse); + + return binder.New(s => new Guest(s.Get(firstName), s.Get(email))); + } + +} diff --git a/FirstClassErrors.RequestBinder.Benchmarks/FirstClassErrors.RequestBinder.Benchmarks.csproj b/FirstClassErrors.RequestBinder.Benchmarks/FirstClassErrors.RequestBinder.Benchmarks.csproj new file mode 100644 index 00000000..9a8b99eb --- /dev/null +++ b/FirstClassErrors.RequestBinder.Benchmarks/FirstClassErrors.RequestBinder.Benchmarks.csproj @@ -0,0 +1,23 @@ + + + + Exe + + net10.0 + enable + enable + latest + false + + true + + + + + + + + + + + diff --git a/FirstClassErrors.RequestBinder.Benchmarks/Program.cs b/FirstClassErrors.RequestBinder.Benchmarks/Program.cs new file mode 100644 index 00000000..7c3007a1 --- /dev/null +++ b/FirstClassErrors.RequestBinder.Benchmarks/Program.cs @@ -0,0 +1,15 @@ +#region Usings declarations + +using BenchmarkDotNet.Running; + +#endregion + +namespace FirstClassErrors.RequestBinder.Benchmarks; + +internal static class Program { + + private static void Main(string[] args) { + BenchmarkSwitcher.FromAssembly(typeof(Program).Assembly).Run(args); + } + +} diff --git a/FirstClassErrors.RequestBinder.Benchmarks/README.md b/FirstClassErrors.RequestBinder.Benchmarks/README.md new file mode 100644 index 00000000..5d2f3779 --- /dev/null +++ b/FirstClassErrors.RequestBinder.Benchmarks/README.md @@ -0,0 +1,116 @@ +# FirstClassErrors.RequestBinder.Benchmarks + +The measurement harness behind issue #151 (*binder: per-property binding uses +uncached reflection and allocates a path string on the happy path* — review +finding 15/19, gated on a decision before the v1 API freeze). It measures the +binder's **happy path**: the per-request cost paid when every argument is +valid, which is the common case at a primary-adapter boundary. + +Run it with: + +```bash +dotnet run -c Release --project FirstClassErrors.RequestBinder.Benchmarks -- --filter '*' +``` + +The project is built by CI (it lives in the solution, so it cannot bit-rot) +but is never packed and never run as a test. Timings below come from a +4-core CI-class container — treat them as relative, not absolute; allocated +bytes are exact (`MemoryDiagnoser`). + +## What each scenario isolates + +| Scenario | Isolates | +|---|---| +| `Manual_Scalars5_HappyPath` (baseline) | The floor: hand-written null checks + the same value objects, no binder. | +| `FullBooking_HappyPath` | The canonical `BookingBinder` shape: 9 inputs, a nested binder, three lists. | +| `Scalars5_HappyPath` / `Scalar1_*` | Scalar scaling; the marginal per-property cost, reference and nullable-value-type. | +| `Scalars5_HoistedSelectors_HappyPath` | The same public API with `Expression<>` selectors pre-built in static fields — everything **except** the call-site expression-tree allocation. | +| `ListOfStrings10_HappyPath` | Per-element costs (paths, list growth). | +| `Nested_Stay_HappyPath` | Nested-binder prefixes. | +| `OutOfDtoArgument_HappyPath` | The no-reflection path (`Argument(...)`): what the DTO path could approach. | +| `Scalars5_OneMissing_FailurePath` | The failure path — optimizations must not regress it. | +| `Micro_ExpressionTreeSelector_Allocation` / `Micro_CachedDelegateSelector_Allocation` | The irreducible call-site cost of an expression-tree selector vs a compiler-cached delegate — what any API alternative would buy. | + +## Results — after the #151 optimization (this tree) + +BenchmarkDotNet, net10.0, Release, 4-core container: + +| Method | Mean | Allocated | +|---|---:|---:| +| FullBooking_HappyPath | 9 712 ns | 11 488 B | +| Scalars5_HappyPath | 2 805 ns | 3 600 B | +| Scalars5_HoistedSelectors_HappyPath | 342 ns | 880 B | +| Scalar1_String_HappyPath | 481 ns | 760 B | +| Scalar1_NullableInt_HappyPath | 432 ns | 728 B | +| ListOfStrings10_HappyPath | 786 ns | 1 432 B | +| Nested_Stay_HappyPath | 981 ns | 1 472 B | +| OutOfDtoArgument_HappyPath | 81 ns | 216 B | +| Scalars5_OneMissing_FailurePath | 4 492 ns | 5 704 B | +| Manual_Scalars5_HappyPath (floor) | 62 ns | 184 B | +| Micro_ExpressionTreeSelector_Allocation | 416 ns | 488 B | +| Micro_CachedDelegateSelector_Allocation | 1 ns | 0 B | + +Byte-exact before/after (same session, `GC.GetAllocatedBytesForCurrentThread` +probe, pre-#151 HEAD vs this tree): + +| Scenario | Before | After | Δ | +|---|---:|---:|---:| +| Scalars5 | 3 728 B | 3 600 B | −128 B | +| Scalar1_String | 896 B | 848 B | −48 B | +| Scalar1_NullableInt | 920 B | 816 B | **−104 B (−11 %)** | +| OutOfDtoArgument | 352 B | 336 B | −16 B | +| ListOfStrings10 | 2 504 B | 1 624 B | **−880 B (−35 %)** | +| Nested_Stay | 1 584 B | 1 536 B | −48 B | +| FullBooking | 13 256 B | 11 992 B | **−1 264 B (−10 %)** | +| Failure_OneMissing | 5 880 B | 5 784 B | −96 B | + +Time deltas (BenchmarkDotNet, before → after): lists −63 % +(2 112 → 786 ns), nullable value type −39 % (703 → 432 ns), full booking +−16 % (11 558 → 9 712 ns), hoisted scalars −17 % (410 → 342 ns); plain +scalars are within run-to-run noise because their cost is dominated by the +call-site expression tree (below). + +## Reading the numbers + +1. **The issue's premise re-measured.** ~700 B/property is confirmed + (3 600 B / 5 scalars ≈ 720 B), but its decomposition is not what the + issue assumed. On the pre-#151 code, a top-level scalar with the default + name provider allocated **no path string at all** (the path was the + runtime-cached `PropertyInfo.Name`); eager path allocation only cost on + list elements (~3 allocations per element), nested prefixes, and custom + name providers. + +2. **~70–75 % of the per-property cost is the call-site expression tree** + (`d => d.GuestEmail`): ~488 B / ~416 ns per property, allocated by the + C# compiler in the *caller's* body — Roslyn caches delegate lambdas, never + expression-tree lambdas. No library-internal change can remove it; hoisting + the selectors into static fields removes it today with the same API + (2 805 → 342 ns, 3 600 → 880 B for 5 properties). + +3. **What #151 removed** (library-internal, no public API change): + * per-call `PropertyInfo.GetValue` reflection → compiled getters cached + per (DTO type, property), which also deletes the nullable-value-type + box and the `Nullable.GetUnderlyingType` check on every re-bind; + * eager list-element paths (`Tags[0]`…`Tags[9]`) and complex-list element + prefixes (`Guests[2]`) → built only when that element actually fails; + * eager DTO-property paths and name-provider calls → deferred to failure + recording (a custom, allocating provider now costs zero on an all-valid + bind; a bound complex property still resolves its one prefix segment); + * the per-binding error `List<>` (top-level, nested, and per element) → + created on first failure; + * unsized result lists → pre-sized from `ICollection.Count`. + +4. **What remains is the API's own shape**: one converter stage + one field + token per property (~88 B), one `Outcome` per converter call (~32 B), + and the expression tree. Removing any of those is an API decision, not an + optimization — see ADR-0023 (keep expression-tree selectors for v1; a + delegate+name overload family or a source generator are the additive + post-v1 options, with the `Micro_*` pair quantifying their ceiling). + +5. **Context for the absolute numbers**: a full realistic request binds in + ~10 µs / ~11 KB against tens-to-hundreds of µs of deserialization, I/O + and domain work around it. The failure path is unchanged (−96 B, time in + noise), and the first bind of each (DTO type, property) pays a one-time + getter compilation (~150 µs, amortized process-wide; environments without + IL emission fall back to the expression interpreter, and, failing that, to + the original reflection read). diff --git a/FirstClassErrors.RequestBinder.Benchmarks/SupportTypes.cs b/FirstClassErrors.RequestBinder.Benchmarks/SupportTypes.cs new file mode 100644 index 00000000..e5f74895 --- /dev/null +++ b/FirstClassErrors.RequestBinder.Benchmarks/SupportTypes.cs @@ -0,0 +1,325 @@ +// ReSharper disable All +#region Usings declarations + +using FirstClassErrors; + +#endregion + +namespace FirstClassErrors.RequestBinder.Benchmarks; + +// --------------------------------------------------------------------------------------------------------------------- +// Request DTOs — the shapes an adapter deserializes an incoming request into. Modeled on the canonical BookingBinder +// example so the measured path is the documented, realistic one. +// --------------------------------------------------------------------------------------------------------------------- + +public sealed class BookingRequest { + + public string? GuestEmail { get; set; } + public string? Reference { get; set; } + public string? Currency { get; set; } + public int? Nights { get; set; } + public int? MaxNights { get; set; } + public StayDto? Stay { get; set; } + public List? Tags { get; set; } + public List? RoomNumbers { get; set; } + public List? Guests { get; set; } + +} + +public sealed class StayDto { + + public string? CheckIn { get; set; } + public string? CheckOut { get; set; } + +} + +public sealed class GuestDto { + + public string? FirstName { get; set; } + public string? Email { get; set; } + +} + +public sealed class FiveScalarsDto { + + public string? First { get; set; } + public string? Second { get; set; } + public string? Third { get; set; } + public string? Fourth { get; set; } + public string? Fifth { get; set; } + +} + +public sealed class OneScalarDto { + + public string? First { get; set; } + +} + +public sealed class OneNullableIntDto { + + public int? Count { get; set; } + +} + +public sealed class ListOnlyDto { + + public List? Items { get; set; } + +} + +// --------------------------------------------------------------------------------------------------------------------- +// Value objects — deliberately minimal converters so the benchmark measures BINDER overhead, not domain-rule cost. +// Every converter succeeds on the happy-path inputs used by the benchmarks. Error codes are cached in static fields +// so the failure path does not pay ErrorCode.Create either. +// --------------------------------------------------------------------------------------------------------------------- + +public sealed class EmailAddress { + + private static readonly ErrorCode InvalidCode = ErrorCode.Create("EMAIL_ADDRESS_INVALID"); + + private EmailAddress(string value) { + Value = value; + } + + public string Value { get; } + + public static Outcome Parse(string raw) { + if (string.IsNullOrEmpty(raw) || raw.IndexOf('@') < 0) { + return Outcome.Failure( + DomainError.Create(InvalidCode, "The value is not a valid email address.") + .WithPublicMessage("The email address is invalid.")); + } + + return Outcome.Success(new EmailAddress(raw)); + } + +} + +public sealed class Currency { + + private static readonly ErrorCode InvalidCode = ErrorCode.Create("CURRENCY_INVALID"); + + private Currency(string value) { + Value = value; + } + + public string Value { get; } + + public static Outcome Parse(string raw) { + if (string.IsNullOrEmpty(raw) || raw.Length != 3) { + return Outcome.Failure( + DomainError.Create(InvalidCode, "The value is not a valid ISO currency code.") + .WithPublicMessage("The currency is invalid.")); + } + + return Outcome.Success(new Currency(raw)); + } + +} + +// A readonly struct, mirroring the Usage project's NightCount: consumer value objects may be structs — the +// class-only rule applies to the library's own invariant-carrying types. +public readonly struct NightCount { + + private static readonly ErrorCode InvalidCode = ErrorCode.Create("NIGHT_COUNT_INVALID"); + + private NightCount(int value) { + Value = value; + } + + public int Value { get; } + + public static Outcome From(int raw) { + if (raw <= 0) { + return Outcome.Failure( + DomainError.Create(InvalidCode, "A night count must be strictly positive.") + .WithPublicMessage("The night count is invalid.")); + } + + return Outcome.Success(new NightCount(raw)); + } + +} + +public sealed class BookingDate { + + private static readonly ErrorCode InvalidCode = ErrorCode.Create("BOOKING_DATE_INVALID"); + + private BookingDate(string value) { + Value = value; + } + + public string Value { get; } + + public static Outcome Parse(string raw) { + if (string.IsNullOrEmpty(raw)) { + return Outcome.Failure( + DomainError.Create(InvalidCode, "The value is not a valid booking date.") + .WithPublicMessage("The date is invalid.")); + } + + return Outcome.Success(new BookingDate(raw)); + } + +} + +public sealed class Tag { + + private static readonly ErrorCode InvalidCode = ErrorCode.Create("TAG_INVALID"); + + private Tag(string value) { + Value = value; + } + + public string Value { get; } + + public static Outcome Parse(string raw) { + if (string.IsNullOrEmpty(raw)) { + return Outcome.Failure( + DomainError.Create(InvalidCode, "A tag cannot be empty.") + .WithPublicMessage("The tag is invalid.")); + } + + return Outcome.Success(new Tag(raw)); + } + +} + +public sealed class RoomNumber { + + private static readonly ErrorCode InvalidCode = ErrorCode.Create("ROOM_NUMBER_INVALID"); + + private RoomNumber(int value) { + Value = value; + } + + public int Value { get; } + + public static Outcome From(int raw) { + if (raw <= 0) { + return Outcome.Failure( + DomainError.Create(InvalidCode, "A room number must be strictly positive.") + .WithPublicMessage("The room number is invalid.")); + } + + return Outcome.Success(new RoomNumber(raw)); + } + +} + +// --------------------------------------------------------------------------------------------------------------------- +// Commands — the bound results. +// --------------------------------------------------------------------------------------------------------------------- + +public sealed class Stay { + + public Stay(BookingDate checkIn, BookingDate checkOut) { + CheckIn = checkIn; + CheckOut = checkOut; + } + + public BookingDate CheckIn { get; } + public BookingDate CheckOut { get; } + +} + +public sealed class Guest { + + public Guest(string firstName, EmailAddress? email) { + FirstName = firstName; + Email = email; + } + + public string FirstName { get; } + public EmailAddress? Email { get; } + +} + +public sealed class PlaceBookingCommand { + + public PlaceBookingCommand(EmailAddress guestEmail, + string reference, + Currency currency, + NightCount nights, + NightCount? maxNights, + Stay stay, + IReadOnlyList tags, + IReadOnlyList rooms, + IReadOnlyList guests) { + GuestEmail = guestEmail; + Reference = reference; + Currency = currency; + Nights = nights; + MaxNights = maxNights; + Stay = stay; + Tags = tags; + Rooms = rooms; + Guests = guests; + } + + public EmailAddress GuestEmail { get; } + public string Reference { get; } + public Currency Currency { get; } + public NightCount Nights { get; } + public NightCount? MaxNights { get; } + public Stay Stay { get; } + public IReadOnlyList Tags { get; } + public IReadOnlyList Rooms { get; } + public IReadOnlyList Guests { get; } + +} + +public sealed class FiveScalarsCommand { + + public FiveScalarsCommand(EmailAddress first, string second, Currency third, Tag fourth, BookingDate fifth) { + First = first; + Second = second; + Third = third; + Fourth = fourth; + Fifth = fifth; + } + + public EmailAddress First { get; } + public string Second { get; } + public Currency Third { get; } + public Tag Fourth { get; } + public BookingDate Fifth { get; } + +} + +// --------------------------------------------------------------------------------------------------------------------- +// Envelope factories — cached codes; the envelope itself is only ever built on the failure path. +// --------------------------------------------------------------------------------------------------------------------- + +public static class BenchmarkErrors { + + private static readonly ErrorCode CommandInvalidCode = ErrorCode.Create("BOOKING_COMMAND_INVALID"); + private static readonly ErrorCode StayInvalidCode = ErrorCode.Create("BOOKING_STAY_INVALID"); + private static readonly ErrorCode GuestInvalidCode = ErrorCode.Create("BOOKING_GUEST_INVALID"); + + public static PrimaryPortError CommandInvalid(PrimaryPortInnerErrors violations) { + return PrimaryPortError.Create( + CommandInvalidCode, + "The booking command is invalid: one or more request arguments failed to bind.", + violations) + .WithPublicMessage("The booking request is invalid.", "One or more fields of the booking request are invalid."); + } + + public static PrimaryPortError StayInvalid(PrimaryPortInnerErrors violations) { + return PrimaryPortError.Create( + StayInvalidCode, + "The stay is invalid: one or more of its dates failed to bind.", + violations) + .WithPublicMessage("The stay is invalid.", "One or more dates of the stay are invalid."); + } + + public static PrimaryPortError GuestInvalid(PrimaryPortInnerErrors violations) { + return PrimaryPortError.Create( + GuestInvalidCode, + "A guest is invalid: one or more of its fields failed to bind.", + violations) + .WithPublicMessage("A guest is invalid.", "One or more fields of a guest are invalid."); + } + +} diff --git a/FirstClassErrors.RequestBinder.UnitTests/DeferredArgumentPathTests.cs b/FirstClassErrors.RequestBinder.UnitTests/DeferredArgumentPathTests.cs new file mode 100644 index 00000000..9f98c8ad --- /dev/null +++ b/FirstClassErrors.RequestBinder.UnitTests/DeferredArgumentPathTests.cs @@ -0,0 +1,159 @@ +#region Usings declarations + +using System.Reflection; + +using NFluent; + +#endregion + +namespace FirstClassErrors.RequestBinder.UnitTests; + +/// +/// Pins the deferred-path contract: an argument's path — and the call behind +/// it — only materializes when a failure is recorded. The observable bound: zero provider calls on an all-valid +/// bind of scalar and list properties; exactly one per bound complex property (its prefix segment); and never +/// more than the historical one-call-per-selected-property upper bound. +/// +public sealed class DeferredArgumentPathTests { + + [Fact(DisplayName = "An all-valid bind of scalar and list properties never consults the name provider.")] + public void AllValidScalarsAndListsNeverConsultTheProvider() { + CountingNameProvider provider = new(); + var bind = Bind.WithOptions(new RequestBinderOptions(provider)).Request(BookingEnvelopeError.CommandInvalid); + var body = bind.PropertiesOf(new BookingRequest("alice@example.org", "REF-1", "EUR", "3", Stay: null, Tags: new[] { "sea", "spa" }, Guests: null)); + + RequiredField email = body.SimpleProperty(r => r.GuestEmail).AsRequired(EmailAddress.Parse); + RequiredField refField = body.SimpleProperty(r => r.Reference).AsRequired(); + RequiredField nights = body.SimpleProperty(r => r.MaxNights).AsRequired(PositiveInt.Parse); + RequiredField> tags = body.ListOfSimpleProperties(r => r.Tags).AsRequired(Tag.Parse); + + Outcome outcome = bind.New(s => $"{s.Get(email).Value}/{s.Get(refField)}/{s.Get(nights)}/{s.Get(tags).Count}"); + + Check.That(outcome.IsSuccess).IsTrue(); + Check.That(provider.Calls).IsEqualTo(0); + } + + [Fact(DisplayName = "A failing bind consults the name provider once per failing argument only.")] + public void FailingBindConsultsTheProviderOncePerFailingArgument() { + CountingNameProvider provider = new(); + var bind = Bind.WithOptions(new RequestBinderOptions(provider)).Request(BookingEnvelopeError.CommandInvalid); + var body = bind.PropertiesOf(new BookingRequest(GuestEmail: null, "REF-1", "not-a-currency", "3", Stay: null, Tags: null, Guests: null)); + + body.SimpleProperty(r => r.GuestEmail).AsRequired(EmailAddress.Parse); // missing -> 1 call + body.SimpleProperty(r => r.Reference).AsRequired(); // valid -> 0 calls + body.SimpleProperty(r => r.Currency).AsRequired(Currency.Parse); // invalid -> 1 call + body.SimpleProperty(r => r.MaxNights).AsRequired(PositiveInt.Parse); // valid -> 0 calls + + Outcome outcome = bind.New(_ => "never"); + + Check.That(outcome.IsFailure).IsTrue(); + Check.That(provider.Calls).IsEqualTo(2); + + // And the paths are the same the eager code produced. + Check.That(outcome.Error!.InnerErrors.Select(BindingAssertions.ArgumentPathOf)).ContainsExactly("GuestEmail", "Currency"); + } + + [Fact(DisplayName = "A list with one invalid element resolves the list name once; valid elements never build a path.")] + public void PartiallyInvalidListResolvesTheListNameOnce() { + CountingNameProvider provider = new(); + var bind = Bind.WithOptions(new RequestBinderOptions(provider)).Request(BookingEnvelopeError.CommandInvalid); + var body = bind.PropertiesOf(new BookingRequest("alice@example.org", "REF-1", "EUR", "3", Stay: null, Tags: new[] { "sea", "not a tag", "spa" }, Guests: null)); + + body.SimpleProperty(r => r.GuestEmail).AsRequired(EmailAddress.Parse); + body.ListOfSimpleProperties(r => r.Tags).AsRequired(Tag.Parse); + + Outcome outcome = bind.New(_ => "never"); + + Check.That(outcome.IsFailure).IsTrue(); + Check.That(provider.Calls).IsEqualTo(1); // the list stem, resolved once, shared by the failing element's path + Check.That(BindingAssertions.ArgumentPathOf(outcome.Error!.InnerErrors.Single())).IsEqualTo("Tags[1]"); + } + + [Fact(DisplayName = "A bound complex property resolves its prefix segment exactly once, valid or not.")] + public void ComplexPropertyResolvesItsPrefixOnce() { + CountingNameProvider provider = new(); + var bind = Bind.WithOptions(new RequestBinderOptions(provider)).Request(BookingEnvelopeError.CommandInvalid); + var body = bind.PropertiesOf(new BookingRequest("alice@example.org", "REF-1", "EUR", "3", new StayDto("2026-08-01", "2026-08-04"), Tags: null, Guests: null)); + + RequiredField stay = body.ComplexProperty(r => r.Stay).FailWith(BookingEnvelopeError.StayInvalid).AsRequired(BindStay); + + Outcome outcome = bind.New(s => s.Get(stay)); + + Check.That(outcome.IsSuccess).IsTrue(); + // The nested binding needs its prefix ("Stay") up front — one provider call, exactly what the eager code + // paid; the nested properties' own paths ("Stay.CheckIn") stayed unbuilt because both dates bound. + Check.That(provider.Calls).IsEqualTo(1); + } + + [Fact(DisplayName = "Failing nested properties still carry their full, prefixed paths.")] + public void FailingNestedPropertiesCarryPrefixedPaths() { + CountingNameProvider provider = new(); + var bind = Bind.WithOptions(new RequestBinderOptions(provider)).Request(BookingEnvelopeError.CommandInvalid); + var body = bind.PropertiesOf(new BookingRequest("alice@example.org", "REF-1", "EUR", "3", new StayDto(CheckIn: null, "not-a-date"), Tags: null, Guests: null)); + + body.ComplexProperty(r => r.Stay).FailWith(BookingEnvelopeError.StayInvalid).AsRequired(BindStay); + + Outcome outcome = bind.New(_ => "never"); + + Check.That(outcome.IsFailure).IsTrue(); + Error stayEnvelope = outcome.Error!.InnerErrors.Single(); + Check.That(stayEnvelope.InnerErrors.Select(BindingAssertions.ArgumentPathOf)).ContainsExactly("Stay.CheckIn", "Stay.CheckOut"); + + // One call per distinct segment — "Stay", "CheckIn", "CheckOut" — completing the exactly-once contract on + // the failing side: the prefix is resolved once and reused (a write-back regression would show 4 calls). + Check.That(provider.Calls).IsEqualTo(3); + } + + [Fact(DisplayName = "A complex-list element failing deep inside carries its indexed, prefixed path.")] + public void FailingComplexListElementCarriesIndexedPrefixedPath() { + var bind = Bind.Request(BookingEnvelopeError.CommandInvalid); + var body = bind.PropertiesOf(new BookingRequest("alice@example.org", "REF-1", "EUR", "3", Stay: null, Tags: null, + Guests: new GuestDto?[] { new("Ada", "ada@example.org"), new(FirstName: null, "no-at-sign") })); + + body.ListOfComplexProperties(r => r.Guests).FailWith(BookingEnvelopeError.GuestInvalid).AsRequired(BindGuest); + + Outcome outcome = bind.New(_ => "never"); + + Check.That(outcome.IsFailure).IsTrue(); + Error guestEnvelope = outcome.Error!.InnerErrors.Single(); + Check.That(guestEnvelope.InnerErrors.Select(BindingAssertions.ArgumentPathOf)).ContainsExactly("Guests[1].FirstName", "Guests[1].Email"); + } + + #region Helpers & fixtures + + private static Outcome BindStay(RequestBinder binder, StayDto dto) { + var stay = binder.PropertiesOf(dto); + + RequiredField checkIn = stay.SimpleProperty(s => s.CheckIn).AsRequired(BookingDate.Parse); + RequiredField checkOut = stay.SimpleProperty(s => s.CheckOut).AsRequired(BookingDate.Parse); + + return binder.New(s => new Stay(s.Get(checkIn), s.Get(checkOut))); + } + + private static Outcome BindGuest(RequestBinder binder, GuestDto dto) { + var guest = binder.PropertiesOf(dto); + + RequiredField firstName = guest.SimpleProperty(g => g.FirstName).AsRequired(); + OptionalReferenceField email = guest.SimpleProperty(g => g.Email).AsOptionalReference(EmailAddress.Parse); + + return binder.New(s => new Guest(s.Get(firstName), s.Get(email))); + } + + /// Counts every name resolution, to observe exactly when the binder consults the provider. + private sealed class CountingNameProvider : IArgumentNameProvider { + + private int _calls; + + public int Calls => _calls; + + public string GetArgumentNameFrom(PropertyInfo property) { + Interlocked.Increment(ref _calls); + + return property.Name; + } + + } + + #endregion + +} diff --git a/FirstClassErrors.RequestBinder.UnitTests/PropertyGetterCacheTests.cs b/FirstClassErrors.RequestBinder.UnitTests/PropertyGetterCacheTests.cs new file mode 100644 index 00000000..82618ec4 --- /dev/null +++ b/FirstClassErrors.RequestBinder.UnitTests/PropertyGetterCacheTests.cs @@ -0,0 +1,180 @@ +#region Usings declarations + +using System.Reflection; + +using NFluent; + +#endregion + +namespace FirstClassErrors.RequestBinder.UnitTests; + +/// +/// Pins the contract of the compiled-getter cache (): reuse across +/// bindings, isolation across DTO types, the eager mis-declaration guard on every call, and the raw-exception +/// bug channel of a throwing DTO getter. +/// +public sealed class PropertyGetterCacheTests { + + [Fact(DisplayName = "Binding the same property twice reuses one compiled getter.")] + public void SamePropertyReusesTheCompiledGetter() { + PropertyInfo property = typeof(BookingRequest).GetProperty(nameof(BookingRequest.GuestEmail))!; + + Func first = PropertyGetters.For(property); + Func second = PropertyGetters.For(property); + + Check.That(second).IsSameReferenceAs(first); + Check.That(first(new BookingRequest("alice@example.org", null, null, null, null, null, null))).IsEqualTo("alice@example.org"); + } + + [Fact(DisplayName = "Two DTO types sharing a property name bind through distinct getters, each reading its own type.")] + public void SamePropertyNameOnDistinctDtoTypesDoesNotCollide() { + var bindFirst = Bind.Request(BookingEnvelopeError.CommandInvalid); + RequiredField first = bindFirst.PropertiesOf(new FirstDto("from-first")).SimpleProperty(d => d.Name).AsRequired(); + + var bindSecond = Bind.Request(BookingEnvelopeError.CommandInvalid); + RequiredField second = bindSecond.PropertiesOf(new SecondDto("from-second")).SimpleProperty(d => d.Name).AsRequired(); + + Check.That(bindFirst.New(s => s.Get(first)).GetResultOrThrow()).IsEqualTo("from-first"); + Check.That(bindSecond.New(s => s.Get(second)).GetResultOrThrow()).IsEqualTo("from-second"); + } + + [Fact(DisplayName = "A property declared on a base type binds through a derived DTO.")] + public void InheritedPropertyBindsThroughTheDerivedDto() { + var bind = Bind.Request(BookingEnvelopeError.CommandInvalid); + var body = bind.PropertiesOf(new DerivedDto { Inherited = "base-value", Own = "own-value" }); + + RequiredField inherited = body.SimpleProperty(d => d.Inherited).AsRequired(); + RequiredField own = body.SimpleProperty(d => d.Own).AsRequired(); + + Outcome outcome = bind.New(s => s.Get(inherited) + "/" + s.Get(own)); + Check.That(outcome.GetResultOrThrow()).IsEqualTo("base-value/own-value"); + } + + [Fact(DisplayName = "A non-nullable value-type property is rejected on every call — the guard is never cached away.")] + public void NonNullableValueTypePropertyIsRejectedOnEveryCall() { + var bind = Bind.Request(BookingEnvelopeError.CommandInvalid); + var body = bind.PropertiesOf(new MisdeclaredDto()); + + ArgumentException first = Assert.Throws(() => body.SimpleProperty(d => d.Count).AsRequired(PositiveIntFromInt)); + Check.That(first.Message).Contains("non-nullable value type"); + + // A second selection must throw again: an invalid property never enters the getter cache. + ArgumentException second = Assert.Throws(() => body.SimpleProperty(d => d.Count).AsRequired(PositiveIntFromInt)); + Check.That(second.Message).Contains("non-nullable value type"); + } + + [Fact(DisplayName = "A DTO getter that throws surfaces its own exception, not a reflection wrapper.")] + public void ThrowingDtoGetterSurfacesTheRawException() { + var bind = Bind.Request(BookingEnvelopeError.CommandInvalid); + var body = bind.PropertiesOf(new ThrowingDto()); + + // The binder's bug channel: a throwing property getter is a programming error and propagates as itself — + // a compiled getter does not wrap it in TargetInvocationException the way PropertyInfo.GetValue did. + InvalidOperationException raw = Assert.Throws(() => body.SimpleProperty(d => d.Boom).AsRequired()); + Check.That(raw.Message).IsEqualTo("getter bug"); + } + + [Fact(DisplayName = "Concurrent first binds of one property all succeed with correct values.")] + public void ConcurrentBindsOfTheSamePropertyAreSafe() { + string?[] results = new string?[32]; + + Parallel.For(0, results.Length, i => { + var bind = Bind.Request(BookingEnvelopeError.CommandInvalid); + RequiredField value = bind.PropertiesOf(new ConcurrencyDto($"value-{i}")).SimpleProperty(d => d.Value).AsRequired(); + results[i] = bind.New(s => s.Get(value)).GetResultOrThrow(); + }); + + for (int i = 0; i < results.Length; i++) { + Check.That(results[i]).IsEqualTo($"value-{i}"); + } + } + + [Fact(DisplayName = "A coercion selector between an enum property and its underlying type still binds, both ways.")] + public void EnumUnderlyingCoercionSelectorStillBinds() { + var bind = Bind.Request(BookingEnvelopeError.CommandInvalid); + var body = bind.PropertiesOf(new EnumDto(Channel.Web, 2)); + + // The reflection-based reader cast the boxed value to the selector's underlying type, which the CLR + // accepts across an enum and its underlying integral — the compiled getter must keep accepting it. + RequiredField code = body.SimpleProperty(d => (int?)d.Channel).AsRequired(IntPass); + RequiredField channel = body.SimpleProperty(d => (Channel?)d.ChannelCode).AsRequired(ChannelPass); + + Outcome outcome = bind.New(s => $"{s.Get(code)}/{s.Get(channel)}"); + Check.That(outcome.GetResultOrThrow()).IsEqualTo("1/Phone"); + } + + [Fact(DisplayName = "A nullable value-type property still binds its value, and its absence, through the cached getter.")] + public void NullableValueTypePropertyBindsThroughTheCache() { + var present = Bind.Request(BookingEnvelopeError.CommandInvalid); + RequiredField bound = present.PropertiesOf(new NullableIntDto(41)).SimpleProperty(d => d.Count).AsRequired(NextInt); + Check.That(present.New(s => s.Get(bound)).GetResultOrThrow()).IsEqualTo(42); + + var missing = Bind.Request(BookingEnvelopeError.CommandInvalid); + missing.PropertiesOf(new NullableIntDto(null)).SimpleProperty(d => d.Count).AsRequired(NextInt); + Outcome outcome = missing.New(_ => -1); + Check.That(outcome.IsFailure).IsTrue(); + Check.That(outcome.Error!.InnerErrors.Single().Code.ToString()).IsEqualTo("REQUEST_ARGUMENT_REQUIRED"); + } + + #region Helpers & fixtures + + private static Outcome PositiveIntFromInt(int raw) { + return Outcome.Success(raw); + } + + private static Outcome NextInt(int raw) { + return Outcome.Success(raw + 1); + } + + private static Outcome IntPass(int raw) { + return Outcome.Success(raw); + } + + private static Outcome ChannelPass(Channel raw) { + return Outcome.Success(raw); + } + + private sealed record FirstDto(string? Name); + + private sealed record SecondDto(string? Name); + + private class BaseDto { + + public string? Inherited { get; set; } + + } + + private sealed class DerivedDto : BaseDto { + + public string? Own { get; set; } + + } + + private sealed record MisdeclaredDto { + + public int Count { get; init; } + + } + + private sealed class ThrowingDto { + + public string? Boom => throw new InvalidOperationException("getter bug"); + + } + + private sealed record ConcurrencyDto(string? Value); + + private sealed record NullableIntDto(int? Count); + + private enum Channel { + + Web = 1, + Phone = 2 + + } + + private sealed record EnumDto(Channel? Channel, int? ChannelCode); + + #endregion + +} diff --git a/FirstClassErrors.RequestBinder/ArgumentPaths.cs b/FirstClassErrors.RequestBinder/ArgumentPaths.cs new file mode 100644 index 00000000..a810f4e6 --- /dev/null +++ b/FirstClassErrors.RequestBinder/ArgumentPaths.cs @@ -0,0 +1,41 @@ +#region Usings declarations + +using System.Reflection; + +#endregion + +namespace FirstClassErrors.RequestBinder; + +/// +/// Resolves a converter's deferred argument path. A converter stage stores its path as a single +/// field holding either the resolved (an out-of-DTO argument's +/// verbatim name, or a path already built) or the selected (a DTO property, whose +/// path — name provider, then prefix — is built only when a path is first needed: a recorded failure, a +/// complex property's nested prefix, or the misconfigured-fallback exception. An all-valid bind of scalar and +/// list-of-scalar properties never materializes one). +/// +/// +/// The resolved string is written back into the field, so the provider is consulted at most once per bound +/// property — the same upper bound as when the path was built eagerly. Bindings are single-threaded by contract, +/// so the unsynchronized write-back is safe. +/// +internal static class ArgumentPaths { + + #region Statics members declarations + + /// Resolves (and caches back) the path held by . + /// The converter's path field: a resolved or a deferred . + /// The binding whose options and prefix the path is resolved with. + /// The full argument path. + internal static string Resolve(ref object argumentPathOrProperty, RequestBinding binding) { + if (argumentPathOrProperty is string resolved) { return resolved; } + + string path = binding.PathOfProperty((PropertyInfo)argumentPathOrProperty); + argumentPathOrProperty = path; + + return path; + } + + #endregion + +} diff --git a/FirstClassErrors.RequestBinder/ComplexPropertyConverter.cs b/FirstClassErrors.RequestBinder/ComplexPropertyConverter.cs index ad442e45..26edcd99 100644 --- a/FirstClassErrors.RequestBinder/ComplexPropertyConverter.cs +++ b/FirstClassErrors.RequestBinder/ComplexPropertyConverter.cs @@ -17,7 +17,7 @@ public sealed class ComplexPropertyConverter { #region Fields declarations private readonly RequestBinding _binding; - private readonly string _argumentPath; + private object _argumentPathOrProperty; private readonly TArgument? _value; private readonly bool _isMissing; private readonly Func _envelope; @@ -27,15 +27,15 @@ public sealed class ComplexPropertyConverter { #region Constructors declarations internal ComplexPropertyConverter(RequestBinding binding, - string argumentPath, + object argumentPathOrProperty, TArgument? value, bool isMissing, Func envelope) { - _binding = binding; - _argumentPath = argumentPath; - _value = value; - _isMissing = isMissing; - _envelope = envelope; + _binding = binding; + _argumentPathOrProperty = argumentPathOrProperty; + _value = value; + _isMissing = isMissing; + _envelope = envelope; } #endregion @@ -52,7 +52,7 @@ public RequiredField AsRequired(Func(_binding, default!); } @@ -60,7 +60,7 @@ public RequiredField AsRequired(Func outcome = bindNested(new RequestBinder(nested), _value!); if (outcome.IsFailure) { - _binding.Record(NestedFailure.Group(outcome.Error!, nested.BuiltEnvelope, _argumentPath, _binding.Options.ArgumentInvalid)); + _binding.Record(NestedFailure.Group(outcome.Error!, nested.BuiltEnvelope, ArgumentPath(), _binding.Options.ArgumentInvalid)); return new RequiredField(_binding, default!); } @@ -86,7 +86,7 @@ public OptionalReferenceField AsOptionalReference(Func outcome = bindNested(new RequestBinder(nested), _value!); if (outcome.IsFailure) { - _binding.Record(NestedFailure.Group(outcome.Error!, nested.BuiltEnvelope, _argumentPath, _binding.Options.ArgumentInvalid)); + _binding.Record(NestedFailure.Group(outcome.Error!, nested.BuiltEnvelope, ArgumentPath(), _binding.Options.ArgumentInvalid)); return new OptionalReferenceField(_binding, value: null); } @@ -95,7 +95,14 @@ public OptionalReferenceField AsOptionalReference(Func { #region Fields declarations private readonly RequestBinding _binding; - private readonly string _argumentPath; + private readonly object _argumentPathOrProperty; private readonly TArgument? _value; private readonly bool _isMissing; @@ -18,11 +18,15 @@ public sealed class ComplexPropertyEnvelopeStage { #region Constructors declarations - internal ComplexPropertyEnvelopeStage(RequestBinding binding, string argumentPath, TArgument? value, bool isMissing) { - _binding = binding; - _argumentPath = argumentPath; - _value = value; - _isMissing = isMissing; + /// The binding failures are recorded into. + /// The resolved path or the selected property, deferred as in . + /// The nested DTO. + /// Whether the property was absent. + internal ComplexPropertyEnvelopeStage(RequestBinding binding, object argumentPathOrProperty, TArgument? value, bool isMissing) { + _binding = binding; + _argumentPathOrProperty = argumentPathOrProperty; + _value = value; + _isMissing = isMissing; } #endregion @@ -37,7 +41,7 @@ internal ComplexPropertyEnvelopeStage(RequestBinding binding, string argumentPat public ComplexPropertyConverter FailWith(Func envelope) { if (envelope is null) { throw new ArgumentNullException(nameof(envelope)); } - return new ComplexPropertyConverter(_binding, _argumentPath, _value, _isMissing, envelope); + return new ComplexPropertyConverter(_binding, _argumentPathOrProperty, _value, _isMissing, envelope); } } diff --git a/FirstClassErrors.RequestBinder/IElementPathSource.cs b/FirstClassErrors.RequestBinder/IElementPathSource.cs new file mode 100644 index 00000000..7cfe98f2 --- /dev/null +++ b/FirstClassErrors.RequestBinder/IElementPathSource.cs @@ -0,0 +1,18 @@ +namespace FirstClassErrors.RequestBinder; + +/// +/// Builds the indexed argument path of one list element (Tags[2]) on demand. Implemented by the list +/// converters and passed (as this — no allocation) into the shared element iteration and into each +/// element's nested binding, so an element path — and the list path it is built from — only materializes when +/// a path is first needed: a failing element, or, inside a complex element, a nested prefix or an +/// Argument name. A list of simple properties whose every element binds never builds a single path +/// string. +/// +internal interface IElementPathSource { + + /// The full indexed path of the element at (list path + [index]). + /// The element's zero-based position. + /// The element's argument path. + string ElementPathAt(int index); + +} diff --git a/FirstClassErrors.RequestBinder/ListOfComplexPropertiesConverter.cs b/FirstClassErrors.RequestBinder/ListOfComplexPropertiesConverter.cs index 95176b33..642feb77 100644 --- a/FirstClassErrors.RequestBinder/ListOfComplexPropertiesConverter.cs +++ b/FirstClassErrors.RequestBinder/ListOfComplexPropertiesConverter.cs @@ -6,12 +6,12 @@ namespace FirstClassErrors.RequestBinder; /// element never hides the others. /// /// The element type of the DTO list. -public sealed class ListOfComplexPropertiesConverter { +public sealed class ListOfComplexPropertiesConverter : IElementPathSource { #region Fields declarations private readonly RequestBinding _binding; - private readonly string _argumentPath; + private object _argumentPathOrProperty; private readonly IEnumerable? _values; private readonly bool _isMissing; private readonly Func _envelope; @@ -21,19 +21,24 @@ public sealed class ListOfComplexPropertiesConverter { #region Constructors declarations internal ListOfComplexPropertiesConverter(RequestBinding binding, - string argumentPath, + object argumentPathOrProperty, IEnumerable? values, bool isMissing, Func envelope) { - _binding = binding; - _argumentPath = argumentPath; - _values = values; - _isMissing = isMissing; - _envelope = envelope; + _binding = binding; + _argumentPathOrProperty = argumentPathOrProperty; + _values = values; + _isMissing = isMissing; + _envelope = envelope; } #endregion + /// + string IElementPathSource.ElementPathAt(int index) { + return ElementPathAt(index); + } + /// /// Binds a required list: only an absent (null) list records REQUEST_ARGUMENT_REQUIRED — /// a list that is present but empty is valid and binds an empty list, because a required list @@ -48,7 +53,7 @@ public RequiredField> AsRequired(Func>(_binding, default!); } @@ -77,15 +82,26 @@ public RequiredField> AsOptional(Func> BindElements(Func> bindElement) where TProperty : notnull { - return _binding.ConvertEachElement(_argumentPath, _values!, source: null, (element, elementPath) => { - RequestBinding nested = new(_envelope, _binding.Options, elementPath); + return _binding.ConvertEachElement(this, _values!, source: null, (element, index) => { + // The element's binding defers its own prefix ("Guests[2]") through this converter: it materializes + // only when a path inside the element is first needed — a recorded failure, a nested complex prefix, + // or an Argument name — never for an element whose properties all bind through simple converters. + RequestBinding nested = new(_envelope, _binding.Options, this, index); Outcome outcome = bindElement(new RequestBinder(nested), element!); if (outcome.IsFailure) { - _binding.Record(NestedFailure.Group(outcome.Error!, nested.BuiltEnvelope, elementPath, _binding.Options.ArgumentInvalid)); + _binding.Record(NestedFailure.Group(outcome.Error!, nested.BuiltEnvelope, ElementPathAt(index), _binding.Options.ArgumentInvalid)); } return outcome; }); } + private string ElementPathAt(int index) { + return $"{ArgumentPath()}[{index}]"; + } + + private string ArgumentPath() { + return ArgumentPaths.Resolve(ref _argumentPathOrProperty, _binding); + } + } diff --git a/FirstClassErrors.RequestBinder/ListOfComplexPropertiesEnvelopeStage.cs b/FirstClassErrors.RequestBinder/ListOfComplexPropertiesEnvelopeStage.cs index 0a6beac7..a58f4200 100644 --- a/FirstClassErrors.RequestBinder/ListOfComplexPropertiesEnvelopeStage.cs +++ b/FirstClassErrors.RequestBinder/ListOfComplexPropertiesEnvelopeStage.cs @@ -10,7 +10,7 @@ public sealed class ListOfComplexPropertiesEnvelopeStage { #region Fields declarations private readonly RequestBinding _binding; - private readonly string _argumentPath; + private readonly object _argumentPathOrProperty; private readonly IEnumerable? _values; private readonly bool _isMissing; @@ -18,11 +18,15 @@ public sealed class ListOfComplexPropertiesEnvelopeStage { #region Constructors declarations - internal ListOfComplexPropertiesEnvelopeStage(RequestBinding binding, string argumentPath, IEnumerable? values, bool isMissing) { - _binding = binding; - _argumentPath = argumentPath; - _values = values; - _isMissing = isMissing; + /// The binding failures are recorded into. + /// The resolved path or the selected property, deferred as in . + /// The list elements. + /// Whether the property was absent. + internal ListOfComplexPropertiesEnvelopeStage(RequestBinding binding, object argumentPathOrProperty, IEnumerable? values, bool isMissing) { + _binding = binding; + _argumentPathOrProperty = argumentPathOrProperty; + _values = values; + _isMissing = isMissing; } #endregion @@ -37,7 +41,7 @@ internal ListOfComplexPropertiesEnvelopeStage(RequestBinding binding, string arg public ListOfComplexPropertiesConverter FailWith(Func envelope) { if (envelope is null) { throw new ArgumentNullException(nameof(envelope)); } - return new ListOfComplexPropertiesConverter(_binding, _argumentPath, _values, _isMissing, envelope); + return new ListOfComplexPropertiesConverter(_binding, _argumentPathOrProperty, _values, _isMissing, envelope); } } diff --git a/FirstClassErrors.RequestBinder/ListOfSimplePropertiesConverter.cs b/FirstClassErrors.RequestBinder/ListOfSimplePropertiesConverter.cs index 556a4120..c8f313da 100644 --- a/FirstClassErrors.RequestBinder/ListOfSimplePropertiesConverter.cs +++ b/FirstClassErrors.RequestBinder/ListOfSimplePropertiesConverter.cs @@ -6,12 +6,12 @@ namespace FirstClassErrors.RequestBinder; /// one bad element never hides the others. /// /// The raw element type of the list. -public sealed class ListOfSimplePropertiesConverter { +public sealed class ListOfSimplePropertiesConverter : IElementPathSource { #region Fields declarations private readonly RequestBinding _binding; - private readonly string _argumentPath; + private object _argumentPathOrProperty; private readonly IEnumerable? _values; private readonly bool _isMissing; private readonly string? _source; @@ -20,16 +20,21 @@ public sealed class ListOfSimplePropertiesConverter { #region Constructors declarations - internal ListOfSimplePropertiesConverter(RequestBinding binding, string argumentPath, IEnumerable? values, bool isMissing, string? source) { - _binding = binding; - _argumentPath = argumentPath; - _values = values; - _isMissing = isMissing; - _source = source; + internal ListOfSimplePropertiesConverter(RequestBinding binding, object argumentPathOrProperty, IEnumerable? values, bool isMissing, string? source) { + _binding = binding; + _argumentPathOrProperty = argumentPathOrProperty; + _values = values; + _isMissing = isMissing; + _source = source; } #endregion + /// + string IElementPathSource.ElementPathAt(int index) { + return ElementPathAt(index); + } + /// /// Binds a required list: only an absent (null) list records REQUEST_ARGUMENT_REQUIRED — /// a list that is present but empty is valid and binds an empty list, because a required list @@ -44,7 +49,7 @@ public RequiredField> AsRequired(Func>(_binding, default!); } @@ -73,12 +78,20 @@ public RequiredField> AsOptional(Func> ConvertElements(Func> convertElement) where TProperty : notnull { - return _binding.ConvertEachElement(_argumentPath, _values!, _source, (element, elementPath) => { + return _binding.ConvertEachElement(this, _values!, _source, (element, index) => { Outcome outcome = convertElement(element!); - if (outcome.IsFailure) { _binding.RecordArgumentInvalid(elementPath, outcome.Error!, _source); } + if (outcome.IsFailure) { _binding.RecordArgumentInvalid(ElementPathAt(index), outcome.Error!, _source); } return outcome; }); } + private string ElementPathAt(int index) { + return $"{ArgumentPath()}[{index}]"; + } + + private string ArgumentPath() { + return ArgumentPaths.Resolve(ref _argumentPathOrProperty, _binding); + } + } diff --git a/FirstClassErrors.RequestBinder/ListOfSimpleValuePropertiesConverter.cs b/FirstClassErrors.RequestBinder/ListOfSimpleValuePropertiesConverter.cs index 1e7f1c6f..7fdcfb16 100644 --- a/FirstClassErrors.RequestBinder/ListOfSimpleValuePropertiesConverter.cs +++ b/FirstClassErrors.RequestBinder/ListOfSimpleValuePropertiesConverter.cs @@ -12,12 +12,12 @@ namespace FirstClassErrors.RequestBinder; /// is unwrapped (element.Value) before conversion. /// /// The underlying (non-nullable) value type of the list elements. -public sealed class ListOfSimpleValuePropertiesConverter where TArgument : struct { +public sealed class ListOfSimpleValuePropertiesConverter : IElementPathSource where TArgument : struct { #region Fields declarations private readonly RequestBinding _binding; - private readonly string _argumentPath; + private object _argumentPathOrProperty; private readonly IEnumerable? _values; private readonly bool _isMissing; private readonly string? _source; @@ -26,16 +26,21 @@ public sealed class ListOfSimpleValuePropertiesConverter where TArgum #region Constructors declarations - internal ListOfSimpleValuePropertiesConverter(RequestBinding binding, string argumentPath, IEnumerable? values, bool isMissing, string? source) { - _binding = binding; - _argumentPath = argumentPath; - _values = values; - _isMissing = isMissing; - _source = source; + internal ListOfSimpleValuePropertiesConverter(RequestBinding binding, object argumentPathOrProperty, IEnumerable? values, bool isMissing, string? source) { + _binding = binding; + _argumentPathOrProperty = argumentPathOrProperty; + _values = values; + _isMissing = isMissing; + _source = source; } #endregion + /// + string IElementPathSource.ElementPathAt(int index) { + return ElementPathAt(index); + } + /// /// Binds a required list: only an absent (null) list records REQUEST_ARGUMENT_REQUIRED — /// a list that is present but empty is valid and binds an empty list, because a required list @@ -51,7 +56,7 @@ public RequiredField> AsRequired(Func>(_binding, default!); } @@ -80,12 +85,20 @@ public RequiredField> AsOptional(Func> ConvertElements(Func> convertElement) where TProperty : notnull { - return _binding.ConvertEachElement(_argumentPath, _values!, _source, (element, elementPath) => { + return _binding.ConvertEachElement(this, _values!, _source, (element, index) => { Outcome outcome = convertElement(element!.Value); - if (outcome.IsFailure) { _binding.RecordArgumentInvalid(elementPath, outcome.Error!, _source); } + if (outcome.IsFailure) { _binding.RecordArgumentInvalid(ElementPathAt(index), outcome.Error!, _source); } return outcome; }); } + private string ElementPathAt(int index) { + return $"{ArgumentPath()}[{index}]"; + } + + private string ArgumentPath() { + return ArgumentPaths.Resolve(ref _argumentPathOrProperty, _binding); + } + } diff --git a/FirstClassErrors.RequestBinder/PropertyGetters.cs b/FirstClassErrors.RequestBinder/PropertyGetters.cs new file mode 100644 index 00000000..3fe35126 --- /dev/null +++ b/FirstClassErrors.RequestBinder/PropertyGetters.cs @@ -0,0 +1,118 @@ +#region Usings declarations + +using System.Collections.Concurrent; +using System.Linq.Expressions; +using System.Reflection; + +#endregion + +namespace FirstClassErrors.RequestBinder; + +/// +/// Process-wide cache of compiled, typed property getters — one per bound (DTO type, property). The reflection +/// cost of reading a DTO property (a invoke, and its boxing of +/// value-type values) is paid once, at the first bind of a property, and every later bind of the same property +/// reuses the compiled delegate: a direct, allocation-free property read. +/// +/// +/// +/// The cache is keyed by the closed generic type (one dictionary per (DTO type, selected value type) pair) and +/// by within it, so two DTO types sharing a property name, or one property selected +/// under two different static types, can never collide. It is deliberately options-independent: a getter +/// depends only on the property, never on a binder's — argument names and +/// paths stay per-binding (see ADR-0012: options are fixed before binding begins, per binder). +/// +/// +/// The mis-declaration guard (a non-nullable value-type property, whose absence the binder cannot detect) +/// lives in the compile step: an invalid property is never cached, so the guard throws on every call — +/// eagerly, at selector time, exactly as before this cache existed. A cache hit implies a valid property. +/// +/// +/// Thread-safety: bindings are single-threaded by contract, but this cache is process-wide and safe — a +/// concurrent first bind of the same property may compile twice, and either delegate wins, both correct. +/// Statics of a generic instantiation live with the DTO type's loader context, so an unloadable consumer +/// assembly is not pinned by the cache. +/// +/// +/// The type of the request DTO the property is read from. +/// The static type the selector yields (the property type, or a base type of it). +internal static class PropertyGetters { + + #region Statics members declarations + + // A static of THIS closed generic, not a single map shared across DTO types: the CLR keeps a closed generic's + // statics in the DTO type's own loader context, so a collectible consumer assembly unloads together with its + // cache entries. Replacing this with one binder-wide dictionary would pin every consumer assembly for the + // process lifetime — the per-closed-generic layout is load-bearing, not stylistic. + private static readonly ConcurrentDictionary> Cache = new(); + + /// + /// The compiled getter of — from the cache, or compiled and cached on first + /// use. + /// + /// The DTO property to read. + /// The compiled, typed getter. + /// + /// Thrown — on every call, an invalid property is never cached — when is a + /// non-nullable value type: a missing value would be indistinguishable from its default. + /// + internal static Func For(PropertyInfo property) { + if (Cache.TryGetValue(property, out Func? getter)) { return getter; } + + return Cache.GetOrAdd(property, CompileValidated); + } + + private static Func CompileValidated(PropertyInfo property) { + // A non-nullable value-type property can never be null, so a missing argument (deserialized to default(T) — + // 0, false, ...) is indistinguishable from a legitimately-sent default: absence would be silently lost. The + // information does not exist at runtime, so reject the mis-declaration loudly (the binder's programming-error + // channel) — the DTO property must be declared nullable so that an absent argument arrives as null. The + // parameter name is the selector every PropertySource method receives. + if (property.PropertyType.IsValueType && Nullable.GetUnderlyingType(property.PropertyType) is null) { + throw new ArgumentException( + $"The request property '{property.Name}' is a non-nullable value type ({property.PropertyType.Name}); a missing value would be indistinguishable from its default. Declare it as {property.PropertyType.Name}? so the binder can detect an absent argument.", + "selector"); + } + + // A canonical tree is compiled from the PropertyInfo instead of compiling the caller's selector: the caller's + // tree may carry a Convert node and cannot be shared across call sites anyway. When the static selector type + // differs from the property type, the value is routed THROUGH OBJECT because that is exactly the conversion + // the reflection-based reader applied (a cast on the boxed result of GetValue): a widening the compiler emits + // (List selected as IEnumerable) still succeeds, and a hand-built selector over an incompatible value + // type still fails with the same InvalidCastException, instead of silently gaining C#'s numeric/user-defined + // conversions. For a nullable value-type selector the boxed value is unboxed to the UNDERLYING type before + // the lift — `(TValue?)(TValue)(object)value` — mirroring the pre-cache cast, whose unbox accepted the CLR's + // boxed-enum/underlying compatibility that a direct unbox to Nullable<> rejects. + ParameterExpression dto = Expression.Parameter(typeof(TDto), "dto"); + Expression body = Expression.Property(dto, property); + if (body.Type != typeof(TValue)) { + Type? liftedUnderlying = Nullable.GetUnderlyingType(typeof(TValue)); + if (liftedUnderlying is null) { + body = Expression.Convert(Expression.Convert(body, typeof(object)), typeof(TValue)); + } else { + ParameterExpression boxed = Expression.Variable(typeof(object), "boxed"); + body = Expression.Block( + new[] { boxed }, + Expression.Assign(boxed, Expression.Convert(body, typeof(object))), + Expression.Condition( + Expression.ReferenceEqual(boxed, Expression.Constant(null, typeof(object))), + Expression.Default(typeof(TValue)), + Expression.Convert(Expression.Convert(boxed, liftedUnderlying), typeof(TValue)))); + } + } + + try { + return Expression.Lambda>(body, dto).Compile(); + } catch (Exception exception) when (exception is NotSupportedException or PlatformNotSupportedException) { + // Ahead-of-time targets without IL emission fall back to the interpreter inside Compile(); the rare + // environment where even that fails keeps the pre-cache behavior: a reflection read whose result is + // cast, boxing included. Known, accepted divergence in this last-resort path only: a coercion selector + // between an enum and its underlying type unbox-fails here, because the generic cast targets TValue + // directly. + return dtoInstance => (TValue)property.GetValue(dtoInstance)!; + } + } + + #endregion + +} diff --git a/FirstClassErrors.RequestBinder/PropertySource.cs b/FirstClassErrors.RequestBinder/PropertySource.cs index a15e75c4..e7780d51 100644 --- a/FirstClassErrors.RequestBinder/PropertySource.cs +++ b/FirstClassErrors.RequestBinder/PropertySource.cs @@ -45,9 +45,9 @@ internal PropertySource(RequestBinding binding, TDto dto) { /// nullable (e.g. int?) for the binder to detect an absent argument. /// public SimplePropertyConverter SimpleProperty(Expression> selector) { - (string path, object? value) = ResolveArgument(selector); + (PropertyInfo property, TArgument? value) = ResolveArgument(selector); - return new SimplePropertyConverter(_binding, path, (TArgument?)value, value is null, source: null); + return new SimplePropertyConverter(_binding, property, value, value is null, source: null); } /// @@ -65,9 +65,9 @@ public SimplePropertyConverter SimpleProperty(ExpressionA direct property access on a nullable value-type property (e.g. d => d.MaxNights). /// The converter stage offering AsRequired / AsOptional and their variants. public SimplePropertyConverter SimpleProperty(Expression> selector) where TArgument : struct { - (string path, object? value) = ResolveArgument(selector); + (PropertyInfo property, TArgument? value) = ResolveArgument(selector); - return new SimplePropertyConverter(_binding, path, value is null ? default : (TArgument)value, value is null, source: null); + return new SimplePropertyConverter(_binding, property, value.GetValueOrDefault(), !value.HasValue, source: null); } /// @@ -78,9 +78,9 @@ public SimplePropertyConverter SimpleProperty(ExpressionA direct property access on the DTO (e.g. d => d.Stay). /// The stage on which the nested envelope is declared. public ComplexPropertyEnvelopeStage ComplexProperty(Expression> selector) { - (string path, object? value) = ResolveArgument(selector); + (PropertyInfo property, TArgument? value) = ResolveArgument(selector); - return new ComplexPropertyEnvelopeStage(_binding, path, (TArgument?)value, value is null); + return new ComplexPropertyEnvelopeStage(_binding, property, value, value is null); } /// @@ -90,9 +90,9 @@ public ComplexPropertyEnvelopeStage ComplexProperty(Expres /// A direct property access on the DTO (e.g. d => d.Tags). /// The converter stage offering AsRequired / AsOptional. public ListOfSimplePropertiesConverter ListOfSimpleProperties(Expression?>> selector) { - (string path, object? value) = ResolveArgument(selector); + (PropertyInfo property, IEnumerable? value) = ResolveArgument(selector); - return new ListOfSimplePropertiesConverter(_binding, path, (IEnumerable?)value, value is null, source: null); + return new ListOfSimplePropertiesConverter(_binding, property, value, value is null, source: null); } /// @@ -108,9 +108,9 @@ public ListOfSimplePropertiesConverter ListOfSimplePropertiesA direct property access on a list of nullable value types (e.g. d => d.Quantities). /// The converter stage offering AsRequired / AsOptional. public ListOfSimpleValuePropertiesConverter ListOfSimpleProperties(Expression?>> selector) where TArgument : struct { - (string path, object? value) = ResolveArgument(selector); + (PropertyInfo property, IEnumerable? value) = ResolveArgument(selector); - return new ListOfSimpleValuePropertiesConverter(_binding, path, (IEnumerable?)value, value is null, source: null); + return new ListOfSimpleValuePropertiesConverter(_binding, property, value, value is null, source: null); } /// @@ -121,27 +121,23 @@ public ListOfSimpleValuePropertiesConverter ListOfSimplePropertiesA direct property access on the DTO (e.g. d => d.Guests). /// The stage on which the per-element envelope is declared. public ListOfComplexPropertiesEnvelopeStage ListOfComplexProperties(Expression?>> selector) { - (string path, object? value) = ResolveArgument(selector); + (PropertyInfo property, IEnumerable? value) = ResolveArgument(selector); - return new ListOfComplexPropertiesEnvelopeStage(_binding, path, (IEnumerable?)value, value is null); + return new ListOfComplexPropertiesEnvelopeStage(_binding, property, value, value is null); } - private (string Path, object? Value) ResolveArgument(Expression> selector) { + /// + /// Resolves the selected property and reads its value through a cached, compiled getter (see + /// — the mis-declaration guard for a non-nullable value-type + /// property lives there, still thrown eagerly on every call). The argument path is not built here: + /// the converter stage carries the and only materializes the path when one is + /// first needed — a recorded failure, or a complex property's nested prefix — so an all-valid bind of + /// scalar and list properties never pays for it. + /// + private (PropertyInfo Property, TArgument Value) ResolveArgument(Expression> selector) { PropertyInfo property = PropertySelectors.GetProperty(selector); - // A non-nullable value-type property can never be null, so a missing argument (deserialized to default(T) — - // 0, false, ...) is indistinguishable from a legitimately-sent default: absence would be silently lost. The - // information does not exist at runtime, so reject the mis-declaration loudly (the binder's programming-error - // channel) — the DTO property must be declared nullable so that an absent argument arrives as null. - if (property.PropertyType.IsValueType && Nullable.GetUnderlyingType(property.PropertyType) is null) { - throw new ArgumentException( - $"The request property '{property.Name}' is a non-nullable value type ({property.PropertyType.Name}); a missing value would be indistinguishable from its default. Declare it as {property.PropertyType.Name}? so the binder can detect an absent argument.", - nameof(selector)); - } - - string path = _binding.PathOf(_binding.Options.ArgumentNameProvider.GetArgumentNameFrom(property)); - - return (path, property.GetValue(_dto)); + return (property, PropertyGetters.For(property)(_dto)); } } diff --git a/FirstClassErrors.RequestBinder/RequestBinding.cs b/FirstClassErrors.RequestBinder/RequestBinding.cs index c6d60bef..c699b8f0 100644 --- a/FirstClassErrors.RequestBinder/RequestBinding.cs +++ b/FirstClassErrors.RequestBinder/RequestBinding.cs @@ -1,3 +1,9 @@ +#region Usings declarations + +using System.Reflection; + +#endregion + namespace FirstClassErrors.RequestBinder; /// @@ -16,19 +22,35 @@ internal sealed class RequestBinding { #region Fields declarations private readonly Func _envelope; - private readonly string? _argumentPrefix; - private readonly List _errors = new(); + private readonly IElementPathSource? _prefixSource; + private readonly int _prefixElementIndex; + private string? _argumentPrefix; + private List? _errors; #endregion #region Constructors declarations + /// Creates a binding with a known prefix: null for a top-level binding, or the already-resolved path of the complex property a nested binding binds under. internal RequestBinding(Func envelope, RequestBinderOptions options, string? argumentPrefix) { _envelope = envelope; Options = options; _argumentPrefix = argumentPrefix; } + /// + /// Creates the binding of one list element, whose prefix (Guests[2]) is built only if something + /// inside the element ever needs a path — a recorded failure, a nested complex prefix, or an + /// name. An element whose properties all bind through simple + /// converters never materializes it. + /// + internal RequestBinding(Func envelope, RequestBinderOptions options, IElementPathSource prefixSource, int prefixElementIndex) { + _envelope = envelope; + Options = options; + _prefixSource = prefixSource; + _prefixElementIndex = prefixElementIndex; + } + #endregion /// The options this binding (and every binding nested under it) binds with; fixed before binding begins. @@ -43,11 +65,13 @@ internal RequestBinding(Func envelope, internal PrimaryPortError? BuiltEnvelope { get; private set; } /// Whether any binding failure has been recorded; a build terminal assembles only when this is false. - internal bool HasErrors => _errors.Count > 0; + internal bool HasErrors => _errors is { Count: > 0 }; /// Records a binding failure; it will surface in the envelope built by a failing build terminal. internal void Record(PrimaryPortError error) { - _errors.Add(error); + // Created on first failure only: an all-valid binding — including every nested and per-element binding — + // never allocates its error list. + (_errors ??= new List()).Add(error); } /// @@ -73,34 +97,38 @@ internal void RecordArgumentInvalid(string argumentPath, Error cause, string? so /// element never hides the others: a null element records the required-argument failure, and /// binds each non-null element — recording its own failure (a converter /// error, or a nested envelope wrapped under the path) and yielding its value on success. The list converters - /// share this single iteration and null-element rule, so a change to either is made in one place. + /// share this single element rule, so a change to it is made in one place; an element's indexed path is built + /// through only when a failure is recorded, never for a valid element. /// /// The element type as stored (a reference or a ). /// The type each element binds to. - /// The list's argument path; each element is reported under argumentPath[index]. + /// Builds an element's indexed path on demand (the list converter itself). /// The list elements. /// The provenance of an out-of-DTO argument list (null for a DTO property). - /// Binds a non-null element at its indexed path, recording its own failure. + /// Binds a non-null element at its index, recording its own failure. /// The bound field token carrying the successfully bound elements. internal RequiredField> ConvertEachElement( - string argumentPath, IEnumerable values, string? source, Func> convertElementAt) where TProperty : notnull { - List converted = new(); - int index = 0; - + IElementPathSource elementPaths, IEnumerable values, string? source, Func> convertElementAt) where TProperty : notnull { + // Pre-sized when the element count is known, so an all-valid list fills without growth garbage. The walk + // itself deliberately stays a plain foreach: an indexed fast path would swap enumeration for indexer calls + // on caller-owned collections — silencing, for example, the List version check that loudly reports a + // converter mutating the list it is binding. + List converted = values is ICollection sized ? new List(sized.Count) : new List(); + + int index = 0; foreach (TStored element in values) { - string elementPath = $"{argumentPath}[{index}]"; - index++; - if (element is null) { - RecordArgumentRequired(elementPath, source); + RecordArgumentRequired(elementPaths.ElementPathAt(index), source); + index++; continue; } - Outcome outcome = convertElementAt(element, elementPath); + Outcome outcome = convertElementAt(element, index); if (outcome.IsSuccess) { converted.Add(outcome.GetResultOrThrow()); } // A failing element was already recorded by convertElementAt (REQUEST_ARGUMENT_INVALID, or the nested // envelope wrapped under the indexed path), so it is simply skipped here. + index++; } // The value is read only through a BindingScope, which a build terminal creates solely when no failure was @@ -110,13 +138,23 @@ internal RequiredField> ConvertEachElementPrepends this binding's argument prefix to a path segment ("CheckIn" -> "Stay.CheckIn"). internal string PathOf(string argumentName) { - return _argumentPrefix is null ? argumentName : $"{_argumentPrefix}.{argumentName}"; + // An element binding resolves its prefix ("Guests[2]") on first use — reached only when a failure inside + // the element records a path; a top-level binding has none and a nested binding's was resolved up front. + string? prefix = _argumentPrefix ??= _prefixSource?.ElementPathAt(_prefixElementIndex); + + return prefix is null ? argumentName : $"{prefix}.{argumentName}"; + } + + /// The full path of a DTO property: its argument name (per the configured provider), under this binding's prefix. + internal string PathOfProperty(PropertyInfo property) { + return PathOf(Options.ArgumentNameProvider.GetArgumentNameFrom(property)); } /// Builds and caches the envelope grouping every recorded failure; the cached instance disambiguates a nested failure by reference. internal PrimaryPortError BuildFailureEnvelope() { PrimaryPortInnerErrors innerErrors = new(); - foreach (PrimaryPortError error in _errors) { + // Only reached by a failing build terminal, i.e. when HasErrors — the list necessarily exists. + foreach (PrimaryPortError error in _errors!) { innerErrors.Add(error); } diff --git a/FirstClassErrors.RequestBinder/SimplePropertyConverter.cs b/FirstClassErrors.RequestBinder/SimplePropertyConverter.cs index e9ba5c2d..a9a29a9a 100644 --- a/FirstClassErrors.RequestBinder/SimplePropertyConverter.cs +++ b/FirstClassErrors.RequestBinder/SimplePropertyConverter.cs @@ -15,7 +15,7 @@ public sealed class SimplePropertyConverter { #region Fields declarations private readonly RequestBinding _binding; - private readonly string _argumentPath; + private object _argumentPathOrProperty; private readonly TArgument? _value; private readonly bool _isMissing; private readonly string? _source; @@ -24,12 +24,21 @@ public sealed class SimplePropertyConverter { #region Constructors declarations - internal SimplePropertyConverter(RequestBinding binding, string argumentPath, TArgument? value, bool isMissing, string? source) { - _binding = binding; - _argumentPath = argumentPath; - _value = value; - _isMissing = isMissing; - _source = source; + /// The binding failures are recorded into. + /// + /// The resolved path (, an out-of-DTO argument) or the selected property + /// (), whose path is built only when a failure is recorded — + /// see . + /// + /// The raw value. + /// Whether the input was absent. + /// The provenance of an out-of-DTO argument; null for a DTO property. + internal SimplePropertyConverter(RequestBinding binding, object argumentPathOrProperty, TArgument? value, bool isMissing, string? source) { + _binding = binding; + _argumentPathOrProperty = argumentPathOrProperty; + _value = value; + _isMissing = isMissing; + _source = source; } #endregion @@ -56,7 +65,7 @@ public RequiredField AsRequired(FuncThe bound field token. public RequiredField AsRequired() { if (_isMissing) { - _binding.RecordArgumentRequired(_argumentPath, _source); + _binding.RecordArgumentRequired(ArgumentPath(), _source); return new RequiredField(_binding, default!); } @@ -86,7 +95,7 @@ public RequiredField AsOptional(Func fallback = convert(rawFallback); if (fallback.IsFailure) { throw new InvalidOperationException( - $"The configured fallback of optional argument '{_argumentPath}' does not convert: {fallback.Error!.DiagnosticMessage}"); + $"The configured fallback of optional argument '{ArgumentPath()}' does not convert: {fallback.Error!.DiagnosticMessage}"); } return new RequiredField(_binding, fallback.GetResultOrThrow()); @@ -142,7 +151,7 @@ public OptionalValueField AsOptionalValue(Func RequiredMissing() { - _binding.RecordArgumentRequired(_argumentPath, _source); + _binding.RecordArgumentRequired(ArgumentPath(), _source); return new RequiredField(_binding, default!); } @@ -158,7 +167,11 @@ private RequiredField RecordIfInvalid(Outcome o } private void RecordInvalid(Error cause) { - _binding.RecordArgumentInvalid(_argumentPath, cause, _source); + _binding.RecordArgumentInvalid(ArgumentPath(), cause, _source); + } + + private string ArgumentPath() { + return ArgumentPaths.Resolve(ref _argumentPathOrProperty, _binding); } } diff --git a/FirstClassErrors.sln b/FirstClassErrors.sln index 1204a08d..f28e430c 100644 --- a/FirstClassErrors.sln +++ b/FirstClassErrors.sln @@ -55,6 +55,8 @@ Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "FirstClassErrors.RequestBin EndProject Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "FirstClassErrors.RequestBinder.Usage.UnitTests", "FirstClassErrors.RequestBinder.Usage.UnitTests\FirstClassErrors.RequestBinder.Usage.UnitTests.csproj", "{04707ACA-FB2E-43BF-A12C-28E01BF5F60D}" EndProject +Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "FirstClassErrors.RequestBinder.Benchmarks", "FirstClassErrors.RequestBinder.Benchmarks\FirstClassErrors.RequestBinder.Benchmarks.csproj", "{1201EA34-8F50-41B8-B829-FCCB62A5F14B}" +EndProject Global GlobalSection(SolutionConfigurationPlatforms) = preSolution Debug|Any CPU = Debug|Any CPU @@ -89,6 +91,18 @@ Global {04707ACA-FB2E-43BF-A12C-28E01BF5F60D}.Release|x64.Build.0 = Release|Any CPU {04707ACA-FB2E-43BF-A12C-28E01BF5F60D}.Release|x86.ActiveCfg = Release|Any CPU {04707ACA-FB2E-43BF-A12C-28E01BF5F60D}.Release|x86.Build.0 = Release|Any CPU + {1201EA34-8F50-41B8-B829-FCCB62A5F14B}.Debug|Any CPU.ActiveCfg = Debug|Any CPU + {1201EA34-8F50-41B8-B829-FCCB62A5F14B}.Debug|Any CPU.Build.0 = Debug|Any CPU + {1201EA34-8F50-41B8-B829-FCCB62A5F14B}.Debug|x64.ActiveCfg = Debug|Any CPU + {1201EA34-8F50-41B8-B829-FCCB62A5F14B}.Debug|x64.Build.0 = Debug|Any CPU + {1201EA34-8F50-41B8-B829-FCCB62A5F14B}.Debug|x86.ActiveCfg = Debug|Any CPU + {1201EA34-8F50-41B8-B829-FCCB62A5F14B}.Debug|x86.Build.0 = Debug|Any CPU + {1201EA34-8F50-41B8-B829-FCCB62A5F14B}.Release|Any CPU.ActiveCfg = Release|Any CPU + {1201EA34-8F50-41B8-B829-FCCB62A5F14B}.Release|Any CPU.Build.0 = Release|Any CPU + {1201EA34-8F50-41B8-B829-FCCB62A5F14B}.Release|x64.ActiveCfg = Release|Any CPU + {1201EA34-8F50-41B8-B829-FCCB62A5F14B}.Release|x64.Build.0 = Release|Any CPU + {1201EA34-8F50-41B8-B829-FCCB62A5F14B}.Release|x86.ActiveCfg = Release|Any CPU + {1201EA34-8F50-41B8-B829-FCCB62A5F14B}.Release|x86.Build.0 = Release|Any CPU {8DA5E330-53BC-4EA3-A9B8-916DABA3878C}.Debug|Any CPU.ActiveCfg = Debug|Any CPU {8DA5E330-53BC-4EA3-A9B8-916DABA3878C}.Debug|Any CPU.Build.0 = Debug|Any CPU {8DA5E330-53BC-4EA3-A9B8-916DABA3878C}.Debug|x64.ActiveCfg = Debug|Any CPU @@ -326,6 +340,7 @@ Global {1FC2C501-8842-4ABE-845E-000ED6575DD6} = {B7C3D08D-EFC5-4F5D-8DE4-5B7938354DBB} {D12EE45B-78F8-4CD4-8E67-E4A9DDA01AE6} = {B7C3D08D-EFC5-4F5D-8DE4-5B7938354DBB} {04707ACA-FB2E-43BF-A12C-28E01BF5F60D} = {B7C3D08D-EFC5-4F5D-8DE4-5B7938354DBB} + {1201EA34-8F50-41B8-B829-FCCB62A5F14B} = {B7C3D08D-EFC5-4F5D-8DE4-5B7938354DBB} EndGlobalSection GlobalSection(ExtensibilityGlobals) = postSolution SolutionGuid = {4988972E-3E0D-4F48-8656-0E67ECE994BF} diff --git a/doc/handwritten/for-maintainers/adr/0023-keep-expression-tree-selectors-for-the-v1-binder-api.fr.md b/doc/handwritten/for-maintainers/adr/0023-keep-expression-tree-selectors-for-the-v1-binder-api.fr.md new file mode 100644 index 00000000..55dad994 --- /dev/null +++ b/doc/handwritten/for-maintainers/adr/0023-keep-expression-tree-selectors-for-the-v1-binder-api.fr.md @@ -0,0 +1,155 @@ +# ADR-0023 | Conserver les sélecteurs à arbres d'expression pour l'API v1 du binder + +🌍 🇬🇧 [English](0023-keep-expression-tree-selectors-for-the-v1-binder-api.md) · 🇫🇷 Français (ce fichier) + +**Statut :** Accepté +**Date :** 2026-07-19 +**Décideurs :** Reefact + +## Contexte + +Le binder de requêtes sélectionne les propriétés des DTO au travers de +sélecteurs à arbres d'expression (`r => r.GuestEmail`), dont il dérive à la +fois la valeur et le nom d'argument derrière chaque chemin d'erreur. +L'issue #151 (constat de revue 15/19) mesurait un coût du chemin nominal +d'environ 2,2–2,4 µs et ~700 o alloués par propriété liée, et conditionnait +le remède à une décision à prendre **avant le gel de l'API v1**. + +La re-mesure sur le code actuel (harnais BenchmarkDotNet dans +`FirstClassErrors.RequestBinder.Benchmarks`, dont le README porte les tables +complètes) a décomposé ce coût : + +* **~70–75 % du coût est l'arbre d'expression lui-même**, alloué par le + compilateur C# *au site d'appel de l'appelant* à chaque exécution — + ~488 o et ~416 ns par propriété. Roslyn met en cache les lambdas + déléguées mais jamais les lambdas d'arbres d'expression, et aucun + changement à l'intérieur de la librairie ne peut supprimer une allocation + qui se produit avant l'entrée dans la librairie. +* Le reste était interne à la librairie — lectures par réflexion non mises + en cache, boxing des types valeur nullables, chaînes de chemin + construites d'avance pour les éléments de liste et les préfixes + imbriqués — et a été éliminé par la mise en cache de getters compilés et + le report de la construction des chemins au chemin d'échec + (l'implémentation de l'issue #151). Ce qui reste à l'intérieur de la + librairie est la forme objet-par-propriété de l'API fluent elle-même. +* Le même benchmark montre que le coût du sélecteur est déjà évitable + **avec l'API actuelle** : des sélecteurs hissés dans des champs statiques + lient 5 propriétés en ~342 ns / 880 o au lieu de ~2 805 ns / 3 600 o, et + un sélecteur délégué mis en cache par le compilateur coûterait + ~1 ns / 0 o. +* Un site d'appel du binder lie un DTO une fois par requête à la frontière + de l'adaptateur primaire ; le travail environnant (désérialisation, E/S, + l'appel du domaine) se mesure en dizaines à centaines de µs. + +Deux remèdes au niveau de l'API existent : des surcharges prenant un nom et +un délégué simple (`SimpleProperty("GuestEmail", d => d.GuestEmail)`), +additives et non cassantes, ou le remplacement pur et simple de l'API à +expressions, qui est cassant. Chaque surcharge de sélecteur existe six fois +(scalaire et liste, référence et type valeur, complexe), si bien qu'une +famille déléguée parallèle double à peu près la surface des sélecteurs et +crée deux façons idiomatiques d'écrire chaque liaison. + +## Décision + +Le binder v1 sélectionne les propriétés des DTO exclusivement au travers de +sélecteurs à arbres d'expression ; le coût d'expression par appel est +accepté pour la v1, et tout chemin rapide à base de délégués est reporté à +une décision post-v1, additive. + +## Justification + +Le coût absolu mesuré — quelques µs par requête — est négligeable face aux +dizaines à centaines de µs qu'une requête passe à la frontière qu'elle lie ; +le coût par propriété ne compte que dans des formes atypiques (DTO très +larges sur des endpoints très chauds), et ces appelants disposent déjà d'une +échappatoire sans rupture : hisser les sélecteurs dans des champs statiques, +ce qui supprime ~85–90 % du temps et ~75 % de l'allocation avec l'API +exactement telle qu'elle est. + +Un idiome de sélection unique vaut davantage pour la v1 que les +nanosecondes résiduelles : une famille déléguée+nom dupliquée doublerait une +surface de six surcharges, fragmenterait le style des sites d'appel entre +bases de code, et réintroduirait le nommage de propriétés par chaînes que +l'API à expressions existe précisément pour empêcher — son point d'entrée +unique est ce qui permet au binder de dériver la valeur, le nom et la garde +de mauvaise déclaration d'une seule construction. Une famille déléguée étant +purement additive, la reporter ne coûte rien : elle peut être livrée dans +n'importe quelle mineure post-v1 si le profilage de consommateurs réels +montre un jour que l'arbre au site d'appel compte, alors que la livrer +maintenant est un engagement de surface que la v1 porterait pour toujours. + +## Alternatives considérées + +### Ajouter dès maintenant des surcharges délégué+nom + +Considérée parce que c'est la seule façon d'atteindre le plancher de ~0 o +par sélecteur (délégués mis en cache par le compilateur) sans rien casser. +Rejetée pour la v1 : elle double la surface des sélecteurs, +institutionnalise deux façons de lier avant la première version stable, et +optimise un coût que le hissage permet déjà aux appelants chauds d'éliminer +aujourd'hui. + +### Remplacer l'API à expressions par délégué+nom + +Considérée pour le même plancher avec un idiome unique. Rejetée : c'est une +refonte cassante de la construction centrale du binder, elle ramène le +nommage des arguments à des chaînes, et contredit le contrat de sélecteur +vérifié à la compilation sur lequel s'appuient les analyseurs et les gardes. + +### Générer les sélecteurs à la compilation (générateur de source) + +Considérée comme la voie de long terme vers l'ergonomie des expressions au +coût des délégués. Rejetée pour la v1 comme périmètre, non sur le fond : +c'est un nouveau composant avec sa propre surface de compatibilité ; la +base d'ADR l'enregistre comme le successeur naturel si le coût résiduel +venait à compter. + +## Conséquences + +### Positives + +* Un seul idiome de sélection en v1 ; la surface de six surcharges reste en + l'état. +* Les optimisations internes de l'issue #151 tiennent par elles-mêmes : les + chemins et la réflexion ne coûtent plus rien sur une liaison entièrement + valide, quelle que soit l'évolution ultérieure de la décision sur les + sélecteurs. +* Les appelants chauds disposent d'une atténuation documentée et mesurée + (sélecteurs hissés) qui n'exige aucun changement de la librairie. + +### Négatives + +* Le style par défaut au site d'appel continue de payer ~488 o / ~416 ns + par propriété liée pour l'arbre d'expression. + +### Risques + +* Si l'adoption de la v1 révèle des charges où le coût du sélecteur domine + en pratique, le remède est additif (surcharges déléguées ou générateur de + source) — le risque se borne à porter le successeur de cet ADR, jamais à + un changement cassant. + +## Actions de suivi + +* Documenter l'atténuation par sélecteurs hissés dans le guide utilisateur + du binder dès que le sujet se présentera chez les consommateurs. +* Relancer `FirstClassErrors.RequestBinder.Benchmarks` au prochain + changement de la surface des sélecteurs du binder, et revisiter cet ADR + chiffres en main. + +## Références + +* Issue #151 — Binder : la liaison par propriété utilise une réflexion non + mise en cache et alloue une chaîne de chemin sur le chemin nominal + (constat de revue 15/19). +* `FirstClassErrors.RequestBinder.Benchmarks/README.md` — harnais de + mesure, tables avant/après complètes et décomposition du coût. +* [ADR-0008](0008-bind-nullable-value-type-properties-through-a-struct-constrained-overload.fr.md) — + la famille de surcharges contrainte `struct` sur laquelle la surface des + sélecteurs est bâtie. +* [ADR-0021](0021-bind-out-of-dto-arguments-as-peers-through-a-source-agnostic-entry.fr.md) — + l'entrée hors-DTO, dont le chemin par nom est la forme non-expression + existante du binder. +* [ADR-0022](0022-floor-the-library-on-net-framework-4-7-2.fr.md) — le + floor .NET Framework 4.7.2 contre lequel le cache de getters compilés a + été vérifié. diff --git a/doc/handwritten/for-maintainers/adr/0023-keep-expression-tree-selectors-for-the-v1-binder-api.md b/doc/handwritten/for-maintainers/adr/0023-keep-expression-tree-selectors-for-the-v1-binder-api.md new file mode 100644 index 00000000..70149ef7 --- /dev/null +++ b/doc/handwritten/for-maintainers/adr/0023-keep-expression-tree-selectors-for-the-v1-binder-api.md @@ -0,0 +1,138 @@ +# ADR-0023 | Keep expression-tree selectors for the v1 binder API + +🌍 🇬🇧 English (this file) · 🇫🇷 [Français](0023-keep-expression-tree-selectors-for-the-v1-binder-api.fr.md) + +**Status:** Accepted +**Date:** 2026-07-19 +**Decision Makers:** Reefact + +## Context + +The request binder selects DTO properties through expression-tree selectors +(`r => r.GuestEmail`), from which it derives both the value and the argument +name behind every error path. Issue #151 (review finding 15/19) measured a +happy-path cost of roughly 2.2–2.4 µs and ~700 B allocated per bound property +and gated the remedy on a decision to take **before the v1 API freeze**. + +Re-measurement on the current code (BenchmarkDotNet harness in +`FirstClassErrors.RequestBinder.Benchmarks`, whose README carries the full +tables) decomposed that cost: + +* **~70–75 % of it is the expression tree itself**, allocated by the C# + compiler *at the caller's call site* on every execution — ~488 B and + ~416 ns per property. Roslyn caches delegate lambdas but never + expression-tree lambdas, and no change inside the library can remove an + allocation that happens before the library is entered. +* The remainder was library-internal — uncached reflection reads, boxing of + nullable value types, eager path strings for list elements and nested + prefixes — and has been eliminated by caching compiled getters and + deferring path construction to the failure path (issue #151's + implementation). What is left inside the library is the fluent API's own + object-per-property shape. +* The same benchmark shows the selector cost is already avoidable **with the + current API**: selectors hoisted into static fields bind 5 properties in + ~342 ns / 880 B instead of ~2 805 ns / 3 600 B, and a compiler-cached + delegate selector would cost ~1 ns / 0 B. +* A binder call site binds a DTO once per request at the primary-adapter + boundary; the surrounding work (deserialization, I/O, the domain call) + is measured in tens to hundreds of µs. + +Two API-level remedies exist: overloads taking a name plus a plain delegate +(`SimpleProperty("GuestEmail", d => d.GuestEmail)`), which are additive and +non-breaking, or replacing the expression API outright, which is breaking. +Every selector overload exists six times (scalar and list, reference and +value-type, complex), so a parallel delegate family roughly doubles the +selector surface and creates two idiomatic ways to write every binding. + +## Decision + +The v1 binder selects DTO properties exclusively through expression-tree +selectors; the per-call expression cost is accepted for v1, and any +delegate-based fast path is deferred to a post-v1, additive decision. + +## Rationale + +The measured absolute cost — a few µs per request — is negligible against +the tens to hundreds of µs a request spends at the boundary it binds for; +the per-property cost matters only in outlier shapes (very wide DTOs on very +hot endpoints), and those callers already have a zero-breaking-change escape +hatch: hoisting selectors into static fields, which removes ~85–90 % of the +time and ~75 % of the allocation with the API exactly as it is. + +A single selector idiom is worth more to v1 than the residual nanoseconds: a +duplicated delegate+name family would double a six-overload surface, split +call-site style across codebases, and re-introduce the stringly-typed +property naming the expression API exists to prevent — its single entry +point is what lets the binder derive value, name, and mis-declaration guard +from one construct. Because a delegate family is purely additive, deferring +it loses nothing: it can ship in any post-v1 minor if profiling of real +consumers ever shows the call-site tree to matter, whereas shipping it now +is a surface commitment v1 would carry forever. + +## Alternatives Considered + +### Add delegate+name overloads now + +Considered because it is the only way to reach the ~0 B selector floor +(compiler-cached delegates) without breaking anyone. Rejected for v1: it +doubles the selector surface, institutionalizes two ways to bind before the +first stable release, and optimizes a cost that hoisting already lets hot +callers remove today. + +### Replace the expression API with delegate+name + +Considered for the same floor with a single idiom. Rejected: it is a +breaking redesign of the binder's central construct, reverts argument +naming to strings, and contradicts the compile-checked selector contract the +analyzers and guards build on. + +### Generate selectors at compile time (source generator) + +Considered as the long-term way to get expression-level ergonomics at +delegate-level cost. Rejected for v1 as a scope, not on merit: it is a new +component with its own compatibility surface; the ADR base records it as +the natural successor if the residual cost ever matters. + +## Consequences + +### Positive + +* One selector idiom in v1; the six-overload surface stays as it is. +* The internal optimizations of issue #151 stand on their own: paths and + reflection no longer cost anything on an all-valid bind, whichever way + the selector decision evolves later. +* Hot callers have a documented, measured mitigation (hoisted selectors) + that requires no library change. + +### Negative + +* The default call-site style keeps paying ~488 B / ~416 ns per bound + property for the expression tree. + +### Risks + +* If v1 adoption surfaces workloads where the selector cost dominates in + practice, the remedy is additive (delegate overloads or a source + generator) — the risk is bounded to carrying this ADR's successor, not to + a breaking change. + +## Follow-up Actions + +* Document the hoisted-selector mitigation in the binder's user guide when + the topic first comes up with consumers. +* Re-run `FirstClassErrors.RequestBinder.Benchmarks` when the binder's + selector surface next changes, and revisit this ADR with numbers. + +## References + +* Issue #151 — Binder: per-property binding uses uncached reflection and + allocates a path string on the happy path (review finding 15/19). +* `FirstClassErrors.RequestBinder.Benchmarks/README.md` — measurement + harness, full before/after tables, and cost decomposition. +* [ADR-0008](0008-bind-nullable-value-type-properties-through-a-struct-constrained-overload.md) — + the struct-constrained overload family the selector surface is built on. +* [ADR-0021](0021-bind-out-of-dto-arguments-as-peers-through-a-source-agnostic-entry.md) — + the out-of-DTO entry, whose name-based path is the binder's existing + non-expression shape. +* [ADR-0022](0022-floor-the-library-on-net-framework-4-7-2.md) — the .NET + Framework 4.7.2 floor the compiled-getter cache was verified against. diff --git a/doc/handwritten/for-maintainers/adr/README.md b/doc/handwritten/for-maintainers/adr/README.md index 720f2227..d3e9a179 100644 --- a/doc/handwritten/for-maintainers/adr/README.md +++ b/doc/handwritten/for-maintainers/adr/README.md @@ -196,3 +196,4 @@ Optional supporting material: | [ADR-0020](0020-materialize-dummies-only-through-generate.md) | Materialize dummies only through Generate() | Accepted | | [ADR-0021](0021-bind-out-of-dto-arguments-as-peers-through-a-source-agnostic-entry.md) | Bind out-of-DTO arguments as peers through a source-agnostic untyped entry | Proposed | | [ADR-0022](0022-floor-the-library-on-net-framework-4-7-2.md) | Floor the library's .NET Framework support at 4.7.2 | Accepted | +| [ADR-0023](0023-keep-expression-tree-selectors-for-the-v1-binder-api.md) | Keep expression-tree selectors for the v1 binder API | Accepted |