Phase 4 (Waves 1–2): collection binding + validation engine by guy-lud · Pull Request #33 · existall/SimpleSettings

guy-lud · 2026-07-15T14:05:22Z

Phase 4 — Collection & Validation Binding (Waves 1–2)

Engine work for the client pre-beta requirements. This PR covers Waves 1–2 of Phase 4; Wave 3 (VAL-01 DI-resolved path + API-02) will be pushed to this same branch, so the diff will grow.

What's included

COLL-01 / COLL-02 (04-01): ListTypeConverter materializes List<T>/IList<T>/ICollection<T>/IReadOnlyList<T>/IReadOnlyCollection<T> from a delimited scalar via a cached per-element factory delegate (no Activator/MakeGenericType on the warm path). New disjoint IsListLike/IsCollectionShape predicates. Shape-aware CreateNullResult: an unset array/List/IEnumerable binds an empty collection, never null — with a fresh-per-bind empty List<T> (arrays stay shared-cached).
COLL-03 (04-02): ConfigurationBinder binds collections from IConfiguration child-section sequences (Section:Key:0, :1, …); children win over a scalar; the comma-scalar form still binds; empty/whitespace → empty. An InternalsVisibleTo grant lets the Binders assembly reuse Core's shape predicate (one source of truth).
VAL-01 core path (04-03): Declared settings validators now run in the populate pipeline — object-level [SettingsValidator] and property-level [SettingsProperty(ValidatorType)], including cross-property rules — aggregated into one value-free SettingsValidationException via a shared ThrowIfAny helper. Validator-free types short-circuit with zero added allocation (SettingsPlan.HasValidators gate).
VAL-02 (04-05): [SettingsProperty(AllowEmpty=false)] rejects empty and whitespace strings (not just null), reusing the value-free SettingsPropertyNullException (no redaction-filter change).

Security

The exception-redaction invariant (S1) is re-verified on the new sequence path: a D-06 3-case regression test proves a secret sequence element never appears in the exception ToString() chain (int[] first-element, int[] later-element, and List<int>). Formal security-auditor sign-off is pending the phase-level security gate.

Verification

Build: 0 warnings / 0 errors for net8.0 + net10.0.
Tests: 138/138 on net10.0 (net8.0 runs in CI).

Notes

Clean branch: source + tests only — GSD .planning/ planning artifacts are intentionally excluded; the src/ tree is byte-identical to the working phase branch.
Draft: Phase 4 is not complete (Wave 3 pending) and has not yet passed phase verification / security sign-off. Please do not merge yet — a merge to master auto-publishes an -alpha.

- Add fixture interfaces for List/IList/ICollection/IReadOnlyList/IReadOnlyCollection<int> - Assert each materializes a List<int> from a delimited scalar - Add per-element DayOfWeek list test (element converter chain)

- Add IsListLike/IsCollectionShape predicates disjoint from IsEnumerable/IsArray - Add ListTypeConverter subclassing CollectionTypeConverter; materialize via cached per-element factory delegate (no warm-path reflection, S-4/A1) - Promote CollectionTypeConverter.Convert to virtual - Register ListTypeConverter after EnumerableTypeConverter, before EnumTypeConverter (disjoint, order preserved)

- Unbound int[] -> empty int[], unbound List<int> -> empty List<int> - Regression: unbound IEnumerable<int> -> empty int[] - B-3: two null-binds of List<int> return reference-distinct, mutation-isolated instances

- CreateNullResult branches IsArray -> IsEnumerable -> IsListLike -> value-type/null - Array/IEnumerable keep the shared cached empty array - List family binds a fresh empty List<T> per bind via a baked factory delegate (B-3), reusing the existing _nullResult slot so PropertyPlan[] layout is unchanged - PropertyConversion.Convert invokes the factory when _nullResult is Func<object>

…inder - child-section sequence binds string[]/int[]/List<string> elements - children win over coexisting scalar - comma-scalar regression guard (prod override) - whitespace scalar and empty sequence bind empty (no [" "] token) - RootSection prefix honored on the sequence path

…nces - grant InternalsVisibleTo to the Binders assembly so ConfigurationBinder reuses Core's internal IsCollectionShape predicate (single source of truth) - add GetChildren() branch: indexed sub-sections materialize a right-sized string[] via a manual two-pass walk (no LINQ on the bind path) - children win over a coexisting scalar; empty sequence falls through - whitespace/empty scalar on a collection target skips SetNewValue so COLL-02's empty-not-null default applies (avoids the [" "] token pitfall) - element chain and RootSection prefix inherited unchanged

- Convert_SecretInSequenceElement_DoesNotLeakValue: int[] secret in a later element is absent from the whole SettingsPropertyValueException.ToString() - first-element case (S-6): element position must not matter - List<int> case (S-6): redaction holds across the array and list converter shapes - drives the new ConfigurationBinder GetChildren() sequence path, not the scalar InMemoryBinder path

…ibute, aggregate exception - ISettingsValidator/ISettingValidation<T>.Validate return ValidationResult (drop Task<>) (D-07) - ValidationContext(object)/ValidationContext<T>(T) constructors carry the settings instance (D-08) - SettingsValidatorAttribute (AttributeTargets.Interface, Type ValidatorType) (D-11) - SettingsValidationException : SimpleSettingsException + static ThrowIfAny shared aggregate-and-throw (D-10/S-3) - Resources.SettingsValidationExceptionMessage composed only from author ValidationError text (D-12)

- object-level [SettingsValidator] and property-level [SettingsProperty(ValidatorType)] invocation - multi-error aggregation into one SettingsValidationException - cross-property rule observing the fully-populated instance - D-12 value-free exception message - B-2 validator-free short-circuit fast path

- thread object-level [SettingsValidator] and property-level [SettingsProperty(ValidatorType)] into the cached plan at build (D-11) - SettingsPlan.HasValidators computed once at build; post-populate hook short-circuits before any allocation, error list allocated lazily (review B-2) - validators run after the property-set loop so cross-property rules see the full instance (D-09) - reflective dispatch selects the Validate overload via GetMethod(name, new[]{closedContextType}) + MakeGenericType context, avoiding AmbiguousMatchException (review S-2) - aggregate-and-throw routed through shared SettingsValidationException.ThrowIfAny (review S-3/D-10)

…tests - AllowEmpty=false rejects "" and whitespace via SettingsPropertyNullException (value-free) - AllowEmpty=true still binds "" and whitespace as-is

…L-02) - Convert now throws value-free SettingsPropertyNullException for null or null-or-whitespace strings when AllowEmpty=false - AllowEmpty=true falls through to normal conversion (empty/whitespace bound as-is) - 04-01 list null-result factory dispatch preserved

guy-lud · 2026-07-15T15:25:50Z

+namespace ExistForAll.SimpleSettings
+{
+	[AttributeUsage(AttributeTargets.Interface)]
+	public class SettingsValidatorAttribute : Attribute


But we have a section attribute we can reuse

Done in b049f8d — SettingsValidatorAttribute is removed and the object-level validator now rides on SettingsSectionAttribute.ValidatorType (read at plan build in ValuesPopulator).

One consequence worth flagging: since [SettingsSection] is also the scan-discovery marker, attaching a validator now makes a type discoverable, so the deliberately-failing validation fixtures would be eagerly built by the whole-assembly ScanAssemblies tests. Making them internal to hide from the scan doesn't work — the Reflection.Emit proxy generator emits into a random-GUID dynamic assembly that can't implement an internal interface (verified: TypeLoadException). So the fixtures moved to a new never-scanned test library (ExistForAll.SimpleSettings.UnitTests.Fixtures), plus a regression test that the validator still runs under the scan path. Full suite 143/143.

guy-lud · 2026-07-15T15:28:01Z

+
+			var closedContextType = ResolveContextType(validatorType);
+
+			var context = Activator.CreateInstance(closedContextType, target)!;


why not to cast?

Done — it casts now. Dispatch is (ISettingsValidator)Activator.CreateInstance(validatorType) then validator.Validate(new ValidationContext(target)), with ISettingValidation<T> default-implementing the non-generic ISettingsValidator.Validate (the DIM bridge). No reflection / MakeGenericType / MethodInfo.Invoke anymore — see the current InvokeValidator in ValuesPopulator.cs.

…y elements - Enumerate the live config view once (a second GetChildren() pass could race a reload and overrun the count-sized array or inject null holes) — review MED-1. - Drop empty entries for parity with the comma-scalar split (RemoveEmptyEntries), so ["1","","3"] no longer diverges from "1,,3" and crashes int[]/List<int> — review MED-2. - PR #33 review comment 1.

- Consolidate the two null branches into one block (throw if required, else the null-result/factory), keeping the whitespace-string reject and the AllowEmpty=true fall-through intact. PR #33 review comment 2.

…idge; guard invocation value-free - ISettingValidation<T> default-implements the base Validate, forwarding to the generic overload, so ValuesPopulator dispatches through a plain ISettingsValidator cast — no MakeGenericType/GetMethod/Invoke reflection and no AmbiguousMatchException workaround (PR #33 comment 4). - Wrap validator invocation: a validator that throws is surfaced as the value-free SettingsValidatorInvocationException (never chains the inner, which may hold a secret) — review MED-3. - ValidationContext ctor accepts object? (drops the !); fixtures implement only the generic Validate; add property-validator positive/discriminating, object+property aggregation, and throwing-validator coverage.

…drop - Assert order-sensitive SequenceEqual on indexed-sequence cases (IsEquivalentTo was order-insensitive and would not catch an order regression) — review test-gap T1. - Add an interspersed-empty-element case proving parity with the comma-scalar — review MED-2.

guy-lud · 2026-07-15T16:06:30Z

Pushed review fixes — 5 commits (3f24f91..d4ebe41). Build 0 warnings / 0 errors on net8.0 + net10.0; full suite 142/142 on net10.

Your inline comments

TrySetChildSequence — rewrote to a single-pass enumeration (fixes the reload TOCTOU, MED-1) and now drops empty elements for parity with the comma-scalar RemoveEmptyEntries (MED-2 — ["1","","3"] no longer flows "" into an int[] bind and crashes). I used an allocation-lean List<string> accumulate rather than .Where().ToArray() so the ConfigBinder allocation benchmark doesn't regress — same single enumeration you asked for. Say the word if you'd rather keep literal LINQ.
PropertyConversion.Convert — consolidated to check value == null once.
"why not cast?" — adopted the DIM bridge: ISettingValidation<T> now default-implements the base ISettingsValidator.Validate, forwarding to the generic overload, so dispatch is a plain ISettingsValidator cast — no MakeGenericType/GetMethod/Invoke reflection and no AmbiguousMatchException workaround. Authors implement only the generic Validate. Wave 3's DI runner will inherit this.
Deferred — needs your call. Folding the object-validator into SettingsSectionAttribute couples "has a validator" to "is a discoverable section" ([SettingsSection] is a scan indicator). The deliberately-failing test fixtures currently avoid [SettingsSection] precisely so ScanAssemblies doesn't pick them up; making them discoverable risks the scan/integration tests, and internal (non-exported) settings interfaces aren't proven proxyable here. Options: (a) keep the separate SettingsValidatorAttribute (current, safest), (b) reuse + rework the scan tests to tolerate the discovered fixtures, (c) reuse + attempt internal fixtures. Which do you want?

Also addressed (from the automated review)

MED-3 (secret leak): validator invocation is now wrapped value-free — a validator that throws surfaces as SettingsValidatorInvocationException (validator/failure Type only, no chained inner). New test proves a secret in a throwing validator's message never reaches ToString().
Test gaps: locked child-sequence element order (T1), property-validator positive + discriminating cases (T2), object+property aggregation (T3), and an empty-element parity test.
Lows: stripped internal review-ID tokens from comments; broadened the AllowEmpty message to "null, empty, or whitespace".

Deferred to Phase 5

Validator-dispatch caching (code-review M2) — perf follow-up; only validator-using types pay it, and the validator-free warm path stays zero-alloc.

…ype (PR #33 comment #3) Reuse the existing section attribute for the object-level validator instead of a separate SettingsValidatorAttribute (now deleted; it was unreleased, so not a breaking change). ValuesPopulator reads the validator Type from SettingsSectionAttribute.ValidatorType; the dispatch and value-free exception wrapping are unchanged. Because [SettingsSection] is the scan-discovery marker, the deliberately-failing validation fixtures would now be eagerly built by the whole-assembly ScanAssemblies tests. Making them internal is not viable: the Reflection.Emit proxy generator emits into a random-GUID dynamic assembly that cannot implement an internal interface (verified: TypeLoadException). So the fixtures move to a new, never-scanned ExistForAll.SimpleSettings.UnitTests.Fixtures library, referenced by UnitTests but never passed to ScanAssemblies. Adds a regression test that the merged validator also runs under the assembly-scan path. Full suite 143/143 on net10; build clean on net8 + net10.

guy-lud added 12 commits July 15, 2026 17:03

test(04-01): add failing tests for List<T>-family conversion

49b307c

- Add fixture interfaces for List/IList/ICollection/IReadOnlyList/IReadOnlyCollection<int> - Assert each materializes a List<int> from a delimited scalar - Add per-element DayOfWeek list test (element converter chain)

test(04-01): add failing tests for empty-not-null collection defaults

64a1e0b

- Unbound int[] -> empty int[], unbound List<int> -> empty List<int> - Regression: unbound IEnumerable<int> -> empty int[] - B-3: two null-binds of List<int> return reference-distinct, mutation-isolated instances

test(04-05): add failing AllowEmpty=false empty/whitespace rejection …

b1156bd

…tests - AllowEmpty=false rejects "" and whitespace via SettingsPropertyNullException (value-free) - AllowEmpty=true still binds "" and whitespace as-is

guy-lud commented Jul 15, 2026

View reviewed changes

guy-lud added 5 commits July 15, 2026 19:05

refactor(04-05): check value==null once in PropertyConversion.Convert

779f0fc

- Consolidate the two null branches into one block (throw if required, else the null-result/factory), keeping the whitespace-string reject and the AllowEmpty=true fall-through intact. PR #33 review comment 2.

chore(04): strip internal review-ID tokens from code comments

d4ebe41

guy-lud marked this pull request as ready for review July 15, 2026 18:11

guy-lud merged commit 52a2c6f into master Jul 15, 2026
2 checks passed

guy-lud mentioned this pull request Jul 15, 2026

Phase 4: land planning/execution records + child-sequence collection-expression tweak #34

Merged

guy-lud deleted the gsd/phase-4-collection-validation-binding-pr branch July 15, 2026 18:29

guy-lud mentioned this pull request Jul 15, 2026

Phase 4 Wave 3: DI-resolved validator path + ISettingsCollection exposure (VAL-01 DI path, API-02) #35

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Phase 4 (Waves 1–2): collection binding + validation engine#33

Phase 4 (Waves 1–2): collection binding + validation engine#33
guy-lud merged 18 commits into
masterfrom
gsd/phase-4-collection-validation-binding-pr

guy-lud commented Jul 15, 2026

Uh oh!

Uh oh!

Uh oh!

guy-lud Jul 15, 2026

Uh oh!

guy-lud Jul 15, 2026

Uh oh!

guy-lud Jul 15, 2026

Uh oh!

guy-lud Jul 15, 2026

Uh oh!

guy-lud commented Jul 15, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant


		var closedContextType = ResolveContextType(validatorType);

		var context = Activator.CreateInstance(closedContextType, target)!;

Uh oh!

Conversation

guy-lud commented Jul 15, 2026

Phase 4 — Collection & Validation Binding (Waves 1–2)

What's included

Security

Verification

Notes

Uh oh!

Uh oh!

Uh oh!

guy-lud Jul 15, 2026

Choose a reason for hiding this comment

Uh oh!

guy-lud Jul 15, 2026

Choose a reason for hiding this comment

Uh oh!

guy-lud Jul 15, 2026

Choose a reason for hiding this comment

Uh oh!

guy-lud Jul 15, 2026

Choose a reason for hiding this comment

Uh oh!

guy-lud commented Jul 15, 2026

Your inline comments

Also addressed (from the automated review)

Deferred to Phase 5

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant