You are correct in your issue. The article 'decouples' ModelState from the IProductService by Listing 5, but does not decouple it from the Create Action in Listing 8.
So you have a few options:
expose the errors on the IValidationDictionary and then in the Create Action add said errors to the Controller's ModelState before returning the View
expose the IValidationDictionary on the IProductService, when creating a new ProductController using the ProductController's IProductService constructor, set the _service's IValidationDictionary to the Controller's ModelState (using wrapper of course) after
setting the local _service variable
modify/improve the design
Something about this design is bothering me and I can't quite put my finger on it. It might be that the IProductService interface is not intention revealing. Meaning any derived class of the IProductService may or may not implement validation of any kind
and the ones that do, how do you know that validation is taking place when inspecting its interface...the ProductService being case in point? It also might be the concept of the ModelStateWrapper, something about that makes me feel like it isn't decoupling
at all, but instead just a hack/shortcut to satisfy what appears to be decoupling. I mean, just because you use interfaces doesn't mean you're decoupling, clearly because it's what started this thread to begin with.
The tutorial shows some decoupling and dependency injection, but is inconsistent. There is no decoupling or dependency injection with the ProductRepository, what’s up with that? So they wanted to show the use of a ‘Service’ layer, but instead made a more
complicated and inconsistent design? Not to mention, that while the article shows the use of try/catch blocks (good thing) the exceptions are eaten (bad thing), especially during the more important operations where knowing what to do next when an exception
occurs is critical rather than just getting a false return value. There goes the use of the HandleErrorAttribute or Error logging, you know?
The other thing is that the concept of having this IProductService that uses an IProductRepository that uses a ProductDBEntities where the ProductService doesn't do anything differently than the ProductRepository other than validation seems completely unnecessary.
The IProductService and IProductRepository are the SAME interface, doesn’t seem right does it? In addition and like everything, there are pros and cons to using constructor based dependency injection rather than property based. More complicated Controllers
will have multiple, more complex dependencies and relying on the constructor of the Controller to set those may become unfavorable. The use of a ControllerFactory and property based injection at that point is ideal, in my opinion.
I guess I would rather keep it a little simpler, with more intention, easier testability, and less structure (like removing IValidationDictionary, ModelStateWrapper, IProductRepository, ProductRepository all together). Like this:
public interface IProductProvider {
ObjectQuery<Product> ProductSet { get; set; }
void AddToProductSet(Product product);
int SaveChanges();
}
public interface IProductService {
IEnumerable<Product> GetProducts();
IDictionary<string, string> ValidateProduct(Product product);
void CreateProduct(Product product);
}
public class ProductService : IProductService {
public IProductProvider Provider { get; set; }
public ProductService() {
this.Provider = new ProductDBEntities();
}
public IEnumerable<Product> GetProducts() {
return this.Provider.ProductSet.ToList();
}
public IDictionary<string, string> ValidateProduct(Product product) {
Guard.AgainstNullParameter(product, "product");
Dictionary<string, string> errors = new Dictionary<string, string>();
if(!product.Name.HasValue()) errors.Add("Name", "Name is required.");
if(!product.Description.HasValue()) errors.Add("Description", "Description is required.");
if(product.UnitsInStock < 0) errors.Add("UnitsInStock", "Units in stock cannot be less than zero.");
return errors;
}
public void CreateProduct(Product product) {
Guard.AgainstNullParameter(product, "product");
this.Provider.AddToProductSet(product);
this.Provider.SaveChanges();
}
}
[HandleError]
public class ProductController : Controller {
public IProductService ProductService { get; set; }
public ProductController() {
this.ProductService = new ProductService();
}
public ActionResult Index() {
return View(this.ProductService.GetProducts());
}
public ActionResult Create() {
return View();
}
[AcceptVerbs(HttpVerbs.Post)]
public ActionResult Create([Bind(Exclude = "Id")] Product product) {
IDictionary<string, string> errors = this.ProductService.ValidateProduct(product);
if(errors.Count > 0) {
this.ModelState.Merge(errors);
return View();
}
else {
this.ProductService.CreateProduct(product);
return RedirectToAction("Index");
}
}
}
public static class ModelExtensions {
public static bool HasValue(this string value) {
return !string.IsNullOrEmpty(value) && value.Trim().Length > 0;
}
public static void Merge(this ModelStateDictionary modelState, IDictionary<string, string> dictionary) {
Guard.AgainstNullParameter(modelState, "modelState");
Guard.AgainstNullParameter(dictionary, "dictionary");
foreach(var item in dictionary) {
modelState.AddModelError(item.Key, item.Value);
}
}
}
public static class Guard {
public static void AgainstNullParameter(object parameter, string parameterName) {
if(parameter == null) throw new ArgumentNullException(parameterName);
}
}
I personally think the part that is missing in this solution is where the validation is being put in the first place. I of course ran into this same problem and immediately kicked myself for missing the flaw. If we expand our logic and validation as far
as logically possible we end up with a stream from the client to the database as such.
Client Input Validation (in this case javascript)
Server-Side Input Validation (the case were are discussing)
Application of Business Rules (item must be in stock, duplicate product name, etc)
Data Input Validation (make sure when putting the entities together that there aren't nulls in fields that should have been handled by the business process but are not data entry fields such as status's etc.)
If you are actually following this structure you really should be using two objects (an Entity object for database actions and a DTO or Data Transfer Object for communicating with the client), but for the sake of discussion we'll say that our model is very
simple and there are no other entities that get involved like our simple ContactManager. In general good object oriented design says that unless we are coupling our objects we should not be moving our logic outside of the closest object in the relation.
For instance, if we have a Dog and we want to make it bark() then we should put the logic for making its mouthmove() and lungsexhale() inside the Dog object. If we then have a bird that chirps we make its beakmove() and its chestcompress(). Now if we want
both of these animals to makenoise() we add an interface and implement makenoise().
No one knows better in this case than the object itself, how it should makenoise. The same can be said for a Product or any other object. I believe this to be why MVC has allowed for the IDataErrorInfo interface to be used to validate an object to be validated
in a decoupled way. The service layer really should be applying the business rules after the validation. I think it makes more sense to use the built in bindings which will call this interface to validate the properties. If implemented correctly you wont
even feel the performance hit. This is an entirely rough implementation that may or may not compile but I think you'll get my point
public class Product : IDataErrorInfo {
private bool _needsValidation = false;
private Dictionary<string, string> errors = new Dictionary<string, string>();
private int _id;
public int Id {
get { return _id; }
set {
if (_id != value) {
_needsValidation = true;
}
_id = value;
}
}
private string _description;
public string Description {
get { return _description; }
set {
if (_description != value)
_needsValidation = true;
_description = value;
}
}
private int _unitsInStock;
public int UnitsInStock {
get { return _unitsInStock; }
set {
if (_unitsInStock != value)
_needsValidation = true;
_unitsInStock = value;
}
}
#region IDataErrorInfo Members
public string Error {
get { return string.Empty; }
}
public string this[string columnName] {
get {
if (_needsValidation) {
Validate();
_needsValidation = false;
}
if (errors.ContainsKey(columnName))
return errors[columnName];
else
return null;
}
}
protected void Validate() {
if(string.IsNullOrEmpty(this.Description) || this.Description.Trim().Length == 0)
errors.Add("Description", "Description is required.");
if(this.UnitsInStock < 0)
errors.Add("UnitsInStock", "Units in stock cannot be less than zero.");
}
#endregion
}
I think it would also be valid to add an interface to add errors to particular properties on the object through an interface and since the Validate() would not need calling again then you wouldn't lose those errors.
RiverX
Member
2 Points
26 Posts
Dependency Injection Using IValidationDictionary
Oct 28, 2009 02:55 AM|LINK
Hi all,
I'm currently using the method provided here http://www.asp.net/Learn/mvc/tutorial-38-cs.aspx
to do my validation. I was wondering how one would go about using dependency injection with that design.
It looks like the ProductController Needs a IProductService object and the IProductService Object needs the ProductControllers ModelState.
When using structuremap i get a stackoverflow error because of the circular dependency. I'm assuming it's due to the circular dependency.
Thanks for any help
asp.net MVC Validation
nicequy
Contributor
2652 Points
496 Posts
Re: Dependency Injection Using IValidationDictionary
Oct 31, 2009 12:09 PM|LINK
You are correct in your issue. The article 'decouples' ModelState from the IProductService by Listing 5, but does not decouple it from the Create Action in Listing 8.
So you have a few options:
Something about this design is bothering me and I can't quite put my finger on it. It might be that the IProductService interface is not intention revealing. Meaning any derived class of the IProductService may or may not implement validation of any kind and the ones that do, how do you know that validation is taking place when inspecting its interface...the ProductService being case in point? It also might be the concept of the ModelStateWrapper, something about that makes me feel like it isn't decoupling at all, but instead just a hack/shortcut to satisfy what appears to be decoupling. I mean, just because you use interfaces doesn't mean you're decoupling, clearly because it's what started this thread to begin with.
The tutorial shows some decoupling and dependency injection, but is inconsistent. There is no decoupling or dependency injection with the ProductRepository, what’s up with that? So they wanted to show the use of a ‘Service’ layer, but instead made a more complicated and inconsistent design? Not to mention, that while the article shows the use of try/catch blocks (good thing) the exceptions are eaten (bad thing), especially during the more important operations where knowing what to do next when an exception occurs is critical rather than just getting a false return value. There goes the use of the HandleErrorAttribute or Error logging, you know?
The other thing is that the concept of having this IProductService that uses an IProductRepository that uses a ProductDBEntities where the ProductService doesn't do anything differently than the ProductRepository other than validation seems completely unnecessary. The IProductService and IProductRepository are the SAME interface, doesn’t seem right does it? In addition and like everything, there are pros and cons to using constructor based dependency injection rather than property based. More complicated Controllers will have multiple, more complex dependencies and relying on the constructor of the Controller to set those may become unfavorable. The use of a ControllerFactory and property based injection at that point is ideal, in my opinion.
I guess I would rather keep it a little simpler, with more intention, easier testability, and less structure (like removing IValidationDictionary, ModelStateWrapper, IProductRepository, ProductRepository all together). Like this:
public interface IProductProvider { ObjectQuery<Product> ProductSet { get; set; } void AddToProductSet(Product product); int SaveChanges(); } public interface IProductService { IEnumerable<Product> GetProducts(); IDictionary<string, string> ValidateProduct(Product product); void CreateProduct(Product product); } public class ProductService : IProductService { public IProductProvider Provider { get; set; } public ProductService() { this.Provider = new ProductDBEntities(); } public IEnumerable<Product> GetProducts() { return this.Provider.ProductSet.ToList(); } public IDictionary<string, string> ValidateProduct(Product product) { Guard.AgainstNullParameter(product, "product"); Dictionary<string, string> errors = new Dictionary<string, string>(); if(!product.Name.HasValue()) errors.Add("Name", "Name is required."); if(!product.Description.HasValue()) errors.Add("Description", "Description is required."); if(product.UnitsInStock < 0) errors.Add("UnitsInStock", "Units in stock cannot be less than zero."); return errors; } public void CreateProduct(Product product) { Guard.AgainstNullParameter(product, "product"); this.Provider.AddToProductSet(product); this.Provider.SaveChanges(); } } [HandleError] public class ProductController : Controller { public IProductService ProductService { get; set; } public ProductController() { this.ProductService = new ProductService(); } public ActionResult Index() { return View(this.ProductService.GetProducts()); } public ActionResult Create() { return View(); } [AcceptVerbs(HttpVerbs.Post)] public ActionResult Create([Bind(Exclude = "Id")] Product product) { IDictionary<string, string> errors = this.ProductService.ValidateProduct(product); if(errors.Count > 0) { this.ModelState.Merge(errors); return View(); } else { this.ProductService.CreateProduct(product); return RedirectToAction("Index"); } } } public static class ModelExtensions { public static bool HasValue(this string value) { return !string.IsNullOrEmpty(value) && value.Trim().Length > 0; } public static void Merge(this ModelStateDictionary modelState, IDictionary<string, string> dictionary) { Guard.AgainstNullParameter(modelState, "modelState"); Guard.AgainstNullParameter(dictionary, "dictionary"); foreach(var item in dictionary) { modelState.AddModelError(item.Key, item.Value); } } } public static class Guard { public static void AgainstNullParameter(object parameter, string parameterName) { if(parameter == null) throw new ArgumentNullException(parameterName); } }Thoughts?
Jason Conway
MCP, MCTS, Certified SCRUM Master
asp.net blog
cberthold
Member
18 Points
4 Posts
Re: Dependency Injection Using IValidationDictionary
Dec 23, 2009 04:34 PM|LINK
I personally think the part that is missing in this solution is where the validation is being put in the first place. I of course ran into this same problem and immediately kicked myself for missing the flaw. If we expand our logic and validation as far as logically possible we end up with a stream from the client to the database as such.
If you are actually following this structure you really should be using two objects (an Entity object for database actions and a DTO or Data Transfer Object for communicating with the client), but for the sake of discussion we'll say that our model is very simple and there are no other entities that get involved like our simple ContactManager. In general good object oriented design says that unless we are coupling our objects we should not be moving our logic outside of the closest object in the relation.
For instance, if we have a Dog and we want to make it bark() then we should put the logic for making its mouthmove() and lungsexhale() inside the Dog object. If we then have a bird that chirps we make its beakmove() and its chestcompress(). Now if we want both of these animals to makenoise() we add an interface and implement makenoise().
No one knows better in this case than the object itself, how it should makenoise. The same can be said for a Product or any other object. I believe this to be why MVC has allowed for the IDataErrorInfo interface to be used to validate an object to be validated in a decoupled way. The service layer really should be applying the business rules after the validation. I think it makes more sense to use the built in bindings which will call this interface to validate the properties. If implemented correctly you wont even feel the performance hit. This is an entirely rough implementation that may or may not compile but I think you'll get my point
public class Product : IDataErrorInfo { private bool _needsValidation = false; private Dictionary<string, string> errors = new Dictionary<string, string>(); private int _id; public int Id { get { return _id; } set { if (_id != value) { _needsValidation = true; } _id = value; } } private string _description; public string Description { get { return _description; } set { if (_description != value) _needsValidation = true; _description = value; } } private int _unitsInStock; public int UnitsInStock { get { return _unitsInStock; } set { if (_unitsInStock != value) _needsValidation = true; _unitsInStock = value; } } #region IDataErrorInfo Members public string Error { get { return string.Empty; } } public string this[string columnName] { get { if (_needsValidation) { Validate(); _needsValidation = false; } if (errors.ContainsKey(columnName)) return errors[columnName]; else return null; } } protected void Validate() { if(string.IsNullOrEmpty(this.Description) || this.Description.Trim().Length == 0) errors.Add("Description", "Description is required."); if(this.UnitsInStock < 0) errors.Add("UnitsInStock", "Units in stock cannot be less than zero."); } #endregion }now in our controller we do:
UpdateModel(product)
if(this.ModelState.IsValid) {
Dictionary<string,string> errors = productService.ShouldWeApplyBusinessRules();
if(errors.Count == 0) productService.ApplyBusinessRules(product)
else MergeErrorsHere();
}
I think it would also be valid to add an interface to add errors to particular properties on the object through an interface and since the Validate() would not need calling again then you wouldn't lose those errors.
Thoughts?
Chris Berthold