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?