NH-3549 - BasicFormatter throws exceptions for certain types of data containing "signal words"

[nhibernate-bot]

Christian Zink created an issue — 4th October 2013, 11:47:55:

Similar to Issue NH-1992 the BasicFormatter throws an exception when Formatting a string like:

UPDATE Table Set Column = @p0; @p0 = 'Some data with an embedded quote in parentheses and signal word ('UPDATE') like this: (don't update)'

Instead of just fixing this specific issue, might it be possible to make the formatter or it's callers more robust. I would prefer an unformatted sql to an exception.

Full code:

new BasicFormatter().Format(@"UPDATE Table Set Column = @p0; @p0 = 'Some data with an embedded quote in parentheses and signal word ('UPDATE') like this: (don't update)'");

[haefele]

Phew, this is fun - running into exactly this issue right now.
For now I can work around it by disabling "FormatSql" - but I agree with the original creator - I would prefer it to be more robust.

If something failes, just log the message unformatted.

But what happens right now is that my application doesn't work seemingly random depending on what the user enters.
And just because NHibernate tries to format the SQL for logging purposes.

I would be up to create a PR for this - just gotta know how you would prefer to have it fixed.

As far as I understood the error happens in this row:
https://github.com/nhibernate/nhibernate-core/blob/master/src/NHibernate/AdoNet/Util/BasicFormatter.cs#L369

I could try and fix this specific case - but even then I'm not very confident that there are no other errors left behind in the BasicFormatter.

So I think best thing to do would be to make the caller SqlStatementLogger (https://github.com/nhibernate/nhibernate-core/blob/master/src/NHibernate/AdoNet/Util/SqlStatementLogger.cs#L48) more robust - when an exception happens to fall back to FormatStyle.None

[bahusoid]

I agree that exception shouldn't be thrown. But from what I see provided SQL in issue is not correct (as ' needs to be escaped inside string as '') - so it's unlikely your command will be executed anyway.
Also I suggest you to disable at least debug NHibernate logging in release version of your application - it's too verbose and might affect perf (and commands are logged only under Debug level).

[bahusoid]

when an exception happens to fall back to FormatStyle.None

I think it should be done right inside FormatProcess.Perform - original sql should be returned on any exception. But let's see what other members think.

[haefele]

I agree that exception shouldn't be thrown. But from what I see provided SQL in issue is not correct (as ' needs to be escaped inside string as '') - so it's unlikely your command will be executed anyway.

I have a different SQL generated from NHibernate - but in my case it actually can be executed correctly, it's just the BasicFormatter that's failing.

I think it should be done right inside FormatProcess.Perform - original sql should be returned on any exception. But let's see what other members think.

Sounds reasonable as well. I didn't look into the other formatter there is, maybe it is also affected.
That's why I thought about putting it into SqlStatementLogger to cover all cases.

[fredericDelaporte]

In FormatProcess.Perform, it would allow to still log the sql, just not formatted. In SqlStatementLogger, it would forbid it to crash the application just for logs, even if another failure is possible elsewhere in its logging logic.

I think both should be done.

[fredericDelaporte]

The invalid SQL appears to be generated by the statement logger itself. Test case:

using (new SqlLogSpy())
{
	var logger = new SqlStatementLogger(false, true);
	using (var cmd = new System.Data.SqlClient.SqlCommand("UPDATE Table Set Column = @p0;"))
	{
		cmd.Parameters.AddWithValue(
			"p0",
			"Some data with an embedded quote in parentheses and signal word ('UPDATE') like this: (don't update)");
		Assert.DoesNotThrow(() => logger.LogCommand(cmd, FormatStyle.Basic));
	}
}

The logger adds to the SQL to be logged the parameter value, without handling quote escaping.