Ayende @ Rahien

It's a girl

NH Prof Deep Dive: Implemented the Unbounded Result Set warning

IconSo, I talked a bit about the architecture and the actual feature, but let us see how I have actually build & implemented this feature.

This is the actual code that goes into the actual product, I want to point out. And this is actually one of the more complex ones, because of the possible state changes.

public class UnboundedResultSetStatementProcessor : IStatementProcessor
{
	public void BeforeAttachingToSession(SessionInformation sessionInformation, 
		FormattedStatement statement)
	{
	}

	public void AfterAttachingToSession(SessionInformation sessionInformation, 
		FormattedStatement statement, OnNewAction newAction)
	{
		if(statement.CountOfRows!=null)
		{
			CheckStatementForUnboundedResultSet(statement, newAction);
			return;
		}
		bool addedAction = false;
		statement.ValuesRefreshed += () =>
		{
			if(addedAction)
				return;
			addedAction = CheckStatementForUnboundedResultSet(statement, newAction);
		};
	}

	public bool CheckStatementForUnboundedResultSet(FormattedStatement statement,
		 OnNewAction newAction)
	{
		if (statement.CountOfRows == null)
			return false;

		// we are discounting statements returning 1 or 0 results because
		// those are likely to be queries on either PK or unique values
		if (statement.CountOfRows < 2)
			return false;

		// we don't check for select statement here, because only selects have row count
		var limitKeywords = new[] { "top", "limit", "offset" };
		foreach (var limitKeyword in limitKeywords)
		{
			//why doesn't the CLR have Contains() that takes StringComparison ??
			if (statement.RawSql.IndexOf(limitKeyword, StringComparison.InvariantCultureIgnoreCase) != -1)
				return true;
		}

		newAction(new ActionInformation
		{
			Severity = Severity.Suggestion,
			Title = "Unbounded result set"
		});
		return true;
	}

	public void ProcessTransactionStatement(TransactionMessageBase tx)
	{
	}
}

And now the test:

[TestFixture]
public class Ticket_51_UnboundedResultSet : IntegrationTestBase
{
	[Test]
	public void Will_issue_alert_for_unbounded_result_sets()
	{
		ExecuteScenarioInDifferentAppDomain<LoadPostsUsingCriteriaQuery>();

		var statement = observer.Model.RecentStatements.Statements
			.OfType<StatementModel>()
			.First();
		Assert.AreEqual(1, statement.Actions.Count);
		Assert.AreEqual("Unbounded result set", statement.Actions[0].Title);
	}
}

And, just for fun, the scenario that we are testing:

public class LoadPostsUsingCriteriaQuery : IScenario
{
    public void Execute(ISessionFactory factory)
    {
        using (var session = factory.OpenSession())
        using (var tx = session.BeginTransaction())
        {
            session.CreateCriteria(typeof(Post))
                .List();

            tx.Commit();
        }
    }
}

And this is it. All you have to do to implement a new feature. This make building the application much easier, because at each point in time, we have to deal with only one thing. It is the aggregation of everything put together that is actually of value.

Also, notice that I heavily optimized my workflow for tests and scenarios. I can write just what I want to happen, not caring about how this is actually happening. Optimizing the ease of test is another architectural concern that I consider very important. If we don't deal with that, the tests would be a PITA to write, so they would either wouldn't get written, or we would get tests that are hard to read.

Also, notice that this is a full integration tests, we execute the entire backend, and we test the actual view model that the UI is going to display. I could have tested this using standard unit testing, but in this case, I chose to see how everything works from start to finish.

Comments

Konstantin
12/27/2008 04:06 AM by
Konstantin

Hey Ayende,

I especially liked the trick with boolean addedAction in closure to make sure that you do checks until you add suggestion or realize it is not required.

IndexOf you use to find limitKeword looks ugly.

What will happen if RawSql contains column named "toptimelimit_offset"? Your code will not detect Unbounded Result Set in this case. Looks like a major bug in this feature to me.

Why is your statement.CountOfRows nullable? Any query always deals with zero or more rows.

Ayende Rahien
12/27/2008 05:06 AM by
Ayende Rahien

Konstantin,

I am not aiming for 100% correctness here, but this can be dealt with using SQL parsing, which I am already doing.

As for CountOfRows being nullable. This means the count of rows returned, so CUD statements has no such thing.

Konstantin
12/27/2008 08:30 AM by
Konstantin

Ayende,

Parsing SQL looks like smell because it was generated by NHibernate and you could have access to it's object implementation... But I guess you are doing this because you can't actually access it via mechanism you have chosen (custom logger), right?

Ayende Rahien
12/27/2008 12:01 PM by
Ayende Rahien

Parsing SQL is actually pretty easy, I already have to do this in order to get the pretty formatting working.

Stephen
01/02/2009 03:18 PM by
Stephen

I haven't used nhibernate to deeply, but I assume it has a provider model to it can understand different database implementations and how to at least generate query strings for it, if not also parse them..

I take it you aren't planning a providers model for the native query parsing (if the nhibernate providers aren't already expected to do this?).

Ayende Rahien
01/02/2009 03:22 PM by
Ayende Rahien

Stephen,

Parsing SQL and how NH does it are two separate issues.

Jeff Brown
01/07/2009 09:50 AM by
Jeff Brown

Seems odd to have to run CheckStatementForUnboundedResultSet if data is available and set up an event handler for ValuesRefreshed that does the same kind of this later on.

Why not model the process as adding a verification behavior to the statement scheduled to be performed when data becomes available (now or later).

In fact, you might want to separate two different parts of the lifecycle: setup of verification behaviors associated with statements, and deep execution / inspection of those statements later on.

Comments have been closed on this topic.