Adjust string parameter sizes for MSSQL

[gliljas]

Fix for #1300. Replaces #480

Additional logic could be applied in the length calculation, to prevent query plan cache misses.

[fredericDelaporte]

This looks great.

As I understand it, a Linq query using something like x.MappedAs(NHibernate.Type.TypeFactory.Basic("AnsiString(200)")) would also honor the length with your change, provided the value is not longer than specified length.

[fredericDelaporte]

For new tests, NUnit constraint model is favored other its classic model, see NUnit two models.

So it should be:

Assert.That(Driver.LastCommandParameters.First().SqlDbType, Is.EqualTo(SqlDbType.NVarChar));
Assert.That(Driver.LastCommandParameters.First().Size, Is.EqualTo(3));
Assert.That(Driver.LastCommandParameters.Last().SqlDbType, Is.EqualTo(SqlDbType.VarChar));
Assert.That(Driver.LastCommandParameters.Last().Size, Is.EqualTo(3));

Or maybe even:

Assert.That(
    Driver.LastCommandParameters.First(),
    Has.Property("SqlDbType").EqualTo(SqlDbType.NVarChar).And.Property("Size").EqualTo(3));
Assert.That(
    Driver.LastCommandParameters.Last(),
    Has.Property("SqlDbType").EqualTo(SqlDbType.VarChar).And.Property("Size").EqualTo(3));

[gliljas]

True. I reused my old tests from the previous PR. Will fix.

[fredericDelaporte]

Maybe move implemented interface declarations out of the #if, in order to avoid duplicating them and only have in the #if what changes.

#if NETFX
    : DriverBase,
#else
    : ReflectionBasedDriver,
#endif
        IEmbeddedBatcherFactoryProvider, IParameterAdjuster

[gliljas]

Good idea.

[gliljas]

I'll add some more tests (e.g using MappedAs, edge lengths etc.)

[fredericDelaporte]

Strange spacing glitch here

[gliljas]

Huh? Strange indeed!

[gliljas]

Hmmm...what do you say @fredericDelaporte? What behavior should we aim for when a value exceeds the max size. Going above e.g. MaxSizeForLengthLimitedString is mostly not a problem (but it may be for SQL Server 2000)

[fredericDelaporte]

About SQL Server 2000, well, its support has completely ended in 2013. So, I am for not caring about an edge case for this product. (Limited string parameters which value size happens to get above the the max limited string size.)

So, when a value exceeds the max mapped size, I think setting the parameter size to the value size is a good choice.
Of course, it will still cause plan cache misses if the application use many value lengths above mapped size. But going to MaxSizeForLengthLimitedString instead of the value size looks overkill to me. I think it is an edge case which does not deserve a fix. And moreover at least for Linq, the developer can handle it by specifying its own length with MappedAs.

In fact I also question the validity of setting the size to MaxSizeForClob for a size above MaxSizeForLengthLimitedString: normally, the parameter size for nvarchar(max)/varchar(max)/ntext/text should be set to -1. Unfortunately, in the documentation, I can only find an example of this, not an explicit statement.
(Moreover, see here, Sql Server CE tends to allocate the memory for the specified parameter size. Fortunately it does no more use the Sql Server driver logic currently.)
This blob size subject is unrelated to the PR here, so it should likely not be handled here.

Side note: the async tests need a regen. This is why they fail currently.

[gliljas]

I think I'm done with this one. Can rebase-squash if so desired.

[hazzik]

Could the tests be enabled for all dialects?

[fredericDelaporte]

Other dialect drivers do not set parameter sizes, so the tests would fail. Unless the size asserts are changed for being done only when the driver is the sql server one.

[hazzik]

Other dialect drivers do not set parameter sizes,

At least ODBC driver does so.

[fredericDelaporte]

No, ODBC does no more specify string parameter sizes since NH-4083 (v5.0, #703, 3be6668).

[fredericDelaporte]

Why is it internal? An external driver implementor may need it.

[gliljas]

It was a prototype, so I kept it from "the public". I wouldn't mind making it public.

[fredericDelaporte]

Since it was conflicting with the latest merge on master, I have rebased it. And I have added a commit for switching the interface to public.

[gliljas]

Thanks!