Is this an abuse of dependency injection? (when are dependencies not dependencies)

Question

We have a multi-tenant web application in which a many pages operate per-tenant. As a result many of our interfaces look like this

interface ISprocketDeployer
{
    void DeploySprocket(int tenantId);
}

It occurred to me that it might be better to simplify these interfaces to be unaware of the tenantId. The pages would also then be unaware of the tenantId, like so

[Inject] // Ninject
public ISprocketDeployer SprocketDeployer { get; set; }

private void _button_OnClick(object sender, EventArgs e)
{
    SprocketDeployer.DeploySprocket();
}

The dependency injection framework would then inject the tenant ID as a dependency by looking at the currently authenticated user. Is this a good idea or just an abuse of dependency injection?

It further occurred to me that many implementations also take additional dependencies just for looking up details about the tenant, and that I could reduce the number of dependencies further by just injecting in that detail directly, for example

class SprocketDeployer
{
    public SprocketDeployer(ITenantRepository tenantRepository)
    {
        _tenantRepository = tenantRepository;
    }

    void DeploySprocket(int tenantId)
    {
        var tenantName = _tenantRepository.GetTenant(tenantId).Name;
        // Do stuff with tenantName
    }
}

Would become

class SprocketDeployer
{
    public SprocketDeployer(Tenant tenant)
    {
        _tenant = tenant;
    }

    void DeploySprocket()
    {
        var tenantName = _tenant.Name;
        // Do stuff with tenantName
    }
}

I then realised that I could also inject in other "dependencies", such as details about the currently logged in user in the same way.

At that point I become unsure. While it seemed like a fantastic idea at first I realised that I wasn't sure when to stop adding extra "dependencies". How do I decide what should be a dependency and what should be a parameter?

Origin

At first blush, I tend to think of dependencies as services needed. Data itself is not really a dependency in my opinion. However, I think that falls into YMMV territory. Is it buying you easier development, maintainability and can meet needs of the future without much change? If so, good on you, use it. But if it's just to feel like you've adhered to DI "better", it doesn't ring true. – Complaisance

@JesseC.Slicer How about things like connection strings? They seem to be data, but are required to construct other dependencies. My experience with DI is somewhat limited, so I'm not really sure what to adhere to. – Origin

if it were me, connection strings are in fact data, but yes, construct a connection (which I feel is an injectable dependency). It's just a guess, but it seems similar distinction between primitive data types (numbers, strings, etc.) and higher order data representations (records, structs, classes, arrays, etc.) – Complaisance

If the connection string itself was needed, and it needed to be injected, you would likely want to do something similar to your TenantRepository. The implementation might just return a hard-coded string from GetConnectionString(). This would also allow you to change the implementation later to (for example) read the string from a config file. – Electrify

@PhilSandler How about with the tenantId? Would it be best to have some interface with a GetTenantId method that knows how to work out what the current tenant ID is? – Origin

See my answer--I updated it a few minutes ago. – Electrify

Whatever the case may be, you should inject an interface, not a primitive. – Flick

Answer 1

I would stop short of calling it abuse, but that said:

The general use case of dependency injection (via a container) is to inject pure services that do not directly represent state. One of the immediate problems is informing the container of which instance of your object it should be injecting at run-time. If your SprocketDeployer requires a Tenant, and your system includes many Tenants, how does the container figure out which tenant to supply at runtime?

If you want to avoid passing Tenant around, consider using Thread Local Storage (TLS). However, there will still be some point in the pipeline where the Tenant needs to be added to TLS.

Edit

From your comment:

I solve the problem of figuring out which tenant to supply at runtime in Ninject by binding the type to a method which examines HttpContext.Current and using InRequestScope. It works fine, but I've not seen anything to indicate that this is (or isn't) a recommended practice.

If I understand you correctly, that sounds like a factory of sorts? If that's the case, I see nothing wrong with it.

A minor nitpick might be: it's nice to be able to not have to be concerned about how your services are scoped. When they are truly stateless services, you can view them as pure swappable components that have no side effects based on container configuration.

Electrify

I solve the problem of figuring out which tenant to supply at runtime in Ninject by binding the type to a method which examines HttpContext.Current and using InRequestScope. It works fine, but I've not seen anything to indicate that this is (or isn't) a recommended practice. – Origin

Answer 2

As with Phil, I would not call this dependency injection abuse, though it does feel a bit odd.

You have at least a few options. I'll detail a couple that seem the best from the detail you've provided, though these may have been what you were referring to when you said 'I then realised that I could also inject in other "dependencies", such as details about the currently logged in user in the same way.'

Option 1: Abstract tenant identification to a factory

It may make perfect sense to have an abstraction that represents the current tenant. This abstraction is a factory, but I prefer the term "provider" because factory connotes creation whereas a provider may simply retrieve an existing object (Note: I realize Microsoft introduced a provider pattern but that's not what I'm referring to). In this context you're not injecting data, instead you're injecting a service. I'd probably call it ICurrentTenantProvider. The implementation is frequently context specific. Right now, for example, it would come from your HttpContext object. But, you could decide a specific customer needed their own server and then inject an ICurrentTenantProvider that would retrieve it from your web.config file.

Option 2: Hide multitenancy entirely

Unless you ever have to do different things based on the tenant[1], it may be better to hide the multitenancy entirely. In this case you'd inject classes, that I'm going to call providers, that are context aware and the result of whose function calls would be based on the current tenant. For example, you might have an ICssProvider and an IImageProvider. These providers alone would be aware that the application supported multitenancy. They may use another abstraction such as the ICurrentTenantProvider referenced above or may use the HttpContxt directly. Regardless of the implementation, they would return context specific to the tenant.

In both cases, I'd recommend injecting a service instead of data. The service provides an abstraction layer and allows you to inject an implementation that's appropriately context aware.

Making the Decision

How do I decide what should be a dependency and what should be a parameter?

I generally only ever inject services and avoid injecting things like value objects. To decide you might ask yourself some questions:

1. Would it make sense to register this object type (e.g., int tenantId) in the IoC container?
2. Is this object/type consistent for the standard lifetime of the application (e.g., instance per http request), or does it change?
3. Will most objects end up dependent on this particular object/type?
4. Would this object need to be passed around a lot if made a parameter?

For (1), it doesn't make sense to inject value objects. For (2), if it is consistent, as the tenant would be, it may be better to inject a service that's aware of the tenant. If yes to (3), it may indicate a missing abstraction. If yes to (4) you may again be missing an abstraction.

In the vein of (3) and (4) and depending on the details of the application, I could see ICurrentTenantProvider being injected in a lot of places, which may indicate it's a little low level. At that point the ICssProvider or similar abstractions may be useful.

[1] - If you inject data, like an int, you're forced to query and you may end up in a situation where you'd want to replace conditional with polymorphism.

Olein

Answer 3

Here is an example of how injecting runtime data (in their case ClaimsPrincipal) is a frequent use case. But directly injecting runtime data might not the best design...

This article explains why injecting runtime data into components in general is not a good idea. It explains that components ideally should be stateless, and that runtime data should not contain behavior, and should not be constructor-injected (should be passed across methods).

But then it mentions how passing runtime data across a chain of callers can pollute the code, and it shows examples of components that can benefit from having runtime data injected into it (Repository and more specifically Unit of Work). Injecting runtime data is especially helpful for lower layers (like data-access) because the lowest you go the more layers you would have to modify just to pass repetitive/boilerplate data.

I don't think it's an abuse, it's just a design choice which makes sense in many cases and can simplify your code tremendously. But you have to make sure you're not injecting objects with shorter lifetime (runtime data) into objects with a longer lifetime (singletons), or else your singletons could be caching the runtime data from the first call.

So we have 3 problems to solve:

1. We don't want to pass repetitive runtime data through a bunch of layers
2. We don't want to inject runtime data directly as it would affect the lifetime of our components
3. We also don't want to inject leaky-abstractions like IHttpContextAccessor into our services

To solve all those issues the safest approach is to create an adapter to resolve your runtime data (your tenant), so the lifetime mismatch is handled in a single place and we avoid dependency-resolution problems. Someone above suggested the name *Provider but I think the most common name here is *Accessor:

public class AspNetTenantAccessor : ITenantAccessor
{ 
  private IHttpContextAccessor _accessor;
  public AspNetTenantAccessor(IHttpContextAccessor accessor)
  {
    _accessor = accessor;
  }

  // or maybe Task<AppTenant> GetCurrentTenantAsync()
  public AppTenant CurrentTenant
  {
    get
    {
      // resolve based on _accessor.HttpContext
      // cache it (per-request) under _acessor.HttpContext.Items
    }
  }
}

As a comparison, ASP.NET ClaimsPrincipal is calculated by ASP.NET Middleware (cheap call based solely on cookies) and it's saved into HttpContext.User. If our tenant resolution was also cheap (or if we needed the tenant in every request) then we could also do the same (resolve tenant in the middleware pipeline and the accessor would just read from the per-context cache). If your calls involve external services (db or cache server) then I think it's better to defer the resolution.

As a curiosity, in 2015 someone from ASP.NET team showed an example of how to Inject ClaimsPrincipal in Service Layer by using an IUserAccessor adapter, but then more recently in 2021 the same team decided to register runtime data like ClaimsPrincipal/HttpRequest/HttpResponse into ASP.NET Core Minimal APIs - now all those are injectables - no need to rely on IHttpContextAccessor.

So injecting Tenant info is not an abuse. But consider using an adapter to save yourself from future headaches.

Rinna

Answer 4

10/14/15 UPDATE BEGIN

A little over three months later I've had a bit of a change of heart on the specific situation I mentioned running into with this approach.

I had mentioned that for a long time now I've also regularly injected the current "identity" (tenantAccount, user, etc.) wherever it was necessary. But, that I had ran into a situation where I needed the ability to temporarily change that identity for just a portion of the code to be executed (within the same execution thread).

Initially, a clean solution to this situation wasn't obvious to me.

I'm glad to say that in the end I did eventually come up with a viable solution - and it has been happily churning away for some time now.

It will take some time to put together an actual code sample (it's currently implemented in a proprietary system) but in the meantime here is at least a high level conceptual overview.

Note: Name the interfaces, classes, methods, etc. whatever you like - even combine things if that makes sense for you. It's just the overall concepts that are important.

First, we define an IIdentityService, exposing a GetIdenity(). This becomes the de facto dependency for getting the current identity anywhere we need it (repos, services, etc. everything uses this).

The IIdentityService implementation takes a dependency on an IIdentityServiceOrchestrator.

In my system the IIdentityServiceOrchestrator implmentation makes use mutliple IIdentityResolvers (of which only two are actually applicable to this discussion: authenticatedIdentityResolver, and manualIdentityResolver). IIdentityServiceOrchestrator exposes a .Mode property to set the active IIdentityResolver (by default this is set to 'authenticated' in my system).

Now, you could just stop there and inject the IIdentityServiceOrchestrator anywhere you needed to set the identity. But, then you'd be responsible for managing the entire process of setting and rolling back the temporary identity (setting the mode, and also backing up and restoring the identity details if it was already in manual mode, etc.).

So, the next step is to introduce an IIdentityServiceOchestratorTemporaryModeSwitcher. Yes, I know the name is long - Name it what you want. ;) This exposes two methods: SetTemporaryIdentity() and Rollback(). SetTemporaryIdentiy() is overloaded so you can set via mode or manual identity. The implementation takes a dependency on the IIdentityServiceOrchestrator and manages all the details of backing up the current existing identity details, setting the new mode/details, and rolling back the details.

Now, again you could just stop there and inject IIdentityServiceOchestratorTemporaryModeSwitcher anywhere you'd need to set the temporary identity. But, then you'd be forced to .SetTemporaryIdentity() in one place and .Rollback() in another and in practice this can get messy if it's not necessary.

So, now we finally introduce the final pieces of the puzzle: TemporaryIdentityContext and ITemporaryIdentityContextFactory.

TemporaryIdentityContext Implements IDisposable and takes a dependency on both the IIdentityServiceOchestratorTemporaryModeSwitcher and an Identity / Mode set via an overloaded constructor. In the ctor we use the IIdentityServiceOchestratorTemporaryModeSwitcher.SetTemporaryIdentity() to set the temporary identity and on dispose we call into IIdentityServiceOchestratorTemporaryModeSwitcher.Rollback to clean things up.

Now, where we need to set the identity we inject the ITemporaryIdentityContextFactory which exposes a .Create() (again overloaded for identity / mode) and this is how we procure our temporary identity contexts. The returned temporaryIdentityContext object itself isn't really touched it just exists to control the lifetime of the temporary identity.

Example flow:

// Original Identity

Using (_TemporaryIdentityContextFactory.Create(manualIdentity)) {

// Temp Identity Now in place
DoSomeStuff();

}

// Back to original Identity again..

That's pretty much it conceptually; obviously a LOT of the details have been left out.

There's also the matter of IOC lifetime that should be discussed. In its purest form as discussed here, generally each of the componenets (IIdentityService, IIdentityServiceOrchestrator, ITemporaryIdentityContextFactory) could be set to a 'PerRequest' lifetime. However, it could get funky if you happen to be spawing multiple threads from a single request... in which case you'd likely want to go with a 'perthread', etc. lifetime to ensure there was no thread crosstalk on the injections.

Ok, hope that actually helps someone (and didn't come across as completely convoluted, lol). I'll post a code sample that should clear things up further as I have time.

10/14/15 UPDATE END

---

Just wanted to chime in and say you're not alone in this practice. I've got a couple multi-tenant apps in the wild that inject the tenant information where it's needed in the same manner.

However, I have more recently ran into an issue where doing this has caused me quite a bit of grief.

Just for the sake of example lets say you have the following (very linear) dependency graph: ISomeService -> IDep2 -> IDep3 -> ISomeRepository -> ITenentInfoProvider

So, ISomeService depends IDep2, which depenends on IDep3... so on and so on until way out in some leaf ITenentInfoProvider is injected.

So, what's the problem? Well, what if in ISomeService you need to act on another tenant than the one you're currently logged in as? How do you get a different set of TenantInfo injected into ISomeRepository?

Well, some IOC containers DO have context-based conditional support (Ninject's "WhenInjectedInto", "WhenAnyAnchestorNamed" bindings for example). So, in simpler cases you could manage something hacky with those.

But what if in ISomeService you need to initiate two operations, each against a different tenant? The above solutions will fail without the introduction of multiple marker interfaces, etc. Changing your code to this extent for the sake of dependency injection just smells bad on multiple levels.

---

Now, I did come up with a container based solution, but I don't like it.

You can introduce an ITenantInfoResolverStratagy and have an implementation for each "way" of resolving the TenantInfo (AuthenticationBasedTenantInfoResolverStratagy, UserProvidedTenantInfoResolverStratagy, etc.).

Next you introduce a CurrentTenantInfoResolverStratagy (registered with the container as PerRequestLifeTime so it's a singlton for the life of your call, etc.). This can be injected anywhere you need to set the strategy that will be used by downstream clients. So, in our example we inject it into ISomeService, we set the strategy to "UserProvided" (feeding it a TenantId, etc.) and now, down the chain, when ISomeRepository asks ITenentInfoProvider for the TenantInfo, ITenentInfoProvider turns around gets it from an injected CurrentTenantInfoResolverStratagy.

Back in ISomeService, the CurrentTenantInfoResolverStratagy could be changed multiple times as needed.

So, why don't I like this?

To me, this is really just an overly complicated global variable. And in my mind just about all the problems associated globals apply here (unexpected behavior due to it being mutable by anyone at any time, concurrency issues, etc. etc. etc.).

The problem this whole thing sets out to solve (mostly just not having to pass the tenantId / tenantInfo around as a parameter) is probably just not worth the inherent issues that come with it.

---

So what's a better solution? Well there probably is some elegant thing that I'm just not thinking of (maybe some Chain Of Command implemenation?).

But, really I don't know.

It may not be elegant but passing a TenantId / TenantInfo around as a parameter in any tenant related method calls would definitely avoid this whole debacle.

If anyone else has better ideas please by all means chime in.

Chromous