I am working on an e-commerce website called Shopless, which is similar to E-bay/Gumtree. Our website integrates with other DMS systems, in order to allow organisations to import products into our system.
The following diagram shows our B2B import process (note: black lines indicate tasks performed by our system)
- Some DMS systems copy their product file into our S3 buckets
- Some DMS systems copy the product file into our FTP server and we copy the file into the S3 bucket
- Some DMS systems call into our API and give us the import file and we copy the file into our S3 bucket (this is the new functionality)
- Some DMS systems have their own API and we call into their API to get the product file
So we have a lot of different third party data types, we map all of these into a common class called NutralProduct and then we have an import process which imports these NutralProducts.
As mentioned above our API software is new, so I have decided the best solution is, when someone calls our API, we serialize the data into json and copy it into our S3 bucket, from there onwards we can use our existing B2B import process.
This code review is about building the API which involves the following task:
- Authenticating the user and checking if they have permission to the organization (client agency)
- Serializing the import objects
- Writing them into S3 bucket
Our website is developed in ASP.NET MVC 5, that's why I have decided to use ASP.NET Web API 2 (i.e. not Web API Core) to keep the versions consistent.
Authentication
I have decided to use JWT for authentication:
AuthenticationController.cs
[RoutePrefix("api")]
public class AuthenticationController : ApiController
{
private readonly ApiAuthenticationService _apiAuthenticationService;
public AuthenticationController(ApiAuthenticationService apiAuthenticationService)
{
_apiAuthenticationService = apiAuthenticationService;
}
[HttpPost]
[Route("authenticate")]
public IHttpActionResult Authenticate(AuthenticationRequestBody authenticationRequestBody)
{
var jwtToken = _apiAuthenticationService.GenerateTokenIfValid(authenticationRequestBody.ApiUserCode, authenticationRequestBody.ApiUserSecret);
if (string.IsNullOrEmpty(jwtToken))
{
return Unauthorized();
}
return Ok(jwtToken);
}
}
ApiAuthenticationService.cs
public class ApiAuthenticationService
{
private readonly IApiUserRepository _apiUserRepository;
private readonly IApiTokenIssuer _apiTokenIssuer;
public ApiAuthenticationService(IApiUserRepository apiUserRepository, IApiTokenIssuer apiTokenIssuer)
{
_apiUserRepository = apiUserRepository;
_apiTokenIssuer = apiTokenIssuer;
}
public string GenerateTokenIfValid(string apiUserCode, string apiUserSecret)
{
var apiUser = _apiUserRepository.GetApiUserByCredentials(apiUserCode, apiUserSecret);
if (apiUser == null)
{
return string.Empty;
}
return _apiTokenIssuer.GenerateTempToken(apiUser.Claims);
}
}
IApiUserRepository.cs (not including the implementation for simplicity)
public interface IApiUserRepository
{
ApiUser GetApiUserByCredentials(string apiUserCode, string apiUserSecret);
}
IApiTokenIssuer
public interface IApiTokenIssuer
{
string GenerateTempToken(List<Claim> userClaims);
}
JwtTokenIssuer.cs
public class JwtTokenIssuer : IApiTokenIssuer
{
public string GenerateTempToken(List<Claim> userClaims)
{
if (userClaims == null || userClaims.Count == 0)
{
return string.Empty;
}
var securityKey = new SymmetricSecurityKey(Encoding.ASCII.GetBytes(SensitiveConfig.JwtSecret));
var signingCredentials = new SigningCredentials(securityKey, SecurityAlgorithms.HmacSha256);
var jwtSecurityToken = new JwtSecurityToken(
B2bConfig.JwtIssuer,
B2bConstants.JwtAudience,
userClaims,
DateTime.UtcNow,
DateTime.UtcNow.AddMinutes(B2bConstants.JwtValidityDurationInMin),
signingCredentials);
return new JwtSecurityTokenHandler().WriteToken(jwtSecurityToken);
}
}
Import Process
Organisation represent a client, the following controller is import products of type Job for an organisation.
Here we are checking if the api user, has write access to organisation, before copying the import file into s3 product.
A few things to note:
- We expect the import file to contain the full list of products, anything product missing from the import list, will be deleted
- If the client wants to delete all products, they should send an empty list
OrganisationJobsController.cs
[Authorize]
[RoutePrefix("api")]
public class OrganisationJobsController : ShoplessBaseApiController
{
private readonly ApiProductUploader<JobApiDto> _apiProductUploader;
public OrganisationJobsController(ApiProductUploader<JobApiDto> apiProductUploader)
{
_apiProductUploader = apiProductUploader;
}
[HttpPost]
[Route("org/{org}/jobs/full-import")]
public IHttpActionResult FullImport(long org, [FromBody] List<JobApiDto> jobs)
{
if (!ModelState.IsValid)
{
return BadRequest(ModelState.ToString());
}
else if (org < 1)
{
return NotFound("Invalid org");
}
else if (!((ClaimsPrincipal)User).HasReadWriteAccessToOrganisation(org))
{
return Unauthorized();
}
_apiProductUploader.Upload(org, jobs, out string message);
return Accepted(message);
}
}
This is my custom base ApiController class:
ShoplessBaseApiController.cs
public class ShoplessBaseApiController : ApiController
{
protected IHttpActionResult NotFound(string message)
{
return Content(HttpStatusCode.NotFound, message);
}
protected IHttpActionResult Accepted(string message)
{
return Content(HttpStatusCode.Accepted, message);
}
}
Here is extension methods which checks if the API user has access to organisation (note that I have not included API user and organisation classes in this code review for simplicity)
ClaimsPrincipalExtensions.cs
public static class ClaimsPrincipalExtensions
{
public static bool HasReadWriteAccessToOrganisation(this ClaimsPrincipal principal, long organisationId)
{
return principal.Claims
.Where(c => c.Type == ApiUserClaimKeys.OrganisationGrantReadWrite)
.Where(c => c.Value == organisationId.ToString())
.Any();
}
public static bool HasReadAccessToOrganisation(this ClaimsPrincipal principal, long organisationId)
{
return principal.Claims
.Where(c => c.Type == ApiUserClaimKeys.OrganisationGrantReadWrite || c.Type == ApiUserClaimKeys.OrganisationGrantRead)
.Where(c => c.Value == organisationId.ToString())
.Any();
}
}
I have created a generic ApiProductUploader. Note that we have different product types such as Job, Car, House, etc
The uploader is responsible to serialize the DTO class and write it to S3 bucket.
ApiProductUploader.cs
public class ApiProductUploader<TApiProductDto>
where TApiProductDto : AdBaseApiDto
{
private const string _deleteAllMessage = "File is empty, all listings will be deleted";
private const string _queuedMessage = "Products are queued to be imported";
private const string _singleErrorHeader = "Import data has the following issue: ";
private const string _multipleErrorsHeader = "Import data has the following issues: ";
protected readonly IOrganisationRepository _organisationRepository;
protected readonly IB2bBucketFiler _b2bBucketFiler;
public ApiProductUploader(IOrganisationRepository organisationRepository, IB2bBucketFiler b2bBucketFiler)
{
_organisationRepository = organisationRepository;
_b2bBucketFiler = b2bBucketFiler;
}
public bool Upload(long organisationId, List<TApiProductDto> apiProductDtos, out string message)
{
message = string.Empty;
var organisation = LoadOrganisation(organisationId);
if (organisation == null)
{
throw new Exception($"OrganisationId='{organisationId}' does not exists, Upload method should not have been called");
}
message = BuildImportMessage(apiProductDtos);
return _b2bBucketFiler.UploadFile(organisation, Serialize(apiProductDtos));
}
private string Serialize(List<TApiProductDto> apiProductDtos)
{
return apiProductDtos.IsNullOrEmpty() ? string.Empty : JsonConvert.SerializeObject(apiProductDtos);
}
private string BuildImportMessage(List<TApiProductDto> apiProductDtos)
{
ImportResponse response = new ImportResponse();
if (apiProductDtos.IsNullOrEmpty())
{
response.Message = _deleteAllMessage;
}
else
{
response.Message = _queuedMessage;
foreach (var p in apiProductDtos)
{
response.AppendErrors(p.Alarms);
}
}
return JsonConvert.SerializeObject(response);
}
private Organisation LoadOrganisation(long organisationId)
{
if (organisationId > 0)
{
return _organisationRepository.FindOrganisationBy(o => o.OrganisationId == organisationId);
}
return null;
}
}
IOrganisationRepository.cs (I am not including the implementation for simplicity)
public interface IOrganisationRepository
{
Branch GetBranchById(long id);
Organisation FindOrganisationBy(Expression<Func<Organisation, bool>> predicate);
Organisation FindOrganisationByWebsite(string website);
List<Organisation> FindAllOrganisationBy(Expression<Func<Organisation, bool>> predicate);
List<Organisation> GetAllOrganisations();
long AddBranch(Branch branch);
long AddOrUpdateOrganisationAndBranches(Organisation organisation, bool hasAdminRole, out string duplicateError);
void UpdateBranch(Branch branch);
void UpdateOrganisationIgnoreBranches(Organisation organisation, bool hasAdminRole);
void DeleteBranch(Branch branch);
void DeleteOrganisation(long organisationId);
}
IB2bBucketFiler.cs this interface is responsible to read/write into S3 bucket (I am not including the implementation for simplicity)
public interface IB2bBucketFiler
{
double GetLastModifiedTimeOfOrganisationBucket(Organisation org);
XDocument ReadOrganisationXmlImportFile(Organisation org, string fileName = "");
S3Image LoadPhoto(string bucketName, string relativePath);
List<S3Image> LoadPhotos(string bucketName, string folderPath, string photosSearchPattern);
bool UploadFile(Organisation organisation, string fileStream);
void DeleteFile(Organisation org, string fileName);
void DeleteFiles(Organisation org, string searchPattern, List<string> photosToDelete);
void DeleteExpiredFiles(Organisation org, int daysToExipre);
}
DTOs
As mentioned before, we have different product types, such as Job, car, House, etc
This is the base class for all types:
AdBaseApiDto.cs
public abstract class AdBaseApiDto
{
protected List<string> _alarms;
public AdBaseApiDto()
{
_alarms = null;
}
public string IdValuePair => $"UniqueThirdPartyId='{UniqueThirdPartyId}'";
public string UniqueThirdPartyId { get; set; }
public short CategoryId { get; set; }
public string Title { get; set; }
public string Description { get; set; }
[JsonIgnore] // do not include in API documentation
public List<string> Alarms
{
get
{
if (_alarms == null)
{
_alarms = new List<string>();
Validate();
}
return _alarms;
}
}
[JsonIgnore]
protected abstract string ProductTypeName { get; } // do not include in API documentation
protected virtual void Validate()
{
if (string.IsNullOrEmpty(UniqueThirdPartyId))
{
_alarms.Add($"UniqueThirdPartyId is empty, this {ProductTypeName} cannot be imported");
}
if (string.IsNullOrEmpty(Title))
{
_alarms.Add($"{IdValuePair}: Title is empty, this {ProductTypeName} cannot be imported");
}
else if (Title.Length < GlobalConstants.MinLengthForAdTitle)
{
_alarms.Add($"{IdValuePair}: Title='{Title}' is too short, this {ProductTypeName} cannot be imported");
}
else if (Title.Length > GlobalConstants.MaxLengthForAdTitle)
{
_alarms.Add($"{IdValuePair}: Title='{Title}' is too long, this {ProductTypeName} cannot be imported");
}
if (string.IsNullOrEmpty(Description))
{
_alarms.Add($"{IdValuePair}: Description is empty, this {ProductTypeName} cannot be imported");
}
else if (Description.Length < GlobalConstants.MaxLengthForAdDescriptionInHtml)
{
_alarms.Add($"{IdValuePair}: Description is too long, this {ProductTypeName} cannot be imported");
}
}
}
This is the DTO for the Job type:
JobApiDto.cs
public class JobApiDto : AdBaseApiDto
{
public string Reference { get; set; }
public string Synopsis { get; set; }
public decimal MinSalary { get; set; }
public decimal MaxSalary { get; set; }
public string SalaryPer { get; set; }
public bool DisplaySalary { get; set; }
public string CustomSalaryText { get; set; }
public string WorkType { get; set; }
public string Location { get; set; }
public string ContactName { get; set; }
public string ContactEmail { get; set; }
public string ContactPhoneNumber { get; set; }
public string ApplicationUrl { get; set; }
public string LogoUrl { get; set; }
public string YouTubeVideo { get; set; }
public DateTime? StartDate { get; set; }
public DateTime? PostedAt { get; set; }
protected override string ProductTypeName => "Job";
protected override void Validate()
{
if (!string.IsNullOrEmpty(Reference) && Reference.Length > GlobalConstants.MaxLengthForCredentials)
{
_alarms.Add($"{IdValuePair}: Reference='{Reference}' is too long");
}
if (MinSalary <= 0)
{
_alarms.Add($"{IdValuePair}: MinSalary is required");
}
if (MaxSalary <= 0)
{
_alarms.Add($"{IdValuePair}: MaxSalary is required");
}
if (string.IsNullOrEmpty(SalaryPer))
{
_alarms.Add($"{IdValuePair}: SalaryPer is required");
}
if (string.IsNullOrEmpty(WorkType))
{
_alarms.Add($"{IdValuePair}: WorkType is required");
}
if (string.IsNullOrEmpty(Location))
{
_alarms.Add($"{IdValuePair}: Location is required");
}
if (!string.IsNullOrEmpty(ApplicationUrl))
{
var match = Regex.Match(ApplicationUrl, GlobalConstants.WebsiteValidationRegex);
if (!match.Success)
{
_alarms.Add($"{IdValuePair}: invalid ApplicationUrl: '{ApplicationUrl}'");
}
}
}
}
This class is used to serialize the response (and validation messages, if any) into json string:
ImportResponse.cs
public class ImportResponse
{
public ImportResponse()
{
Message = string.Empty;
ValidationErrors = new List<string>();
}
public string Message { get; set; }
public List<string> ValidationErrors { get; }
public void AppendErrors(List<string> errors)
{
ValidationErrors.AddRange(errors);
}
}
Error Handling
I have the following class, to catch the server errors ApiExceptionHandlingFilter.cs
public class ApiExceptionHandlingFilter : ExceptionFilterAttribute
{
public override void OnException(HttpActionExecutedContext context)
{
WebLog.Logger.Fatal(context.Exception, GetMessage(context), context.ActionContext.ControllerContext.Controller.GetType());
context.Response = new HttpResponseMessage(HttpStatusCode.InternalServerError)
{
Content = new StringContent("Internal Server Error"),
ReasonPhrase = "Internal Server Error"
};
}
private string GetMessage(HttpActionExecutedContext httpActionExecutedContext)
{
if (httpActionExecutedContext.Exception.InnerException == null)
{
return httpActionExecutedContext.Exception.Message;
}
return httpActionExecutedContext.Exception.InnerException.Message;
}
}
And this class for catching whatever the above class cannot catch: GlobalExceptionHandler.cs
// this class should catch exceptions which are not caught by ApiExceptionHandlingFilter
public class GlobalExceptionHandler : ExceptionHandler
{
public override void Handle(ExceptionHandlerContext context)
{
WebLog.Logger.Fatal(context.Exception, GetMessage(context), "GlobalExceptionHandler");
var response = new HttpResponseMessage(HttpStatusCode.InternalServerError)
{
Content = new StringContent("An unexpected error occured"),
ReasonPhrase = "An unexpected error occured"
};
context.Result = new ResponseMessageResult(response);
}
private string GetMessage(ExceptionHandlerContext context)
{
if (context.Exception.InnerException == null)
{
return context.Exception.Message;
}
return context.Exception.InnerException.Message;
}
}
This is the first time that I have build an API and I don't know if I have followed the correct pattern...
