Concerns about Serenity 5.0's implementation of .NET Dependency Injection

[MarcusOy]

Hi there,

We've started to work on a project that uses the newest StartSharp template (5.0.41.0). We are relieved to see Serenity supporting newer .NET technologies, however, we have some concerns about Serenity's Dependency Injection situation (mainly in repository classes). Please let me know at any time if I've misunderstood Serenity's DI implementation or if I am completely wrong.

Here is a snippet of a generated repository for reference:

/* ...imports... */
namespace Serenity.Demo.Northwind.Repositories
{
    public class OrderRepository : BaseRepository
    {
        public OrderRepository(IRequestContext context)
             : base(context)
        {
        }

        private static MyRow.RowFields fld { get { return MyRow.Fields; } }

        public SaveResponse Create(IUnitOfWork uow, SaveRequest<MyRow> request)
        {
            return new MySaveHandler(Context).Process(uow, request, SaveRequestType.Create);
        }

        /* ... other repository methods ... */

        private class MySaveHandler : SaveRequestHandler<MyRow>
        {
            public MySaveHandler(IRequestContext context)
                 : base(context)
            {
            }
        }

        /* ... other repository handlers ... */
    }
}

Personally, I'm pretty new to .NET (Core) dependency injection with only about 6 months of experience, but I feel like I have a decent understanding of how it should work. To my understanding, what I am seeing above is mostly correct: OrderRepository currently has one dependency, which is the IRequestContext, and this context is being injected through the constructor and is stored as field Context (that process is abstracted away from us through the parent class constructor base(context) which calls BaseRepository's constructor). The problem that I am observing is how this IRequestContext is being "injected."

Here is a snippet of a generated endpoint for reference:

/* ...imports... */
namespace Serenity.Demo.Northwind.Endpoints
{
    [Route("Services/Serenity.Demo.Northwind/Order/[action]")]
    [ConnectionKey(typeof(MyRow)), ServiceAuthorize(typeof(MyRow))]
    public class OrderController : ServiceEndpoint
    {
        [HttpPost, AuthorizeCreate(typeof(MyRow))]
        public SaveResponse Create(IUnitOfWork uow, SaveRequest<MyRow> request)
        {
            return new MyRepository(Context).Create(uow, request);
        }

        /* ...other endpoint methods ... */
    }
}

The problem I've observed is that rather than OrderRepository being injected into the OrderController (aka endpoint), the endpoint is manually injecting the Context field into the OrderRepository's constructor. This doesn't seem like true dependency injection to me.

Here's an example of how I would interpret Microsoft's DI documentation:

namespace Serenity.Demo.Northwind.Endpoints
{
    [Route("Services/Serenity.Demo.Northwind/Order/[action]")]
    [ConnectionKey(typeof(MyRow)), ServiceAuthorize(typeof(MyRow))]
    public class OrderController : ServiceEndpoint
    {
        private readonly IOrderRepository _orderRepository;
        public OrderController(IOrderRepository orderRepository)
        {
            _orderRepository = orderRepository;
        }

        [HttpPost, AuthorizeCreate(typeof(MyRow))]
        public SaveResponse Create(IUnitOfWork uow, SaveRequest<MyRow> request)
        {
            return _orderRepository.Create(uow, request);
        }

        /* ...other endpoint methods ... */
    }
}

This doesn't seem like a big issue until your repository needs additional services (ex: ILogger, IStorageService, INotificationService, etc), because then the current setup would force you to first inject those services into the endpoint, then to the repository.

...
        [HttpPost, AuthorizeCreate(typeof(MyRow))]
        public SaveResponse Create(IUnitOfWork uow, SaveRequest<MyRow> request)
        {
            return new MyRepository(Context, _logger, _storageService, _notificationService).Create(uow, request);
        }
...

What if one repository method required a service that another repository method doesn't need (Create vs List)? Would you have to inject services (to satisfy a repository constructor) you don't need in order to use a certain method?

Not to mention if you'd like to call a repository method from another repository, you would also need to inject seemingly unrelated services into an unrelated endpoint class. Without isolated dependencies per service/repository/module, I can see how quickly a project could turn into dependency hell.

I think the root problem I am seeing is that Repositories and ListHandlers should exist within the DI container provided by aspnet, rather than being created at will using a constructor. Letting the DI container construct the Repositories and ListHandlers as singleton/scoped/transient services would make sure that each injected service only deals with its own dependencies, not the dependencies of services it will call. The DI container already handles the different Endpoint/Controller classes, which is why services are able to be injected into them.

I haven't seen any issues or discussions relating to dependency injection, so if you have any thoughts about it at all, comment on this discussion.

Best regards,
Marcus

[volkanceylan]

If you want to define interfaces for your repositories, and implement them you are free to do so. In my opinion, having one interface per repository that is not going to be useful for anything else is over engineering. We are not preventing you from doing that if you need that though.

IRequestContext is there as we don't want everybody elses code to break in the future if we'll need to add another interface to repositories / base handlers that we'll need in the future.

You may subclass that and add more interfaces that are needed in all / most of your repositories but this is not a recommendation.

You can always add more parameters (e.g. services) to your repository constructor. If just some methods are going to need some additional interface then pass them as method parameters (at the end) and inject them in the endpoint using [FromServices]. If you don't like that, you are free to define multiple interfaces like ICreateService<TEntity>, IListService<TEntity> etc.

[MarcusOy]

Thanks for the response Volkan!

My example using an interface was just circumstantial, as in .net you can register a service with just its concrete type. And I understand the purpose of IRequestContext as all Repositories and Handlers will be using them.

I'm still worried about Serenity's direction into DI. More specifically, my primary concern revolves around the relationship Endpoint and Repository classes NOW have with each other. It seems to me that it is the Endpoint's responsibility to provide and inject services needed with its respective repository, creating a tight coupling with each other.

I would say SOLID principles drive best practices for Microsoft's DI implementation, and in my opinion, the above violates a few SOLID principles. For example, I would argue that both the Endpoint and Repository classes should have one responsibility, to provide an external (REST) entry point into a Repository and to provide CRUD+ interfaces for a specific entity in the database, respectively. The Endpoint should not have the added responsibility of making the Repository work by providing its dependencies.

To further clarify my concerns, I've also created a diagram to showcase why I think its a bad idea for more parameters to be (and what I mean by "dependency hell"). I've outlined three scenarios: the first is how Serenity generates Endpoints and Repositories for a given module, the second is when a repository starts to gain dependencies of its own, and the third is when a second repository (that has its own dependencies) is required within the first repository.

Here's my interpretation of Serenity 5.0's implementation:

Observe the following:

• When OrderRepository has dependencies in order to function, OrderEndpoint has to be aware of its exact dependencies and has to provide them to OrderRepository
• When OrderRepository has to refer to another Repository that has its own dependencies, the OrderEndpoint has to be aware of exact dependencies of both repositories...
• ... and so on
• Additionally, if a Repository's dependencies changed, you would need to modify dependencies n amounts of times in the project, where n is the amount of services dependent on that Repository

Here's my proposed implementation:

Observe the following:

• Each of the three classes shown have increased isolation from each other:
  • The OrderEndpoint does not need to be aware of OrderRepository's dependencies.
  • The OrderRepository does not need to be aware of OrderDetailRepository's dependencies.
  • All classes are ONLY responsible for its own dependencies, not the dependencies of its dependencies
  • Additionally, if a Repository's dependencies need to change, they only need to change in one place

Continued in my other reply...

[volkanceylan]

Btw it is not valid to say base class / framework injecting IRequestContext is violating DI, see how Controller/ControllActivator injects HttpContext into controller classes, do you have a HttpContext in your controller constructor? It is exactly modelled after ASP.NET Core MVC injection mechanism. DI does not mean constructor injection, it is just a way of dependency injection.

[MarcusOy]

Sure, I'm not saying base class injecting IRequestContext is the problem here (because yes, an ASP.NET Controller abstracts that HttpContext injection out for you). However, if I had a service that is not a Controller where I would need to access HttpContext, I would add IHttpContextAccessor into the parameters of my service's constructor, add said service to the DI container (services.AddSingleton/Scoped/Transient(...)), and have the DI container inject it for me rather than me manually passing/injecting it from the Controller to said service (links are to examples I've made in another repository). I don't want that Controller to have that added responsibility of making sure said service works, I want said service to handle its own dependencies (HttpContext) by itself. This allows me to call methods on said service from places that may not have HttpContext within its scope (like another service without HttpContext).

[volkanceylan]

I understand your points and we originally thought about adding defining repository interfaces or classes and registering them in startup (either manually / via automatic discovery through reflection) etc. but the complexity involved does not outweight the advantages offered. So we designed it this way. But this design does not prevent you from using them that way, it is just not the default. Here are some of the reasons we didn't implement that way:

• It should have been easy to port existing code written without DI, with just a few regex replaces. One of major reasons.
• Majority of repositories does not need anything other than IRequestContext.
• Majority of users don't need that level of abstraction
• You are not supposed to write much code in repository / endpoints. Business logic is supposed to go within service behaviors in Serenity apps. Repository / endpoints are just access points to handlers. Actually, you can simply remove the repository class and create/inject service handlers in your endpoints. Maybe the suffix repository is indicating as if repository classes are repository pattern but they are not. I used that naming initally to ensure no code was written directly in endpoints so that code can be reused in non-web apps. Day by day, usefulness of repositories are decreasing and maybe in a future version we may remove them altogether. They are just there because of familiarity and compatibility reasons.
• Behaviors themselves require dependency injection via constructors, thus the factory which creates behaviors has to have a reference to service provider as we don't know what services they might need priori. Another reason for IRequestContext.
• I don't like registering concrete classes as services. E.g. AddSingleton<SomeRepository>. This means we would need to also add an interface, ISomeRepository, it also means we would need to add a method to the interface, the repository, and endpoint. So instead of 2, 3 for just adding a method. I am even thinking about removing endpoints altogether, as i find it cumbersome having to add a method in two places, and i say no to write a method in three places.
• Not all repositories have all methods (Create, Update, Delete, List, Retrieve) or some even has more methods, so defining a generic repository class does not solve the main issue and you would have to register same repository for multiple interfaces if you choose to still use a generic class.
• We have thousands of repository tests and didn't have any problem testing the repository / endpoint code as it is currently designed.
• Current design does not prevent you from using full abstraction if you wish to do it so.

Let's make a simple repository interface:

public interface ISaveRepository<TEntity>
...

public interface IRepository<TEntity> : ISaveRepository<TEntity>, IListRepository<TEntity>, .....
{
...
}

// public interface IMySampleRepository // id prefer this
....

public class MySampleRepository : IRepository<MySampleRow>
// or public class MySampleRepository : IMySampleRepository // the version i'd prefer 
{
     public MySampleRepository(IRequestContext requestContext, IAnotherService another) 
     {
     }

}
services.AddTransient<ISaveRepository<MySampleRow>, MySampleRepository>(); 
//services.AddTransient<IMyRepository, MySampleRepository>(); 


public class MySampleController : ServiceEndpoint 
{
    public MySampleController(IRepository<MyRow> repository)
...

    public void Save(IDbConnection connection, SaveRequest<MyRow> request)
    {
        repository.Save(connection, request);        
    }
}

So it works just fine.

[volkanceylan]

As a side note, i completely agree with all below points:

Observe the following:

Each of the three classes shown have increased isolation from each other:
The OrderEndpoint does not need to be aware of OrderRepository's dependencies.
The OrderRepository does not need to be aware of OrderDetailRepository's dependencies.
All classes are ONLY responsible for its own dependencies, not the dependencies of its dependencies
Additionally, if a Repository's dependencies need to change, they only need to change in one place

And think your design is good. Just i don't want to make it the default.