Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Codec: Fix EventId, Require timestamp #84

Merged
merged 4 commits into from
Sep 6, 2022
Merged

Codec: Fix EventId, Require timestamp #84

merged 4 commits into from
Sep 6, 2022

Conversation

bartelink
Copy link
Collaborator

No description provided.

@bartelink
Copy link
Collaborator Author

bartelink commented Sep 5, 2022

@nordfjord This exposes the ctors for EventData and TimelineEvent (which I'll migrate Equinox to use in order to make as many field mappings as possible explicit)

Added characterization tests for the unwritten 'obvious' spec of EventData/TimelineEvent.Create


Regarding the suggestions you had for changing mapCausation result type...

Right now each of the Codec.Create overloads progressively removes options from the table.

I was considering removing the Guid, but not sure that'd serve your usage needs directly anyway, while also being a breaking change)?

The basic thinking for the mpaCausation signature is that any of a) the Event b) the Meta c) Context might be used to derive a deterministic value for the EventId guid. Having that be present in the signature is intended to convey those (potential) relationships.

Now would definitely be a good time to add an overload (and/or tweak a signature, if that's ever to be done) if we can come up with a useful one (and, ideally, get it seconded by someone else that's given this some thought and/or used it in a real system)

@nordfjord
Copy link
Contributor

These changes look good to me!

Definitely appreciate the change to make the timestamp required in the bottom codec constructor.

Regarding the suggestions you had for changing mapCausation result type...

For the record (the suggestion is on the discord, but might be good to have it here), my suggested overload was:

static member Create<'Contract, 'Context when Contract :> TypeShape.UnionContract.IUnionContract>(
  getCausation,
  ?options,
  ?rejectNullaryCases
) =
  let up = snd
  let down event = event, None, None
  let mapCausation (ctx,_) =
    let causation, correlation = getCausation ctx
    ctx, (Guid.NewGuid()), causation, correlation
  FsCodec.Core.Codec.Create(mkEncoder options, up, down, mapCausation, rejectNullaryCases)

My preference would be not needing getCausation at all but rather to chuck the context into meta by default.

This is based on the context I'm in a the moment where 3 things hold true

  1. My decisions are made in a context
  2. My store doesn't have special handling of correlation and causationId's
  3. I don't have event specific metadata except in very rare cases

When the above hold true then by default I create a codec like this:

let codec =
  FsCodec.SystemTextJson.Codec.Create<Event,Event,Context,Context>(
    snd,
    (fun ev -> ev,None,None),
    (fun (ctx, _) -> ctx,Guid.NewGuid(),"","")
  )

Which is very close to what the proposed overload does. Hope this helps 🙏

@bartelink bartelink merged commit dd205ba into master Sep 6, 2022
@bartelink bartelink deleted the fix-eventid branch September 6, 2022 14:27
@bartelink
Copy link
Collaborator Author

bartelink commented Sep 6, 2022

Thanks for sharing the full context here - hopefully things will eventually converge...

When the above hold true then by default I create a codec like this ...

You're not wrong about it being ugly - I'd consider making it more explicit by actually using the lower level overload https:/jet/FsCodec/blob/master/src/FsCodec.SystemTextJson/Codec.fs#L47 like so:

 FsCodec.SystemTextJson.Codec.Create<Event,Event,Context,Context>(
    (fun (_raw, e) -> e),
    (fun (ctx, e) -> e, ValueSome ctx, Guid.NewGuid(), null, null, DateTimeOffset.UtcNow
  )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants