I'm struggeling with a piece of code, and could use some pointers.
I'm creating an API for a frontend website. This API can call various backend systems based on the tenant. Which system to call is saved in a tenant entry in the database.
My API project consists of a couple of different projects:
My API calls my logic layer, for example to retrieve a customer object. In this call a tenantId is passed (which is taken from the authorized identity claim). My logic does some checks and then wants to call the tenant specific backend system. To do this, I need to do the following:
The data class implements an interface IBackendConnector.
Because all my logic classes could potentially call the tenant specific data, i want to create a single factory that accepts a tenantId, and returns a class that implements IBackendConnector. The call to the database to retrieve the tenant is async, and the API call itself is async.
I created an abstract class called 'DataFactory' that has the (public and private) property IBackendConnector, lets name it Connector. It also has a constructor that expects a tenantid, which it saves as a private property. Every logic class implements this DataFactory. The moment i need to call the backend, I can simple call some nice and clean code:
Connector.MethodIWhishToCall();
In the getter of the public property I check if there is a value in the private property, and if not, it needs to retrieve the tenant settings, and create the correct data class, so we only create the class when we need it. Here is where i failed, because i can't call the async method in this property getter (without deadlocking).
I changed this code a lot, and i'm not happy with the current solution i'm using, which is an abstract class that is implemented in every logic class (that needs it), with a static async method in it.
public abstract class ExternalDataFactory
{
public static async Task<IBackendDataConnector> GetDataHandler(string tenantId)
{
var logic = new TenantLogic();
var tenant = await logic.GetTenantById(tenantId);
var settings = tenant.Settings;
if (settings == null)
throw new ArgumentNullException("Settings");
switch (settings.EnvironmentType)
{
case Constants.EnvironmentType.ExampleA:
return new DataHandlerClass(settings);
case Constants.EnvironmentType.ExampleB:
throw new NotImplementedException();
default:
throw new Exception("Invalid EnvironmentType");
};
}
}
And every method that uses this handler calls the following code:
var handler = await GetDataHandler(tenantId);
return await handler.DoSomething();
Or in a single line:
return await (await GetDataHandler(tenantId)).DoSomething();
The ultimate goal: All logic classes must be able to call the correct data class without having to worry which data class, or having to manually retrieve the settings first and pass it to the factory. The factory should be able to retrieve the settings (if the settings are null) and return the correct IBackendConnector implementation. What is best-practice / best pattern to do this?
TL/DR: Trying to implement a factory pattern that needs to call an async method to decide which concrete class to return. What is the best way to go about this?
I don't see any good reason in your code to define abstract class, when only method it defines is static. So first thing I would suggest is to make your class non abstract.
Let's rename GetDataHandler to CreateDataHandlerAsync to clearly indicate to consumers of your factory that this method is async. You can also see I removed static from the method signature.
To me it does too many things, let's make TenantLogic a dependency for this factory.
public interface ITenantLogic
{
Task<Tenant> GetTenantByIdAsync(int tenantId);
}
public class TenantLogic: ITenantLogic
{
public async Task<Tenant> GetTenantByIdAsync(int tenantId)
{
// logic to get the tenant goes here
}
}
Now let's define it as dependency in our factory
public class ExternalDataFactory
{
private readonly ITenantLogic _tenantLogic;
public ExternalDataFactory(ITenantLogic tenantLogic)
{
if(tenantLogic == null) throw new ArgumentNullException("tenantLogic");
_tenantLogic = tenantLogic;
}
public async Task<IBackendDataConnector> CreateDataHandlerAsync(string tenantId)
{
var tenant = await _tenantLogic.GetTenantByIdAsync(tenantId);
var settings = tenant.Settings;
if (settings == null)
throw new ArgumentException("Specified tenant has no settings defined");
switch (settings.EnvironmentType)
{
case Constants.EnvironmentType.ExampleA:
return new DataHandlerClass(settings);
case Constants.EnvironmentType.ExampleB:
throw new NotImplementedException();
default:
throw new Exception("Invalid EnvironmentType");
};
}
}
I would suggest to also change method name for DoSomething, as it is running async made it DoSomethingAsync(). As a rule of thumb I would advise to postfix Async to all your async methods.
And now the client code.
var factory = new ExternalDataFactory(new TenantLogic());
IBackendDataConnector dataConnector =
await factory.CreateDataHandlerAsync(tenantId);
return await dataConnector.DoSomethingAsync();
Last but not least, I would also think how to get rid off switch/case in CreateDataHandlerAsync method.
I think this version can be a good starting point - it will met your ultimate goal - upon supporting new environments, you'll implement the concrete data class, add a new case statement (seriously, I would remove it though) and all your clients can enjoy benefit of factory supporting a new environment.
Hope this helps.
Looking further into this I want to add one more thing.
I don't really think ExternalDataFactory should know about tenants, how to get a tenant etc.
I think consumer code of the factory should handle retrieval of tenant and pass environment down to factory, which in turn would create a concrete data class and return it.
And as I stated before, we can make factory more flexible & elegant (IMO) if we can get rid off switch/case.
Let's first redefine data connector related classes/interfaces
public interface IBackendDataConnector
{
public Task<Something> DoSomethingAsync(Settings settings);
public bool CanHandleEnvironment(EnvironmentType environment);
}
public abstract class DataHandlerAbstractBase: IBackendDataConnector
{
protected abstract EnvironmentType Environment { get; }
// interface API
public abstract async Task<Something> DoSomethingAsync(Settings settings);
public virtual bool CanHandleEnvironment(EnvironmentType environment)
{
return environment == Environment;
}
}
public class DataHandlerClass: DataHandlerAbstractBase
{
protected override EnvironmentType Environment
{
get { return EnvironmentType.ExampleA; }
}
public override async Task<Something> DoSomethingAsync(Settings settings)
{
// implementation goes here
}
}
Now with above in place let's revisit ExternalDataFactory
public class ExternalDataFactory
{
private readonly IEnumerable<IBackendDataConnector> _dataConnectors =
new [] {new DataHandlerClass() /* other implementations */}
public IBackendDataConnector CreateDataHandler(Settings setting)
{
IBackendDataConnector connector = _dataConnectors
.FirstOrDefault(
c => c.CanHandleEnvironment(setting.EnvironmentType));
if (connector == null)
{
throw new ArgumentException("Unsupported environment type");
}
return connector;
}
}
Few words on refactoring.
As stated before now factory method doesn't know/bother getting tenant by it's id etc. All it can do is create concrete data class based on passed environment (which it reads from settings passed into it).
Now each concrete data class can say whether it can handle a given environment or not, so we simply delegate call to it from factory. Every concrete data class implementation would have to define Environment property (due to inheriting from abstract base DataHandlerAbstractBase.
With these changes, now client will have to take care of getting tenant and passing it to factory
var tenantLogic = new TenantLogic();
var tenant = await tenantLogic.GetTenantByIdAsync(tenantId);
var factory = new ExternalDataFactory();
IBackendDataConnector dataConnector =
factory.CreateDataHandler(tenant.Settings);
return await dataConnector.DoSomethingAsync(tenant.Settings);