Separation of validator and service with external API calls

Question

I'm currently building a web application and attempting to design it following good MVC and service-oriented architecture.

I have, however, hit a bit of a wall in connecting the presentation layer (i.e. my controllers) and the back-end services while still maintaining good error/validation reporting back to the user.

I read a really good SO post here about how to separate Validation logic from the service layer and for the most part it all made sense. However there was one "flaw", if you can call it that, in this model that niggled at me: How do you avoid duplicating effort when looking up objects that are required by both the validator and the service?

I think it'd be easier to explain with a reasonably simple example:

Let's say I have an application that allows users to share code snippets around. Now, I've decided to add a new feature which allows a user to attach their GitHub account to their account on my site (i.e. to build up a profile). For the purpose of this example I'm going to simply assume that all my users are trustworthy and would only attempt to add their own GitHub accounts, not anyone else's :)

Following the aforementioned SO article I've set up a basic GitHub service for retrieving GitHub user info.

interface IGitHubUserService {
    GitHubUser FindByUserName(string username);
}

The concrete implementation of GitHubUserService makes an expensive call to https://api.github.com/users/{0} in order to pull user information. Again, following the article's model I implemented the following command to link a user account to a GitHub user:

// Command for linking a GitHub account to an internal user account
public class GitHubLinkCommand {
    public int UserId { get; set; }
    public string GitHubUsername { get; set }
};

My validator needs to validate that the username entered by the user is a valid GitHub account. This is very straightforward: call FindByUserName on the GitHubUserService and make sure that the result isn't null:

public sealed class GitHubLinkCommandValidator : Validator<GitHubLinkCommand> {
    private readonly IGitHubUserService _userService;

    public GitHubLinkCommandValidator(IGitHubUserService userService) {
        this._userService = userService;
    }

    protected override IEnumerable<ValidationResult> Validate(GitHubLinkCommand command) {
        try {
            var user = this._userService.FindByUserName(command.GitHubUsername);
            if (user == null)
                yield return new ValidationResult("Username", string.Format("No user with the name '{0}' found on GitHub's servers."));
        }
        catch(Exception e) {
            yield return new ValidationResult("Username", "There was an error contacting GitHub's API.");
        }
    }
}

Okay that's great! The validator is really straightforward and makes sense. Now it's time to make the GitHubLinkCommandHandler:

public class GitHubLinkCommandHandler : ICommandHandler<GitHubLinkCommand>
{
    private readonly IGitHubUserService _userService;

    public GitHubLinkCommandHandler(IGitHubUserService userService)
    {
        this._userService = userService;
    }

    public void Handle(GitHubLinkCommand command)
    {
        // Get the user details from GitHub:
        var user = this._userService.FindByUserName(command.GitHubUsername);

        // implementation of this entity isn't really relevant, just assume it's a persistent entity to be stored in a backing database
        var entity = new GitHubUserEntity
        {
            Name = user.Login,
            AvatarUrl = user.AvatarUrl
            // etc.
        };

        // store the entity:
        this._someRepository.Save(entity);
    }
}

Again, this looks really neat and straightforward. However there's one glaring issue: The duplicate calls to IGitHubUserService::FindByUserName, one from the validator and one from the service. On a bad day such a call can take 1-2 seconds without server-side caching, making duplication far too expensive to use this architectural model.

Has anyone else encountered such an issue when writing validators/services around external APIs and how did you reduce the duplication of effort outside of implementing a cache in your concrete class?

Answer 1

From my point of view, the problem is that neither the LinkCommandHandler nor the LinkCommandValidator should be retrieving the GitHub user in the first place. If you think about in terms of the Single Responsibility Principle, the Validator has a single job, to validate the existence of a user, and the LinkCommandHanlder has a single job to load the entity into the repository. Neither of them should have the job of pulling the Entity/User from GitHub.

I like to structure my code in the following pattern, each representing an attributable layer. Each layer can speak with the layer above and the layer below but it cant skip over a layer.

1. Data Layer -- this represents a datasource such as a database or a service usually you don't write code for this, you just consume it.
2. Access Layer -- this represents the code to interact with the datalayer
3. Peristence Layer -- this represents the code to get items ready for the call to the Access Layer such as data translations, building entities from the data, or grouping multiple calls to the access layer into a single request to retrieve data or store data. Also, the decision to cache, and the mechanisms for caching and clearing the cache would reside in this layer.
4. Processor Layer - this represents the code that performs the business logic. It is also where you would make use of validators, other processors, parsers, etc.

And then I keep all the above separate from my presentation layer. The concept being that the core code and functionality shouldn't know if it is being used from a website or a desktop app, or a WCF Service.

So in your example, I would have a GitHubLinkProcessor object a method called LinkUser(string username). Within that class, I would instantiate my GitHubPeristenceLayer class and call its FindUserByName(string username) method. Next, we proceed to instantiate a GitHubUserValidator class to validate the user is not null and all the necessary data is present. One validation is passed, a LinkRepositoryPersistence object is instantiated and passed the GitHubUser for persistence into the AccessLayer.

But I want to strongly note that this is just the way I would do it, and by no means do I want to imply that other methodologies are any less valid.

EDIT:

I was going for a simple answer because I was afraid my response was already too long and boring. =) I am going to split hairs here for a moment, so please bear with me. To me, you are not validating the user by calling Git. You are checking for the existence of a remote resource, which may or may not fail. An analogy may be that you can validate that (800) 555-1212 is a valid format for a US Phone number, but not that the phone number existing and belongs to the correct person. That is a separate process. Like I said, it is splitting hairs, but by doing so it allows for the overall code pattern I describe.

So let's assume your local user object has a property for UserName and Email that cannot be null. You would validate for those and only move on to checking for the resource if that validation was correct.

public class User 
{
    public string UserName { get; set; }
    public string Email { get; set; }

    //git related properties
    public string Login { get; set; }
    public string AvataUrl { get; set; }
}

//A processor class to model the process of linking a local system user
//to a remote GitHub User
public class GitHubLinkProcessor()
{
    public int LinkUser(string userName, string email, string gitLogin) 
    {
            //first create our local user instance
            var myUser = new LocalNamespace.User { UserName = userName, Email = email };

        var validator = new UserValidator(myUser);
        if (!validator.Validate())
            throw new Exception("Invalid or missing user data!");

        var GitPersistence = new GitHubPersistence();

        var myGitUser = GitPersistence.FindByUserName(gitLogin);
        if (myGitUser == null)
            throw new Exception("User doesnt exist in Git!");

        myUser.Login = myGitUser.Login;
        myUser.AvatorUrl = myGitUser.AvatarUrl;

        //assuming your persistence layer is returning the Identity
        //for this user added to the database
        var userPersistence = new UserPersistence();
        return userPersistence.SaveLocalUser(myUser);

        }
}

public class UserValidator
{
    private LocalNamespace.User _user;

    public UserValidator(User user)
    {
        this._user = user;
    }

    public bool Validate()
    {
        if (String.IsNullOrEmpty(this._user.UserName) ||
            String.IsNullOrEmpty(this._user.Email))
        {
            return false;
        }
    }
}