Problem Description
I am working on an e-commerce website, when a user wants to sell a product, he would open the product page and he can upload up to 12 photos:
Image Upload Process
This is the process that I follow for saving images. Once user drops an image on the uploader, it would get saved with a temporary prefix and a timespan. So for example when user drops 3 images, I would have:
tmp-0-20190911123456787.jpg
tmp-1-20190911123456777.jpg
tmp-2-20190911123457777.jpg
Now once the user completes the process, he clicks on DONE button, I create an array of 12 strings, representing the latest image names in the 12 uploaders.
Then I open the corresponding folder (where product's temporary images are dropped). I get all images dropped in the folder, if the images is present in the array, I rename it to a permanent image name, if the uploaded image is not present in the array, I would delete the temp image from the upload folder. This is how photos folder looks like once user click on DONE:
img-0-20190911123456787-product-title-is-appended-in-image-name.jpg
img-1-20190911123456777-product-title-is-appended-in-image-name.jpg
img-2-20190911123457777-product-title-is-appended-in-image-name.jpg
Infrastructure Consideration
I use different file systems to save the image, in case of Dev environment the images are stored in local file system. In case of Prod environment, images are stored in AWS S3 Bucket.
DDD/Architecture Considerations
I want to follow DDD principles and also I want to keep the code DRY. Since I have two different infrastructure layers (S3 & local disk) I want to remove as much logic as possible from the infrastructure layer (so I don't have to repeat the logic in each infrastructure layer).
Since I want min logic in Infrastructure layer, I need to put the logic somewhere else. (But where should this logic go?)
I assume in DDD, domain is a good place for putting the logic, but at the same time, in DDD an entity should not have dependency on infrastructure layer, and here, FileInfo (i.e. System.IO) is the infrastructure layer for Disk I/O/... I am finding it very difficult to think of a domain object which does not include FileInfo or FileStream.
So finally this is how my DDD layers look like:
The Code:
Infrastructure Layer Contract
IImageRepository.cs
This interface defines the contracts the S3 and Disk IO should implement:
public interface IImageRepository
{
string CreateAdFolder(string folderName);
List<string> GetPermanentImageNames(string photosUrlPath);
string UploadTempImage(FileUpload tempImageUpload);
void SavePermanentImages(List<string> curPhotos, string photosUrlPath, ImageInfo imageInfo);
}
Domain Objects (this is only for Upload Bounded Context)
FileUpload.cs
// when user upload an images, we save the file with a temporary prefix, once user Save the form we rename the temp image name to a permanent image name
// user may upload a couple of images, each image is uploaded through an Uploader. In case of advertisement we have 12 uploaders on the form, but in case of
// profile picture, we only have 1 uploader on the form. Regardless of number of uploaders on the form, user may upload several phones on the same uploader
// befor hitting the save button. When user hits save, we need to delete all the temporary images except the final version when is currently in the uploader
// we need to rename this final images from temp name to a permanent image name
public class FileUpload
{
public FileUpload(HttpPostedFileBase fileToUpload, string urlPath, string upoaderNumber)
{
FileToUpload = fileToUpload;
UrlPath = urlPath;
UploaderNumber = upoaderNumber;
}
public HttpPostedFileBase FileToUpload { get; }
public string FileName
{
get
{
return FileToUpload.FileName;
}
}
public string UrlPath { get; }
public string UploaderNumber { get; }
public void ValidateUploaderNumber()
{
// we have 12 image uploaders on the screen, and they are associated with 12 files in ad folder, we need to check uploaderNumber is within this range
if (int.TryParse(UploaderNumber, out int uploaderNumber))
{
if (uploaderNumber >= 0 && uploaderNumber <= GlobalConstants.NoOfImagesPerAd)
{
return;
}
}
throw new Exception($"Invalid Upload Request: Image Uploader Number: {UploaderNumber}");
}
}
ImageInfo.cs
// Details that we need for saving an image, these details would saved as the text properties of the image
public class ImageInfo
{
public ImageInfo(string title, string category)
{
Title = title;
Category = category;
}
public string Title;
public string Category;
public string GetJPEGTitle()
{
return $"{Category}: {Title} - Buy and Sell";
}
public string GetJPEGComment()
{
return $"Buy and Sell for Free from Shopless {GlobalConfig.CountryLongName}";
}
public string GetJPEGCopyrite()
{
return $"Shopless {DateTime.Now.Year.ToString()}";
}
}
UserUploadPath.cs
public class UserPhotosPath
{
public UserPhotosPath(long userId)
{
UserId = userId;
}
public long UserId { get; }
/// <summary>
/// each user has his own images folder, append folder name under user's images folder
/// </summary>
/// <param name="folderName"></param>
/// <returns>// returns something like: /file-uploads/images/24/$(foldername)</returns>
public string AppendtoUserPhotosUrlPath(string folderName)
{
return $"{GetUserPhotosUrl()}{folderName}";
}
public void ValidateUserPermissionToPath(string photosUrlPath)
{
if (string.IsNullOrEmpty(photosUrlPath))
{
throw new Exception($"Invalid empthry Path, UserId: {UserId}");
}
// since photosUrlPath is coming from the browser, a hacker could construct a path with a different userId and upload images to that path
// here, we need to validate the requested path matches the current user's photo upload folder
string curUserPath = GetUserPhotosUrl().TrimStart('/');
string trimmedPhotosUrlPath = photosUrlPath.TrimStart(new char[] { '/' });
if (trimmedPhotosUrlPath.StartsWith(curUserPath, StringComparison.InvariantCultureIgnoreCase))
{
return;
}
throw new Exception($"Access denied for UserId: {UserId}, Path: {photosUrlPath}");
}
// returns something like: /file-uploads/images/24/
private string GetUserPhotosUrl()
{
return $"{GlobalConfig.ImageUploadRelativeRoot}{GlobalConstants.UrlSeparator}{UserId.ToString()}{GlobalConstants.UrlSeparator}";
}
}
Common Helper
I could not think of any place to keep this logic (this is for generating temporary and permanent image name) so I have created a static helper class in Common project:
ImageNameHelper.cs
public static class ImageNameHelper
{
public static string GenerateUniqueAdFolderName()
{
return GetIncrementalUniqueString();
}
/// <summary>
/// Permanent images name should be generated using this pattern: img(UploaderNumber)-(dash-separated-title)-(timestamp).(originalFileExtension)
/// </summary>
/// <param name="fileName">original/temporary file name</param>
/// <param name="imageUploaderNumber">image uploader number</param>
/// <param name="title">Title of the advertisement</param>
/// <returns>something like this: img1-beautiful-scarf-for-sale-20180423134055768-hashcode.jpg</returns>
public static string GenerateUniquePermanentImageName(string fileName, string imageUploaderNumber, string title)
{
title = title.ToDashSeparatedString();
if (string.IsNullOrEmpty(title))
{
title = "picture";
}
return $"{GetPermanentPrefix(imageUploaderNumber)}{title}-{GetIncrementalUniqueString()}{Path.GetExtension(fileName)}";
}
/// <summary>
/// Temporary images name should be generated using this pattern: tmp(uploaderNumber)-(TimeStamp-UniqueCode).(originalFileExtension)
/// </summary>
/// <param name="fileName">OrgiginalImageName.jpg</param>
/// <param name="imageUploaderNumber">uploader number, should be between 0 to 11</param>
/// <returns>something like this: tmp-1-TimeStamp-Guid.jpg</returns>
public static string GenerateUniqueTemporaryImageName(string fileName, string imageUploaderNumber)
{
return GetTemporarytPrefix(imageUploaderNumber) + GetIncrementalUniqueString() + Path.GetExtension(fileName);
}
public static string GetPermanentImagePrefixPattern()
{
return GlobalConstants.PermanentPrefix + "*";
}
public static string GetTemporaryImagePrefixPatternForUploader(string imageUploaderNumber)
{
// all temporary images with uploader 1 should start with tmp-1 (pattern would be "tmp1-*")
return GetTemporarytPrefix(imageUploaderNumber) + "*";
}
public static bool DoesImageHaveCorrectPermanentPrefix(string imageName, string imageUploaderNumber)
{
if (imageName.StartsWith(GetPermanentPrefix(imageUploaderNumber), StringComparison.InvariantCultureIgnoreCase))
{
return true;
}
return false;
}
private static string GetPermanentPrefix(string imageUploaderNumber)
{
return GlobalConstants.PermanentPrefix + imageUploaderNumber + "-";
}
private static string GetTemporarytPrefix(string imageUploaderNumber)
{
return GlobalConstants.TemporaryPrefix + imageUploaderNumber + "-";
}
// I am using TimeStap to ensure names generated names are incremental and a Guid to ensure they are unique
private static string GetIncrementalUniqueString()
{
return DateTime.Now.ToCompactDateTimeString() + "-" + Math.Abs(Guid.NewGuid().GetHashCode()).ToString();
}
}
Infrastructure Layer (Disk)
This is the implementation of the contracts for saving images in the file system:
public class DiskImageRepository : IImageRepository
{
private UserPhotosPath _userPath;
private DiskPhysicalPathMapper _physicalPathMapper;
public DiskImageRepository(UserPhotosPath userPath, DiskPhysicalPathMapper physicalPathBuilder)
{
_userPath = userPath;
_physicalPathMapper = physicalPathBuilder;
}
public string CreateAdFolder(string folderName)
{
string photosUrlPath = _userPath.AppendtoUserPhotosUrlPath(folderName);
// no need to check if User directory already exists or not, since it would be automatically created if missing
// Create directory will throw exception if it cannot create the directory
string physicalPath = _physicalPathMapper.ConvertUrlToPhysicalPath(photosUrlPath);
Directory.CreateDirectory(physicalPath);
return photosUrlPath;
}
public List<string> GetPermanentImageNames(string photosUrlPath)
{
int i = 0;
List<string> photos = new List<string>(new string[GlobalConstants.NoOfImagesPerAd]); // initialize list to contain 12 elements
string physicalPath = GetValidatedPhysicalPath(photosUrlPath);
FileInfo[] images = GetFiles(physicalPath, ImageNameHelper.GetPermanentImagePrefixPattern());
foreach (FileInfo image in images)
{
if (i >= GlobalConstants.NoOfImagesPerAd)
{
LogConfig.Logger.Error($"{photosUrlPath} contains more than {GlobalConstants.NoOfImagesPerAd} files, the cause need to be investigated.");
break;
}
photos[i++] = image.Name;
}
return photos;
}
public string UploadTempImage(FileUpload tempImageUpload)
{
// exception is thrown if any of the following validation fails
_userPath.ValidateUserPermissionToPath(tempImageUpload.UrlPath);
tempImageUpload.ValidateUploaderNumber();
var physicalPath = GetValidatedPhysicalPath(tempImageUpload.UrlPath);
DeleteSuperceededTemporaryImagesByUploaderNumber(physicalPath, tempImageUpload.UploaderNumber);
var tmpFileName = ImageNameHelper.GenerateUniqueTemporaryImageName(tempImageUpload.FileName, tempImageUpload.UploaderNumber);
var fullPhysicalPath = Path.Combine(physicalPath, tmpFileName);
tempImageUpload.FileToUpload.SaveAs(fullPhysicalPath);
return tmpFileName;
}
public void SavePermanentImages(List<string> curPhotos, string photosUrlPath, ImageInfo imageInfo)
{
_userPath.ValidateUserPermissionToPath(photosUrlPath);
var physicalPath = GetValidatedPhysicalPath(photosUrlPath);
FileInfo[] images = GetFiles(physicalPath);
for (int i = 0; i < curPhotos.Count; i++)
{
if (!string.IsNullOrEmpty(curPhotos[i]))
{
if (images.Where(img => string.Equals(img.Name, curPhotos[i], StringComparison.InvariantCultureIgnoreCase)).Any() == false)
{
LogConfig.Logger.Error($"photo: {curPhotos[i]}, was not found in ad folder: {photosUrlPath}. This is either a bug or a malicious request.");
curPhotos[i] = string.Empty;
}
}
}
foreach (FileInfo image in images)
{
var index = curPhotos.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(), imageInfo.Title);
string fullPhysicalPath = Path.Combine(physicalPath, permanentImageName);
image.MoveTo(fullPhysicalPath);
SetJpegMetadata(fullPhysicalPath, imageInfo);
curPhotos[index] = permanentImageName;
}
}
else
{
image.Delete();
}
}
}
private string GetValidatedPhysicalPath(string urlPath)
{
string physicalPath = _physicalPathMapper.ConvertUrlToPhysicalPath(urlPath);
if (!Directory.Exists(physicalPath))
{
throw new Exception($"Path does not exists: {physicalPath}");
}
return physicalPath;
}
// We have 12 images uploader on the screen which correspond to 12 files (place holders) in ad folder, on the disk drive. If an image is
// uploaded on uploader 1, we need to delete all other temporary images uploaded on uploader 1. We should not delete permanent images at
// this stage, because user may not saves changes, and in that case we need to delete all temporary images
// NOTE: it is considered that path and uploader number are already validated.
private void DeleteSuperceededTemporaryImagesByUploaderNumber(string adFolderFullPhysicalPath, string uploaderNumber)
{
FileInfo[] oldTmpImages = GetFiles(adFolderFullPhysicalPath, ImageNameHelper.GetTemporaryImagePrefixPatternForUploader(uploaderNumber));
foreach (var oldTmpImage in oldTmpImages)
{
oldTmpImage.Delete();
}
}
private FileInfo[] GetFiles(string physicalPath, string searchPattern = "")
{
if (string.IsNullOrEmpty(searchPattern))
{
searchPattern = "*";
}
DirectoryInfo di = new DirectoryInfo(physicalPath);
return di.GetFiles(searchPattern).OrderBy(x => x.Name.PadNumbersForAlphanumericSort()).ToArray();
}
private void SetJpegMetadata(string physicalPathToJpeg, ImageInfo imageInfo)
{
var jpeg = new JpegMetadataAdapter(physicalPathToJpeg);
jpeg.Metadata.Title = imageInfo.GetJPEGTitle();
jpeg.Metadata.Comment = imageInfo.GetJPEGComment();
jpeg.Metadata.Copyright = imageInfo.GetJPEGCopyrite();
jpeg.Save();
}
}
DiskPhysicalPathMapper.cs
This is just a mapper to translate URL paths to Physical Path (for Disk):
public class DiskPhysicalPathMapper
{
private readonly string _physicalRoot;
private readonly string _urlRoot;
public DiskPhysicalPathMapper(string urlRoot, string physicalRoot)
{
_urlRoot = urlRoot;
_physicalRoot = physicalRoot;
}
public string ConvertUrlToPhysicalPath(string urlPath)
{
if (string.IsNullOrEmpty(urlPath))
{
throw new Exception($"invalid photosUrlPath: {urlPath}");
}
// replace url root with physical root, case insensitive
var path = Regex.Replace(urlPath, _urlRoot, _physicalRoot, RegexOptions.IgnoreCase);
return path.Replace(GlobalConstants.UrlSeparator, @"\");
}
}
I also have a JPegMethaDataAdapter class that I have not included in the review, it just sets JPEG image properties.
I have also not included the S3 infrastructure layer.
