Comparative operations on ValueObjects

[rbanks54]

While testing the analyzer for #684 I was playing around with various queries and I tried this (using the Onion sample code).

context.Entities.Where(e => e.Name.StartsWith("Fred")).Where(e => e.Id >= id).ToList();

The query fails to compile because the .StartsWith() method and the >= operator don't exist on the Name or Id properties. To get the query to work, I had to cast those properties to their base types.

context.Entities.Where(e => ((string)e.Name).StartsWith("Fred")).Where(e => (int)e.Id >= id).ToList();

That said, this isn't really a discussion about EF. It's more a discussion about collection filtering in general. For example, it would be a fairly common use case that a domain service will need to filter a collection of entities in some way.

Should we look at extending the source generator to hoist the methods/operators from the base classes we need to better support collection filtering scenarios?

[SteveDunn]

Thanks for starting this discussion, Richard!
I agree that it's a common scenario, and it's certainly confusing/frustrating for someone wanting to use Vogen in EFCore.

Re. the operators: here's a discussion about those. But the context there was mostly different; it was related to removing CA warnings about types that implement IComparable should also define operators <, > etc. (that's been fixed with 'suppressor'). The current situation regarding operators is that it is possible to implement since the invention of static abstracts in interfaces (IComparisonOperators, IAdditionOperators etc.), but wouldn't necessarily make sense in a value object (at least outside of the EFCore scenario).
Why wouldn't it make sense? The example in that thread showed how a += operator would be useful on a Score. But in domain terms, what happens with a score is that it's increased by a certain amount of points, so instead of the primitive use of score += 10, we'd have score = score.IncreaseBy(10).

There's a trade off between pure 'domain concepts' and making value objects behave nicely in the infrastructure layer.

[rbanks54]

The suggestion was triggered by the EF testing, but it's not an EF specific challenge :)

To me, being able to compare value objects that wrap primitives seems like a reasonable expectation and makes code easier to read and understand.

Let me try with example from the Onion sample where an Employee has a HireDate.

Assume I have a domain service that is passed a collection of employees. Within the service I need to find employees who joined this year. I'd then want to write a linq query over the employee collection as follows:

var startOfYear = new DateOnly(TimeProvider.System.GetLocalNow().Year, 1, 1);      
var selection = employees.Where(e => e.HireDate >= HireDate.From(startOfYear)).ToList();

The >= operator is the natural comparison approach. Without that operator, I'm forced to use CompareTo(), which looks like this:

var hireDate = HireDate.From(new DateOnly(TimeProvider.System.GetLocalNow().Year, 1, 1));
var selection = employees.Where(e => e.HireDate.CompareTo(hireDate) >= 0).ToList();

It works, but it's cumbersome and much harder to read.

Let's say the service also needs to check for employees nearing retirement age. We'd have a similar scenario:

var retiringEmployees = employees.Where(e => e.Age.CompareTo(Age.From(64)) >= 0).ToList();

or I can drop down to using .Value and just compare against the underlying primitive:

var retiringEmployees = employees.Where(e => e.Age.Value >= 64).ToList();

Not having the greater/less than style comparison operators makes things awkward, or forces people to think about the underlying primitive and use that instead.

In the case of string ValueObjects, I think exposing the underlying strings-specific comparison methods (contains, startsWith, etc.) would also make sense. Yes, I can always add them to each and every value object, or I could also drop down to using .Value.Contains("xyz"), but it feels like extra work I don't need to do if the source generator could make it easier for me.

Hopefully that makes it a little clearer :)

P.S. I agree with the approach regarding the += operator, but that's not a comparison operator :)

[SteveDunn]

I think it's clear that these would be useful. The next question is:

What is the best way to specify what you want hoisted?

Currently, we have two attributes that control what's generated, VogenDefaults for assembly-wide (global) configuration, and ValueObject for per-type (local) configuration. Settings in the local configuration overrides the global configuration.

Before generating a value object, both the local and global configs are merged into this structure:

    public static readonly VogenConfiguration DefaultInstance = new(
        UnderlyingType: null,
        ValidationExceptionType: null,
        // ReSharper disable once RedundantNameQualifier
        Conversions: Vogen.Conversions.Default,
        Customizations: Customizations.None,
        DeserializationStrictness: DeserializationStrictness.Default,
        DebuggerAttributes: DebuggerAttributeGeneration.Full,
        Comparison: ComparisonGeneration.UseUnderlying,
        StringComparers: StringComparersGeneration.Omit,
        ToPrimitiveCasting: CastOperator.Explicit,
        FromPrimitiveCasting: CastOperator.Explicit,
        DisableStackTraceRecordingInDebug: false,
        ParsableForStrings: ParsableForStrings.GenerateMethodsAndInterface,
        ParsableForPrimitives: ParsableForPrimitives.HoistMethodsAndInterfaces,
        TryFromGeneration: TryFromGeneration.GenerateBoolAndErrorOrMethods,
        IsInitializedMethodGeneration: IsInitializedMethodGeneration.Generate,
        SystemTextJsonConverterFactoryGeneration: SystemTextJsonConverterFactoryGeneration.Generate,
        StaticAbstractsGeneration: StaticAbstractsGeneration.Omit,
        OpenApiSchemaCustomizations: OpenApiSchemaCustomizations.Omit,
        ExplicitlySpecifyTypeInValueObject: false,
        PrimitiveEqualityGeneration: PrimitiveEqualityGeneration.GenerateOperatorsAndMethods);

I personally find it quite cumbersome to use these, and the granularity is often too narrow or too broad. What would be nice is to have a .vogen file that specifies config, even if it's just config related to hoisting, something like:

"M:string.StartsWith();" // Method on string
"I:int.ComparisonOperators" //Interface on int

But this could quickly get out of control and confusing, for instance, if the string.StartsWith is part of an interface on string and both the interface and method are specified. And users might want all the interface members, except certain ones, but not have the value object implement that interface.

Having typed out my thoughts, I now think it might be easier to have a general flag that says ImplementComparisonOperators and have a set of pre-defined operators on pre-defined types, so that'd be (off the top of my head)

Strings

• Contains, StartsWith, EndsWith

Numbers (or anything that implements IComparisonOperators<,>

This includes < <= > and >=
... for any of these types:

---

Conflicts

Some users want their types to be opaque. For example, if an ID is an arbitrary number and not sequential, then it makes no sense to expose comparison operators (other than == of course). One of the flags in Vogen configuration is this:

    public enum ComparisonGeneration
    {
        /// <summary>
        /// Use the default, which is <see cref="UseUnderlying"/>. This entry generally isn't used but is present
        /// to signify a default value which has not been specified in the attributes. 
        /// </summary>
        [EditorBrowsable(EditorBrowsableState.Never)]
        Default,

        /// <summary>
        /// Omits the IComparable interface and implementation. Useful for opaque types such as tokens or IDs where comparison
        /// doesn't make sense.
        /// </summary>
        Omit,
        
        /// <summary>
        /// Uses the default IComparable from the underlying type.
        /// </summary>
        UseUnderlying,
    }

This relates to types who's underlying type implements IComparable. This only covers the CompareTo method, so if the user wants to omit this, they'd also want to omit the comparison operators. Actually, at this point, I'm glad I named this to be less specific that just IComparisonGeneration, as the Omit can now cover the functionality from IComparable and IComparisonOperators<>

Comparison vs. Equality

Vogen currently has a config flag to determine if equality to the primitive is generated:

/// <summary>
/// Defines if equality operators to the underlying primitives are generated.
/// </summary>
[Flags]
public enum PrimitiveEqualityGeneration
{
    [DebuggerBrowsable(DebuggerBrowsableState.Never)]
    Unspecified = -1,

    /// <summary>
    /// Do not generate.
    /// </summary>
    Omit = 0,

    /// <summary>
    /// Generate equals operators for primitives.
    /// </summary>
    GenerateOperators = 1 << 0,

    /// <summary>
    /// Generate equals methods for primitives.
    /// </summary>
    GenerateMethods = 1 << 1,
    
    /// <summary>
    /// Generate both operators and methods.
    /// </summary>
    GenerateOperatorsAndMethods = GenerateOperators | GenerateMethods
}

I think this is slightly different to what we're proposing here. The generated code for this enables things such as MyId == 123, where the comparison is between the value object and the primitive.
What we're proposing here are comparisons between two value objects (yes, they'll use the underlying's implementation, but we're still comparing apples and apples)

Summary

I think these comparisons are useful, and I think we need a new config flag and enum. I think that switching to a .vogen file-based config will be confusing.

[rbanks54]

Reading through things (nicely expressed, btw!) wouldn't changing the ComparisonGeneration.UseUnderlying option to generate both the IComparable implementation and the relevant IComparisonOperators or string methods be sufficient? It's an additive change that fits with the concept of comparison generation, and it's a change that shouldn't break any existing code. It also keeps the configuration options the same as they are now.

If people want to skip them to keep their code lighter, having an OmitOperatorsAndMethods option could be useful.

For the string methods, I think the 3 you mentioned should be sufficient. String identifiers with meaningful prefixes/suffixes are pretty common and those 3 methods should give people enough flexibility.

P.S. Thanks for staying away from the config file approach!