Closed SurajGupta closed 6 years ago
I think this is a worthwhile piece of functionality to add. My main comment is that we should do it in a way that doesn't break backwards compatibility.
I would think it should be possible to use the serialize and deserialize delegates as facades to the NewtonSoft.Json serialization methods by default, so that an explicit call to UseDefaultSerialization
isn't necessary. This would also probably simplify some of the null checking in the serialization methods.
Another approach might be to allow registration of serializers per aggregate type or per event type in the internal PocketContainer
which might be a bit more flexible overall.
Good idea! Let me give it a shot and add to the PR...
Ok, no one will be broken now =) No need to explicitly set default serialization.
Also, I tried to push-down the usage of the funcs as much as possible, but was limited by the existing contracts. Note...
Serializer.DeserializeEvent
is a public method so I have to keep the serializerSettings
parameterEventExtensions.ToDomainEvent
is a public method so I have to continue passing serializerSettings.Value
to Serializer.DeserializeEvent
EventExtensions.ToSotrableEvent(IEvent)
has been reverted to the original because it isn't called by SqlEventSourcedRepository
. None of the callers pass-in a serializationFunc
EventExtensions.ToStorableEvent<TAggregate>
is internal and only has only one caller - SqlEventSourcedRepository
. So I could have pushed the JsonConvert
call back into SqlEventSourcedRepository
and relied solely on the serializationFunc
. However I would have to duplicate the Lazy<JsonSerializerSettings> serializerSettings
which clones the settings and then swaps-out the ContractResolver. That felt wrong and I figured if that were broadly reuseable code it would have already been in Serializer.cs
Questions
I don't think we need UseDefaultSerialization
.
I was able to push the coalesce construction of serialize
all the way back to the repository construtor and guarantee that it won't break anyone. For deserialize
, I am also doing the coalesce construction in the repository constructor. However, I must continue to perform the coalesce inside Serializer.DeserializeEvent
otherwise I will break callers. While this could be fixed for all other Its.* callers, this method is public and so some outside consumer is expecting that that method does what it did before. So unless we're willing to break outside consumers, I think we need to continue to take serializationSettings
as a parameter and build a deserializer using that parameter (if deserialize
is not provided). For SQL repository users, the coalesce will just return deserialize
, so they will get the perf improvement.
I got burned by the serialization settings being a static property (I depend on a project that also uses Its.* and that property was being reset from underneath me). Anyways, it's a good idea to let folks provide a custom serializer for domain events, so I added that feature to SqlEventSourcedRepository. I did not add the feature to the InMemory repository or other places - I went down that path but there was just too much for me to ramp-up on and hoping someone else can pickup the ball from here.
So if you want to provide a custom serializer, it's as easy as...
The signature of those delegates were designed to mimic the JSON.net methods that are currently being called (hence why serialization returns an
object
and deserialization takes aType
as it's second parameter). This is clear in various Serializer.cs methods where either the func (if not null) or JSON.net (if the func is null) is called side-by-side.A consequence is that anyone who wants to use the default behavior must call
.UseDefaultSerialization()
otherwise the DI container throws an exception.