feat: Maybe<Money>, MonetaryAmount, HandleNull fix, ReplaceDbProvider, IComparable#355
Merged
xavierjohn merged 17 commits intomainfrom Mar 29, 2026
Merged
feat: Maybe<Money>, MonetaryAmount, HandleNull fix, ReplaceDbProvider, IComparable#355xavierjohn merged 17 commits intomainfrom
xavierjohn merged 17 commits intomainfrom
Conversation
Fixes: composite ValueObjects (e.g., ShippingAddress) can now yield scalar VOs in GetEqualityComponents without .Value — ScalarValueObject inherits IComparable through ValueObject. Added non-generic IComparable.CompareTo(object?) that delegates to the typed CompareTo(ValueObject?). Added 2 tests with composite VO containing StreetName/CityName scalar VOs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
MaybeConvention now detects when the inner type of Maybe<T> is an EF Core owned type (e.g., Money) and creates an optional ownership navigation via the source-generated backing field instead of attempting scalar column mapping. MoneyConvention configures nullable columns with correct naming. Changes: - MaybeConvention: detect IsOwned(), create HasOwnership via FieldInfo, store property name annotation for MoneyConvention - MoneyConvention: read annotation for column prefix, mark columns nullable for optional Money - Convention order: MaybeConvention first, MoneyConvention second - MaybeUpdateExtensions: guard against ExecuteUpdate on composite VOs with clear InvalidOperationException - MaybeModelExtensions: diagnostics API reports owned Maybe<T> mappings with actual column metadata from owned entity type - 13 new tests: model building, metadata, round-trips (Some/None/mixed), update transitions (Some->None, None->Some), ExecuteUpdate guard, diagnostics API - Documentation: copilot instructions, API reference, READMEs, docfx article all updated with Maybe<Money> examples and ExecuteUpdate warning Resolves ARCL feedback FP-1 and SF-1. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ce validation errors System.Text.Json's JsonConverter<T>.HandleNull defaults to false for reference types. When false, the serializer handles null JSON tokens by setting the property to null WITHOUT calling Read(). This meant ValidatingJsonConverter.OnNullToken() was never invoked during DTO deserialization, silently accepting null values for required scalar VOs. The fix adds HandleNull => true to: - ScalarValueJsonConverterBase: so ValidatingJsonConverter and MaybeScalarValueJsonConverter intercept null tokens - PropertyNameAwareConverter: so the production wrapper also intercepts null tokens before delegating to the inner converter Added 16 new tests covering: - Explicit null JSON for int/decimal/long/bool scalar VOs - Full DTO deserialization with null and missing properties - Mixed valid and null properties - Zero/false values correctly distinguished from null Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
MonetaryAmount: single-currency monetary value (1 decimal column). - Non-negative, rounds to 2 decimal places - Arithmetic: Add, Subtract, Multiply with overflow protection - Invariant culture formatting/parsing for JSON safety - Follows Percentage pattern (hand-implemented ScalarValueObject) RequiredDecimal/RequiredInt doc fixes: - 'Required' means not null, not 'non-zero' - Zero is a valid value for both types - Only RequiredString additionally rejects empty - RequiredGuid correctly rejects Guid.Empty (semantically null) Documentation: copilot instructions, API reference, primitives README, docfx articles all updated with MonetaryAmount. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add 'Composite ValueObjects as EF Core Owned Types' section to copilot instructions covering: private parameterless constructor, private set, null! initializers, TryCreate factory, OwnsOne configuration, and nested OwnsOne/OwnsMany ToTable() guidance. Resolves ARCL feedback FP-3 (decision tree) and FP-4 (owned entity boilerplate documentation). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
ReplaceDbProvider<TContext>(IServiceCollection, Action<DbContextOptionsBuilder>): - Cleanly swaps EF Core database providers in WebApplicationFactory tests - Removes TContext, DbContextOptions<TContext>, and all EF Core internal services generic over TContext (including IDbContextOptionsConfiguration<T> in EF Core 10) to prevent dual-provider conflicts - Re-registers via AddDbContext<TContext> - 6 new tests including full integration round-trip TRLSGEN100 diagnostic test coverage: - Diagnostic was already implemented in MaybePartialPropertyGenerator - Added 5 tests verifying: fires for non-partial Maybe<T>, silent for partial Maybe<T>, silent for non-partial class, multiple properties, message includes inner type name Resolves ARCL feedback FP-2 and FP-5. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Test Results4 134 tests +145 4 119 ✅ +145 3m 56s ⏱️ +30s Results for commit d7b7341. ± Comparison against base commit 045a2ef. This pull request removes 1 and adds 146 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #355 +/- ##
==========================================
+ Coverage 76.60% 85.31% +8.70%
==========================================
Files 202 203 +1
Lines 8216 7144 -1072
Branches 1431 1391 -40
==========================================
- Hits 6294 6095 -199
+ Misses 1496 653 -843
+ Partials 426 396 -30 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Added ReplaceDbProvider section to trellis-api-testing-reference.md with usage example and AddDbContextFactory limitation warning. Added to Trellis.Testing README Service Collection Extensions table. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes System.Text.Json null-token handling for scalar value objects, extends EF Core conventions to support Maybe<Money> as an optional owned type, and adds new primitives/testing helpers (including MonetaryAmount and ReplaceDbProvider<TContext>), along with extensive test and documentation updates.
Changes:
- Fix JSON deserialization so explicit
nullfor reference-type scalar VOs triggers converter-level validation errors (instead of being silently accepted). - Enhance EF Core conventions/diagnostics/update helpers to support
Maybe<Money>(optional owned navigation) and to guard unsupportedExecuteUpdatescenarios. - Add
MonetaryAmount(single-currency scalar VO) andReplaceDbProvider<TContext>(test helper), plus docs/tests for the new APIs and behaviors.
Reviewed changes
Copilot reviewed 31 out of 31 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| trellis-api-reference.md | Documents MonetaryAmount, Maybe<Money> EF behavior, and ExecuteUpdate limitations. |
| docs/docfx_project/articles/primitives.md | Adds MonetaryAmount to primitives documentation and examples. |
| docs/docfx_project/articles/integration-ef.md | Documents EF mapping guidance for MonetaryAmount and Maybe<Money>. |
| Trellis.Testing/tests/Trellis.Testing.Tests.csproj | Adds EF Core provider packages needed by new testing helper tests. |
| Trellis.Testing/tests/ServiceCollectionDbProviderExtensionsTests.cs | Adds tests for ReplaceDbProvider<TContext>. |
| Trellis.Testing/src/Trellis.Testing.csproj | Adds EF Core dependency for the new testing extension. |
| Trellis.Testing/src/ServiceCollectionDbProviderExtensions.cs | Introduces ReplaceDbProvider<TContext> DI helper for integration tests. |
| Trellis.Primitives/tests/MonetaryAmountTests.cs | Adds unit tests for MonetaryAmount creation/arithmetic/JSON behavior. |
| Trellis.Primitives/src/RequiredInt.cs | Updates XML documentation to reflect null-handling/“explicitly provided” semantics. |
| Trellis.Primitives/src/RequiredDecimal.cs | Updates XML documentation wording around null-handling/required semantics. |
| Trellis.Primitives/src/Primitives/MonetaryAmount.cs | Adds the new MonetaryAmount scalar value object implementation. |
| Trellis.Primitives/README.md | Documents MonetaryAmount in the package README. |
| Trellis.EntityFrameworkCore/tests/MaybeMoneyTests.cs | Adds integration tests validating Maybe<Money> optional owned mapping + update guard behavior. |
| Trellis.EntityFrameworkCore/src/MoneyConvention.cs | Adjusts column naming/nullability for Maybe<Money>-created owned navigations. |
| Trellis.EntityFrameworkCore/src/ModelConfigurationBuilderExtensions.cs | Changes convention registration order to allow Maybe<Money> ownership configuration before money column finalization. |
| Trellis.EntityFrameworkCore/src/MaybeUpdateExtensions.cs | Adds guard to prevent ExecuteUpdate helpers on composite owned types (e.g., Money). |
| Trellis.EntityFrameworkCore/src/MaybeModelExtensions.cs | Extends diagnostics to report owned Maybe<T> mappings (e.g., Maybe<Money>). |
| Trellis.EntityFrameworkCore/src/MaybeConvention.cs | Extends MaybeConvention to create optional ownership navigations for owned inner types (e.g., Money). |
| Trellis.EntityFrameworkCore/generator-tests/MaybePartialPropertyGeneratorTests.cs | Adds tests for TRLSGEN100 diagnostic behavior around non-partial Maybe<T> properties. |
| Trellis.EntityFrameworkCore/README.md | Documents Maybe<Money> support and mapping guidance. |
| Trellis.EntityFrameworkCore/NUGET_README.md | Updates NuGet README to mention Maybe<Money> support. |
| Trellis.DomainDrivenDesign/tests/ValueObjects/ValueObjectTests.cs | Adds tests ensuring composite VOs can yield scalar VOs in equality components. |
| Trellis.DomainDrivenDesign/src/ValueObject.cs | Implements non-generic IComparable to support scalar VOs in GetEqualityComponents(). |
| Trellis.Asp/tests/ServiceCollectionExtensionsTests.cs | Updates expected behavior for explicit JSON nulls to collect validation errors. |
| Trellis.Asp/tests/ScalarValueValidationTests.cs | Updates assertions/comments to reflect converter-level null validation behavior. |
| Trellis.Asp/tests/NullAndMissingPropertyValidationTests.cs | Adds comprehensive tests for explicit-null vs missing-property behavior for scalar VOs in JSON. |
| Trellis.Asp/src/Validation/ScalarValueJsonConverterBase.cs | Sets HandleNull => true so converters run on null tokens for reference-type results. |
| Trellis.Asp/src/Validation/PropertyNameAwareConverter.cs | Sets HandleNull => true so wrapped converters run on null tokens for reference-type properties. |
| .github/copilot-instructions.md | Updates repository guidance docs (Copilot instructions) for new EF/VO behaviors and review checklist. |
Comment on lines
57
to
60
| private static readonly Type s_moneyType = typeof(Money); | ||
|
|
||
| /// <summary> | ||
| /// After the model is built, discovers all <see cref="Maybe{T}"/> CLR properties on entity types |
There was a problem hiding this comment.
s_moneyType is declared but never used. Consider removing it (and the using Trellis.Primitives; if it becomes unnecessary) to avoid dead code and keep the convention focused on IsOwned(maybeProperty.InnerType) rather than a special-case field.
Suggested change
| private static readonly Type s_moneyType = typeof(Money); | |
| /// <summary> | |
| /// After the model is built, discovers all <see cref="Maybe{T}"/> CLR properties on entity types | |
| /// <summary> | |
| /// After the model is built, discovers all <see cref="Maybe{T}"/> CLR properties on entity types | |
| /// After the model is built, discovers all <see cref="Maybe{T}"/> CLR properties on entity types |
Comment on lines
+324
to
+326
| int IComparable.CompareTo(object? obj) => | ||
| obj is ValueObject other ? CompareTo(other) : throw new ArgumentException($"Cannot compare {GetType()} to {obj?.GetType()}"); | ||
|
|
There was a problem hiding this comment.
IComparable.CompareTo(object?) should handle null per .NET convention (a non-null instance is greater than null). As written, passing null throws ArgumentException because it falls into the non-ValueObject branch; consider returning 1 when obj is null, delegating when obj is ValueObject, and throwing only for other types.
Suggested change
| int IComparable.CompareTo(object? obj) => | |
| obj is ValueObject other ? CompareTo(other) : throw new ArgumentException($"Cannot compare {GetType()} to {obj?.GetType()}"); | |
| int IComparable.CompareTo(object? obj) | |
| { | |
| if (obj is null) | |
| return 1; | |
| if (obj is ValueObject other) | |
| return CompareTo(other); | |
| throw new ArgumentException($"Cannot compare {GetType()} to {obj.GetType()}"); | |
| } |
Comment on lines
62
to
+70
| /// // Create from existing integer with validation | ||
| /// var result1 = TicketNumber.TryCreate(12345); | ||
| /// // Returns: Success(TicketNumber) if value != 0 | ||
| /// // Returns: Failure(ValidationError) if value == 0 | ||
| /// // Returns: Success(TicketNumber) | ||
| /// // Returns: Failure(ValidationError) if null | ||
| /// | ||
| /// // Create from string with validation | ||
| /// var result2 = TicketNumber.TryCreate("12345"); | ||
| /// // Returns: Success(TicketNumber) if valid integer format | ||
| /// // Returns: Failure(ValidationError) if invalid format or zero | ||
| /// // Returns: Failure(ValidationError) if invalid format |
There was a problem hiding this comment.
The XML example here says TryCreate(int) can fail "if null", but int can’t be null. Null validation applies to TryCreate(int?), while TryCreate(int) can only fail due to range/custom validation (if any). Consider updating the example return comments to reflect the actual overload behaviors.
| /// <para> | ||
| /// Use <see cref="MonetaryAmount"/> when your bounded context operates in a single currency | ||
| /// (e.g., all USD). The currency is a system-wide policy, not per-row data. | ||
| /// Maps to a single <c>decimal(18,2)</c> column in EF Core via <c>ApplyTrellisConventions</c>. |
There was a problem hiding this comment.
The doc comment says this "maps to a single decimal(18,2) column" via ApplyTrellisConventions, but ApplyTrellisConventions only registers a value converter and does not configure precision/scale. Unless there is an additional convention elsewhere, this should be documented as “a single decimal column (provider default; typically decimal(18,2) on SQL Server)” or you should add an EF convention to explicitly set precision/scale for MonetaryAmount.
Suggested change
| /// Maps to a single <c>decimal(18,2)</c> column in EF Core via <c>ApplyTrellisConventions</c>. | |
| /// Maps to a single decimal column in EF Core via <c>ApplyTrellisConventions</c> (provider default precision/scale; typically <c>decimal(18,2)</c> on SQL Server). |
…beConvention Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Non-null instance is greater than null. Previously threw ArgumentException for null because it fell into the non-ValueObject branch. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
MonetaryAmount: TryCreate(decimal?), Parse, TryParse, Multiply(decimal), explicit cast, overflow for Add and Multiply (+12 tests) ReplaceDbProvider: null argument guards and tests (+2 tests) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add static abstract TryCreate(string?, string?) to IScalarValue<TSelf, TPrimitive> for parsing scalar values from string representations. Implementations added for Age (int parsing), MonetaryAmount (decimal with InvariantCulture), and Percentage (decimal with % suffix stripping). All test/example types updated to satisfy the new interface requirement. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…Parse/TryParse - Add IFormatProvider? parameter to TryCreate(string?, string?) on IScalarValue interface - Update all 12 hand-implemented types (Age, MonetaryAmount, Percentage, CountryCode, CurrencyCode, EmailAddress, Hostname, IpAddress, LanguageCode, PhoneNumber, Slug, Url) - For Age, MonetaryAmount, Percentage: use provider in parsing with fallback to InvariantCulture - For string-based types: add parameter but ignore it (string validation is culture-neutral) - Simplify Parse/TryParse on Age, MonetaryAmount, Percentage to delegate to TryCreate - Update source generator (RequiredPartialClassGenerator) to emit new signature - Update ASP source generator (ScalarValueJsonConverterGenerator) for string/non-string types - Update ~50 test stub types across all test projects - Add TryCreateFormatProviderTests with 16 new culture-aware tests Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
New interface extending IScalarValue with TryCreate(string?, IFormatProvider?, string?) for numeric and date VOs where culture affects parsing (decimal separators, date formats). String-based VOs (EmailAddress, etc.) stay on IScalarValue only. Implemented by: Age, MonetaryAmount, Percentage (hand-implemented) + RequiredInt, RequiredDecimal, RequiredLong, RequiredDateTime (source-generated). Source generator fixes: - All TryCreate(string?) overloads now use InvariantCulture consistently (was current-culture, which silently changed parsed values by locale) - DateTime provider overload separates parse failure from MinValue check - Parse/TryParse delegate to TryCreate(string, provider) for formattable types Documentation: copilot instructions, API reference, primitives README, docfx article all updated. 40 new tests including German/French culture parsing for both hand-implemented and source-generated types. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
xavierjohn
force-pushed
the
dev/xavier/rr9
branch
from
389cd64 to
7ca92ba
Compare
Comment on lines
+8
to
+13
| /// A non-negative monetary amount without currency — for single-currency systems. | ||
| /// <para> | ||
| /// Use <see cref="MonetaryAmount"/> when your bounded context operates in a single currency | ||
| /// (e.g., all USD). The currency is a system-wide policy, not per-row data. | ||
| /// Maps to a single <c>decimal(18,2)</c> column in EF Core via <c>ApplyTrellisConventions</c>. | ||
| /// </para> |
There was a problem hiding this comment.
The XML doc claims MonetaryAmount maps to a single decimal(18,2) column via ApplyTrellisConventions, but the EF Core conventions in this repo only register a value converter for scalar VOs and do not configure precision/scale. Unless there is a separate convention elsewhere, this should be reworded to avoid promising a specific store type (provider-dependent) or the EF convention should be updated to explicitly set precision/scale for MonetaryAmount/decimal-based scalar VOs.
Comment on lines
+1213
to
+1214
| // IFormattableScalarValue TryCreate overloadfor culture-sensitive parsing | ||
| source += $@" |
There was a problem hiding this comment.
Typo in the generated-source comment: "overloadfor" should be "overload for".
Source generators run at compile time, not runtime. Coverage tools cannot measure them. Added [ExcludeFromCodeCoverage] to all 3 generator classes: RequiredPartialClassGenerator, MaybePartialPropertyGenerator, ScalarValueJsonConverterGenerator. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…lue and improve parsing methods
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🐛 Bug Fix: Null VO properties silently accepted in JSON deserialization
System.Text.Json's HandleNull defaults to false for reference types, so
ValidatingJsonConverter.OnNullToken() was never called. Properties like RequiredInt set to null in
JSON were silently accepted instead of returning 400. Fixed by adding HandleNull => true to
ScalarValueJsonConverterBase and PropertyNameAwareConverter. (+16 tests)
✨ Feature: Maybe auto-configures as optional owned type
MaybeConvention now detects when T is an owned type (via IsOwned()) and creates an optional ownership
navigation instead of crashing. MoneyConvention configures nullable columns with correct naming. Convention
order swapped (MaybeConvention first). MaybeModelExtensions diagnostics API extended. MaybeUpdateExtensions
guards ExecuteUpdate on composite VOs with clear exception. (+13 tests)
✨ Feature: MonetaryAmount scalar value object
Single-currency monetary amount — maps to 1 decimal column (vs Money = 2 columns). Non-negative, rounds to 2
decimal places, arithmetic with overflow protection, invariant culture JSON. Hand-implemented
ScalarValueObject following Percentage pattern. (+15 tests)
✨ Feature: ReplaceDbProvider testing helper
Extension method on IServiceCollection that cleanly swaps EF Core database providers in
WebApplicationFactory tests. Removes all EF Core internal services generic over TContext (including
IDbContextOptionsConfiguration in EF Core 10). (+6 tests)
✨ Feature: ValueObject implements IComparable
Fixes composite ValueObjects (e.g., ShippingAddress) that yield return scalar VOs in GetEqualityComponents()
. Without this, yield return Street; caused CS0266. (+2 tests)