PrimeDigit – A Design Blog by Will Shaver

Having successfully submitted a few patches to NHibernate [NH-1280][NH-1260][NH-1259], Ayende Rahien asked me to comment on the difficulty of adding a totally new feature, and provide some hints for others. I am by no means an expert on NHibernate, just a regular user so here goes…

My most recent change set was updating the Criteria Query API to allow for queries involving HAVING clauses. In performing this change I also discovered a bug whereby complicated queries would get parameters out of order. In this post I will cover:

• The feature added (need and implementation)
• Determining problem need
• Hints for getting your patch accepted

The Feature

The Criteria Query API provides some valuable tools for creating dynamic queries without doing crazy string manipulation (yuck). I had a situation where I wanted to validate entities to check for duplicates prior to the transaction being committed. Because of the temporal nature of the project I’m working on, I can’t do it with a table constraint or other simple means and need to check valid dates.

To solve it with HQL would involved about 10 lines of messy reflection and string concats where solving it with the criteria API took about 4. Here’s the code:

ICriteria criteria = session.CreateCriteria(typeof(Person));
criteria.SetProjection(Projections.ProjectionList()
  .Add(Projections.Max("Id"))
  .Add(Projections.GroupProperty("Name")))
  .ProjectionCriteria.Add(Restrictions.Gt(Projections.Count("Name"), 1));

The resulting SQL for this code should be:

"SELECT MAX(Id), Name FROM Person GROUP BY Name HAVING COUNT(Name) > 1"

Imagine my surprise when this relatively simple Criteria Query failed while the same HQL query succeeded. After examining the SQL it produced I determined that there was no HAVING clause being produced.

Fearing that I was doing something wrong I opened up the source code for NHibernate and performed a textual search for “having” as I knew that at some point it must be produced by the SQL generator. I was unable to locate anything in the Criteria API that was even remotely related to HAVING queries.

Having exhausted that possibility I posted to the google nhusers group to make sure that I wasn’t missing something. After some discussion it was obvious that this was indeed a lacking feature and that it would be appreciated by the community.

After determining that it was indeed a problem with the codebase I launched into patching it. I wrote test cases to be able to debug into the code in the Criteria interface. I recommend adding the folder for your tests as:
\src\NHibernate.Test\NHSpecificTest\NH1999
Then migrating it to the specific issue number after you’ve made progress enough to report the issue. This prevents nasty collision problems if someone else takes the next number.
\src\NHibernate.Test\NHSpecificTest\NH1280
I’ve also recently taken to adding a description after the folder such as “NH1291AnonExample” in my latest patch. This makes it easier to determine which set of tests has failed for other developers.

The patch itself is rather complicated and out of the scope of this post as it contained changes to almost 50 files. In writing the patch I tried a couple of different methods for adding my features and even had to revert a couple of files from the source. I worked from the output to the input, backing out from the SQL generator to the individual Criteria and Criterion classes.

As I wrote the patch I made sure that I didn’t negatively impact other use cases. If your patch breaks other tests it won’t be accepted even if it implements “teh best featre evar”! For added tests I merged my changes into both my active work project and the Linq to NHibernate project. Doing so revealed cases that I hadn’t considered in my initial tests and allowed me to add in those tests and deliver a more solid patch.

Before submitting any patch I always go through a cleanup of my changed code. This involved going through my patch file by file making sure that I’m only submitting code that pertains to the patch and only changes things that need to be changed. (Using TortoiseSVN this is quite simple, folder menu -> check for modifications)

I try and keep my formatting changes to a minimum. Resist the urge to re-arrange existing code, convert tabs to spaces etc. While NHibernate and many other open source projects could use some cleanup work, that should be performed by dedicated committers. In our case the organization is loosely tied to the Hibernate project and would make it more difficult to port newer features if the NHibernate code is re-arranged.

Submit the patch and leave a clear explanation of the problem it solves and how it solves it. Best of luck and thanks for contributing to the project!