The wages of sinHit that database one more time…
Originally posted at 3/11/2011
This time, this is a review of the Sharp Commerce application. Again, I have stumbled upon the application by pure chance, and I have very little notion about who wrote it.
You might have wondered why I named this blog series the way I did, I named it because of the method outline below. Please note that I had to invent a new system to visualize the data access behavior on this system:
- In red, we have queries that are executed once: 3 queries total.
- In aqua, we have queries that are executed once for each item in the order: 2 queries per each product in the order.
- In purple, we have queries that are executed once for each attribute in each of the products in the order: 1 query per attribute per product in the order.
Now, just to give you some idea, let us say that I order 5 items, and each item have 5 attributes…
We get the following queries:
- 3 queries – basic method cost
- 10 queries – 2 queries per each product
- 25 queries – 1 query for each attribute for each product
Totaling in 38 queries for creating a fairly simple order. After seeing this during the course review of the application, I have used that term for this method, because it is almost too easy to make those sort of mistakes.
More posts in "The wages of sin" series:
- (24 Mar 2011) Hit that database one more time…
- (23 Mar 2011) Inverse leaky abstractions
- (22 Mar 2011) Proper and improper usage of abstracting an OR/M
- (21 Mar 2011) Re-creating the Stored Procedure API in C#
- (18 Mar 2011) Over architecture in the real world
Putting aside the inefficient database access for second, let me ask this: do the entities in this application actually encapsulate any meaningful behaviour whatsoever? And if not then what's the point of using an ORM to materialize stuff as an entities?
I'm seriously asking as I haven't looked at the application myself, but seeing even basic calculations performed outside of the entities raises a warning flag to me. It's possible that building all this fancy architecture in front of NHibernate makes people forget the purpose of using an ORM in the first place in addition to making the kind of mistakes you point out in your post much more likely.
A lot of people will use NHibernate simply to streamline data access and to abstract the complexities of a data model they may not have control over, as well as mapping the relational world to C#....and little more. The objects end up being DTOs and thats it, not entities in the OOP sense.
Then any kind of business logic, behavior, or process, is located in service classes.
@Francois - Why bother with NHibernate in this case? WebMatrix.Data by all means will be a better choice.
These posts would be of so much more value if Ayende would post the "correct" way of doing it. Yes, we see the wrong way, now what is the right way?
@Tim, my thoughts exactly.
You should include a screenshot of the NH Prof warnings? Or are you finding these issues by browsing the source?
The correct way is to buy UberProf and use it to make your data access queries better. No, seriously!
All kidding aside, this is database-101 and ORMs-101 (select n+1). That's what HQL FETCH is for, but I don't know how to do that with the new IQueryable queries being that I don't yet have the luxry of using them.
NH Prof is basically me, package in software that you can download.
I am running the standard set of code reviews on the codebase.
NH Prof does the same thing, but it doesn't require my presence.
Sure, but presumably some of those reasons don't apply in this case, for instance, having a wacky database schema that you can't control. Moreover, if NHibernate is just an implementation detail for a bunch of DAOs and the objects returned from them have their guts spilled out for a service class to go poking around in, what benefit are you deriving from, say, NHibernate's change tracking mechanisms? If the methods of your service class are just Transaction Script style procedures that go "I get this data, I change this data, I save this data and I'm done" then there's no need for anything to be tracking what's changed since the service method knows full-well what it's done.
I guess all I'm saying is that if people want to do things that way then they probably want something simpler than NHibernate.
@Tim & @Fred,
You fix it by using a combination of NHibernate Futures
using fetch in certain queries to avoid n+1 situations.
Also, it is very difficult to do both of these correctly with a repository structure like the application above because the repository structure makes it difficult to access these useful features by trying to hide NHibernate too much.
The point of these reviews is not to specifically fix this project's code, but to point out that it is very easy to write very nasty code with the conventional repository pattern used in this project. I mean it takes 38 queries to process an order with 5 item where each item has 5 attributes.
As Ayende pointed out, these types of scenarios are far too easy to build. I would also appreciate a post showing a more efficient way of performing the same logic, both NH specific and with more general patterns.
I have some code that is very similar to the ProductAttributes loop in the demo code using the Entity Framework 4. Since EF4 allows assigning foreign keys directly, I have simply created a cache of the IDs to dramatically reduce the DB hits. I'd be interested in seeing other options that solve the scenario of "if in the DB, assign, and if not, create and assign" pattern.
@SteveR: IMO intelligent entities are much overrated. The domain model should contain stable domain stuff. Usually that turns out to be the basic form of the domain/data, not the behavior. So to me having dump or barely smart data in the data layer is usually a good thing.
Unless there is no meaningful behaviour to model, then I'd dispute that what you describe qualifies as a domain model. If it doesn't embody the rules of the business domain then how can it be said to model it?
If all you want to do is bring back a bunch of dumb data containers and manipulate them procedurally then I don't see the point of using an ORM; something with much simpler capabilities would suffice. If you're not going to use OOP techniques to deal with the complexities and variability of the rules in your domain there's little point in manifesting that data in terms of classes.
Isn't it enough to call session.Load(userName) instead of loading all data about user for this case ?