-
Notifications
You must be signed in to change notification settings - Fork 257
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
Add Default Message Mapper #3407
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ Code Health Quality Gates: OK
Change in average Code Health of affected files: +0.07 (9.17 -> 9.24)
@@ -379,7 +379,9 @@ public static MessageMapperRegistry MessageMapperRegistry(IServiceProvider provi | |||
|
|||
var messageMapperRegistry = new MessageMapperRegistry( | |||
new ServiceProviderMapperFactory(provider), | |||
new ServiceProviderMapperFactoryAsync(provider) | |||
new ServiceProviderMapperFactoryAsync(provider), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may be your comment around async and sync, the fact that we have to register two. The only way around this would be to make all default message mappers implement both. That might be a problem in contexts where it is hard to provide either one of those, due to constraints on libraries used. But it is a little gnarly.
However, we can provide default mappers that implement both
public Dictionary<Type, Type> Mappers { get; } = new Dictionary<Type, Type>(); | ||
public Type DefaultMessageMapper { get; private set; } = typeof(JsonMessageMapper<>); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to default the default here, or do we just do that in the extensions DSLs?
|
||
var header = new MessageHeader(messageId: request.Id, topic: publication.Topic, messageType: messageType); | ||
|
||
var body = new MessageBody(JsonSerializer.Serialize(request, JsonSerialisationOptions.Options)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In principle there is an async serializer. I am not sure if it actually has meaning. But we should investigate. The MS route here is to pass an enum flag to the common method and then just use sync or async code in the body, based on that flag as needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the Flag Argument Hack here: https://learn.microsoft.com/en-us/archive/msdn-magazine/2015/july/async-programming-brownfield-async-development
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks really good. I can add a default Avro as well later
First look into Default Message Mapper
#3355