Part 5: Fixing the Broken Stuff
This is part 5 of my series on ASP.NET MVC with NHibernate. So far, we concentrated on NHibernate and persistence concerns. In this part, we’re going to correct our model and mappings to pass our tests. This will be the last full-time NHibernate post for a while. The next part will be focused on integrating Ninject, our inversion of control / dependency injection framework, with ASP.NET MVC.
If you’re just joining us, you can still catch up.
- Part 1 – Solution setup
- Part 2 – Model design
- Part 3 – Persistence mapping
- Part 4 – Persistence tests
First, some trivia. According to Fabio Maulo, the NHibernate logo is probably a sleeping marmot.
Know what you’re fixing
When correcting bugs, you should correct only bugs. This seems obvious. Yes, we write tests so we can find out what’s broken. The less obvious purpose is to know what’s not broken.
Confession: Sometimes I code first, then test. Sometimes I put on my pants, then my shirt. As long as you leave the house fully dressed, the order isn’t all that important. As long as you write your code and tests every day, the order isn’t all that important.
Now, when you’re on a team working on a project, I assume things *should* work a little different. I wouldn’t know. My project team is just me, and I’ve picked up a lot of bad habits from my team over the years.
Here are the results of the NUnit tests from part 4: 2 passed, 3 failed, 5 threw exceptions. 2 out of 10 is actually pretty good for me. Let’s work through these 8 problems one at a time.
Bare-minimum NHibernate debugging
NHibernate makes extensive use of the log4net log framework. It’s quick and painless to expose this log to NUni or most other test runners.
- In your data test project, add a reference to log4net.dll
- Add an app.config
- Add a new class called BaseFixture
- Set all of your test fixtures to inherit from the base fixture.
Here’s what your app.config should look like with the log4net configuration:
<?xml version="1.0" encoding="utf-8" ?>
<configuration>
<configSections>
<section name="log4net" type="log4net.Config.Log4NetConfigurationSectionHandler,log4net"/>
</configSections>
<log4net>
<appender name="Debugger" type="log4net.Appender.ConsoleAppender">
<layout type="log4net.Layout.PatternLayout">
<conversionPattern value="%date [%thread] %-5level %logger - %message%newline"/>
</layout>
</appender>
<logger name="NHibernate.SQL">
<level value="ALL"/>
<appender-ref ref="Debugger"/>
</logger>
</log4net>
</configuration>
Here’s the code for BaseFixture.
Public MustInherit Class BaseFixture
Protected Shared ReadOnly Log As log4net.ILog = GetLogger()
Private Shared Function GetLogger() As log4net.ILog
log4net.Config.XmlConfigurator.Configure()
Return log4net.LogManager.GetLogger(GetType(BaseFixture))
End Function
End Class
We’re calling log4net.Config,XmlConfiguration.Configure() just once. This loads the logging configuration from the app.config, which wires up log4net with Console.Out through the ConsoleAppender. With the example configuration, we'll get to see the SQL NHibernate is executing.
If you want something a lot more powerful, check out Ayende’s NHProf.
Problem #1
NStackExample.Data.Tests.CourseMappingTests.CanCascadeOrphanDeleteFromCourseToSections: NHibernate.TransientObjectException : object references an unsaved transient instance - save the transient instance before flushing. Type: NStackExample.Section, Entity: NStackExample.Section
Dim Course As New Course() With { _
.Subject = "SUBJ", _
.CourseNumber = "1234", _
.Title = "Title", _
.Description = "Description", _
.Hours = 3}
Dim Section As New Section() With { _
.FacultyName = "FacultyName", _
.RoomNumber = "R1", _
.SectionNumber = "1"}
Term.AddSection(Section)
Course.AddSection(Section)
Using Tran = Session.BeginTransaction()
ID = Session.Save(Course)
Session.Save(Section)
Tran.Commit() ' <==== Exception here
End Using
Session.Clear()
When a transaction is committed, the session is flushed to the database. That just means data changes are written to the database. This exception is telling us we’re trying to save an object, but it references another object that isn’t saved. We can infer that this means cascading is turned off for this relationship. When we go to this particular line in the code, we see that this transaction is committing a save (INSERT) of a new course, and that this course references a new section. If this were a TestCascadeSaveFromParentToChild test, we would adjust our mapping. In this case, we’re testing the delete-orphan functionality, not the cascade of inserts and updates. We’ll explicitly save the section in this transaction as well.
After making the change and re-running our tests, we see that the same test is still failing, although it got further.
'Test removing
Course.RemoveSection(Section)
Using Tran = Session.BeginTransaction()
Session.Save(Course)
Tran.Commit() ' <==== Exception here
End Using
Session.Clear()
Now we're violating a unique constraint. This is because we've called Session.Save(Course) twice. Session.Save is for saving new objects only. Session.SaveOrUpdate or simply Session.Update should be used to save the course. Since neither of those return the identifier, we'll need to get that from our initial Save. We make those change, recompile, and test.
Next, we get this:
NStackExample.Data.Tests.CourseMappingTests.CanCascadeOrphanDeleteFromCourseToSections: NHibernate.Exceptions.GenericADOException : could not delete collection: [NStackExample.Course.Sections#912b489a-4d12-4bc9-9d68-9c6b0147b799][SQL: UPDATE "Section" SET Course_id = null WHERE Course_id = @p0] ----> System.Data.SQLite.SQLiteException : Abort due to constraint violation Section.Course_id may not be NULL
This message is telling us that when we disassociated the course from the section, NHibernate tried to set the Section's Course_id to NULL. This violated a not-null constraint. More importantly, this violated our business rule. The section was orphaned and should have been deleted. To corrected it, we update our mappings. In our course mapping, we’ll add Cascade.AllDeleteOrphan() to the one-to-many sections relationship.
HasMany(Function(x As Course) x.Sections) _
.AsSet() _
.WithForeignKeyConstraintName("CourseSections") _
.Cascade.AllDeleteOrphan()
After a compile and retest, we get this:
NStackExample.Data.Tests.CourseMappingTests.CanCascadeOrphanDeleteFromCourseToSections: NHibernate.PropertyValueException : not-null property references a null or transient valueNStackExample.Section.Course
This error is strange. Basically, even though we’re going to delete the section now that it’s orphaned, NHibernate is complaining that we’ve set Section.Course = null / nothing. For now, simply to appease the marmot god, we’ll remove our not null constraint on Section.Course. If you turn on log4net NHibernate.SQL logging, you’ll see that this operation wouldn’t violate the NOT NULL database constraint. The orphaned row is being deleted. We’re only failing an internal NHibernate property check. I’m hoping for a better explanation from Tuna, one of the NHibernate gurus, who’s been extremely helpful with this series.
The second problem is basically a disconnect between relational database concepts and object relations. All one-to-many database relationships are bidirectional. The many-to-one is implied. In an object graph, we can have a reference from a parent to its children but not reference from the child back to the parent, or vice-versa. Object relationships are unidirectional. Even though it would indicate a bug in most circumstances, we still have to tell NHibernate which of our two unidirectional relationships is the “real” one that we want to persist to the database. The default is to use the one-to-many. This means that the relationship that is saved is based on membership in a course’s sections collection. We would rather have the relationship based on the many-to-one relationship: the Section’s Course property. To do this, we specify Inverse() in our mapping for Course.Sections. This tells NHibernate that the “other side” of the bidirectional relationship is the one we want to persist.
Bug solved. Onward! Wait. Compile it and rerun your tests. You may have unknowingly fixed other problems.
Problem #2
NStackExample.Data.Tests.CourseMappingTests.CanCascadeSaveFromCourseToSections: Expected: <nstackexample.section> But was: <nstackexample.section>
This is another misleading issue. Our test is checking the equality of two sections.
Q: How did we define the equality of a section?
A: We didn’t, so Object.Equals is just looking to see if these two happen to be the same instance. Since one is rehydrated from the database, they aren’t. We’ll have to define our own equality check.
Q: How should we define equality?
A: If two instances represent the same section, they are equal. Wait. Why are we just talking about sections? Let’s expand that to cover all entities.
Q: Where can we put this rule?
A: We should override Equals In our base Entity class, so all entities can use it.
Q: How do we know if two instances represent the same entity?
A: The ID fields will be equal.
Q: What about when we haven’t persisted the object and don’t have an ID yet?
A: We’ll assume they’re not equal. If a specific class needs something more accurate, it can override Equals again.
Here’s the code:
Public Overrides Function Equals(ByVal obj As Object) As Boolean
Dim other As Entity = TryCast(obj, Entity)
If other Is Nothing Then Return False
Return ID.Equals(other.ID) AndAlso Not ID.Equals(Guid.Empty)
End Function
Let’s recompile and test again. Look at that! We have 6 out of 10 tests passing now.
Problem #3
NStackExample.Data.Tests.SectionMappingTests.CanCascadeSaveFromSectionToStudentSections: NHibernate.PropertyValueException : not-null property references a null or transient valueNStackExample.Student.MiddleName
This particular error can be fixed in two ways. We have defined our Student mapping to not allow null middle names. Our test of the Sections cascade is failing because it doesn’t set a value in middle name. We can either change our test to put something, even an empty string in middle name, or we can change our mapping to allow nulls. I choose option #1. Changing our mapping to allow nulls could lead to NullReferenceExceptions. Let’s set MiddleName = String.Empty around line 83. After a compile and test, we get this error.
NStackExample.Data.Tests.SectionMappingTests.CanCascadeSaveFromSectionToStudentSections: NHibernate.TransientObjectException : object references an unsaved transient instance - save the transient instance before flushing. Type: NStackExample.StudentSection, Entity: NStackExample.StudentSection
This error is saying that our cascade is failing. Why? Because we didn’t actually specify cascade on one of the one-to-many relationships pointing to StudentSection. Since we know both Sections and Students should cascade to StudentSection, go add Cascade.All to both. Add Inverse() while you’re there.
Compile and retest. Success.
Problem #4
NStackExample.Data.Tests.StudentMappingTests.CanCascadeSaveFromStudentToStudentSection: NHibernate.TransientObjectException : object references an unsaved transient instance - save the transient instance before flushing. Type: NStackExample.Student, Entity: NStackExample.Student
This one is a bug in our test. If you look at what we're testing and what we're actually saving, you'll realize that we should be saving Student, not Section. Fix it and try again. Now we have the same MiddleName bug we had in problem #3. Fix it as well. Test again. Now we get a NullReferenceException. Why?
If you look at our test of the Student mapping, you’ll see that we’re not checking the correct results. This was most likely a sloppy cut-and-paste job in the middle of a conference call or some other distracting scenario. Swap in the correct expected results:
'Check the results
Using Tran = Session.BeginTransaction
Student = Session.Get(Of Student)(ID)
Assert.AreEqual(1, Student.StudentSections.Count)
Assert.AreEqual(Student.StudentSections(0), StudentSection)
Tran.Commit()
End Using
It works!
Problem #5
NStackExample.Data.Tests.TermMappingTests.CanCascadeSaveFromTermToSections: NHibernate.TransientObjectException : object references an unsaved transient instance - save the transient instance before flushing. Type: NStackExample.Section, Entity: NStackExample.Section
This is the same as problem #3. Our cascade from term is not cascading the save down to the section. Go add Cascade,All()and Inverse() to Term.Sections.
Problem #6
NStackExample.Data.Tests.TermMappingTests.CanSaveAndLoadTerm: Expected: "Fall 2009" But was: null
In this test, we see that we were expecting a value in the Name property of Term, but we got null / nothing. Whenever you see this, you should first check your mapping. In this case, you'll quickly discover that we didn't map that property. Go map it. Next, you'll discover a bug in our tests. We're comparing the wrong date. EndDate should be compared with December 1st, 2009.
It works!
That really wasn’t so terrible. It probably took more effort to read this post than it did to correct those bugs.
Oh yeah, and get some source control.
Before I post the source code, I’ll be updating to Fluent NHibernate v1.0 RC and fixing some of the typos and reference problems you’ve commented about. With any luck, the corrected source code for this part, along with the next part will be out before the weekend is over.
Edit: Download the entire solution in VB or C#. I’ve upgraded to Fluent NHibernate v1 RC and updated most of the other assemblies.
Jason
- Glad to be moving on to Ninject soon.
Subscribe to my blog
Project Lightning Bug
