[Host.Serialization] HybridMessageSerializer builder does not resolve specialised
Closed this issue · 11 comments
When adding the hybrid message serializer, instances of the specialised serializers need to be supplied which complicates the dependency management of those serializers.
It would be better if instances of the specialised message serializers were resolved from the DI container instead.
Yes, I agree. It's something I was also thinking about. Thanks for looking into this.
One minor ask, please raise the discussion first in the issue before starting to implement the PR feature.
I would like to have at least a chat to align on the direction. Sometimes there would be things I was thinking about for the future that would be good to discuss or shed some light on related things :)
Nevertheless, good idea!
One minor ask, please raise the discussion first in the issue before starting to implement the PR feature. I would like to have at least a chat to align on the direction. Sometimes there would be things I was thinking about for the future that would be good to discuss or shed some light on related things :)
That's a perfectly reasonable request. It started off as a very minor extension method change that I thought I would just start the conversation with but snowballed fairly rapidly :/
@EtherZa have a look at a few minor things.
I am also wondering if there is a way we could not require hybrid to be configured as the last (while ensuring backward compatibility with the overall serializer configuration experience).
Looks good overall!
It definitely has a smell about it. One solution could be to allow for registration to happen in the HybridSerializerOptionsBuilder itself instead of beforehand.
Something like
services
.AddSlimMessageBus(mbb =>
{
mbb
.AddHybridSerializer(
o => {
o..AddJsonSerializer(options)
.AsDefault();
o..AddAvroSerializer()
.For(typeof(Message1), typeof(Message2));
o..AddGoogleProtobufSerializer()
.For(typeof(Message3));
})
...
} In order to facilitate this, an interface could be added to SlimMessageBus.Host.Serialization which would need an implementation in each of the serializers
i.e
// In SlimMessageBus.Host.Serialization
public interface IHybridBuilder
{
IHybridBuilderOptions RegisterSerializer<TSerializer>(Action<IServiceCollection> services = null) where TSerializer : IMessageSerializer;
}
public interface IHybridBuilderOptions
{
IHybridBuilder For(params Type[] types);
IHybridBuilder AsDefault();
}
// In each of the serializer assemblies
public static class MessageBusBuilderExtensions
{
// Original builder
public static MessageBusBuilder AddJsonSerializer(this MessageBusBuilder mbb, JsonSerializerOptions options = null)
{
...
}
// An overload for hybrid
public static IHybridBuilderOptions AddJsonSerializer(this IHybridBuilder builder, JsonSerializerOptions options = null)
{
return builder.RegisterSerializer<JsonMessageSerializer>(services =>
{
services.TryAddSingleton(svp => new JsonMessageSerializer(options ?? svp.GetService<JsonSerializerOptions>()));
});
}
}By making the services parameter in IHybridBuilder.RegisterSerializer optional, it could be used for both DI registration or to add an already registered serializer to the hybrid builder (third party based on previous implementations).
The hybrid builder would still need to replace any previous IMessageSerializer registrations and, while the implementation would require a migration from previous implementations, it could be a more succinct approach.
It would also require a bump to the SlimMessageBus.Host.Serialization nuget package which would be a consideration.
@EtherZa building on your idea, I am proposing to extract an ISerializationBuilder that will allow us to configure the serializer at the bus builder level (as is now) plus will allow the hybrid serializer to perform the same registrations inside of itself (the hybrid serializer builder would implement ISerializationBuilder too). See the PR #241 to showcase what I mean: - specifically this change: https://github.com/zarusz/SlimMessageBus/pull/241/files#diff-36eae243ff3b086e9cdd6613548985c42ffbee4d1fe99f5aa9260d425a35a74b
This could allow achieving your provided sample - still, be backwards compatible and have a nice developer API to configure this:
services
.AddSlimMessageBus(mbb =>
{
mbb
.AddHybridSerializer(
o => {
o.AddJsonSerializer(options)
.AsDefault();
o.AddAvroSerializer()
.For(typeof(Message1), typeof(Message2));
o.AddGoogleProtobufSerializer()
.For(typeof(Message3));
})
...
} Could you accommodate your changes on top of the proposed PR?
Does that work conceptually?
@zarusz It's a nice suggestion but would leave two issues that would need to be overcome:
- a lack of context when the "AddSerializer" specialisation returns (the builder would not be privy to which serializer was just added)
- The fluent extension methods (.AsDefault() and .For()) would both need to be on the builder which then allows o.AsDefault() to be called without having set a serializer first.
services
.AddSlimMessageBus(mbb =>
{
mbb
.AddHybridSerializer(
o => {
o.AsDefault();
o.For(typeof(Message1), typeof(Message2));
})
...
}Unless, of course, I am missing something? :)
@EtherZa so I was thinking that the o in your example would be a custom hybrid builder object (HybridSerializationBuilder from hybrid serializer assembly), that would implement ISerializationBuilder hence allowing the standard serializer methods to apply. Then that same type HybridSerializationBuilder to has .AsDefault() or .For().
Then .AddHybridSerializer(o => {}) wrapping the other serializer setup would do the Replace on IMessageSerializer (in the PostConfigurationActions) as the last (so it would be privy?). What might be needed is to know what serializer implementation was registered is to run the post actions there to capture the side effects of each o.AddJsonSerializer() and to know to pair it with the respective AsDefault() or For() - it could have a ServiceCollection wrapper (or its own instance for just one pass) to introspect what was added.
Now, I think experimenting with the code might highlight issues in my thinking but I believe this could work ;)
@zarusz We are thinking about the same implementation (o is HybridSerializationBuilder which inherits ISerializationBuilder in the example).
I have updated the PR with this implementation, but it is a little crude.
The implementation passes an empty ServiceCollection instance to the wrapped PostConfiguration to collect the registered services and then iterates through the services to look for the associated IMessageSerializer type. Ideally, I would like to get the type associated with the IMessageSerializer registration but, when it is a delegate, the ReturnType is the interface and not the implemented type. The only way to divulge the implemented type at this point is to build the service provider and invoke the delegate so the that resultant object can be typed. Building the service provider would need to happen for each serializer registration and could potentially have unintended side-effects. The work around is to iterate through the ServiceDescriptors to look for a ServiceType that inherits IMessageSerializer.
When faced with a scenario like below, the logic will completely fall apart as the type cannot be determined without invoking the delegate.
public static TBuilder AddAnotherSerializer<TBuilder>(this TBuilder builder, AnotherSerialzierOptions options)
where TBuilder : ISerializationBuilder
{
builder.PostConfigurationActions.Add(services =>
{
services.TryAddSingleton<IMessageSerializer>(svp => ActivatorUtilities.CreateInstance<AnotherSerializer>(svp, options));
});
return builder;
}Further to this, it also plays havoc on intellisense which will complicate the development experience. The builder will be in the incorrect state for some of the options (.For(), .AsDefault()) which will result in a runtime error; and, due to the incomplete IServiceCollection, will have unexpected results if the developer were to invoke some of the other available extension methods.
The solution does work but is somewhat fragile, relying on the current two-step pattern being followed.
I will submit an alternative PR with the original proposal as well, and perhaps when seen side by side, there will be a clearer path ahead.
I will submit an alternative PR with the original proposal as well, and perhaps when seen side by side, there will be a clearer path ahead.
@zarusz I have added an alternative PR using the specialised builder proposal for review.
If you still prefer the common implementation, please let me know. There are a few more unit tests that will need to be added if you want to follow this path.
@EtherZa thanks for your patience on this, and for going back and forth with me.
I merged the PR #241 of mine to rule out some moving parts from the conversation as I think it won't hurt and might be used in the overall end-state solution. Apologies if it causes conflicts now in the PR.
Regarding the alternative PR #242 am not a fan of adding hybrid specialized config elements into .Host.Configuration, secondly having to maintain alternative config extension methods in the individual serializer packages. It's a good approach too, just still thinking about massaging this initial idea a bit more might do the trick.
Going back to this idea. I think we could still improve the intellisense in this last approach presented in your original #239, by perhaps starting with AsDefault/For first:
.AddHybridSerializer((HybridSerializerBuilder o) =>
{
o.AsDefault().AddJsonSerializer(); // We start with first AsDefault returns its own instance of ChildHybridSerializerBuilder
o.For(typeof(CustomerEvent)).AddAvroSerizaliser();
});
class HybridSerializerBuilder {
ChildHybridSerializerBuilder AsDefault() {}
ChildHybridSerializerBuilder For(params Type[] types) {}
}
class ChildHybridSerializerBuilder : ISerializationBuilder {
// this does the post actions visitation, only takes the builders and action delegates into account that actually configured the ServiceCollection
}So essentially, the difference is that the AsDefault/For needs to be used first which then create their own builders
That way both the intellisense should get better, as well as it should get less error prone.
Let me know. I am happy to move this one forward.
@EtherZa thanks for your patience on this, and for going back and forth with me.
@zarusz Absolutely no issue here. It's worth getting it right.
Moving the .For() and .Type() methods up the "stack" is an interesting idea.
Would you be happy to take a bite from PR #242 and add a method to the ISerializationBuilder to be used for registration by the message serializers (in order to capture the IMessageSerialzier type)?
The implementation in MessageBusBuilder would then also be responsible for registering the service as an IMessageSerializer in IServiceCollection.
public interface ISerializationBuilder
{
void RegisterSerializer<TMessageSerializer>(Action<IServiceProvider> services) where TMessageSerializer : IMessageSerializer;
}
public class MessageBusBuilder : ISerializationBuilder
{
public void RegisterSerializer<TMessageSerializer>(Action<IServiceProvider> services) where TMessageSerializer : IMessageSerializer
{
this.PostConfigurationActions.Add(services);
services.TryAddSingleton<IMessageSerializer>(svp => svp.GetRequiredService<JsonMessageSerializer>());
}
}
public class ChildHybridSerializerBuilder : ISerializationBuilder
{
public void RegisterSerializer<TMessageSerializer>(Action<IServiceProvider> services) where TMessageSerializer : IMessageSerializer
{
this.PostConfigurationActions.Add(services);
// do something with typeof(TMessageSerializer)
}
}
// MessageBusBuilderExtensions
public static MessageBusBuilder AddJsonSerializer(this MessageBusBuilder mbb, JsonSerializerOptions options = null)
{
mbb.PostConfigurationActions.Add(services =>
{
services.TryAddSingleton(svp => new JsonMessageSerializer(options ?? svp.GetService<JsonSerializerOptions>()));
// Removed (added by ISerializationBuilder where necessary)
// services.TryAddSingleton<IMessageSerializer>(svp => svp.GetRequiredService<JsonMessageSerializer>());
});
return mbb;
}The implementation would solve most of the issues but would leave the fluent building in HybridSerializerBuilder able to add multiple serializers as the default/for the same type. This is probably an acceptable caveat though as it could be done with multiple entries anyway.
var hsb = new HybridSerializerBuilder();
hsb
.AsDefault()
.AddSampleSerializer()
.RegisterSerializer<SampleSerializer>(sp => { });
hsb
.For(typeof(string))
.AddSampleSerializer()
.AddSampleSerializer();
// same issue (cannot be avoided at compile time)
hsb
.AsDefault()
.AddSampleSerializer();
hsb
.AsDefault()
.RegisterSerializer<SampleSerializer>(sp => { });
hsb
.For(typeof(string))
.AddSampleSerializer();
hsb
.For(typeof(string))
.AddSampleSerializer();