Simplify value object infrastructure.fs in samples

[chinwobble]

In the sample project we have SkuId as a value object implemented as a class hat inherits a Comparable

/// Endows any type that inherits this class with standard .NET comparison semantics using a supplied token identifier
[<AbstractClass>]
type Comparable<'TComp, 'Token when 'TComp :> Comparable<'TComp, 'Token> and 'Token : comparison>(token : 'Token) =
    member private __.Token = token // I can haz protected?
    override x.Equals y = match y with :? Comparable<'TComp, 'Token> as y -> x.Token = y.Token | _ -> false
    override __.GetHashCode() = hash token
    interface IComparable with
        member x.CompareTo y =
            match y with
            | :? Comparable<'TComp, 'Token> as y -> compare x.Token y.Token
            | _ -> invalidArg "y" "invalid comparand"

/// SkuId strongly typed id
[<Sealed; JsonConverter(typeof<SkuIdJsonConverter>); AutoSerializable(false); StructuredFormatDisplay("{Value}")>]
// (Internally a string for most efficient copying semantics)
type SkuId private (id : string) =
    inherit Comparable<SkuId, string>(id)
    [<IgnoreDataMember>] // Prevent swashbuckle inferring there's a "value" field
    member __.Value = id
    override __.ToString () = id
    new (guid: Guid) = SkuId (guid.ToString("N"))
    // NB tests (specifically, empty) lean on having a ctor of this shape
    new() = SkuId(Guid.NewGuid())
    // NB for validation [and XSS] purposes we prove it translatable to a Guid
    static member Parse(input: string) = SkuId (Guid.Parse input)
/// Represent as a Guid.ToString("N") output externally
and private SkuIdJsonConverter() =
    inherit JsonIsomorphism<SkuId, string>()
    /// Renders as per Guid.ToString("N")
    override __.Pickle value = value.Value
    /// Input must be a Guid.Parseable value
    override __.UnPickle input = SkuId.Parse input

Can this be simplified to a record type like this?

[<JsonConverter(typeof<SkuIdJsonConverter>)>]
type SkuId = private { [<IgnoreDataMember>] Value: Guid } with
    static member Create (value: string): SkuId =
        if (String.IsNullOrWhiteSpace(value)) then
            invalidArg "value" "cannot be whitespace"
        { SkuId.Value = Guid.Parse(value) }
type SkuIdJsonConverter() =
    inherit JsonIsomorphism<SkuId, string>()
    override __.Pickle value = value.Value.ToString("N")
    override __.UnPickle input = SkuId.Create(input)

[bartelink]

Firstly, apologies for not taking the time to write a shorter letter, and please keep good questions like this coming!

In general, you're correct that this would technically work; For consistency, the following microtweaks would be recommended:

• may need ToString override for cases where it gets rendered that way (obviously it would be ideal if nobody actually leant on that)
• reinstate StructuredFormatDisplay so sprintf %A works
• reinstate new() and new(Guid) (latter can be a matter of renaming Create)
• I don't like special casing whitespace or empty and would prefer just a simple null check

Reformatting:

• Value: Guid -> value:Guid (I know that's not CLSCompliant, but for better or worse that's Jet house style and something I'd use even outside as a) its [subjectively] prettier b) it means you dont need to use json.net camelCase conversion semantics)
• if String.IsNullOrWhiteSpace value then (outer parens are not idiomatic F#, inner ones are subjective but can be omitted where one arg)
• { value = Guid.Parse value } (In general, I avoid prefixing SkuId.)

However the more fundamental issue is that, when running larger Sets of data in the SavedForLater sample, the difference in representation between a 128bit value type and a 32/64 bit reference type is significant to the degree that it produces a noticeable difference in perf running the tests, as alluded to by // (Internally a string for most efficient copying semantics) (I have not personally verified this independently but am pretty sure.) I'm not sure whether that's down to more efficient GetHashCode, Equals, Compare or copying semantics (probably not the last one as AIUI that does not come into play)

So... for things like ClientId in the samples, which are not heavily used in lists or Sets, the simplest and most most direct code definitely wins, but for SkuId, these other concerns come into play.

For the other types (i.e. aside from SkuId) in https://github.com/jet/equinox/blob/master/samples/Store/Domain/Infrastructure.fs - a PR to simplify the other types (as long as it considers the above factors) would be of interest. It would likely also propagate to the F# template and C# template (Hm, I forgot the IgnoreDataMember there 🙍‍♂️ UPDATE: Fixed, simplified the C# - it's effectively your impl now)

[bartelink]

@chinwobble I hope you'll be happier with #89 - I tried hard to go down the record route, but ultimately not being able to customize constructors etc becomes painful (i.e. having to make a static Create all the time)

The other benefit is that this approach translates better to C#, and hence hopefully people's understanding when scanning what should be a boring set of types.

Definitely thanks for pushing though, this wouldnt have happened without your nudge so much appreciated.

(I'll apply these changes to the templates when the dust settles - its not too late to provide more feedback!)

[chinwobble]

@bartelink Thanks for your effort!

Its ashame that these values objects can't be more succinctly represented in the language.

[bartelink]

After some pushing it around this morning with Eirik, I reached a conclusion (out of band) that they can and should be.

The reason we ended up with the Requestid and CartId still being custom types is to represent a case where
a) data is stored rendered as a Guid but without a consistent format that one knows to be string-comparable (meaning they need to be canonicalized by parsing from a Guid)
b) we want and need to represent the data as a string from a domain modelling perspective in order to be able efficiently compare/hash/sort them (meaning they have to be strings)

In the original implementation, we'd conflated the canonicalization need with the the storage need. As a result, there's a need to ensure the canonicalization and conversion takes place at the time of (de)serialization.

The resolution is that we'll recommend in any such case to bring in an intermediate/upconversion layer to the decoding step:-

module Events =
    type [<UnitOfMeasure>]cartId
    module CartId =
        let (|Canonical|) s = let g = Guid.Parse s in g.ToString("N") |> UoM.tag<cartId> 

    type [<UnitOfMeasure>]skuId
    module SkuId =
        let (|Canonical|) s = let g = Guid.Parse s in g.ToString("N") |> UoM.tag<skuId> 

    type Event = CartItemAdded
    and CartItemAdded = { cartId: string<cartId>; sku: string<skuId> } 

    module Raw =
        type Event = CartItemAdded
        and CartItemAdded = {cartID : string; sku_id: string }

        let upconvert : Raw.Event -> Events.Event = function
            | CartItemAdded { cartID = CartId.Canonical cart; sku_id = SkuId.Canonical sku } -> { cartId = cart; sku = sku }

    let tryDecode = encoder.tryDecode<Raw.Event> >> Option.map 
Raw.upconvert

The normal case will be that a strongly typed e.g. string<cartId> is used, with initialization and/or deserialization assumed to result in a canonicalized value, and rendering being simple blitting out as a string. While in some cases this is imperfect as there's always an allocation, its a win for expressiveness and succinct code, and for general perf in the case where the value needs to go in a dictionary (e.g. for skus).

This upconversion / Anti Corruption Layer slots in with stuff one'd do in line with requirements in various cases discussed in https://leanpub.com/esversioning

The other thing this ties in with is that for json-encoded schemes, even if something is technically a Guid, that's not a very idiomatic way to encode such data (especially when the expectation is that it represents a natural identifier), so a string is just fine. The thing that needs to be borne in mind by those writing any anticorruption layer and/or binding later externally is that one should do a length sanity check and/or xss check before generating a string<skuId>. In practice the quick way to do that is to represent as a Guid in the binding layer and then map to the domain model by doing Guid.Parse >> UnitOfMeasure.tag<skuId>.

Finally, best of all, the project for the strongly typed ids has been moved to fsprojects:- https://github.com/fsprojects/UnitsOfMeasure.Extra - when the NuGet arrives, I'll do the port, which will make most of the accidental confusing junk you so rightly called out melt away in the dotnet new template, where it's most offensive (it'll still stay for the C# of course ;) )

[bartelink]

Fixed in release 1.0.4

• master uses FSharp.UMX via Clean up Identity Types #89
• pending work in https://github.com/jet/equinox/tree/idtypes-extra might remove SkuId boilerplate eventually (it involves me having a fight with FsCheck)
• dotnet new eqxweb also has the changes as of https://github.com/jet/dotnet-templates/releases/tag/1.2.0

[bartelink]

thanks for the nudge @chinwobble - I'll let you close at your leisure

[bartelink]

Closing for age - thanks again; feel free to comment or add notes when you review @chinwobble