As already shown by #23 and #16, LightWorker has some issues, but none crucial. However, its design suffers from many issues that I'm gathering in this single issue:
Consumers are unnecessarily require to inherit LightWorker
(I'm using the term "queue" here generically)
As we can see from the Discovery method (significant part reproduced below), classes that wants to use LightWorker infrastructure are required to derive from LightWorker.
However, those classes are not supposed to access members of LightWorker, in fact, the opposite. That class contains logic that control all consumers.
Also, it's already necessary that such classes have methods marked with a TopicAttribute or QueueAttribute, so no inheritance would be necessary.
Code to manage queues are shared between objects that consumes queues
Following the issue above, we can realize that LightWorker instances accumulates the managing part of queues as well as the consuming part, which is bad coupling that results in more maintenance than necessary, creates opportunities for lower-level classes to mess with the functioning of higher level structures, etc.
It would be a very nice idea to move managing into it's own class.
Every implementation is a HostedService
Hosted services are used for background processing. On many queue scenarios a push technique is used, so the client doesn't need to do polling and, therefore, there's no background processing to be done.
Inheriting from IHostedService assumes that every queue read implementation will do some sort of polling.
MessageBus attribute is unused
While code doesn't really tells us what MessageBusAttribute is for, by looking for it we can see that it isn't being used by anything. On the other hand, it seems that the idea was to enable users to consume topics and queues from multiple buses.
There is no access by the key (MethodInfo), the dictionary is always enumerated. This is a waste of precious computing resources.
Unmatching messages results in failtures without additional information
JsonConvert.Deserialize will fail if the provided type doesn't matches the message - and the error informed is very low level (it's a parser error informing which character wasn't expected). This means a debugging hell.
Authorization isn't secure
You can annotate a method in the worker with AuthorizeAttribute in order to enable the framework to check the message for a principal and, when found, to compare it to roles attributed in the attribute. Thing is, the ClaimsPrincipal in the message is never verified, therefore, anyone can post a message with a fake list of claims, including the desired role, and it will work.
Liquid's architecture is based on using JWT for security. If this mechanism is necessary, then, we could rebase our approach on having the JWT being sent between services and use the claims there (and not on the serialized message) to check for roles. Of course, this would also mean that we need to make sure the JWT is valid.
(This also brings a new problem - messages are coming from queues, so the JWT might be expired. Validating the JWT would prove that it's was genuinely emitted, but, it would not guarantee that it wasn't hijacked and is being reused.
If we decide to rely on the infrastructure - the queues are private and no outsiders can post messages directly to them - we could decide that the poster is friendly and should have it's request fulfilled... BUT, once we go down this road, we are also saying that we could rely on the ClaimsPrincipal on the queue, and then this issue kinda goes away. )
Costly reflection is being done to perform deep validation
LightWorker.ValidateInput uses the LightViewModel.Validate to produce a Validator and then validate the code. Besides errors already mentioned on #149 (which refers to another place where the code is actually DUPLICATED), there's another thing going on there:
The lines reproduced below are using reflection to inspect all properties of the object;
For each property, its get method is called (GetValue) which potentially runs costly code;
The type of the returned object is compared to IList which is iterated (which also disregards other type of collections)
The base type of each of the objects found on the list are compared to LightViewModel, disregarding second-level inheritance (A inherits B inherits LightViewModel), which seems unintended
DLR is used to access the returned code instead of a simple object, resulting in unneeded inefficient code (already discussed on #15)
None of the reflection performed is ever cached, resulting in poor performance
This verification of complex properties seems a fair use case, but one that could be easily left to the programmer implement when building the Validator (as should be the case per FluentValidator docs).
As already shown by #23 and #16,
LightWorkerhas some issues, but none crucial. However, its design suffers from many issues that I'm gathering in this single issue:Consumers are unnecessarily require to inherit
LightWorker(I'm using the term "queue" here generically)
As we can see from the
Discoverymethod (significant part reproduced below), classes that wants to use LightWorker infrastructure are required to derive fromLightWorker.https://github.com/Avanade/Liquid-Application-Framework/blob/3db86aa5390498bc253d7599f0a44f922afc40dc/src/Liquid.Activation/Worker/LightWorker.cs#L82-L86
However, those classes are not supposed to access members of LightWorker, in fact, the opposite. That class contains logic that control all consumers.
Also, it's already necessary that such classes have methods marked with a
TopicAttributeorQueueAttribute, so no inheritance would be necessary.Code to manage queues are shared between objects that consumes queues
Following the issue above, we can realize that LightWorker instances accumulates the managing part of queues as well as the consuming part, which is bad coupling that results in more maintenance than necessary, creates opportunities for lower-level classes to mess with the functioning of higher level structures, etc.
It would be a very nice idea to move managing into it's own class.
Every implementation is a HostedService
Hosted services are used for background processing. On many queue scenarios a push technique is used, so the client doesn't need to do polling and, therefore, there's no background processing to be done.
Inheriting from
IHostedServiceassumes that every queue read implementation will do some sort of polling.MessageBus attribute is unused
While code doesn't really tells us what
MessageBusAttributeis for, by looking for it we can see that it isn't being used by anything. On the other hand, it seems that the idea was to enable users to consume topics and queues from multiple buses.https://github.com/Avanade/Liquid-Application-Framework/blob/5587dda08cfd8ba324dd06bc91d993c4b2960d9d/src/Liquid.Activation/Worker/LightWorker.cs#L104
I have to take a better look, but I must check how is the configuration working.
(This warrants its own issue I think, because it can cause bugs in production).
A list of pairs is being implemented as a dictionary
Both _queues and _topics are used as a list of pairs:
https://github.com/Avanade/Liquid-Application-Framework/blob/ac834f46809cd445dda8783ff616ea4ae7af6b33/src/Liquid.Activation/Worker/LightWorker.cs#L96
https://github.com/Avanade/Liquid-Application-Framework/blob/ac834f46809cd445dda8783ff616ea4ae7af6b33/src/Liquid.Activation/Worker/LightWorker.cs#L115
There is no access by the key (MethodInfo), the dictionary is always enumerated. This is a waste of precious computing resources.
Unmatching messages results in failtures without additional information
JsonConvert.Deserializewill fail if the provided type doesn't matches the message - and the error informed is very low level (it's a parser error informing which character wasn't expected). This means a debugging hell.Authorization isn't secure
You can annotate a method in the worker with
AuthorizeAttributein order to enable the framework to check the message for a principal and, when found, to compare it to roles attributed in the attribute. Thing is, theClaimsPrincipalin the message is never verified, therefore, anyone can post a message with a fake list of claims, including the desired role, and it will work.Liquid's architecture is based on using JWT for security. If this mechanism is necessary, then, we could rebase our approach on having the JWT being sent between services and use the claims there (and not on the serialized message) to check for roles. Of course, this would also mean that we need to make sure the JWT is valid.
(This also brings a new problem - messages are coming from queues, so the JWT might be expired. Validating the JWT would prove that it's was genuinely emitted, but, it would not guarantee that it wasn't hijacked and is being reused.
If we decide to rely on the infrastructure - the queues are private and no outsiders can post messages directly to them - we could decide that the poster is friendly and should have it's request fulfilled... BUT, once we go down this road, we are also saying that we could rely on the ClaimsPrincipal on the queue, and then this issue kinda goes away. )
Costly reflection is being done to perform deep validation
LightWorker.ValidateInputuses theLightViewModel.Validateto produce a Validator and then validate the code. Besides errors already mentioned on #149 (which refers to another place where the code is actually DUPLICATED), there's another thing going on there:GetValue) which potentially runs costly code;IListwhich is iterated (which also disregards other type of collections)LightViewModel, disregarding second-level inheritance (A inherits B inheritsLightViewModel), which seems unintendedobject, resulting in unneeded inefficient code (already discussed on #15)https://github.com/Avanade/Liquid-Application-Framework/blob/41cc17fc02659ff70660accdc8ad7c3b8ea4ae5a/src/Liquid.Activation/Worker/LightWorker.cs#L211-L249
This verification of complex properties seems a fair use case, but one that could be easily left to the programmer implement when building the Validator (as should be the case per FluentValidator docs).