Friday, April 30, 2010

Agatha and a Unified Request Handler Theory

At my last job we were using the Agatha library to simplify requests and responses through our WCF service. All in all it was pretty cool. But there was one thing I didn't like. Every request/response pair needed its own handler class.

Now, normally this makes sense. However, we were dealing with a growing enterprise domain. We're talking about potentially hundreds upon hundreds of handlers. From a code organizational perspective, it made more sense to group our domain methods into categories. Thus were created processor classes. Each subset of the overall domain logic (product ordering, reporting, security, etc.) would have a processor class (or namespace, with further divided classes) with all the methods in there.

This left us with a growing namespace of handler classes, each of which was nearly identical. We had our base handler which inherited from Agatha's handler and did our common stuff (error trapping, unit of work encapsulation, etc.) that every service call needs. It included an abstract method that its Handle method would call. Then, for every request/response pair in the domain, a handler was written which implemented this abstract method and passed control along to the proper processor method.

This means that we were going to have hundreds upon hundreds (maybe more) of identical handlers, all doing precisely the same thing. That's a lot of repetition in the code, and it's an easy point for a new junior dev to get hung up on when he's not used to the system. He shouldn't be expected to "just know" that he has to implement this intuitively useless class. All he should have to do is create his request/response types and his domain logic method. The base handler should take care of directing traffic.

It was a dream born of a desire to simplify and to make the programming as plug-and-play as possible for new junior devs on the team. And this dream has now produced a prototype. Keep in mind that this is not finished. Also, it's SLOW. But it works. Hopefully I can make enhancements to it (feel free to suggest any) or even just re-implement the idea in an entirely different and better way. This is just a proof of concept:

public class MyBaseRequestHandler : IRequestHandler
  where TREQUEST : MyBaseRequest
{
  public Response Handle(TREQUEST request)
  {
    try
    {
      // TODO: Error-check the request

      // Get the assembly for the domain code
      // At my last job they had an empty "marker" class.  Made sense.
      var assembly = Assembly.GetAssembly(typeof(DomainAssemblyMarker));

      // Create a response based on the request type
      // I use a custom attribute called "ResponseTypeAttribute" to
      // store the response type for any given request type at the
      // class level.
      var response = Activator.CreateInstance(
        ((ResponseTypeAttribute)
          request.GetType().GetCustomAttributes(
                              typeof(ResponseTypeAttribute),
                              false)[0]
        ).TypeName
      );

      try
      {
        // Do the process for this request
        // The convention is that there should be exactly ONE method
        // in the processors which takes ONE argument (the request)
        // and returns the response.
        // TODO: Implement a true "unit of work" here
        foreach (var type in assembly.GetTypes())
          if (type.IsClass)
            foreach (var method in type.GetMethods())
              if (method.GetParameters().Length == 1)
                if (method.GetParameters()[0].ParameterType == typeof(TREQUEST))
                {
                  // Get an instance from the IoC and invoke the method
                  var obj = ObjectFactory.GetInstance(type);
                  return (Response)method.Invoke(obj, new object[] { request });
                }

        // Couldn't find the right method, return a failed response
        // The .Failed extension (and .Success) was something they
        // implemented at my old job.  Kind of got me hooked on
        // extensions.  So expect more in the future.
        return ((MyBaseResponse)response).Failed(
          new Message
          {
            DisplayMessage = "Service Error",
            ErrorMessage = "Unable to find processor method for "
                     + request.GetType().ToString(),
            MessageType = MessageType.Error
          }
        );
      }
      catch (Exception ex)
      {
        // Caught an exception trying to find and/or invoke the handler method
        return ((MyBaseResponse)response).Failed(ex);
      }
    }
    catch (Exception ex)
    {
      // Caught an exception trying to create a properly typed reponse based on
      // the request type.  Just return a base response with the failed message,
      // even though the client probably won't like the type.
      return new MyBaseResponse().Failed(ex);
    }
  }

  public Response Handle(Request request)
  {
    return Handle((TREQUEST)request);
  }

  public void Dispose()
  {
    // TODO: Handle disposal
  }

  public Response CreateDefaultResponse()
  {
    return new MyBaseResponse();
  }
}

Ya, you can see the slow part. It's kind of obvious. So the main improvement to be made here is in finding the method to invoke. Hey, at least it's properly asking for the object from the IoC container (StructureMap), right? I'm thinking of doing more with custom attributes (an area that's somewhat new to me) to assist in that method discovery process, similar to what I did with the request/response discovery (which I'd also like to improve a bit if possible).

Also, before I forget, I had to tell the StructureMap configuration that all IRequestHandlers should use MyBaseRequestHandler:

For(typeof(IRequestHandler<>)).Use(typeof(MyBaseRequestHandler<>));

You'll also notice that the last catch block in the Handle method returns a not-properly-typed response. I can confirm that the client does not like this. When I try to Get<>() a specific response from the dispatcher it blows up saying the sequence contains no elements. Ya, that response isn't in there. MyBaseResponse is. So hopefully I can find a way to clean this up as well.

I'd love some input on this, since as I said it's just an early prototype.

6 comments:

  1. I need to read over the code and post again I think to fully understand but if you don't rely of StructureMap for any of it's features you could have your code just rely on CommonServiceLocator so anyone could use it with any IOC they wanted.

    ReplyDelete
  2. Ya, I definitely need to do a better job of abstracting out my IoC stuff. I'm still learning the ins and outs of that.

    ReplyDelete
  3. So it sounds like you are trying to avoid having a bunch of request handlers that are pass throughs. Perhaps refactoring what the handlers do is something to do. There is a refactoring pattern about removing a middle man that might be worth considering:

    http://www.refactoring.com/catalog/removeMiddleMan.html

    Also I kind of forget what the request handlers did for us at our old job. I believe they were to separate out the external service interface from the internal one. So they did server a purpose.

    Alternatively perhaps you remove the idea of the processor from the architecture. Just have the request handler perform the logic and have the dependencies needed for the action.

    Back to your code. Yeah some kind of data structure or use of an IoC feature that I'm not aware of needs to replace that double for loop that uses reflection. As 'The Cathedral and the Bazaar' says "Smart data structures and dumb code works a lot better than the other way around".

    ReplyDelete
  4. I think I may have found a fairly decent compromise, but we'll have to see how it works in practice. The goal was to reduce the number of disparate pieces of logic, right? Dozens of processor classes instead of hundreds (even thousands) of handler classes, so things could be logically separated into more intuitive business groupings.

    My idea fits with your quote about data structures... Basically re-factor the concept of the request and response. Instead of having many small requests and responses, have fewer large ones.

    So instead of a "GetUserProfileRequest" and a "GetUserRolesRequest" and so on and so forth, there would be a "SecurityRequest" which can cover many bases. The request would be populated with whatever the user has in order to get the data he needs. Then a handler custom to that request picks it up (like at our old job, not this Reflection loop I have) and the handler's only job is to see what the request has so it knows where to send it.

    The handler would send it to multiple domain methods based on what it looks like the requester wants. So each "processor" has a handler which directs traffic for that section of domain logic to the various methods in that processor.

    I'll have to put together a diagram for this and post it here sometime.

    ReplyDelete
  5. Looks like it's working, and I really like the design. Ya, I definitely need to draw a diagram with some class examples.

    The biggest challenge in implementing it at my current job is the sheer volume of legacy software. We're having some increasingly regular meetings to discuss the overall architecture and my input has been pretty well received.

    The direction we're going right now is to try to create a rough roadmap of where we are and where we want to be and try to come up with some discrete steps to get us there. If we can implement proper agile here (which my boss is all for, at least in talk) then my goal is to improve throughput enough to justify a day or two per iteration of infrastructure work. Kind of like how Google allows a day per week of research work.

    ReplyDelete
  6. Hmm... I might need a diagram or some example code of how you are meaning to break it all out. I'm not sure what you are gaining nor if I understand what you mean.

    So the handler would inspect the request and decide where it was suppose to go? So the the same request type could be passed to different processor actions depending on input?

    ReplyDelete