Support for Interface-based MessageHandlers

[ZaradarBH]

It would be nice if we could register global message handlers that map incoming events to strongly typed interfaces so we dont have to make a concret handler for each event type. This would enable us to do general purpose "catch-all" handlers to detect new events coming in on topics and much more.

• [ ] Create feature branch
• [ ] Examine impact of replacing generic constraint TMessage : class, new() with TMessage : IMessage
• [ ] Implement IMessage interface
• [ ] Document new behavior
• [ ] Review with Martin O

[martinosk]

If you add a T_Message:IMessage contraint, wont that make Dafda more obtrusive? That basically means you cannot produce a message for Dafda, without using Dafda to produce it? A good thing about Dafda is that it's non-obtrusive on the producer side and you don't need to take on any Dafda dependencies to create messages that can be understood by Dafda consumers.

[ZaradarBH]

Considering that everything eventually ends up as ConsumeResult<string, string> before its shipped off to the topic, I would say that depends very much on how you decide to setup the serialization/deserialization behavior in your application. The generics primary purpose in Dafda, as I understand it, is to provide a convenient way to fetch a strongly typed payload when consuming messages. However from a pure development perspective changing the constaint to an interface which maps 1-to-1 to the DFDS event schema will make any one capable of deserializing json capable of reading data from a topic with or without Dafda, while at the same time allowing developers to write more general purpose message handlers for topics which will make their code more DRY.

[ZaradarBH]

You can see an example interface here for a message payload @ https://github.com/dfds/code-ops/blob/master/src/CloudEngineering.CodeOps.Abstractions/Events/IIntegrationEvent.cs

You can see an example of an interfaced based producer here @ https://github.com/dfds/azure-devops-janitor/blob/main/src/AzureDevOpsJanitor.Host.EventForwarder/Controllers/ForwarderController.cs

The configuration for the producer can be found here @ https://github.com/dfds/azure-devops-janitor/blob/a8bbc2920bc9ec1ada93c9c74c29187bb1c1c7ee/src/AzureDevOpsJanitor.Infrastructure/DependencyInjection.cs#L43

The consumer which reads messages from this particular producer can be found here @ https://github.com/dfds/azure-devops-janitor/blob/main/src/AzureDevOpsJanitor.Host.EventConsumer/Strategies/VstsWebHookConsumptionStrategy.cs

Its worth mentioning that deserializing to an interface is not currently supported by System.Text.Json @ https://github.com/dotnet/runtime/issues/30956 , only time will tell if it does get included as it is still being debated in the community due to security concerns related to fuzzy-logic-style-attacks that could lead to memory leaks or buffer overflows. However if we do not wish to implement a physical representation of an interface, as we have done in our consumption strategy, we can use NewtonSoft to work around this short-coming as it supports interface deserialization out-of-the-box

[chamook]

I'm not sure how much use adding an interface would be for providing a "catch-all" handler, because it's already possible to create a handler that doesn't look at the specifics of the message payload by implementing IMessageHandler<object>:

type NoOpHandler(logger: ILogger<NoOpHandler>) =
    interface IMessageHandler<Object> with
        member x.Handle(_, context) =
            logger.LogInformation("Ignoring message type: {objectType}", context.MessageType)
            Task.CompletedTask

The values that you specify in your IIntegrationEvent example seem to be very similar to those that are included in the Metadata type in Dafda, which is provided to a message handler through the MessageHandlerContext argument.

Does that do what you would need, or am I missing something here?

[ZaradarBH]

I realize this can be done using Object, however that does not provide me with any runtime guarantees, in terms of the contract that the underlying type supports and removes my ability to duck-type things. As I hope we can all agree that System.Object is a rather poor duck.

The values in the IIntegrationEvent are based on the same schema as Dafda, so they should be similar. Overall I just do not agree with the need to implement multiple handlers to maintain type safety in the delegate that handles the message, I do not want to end up WET because I have to write a lot of is-guards to ensure that the object supports the members that are expected and the only thing preventing us from using a IMessage interface, instead of T: class, new(), is the generic constraint which utilizes a underlying object initialization strategy (Activator-based) that is x5 - x10 times slower then new default(T) which will make Dafda poor for situations where you have to digest a high volume of messages with out a full rewrite. Lastly we also need to look at support for IAsyncEnumerable and ValueTask, but thats a whole diff story. :)

We can as stated pay the cost of re-writing this, but granted that the configuration system is outdated (its based on a 10+ year old design pattern) that has now largely been substituted in favor of a Options-pattern and its serialization logic doesnt even begin to consider the interfaces from Confluent, I have come to the conclusion that doing anything about Dafda is a waste of time, compared to simply just using a Confluent lib with a thing shim, which I am in the process of building.

So as such we can just close this issue if it makes you all happy :)

[chamook]

Ok, much as I do not enjoy defending Dafda's design...

the only thing preventing us from using a IMessage interface, instead of T: class, new(), is the generic constraint which utilizes a underlying object initialization strategy (Activator-based) that is x5 - x10 times slower then new default(T)

This isn't why the generic constraint is there - because afaict by default Dafda uses System.Text.Json to create a new object which doesn't use Activator.CreateInstance which everyone is up in arms about lately. The reason the constraint is there is because it used to be required by that (not sure it still is).

But to address your other point about duck-typing - have you considered not using interfaces to describe properties on an entity? When dafda gets data over the wire, it just has to deserialize it into something that .net likes so you could make a type that only contains the fields that you would put on your interface and have it fulfill the generic constraints :)

[ZaradarBH]

Dafda doesnt have a design worth defending ;) and as much as I enjoy having a good strawman argument, I doubt that is the correct path to make Dafda great again, so I will simply say that using Activator.CreateInstance is a bad idea, which I pinged you all about in the chat yesterday, regardless your comment does not address the other issues like the missing options pattern @ https://github.com/dfds/code-ops/blob/master/src/CloudEngineering.CodeOps.Infrastructure.Kafka/KafkaOptions.cs and lack of support for the built-in serialization interfaces in Confluents own library @ e.g. https://github.com/dfds/code-ops/blob/master/src/CloudEngineering.CodeOps.Infrastructure.Kafka/Serialization/DefaultValueSerializer.cs

If Dafda actually uses the Activator for message creation or only for producers (via the ActivatorUtils which looks like a ton of in-mem gymnastics @ https://github.com/aspnet/DependencyInjection/blob/master/shared/Microsoft.Extensions.ActivatorUtilities.Sources/ActivatorUtilities.cs) I cant tell, but I trust you implicitly, so I will assume that is the case, which is good, but still it doesnt really explain the need to enforce the T : new, class generic constraint since System.Text.Json recently added support for JsonConstructor @ https://github.com/dfds/code-ops/blob/master/src/CloudEngineering.CodeOps.Abstractions/Events/IntegrationEvent.cs

To summerize I have already successfully implemented an alternative to Dafda, with ALOT less code, for our own stuff which enables me to move forward in a world where you need interfaces to stay SOLID and its available for anyone who can access our internal nuget feeds :)

[ZaradarBH]

Lastly, and I almost forgot this, we stopped using Dafda in general around the end of 2019 as we ran into some issues with DI scopes and EF that seems to be caused by including Dafda and forced os to scale down all our Outbox workers to a single replica to avoid crashing some of our legacy janitors (written by people that where here before Emil and myself)

[martinosk]

Won't fix