Application Layer refactor

[MatteoZampariniDev]

This is the first step of a major refactoring I'm doing in order to decouple each layer a bit better.
I'm doing this because I had quite a few problems while trying to add/remove features because of many dependencies between layers that don't have much sense in my opinion.

At the moment I cleaned the Application layer just keeping the essential logic.
This could be an improvement by itself, now I'm going to try to improve the infrastructure layer.

I know Jason Taylor's clean architecture quite well and I'm trying to keep it as clean as possible applying the same concepts.

Feel free to reject this request, I understand there are going to be quite a few changes and not everybody is going to agree on these choices.

[Bram1903]

This looks promising! @neozhu will take a look!

[MatteoZampariniDev]

Refactoring the infrastucture layer requires a bit more work and I might have to touch some code.
At the moment I just moved a few things and simplified the dependency injection.

I removed all those extensions files with methods for the dependency injection that are not useful while adding/removing actual features to the project, I prefer having less files and a few more methods in the DependencyInjection class, it's unlikely that we are going to look for them very often and it's still clean.

[neozhu]

@Bram1903, of course，I'm glad to hear you like the direction as well.
@MatteoZampariniDev,
Thank you for the update and effort, I understand your perspective and support the idea of streamlining the code structure and dependency injection. Indeed, an overabundance of files and methods can sometimes complicate maintenance, especially in projects that change frequently. Please ensure the code remains clean and readable, and that all functionalities work as expected after the refactoring. I look forward to seeing further improvements!

[neozhu]

@MatteoZampariniDev @Bram1903
Actually, I have a point of confusion too. Why does the Infrastructure Project reference (depend on) the Application Project? This structure prevents the Application project's class from using some common parameters and configuration information defined in the Infrastructure project. I'm wondering if it might be beneficial to introduce a Shared Project to manage these common parameters and configuration information classes.

[MatteoZampariniDev]

@MatteoZampariniDev @Bram1903 Actually, I have a point of confusion too. Why does the Infrastructure Project reference (depend on) the Application Project? This structure prevents the Application project's class from using some common parameters and configuration information defined in the Infrastructure project. I'm wondering if it might be beneficial to introduce a Shared Project to manage these common parameters and configuration information classes.

The infrastructure layer must reference the application layer because it's a concrete implementation of it.

If you need any configuration in the application layer you could just add them there or use an interface like I did with the IdentitySettings; I opted for using an interface in order to keep all the settings files and injection in one place.

The IdentitySettings class is the only one being used by the application layer at the moment but in theory the application layer shouldn't even know much about identity implementation, the Microsoft.AspNetCore.Identity.EntityFrameworkCore nuget should be in the infrastucture layer instead of the domain layer but it's an exception that makes sense as long as you are using entity framework identity.

For practical reasons the application layer is just one project but it contains two very different aspects, the abstraction and the actual business logic; the infrastructure layer is an implementation of the abstractions defined in the application, if you need something shared across the application and the infrastructure you should place it in the "application.abstraction" (= our "Common" folder), then it's up to you how to implement it (see IdentitySettings).

Is there any specific configuration that don't fit in this view?

[MatteoZampariniDev]

I moved hubs to the server because it's the correct layer but I had to create a wrapper service in order to access it from the infrastructure layer... I think that this could be improved (maybe using mediatR notifications), I don't have much experience with signalR but I'm thinking about a project that would use it a lot, I'll come back to it in the future.

Also I fixed how the ClientHub handles received calls, should be less error prone now.

[neozhu]

@MatteoZampariniDev
Would it be better to merge the Components Folder and Share Folder in Server.UI project? What do you think?

[MatteoZampariniDev]

@MatteoZampariniDev Would it be better to merge the Components Folder and Share Folder in Server.UI project? What do you think?

I was about to do that ahaha

Anyway I chose to split the server in two projects in order to keep clear the separation between server-only code (endpoints/server hub and possibly other services) and the actual blazor code provided to the client.

One of the objective of this refactoring process is to keep the core server logic as separated as possible from the UI framework, any changes to the front-end framework will be easier (even just moving to blazor wasm or hybrid).

[MatteoZampariniDev]

I moved all the c# code in razor pages to code behind (= partial razor.cs classes) because:

• intellisense ha some problems in .razor pages
• some errors and warnings are not detected at editor/compile time
• code cleanup doesn't work in .razor pages
• we avoid having huge files and reduce the amount of scrolling up/down

[MatteoZampariniDev]

Also I renamed files that inherits from ComponentBase to FILE_NAME.razor.cs in order to identify by the name that are components even without a .razor file

[MatteoZampariniDev]

@neozhu I'm not sure this is correct (current code from ProductCacheKey), why the Cancel() method is called on the CancellationTokenSource that was just created instead of being called before the replacement of the previous one?

EDIT: or am I wrong and it gets replaced on the next call after the cancellation? In this case wouldn't be better replacing it immediately after the Cancel() method is called?

[neozhu]

49f2be3
like this?

[MatteoZampariniDev]

49f2be3 like this?

yeah more or less, the same tokenSource replacement should be done in CacheInvalidationBehaviour too I think... it probably wasn't wrong but I think that replacing the it as soon as Cancel() is called is safer if I'm understanding this right... I'm not familiar with thread handling.

Anyway I'm trying to improve the caching system, I'll share it if I succeed.

[MatteoZampariniDev]

@neozhu I'm just leaving this here not to forget for the future... as I said I'm working on improving the caching system and I found out that with the current implementation o MemoryCacheBehaviour if there is a cached request behaviours registered after the MemoryCacheBehaviour do not run. I'm using a different project to test this and I might be missing something but I'm quite sure about that, AuthorizationBehaviour might not work for cached requests.

Anyway I've almost completed the caching system, I'll share a repository as sample very soon.

[MatteoZampariniDev]

I'm realizing that this could have bigger implications, if the whole mediatR pipeline gets cached there is the risk that when it's executed from the cache variables from previous requests are used instead of those of the current request. This would be a big deal because identity data could be cached too.

[neozhu]

from my current review of the code, I haven't observed the issue you pointed out. The key for caching is determined by the request parameters, which should ideally make every cache unique. However, you do have a point regarding the potential thread-unsafety of static types. As for caching identity data, it's crucial to carefully consider the conditions for cache invalidation and the events that would trigger such invalidation.

[MatteoZampariniDev]

Here is the sample project (check the cache-system branch). Quick explanation below.

Feature-specific cache data gets registered as singleton, here is the code to declare them:

Cacheable requests and invalidator are marked by attributes:

This enables more complex scenarios where invalidating more than one cache is required:

If you'd like to debug it, I suggest using this breakpoint in order to check the "currentCached" value:

The only missing piece is the invalidation from the front end but I still have not understood when that's needed.

EDIT: also this makes requests models way more clean.

[MatteoZampariniDev]

@neozhu if you like that refactoring I can do the implementation myself, just let me now... and no hurry 😁

[neozhu]

@neozhu if you like that refactoring I can do the implementation myself, just let me now... and no hurry 😁

implementing caching through attributes is a fantastic approach. Is this method also based on the mediator pipeline request?

[neozhu]

@neozhu if you like that refactoring I can do the implementation myself, just let me now... and no hurry 😁

implementing caching through attributes is a fantastic approach. Is this method also based on the mediator pipeline request?

[MatteoZampariniDev]

@neozhu if you like that refactoring I can do the implementation myself, just let me now... and no hurry 😁

implementing caching through attributes is a fantastic approach. Is this method also based on the mediator pipeline request?

Yes it uses the mediator pipeline, you can check my repository https://github.com/MatteoZampariniDev/Playground/tree/cache-system

[neozhu]

Thank you for sharing your repository with me. After a thorough review of your code, I have a few concerns:

Using the request as a key to push into the cache is a reasonable approach. However, I noticed potential issues when it comes to refreshing the cache by using the key. Consider a scenario where I'm searching based on a keyword, and the keyword changes 10 times. This would result in 10 different cache entries. If we need to invalidate all these cached items, merely relying on a single key would not be sufficient.

In scenarios like these, using a CancellationTokenSource to handle cache invalidation might be a more effective solution. It allows for a more granular control over the cache entries and can help ensure that stale or outdated cache entries are effectively cleared.

[MatteoZampariniDev]

Thank you for sharing your repository with me. After a thorough review of your code, I have a few concerns:

Using the request as a key to push into the cache is a reasonable approach. However, I noticed potential issues when it comes to refreshing the cache by using the key. Consider a scenario where I'm searching based on a keyword, and the keyword changes 10 times. This would result in 10 different cache entries. If we need to invalidate all these cached items, merely relying on a single key would not be sufficient.

In scenarios like these, using a CancellationTokenSource to handle cache invalidation might be a more effective solution. It allows for a more granular control over the cache entries and can help ensure that stale or outdated cache entries are effectively cleared.

Uhm I kept the same behavior using the cancellation token source, the only thing I changed is how request keys are built (before they were declared manually in each feature cache).

Actually I realized that the Remove() call can be removed (no pun intended) because it's the CancellationTokenSource that handles it.

Am I missing something?

[MatteoZampariniDev]

Actually I realized that the Remove() call can be removed (no pun intended) because it's the CancellationTokenSource that handles it.

This implies that the "Key" property can be removed too from the BaseCache in my sample

EDIT: I updated my Playground repository

[neozhu]

Actually I realized that the Remove() call can be removed (no pun intended) because it's the CancellationTokenSource that handles it.

This implies that the "Key" property can be removed too from the BaseCache in my sample

EDIT: I updated my Playground repository

thanks for the update.

I recently discovered a caching library called FusionCache. It seems to have a robust approach to caching with many innovative methods. If you're interested, maybe we can explore it together to see if there are any insights we can glean or integrate into our project.

[MatteoZampariniDev]

Sure!

[MatteoZampariniDev]

After a quick look I can think about an implementation using the pattern of my Playground repository but I have to look a bit better the "Backplane" because it's the tricky part... if there are other features to be implemented I will take a look at that too.

I'll be busy for the few next days, I can take a deeper look from Wednesday but if you'd like to do it yourself no problem.

Also I started to move some of the Identity logic to the Application layer from the blazor components in order to make it more generic (maybe reused with another front-end framework) and to reduce the code in components as we talked about earlier.

[neozhu]

Hi @MatteoZampariniDev, to ensure the code generator works more effectively, I've had to revert the project structure of the Domain back to its original format. Once again, I appreciate your contributions to this project. I'm getting ready to merge this PR into the main branch. Is there anything else you'd like to modify? Also, @Bram1903, do you have any suggestions?

[MatteoZampariniDev]

Hi @MatteoZampariniDev, to ensure the code generator works more effectively, I've had to revert the project structure of the Domain back to its original format. Once again, I appreciate your contributions to this project. I'm getting ready to merge this PR into the main branch. Is there anything else you'd like to modify? Also, @Bram1903, do you have any suggestions?

I'm taking a look at the caching system as I said but I've been more busy than expected and I still have some tests to do. At the moment I'm not working on anything else, the only thing I left behind is moving the business logic from the authentication blazor pages to the backend as I did with the ResetPassword page (commit 4569435) but I don't think I'll have time to do it soon, sorry.

[neozhu]

Thanks for the update! I understand. Thank you