Grouping low-level services together into a high-level service

Question

I am working on an e-commerce website. What I need to do is to save an advertisement. The ad has some properties and some photos... properties should be save to DB and photos should be saved to file system (I am using an amazon S3 bucket for photo storage).

So, in order to simplify the process of saving an ad, I have created a new server (an umbrella/combined service) which brings DB-Repository and File-Writer services together, I have called it AdPersister:

// an ad data is written to DB, while its photos are written to file system (S3 bucket)
// in order to save the ad to persistant storage we need to use AdPersister which 
// internaly uses both AdRepository & AdImagePersister (internal classes) to save the ad
public class AdPersister<TEntity> : IAdPersister<TEntity>
    where TEntity : AdBase
{
    private AdRepository<TEntity> _adRepository;
    private IAdImagePersister _adImagePersister;

    /*
     * This constructor is tightly coupled to AdRepository and S3AdImagePersister
     * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     */
    public AdPersister(ApplicationDbContext applicationDbContext)
    {
        // these services are internal to infrastruce layer, the reason for them being 
        // internal is that we don't want the outside world to use them directly
        // all the communication with these services should go through AdPersister
        _adRepository = new AdRepository<TEntity>(applicationDbContext);
        _adImagePersister = new S3AdImagePersister();
    }

    public Type GetAdType()
    {
        return typeof(TEntity);
    }

    public void CreateFolder(string photosFolderRelativePath, long userId)
    {
        _adImagePersister.CreateFolder(photosFolderRelativePath, userId);
    }

    public TEntity GetActiveAd(long adBaseId)
    {
        TEntity ad = _adRepository.GetActiveAd(adBaseId);
        LoadImages(ad);
        return ad;
    }

    public TEntity GetAdIfUserHasPermission(long adBaseId, long userId)
    {
        TEntity ad = _adRepository.GetAdIfUserHasPermission(adBaseId, userId);
        LoadImages(ad);
        return ad;
    }

    public void AddOrUpdate(TEntity adBase)
    {
        // Important: call BuildPermanentPhotoNamesAndSave before saving ad to the DB
        // as it changes photo names to permanent photo names
        _adImagePersister.BuildPermanentPhotoNamesAndSave(adBase);
        _adRepository.AddOrUpdate(adBase);
    }

    public void AddOrUpdate(List<TEntity> adBases)
    {
        // Important: call BuildPermanentPhotoNamesAndSave before saving ad to the DB 
        // as it changes photo names to permanent photo names
        foreach (TEntity adBase in adBases)
        {
            _adImagePersister.BuildPermanentPhotoNamesAndSave(adBase);
        }

        _adRepository.AddOrUpdate(adBases);
    }

    public void InactivateAds(List<long> adBaseIds)
    {
        _adRepository.InacticaveAds(adBaseIds);
    }

    private void LoadImages(TEntity ad)
    {
        ad.Photos = _adImagePersister.GetAdPhotos(ad.PhotosFolderRelativePath);
    }
}

AdPersister is in my infrastructure layer and under the hood uses AdRepository and IAdImagePersister which are also defined in the infrastructure layer.

The challenge is, I want to enforce the outside world (the web project) to use AdPersister for adding/updating ads... so I have made AdRepository and IAdImagePersister internal to infrastructure layer. The outside world knows only about AdPersister.

This is my AdRepository class:

// don't call this class from outside world, use AdPersister
internal class AdRepository<TEntity> 
    where TEntity : AdBase
{
    private ApplicationDbContext _context;

    public AdRepository(ApplicationDbContext context)
    {
        _context = context;
    }

    internal Type GetAdType()
    {
        return typeof(TEntity);
    }

    internal TEntity GetActiveAd(long adBaseId)
    {
        return _context.AdBase.OfType<TEntity>()
                    .AsNoTracking()
                    .Include(r => r.Address)
                    .Include(r => r.UserContact)
                    .Where(r => r.IsActive == true && r.AdBaseId == adBaseId)
                    .FirstOrDefault();
    }

    internal TEntity GetAdIfUserHasPermission(long adBaseId, long userId)
    {
        return _context.AdBase.OfType<TEntity>()
                        .AsNoTracking()
                        .Include(r => r.Address)
                        .Include(r => r.UserContact)
                        .Where(a => a.AdBaseId == adBaseId && a.UserId == userId)
                        .FirstOrDefault();
    }

    internal void AddOrUpdate(TEntity adBase)
    {
        if (adBase.AdBaseId > 0)
        {
            // update existing ad
            var adBasecurState = GetAdBaseCurState(adBase.AdBaseId);
            if (adBase.DoesUserHavePermission(adBasecurState) == false)
            {
                throw new Exception($"This account does not have modification permission for Ad: {adBase.AdBaseId}");
            }

            PrepareContextForAdUpdate(adBasecurState, adBase);
        }
        else
        {
            PrepareContextForAdInsert(adBase);
        }

        _context.SaveChanges();
    }

    internal void AddOrUpdate(List<TEntity> adBases)
    {
        List<long> searchIds = adBases.Select(ad => ad.AdBaseId).ToList();
        var adBaseCurStateList = _context.AdBase.OfType<TEntity>().Where(ab => searchIds.Contains(ab.AdBaseId)).ToList();

        foreach (TEntity adBase in adBases)
        {
            var adBaseCurState = adBaseCurStateList.Where(cs => cs.AdBaseId == adBase.AdBaseId).FirstOrDefault();

            if (adBase.AdBaseId > 0)
            {
                PrepareContextForAdUpdate(adBaseCurState, adBase);
            }
            else
            {
                PrepareContextForAdInsert(adBase);
            }
        }

        _context.SaveChanges();
    }

    internal void InacticaveAds(List<long> adBaseIds)
    {
        if (adBaseIds.Count > 0)
        {
            var ads = _context.AdBase.OfType<TEntity>().Where(ab => adBaseIds.Contains(ab.AdBaseId)).ToList();
            foreach (var ad in ads)
            {
                ad.IsActive = false;
                ad.IsDeleted = true;
            }

            _context.SaveChanges();
        }
    }

    private TEntity GetAdBaseCurState(long adBaseId)
    {
        return _context.AdBase.OfType<TEntity>()
                .AsNoTracking()
                .Where(r => r.AdBaseId == adBaseId)
                .FirstOrDefault();
    }

    private void PrepareContextForAdUpdate(TEntity adBaseCurState, TEntity adBaseNewState)
    {
        adBaseNewState.SetStartDate(adBaseCurState);
        _context.AdBase.Attach(adBaseNewState);
        _context.Entry(adBaseNewState).State = EntityState.Modified;
        _context.Entry(adBaseNewState).Property(x => x.UserId).IsModified = false;
        _context.Entry(adBaseNewState.UserContact).State = EntityState.Detached; // <-- Don't update UserContact View
        _context.Entry(adBaseNewState.Address).State = EntityState.Modified;
    }

    private void PrepareContextForAdInsert(TEntity adBase)
    {
        adBase.SetStartDate();
        _context.AdBase.Add(adBase);
        _context.Entry(adBase.UserContact).State = EntityState.Detached;   
    }
}

This is IAdImagePersister interface (note that it is also internal):

internal interface IAdImagePersister
{
    void CreateFolder(string photosFolderRelativePath, long userId);

    List<string> GetAdPhotos(string photosFolderRelativePath);

    void BuildPermanentPhotoNamesAndSave(AdBase ad);
}

And this is S3AdImagePersister which implements IAdImagePersister

// don't call this class from outside world, use AdPersister 
internal class S3AdImagePersister : S3CdnBase, IAdImagePersister
{
    public void CreateFolder(string photosFolderRelativePath, long userId)
    {
        PathBuilder.ValidateUserPermissionToPath(photosFolderRelativePath, userId);
        string s3Path = S3PathMapper.GetS3PathForHighLevelApi(photosFolderRelativePath);
        S3DirectoryInfo di = new S3DirectoryInfo(_cdnClient, _cdnBucketName, s3Path);
        if (di.Exists)
        {
            throw new Exception($"{photosFolderRelativePath} already exists.");
        }

        di.Create();
    }

    public List<string> GetAdPhotos(string photosFolderRelativePath)
    {
        int i = 0;
        List<string> photos = new List<string>(new string[GlobalConstants.NoOfPhotosPerAd]); // initialize list to contain 16 elements
        var s3Photos = GetFiles(photosFolderRelativePath, ImageNameHelper.GetPermanentImagePrefixPattern());

        foreach (S3FileInfo s3Photo in s3Photos)
        {
            if (i >= GlobalConstants.NoOfPhotosPerAd)
            {
                WebLog.Logger.Error($"{photosFolderRelativePath} contains more than {GlobalConstants.NoOfPhotosPerAd} files, the cause need to be investigated.");
                break;
            }

            photos[i++] = s3Photo.Name;
        }

        return photos;
    }

    public void BuildPermanentPhotoNamesAndSave(AdBase ad)
    {
        ad.ValidateUserPermissionToPath();
        var images = GetFiles(ad.PhotosFolderRelativePath);

        for (int i = 0; i < ad.Photos.Count; i++)
        {
            if (!string.IsNullOrEmpty(ad.Photos[i]))
            {
                if (images.Where(img => string.Equals(img.Name, ad.Photos[i], StringComparison.InvariantCultureIgnoreCase)).Any() == false)
                {
                    WebLog.Logger.Error($"photo: {ad.Photos[i]}, was not found in ad folder: {ad.PhotosFolderRelativePath}. This is either a bug or a malicious request.");
                    ad.Photos[i] = string.Empty;
                }
            }
        }

        foreach (S3FileInfo image in images)
        {
            var index = ad.Photos.FindIndex(p => p.Equals(image.Name, StringComparison.InvariantCultureIgnoreCase));
            if (index >= 0)
            {
                if (ImageNameHelper.DoesImageHaveCorrectPermanentPrefix(image.Name, index.ToString()) == false)
                {
                    var permanentImageName = ImageNameHelper.GenerateUniquePermanentImageName(image.Name, index.ToString(), ad.Title);
                    string fullS3Path = S3PathMapper.CombineHighLevelPath(ad.PhotosFolderRelativePath, permanentImageName);
                    image.MoveTo(_cdnBucketName, fullS3Path);
                    ad.Photos[index] = permanentImageName;
                }
            }
            else
            {
                image.Delete();
            }
        }
    }
}

The above code has the benefit of hiding the internals of AdPersister from the web project and it ensures that anyone wanting to change an ad has to use AdPersister...

The drawback is that I cannot use DI to instantiate the internal classes, what I mean is, the DI wiring code in the web project does not know about the internal classes, so I cannot inject them.

So as shown earlier the constructor of AdPersister is tightly coupled with the internal classes:

public AdPersister(ApplicationDbContext applicationDbContext)
{
    _adRepository = new AdRepository<TEntity>(applicationDbContext);
    _adImagePersister = new S3AdImagePersister();
}

This is my DI code in the web project (using ninject):

private static void RegisterServices(IKernel kernel)
{
    kernel.Bind<ApplicationDbContext>().ToSelf().InRequestScope();
    Kernel.Bind(typeof(IAdPersister<>)).To(typeof(AdPersister<>)).InRequestScope();

   /* I am no longer able to inject the internal classes */
   //kernel.Bind<IAdImagePersister>().To<S3AdImagePersister>().InRequestScope();
   //kernel.Bind(typeof(AdRepository<>)).ToSelf().InRequestScope();
}

Any feedback on this approach? Is there a better way that I could initialize AdPersister class and remove the tight coupling from its constructor?

Answer 1

First refactor the implementation to use an internal constructor

internal AdPersister(AdRepository<TEntity> adRepository, IAdImagePersister adImagePersister){
    _adRepository = adRepository;
    _adImagePersister = adImagePersister;
}

Effectively hiding it from use externally and giving you full controll of the class's activation

The lower layer in this case would need to expose an extensibility point to populate the used container by implementing the IProvider interface (in Ninject.Activation)

Infrastructure Layer

public class AdPersisterProvider : IProvider {
    public Type Type => typeof(AdPersister<>);

    public object Create(IContext context) {
        var genericArguments = context.GenericArguments;
        var genericType = this.Type.MakeGenericType(genericArguments); //AdPersister<T>

        var dbContextType = typeof(ApplicationDbContext);
        var repoGenericType  = typeof(AdRepository<>).MakeGenericType(genericArguments);

        var dbContext = context.Kernel.Get(dbContextType); //assumed registered
        var adRepository = Activator.CreateInstance(repoGenericType, dbContext); //dbContext injected
        var adImagePersister = new S3AdImagePersister();

        var argTypes = new [] {
            repoGenericType,
            typeof(IAdImagePersister)
        };
         // Get the internal constructor that take the provided arguments
        var constructor = genericType.GetConstructor(BindingFlags.Instance | BindingFlags.NonPublic, null, argTypes, null);

        var parameters = new object[] {
            adRepository,
            adImagePersister
        };
        return constructor.Invoke(parameters);
    }
}

And used in composition root in the web project (using ninject):

private static void RegisterServices(IKernel kernel) {
    kernel.Bind<ApplicationDbContext>().ToSelf().InRequestScope();
    kernel.Bind(typeof(IAdPersister<>)).ToProvider<AdPersisterProvider>().InRequestScope();
}