diff --git a/FirstClassErrors.RequestBinder.PropertyTests/BinderPropertyModel.cs b/FirstClassErrors.RequestBinder.PropertyTests/BinderPropertyModel.cs new file mode 100644 index 00000000..3f1557a0 --- /dev/null +++ b/FirstClassErrors.RequestBinder.PropertyTests/BinderPropertyModel.cs @@ -0,0 +1,104 @@ +#region Usings declarations + +using FsCheck; +using FsCheck.Fluent; + +#endregion + +namespace FirstClassErrors.RequestBinder.PropertyTests; + +#region Request DTO and value object + +/// A request DTO carrying a single list property — the surface the list-binding invariants bind against. +internal sealed record TokenListRequest(IReadOnlyList? Tokens); + +/// +/// A minimal value object: valid exactly when the raw token is a non-empty run of letters or digits. It +/// deliberately rejects whitespace, so a generated token can be made valid or invalid on demand — which is what +/// lets a property know, before binding, which elements the binder must reject. +/// +internal sealed class Token { + + private Token(string value) { + Value = value; + } + + public string Value { get; } + + public static Outcome Parse(string raw) { + return raw.Length > 0 && raw.All(char.IsLetterOrDigit) + ? Outcome.Success(new Token(raw)) + : Outcome.Failure(TokenError.Invalid(raw)); + } + +} + +#endregion + +#region Error factories + +/// The leaf error a token fails to parse with. +internal static class TokenError { + + private static readonly ErrorCode Code = ErrorCode.Create("PROP_TOKEN_INVALID"); + + public static DomainError Invalid(string raw) { + return DomainError.Create(Code, $"'{raw}' is not a valid token.") + .WithPublicMessage("The token is invalid."); + } + +} + +/// The envelope the binder groups every recorded violation into. +internal static class CommandError { + + private static readonly ErrorCode Code = ErrorCode.Create("PROP_COMMAND_INVALID"); + + public static PrimaryPortError Invalid(PrimaryPortInnerErrors violations) { + return PrimaryPortError.Create(Code, "The command is invalid.", violations) + .WithPublicMessage("The request is invalid."); + } + +} + +#endregion + +#region Generators + +/// +/// Generates lists of token slots for the property-based tests. A slot pairs the raw value a request would +/// carry with whether that value must fail to bind — a classification that is a pure function of a generated +/// integer, so the expected outcome (which elements fail, and at which index) is known before binding. +/// +internal static class BinderGen { + + #region Statics members declarations + + /// + /// Classifies an arbitrary integer into a token slot: one third bind as a valid token, one third as an + /// invalid (whitespace-bearing) token, one third as a null element. Both invalid and null are failing + /// slots — the binder records exactly one error for each. + /// + public static (string? Raw, bool Fails) ToSlot(int seed) { + int nonNegative = seed & int.MaxValue; + + return (nonNegative % 3) switch { + 0 => ("t" + nonNegative, false), // letters and digits only → binds + 1 => ("t " + nonNegative, true), // embeds a space → REQUEST_ARGUMENT_INVALID + _ => ((string?)null, true) // absent element → REQUEST_ARGUMENT_REQUIRED + }; + } + + /// + /// Generates a list of token slots of arbitrary length — the empty list included — each element + /// independently classified as valid, invalid, or null. + /// + public static Gen<(string? Raw, bool Fails)[]> Slots() { + return ArbMap.Default.GeneratorFor().Select(seeds => seeds.Select(ToSlot).ToArray()); + } + + #endregion + +} + +#endregion diff --git a/FirstClassErrors.RequestBinder.PropertyTests/FirstClassErrors.RequestBinder.PropertyTests.csproj b/FirstClassErrors.RequestBinder.PropertyTests/FirstClassErrors.RequestBinder.PropertyTests.csproj new file mode 100644 index 00000000..3c3ac82f --- /dev/null +++ b/FirstClassErrors.RequestBinder.PropertyTests/FirstClassErrors.RequestBinder.PropertyTests.csproj @@ -0,0 +1,36 @@ + + + + net10.0 + enable + enable + false + + + + + all + runtime; build; native; contentfiles; analyzers; buildtransitive + + + + + + + all + runtime; build; native; contentfiles; analyzers; buildtransitive + + + + + + + + + + + + + + diff --git a/FirstClassErrors.RequestBinder.PropertyTests/ListBindingInvariantTests.cs b/FirstClassErrors.RequestBinder.PropertyTests/ListBindingInvariantTests.cs new file mode 100644 index 00000000..e3190baf --- /dev/null +++ b/FirstClassErrors.RequestBinder.PropertyTests/ListBindingInvariantTests.cs @@ -0,0 +1,94 @@ +#region Usings declarations + +using FsCheck; +using FsCheck.Fluent; + +using JetBrains.Annotations; + +#endregion + +namespace FirstClassErrors.RequestBinder.PropertyTests; + +/// +/// Property-based (fuzzing-style) invariants for the request binder's list binding. Where the example-based +/// unit tests pin a handful of hand-picked cases, these assert the binder's defining guarantees across a wide +/// range of randomly generated lists: the collect-all contract — one recorded error per failing element, never +/// fewer, never more — and the positional stability of the indexed error paths, so that reordering the elements +/// reorders their paths accordingly and an element's path never depends on its neighbours. +/// +[TestSubject(typeof(ListOfSimplePropertiesConverter<,>))] +public sealed class ListBindingInvariantTests { + + [Fact(DisplayName = "Collect-all: a required list records exactly one error per failing element, and none for a valid one.")] + public void CollectAllRecordsExactlyOneErrorPerFailingElement() { + Prop.ForAll(BinderGen.Slots().ToArbitrary(), + slots => { + int expectedFailures = slots.Count(slot => slot.Fails); + Outcome> outcome = BindTokens(slots); + + return expectedFailures == 0 + ? outcome.IsSuccess && outcome.GetResultOrThrow().Count == slots.Length + : outcome.IsFailure && outcome.Error!.InnerErrors.Count == expectedFailures; + }) + .QuickCheckThrowOnFailure(); + } + + [Fact(DisplayName = "Indexed paths are exactly the positions of the failing elements — nothing more, nothing less.")] + public void ErrorPathsAreExactlyThePositionsOfFailingElements() { + Prop.ForAll(BinderGen.Slots().ToArbitrary(), + slots => FailingPaths(BindTokens(slots)).SetEquals(PositionsOfFailures(slots))) + .QuickCheckThrowOnFailure(); + } + + [Fact(DisplayName = "Reordering the elements reorders their error paths accordingly: a path tracks position, never content or neighbours.")] + public void ReorderingElementsReordersTheirErrorPathsAccordingly() { + Prop.ForAll(BinderGen.Slots().ToArbitrary(), + slots => { + (string? Raw, bool Fails)[] reversed = slots.Reverse().ToArray(); + + return FailingPaths(BindTokens(slots)).SetEquals(PositionsOfFailures(slots)) + && FailingPaths(BindTokens(reversed)).SetEquals(PositionsOfFailures(reversed)); + }) + .QuickCheckThrowOnFailure(); + } + + #region Statics members declarations + + /// Binds the generated tokens as a required list and returns the whole outcome. + private static Outcome> BindTokens((string? Raw, bool Fails)[] slots) { + TokenListRequest request = new(slots.Select(slot => slot.Raw).ToArray()); + var binder = Bind.PropertiesOf(request).FailWith(CommandError.Invalid); + + RequiredField> tokens = binder.ListOfSimpleProperties(r => r.Tokens).AsRequired(Token.Parse); + + return binder.New(scope => scope.Get(tokens)); + } + + /// The indexed paths a correct binder must record — one per failing slot, at that slot's position. + private static ISet PositionsOfFailures((string? Raw, bool Fails)[] slots) { + return slots.Select((slot, index) => (slot, index)) + .Where(entry => entry.slot.Fails) + .Select(entry => $"Tokens[{entry.index}]") + .ToHashSet(); + } + + /// The set of indexed argument paths actually recorded by a binding outcome (empty on success). + private static ISet FailingPaths(Outcome> outcome) { + if (outcome.IsSuccess) { return new HashSet(); } + + return outcome.Error!.InnerErrors + .Select(ArgumentPathOf) + .Where(path => path is not null) + .Select(path => path!) + .ToHashSet(); + } + + private static string? ArgumentPathOf(Error error) { + error.Context.ToNameDictionary().TryGetValue("RequestArgument", out object? path); + + return path as string; + } + + #endregion + +} diff --git a/FirstClassErrors.RequestBinder.UnitTests/ListBindingTests.cs b/FirstClassErrors.RequestBinder.UnitTests/ListBindingTests.cs index f56b2f04..b3de7376 100644 --- a/FirstClassErrors.RequestBinder.UnitTests/ListBindingTests.cs +++ b/FirstClassErrors.RequestBinder.UnitTests/ListBindingTests.cs @@ -41,6 +41,17 @@ public void RequiredSimpleListMissing() { Check.That(BindingAssertions.ArgumentPathOf(required)).IsEqualTo("Tags"); } + [Fact(DisplayName = "A required list that is present but empty is valid: it binds an empty list and records nothing — required constrains presence, not element count.")] + public void RequiredSimpleListPresentButEmptyBindsEmpty() { + var bind = Bind.PropertiesOf(RequestWith(tags: [])).FailWith(BookingEnvelopeError.CommandInvalid); + + RequiredField> tags = bind.ListOfSimpleProperties(r => r.Tags).AsRequired(Tag.Parse); + + Outcome> outcome = bind.New(s => s.Get(tags)); + Check.That(outcome.IsSuccess).IsTrue(); + Check.That(outcome.GetResultOrThrow()).IsEmpty(); + } + [Fact(DisplayName = "Every failing element of a list is collected, each under its indexed path — one bad element never hides the others.")] public void EveryFailingElementIsCollected() { var bind = Bind.PropertiesOf(RequestWith(tags: ["ok", "not ok", null, "fine"])).FailWith(BookingEnvelopeError.CommandInvalid); @@ -79,6 +90,18 @@ public void RequiredComplexListBinds() { Check.That(outcome.GetResultOrThrow()[1].Email).IsNull(); } + [Fact(DisplayName = "A required complex list that is present but empty is valid: it binds an empty list and records nothing.")] + public void RequiredComplexListPresentButEmptyBindsEmpty() { + var bind = Bind.PropertiesOf(RequestWith(guests: [])).FailWith(BookingEnvelopeError.CommandInvalid); + + RequiredField> guests = + bind.ListOfComplexProperties(r => r.Guests).FailWith(BookingEnvelopeError.GuestInvalid).AsRequired(BindGuest); + + Outcome> outcome = bind.New(s => s.Get(guests)); + Check.That(outcome.IsSuccess).IsTrue(); + Check.That(outcome.GetResultOrThrow()).IsEmpty(); + } + [Fact(DisplayName = "Each failing element of a complex list records its own envelope, whose inner paths carry the indexed prefix.")] public void FailingComplexElementsRecordTheirEnvelopes() { var bind = Bind.PropertiesOf(RequestWith(guests: [new GuestDto("Alice", null), new GuestDto(null, "nope"), new GuestDto(null, null)])) diff --git a/FirstClassErrors.RequestBinder.UnitTests/ValueTypePropertyBindingTests.cs b/FirstClassErrors.RequestBinder.UnitTests/ValueTypePropertyBindingTests.cs index d0eecded..2cd000c9 100644 --- a/FirstClassErrors.RequestBinder.UnitTests/ValueTypePropertyBindingTests.cs +++ b/FirstClassErrors.RequestBinder.UnitTests/ValueTypePropertyBindingTests.cs @@ -162,6 +162,17 @@ public void ValueTypeListOptionalVsRequiredWhenAbsent() { Check.That(required.New(_ => "never").Error!.InnerErrors.Single().Code.ToString()).IsEqualTo("REQUEST_ARGUMENT_REQUIRED"); } + [Fact(DisplayName = "A required value-type list that is present but empty is valid: it binds an empty list and records nothing.")] + public void RequiredValueTypeListPresentButEmptyBindsEmpty() { + var bind = Bind.PropertiesOf(Request(quantities: [])).FailWith(BookingEnvelopeError.CommandInvalid); + + RequiredField> quantities = bind.ListOfSimpleProperties(r => r.Quantities).AsRequired(PositiveQty.From); + + Outcome> outcome = bind.New(s => s.Get(quantities)); + Check.That(outcome.IsSuccess).IsTrue(); + Check.That(outcome.GetResultOrThrow()).IsEmpty(); + } + // ── Regressions: reference/string paths keep resolving to the original overloads ────────────────────── [Fact(DisplayName = "A string property still resolves to the reference overload — no ambiguity introduced by the value-type overload.")] diff --git a/FirstClassErrors.RequestBinder/ListOfComplexPropertiesConverter.cs b/FirstClassErrors.RequestBinder/ListOfComplexPropertiesConverter.cs index 94ce653e..378dd6cf 100644 --- a/FirstClassErrors.RequestBinder/ListOfComplexPropertiesConverter.cs +++ b/FirstClassErrors.RequestBinder/ListOfComplexPropertiesConverter.cs @@ -36,8 +36,10 @@ internal ListOfComplexPropertiesConverter(RequestBinder #endregion /// - /// Binds a required list: a missing list records REQUEST_ARGUMENT_REQUIRED; each failing element - /// records its envelope under its indexed path. + /// 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 + /// constrains the list's presence, not its element count. Each failing element records its envelope + /// under its indexed path. /// /// The type each element's nested binding produces. /// The nested binding function applied to each element (typically a method group). diff --git a/FirstClassErrors.RequestBinder/ListOfSimplePropertiesConverter.cs b/FirstClassErrors.RequestBinder/ListOfSimplePropertiesConverter.cs index b0c587bd..0446709b 100644 --- a/FirstClassErrors.RequestBinder/ListOfSimplePropertiesConverter.cs +++ b/FirstClassErrors.RequestBinder/ListOfSimplePropertiesConverter.cs @@ -30,8 +30,10 @@ internal ListOfSimplePropertiesConverter(RequestBinder binder, string #endregion /// - /// Binds a required list: a missing list records REQUEST_ARGUMENT_REQUIRED; each failing element - /// records REQUEST_ARGUMENT_INVALID under its indexed path. + /// 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 + /// constrains the list's presence, not its element count. Each failing element records + /// REQUEST_ARGUMENT_INVALID under its indexed path. /// /// The type of the element value object. /// The value-object converter applied to each element. diff --git a/FirstClassErrors.RequestBinder/ListOfSimpleValuePropertiesConverter.cs b/FirstClassErrors.RequestBinder/ListOfSimpleValuePropertiesConverter.cs index a9297bd8..4cd65fb6 100644 --- a/FirstClassErrors.RequestBinder/ListOfSimpleValuePropertiesConverter.cs +++ b/FirstClassErrors.RequestBinder/ListOfSimpleValuePropertiesConverter.cs @@ -36,8 +36,10 @@ internal ListOfSimpleValuePropertiesConverter(RequestBinder binder, st #endregion /// - /// Binds a required list: a missing list records REQUEST_ARGUMENT_REQUIRED; each failing element - /// records REQUEST_ARGUMENT_INVALID under its indexed path, and a null element records + /// 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 + /// constrains the list's presence, not its element count. Each failing element records + /// REQUEST_ARGUMENT_INVALID under its indexed path, and a null element records /// REQUEST_ARGUMENT_REQUIRED. /// /// The type of the element value object. diff --git a/FirstClassErrors.sln b/FirstClassErrors.sln index 457f2083..affa9e4a 100644 --- a/FirstClassErrors.sln +++ b/FirstClassErrors.sln @@ -49,6 +49,8 @@ Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Dummies", "Dummies\Dummies. EndProject Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Dummies.UnitTests", "Dummies.UnitTests\Dummies.UnitTests.csproj", "{BC4F2BE8-55C9-4AA4-90FA-699EC83A8985}" EndProject +Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "FirstClassErrors.RequestBinder.PropertyTests", "FirstClassErrors.RequestBinder.PropertyTests\FirstClassErrors.RequestBinder.PropertyTests.csproj", "{1FC2C501-8842-4ABE-845E-000ED6575DD6}" +EndProject Global GlobalSection(SolutionConfigurationPlatforms) = preSolution Debug|Any CPU = Debug|Any CPU @@ -261,6 +263,18 @@ Global {BC4F2BE8-55C9-4AA4-90FA-699EC83A8985}.Release|x64.Build.0 = Release|Any CPU {BC4F2BE8-55C9-4AA4-90FA-699EC83A8985}.Release|x86.ActiveCfg = Release|Any CPU {BC4F2BE8-55C9-4AA4-90FA-699EC83A8985}.Release|x86.Build.0 = Release|Any CPU + {1FC2C501-8842-4ABE-845E-000ED6575DD6}.Debug|Any CPU.ActiveCfg = Debug|Any CPU + {1FC2C501-8842-4ABE-845E-000ED6575DD6}.Debug|Any CPU.Build.0 = Debug|Any CPU + {1FC2C501-8842-4ABE-845E-000ED6575DD6}.Debug|x64.ActiveCfg = Debug|Any CPU + {1FC2C501-8842-4ABE-845E-000ED6575DD6}.Debug|x64.Build.0 = Debug|Any CPU + {1FC2C501-8842-4ABE-845E-000ED6575DD6}.Debug|x86.ActiveCfg = Debug|Any CPU + {1FC2C501-8842-4ABE-845E-000ED6575DD6}.Debug|x86.Build.0 = Debug|Any CPU + {1FC2C501-8842-4ABE-845E-000ED6575DD6}.Release|Any CPU.ActiveCfg = Release|Any CPU + {1FC2C501-8842-4ABE-845E-000ED6575DD6}.Release|Any CPU.Build.0 = Release|Any CPU + {1FC2C501-8842-4ABE-845E-000ED6575DD6}.Release|x64.ActiveCfg = Release|Any CPU + {1FC2C501-8842-4ABE-845E-000ED6575DD6}.Release|x64.Build.0 = Release|Any CPU + {1FC2C501-8842-4ABE-845E-000ED6575DD6}.Release|x86.ActiveCfg = Release|Any CPU + {1FC2C501-8842-4ABE-845E-000ED6575DD6}.Release|x86.Build.0 = Release|Any CPU EndGlobalSection GlobalSection(SolutionProperties) = preSolution HideSolutionNode = FALSE @@ -281,6 +295,7 @@ Global {BB28120C-1D68-469D-928A-51AE454D2487} = {B7C3D08D-EFC5-4F5D-8DE4-5B7938354DBB} {7991C766-2720-43E5-A9CB-2F3D6C2025FC} = {02EA681E-C7D8-13C7-8484-4AC65E1B71E8} {75EA57DD-0128-4EB9-ADBE-567BFF602A93} = {B7C3D08D-EFC5-4F5D-8DE4-5B7938354DBB} + {1FC2C501-8842-4ABE-845E-000ED6575DD6} = {B7C3D08D-EFC5-4F5D-8DE4-5B7938354DBB} EndGlobalSection GlobalSection(ExtensibilityGlobals) = postSolution SolutionGuid = {4988972E-3E0D-4F48-8656-0E67ECE994BF} diff --git a/doc/handwritten/for-maintainers/adr/0014-bind-a-required-list-by-presence-not-cardinality.fr.md b/doc/handwritten/for-maintainers/adr/0014-bind-a-required-list-by-presence-not-cardinality.fr.md new file mode 100644 index 00000000..088c859f --- /dev/null +++ b/doc/handwritten/for-maintainers/adr/0014-bind-a-required-list-by-presence-not-cardinality.fr.md @@ -0,0 +1,135 @@ +# ADR-0014 | Lier une liste requise par la présence, pas la cardinalité + +🌍 🇬🇧 [English](0014-bind-a-required-list-by-presence-not-cardinality.md) · 🇫🇷 Français (ce fichier) + +**Statut :** Accepté +**Date :** 2026-07-18 +**Décideurs :** Reefact + +## Contexte + +* Le binder lie une propriété de requête de type liste via `AsRequired` ou `AsOptional`, + le même choix de présence qu'il offre pour une propriété scalaire. +* Le binder déduit « l'argument est manquant » du fait que la propriété du DTO est `null`. + Un sérialiseur mappe un tableau JSON absent vers une propriété `null` ; un tableau JSON + présent mais vide (`[]`) se désérialise en une liste non-null de zéro élément. +* Sur une liste manquante (`null`), `AsRequired` enregistre `REQUEST_ARGUMENT_REQUIRED` et + `AsOptional` lie une liste vide sans rien enregistrer. +* Avant cette décision, le comportement d'une liste requise *présente mais vide* était non + spécifié : ni documenté, ni épinglé par un test. L'implémentation la liait en succès — une + liste vide — car « manquant » n'est que `null`, si bien qu'une liste vide atteint la boucle + par élément, qui itère zéro fois. +* La revue de niveau mainteneur du request binder (spéc de conception #126) a soulevé ce + point comme constat 19 sur 19 (issue #155) : malgré 100 % de couverture ligne et branche, + aucune assertion ne fixait si une liste requise vide est valide, si bien que le comportement + pouvait changer silencieusement. +* Pour une propriété scalaire, « requis » signifie « présent » — un argument non-null ; la + conversion de sa valeur est une préoccupation distincte que porte la fabrique d'objet valeur. +* Le rôle du binder est la présence et la conversion par élément : il enregistre une erreur + codée par élément en échec et collecte chaque échec dans une seule enveloppe. Il ne porte + aucune notion de taille de collection. +* Une règle de cardinalité minimale (« au moins un élément ») est une règle métier qui varie + selon le cas d'usage, et la bibliothèque place déjà les invariants métier dans l'objet valeur + ou la commande qu'une valeur liée alimente. +* La bibliothèque est en pré-version, non publiée sur NuGet et sans consommateur externe : + fixer le contrat maintenant n'entraîne aucun coût de migration. + +## Décision + +Une liaison de liste requise contraint uniquement la présence de la liste — une liste absente +(`null`) enregistre `REQUEST_ARGUMENT_REQUIRED`, tandis qu'une liste présente mais vide est +valide et se lie à une liste vide. + +## Justification + +* La présence est le seul sens que « requis » porte déjà pour une propriété scalaire, et une + liste est une propriété comme une autre ; laisser une liste vide compter comme « manquante » + ferait signifier à `AsRequired` « présent » pour un scalaire et « présent et non vide » pour + une liste — deux contrats sous un même nom, le genre d'incohérence que la conception + délibérément à faible surface du binder évite. +* Garder la cardinalité hors du binder respecte la séparation que la bibliothèque trace déjà : + le binder répond à « l'argument a-t-il été envoyé, et chaque élément se convertit-il ? », et + l'objet valeur ou la commande qu'il alimente répond à « est-ce une valeur métier valide ? », + là où une règle de taille minimale a sa place. Inscrire « non vide » dans chaque liste requise + imposerait la politique d'un domaine à toutes, et cette politique varie réellement + (zéro-ou-plus ici, au-moins-un ailleurs). +* Lire « manquant » comme `null` garde le binder fidèle à ce que le client a réellement envoyé : + un tableau absent et un tableau vide sont deux messages différents sur le fil, et une liste + vide est une valeur délibérée, pas une absence. Enregistrer `REQUEST_ARGUMENT_REQUIRED` pour + une liste que le client a bien envoyée rapporterait mal ce message. +* La revue n'a trouvé aucun défaut dans le comportement, seulement qu'il n'était pas asserté ; + ratifier le comportement existant comme contrat — plutôt que le changer — est le choix le + moins surprenant, et le statut de pré-version permet de fixer le contrat maintenant sans + consommateur à migrer. + +## Alternatives considérées + +### Traiter une liste requise présente mais vide comme manquante + +Envisagée parce que « requis » peut familièrement suggérer « au moins un », si bien qu'un +appelant pourrait s'attendre à ce qu'une liste requise vide soit rejetée. + +Rejetée parce qu'elle confond présence et cardinalité : elle donnerait à `AsRequired` deux sens +différents pour les scalaires et les listes, inscrirait la politique de taille minimale d'un +domaine dans le binder, et rapporterait mal comme une absence un tableau vide délibérément +envoyé. + +### Ajouter une liaison requise non-vide distincte (ou une option de compte minimal) + +Envisagée parce que certaines requêtes ont réellement besoin d'au moins un élément, et un +`AsRequiredNonEmpty` explicite (ou une option de compte) l'exprimerait au niveau du binder. + +Rejetée pour l'instant parce qu'elle élargit la surface du binder avec une préoccupation de +cardinalité que l'objet valeur ou la commande exprime déjà, et qu'aucune exigence actuelle ne la +réclame. Elle reste ouverte comme opt-in futur : cette décision ne fixe que le sens par défaut de +« requis », si bien qu'une variante non-vide ultérieure l'étendrait, sans la contredire. + +### Laisser le comportement non spécifié + +Envisagée parce que c'est le plus petit changement — l'implémentation lie déjà une liste requise +vide en succès. + +Rejetée parce qu'un contrat non asserté est précisément le constat de #155 : il peut changer +silencieusement lors d'un remaniement. Une décision énoncée plus un test d'épinglage le rend +stable. + +## Conséquences + +### Positives + +* « Requis » a un sens unique pour les propriétés scalaires et de liste : présent, non-null. +* Le binder reste une frontière de présence et de conversion ; les règles de cardinalité métier + restent dans le domaine, où elles peuvent varier selon le cas d'usage. +* Le contrat est documenté et épinglé par des tests, si bien qu'il ne peut plus dériver + silencieusement. + +### Négatives + +* Une requête qui a réellement besoin d'une liste non vide doit l'imposer dans l'objet valeur ou + la commande qu'elle alimente, pas via le binder — une petite étape explicite pour le + consommateur. + +### Risques + +* Un besoin futur de cardinalité minimale exigerait une nouvelle surface de liaison opt-in ; + atténué par le fait que cette décision ne fixe que le sens par défaut (laissant la place à une + telle surface) et que la cardinalité est exprimable dans le domaine aujourd'hui. + +## Actions de suivi + +* Documenté dans le même changement : la doc de référence du binder + ([`RequestBinder.fr.md`](../../for-users/RequestBinder.fr.md) et sa version anglaise) et la + doc XML de l'API sur les trois convertisseurs de liste. +* Épinglé dans le même changement : des tests unitaires sur les convertisseurs de liste simple, + de type valeur et complexe, plus des invariants property-based pour les garanties de + collecte-globale et de stabilité des chemins dans `FirstClassErrors.RequestBinder.PropertyTests`. + +## Références + +* Issue #155 — le constat que cette décision résout (constat 19 sur 19 de la revue du request + binder). +* Pull request #126 — la spéc de conception du request binder à laquelle ce contrat appartient. +* ADR-0007 — nommer les terminaux du binder New et Create ; une décision d'API publique sœur sur + le même binder. +* ADR-0012 — fixer les options du binder avant le début de la liaison ; une autre décision de + contrat du binder. diff --git a/doc/handwritten/for-maintainers/adr/0014-bind-a-required-list-by-presence-not-cardinality.md b/doc/handwritten/for-maintainers/adr/0014-bind-a-required-list-by-presence-not-cardinality.md new file mode 100644 index 00000000..7f519bdf --- /dev/null +++ b/doc/handwritten/for-maintainers/adr/0014-bind-a-required-list-by-presence-not-cardinality.md @@ -0,0 +1,130 @@ +# ADR-0014 | Bind a required list by presence, not cardinality + +🌍 🇬🇧 English (this file) · 🇫🇷 [Français](0014-bind-a-required-list-by-presence-not-cardinality.fr.md) + +**Status:** Accepted +**Date:** 2026-07-18 +**Decision Makers:** Reefact + +## Context + +* The request binder binds a list request property through `AsRequired` or `AsOptional`, + the same presence choice it offers for a scalar property. +* The binder derives "the argument is missing" from the DTO property being `null`. A + serializer maps an absent JSON array to a `null` property; a present-but-empty JSON + array (`[]`) deserializes to a non-null list of zero elements. +* On a missing (`null`) list, `AsRequired` records `REQUEST_ARGUMENT_REQUIRED` and + `AsOptional` binds an empty list and records nothing. +* Before this decision, the behavior of a required list that is *present but empty* was + unspecified: it was neither documented nor pinned by a test. The implementation bound + it as success — an empty list — because "missing" is `null` only, so an empty list + reaches the per-element loop, which iterates zero times. +* The maintainer-grade review of the request binder (design spec #126) raised this as + finding 19 of 19 (issue #155): despite 100% line and branch coverage, no assertion + fixed whether an empty required list is valid, so the behavior could change silently. +* For a scalar property, "required" means "present" — a non-null argument; conversion of + its value is a separate concern the value-object factory owns. +* The binder's role is presence and per-element conversion: it records one coded error + per failing element and collects every failure into a single envelope. It carries no + notion of collection size. +* A minimum-cardinality rule ("at least one element") is a domain rule that varies by use + case, and the library already places domain invariants in the value object or command a + bound value feeds. +* The library is pre-release, unpublished on NuGet with no external consumers, so fixing + the contract now carries no migration cost. + +## Decision + +A required list binding constrains only the list's presence — an absent (`null`) list +records `REQUEST_ARGUMENT_REQUIRED`, while a list that is present but empty is valid and +binds to an empty list. + +## Rationale + +* Presence is the one meaning "required" already carries for a scalar property, and a + list is a property like any other; letting an empty list count as "missing" would make + `AsRequired` mean "present" for a scalar and "present and non-empty" for a list — two + contracts under one name, the kind of inconsistency the binder's deliberate, + small-surface design avoids. +* Keeping cardinality out of the binder respects the separation the library already + draws: the binder answers "was the argument sent, and does each element convert?", and + the value object or command it feeds answers "is this a valid domain value?", where a + minimum-size rule belongs. Baking "non-empty" into every required list would impose one + domain's policy on all of them, and that policy genuinely varies (zero-or-more here, + at-least-one there). +* Reading "missing" as `null` keeps the binder faithful to what the client actually sent: + an absent array and an empty array are different messages on the wire, and an empty list + is a deliberate value, not an absence. Recording `REQUEST_ARGUMENT_REQUIRED` for a list + the client did send would misreport that message. +* The review found no defect in the behavior, only that it was unasserted; ratifying the + existing behavior as the contract — rather than changing it — is the least-surprising + choice, and the pre-release status lets the contract be fixed now with no consumers to + migrate. + +## Alternatives Considered + +### Treat a present-but-empty required list as missing + +Considered because "required" can colloquially suggest "at least one", so a caller might +expect an empty required list to be rejected. + +Rejected because it conflates presence with cardinality: it would give `AsRequired` two +different meanings for scalars and lists, hard-code one domain's minimum-size policy into +the binder, and misreport a deliberately sent empty array as an absence. + +### Add a distinct non-empty required binding (or a minimum-count option) + +Considered because some requests genuinely need at least one element, and an explicit +`AsRequiredNonEmpty` (or a count option) would express that at the binder. + +Rejected for now because it widens the binder's surface with a cardinality concern the +value object or command already expresses, and no current requirement calls for it. It +remains open as a future opt-in: this decision fixes only the default meaning of +"required", so a later non-empty variant would extend it, not contradict it. + +### Leave the behavior unspecified + +Considered because it is the smallest change — the implementation already binds an empty +required list as success. + +Rejected because an unasserted contract is exactly the finding of #155: it can change +silently under a refactor. A stated decision plus a pinning test makes it stable. + +## Consequences + +### Positive + +* "Required" has a single meaning across scalar and list properties: present, non-null. +* The binder stays a presence-and-conversion boundary; domain cardinality rules stay in + the domain, where they can vary per use case. +* The contract is documented and pinned by tests, so it can no longer drift silently. + +### Negative + +* A request that truly needs a non-empty list must enforce that in the value object or + command it feeds, not through the binder — a small explicit step for the consumer. + +### Risks + +* A future minimum-cardinality need would require a new opt-in binding surface; mitigated + by this decision fixing only the default meaning (leaving room for one) and by + cardinality being expressible in the domain today. + +## Follow-up Actions + +* Documented in the same change: the binder reference docs + ([`RequestBinder.en.md`](../../for-users/RequestBinder.en.md) and its French + translation) and the API XML docs on the three list converters. +* Pinned in the same change: unit tests across the simple, value-type, and complex list + converters, plus property-based invariants for the collect-all and path-stability + guarantees in `FirstClassErrors.RequestBinder.PropertyTests`. + +## References + +* Issue #155 — the finding this decision resolves (finding 19 of 19 of the request binder + review). +* Pull request #126 — the request binder design spec this contract belongs to. +* ADR-0007 — name the binder terminals New and Create; a sibling public-API decision on + the same binder. +* ADR-0012 — fix the binder options before binding begins; another binder contract + decision. diff --git a/doc/handwritten/for-maintainers/adr/README.md b/doc/handwritten/for-maintainers/adr/README.md index 17220ad3..09f504dd 100644 --- a/doc/handwritten/for-maintainers/adr/README.md +++ b/doc/handwritten/for-maintainers/adr/README.md @@ -187,3 +187,4 @@ Optional supporting material: | [ADR-0011](0011-host-dummies-as-a-standalone-package.md) | Host Dummies as a standalone package in this repository | Proposed | | [ADR-0012](0012-fix-the-binder-options-before-binding-begins.md) | Fix the binder options before binding begins | Accepted | | [ADR-0013](0013-gate-distinct-collections-by-cardinality-else-bounded-draw.md) | Gate distinct collections by cardinality, otherwise by a bounded draw | Proposed | +| [ADR-0014](0014-bind-a-required-list-by-presence-not-cardinality.md) | Bind a required list by presence, not cardinality | Accepted | diff --git a/doc/handwritten/for-users/RequestBinder.en.md b/doc/handwritten/for-users/RequestBinder.en.md index b0dd0826..66e801d5 100644 --- a/doc/handwritten/for-users/RequestBinder.en.md +++ b/doc/handwritten/for-users/RequestBinder.en.md @@ -361,10 +361,14 @@ private static Outcome BindGuest(RequestBinder guest) { // A failure in the second guest's email reports Guests[1].Email. ``` -For both list selectors, `AsRequired` records `REQUEST_ARGUMENT_REQUIRED` when the -list is absent, while `AsOptional` treats an absent list as an **empty** list -(never `null`) and records nothing. A `null` *element* of a present list is -recorded as a missing argument at its index. +For both list selectors, `AsRequired` records `REQUEST_ARGUMENT_REQUIRED` only when +the list itself is **absent** (`null`): a list that is **present but empty** is a +valid required list — it binds to an empty list and records nothing, because a +required list constrains **presence, not size**. `AsOptional` treats an absent list +as an **empty** list (never `null`) and records nothing. A `null` *element* of a +present list is recorded as a missing argument at its index. When the domain needs +at least one element, enforce that cardinality in the value object or command the +bound list feeds. ## Argument names and the wire format diff --git a/doc/handwritten/for-users/RequestBinder.fr.md b/doc/handwritten/for-users/RequestBinder.fr.md index f86dac01..6cb0957d 100644 --- a/doc/handwritten/for-users/RequestBinder.fr.md +++ b/doc/handwritten/for-users/RequestBinder.fr.md @@ -372,10 +372,14 @@ private static Outcome BindGuest(RequestBinder guest) { ``` Pour les deux sélecteurs de liste, `AsRequired` enregistre -`REQUEST_ARGUMENT_REQUIRED` quand la liste est absente, tandis qu’`AsOptional` -traite une liste absente comme une liste **vide** (jamais `null`) et n’enregistre -rien. Un *élément* `null` d’une liste présente est enregistré comme un argument -manquant à son index. +`REQUEST_ARGUMENT_REQUIRED` uniquement quand la liste elle-même est **absente** +(`null`) : une liste **présente mais vide** est une liste requise valide — elle se +lie à une liste vide et n’enregistre rien, car une liste requise contraint la +**présence, pas la taille**. `AsOptional` traite une liste absente comme une liste +**vide** (jamais `null`) et n’enregistre rien. Un *élément* `null` d’une liste +présente est enregistré comme un argument manquant à son index. Quand le domaine +exige au moins un élément, imposez cette cardinalité dans l’objet valeur ou la +commande que la liste liée alimente. ## Noms d’arguments et format du fil