Fixing cyclic dependencies by adding a new interface

Question

Below is some code from a book which shows how cyclic dependencies:

public interface IAuditTrailAppender {
   void Append(Entity changedEntity);
}

public class SqlAuditTrailAppender : IAuditTrailAppender {
   private readonly IUserContext userContext;
   private readonly CommerceContext context;
   private readonly ITimeProvider timeProvider;

   public SqlAuditTrailAppender(IUserContext userContext, CommerceContext context, ITimeProvider timeProvider) {
      this.userContext = userContext;
      this.context = context;
      this.timeProvider = timeProvider;
   }

   public void Append(Entity changedEntity) {
      AuditEntry entry = new AuditEntry {
         UserId = this.userContext.CurrentUser.Id,
         TimeOfChange = this.timeProvider.Now, 
         EntityId = entity.Id,
         EntityType = entity.GetType().Name
      };

      this.context.AuditEntries.Add(entry);
   }
}

public class AspNetUserContextAdapter : IUserContext {
   private static HttpContextAccessor Accessor = new HttpContextAccessor();

   private readonly IUserRepository repository; 

   public AspNetUserContextAdapter(IUserRepository repository) {
      this.repository = repository;
   }

   public User CurrentUser {
      get {
         var user = Accessor.HttpContext.User;
         string userName = user.Identity.Name;
         return this.repository.GetByName(userName);
      }
   }
}

public class SqlUserRepository : IUserRepository {
   public SqlUserRepository(CommerceContext context, IAuditTrailAppender appender) {
      this.appender = appender;
      this.context = context;
   }

   public void Update(User user) {
      this.appender.Append(user);
   }

   public User GetById(Guid id) { ... } 

   public User GetByName(string name) { ... }   // <--- used by CurrentUser property of AspNetUserContextAdapter
}

You can see cyclic dependencies exist as the picture below shows:

The author says "these kind of dependency cycles are typically caused by single-responsibility principle (SRP) violation. To fix it, the author adds a new interface IUserByNameRetriever:

public interface IUserByNameRetriever {
   User GetByName(string name);
}

public class SqlUserByNameRetriever : IUserByNameRetriever {
   public SqlUserByNameRetriever(CommerceContext context) {
      this.context = context;
   }

   public User GetByName(string name) { ... }
}

public class SqlUserRepository : IUserRepository {
   public SqlUserRepository(CommerceContext context, IAuditTrailAppender appender) {
      this.appender = appender;
      this.context = context;
   }

   public void Update(User user) {
      this.appender.Append(user);
   }

   public User GetById(Guid id) { ... } 

   // public User GetByName(string name) { ... }  don't need this method anymore
}

public class AspNetUserContextAdapter : IUserContext {
   private static HttpContextAccessor Accessor = new HttpContextAccessor();

   private readonly IUserByNameRetriever retriever; 

   public AspNetUserContextAdapter(IUserByNameRetriever retriever) {
      this.retriever = retriever;
   }

   public User CurrentUser {
      get {
         var user = Accessor.HttpContext.User;
         string userName = user.Identity.Name;
         return this.retriever.GetByName(userName);
      }
   }
}

I can understand how the introduce of IAuditTrailAppender stops the dependency cycles, but I feel like it is just a workaround. I don't understand why the author said this dependency cycle is caused by SRP violation. Because if you look at SqlUserRepository, its GetByName method is a nature to have it in the class just like GetById method (consumers can search a user by its id, and of course, it is nature for consumers to search a user by name), I can't see why having GetByName method in SqlUserRepository is a SRP violation?

Answer 1

It's pretty hard to see, but the problem is with the definition of IUserRepository.

The question is: Is this object supposed to be used across multiple requests, to provide the user repository to the system as a whole, or is it created for each request to provide a user repository to that specific requests. Both types of repository are used in practice, but the original example seems to want it to be both at the same time, which is indeed an SRP problem.

If it's intended to be used across multiple requests, then it doesn't make sense that it logs its audit trail in the context of one specific user. A user repository's audit logger should not then require a IUserContext. This is not what the author has decided.

The author has decided to go the other way, and construct a IUserRepository for each request, so that all of its operations are performed in the context of the specific user that performs them -- probably a good idea. That means that the audit logger is fine, but it doesn't make sense that you would require an IUserRepository to create a user context in the first place. You can't perform any of the dangerous user repository methods without a user context to audit them against.

So you need a way to bootstrap a user context without a full repository. The narrower IUserByNameRetriever class fills this role. Either it cannot perform any sensitive operations that require logging, or it creates adequate logs through some other path that doesn't require a user-tied IAuditTrailAppender.