Ayende @ Rahien

Refunds available at head office

Reviewing RavenDB app: ReleaseCandidateTracker

ReleaseCandidateTracker is a new RavenDB based application by Szymon Pobiega. I reviewed version 5f7e42e0fb1dea70e53bace63f3e18d95d2a62dd. At this point, I don’t know anything about this application, including what exactly it means, Release Tracking.

I downloaded the code and started VS, there is one project in the solution, which is already a Good Thing. I decided to randomize my review approach and go and check the Models directory first.

Here is how it looks:

image

This is interesting for several reasons. First, it looks like it is meant to keep a record of all deployments to multiple environments, and that you can lookup the history of each deployment both on the environment side and on the release candidate side.

Note that we use rich models, which have collections in them. In fact, take a look at this method:

image

Which calls to this method:

image

You know what the really fun part about this?

It ain’t relational model. There is no cost of actually making all of these calls!

Next, we move to the Infrastructure folder, where we have a couple of action results and the RavenDB management stuff. Here it how RCT uses RavenDB:

public static class Database
{
    private static IDocumentStore storeInstance;

    public static IDocumentStore Instance
    {
        get
        {
            if (storeInstance == null)
            {
                throw new InvalidOperationException("Document store has not been initialized.");
            }
            return storeInstance;
        }
    }

    public static void Initialize()
    {
        var embeddableDocumentStore = new EmbeddableDocumentStore {DataDirectory = @"~\App_Data\Database"};
        embeddableDocumentStore.Initialize();
        storeInstance = embeddableDocumentStore;
    }
}

It is using an embedded database to do that, which makes it very easy to use the app. Just hit F5 and go. In fact, if we do, we see the fully functional website, which is quite awesome Smile.

Let us move to seeing how we are managing the sessions:

public class BaseController : Controller
{
    public IDocumentSession DocumentSession { get; private set; }
    public CandidateService CandidateService { get; private set; }
    public ScriptService ScriptService { get; private set; }

    protected override void OnActionExecuting(ActionExecutingContext filterContext)
    {
        if (filterContext.IsChildAction)
        {
            return;
        }
        DocumentSession = Database.Instance.OpenSession();
        CandidateService = new CandidateService(DocumentSession);
        ScriptService = new ScriptService(DocumentSession);
        base.OnActionExecuting(filterContext);
    }
    
    protected override void OnActionExecuted(ActionExecutedContext filterContext)
    {
        if (filterContext.IsChildAction)
        {
            return;
        }
        if(DocumentSession != null)
        {
            if (filterContext.Exception == null)
            {
                DocumentSession.SaveChanges();
            }
            DocumentSession.Dispose();
        }
        base.OnActionExecuted(filterContext);
    }
}

This is all handled inside the base controller, and it is very similar to how I am doing that in my own apps.

However, ScriptService and CandidateService seems strange, let us explore them a bit.

public class ScriptService
{
    private readonly IDocumentSession documentSession;

    public ScriptService(IDocumentSession documentSession)
    {
        this.documentSession = documentSession;
    }

    public void AttachScript(string versionNumber, Stream fileContents)
    {
        var metadata = new RavenJObject();
        documentSession.Advanced.DatabaseCommands.PutAttachment(versionNumber, null, fileContents, metadata);
    }

    public Stream GetScript(string versionNumber)
    {
        var attachment = documentSession.Advanced.DatabaseCommands.GetAttachment(versionNumber);
        return attachment != null 
            ? attachment.Data() 
            : null;
    }
}

So this is using RavenDB attachment to store stuff, I am not quite sure what yet, so let us track it down.

This is being used like this:

[HttpGet]
public ActionResult GetScript(string versionNumber)
{
    var candidate = CandidateService.FindOneByVersionNumber(versionNumber);
    var attachment = ScriptService.GetScript(versionNumber);
    if (attachment != null)
    {
        var result = new FileStreamResult(attachment, "text/plain");
        var version = candidate.VersionNumber;
        var product = candidate.ProductName;
        result.FileDownloadName = string.Format("deploy-{0}-{1}.ps1", product, version);
        return result;
    }
    return new HttpNotFoundResult("Deployment script missing.");
}

So I am assuming that the scripts are deployment scripts for different versions, and that they get uploaded on every new release candidate.

But look at the CandidateService, it looks like a traditional service wrapping RavenDB, and I have spoken against it multiple times.

In particular, I dislike this bit of code:

public ReleaseCandidate FindOneByVersionNumber(string versionNumber)
{
    var result = documentSession.Query<ReleaseCandidate>()
        .Where(x => x.VersionNumber == versionNumber)
        .FirstOrDefault();
    if(result == null)
    {
        throw new ReleaseCandidateNotFoundException(versionNumber);
    }
    return result;
}

public void Store(ReleaseCandidate candidate)
{
    var existing = documentSession.Query<ReleaseCandidate>()
        .Where(x => x.VersionNumber == candidate.VersionNumber)
        .Any();
    if (existing)
    {
        throw new ReleaseCandidateAlreadyExistsException(candidate.VersionNumber);
    }
    documentSession.Store(candidate);
}

From looking at the code, it looks like the version number of the release candidate is the primary way to look it up. More than that, in the entire codebase, there is never a case where we load a document by id.

When I see a VersionNumber, I think about things like “1.0.812.0”, but I think that in this case the version number is likely to include the product name as well, “RavenDB-1.0.812.0”, otherwise you couldn’t have two products with the same version.

That said, the code above it wrong, because it doesn’t take into account RavenDB’s indexes BASE nature. Instead, the version number should actually be the ReleaseCandidate id. This way, because RavenDB’s document store is fully ACID, we don’t have to worry about index update times, and we can load things very efficiently.

Pretty much all of the rest of the code in the CandidateService is only used in a single location, and I don’t really see a value in it being there.

For example, let us look at this one:

[HttpPost]
public ActionResult MarkAsDeployed(string versionNumber, string environment, bool success)
{
    CandidateService.MarkAsDeployed(versionNumber, environment, success);
    return new EmptyResult();
}

image

As you can see, it is merely loading the appropriate release candidate, and calling the MarkAsDeployed method on it.

Instead of doing this needless, forwarding, and assuming that we have the VersionNumber as the id, I would write:

[HttpPost]
public ActionResult MarkAsDeployed(string versionNumber, string environment, bool success)
{
    var cadnidate = DocumentSession.Load<ReleaseCandidate>(versionNumber);
    if (cadnidate == null)
        throw new ReleaseCandidateNotFoundException(versionNumber);
    var env = DocumentSession.Load<DeploymentEnvironment>(environment);
    if (env == null)
        throw new InvalidOperationException(string.Format("Environment {0} not found", environment));

    cadnidate.MarkAsDeployed(success, env);
    return new EmptyResult();
}

Finally, a word about the error handling, this is handled via:

protected override void OnException(ExceptionContext filterContext)
{
    filterContext.Result = new ErrorResult(filterContext.Exception.Message);
    filterContext.ExceptionHandled = true;
}

public class ErrorResult : ActionResult
{
    private readonly string message;

    public ErrorResult(string message)
    {
        this.message = message;
    }

    public override void ExecuteResult(ControllerContext context)
    {
        context.HttpContext.Response.Write(message);
        context.HttpContext.Response.StatusCode = 500;
    }
}

The crazy part is that OnException is overridden only on some of the controllers, rather than in the base controller, and even worse. This sort of code leads to error details loss.

For example, let us say that I get a NullReferenceException. This code will dutifully tell me all about it, but will not tell me where it happened.

This sort of thing make debugging extremely hard.

Comments

Andreas Kroll
05/16/2012 12:36 PM by
Andreas Kroll

Hi Ayende,

I find it extremely helpful that you take the time to look through code written by other developers. I can understand that not all developers like that, but after all it's best to make errors only once, and learn from them. So the "service" you provide free of cost is really helpful for a lot of people. Thanks for that.

I was wondering what type of tool you use to generate the sequence diagrams. I am personally using Enterprise Architect, but the output is not as nice as yours and I am struggeling a lot with the process of creating such a sequence diagram.

Keep up the good work!

Best regards, Andreas KRoll

Ayende Rahien
05/16/2012 12:41 PM by
Ayende Rahien

Andreas. I use Generate Sequence Diagram in VS

Daniel Lang
05/16/2012 01:02 PM by
Daniel Lang

Nice to have a review about a RavenDB application. I'd like to see more of them! :-)

I don't agree with your point about having the VersionNumber as the document key. You don't want to have it locked forever into being unique. This requirement could easily change and if you used the VersionNumber as the document key you run straight into the migration hell.

I'd rather use the UniqueContraints bundle (which I can easily remove it later) and put a WaitForNonStaleResults on the query, because this kind of application is very unlikely to have a high number of write operations, so performance shouldn't be a problem.

Matt Warren
05/16/2012 01:28 PM by
Matt Warren

@Daniel You could also use the method outlined http://ravendb.net/docs/faq/unique-constraints (unless I'm mis-understanding your comment?)

Stefan
05/16/2012 01:47 PM by
Stefan

Hi! One question regarding your approach of opening the sesssion. What happens if I unit/integration-test the controller? How do I pass a session to the controller in my tests? From what I see, I'll need to open up a bit the visibility of the DocumentSession setter of the base controller to be able to inject it through this property, right?

Ayende Rahien
05/16/2012 01:48 PM by
Ayende Rahien

Stefan, I do something like:

new MyController { Session = theSession };

Daniel Lang
05/16/2012 01:54 PM by
Daniel Lang

@Matt Yes, this is what the UniqueConstraints bundle does automatically. Sorry once again for my bad English... ;)

Matt Warren
05/16/2012 02:31 PM by
Matt Warren

@Daniel, oooppppss I should've know that, I wrote some of the code for it (see http://ravendb.net/docs/server/bundles/unique-constraints near the bottom)

Andreas Kroll
05/16/2012 02:48 PM by
Andreas Kroll

Thanks Ayende,

sometimes the solution to a problem is closer than you begin to search for.

Brian Coleman
05/16/2012 11:53 PM by
Brian Coleman

You mention that the BaseController class is very similar to the way you do it in your own apps. I was wondering if you have a simple example application (preferably ASP.NET MVC3) that has a typical infrastructure that you use in your applications (global.asax, BaseController, etc) when you are using RavenDB for persistence.

Thanks in advance!

Ayende Rahien
05/17/2012 12:00 AM by
Ayende Rahien

Brian, This blog is open source, you can look at its code. https://github.com/ayende/raccoonblog

Sean Kearon
05/17/2012 09:55 AM by
Sean Kearon

Agree with Daniel - more Raven app reviews would be very useful. And, the Release Candidate Tracker looks like it might be pretty useful too!

Ubaldo
05/17/2012 10:37 AM by
Ubaldo

Ayende

as Effectus you could write a sample desktop application on RavenDB?

Arnis Lapsa
05/18/2012 09:22 AM by
Arnis Lapsa

I would say that Szymon has been quite successful at "Ayende`s test" :)

joff
05/22/2012 11:33 AM by
joff

And yet, that may be due to the fact that he is using Ayende's product

Ayende Rahien
05/22/2012 11:38 AM by
Ayende Rahien

Joff, Actually, that is because RavenDB will actually prevent a lot of the things that I comment about as bad practices in this blog

Comments have been closed on this topic.