NH-2785 - StaleStateExceptions discarded on optional <join> table

[nhibernate-bot]

Mike Abraham created an issue — 1st July 2011, 11:05:52:

When extending an existing table via an optional (see abridged mapping below) and specifying optimistic locking via optimistic-lock="dirty" dynamic-update="true" (since <version> cannot be used in a <join>), updates to existing records in the join table that have been modified by another user, correctly throw a StaleStateException. However, the exception is caught and discarded because IsNullableTable is true (because the <join> is optional) in the following code from AbstractEntityPersister.

protected bool Check(int rows, object id, int tableNumber, 
IExpectation expectation, IDbCommand statement) 
{ 
		try 
		{ 
				expectation.VerifyOutcomeNonBatched(rows, statement); 
		} 
		catch (StaleStateException) // THE StaleStateException IS THROWN
		{ 
				if (!IsNullableTable(tableNumber)) // ... BUT IS DISCARDED HERE
				{ 
						if (Factory.Statistics.IsStatisticsEnabled) 
								Factory.StatisticsImplementor.OptimisticFailure(EntityName); 


						throw new StaleObjectStateException(EntityName, id); 
				} 
		} 
		catch (TooManyRowsAffectedException ex) 
		{ 
				throw new HibernateException("Duplicate identifier in table for: " 
+ MessageHelper.InfoString(this, id, Factory), ex); 
		} 
		catch (Exception) 
		{ 
				return false; 
		} 
		return true; 
} 

<class name="Case" schema="Cases" table="`Case`" optimistic-lock="dirty" dynamic-update="true"> 
		<id name="CaseId" > 
		  <generator class="identity"/> 
		</id> 
		<property name="CaseCode"/> 
		<property name="MarketingCaseCode"/> 
		<property name="Name" column="CaseName"/> 
		<property name="Status" column="StatusId"/> 


		<join schema="CSA" table="CaseEx" optional="true"> 
				<key column="CaseId"/> 


				<property name="Details"/> 
				<property name="Template"/> 
		</join> 
</class> 

---

Mike Abraham added a comment — 6th July 2011, 14:12:22:

Results of further investigation of the issue (based solely on reading the code, not on running any tests).

1. AbstractEntityPersister.Check returns a bool which is ignored by all callers of Check except for AbstractEntityPersister.Update, which returns the bool to its caller, UpdateOrInsert.

2. If this return value is false and if the object has any non-null fields, UpdateOrInsert assumes that the false is the result of an attempt to update a non-existent record, and so it does an insert.

3. I don't fully understand the circumstances under which UpdateOrInsert will be unsure whether or not the record already exists.

4. AbstractEntitypersister.Check returns false if its call to expectation.VerifyOutcomeNonBatched throws an unexpected exception. However, all implementations of expectation.VerifyOutcomeNonBatched do not throw any unexpected exceptions, so the code path triggered by Check returning false (see point 2) is never followed. Specifically, if the record is not there, expectation.VerifyOutcomeNonBatched will throw a StaleStateException, which will be ignored.

5. So unless there is some situation in which UpdateOrInsert will attempt an update for a non-existent record, it seems to me that the correct behavior is to catch the StaleStateException and throw a StaleObjectStateException, i.e., behave as if the test on IsNullableTable were removed. And if there is a situation where UpdateOrInsert attempts an update for a non-existent record, the current code won't handle that properly because it will silently discard the StaleStateException, thus acting as if the update had succeeded.