Relaxing requireRecordFields = true

[dharmaturtle]

Hi!

Would you be interested if I opened a PR to relax requireRecordFields = true?

FsCodec/src/FsCodec.NewtonsoftJson/Codec.fs

Lines 84 to 88 in 8f765f7

	// For now, we hard wire in disabling of non-record bodies as:
	// a) it's extra yaks to shave
	// b) it's questionable whether allowing one to define event contracts that preclude adding extra fields is a useful idea in the first instance
	// See VerbatimUtf8EncoderTests.fs and InteropTests.fs - there are edge cases when `d` fields have null / zero-length / missing values
	requireRecordFields = true,

FsCodec/src/FsCodec.SystemTextJson/Codec.fs

Lines 54 to 56 in 8f765f7

	// Round-tripping cases like null and/or empty strings etc involves edge cases that stores,
	// FsCodec.NewtonsoftJson.Codec, Interop.fs and InteropTests.fs do not cover, so we disable this
	requireRecordFields = true,

To address your comments:

a) it's extra yaks to shave

I don't disagree :) However, I am willing to shave this yak.

b) it's questionable whether allowing one to define event contracts that preclude adding extra fields is a useful idea in the first instance

Many of my events simply contain a primitive, e.g.

type BranchId = Guid<branchId>
and [<Measure>] branchId

type DefaultBranchUpdated = { BranchId: BranchId }

type Event =
    | DefaultBranchUpdated of DefaultBranchUpdated

It would simplify my code (and make it more implementation-agnostic) if I could refactor the above to

type Event =
    | DefaultBranchUpdated of BranchId

See VerbatimUtf8EncoderTests.fs and InteropTests.fs - there are edge cases when d fields have null / zero-length / missing values

I'm not sure what a d field is. I'm currently only interested in relaxing the rules to include Guids, but I could easily see this extending to other primitives.

Round-tripping cases like null and/or empty strings etc involves edge cases that stores, FsCodec.NewtonsoftJson.Codec, Interop.fs and InteropTests.fs do not cover, so we disable this

I'm totally okay with excluding string.

---

I got this far with the implementation before realizing "I should probably ask first." >_>

        let shape =
            match shapeof<'Contract> with
            | Shape.FSharpUnion (:? ShapeFSharpUnion<'Contract> as s) -> s
            | _ ->
                sprintf "Type '%O' is not an F# union" typeof<'Contract>
                |> invalidArg "Union"
        let isAllowed (scase : ShapeFSharpUnionCase<_>) =
            match scase.Fields with
            | [| field |] ->
                match field.Member with
                | Shape.FSharpRecord _
                | Shape.Guid _ -> true
                | _ -> false
            | _ -> false

[bartelink]

Sorry, I read this in notifications at the weekend and then managed to forget about it...

Firstly, thanks for taking the time to write, (and for realizing asking was the way to go; I've often gone deeper into rabbit holes before thinking!)

The issue is that its important for the value that FsCodec produces
a) roundtrips - there's not much point otherwise obviously
b) ideally, roundtrips in a clean way on concrete stores

Its the second bit that gets interesting:-

• For EventStore versions up to and including 5, the storage is a byte[] with no constraints on what goes in, Data can be null
• For SqlStreamStore, I think Data can not be null, but the impl works around that by putting in an empty string
• For EventStore >= 20 (new year based release naming), the Data can be null. IIRC Data is not allowed to be null
• For CosmosDB, the Data is stored in a d field in the document (that's what the comment cryptically refers to, feel free to improve it to be more explicit). That value must be valid JSON, so a string, null, or a JSON object with { and } wrapping. There are integration tests in Equinox.Cosmos / Equinox.CosmosStore that might complain if you put in those values, but also hold the key to supporting this

We def need the following to be supported in all cases:

• nullary cases - i.e. DU cases with no args - this is allowNullary cases - IIRC it is. Ideally this would be covered by the integration tests for each store in Equinox roundtripping such a value
• record cases - this is the bread and butter, and in F#5 with anonymous records is not a major pain to do

I am not against providing a way for people to use string, bool and/or other roundtrippable values, but the constraints are:

• each store should do something clear with them; ideally roundtripping no matter what (throwing NotSupportedException is at the edge of that, more cryptic is not fair on users)
• each store should have tests demonstrating what they do (if only to demonstrate that you dont get a cryptic exception)

So, unless we are talking something more limited - having FsCodec provide this as something that doesn't default to on, and you still want to do it, I'd ask that you integrate it with at least one of the stores in Equinox, and I'll do the others (unless you want to do them all, I'd be delighted if you did).

If its just a matter of exposing the option for some usage outside of Equinox, then providing it as a not-on-by-default mode would make more sense.

The final point is that supporting single field values like this is arguably a trap when it comes to versioning contracts....

If you have a record { value : string }, then. it will render as {"value": "abc"} and we can go more ways if we need to adjust things, while still leaving it roundtrippable:

• add: { value : string; count: int option }
• rename and/or otherwise version: { value : string option; fancyValue : NewRepresentation option }

... forcing people to use records leaves people in a better place from that perspective. Having said that, changing data contracts for events is generally something to avoid - in general minting a new EventType (i.e. "myEvent-2") and then having an upconversion from the old to the fresh contract is a better approach

[dharmaturtle]

I added a small test (and domain) in this commit.

I'm not PRing that branch, it just serves as a scratchpad for now. Just seeking feedback before making a full-fledged PR.

Here's the only (real) change in FsCodec:

I'll make it more robust, of course, just including it so you can see what's up with the fsproj reference.

[bartelink]

That seems a reasonable test

On the FsCodec side, the best thing to do is probably to add a new, optional ?requireRecordFields that defaults to true. You can add notes attached to that arg i.e. "NOTE: its generally recommended to use records as they offer more flexibility in terms of handling versioning cleanly."
That's also where the "NOTE: While this mode is supported on Equinox.EventStore, it is not yet supported on other stores" would go ;)

Do you envisage replicating the test for the other stores too? (if you even get it basically working I'll run it against Cosmos if you don't have one to hand - unfortunately running against the emulator and/or an ephemeral Cosmos Container is not yet rigged in the build environment)

The test case looks good, but I think I'd lose the 'snapshot' stuff as it dilutes the point of what you're doing? (will expand on that when there is an actual PR)

[dharmaturtle]

By 'snapshot' stuff do you mean 'Initial/Active/Activate'? I wasn't sure how else to represent the start of a stream x|

[bartelink]

Yes, just saying you can prove the point for the purposes of the test case with less moving parts in the code - this is only a tiny nit point though

We can finesse it in the context of the Equinox PR.

[dharmaturtle]

The final point is that supporting single field values like this is arguably a trap when it comes to versioning contracts....

If you have a record { value : string }, then. it will render as {"value": "abc"} and we can go more ways if we need to adjust things, while still leaving it roundtrippable:

• add: { value : string; count: int option }
• rename and/or otherwise version: { value : string option; fancyValue : NewRepresentation option }

... forcing people to use records leaves people in a better place from that perspective.

Upon further thought, I think I can implement this feature in such a way that it upconverts well. Disregarding what's currently implemented in the linked PR, I might be able to create an (anonymous?) record at runtime (via reflection) to wrap the primitive/nonrecord type. Or, a simple dictionary with a single key/value might work. The (key) name can be provided by an attribute like so:

type DuFieldNameAttribute (DuFieldName : string) =
    inherit Attribute ()
type Event =
    | [<DuFieldName("BranchId")>] DefaultBranchChanged of Guid

(Unfortunately, it doesn't seem like we can put attributes on a du's fields.)

The above event would then be serialized to

{
   "BranchId":"6C96B774-B4EC-4E19-925C-EFEC5EE6DC5D"
}

In addition to the upconversion advantage, this type of implementation should be able to handle any type - not just selected primitives. My own project uses Nodatime - it would be nice if I could pass around a bare Instant without having to wrap it in a record.

A disadvantage is that serialized events will be larger.

[bartelink]

Interesting.

For | [<DuFieldName("BranchId")>] DefaultBranchChanged of Guid you can name it via | DefaultBranchChanged of branchId : Guid - all you'd have to do is complain about unnamed ones (that I think default to Item ?). This can be accessed via FSharp.Reflection (The UnionConverter should demonstrate the basic technique). This does become something that the validator can (but also needs) to detect - until that's done you risk having to throw a runtime exception, which is not a great experience. (Also, in general custom attributes are to be avoided unless there's really no other way; they're difficult to discover and/or you'd need to open FsCodec etc)

Perhaps the upconversion mode could be represented by passing something like , autoWrap = true (and leaving requireRecordFields to its default: true) to Create() ?

The size of events definitely doesn't matter.

Regarding implementation technique, the Dictionary seems the most tempting, though a dynamically generated Struct Record would be a touch more efficient in terms of allocations (but you'd be having to dynamically name the field too)

[dharmaturtle]

I think I need to modify the way TypeShape implements mkFieldEncoders. Since I'm doubtful that Eirik would accept a PR for this admittedly edge case, how do you feel about copy/pasting a custom implementation of the Impl module to FsCodec?

[bartelink]

Hm, A large part of the value of UnionContract for me is that it lives elsewhere, and has clear docs, so I'd be reluctant.

If you can distil the extension point you'd need in there to achieve your ends, I wouldn't rule out Eirik being amenable to it.

More importantly, if you can show a prototype impl or explain the nature of a change you need, he may be able to suggest a better approach (I personally don't have an implementation approach of any kind jumping into my head!).

I'm also struggling to remember the evolution and reasons why Eirik moved to majoring on unions - IIRC UnionEncoder once supported [named] tupled cases. IIRC he may have cut that feature due to usability concerns (people running into trouble making hard to version contracts due to having unnamed tuple cases which default to Item1 and Item2?). @eiriktsarpalis Can you recall the reasoning? Can you infer what @dharmaturtle is suggesting and rule it in or out, or do you need more context on what's needed?

[dharmaturtle]

We can inject a custom IEncoder<'Format> into mkFieldEncoders, which is groovy. The encoder can then do something like (duPayload.GetType() |> FSharp.Reflection.FSharpType.IsRecord) to determine if it should be serialized as normal, or if it should be thrown into a dictionary or (anonymous) record and then serialized. The decoder does the opposite.

However, the encoder isn't passed sfield.Label, which I need to build the dictionary or (anonymous) record. Without that, I can't customize the "key" name. I think the simplest thing would be a way to inject a custom IMemberVisitor.

[dharmaturtle]

To be even more concrete, here's a commit with a proof of concept: dharmaturtle/TypeShape@43c9788

[bartelink]

Nice - Well now, you can link to it in a question issue on the TypeShape repo ;)

(I have not read enough TypeShape in recent times to meaningfully discuss it and/or comment on whether a cleaner solution exists)

[bartelink]

Well, you have an answer now ;P

While part of me likes the idea of tuples and/or single field union cases as an on-ramp to records, like Eirik, I've seen the mess that can be caused too, and things do get hairy with converters etc (you only need to look at the fun of implementing FsCodec's UnionConverter for STJ and Newtonsoft to know its not something you want to get into if you can avoid)

Overall the actual problem is I'm underwhelmed by the prospect of lumbering FsCodec maintainers (while that's presently me, that's more by accident than by design) maintaining a fork of UnionContractEncoder for the foreseeable future.

And a really bad throwaway idea - one could probably make a converter that lifts fields (inc tuples) to a record using JsonIsomorphism, i.e.
type X = Case1 of [<JsonRec>] x :int | Case2 of [<JsonRec>] x: int * y: int Wait, that would get really messy with System.Text.Json, forget I said anything!

Have you tried using anonymous unions in your consumption scenario? (IME you typically end up just defining a record type in the end, but it does enable neat succinct modelling on the fly as you type)

[dharmaturtle]

I completely agree with Eirik and definitely don't want to add to your workload for honestly what is a cosmetic improvement. As such I'm closing this issue.

[dharmaturtle]

Returning to a different topic...

I think I'd lose the 'snapshot' stuff

The State of my stream is typically a DU with 3 cases:

module Events =
    type Snapshot = // lots of stuff


module Fold =
    type State =
        | Initial
        | Active of Events.Snapshot
        | Deleted

This is a pattern that I made up. Is there a better way to model the state of a stream/entity? I expect the answer is "it depends on your domain", but virtually all examples in Equinox set Fold.initial to empty list/array/set or a record whose fields are 0/null/false.

Is Initial/Active/Deleted a "smell"?

[bartelink]

Good question!

While that pattern can work, I'd be careful with it, and not let it drive my design...

Firstly, while snapshots are free and/or a good idea in Equinox.Cosmos, they're generally something you want to avoid in ESDB for as long as you can. Even for that though, I'd be cautious about always doing one - in V3 for short streams, it will all be in one document anyway and hence be obtained by one point read anyway (and for ESDB touching a stream that's warm is insanely cheap and efficient anyway)

The FsCodec UnionConverter can also be used to serialize your entire state (inc Deleted vs Active in your example above)

However, I'd tend to be very careful about having your model to be constrained to be something that must be able to be serialized directly as yours does

Better to force yourself to not actually model stuff in your type State without serialization concerns in your module Fold, and then have a

module State =
    let ofSnapshot (s : Events.Snapshot) : State = function ... 
    let toSnapshot (s : State) : Events.Snapshot) = function ...

The ofSnapshot can be used in your evolve impl `

---

Having said all that, that general technique can work well for CRUDdy aggregates and lets you get that done without introducing new tech. The most obvious problem with it is that you will often end up adding option fields to your Events.Snapshot as things evolve, vs considering actually saying "no, there is a new state here and I should do this properly"

I personally have never allowed myself to use the Snapshot event as a type in my state

---

As usual with these things, I'd say the best thing is to get another set of eyes on the aggregate and/or the events. Sometimes things are just CRUD and not the interesting stuff where there's actually value in doing Real Modelling. Consider putting something more real on the DDD-CQRS-ES Slack if you can, or DM me, because the real answer is obviously It Depends, until its clear what the real use case is!

[bartelink]

Damn, forgot to hit Comment on this earlier...

I completely agree with Eirik and definitely don't want to add to your workload for honestly what is a cosmetic improvement. As such I'm closing this issue.

Cool; I'm glad you took the time to explore rabbit hole nonetheless - time spent on pushing polish stuff like this is never entirely wasted IME.