Patches to improve some error messages and logging

[Evgeny Potashnik]

Hi,

I'd like to contribute a couple of minor patches. The first contains the one-character fix to the Postgres NorthwindCustom.cs which reduces the number of failing test from >200 to 85 and some minor error-handling improvements that I made while investigating while tests are failing (so you might get a meaningful error message instead of NullReferenceException). The second fixes two problems with the logging which caused the logged expression tree to be incomplete - a NullReferenceException and not flushing the TextWriter at the end.

Regards,

Evgeny Potashnik

[Federico Di Gregorio]

Hi Evgeny,

thank you very much for the patch. In your next contribution would be
much better to split the vendor-specific part (PostgreSQL) from the
rest: that will make the review work much easier.

I just applied the PostgreSQL part of the patch but the rest will have
to wait for jonp.

federico

--
Federico Di Gregorio http://people.initd.org/fog
Debian GNU/Linux Developer f...@debian.org
INIT.D Developer f...@initd.org
One key. One input. One enter. All right. -- An american consultant
(then the system crashed and took down the *entire* network)

[Jonathan Pryor]

One question about the EP1-error-handling.patch: Why do some classes use
_PascalCase (e.g. DbLinq.Mssql.Example) while others use _camelCase
(PosgtreSQL, SQLite).

Shouldn't these be _consistent_? :-) (Or are we not using DbMetal to
generate all of these types? For that matter, iirc we don't even have a
SchemaLoader implementation for Microsoft SQL Server, meaning we can't
even point our DbMetal at SQL Server to generate sources.)

For that matter, we seem to have broken DbMetal. This used to work
(when run from the tests directory):

..\build.dbg\DbMetal.exe /namespace:nwind/code:Northwind.cs
/pluralize "/conn:Data Source=Northwind.db3" /provider:Sqlite

I know this because I wrote it down in the r1075 commit message. :-)

Now, however, it fails:

DbMetal failed:System.Collections.Generic.KeyNotFoundException: The given key was not present in the dictionary.
at System.ThrowHelper.ThrowKeyNotFoundException()
at System.Collections.Generic.Dictionary`2.get_Item(TKey key)
at DbLinq.Vendor.Implementation.SchemaLoader.LoadForeignKey(Database schema, Table table, String columnName, String tableName, String tableSchema, String ref
erencedColumnName, String referencedTableName, String referencedTableSchema, String constraintName, NameFormat nameFormat, Names names) in Z:\Development\mono-HEAD\dblinq2007\src\DbLinq\Vendor\Implementation\SchemaLoader.ForeignKey.cs:line 0
at DbLinq.Sqlite.SqliteSchemaLoader.LoadConstraints(Database schema, SchemaName schemaName, IDbConnection conn, NameFormat nameFormat, Names names) in Z:\Development\mono-HEAD\dblinq2007\src\DbLinq.Sqlite\SqliteSchemaLoader.cs:line 84
at DbLinq.Vendor.Implementation.SchemaLoader.Load(String databaseName, INameAliases nameAliases, NameFormat nameFormat, Boolean loadStoredProcedures, String contextNamespace, String entityNamespace) in Z:\Development\mono-HEAD\dblinq2007\src\DbLinq\Vendor\Implementation\SchemaLoader.cs:line 123
at DbMetal.Generator.Implementation.Processor.ReadSchema(Parameters parameters, ISchemaLoader& schemaLoader) in Z:\Development\mono-HEAD\dblinq2007\src\DbMetal\Generator\Implementation\Processor.cs:line 218
at DbMetal.Generator.Implementation.Processor.ProcessSchema(Parameters parameters) in Z:\Development\mono-HEAD\dblinq2007\src\DbMetal\Generator\Implementation\Processor.cs:line 112

Something broke, somewhere. :-(

- Jon

[Jonathan Pryor]

It looks good, but three issues:

1. You're not using Evolution, are you? I can't apply these patches
(and, oddly, patch(1) reports that I'm out of disk space when it tries
to apply them -- and I have 11GB free!). You might try attaching a .zip
file with the patches (I could be so lucky that would work, right?).

2. Several of these changes appear to change behavior (e.g.
src/DbLinq/Data/Linq/Mapping/LambdaMetaAccessor.cs returns additional
data), but I don't see any changes to the tests.

Are there tests for these non-logging changes?

For that matter, do all the tests still pass? (I'd check myself, but
(1) makes this difficult w/o manually applying all the changes.)

3. Please pay attention to whitespace (changes, what it's made of,
etc.).

Comments on the patches below...

> Index: src/DbLinq/Data/Linq/Mapping/AttributedMetaModel.cs
> ===================================================================
> --- src/DbLinq/Data/Linq/Mapping/AttributedMetaModel.cs (revision 1239)
> +++ src/DbLinq/Data/Linq/Mapping/AttributedMetaModel.cs (working copy)
> @@ -204,9 +204,13 @@
>
> MetaTable metaTable;
> if (_Tables.TryGetValue(tableType, out metaTable))
> - yield return metaTable;
> + yield return metaTable;

Whitespace only change. Sure, this removes tabs with spaces, but it
also increases the patch size and increases my mental overhead. :-)

> else
> - yield return AddTableType(tableType);
> + {
> + metaTable = AddTableType(tableType);
> + if (metaTable != null)
> + yield return metaTable;
> + }

This looks like a semantic change that I worry about. It used to
(potentially) return null. Now it won't. Does this still pass all
tests? Do we need a new test to check this behavior?

> Index: src/DbLinq/Data/Linq/Mapping/LambdaMetaAccessor.cs
> ===================================================================
> --- src/DbLinq/Data/Linq/Mapping/LambdaMetaAccessor.cs (revision 1239)
> +++ src/DbLinq/Data/Linq/Mapping/LambdaMetaAccessor.cs (working copy)
> @@ -14,18 +14,28 @@
> //This will go away with C# 4.0 ActionExpression
> static Delegate MakeSetter(MemberInfo member, Type memberType, Type declaringType)
> {
> + if (member == null)
> + throw new ArgumentNullException("member");
> +
> Type delegateType = typeof(Action<,>).MakeGenericType(declaringType, memberType);
>
> switch (member.MemberType)
> {
> case MemberTypes.Property: {
> - MethodInfo method = ((PropertyInfo)member).GetSetMethod();
> + var property = (PropertyInfo)member;

'property' seems improperly indented here. (The line contains mixed
tabs and spaces; please use only spaces.)

> + MethodInfo method = property.GetSetMethod();
> if (method != null)
> return Delegate.CreateDelegate(delegateType, method);
> var ca = member.GetCustomAttributes(typeof(ColumnAttribute), true)[0] as ColumnAttribute;
> member = declaringType.GetField(ca.Storage,
> BindingFlags.NonPublic | BindingFlags.Public | BindingFlags.Instance | BindingFlags.Static);
> - goto case MemberTypes.Field;
> + if (member == null)
> + {
> + throw new ApplicationException(string.Format("Failed to find field '{0}' on type '{1}', which was"
> + + " specified as the Storage field of property '{2}'.", ca.Storage, declaringType.FullName, property.Name));
> + }
> +
> + goto case MemberTypes.Field;

Ditto the indentation comments here.

> Index: src/DbLinq/Data/Linq/Sugar/Implementation/SqlBuilder.cs
> ===================================================================
> --- src/DbLinq/Data/Linq/Sugar/Implementation/SqlBuilder.cs (revision 1239)
> +++ src/DbLinq/Data/Linq/Sugar/Implementation/SqlBuilder.cs (working copy)
> @@ -164,11 +164,14 @@
> var literalOperands = new List<SqlStatement>();
> foreach (var operand in operands)
> {
> - var operandPrecedence = ExpressionQualifier.GetPrecedence(operand);
> - var literalOperand = BuildExpression(operand, queryContext);
> - if (operandPrecedence > currentPrecedence)
> - literalOperand = sqlProvider.GetParenthesis(literalOperand);
> - literalOperands.Add(literalOperand);
> + if (operand != null)
> + {
> + var operandPrecedence = ExpressionQualifier.GetPrecedence(operand);
> + var literalOperand = BuildExpression(operand, queryContext);
> + if (operandPrecedence > currentPrecedence)
> + literalOperand = sqlProvider.GetParenthesis(literalOperand);
> + literalOperands.Add(literalOperand);
> + }

Instead of re-indenting this entire block, I would instead prefer

foreach (var operand in operands)
{
if (operand == null)
continue;
// as before
}

It makes the patch smaller (nice), and it also minimizes indentation
(doubly nice, unless you like to see blocks starting at column 80...).

I also wonder if this is the right place to fix things; changing the
calling code so that it doesn't return 'null's may be preferable. (I
know when making other fixes that's the route I took, removing 'null's.)

> Index: src/DbLinq/Util/TextWriterExtension.cs
> ===================================================================
> --- src/DbLinq/Util/TextWriterExtension.cs (revision 1239)
> +++ src/DbLinq/Util/TextWriterExtension.cs (working copy)
> @@ -50,6 +50,7 @@
> // we just ignore NREs
> catch (NullReferenceException)
> {
> + // TODO: fix the NREs properly and remove this, so expression trees in the log are complete.
> }
> }
>
> @@ -166,7 +167,7 @@
> var lines = new List<string>
> {
> WriteHeader(expression, header, depth++),
> - WriteLiteral(expression.Constructor.Name, "Ctor", depth)
> + WriteLiteral(System.Reflection.ConstructorInfo.ConstructorName, "Ctor", depth)
> };

I'm not sure what this does, exactly. Can you further explain what the
intent is?

Thanks,
- Jon

[Evgeny Potashnik]

Hi Jon,

Thanks for reviewing my patch. No, I'm using GMail, not Evolution, but I've attached the updated patch zipped. The whitespace has been fixed and I've removed the "if (operand == null)" check, since
I don't know enough to figure out the best place to fix it.

The only changes in behaviour that user code would notice are:
1) To flush the log at the end.
2) To write an extra line ("-- Executing database command:"). I added that so I could easily find the SQL statements in the log and separate them from the expression trees.
3) To fix the NRE in TextWriterExtension, which was preventing part of the expression tree from being written. expression.Constructor is sometimes null. We could check for null there, but ConstructorInfo.Name always returns ConstructorInfo.ConstructorName (".ctor") anyway, so I just replaced it with that.

The other changes only improve error messages. I made while trying to fix some of the failing Postgres tests. While I didn't actually fix any tests I thought I'd contribute these changes anyway so they might save someone else a little debugging time. So there are tests that exercise this code, but of course the tests do not specifically check the exception message since they expect no exception at all. With regard to GetTables() no longer returning null, if you look at the way it's used the caller immediately throws NullReferenceException if any nulls are returned.

The tests for DbLinq core, SQLite and SQL Server pass in the Release build. 85 Postgres tests fail, which is the same number as before my changes. I'm unable to test the other DBs.

Regards,

Evgeny

2009/10/24 Jonathan Pryor <jonp...@vt.edu>

[Jonathan Pryor]

On Mon, 2009-10-26 at 09:53 +1100, Evgeny Potashnik wrote:
> Thanks for reviewing my patch.

Sorry for the delay.

The short version is that you may commit this patch.

The long version is that I'm learning that patch(1) hates my guts.

Specifically, patch(1) under Linux often (always so far!) rejects
patches because the "Index:", @@...@@, ^+++, and ^--- lines end with \r
\n, and the \r causes an immediate rejection. I can manually fix this,
but it's annoying.

Alternatively, I can use patch(1) under Cygwin, but that results in
changing the EOL character on the entire file. Result: instead of small
changes, the entire file is changed. FAIL.

I'm wondering what people who regularly deal with open-source software
under Windows do (aside from avoid all DOS-formatted files).

That aside, the patch looks fine, and if you say it doesn't regress any
tests then that's fine with me. :-)

Thanks,

- Jon

[Evgeny Potashnik]

Thanks. I can't actually commit it myself as I don't have write access to Subversion. I've attached a version of the file with \n newlines for you. :)

Regards,

Evgeny

2009/11/3 Jonathan Pryor <jonp...@vt.edu>