views:

149

answers:

3

What is the best way to do entity-based validation (each entity class has an IsValid() method that validates its internal members) in ASP.NET MVC, with a "session-per-request" model, where the controller has zero (or limited) knowledge of the ISession? Here's the pattern I'm using:

  1. Get an entity by ID, using an IFooRepository that wraps the current NH session. This returns a connected entity instance.

  2. Load the entity with potentially invalid data, coming from the form post.

  3. Validate the entity by callings its IsValid() method.

  4. If valid, call IFooRepository.Save(entity), which delegates to ISession.Save(). Otherwise, display error message.

The session is currently opened when the request begins and flushed when the request ends. Since my entity is connected to a session, flushing the session attempts to save the changes even if the object is invalid.

What's the best way to keep validation logic in the entity class, limit controller knowledge of NH, and avoid saving invalid changes at the end of a request?


Option 1: Explicitly evict on validation failure, implicitly flush: if the validation fails, I could manually evict the invalid object in the action method. If successful, I do nothing and the session is automatically flushed.

Con: error prone and counter-intuitive ("I didn't call .Save(), why are my invalid changes being saved anyways?")

Option 2: Explicitly flush, do nothing by default: By default I can dispose of the session on request end, only flushing if the controller indicates success. I'd probably create a SaveChanges() method in my base controller that sets a flag indicating success, and then query this flag when closing the session at request end.

Pro: More intuitive to troubleshoot if dev forgets this step [relative to option 1]

Con: I have to call IRepository.Save(entity)' and SaveChanges().

Option 3: Always work with disconnected objects: I could modify my repositories to return disconnected/transient objects, and modify the Repo.Save() method to re-attach them.

Pro: Most intuitive, given that controllers don't know about NH.

Con: Does this defeat many of the benefits I'd get from NH?

A: 

How about a validation service with an IsValid (or something similar) method which validates the object passed to it, if it fails it could publish a ValidationFailed event. Then when your request finishes instead of calling the session's flush you could publish a RequestEnd event. You could then have a handler that listens for both RequestEnd events and ValidationFailed events - if there is a ValidationFailed event then don't flush the session but if not then flush it.
Having said that I just do Option 2!

Gareth
Thanks for the suggestion, but the entity-based validation is pretty much set in stone for this project, I'm trying to bring in NH with as minimal changes as possible :)
Seth Petry-Johnson
+1  A: 

Option 1 without a doubt. It's not counter intuitive, it's how NH works. Objects retrieved using NH are persistent and changes will be saved when the session is flushed. Calling Evict makes the object transient which is exactly the behavior you want.

You don't mention it but another option might be to use Manual or Commit FlushMode.

Jamie Ide
Thanks for the answer. Unfortunately, I'm trying to introduce NH onto a project that was started using a code-generated DAL to which other developers are accustomed. I want to keep the programming patterns as similar as possible, which means I am currently favoring "implicit rollback, explicit commit" solutions over "explicit rollback, implicit commit" ones. To someone unfamiliar with NH, Option 1 seems like the latter. I'm starting to agree, however, that the _best_ solution is one that mirrors NH's design, rather than hides it.
Seth Petry-Johnson
A: 

As Mauricio and Jamie have pointed out in their answers/comments, it's not easy (and probably not desirable) to do exactly what the question asks. NH returns persistent objects, so exposing those objects to the controllers means the controllers are responsible for treating them as such. I want to use lazy loading, so exposing detached instances won't work.

Option 4: Introduce a new pattern that provides the desired semantics

The point of this question is that I'm introducing NH+Repositories to an existing project using a hand-rolled, Active-Record-like DAL. I want code written NH to use patterns similar to the legacy code.

I created a new class called UnitOfWork that is a very thin wrapper over an ITransaction and that knows how to access the ambient session related to the current HttpRequest. This class is designed to be used in a using block, similar to TransactionScope which the team is familiar with:

using (var tx = new UnitOfWork()) {
    var entity = FooRepository.GetById(x);
    entity.Title = "Potentially Invalid Data";

    if (!entity.IsValid()) {
        tx.DiscardChanges();
        return View("ReloadTheCurrentView");
    }
    else {
        tx.Success();
        return RedirectToAction("Success");
    }
}

The tx.DiscardChanges() is optional, this class has the same semantics as TransactionScope which means it will implicitly rollback if it is disposed before the success flag is set.

On a greenfield NH project I think it's preferable to use Option 1, as Jamie indicates in his answer. But I think Option 4 is a decent way to introduce NH on a legacy project that already uses similar patterns.

Seth Petry-Johnson