MVC Storefront: Intermission's Over, Made Some Changes
I'm sure this isn't the last time I'll have to wrestle with Linq/Linq To Sql; I'm just happy it's over. The good thing is I feel good about what I came up with. The client doesn't like delays, but it's important to think these things through and do it correctly, or your project will explode!
It's Not Linq's Fault
Steveo left this comment on my last post, in which I explained a roadblock due to some platform weirdness:
This is a great chance to refactor with NHibernate 1.2 and take advantage of a more mature ORM with a great community of support and fellow developers.
It's not too late - show us those refactoring skills now of how it is possible to swap out your data layer.
I recommend Spring.NET and NHibernate - mixed in with some NHibernate Query Generator. If your really stuck on LINQ you can get Ayende's Linq over NHibernate.
:)
NHibernate's pretty cool, to be sure, but I'm not entirely certain how it will make my LazyList work. The problem is a platform one, and what I found out today is that if you set any Enumerable anything as a property, it's Count property will be accessed when you load the parent object. This negates using any deferred loading for any POCOs, period.
The thing about this kind of thinking is that it's all too common - and I do it way too much. "Just toss what you've spent weeks on and go with Solution X". Now I'm always willing to admit I'm wrong (I do it often), but in this case I identified the problem pretty clearly, and changing directions was pretty simple. A wholesale swap out doesn't solve any problems - it usually creates new ones!
Yes, I know NHibernate has eager/lazy loading built in, but I don't think rebuilding my engine right now is going to improve my gas mileage :p so I've changed directions a bit. And I'm sure I'll get some comments on this...
Getting Dumber
The problem, as it turned out, was that I have to respect that what I'm doing involves seriously DUMB repositories with no logic in them at all. Yes, I do know that in the classic Repository pattern it hands back objects and does the querying. But in Rob's Repository, I want the Repositories to be a pipeline - i'll fill the cups on the other end.
To get around my LazyList problem, I simply stripped the list definitions from the Repository, and let the Service class do the "knitting". This added two lines of code to my service class for Catalog:
/// <summary> /// Returns a single product by ID /// </summary> /// <param name="id">The Product ID</param> /// <returns></returns> public Product GetProduct(int id) { //Get the product from the repository Product p = _repository.GetProducts() .WithProductID(id).SingleOrDefault(); //Images p.Images = _repository.GetProductImages().ImagesForProduct(p).ToList(); //Reviews p.Reviews = _repository.GetReviews().ReviewsForProduct(p).ToList(); return p; }
Unfortunately, going with this pattern means I have to add a class to the project that describes the relationship between Product and Category:
public class ProductCategoryMap { public int CategoryID { get; set; } public int ProductID { get; set; } }
And then provide a method in the Repository to get this information (as well as add an interface for it):
/// <summary> /// Gets the relationships from the DB for Products/Categories /// </summary> /// <returns></returns> public IQueryable<ProductCategoryMap> GetProductCategoryMap() { return from pc in db.Categories_Products select new ProductCategoryMap { ProductID = pc.ProductID, CategoryID = pc.CategoryID }; }
At first this really bumbed me out - I don't want DB structure bits to bleed into my application. At the same time, doing this allows me some greater power in terms of querying, and I'm not sure I've really violated anything here - I've just made life a whole lot simpler.
Finally, in order to load the Products for a selected Category, I had to adjust the GetCategory() method on my service class:
/// <summary> /// Returns a single category by ID /// </summary> /// <param name="id">The Category ID</param> /// <returns>Category</returns> public Category GetCategory(int id) { Category result = _repository.GetCategories() .WithCategoryID(id) .SingleOrDefault(); result.Products = (from p in _repository.GetProducts() join cp in _repository.GetProductCategoryMap() on p.ID equals cp.ProductID where cp.CategoryID ==id select p).ToList(); return result; }
I tweaked my tests, and everything is back in order! I have to say - it feels much better at this point; leaner and meaner, and my goal of keeping the Repository simple is working nicely.
Shopping Cart Redo
I think I got far too little sleep the other day. I looked over the cart refactor I did and slapped myself. Thank you for your patience in this matter...
You'd think that a Shopping Cart would be simple after how many times I've done this. But, mix it with Linq and TDD and, well, it sort of highlights the pain we as developers go through when new stuff comes out: how do we do the simple stuff with these new tools?
Anyway - I've greatly simplified the whole cart thing, and it's back to being it's dumb self (sort of). I've kept the list operations (Add, Remove, Find) on the cart, and refactored the Cart Repository to have 4 total method:
public interface IShoppingCartRepository { IQueryable<ShoppingCart> GetCarts(); IQueryable<ShoppingCartItem> GetCartItems(); void SaveCart(ShoppingCart cart); bool DeleteCart(ShoppingCart cart); }
The CartService class is back in action, and does what you'd think it should do - Gets and Saves a cart. This will change as we get into migrating unknown users - and yes I'll get there.
The next webcast will go farther down the ShoppingCart path, as we swap Repositories for authenticated users :).
RE: Implementing in NHibernate
Source code's available right? Just let the community implement their own implementation, from the NHibernate side for comparison. It would cut down on your manual loading/matching that you're doing in your repositories for sure. Why not LLBLGen, or CSLA, Wilson OR/M or EF for that matter? Meh.
Regarding the IShoppingCartRepository, typically you don't see more than one Aggregate Root per repository. In fact, I usually do a marker interface, with some generic constraints to prevent this. Don't want child entities accessed outside of their root, right? :)
@Jimmy: >>Source code's available right? Just let the community implement their own implementation,<<
Yep - that's what I'm all about. But there's not really a way to do this with NHib - from what I see here, it's a platform issue. I don't want the tool to bleed into the app and I understand NHib is supposed to be free of that - but is it really? You wanna work something up for me? I'd love it!
>>>It would cut down on your manual loading/matching that you're doing in your repositories for sure<<<
I would suggest that the "manual loading" is pretty minimal, yes? I'll go toe to toe with you on this as long as you count the XML/config files NHib needs to run :) as LOC.
>>>typically you don't see more than one Aggregate Root per repository. In fact, I usually do a marker interface, with some generic constraints to prevent this. Don't want child entities accessed outside of their root, right? :)<<<
If you wouldn't mind translating this for non-DDD typed (like myself) I'd surely appreciate it! :):)
From how I've understood DDD, the aggregate root in your IShoppingCartRepository is the ShoppingCart, and ShoppingCartItem is its child. DDD says that a child should only ever be accessed through it's aggregate root - that is, to get to any ShoppingCartItem, you should have to go through the ShoppingCart. GetCartItems() violates this "rule".
Great series of posts by the way, keep it up :)
Could you elaborate on the "platform" issue? Have you been able to isolate with something NOT using Linq to SQL? Is it related to IQueryable?
The count property will be accessed when you load the parent object?? What does that mean? Would you have a stack trace available?
I haven't been able to reproduce easily.
I'm not clear on why you think this is "not LINQ's fault". From your explanation so far, it seems like it IS LINQ's fault. Can you explain your reasoning?
Rob,
I want to commend you on the work you are doing here. All too often people posts lessons & tutorials on the web and they are either very slight modifications of the same code all over the web OR they are canned tutorials where EVERYTHING goes right and real world issues are not addressed.
The fact that you found a "serious" issue and then refactored it is great. The fact that you posted about your confusion & frustration and wrote out your brainstorming is AWESOME. I especially like how you are dialoging with other developers on alternative solutions. I encounter these situations all the time and it helps me gain perspective on how to handle these situations.
I hope that when this project is completed you and others will be inspired to pick up a new project. As someone who considers himself to still be a novice this type of information is something that I would even pay for - it is that valuable to me!
Aaron
An alternative approach I use to returning IQueryable<T> is to return IEnumerable<T> instead, which is what IQueryable<T> inherits from so you can simply return your query results. I can use ToList() on the returned IEnumerable<T> or alternativley write additional LINQ queries directly against it.
Rob,
IMHO You should be able to use L2S EF NH XML or product X for persistence.
Linq is a great step in this direction as it standardizes the query language, something we have needed for a long time, however there is no DataContext equivalent which creates a problem.
Follow your gut, this is a great journey, don't let the purist on either side cloud your decisions. I think in the end we will land somewhere in the middle.
Keep up the good work!
@Francois: >>>Could you elaborate on the "platform" issue? Have you been able to isolate with something NOT using Linq to SQL? Is it related to IQueryable?<<<
Sure - by "platform" I mean .NET; no I wasn't able to isolate this without LINQ because I am specificly using Linq here (IQueryable).
>>>The count property will be accessed when you load the parent object?? What does that mean? Would you have a stack trace available?<<<
Using the Product example - if I have a property called "Images" that is IList - when I set that property, the parent object (in this case Product) calls it's Count property, thus firing off the query. I reset IList to be IEnumerable and IQueryable - it doesn't matter, Count gets called every time.
The Call Stack is in my last post - you can see the highlighted spot where Count is called.
@David Nelson: >>>I'm not clear on why you think this is "not LINQ's fault". From your explanation so far, it seems like it IS LINQ's fault. Can you explain your reasoning?<<<
Sure. What I've found is when you set an IList as a property, the CopyTo() method is called on the "from" list - transferring it's list items to the "to" list. This, in turn, calls the "Count" property since it needs to know the length of the List array being copied (which is weird, but...).
This isn't part of LINQ - this is an IEnumerable feature, thus the platform. Given this, you can see how using IQueryable as a list is not going to solve any problems for me :).
@Aaron - thank you :). The positive feedback keeps this project alive :).
@Des: Same thing happens with IEnumerable unfortunately :(. I don't want my results yet - only when I call for em.
@Mike: Good advice :). You forgot to tell me how DDD will save my soul :).
@Rob: What I meant (and said) was trying to isolate LINQ to SQL; that is, trying to reproduce with your TestCatalogRepository for example.
The test repository is using LINQ to objects and I'd bet you wouldn't have the same bug. That is why I also suspect it is a LINQ to SQL specific issue, not simply a LINQ + IQueryable concern.
From what I've seen in reflector, it looks like ObjectReaderCompiler is doing some crazy stuff but the problem is it is doing this crazy stuff in a dynamic method, hence very difficult to debug.
I'd submit this as a concern/bug to the LINQ to SQL team; at least to know what they're doing behind the scene.
@Francois: I understand what you mean - the thing is that if I go after Linq To SQL I may find the answer, but it may not be the core answer - just the symptom inside Linq To Sql.
In other words - what I see in front of me is IQueryable being called from IList; neither of these have anything to do with Linq To Sql.
Having said that, I am beginning to smell smoke WRT Linq To Sql since I can't repro this with Ling To Objects. I'm on IM with their team but I can't open a bug until I have a clear path on this - which I don't just yet... but I'm lookin!
I'm not sure if it is me, but it seems as though the latest code drop passes all the unit tests but doesn't actually work when you run the website.
I was getting a null reference exception on the viewdata I think, sorry can't be more exact, on an old machine now.
OK,
In checking the below code, it appears to give you true Lasy List Type behavior without the issues you were seeing. Does this make your objects too smart for what you are trying to accomplish? Am I missing something Major (likely in that what I put below doesn't really work)? What is different about this type of implementation and the Lazy List that gives it different behaviour?
public static class OrderObjectHelper<T>
{
public static IList<T> MakeLazy (ref IList<T> privatelist, IQueryable<T> qry ,ref bool retrieved )
{if (!retrieved)
{
privatelist = qry.ToList();
retrieved = true;
}
return privatelist;
}
}
public class Order
...
private OrderRepository _db;
public OrderRepository DB
{
get
{ if (_db == null) _db= new SQLOrderRepository();
return _db;}
set
{ _db = DB; }
}
private bool OrderItemsRetrieved = false;
private IList<OrderItemData> _orderitems;
public IList<OrderItemData> OrderItems
{
get
{
return OrderObjectHelper<OrderItem>.MakeLazy(ref _orderitems, DB.GetOrderItems().Where(p => p.OrderID == OrderID & p.ParentID == null) ,ref OrderItemsRetrieved);
}
set
{
OrderItemsRetrieved=true;
_orderitems=value;
}
}
}
Hmmm, Reading your Further Comment, not sure I was testing the same situation. Will Look further. Thanks Again for this series and discussion.
Thinking more, I guess with what I put, you do technically avoid setting the Ilist as a property (at least Linq does) so maybe it would avoid the situation. It would still allow you to set it from joied data in cases where you wanted to prefetch the information.
My point about IEnumerable<T> was that instead of having IQueryable<T> in my interface definitions, I have IEnumerable<T>. That way I don't have an issue when implementing a repository that doesn't have a LINQ provider.
Challenge accepted!
www.lostechies.com/.../the-mvc-storefr
Ahhhh...probably going to be a huge disaster.
@Rob:
When people make big suggestions (a la switching to NHibernate), I suggest you use Ayende's new motto: "Great idea! Please send the patch" :)
Or rather "That's a wonderful idea! I eagerly await your patch!"
This sounds a bit simplistic... and bear in mind I haven't actually tested it with SQL Profiler... just web compiler I'm afraid ;). But essentially if you refactored the LazyList.Count as follows:
Public ReadOnly Property Count() As Integer Implements ICollection(Of T).Count
Get
If Me._Inner IsNot Nothing Then
Return _Inner.Count
Else
Return query.Count
End If
End Get
End Property
if the inner list hasn't been loaded, would the entire object graph still be loaded if you call query.Count or would Linq2SQL be smart about it and just execute something along the lines of select Count(*) from... ?
It probably can't be this simple, but what do y'all think?
PS @Rob - The introduction of mapping classes is really ala ProductCatagoryMapping is really unfortunate. They really have no place in a domain model being concerns specific to the data access layer.
Well I have confirmed that the following model would provide Lazy Loading without requiring the Objects to be aware of the Repository Or the Services. Or moving the mappings up to the service layer.
Create a delegate that calls itself that object can use to populate their children.
public delegate IQueryable<T> CommerceGetChildDelegate<T, T2>(T2 me);
Add the delegate as a settable property to your objects for each Ienumerable you want to lazy Load like
public IList<Product> Products { get; set; }
public MVChelper.CommerceGetChildDelegate<Product, Category> ProductsMethod { set { Products= new LazyList<Product>(value(this)); } }
This Really only adds the one extra property to the object class. And Either the service Class or the Repository Class could use it for Lazy Loading.
Change the assignment in the repository (or service) to
ProductsMethod = new MVChelper.CommerceGetChildDelegate<Product,Category>(GetProductsByCategory)
Add A Method in your repository (or service) for objects to pass them selves to for getting their Ienumerable Children. It can be private.
private IQueryable<Product> GetProductsByCategory (Category c)
{ return from p in GetProducts() join cp in db.Categories_Products on p.ID equals cp.ProductID
where cp.CategoryID == c.ID
select p; }
Now everything Lazy Loads and it only requires 1 extra settable property on your POCO Objects. Is this enough to make them no longer POCO?
What do you think?
Erik
@Rob:
"""
Sure. What I've found is when you set an IList as a property, the CopyTo() method is called on the "from" list - transferring it's list items to the "to" list. This, in turn, calls the "Count" property since it needs to know the length of the List array being copied (which is weird, but...).
This isn't part of LINQ - this is an IEnumerable feature, thus the platform. Given this, you can see how using IQueryable as a list is not going to solve any problems for me :).
"""
While I agree it's not LINQ's (or LINQ to SQL's) fault, I fail to see that you've correctly explained that the behavior (that assigning and IList to a simple property) causes a CopyTo. IEnumerable is just an interface, and I've assigned instances of various IEnumerable types -- including IList -- to many different simple and non-simple properties without seeing the behavior I think you're claiming.
Are you certain that it's not an artifact of how you wrote a property setter, or some other forehead-slapper?
As far as IE<T> vs IQ<T> -- I religiously stick to IQueryable if the source is IQueryable. I *want* the query to be composed as far as inhumanly possible, to push as much processing to the server-end of the connection as I can, avoiding a chatty relationship.
@Keith: I'm not completely positive that this behavior is causing the problem. What I do know is that LazyList works - I tested it before I used it in the app (a spike with Ayende). When I used it as a property, however, assigning it to an IList, it did not work and the only thing the stack showed was a call to Count/CopyTo, which would cause the enumeration.
I'm with you on the "last possible moment" - I very well may just push the boundaries here and set Category::Products to IQueryable :).
Scott Allen pointed out, I think, that the issue was partly because of the query:
http://odetocode.com/Blogs/scott/archive/2008/05/21/12122.aspx.
@Rob,
Can you create a unit test that reproduces the behavior you are seeing? I am not able to replicate it with the May 14th check-in; I think I am not understanding your description of the problem. Unless you have moved on and don't care about this problem anymore :)
You say the problem is that Count is being called during a property setter when you don't expect it. Can you not tell from the stack trace how Count is being called? Maybe if you posted the stack trace...
David in fairness I'm having a problem nailing this one too in terms of repro. Damien Guard (now on the Linq team) is trying to repro it and can't.
I can see that LazyList does indeed work just fine on it's own, but I think the issue is when you use Linq To Sql behind it, using the new keyword when returning a result.
@Stan I read Scott Allen's post - it's interesting reading and I was fairly positive I'd find a way around the problem, but the solution (for me) was to go sideways and not throw more code at the problem I like where it ended up a lot.