Catch practices: avoid losing catched exception information. (#1555)

* Catch practices: avoid losing catched exception information.

All catches have been reviewed for avoiding losing original exception information:

 * When another exception is raised, it should embed the original one as an inner exception.
 * Otherwise when re-thrown, it should be done without altering its original stack-trace.
   Preferred way is of course to use "throw;" in the catch clause without specifying the exception, but otherwise ReflectHelper provides a way to preserve the stack-trace.
 * Otherwise it should be transmitted to the logger.

Some cases still do not follow this pattern for avoiding being too noisy, like "Try" utilities, failure in closing an already failed object, some "implemented by exception" features, ...

Author: fredericDelaporte

src/NHibernate/Async/Cache/StandardQueryCache.cs

@@ -115,14 +115,14 @@ public async Task<IList> GetAsync(QueryKey key, ICacheAssembler[] returnTypes, b
 						result.Add(await (TypeHelper.AssembleAsync((object[])cacheable[i], returnTypes, session, null, cancellationToken)).ConfigureAwait(false));
 					}
 				}
-				catch (UnresolvableObjectException)
+				catch (UnresolvableObjectException ex)
 				{
 					if (isNaturalKeyLookup)
 					{
 						//TODO: not really completely correct, since
 						//      the UnresolvableObjectException could occur while resolving
 						//      associations, leaving the PC in an inconsistent state
-						Log.Debug("could not reassemble cached result set");
+						Log.Debug(ex, "could not reassemble cached result set");
 						await (_queryCache.RemoveAsync(key, cancellationToken)).ConfigureAwait(false);
 						return null;
 					}

src/NHibernate/Async/Event/Default/DefaultMergeEventListener.cs

@@ -263,11 +263,11 @@ private async Task<object> MergeTransientEntityAsync(object entity, string entit
 
 				if (((EventCache)copyCache).IsOperatedOn(propertyFromEntity))
 				{
-					log.Info("property '{0}.{1}' from original entity is in copyCache and is in the process of being merged; {1} =[{2}]", copyEntry.EntityName, propertyName, propertyFromEntity);
+					log.Info(ex, "property '{0}.{1}' from original entity is in copyCache and is in the process of being merged; {1} =[{2}]", copyEntry.EntityName, propertyName, propertyFromEntity);
 				}
 				else
 				{
-					log.Info("property '{0}.{1}' from original entity is in copyCache and is not in the process of being merged; {1} =[{2}]", copyEntry.EntityName, propertyName, propertyFromEntity);
+					log.Info(ex, "property '{0}.{1}' from original entity is in copyCache and is not in the process of being merged; {1} =[{2}]", copyEntry.EntityName, propertyName, propertyFromEntity);
 				}
 
 				// continue...; we'll find out if it ends up not getting saved later

src/NHibernate/Async/Impl/SessionFactoryImpl.cs

@@ -11,6 +11,7 @@
 using System;
 using System.Collections.Concurrent;
 using System.Collections.Generic;
+using System.Collections.ObjectModel;
 using System.Data.Common;
 using System.Linq;
 using System.Runtime.Serialization;

src/NHibernate/Cache/StandardQueryCache.cs

@@ -133,14 +133,14 @@ public IList Get(QueryKey key, ICacheAssembler[] returnTypes, bool isNaturalKeyL
 						result.Add(TypeHelper.Assemble((object[])cacheable[i], returnTypes, session, null));
 					}
 				}
-				catch (UnresolvableObjectException)
+				catch (UnresolvableObjectException ex)
 				{
 					if (isNaturalKeyLookup)
 					{
 						//TODO: not really completely correct, since
 						//      the UnresolvableObjectException could occur while resolving
 						//      associations, leaving the PC in an inconsistent state
-						Log.Debug("could not reassemble cached result set");
+						Log.Debug(ex, "could not reassemble cached result set");
 						_queryCache.Remove(key);
 						return null;
 					}

src/NHibernate/Cfg/SettingsFactory.cs

@@ -62,9 +62,9 @@ public Settings BuildSettings(IDictionary<string, string> properties)
 			{
 				sqlExceptionConverter = SQLExceptionConverterFactory.BuildSQLExceptionConverter(dialect, properties);
 			}
-			catch (HibernateException)
+			catch (HibernateException he)
 			{
-				log.Warn("Error building SQLExceptionConverter; using minimal converter");
+				log.Warn(he, "Error building SQLExceptionConverter; using minimal converter");
 				sqlExceptionConverter = SQLExceptionConverterFactory.BuildMinimalSQLExceptionConverter();
 			}
 			settings.SqlExceptionConverter = sqlExceptionConverter;

src/NHibernate/Cfg/XmlHbmBinding/ClassBinder.cs

@@ -371,9 +371,9 @@ protected void BindAnyMeta(IAnyMapping anyMapping, Any model)
 					string entityName = GetClassName(metaValue.@class, mappings);
 					values[value] = entityName;
 				}
-				catch (InvalidCastException)
+				catch (InvalidCastException ice)
 				{
-					throw new MappingException("meta-type was not an IDiscriminatorType: " + metaType.Name);
+					throw new MappingException("meta-type was not an IDiscriminatorType: " + metaType.Name, ice);
 				}
 				catch (HibernateException he)
 				{

src/NHibernate/Collection/AbstractPersistentCollection.cs

@@ -61,9 +61,11 @@ public object Current
 						{
 							return enclosingInstance.operationQueue[position].AddedInstance;
 						}
-						catch (IndexOutOfRangeException)
+						catch (IndexOutOfRangeException ex)
 						{
-							throw new InvalidOperationException();
+							throw new InvalidOperationException(
+								"MoveNext as not been called or its last call has yielded false (meaning the enumerator is beyond the end of the enumeration).",
+								ex);
 						}
 					}
 				}

src/NHibernate/Engine/StatefulPersistenceContext.cs

@@ -1439,7 +1439,7 @@ void IDeserializationCallback.OnDeserialization(object sender)
 				}
 				catch (HibernateException he)
 				{
-					throw new InvalidOperationException(he.Message);
+					throw new InvalidOperationException(he.Message, he);
 				}
 			}
 
@@ -1467,7 +1467,7 @@ void IDeserializationCallback.OnDeserialization(object sender)
 				}
 				catch (MappingException me)
 				{
-					throw new InvalidOperationException(me.Message);
+					throw new InvalidOperationException(me.Message, me);
 				}
 			}
 		}

src/NHibernate/Event/Default/DefaultMergeEventListener.cs

@@ -266,11 +266,11 @@ private object MergeTransientEntity(object entity, string entityName, object req
 
 				if (((EventCache)copyCache).IsOperatedOn(propertyFromEntity))
 				{
-					log.Info("property '{0}.{1}' from original entity is in copyCache and is in the process of being merged; {1} =[{2}]", copyEntry.EntityName, propertyName, propertyFromEntity);
+					log.Info(ex, "property '{0}.{1}' from original entity is in copyCache and is in the process of being merged; {1} =[{2}]", copyEntry.EntityName, propertyName, propertyFromEntity);
 				}
 				else
 				{
-					log.Info("property '{0}.{1}' from original entity is in copyCache and is not in the process of being merged; {1} =[{2}]", copyEntry.EntityName, propertyName, propertyFromEntity);
+					log.Info(ex, "property '{0}.{1}' from original entity is in copyCache and is not in the process of being merged; {1} =[{2}]", copyEntry.EntityName, propertyName, propertyFromEntity);
 				}
 
 				// continue...; we'll find out if it ends up not getting saved later

src/NHibernate/Hql/Ast/ANTLR/HqlParser.cs

@@ -2,6 +2,7 @@
 using Antlr.Runtime;
 
 using NHibernate.Hql.Ast.ANTLR.Tree;
+using NHibernate.Util;
 using IToken = Antlr.Runtime.IToken;
 using RecognitionException = Antlr.Runtime.RecognitionException;
 
@@ -418,6 +419,7 @@ public IASTNode HandleIdentifierError(IToken token, RecognitionException ex)
 			}
 
 			// Otherwise, handle the error normally.
+			ReflectHelper.PreserveStackTrace(ex);
 			throw ex;
 		}
 	}

src/NHibernate/Hql/Ast/ANTLR/HqlSqlWalker.cs

@@ -1097,7 +1097,7 @@ private void HandleWithFragment(FromElement fromElement, IASTNode hqlWithNode)
 			}
 			catch (Exception e)
 			{
-				throw new SemanticException(e.Message);
+				throw new SemanticException(e.Message, e);
 			}
 		}
 	}

src/NHibernate/Hql/Ast/ANTLR/QuerySyntaxException.cs

@@ -10,6 +10,7 @@ public class QuerySyntaxException : QueryException
 	{
 		protected QuerySyntaxException() {}
 		public QuerySyntaxException(string message, string hql) : base(message, hql) {}
+		public QuerySyntaxException(string message, string hql, Exception inner) : base(message, hql, inner) {}
 
 		public QuerySyntaxException(string message) : base(message) {}
 		public QuerySyntaxException(string message, Exception inner) : base(message, inner) {}
@@ -24,9 +25,9 @@ public static QuerySyntaxException Convert(RecognitionException e)
 		public static QuerySyntaxException Convert(RecognitionException e, string hql)
 		{
 			string positionInfo = e.Line > 0 && e.CharPositionInLine > 0
-			                      	? " near line " + e.Line + ", column " + e.CharPositionInLine
-			                      	: "";
-			return new QuerySyntaxException(e.Message + positionInfo, hql);
+				? " near line " + e.Line + ", column " + e.CharPositionInLine
+				: "";
+			return new QuerySyntaxException(e.Message + positionInfo, hql, e);
 		}
 	}
-}
+}

src/NHibernate/Hql/Ast/ANTLR/QueryTranslatorImpl.cs

@@ -362,8 +362,6 @@ private void DoCompile(IDictionary<string, string> replacements, bool shallow, S
 			}
 			catch ( RecognitionException e ) 
 			{
-				// we do not actually propogate ANTLRExceptions as a cause, so
-				// log it here for diagnostic purposes
 				if ( log.IsInfoEnabled() ) 
 				{
 					log.Info(e, "converted antlr.RecognitionException");

src/NHibernate/Hql/Ast/ANTLR/Tree/FromElement.cs

@@ -544,11 +544,12 @@ public void HandlePropertyBeingDereferenced(IType propertySource, string propert
 						_dereferencedBySuperclassProperty = true;
 					}
 				}
-				catch (QueryException)
+				catch (QueryException ex)
 				{
 					// ignore it; the incoming property could not be found so we
 					// cannot be sure what to do here.  At the very least, the
 					// safest is to simply not apply any dereference toggling...
+					Log.Debug(ex, "Unable to find property {0}, no dereference will be handled for it.", propertyName);
 				}
 			}
 		}

src/NHibernate/Hql/Util/SessionFactoryHelper.cs

@@ -87,13 +87,13 @@ public IQueryableCollection GetCollectionPersister(String role)
 			{
 				return (IQueryableCollection)sfi.GetCollectionPersister(role);
 			}
-			catch (InvalidCastException)
+			catch (InvalidCastException ice)
 			{
-				throw new QueryException("collection is not queryable: " + role);
+				throw new QueryException("collection is not queryable: " + role, ice);
 			}
-			catch (Exception)
+			catch (Exception ex)
 			{
-				throw new QueryException("collection not found: " + role);
+				throw new QueryException("collection not found: " + role, ex);
 			}
 		}
 
@@ -186,15 +186,15 @@ public IQueryableCollection RequireQueryableCollection(String role)
 				}
 				return queryableCollection;
 			}
-			catch (InvalidCastException)
+			catch (InvalidCastException ice)
 			{
 				throw new QueryException(
-						"collection role is not queryable: " + role);
+						"collection role is not queryable: " + role, ice);
 			}
-			catch (Exception)
+			catch (Exception ex)
 			{
 				throw new QueryException("collection role not found: "
-						+ role);
+						+ role, ex);
 			}
 		}
 	}

src/NHibernate/Id/IdentifierGeneratorFactory.cs

@@ -309,9 +309,9 @@ public static System.Type GetIdentifierGeneratorClass(string strategy, Dialect.D
 					clazz = ReflectHelper.ClassForName(strategy);
 				}
 			}
-			catch (Exception)
+			catch (Exception ex)
 			{
-				throw new IdentifierGenerationException("Could not interpret id generator strategy: " + strategy);
+				throw new IdentifierGenerationException("Could not interpret id generator strategy: " + strategy, ex);
 			}
 			return clazz;
 		}

src/NHibernate/Impl/SessionFactoryImpl.cs

@@ -205,9 +205,10 @@ public SessionFactoryImpl(Configuration cfg, IMapping mapping, Settings settings
 					SchemaMetadataUpdater.QuoteTableAndColumns(cfg, Dialect);
 				}
 			}
-			catch (NotSupportedException)
+			catch (NotSupportedException ex)
 			{
-				// Ignore if the Dialect does not provide DataBaseSchema 
+				// Ignore if the Dialect does not provide DataBaseSchema
+				log.Warn(ex, "Dialect does not provide DataBaseSchema, but keywords import or auto quoting is enabled.");
 			}
 
 			#region Caches
@@ -339,9 +340,9 @@ public SessionFactoryImpl(Configuration cfg, IMapping mapping, Settings settings
 			{
 				uuid = (string)UuidGenerator.Generate(null, null);
 			}
-			catch (Exception)
+			catch (Exception ex)
 			{
-				throw new AssertionFailure("Could not generate UUID");
+				throw new AssertionFailure("Could not generate UUID", ex);
 			}
 
 			SessionFactoryObjectFactory.AddInstance(uuid, name, this, properties);

src/NHibernate/Mapping/Collection.cs

@@ -154,9 +154,9 @@ public object Comparer
 					{
 						comparer = Cfg.Environment.BytecodeProvider.ObjectsFactory.CreateInstance(ReflectHelper.ClassForName(ComparerClassName));
 					}
-					catch
+					catch (Exception ex)
 					{
-						throw new MappingException("Could not instantiate comparator class [" + ComparerClassName + "] for collection " + Role);
+						throw new MappingException("Could not instantiate comparator class [" + ComparerClassName + "] for collection " + Role, ex);
 					}
 				}
 				return comparer;

src/NHibernate/Mapping/PersistentClass.cs

@@ -890,9 +890,9 @@ private Property GetRecursiveProperty(string propertyPath, IEnumerable<Property>
 					}
 				}
 			}
-			catch (MappingException)
+			catch (MappingException ex)
 			{
-				throw new MappingException("property [" + propertyPath + "] not found on entity [" + EntityName + "]");
+				throw new MappingException("property [" + propertyPath + "] not found on entity [" + EntityName + "]", ex);
 			}
 
 			return property;

src/NHibernate/Persister/Entity/AbstractEntityPersister.cs

@@ -2206,14 +2206,14 @@ protected bool Check(int rows, object id, int tableNumber, IExpectation expectat
 			{
 				expectation.VerifyOutcomeNonBatched(rows, statement);
 			}
-			catch (StaleStateException)
+			catch (StaleStateException sse)
 			{
 				if (!IsNullableTable(tableNumber))
 				{
 					if (Factory.Statistics.IsStatisticsEnabled)
 						Factory.StatisticsImplementor.OptimisticFailure(EntityName);
 
-					throw new StaleObjectStateException(EntityName, id);
+					throw new StaleObjectStateException(EntityName, id, sse);
 				}
 			}
 			catch (TooManyRowsAffectedException ex)

src/NHibernate/QueryException.cs

@@ -46,6 +46,21 @@ public QueryException(string message, string queryString) : base(message)
 			this.queryString = queryString;
 		}
 
+		/// <summary>
+		/// Initializes a new instance of the <see cref="QueryException"/> class.
+		/// </summary>
+		/// <param name="message">The message that describes the error. </param>
+		/// <param name="queryString">The query that contains the error.</param>
+		/// <param name="innerException">
+		/// The exception that is the cause of the current exception. If the innerException parameter
+		/// is not a null reference, the current exception is raised in a catch block that handles
+		/// the inner exception.
+		/// </param>
+		public QueryException(string message, string queryString, Exception innerException) : base(message, innerException)
+		{
+			this.queryString = queryString;
+		}
+
 		/// <summary>
 		/// Initializes a new instance of the <see cref="QueryException"/> class.
 		/// </summary>

src/NHibernate/StaleObjectStateException.cs

@@ -23,7 +23,17 @@ public class StaleObjectStateException : StaleStateException
 		/// <param name="entityName">The EntityName that NHibernate was trying to update in the database.</param>
 		/// <param name="identifier">The identifier of the object that is stale.</param>
 		public StaleObjectStateException(string entityName, object identifier)
-			: base("Row was updated or deleted by another transaction (or unsaved-value mapping was incorrect)")
+			: this(entityName, identifier, null)
+		{
+		}
+		/// <summary>
+		/// Initializes a new instance of the <see cref="StaleObjectStateException"/> class.
+		/// </summary>
+		/// <param name="entityName">The EntityName that NHibernate was trying to update in the database.</param>
+		/// <param name="identifier">The identifier of the object that is stale.</param>
+		/// <param name="innerException">The original exception having triggered this exception.</param>
+		public StaleObjectStateException(string entityName, object identifier, Exception innerException)
+			: base("Row was updated or deleted by another transaction (or unsaved-value mapping was incorrect)", innerException)
 		{
 			this.entityName = entityName;
 			this.identifier = identifier;

src/NHibernate/StaleStateException.cs

@@ -9,10 +9,13 @@ public class StaleStateException : HibernateException
 		public StaleStateException(string message) : base(message)
 		{
 		}
+		public StaleStateException(string message, Exception innerException) : base(message, innerException)
+		{
+		}
 
 		protected StaleStateException(SerializationInfo info, StreamingContext context)
 			: base(info, context)
 		{
 		}
 	}
-}
+}