Fix enums used as parameter for string columns

[csharper2010]

A special treatment of enums mapped on string columns is required, because all reasons for checking string lengths or streamlining the length of parameters of queries on string columns to the length of the column for SQL Server
are also valid for enums, since they are sometimes by convention used to model the possible values of a string column.

Fixes #2621.

[csharper2010]

Sorry for not seeing this... Adding the logic directly to the AbstractStringType is surely better.

One question: should we make it more explicit for Enums because the Convert.ToType is very generic. Might help in some cases but might also cover real bugs in an application.

So handle Enums explicitly like I did in the Dialect classes?
if (value is Enum) value = Enum.GetName(value.GetType(), value);

Or leave it as is?

[hazzik]

Or leave it as is?

I don't know.

[hazzik]

Why not using one of the following methods?

/* 1 */ query.SetParameter("kind", Kind.SomeKind, NHibernateUtil.Enum(typeof(Kind)));
/* 2 */ query.SetEnum("kind", Kind.SomeKind);
/* 3 */ query.SetParameter("kind", Kind.SomeKind.ToString());

[csharper2010]

Because it's an existing code base where I cannot check all calls to SetParameter.

I would say that the change is a very specific one that does not hurt.

[hazzik]

Why not just value = value.ToString()?

[gliljas]

If the objective is to solve a problem in an existing code base, why not just change the string type used, to one where this modification has been applied? That would require fixing the mapping, not all calls to SetParameter.

Using Enums as string constants is bad practice.

[fredericDelaporte]

I am not in favor of this change. It is too specific, and easy to manage on user side by deriving a custom type as suggested by Gunnar.

[hazzik]

@fredericDelaporte what about changes as in e8c8a3b (minus null check)?

[fredericDelaporte]

That sounds better to me, why not accepting this e8c8a3b (minus null check).

[hazzik]

@fredericDelaporte @gliljas please check. This change is consistent with how some other types handle parameters, for example:

nhibernate-core/src/NHibernate/Type/Int32Type.cs

Lines 59 to 62 in 7f60bc3

	public override void Set(DbCommand rs, object value, int index, ISessionImplementor session)
	{
	rs.Parameters[index].Value = Convert.ToInt32(value);
	}

[csharper2010]

@fredericDelaporte @gliljas please check. This change is consistent with how some other types handle parameters, for example:

nhibernate-core/src/NHibernate/Type/Int32Type.cs

Lines 59 to 62 in 7f60bc3

	public override void Set(DbCommand rs, object value, int index, ISessionImplementor session)
	{
	rs.Parameters[index].Value = Convert.ToInt32(value);
	}

Sorry for picking up that discussion that late but to be honest, I fear that the general approach using Convert.ToString might cover programming bugs of app developers. Convert.ToString(new object()) leads to "System.Object", the result of Convert.ToString(1.5m) is locale-dependent.

On the one hand I am interested in a solution but that might be a bit risky. That's why I originally had the enum handled specifically because it has a natural way of being expressed as string (admittedly there are the edge cases with integers not being represented as enum string so my solution with Enum.GetName might not be perfect, e.g. it leads to null for (FileAccess)0 whereas Convert.ToString((FileAccess)0) gives the string "0").

Maybe we should guard the Convert.ToString with a list of defined cases where conversion to string is safe in some way.

var stringValue = value as string;
if (stringValue == null && value != null)
{
    if (value is Enum || value is xxx || value is yyy)
        stringValue = Convert.ToString(value);
    else
        throw new ArgumentException();
}

[bahusoid]

I fear that the general approach using Convert.ToString might cover programming bugs of app developers. Convert.ToString(new object()) leads to "System.Object", the result of Convert.ToString(1.5m) is locale-dependent.

I totally agree with @csharper2010 on this. We should not silently convert parameters with incorrect types to string. I see it as a source of hard to find bugs.

If you think it's useful let's do it for enums only but let's not make it a general fix.

[csharper2010]

Hi again,

I would like to pick that up again...
Should I revert the change back to the original one with just handling Enums? Do I have a chance to get that landed? There will not be a risk of regression bugs if we make it small and there is also no risk of covering application bugs as with Convert.ToString().

Thanks

[hazzik]

Rebased and dropped all other commits.

[hazzik]

Reverted the 'fix' as I'm trying to understand what we are fixing here.

[hazzik]

Ok, this was dragging for a long time. We need to decide what to do. Just for context enum might not have a name (!) and Enum.GetName(...) will return null. It is the case for [Flags] enums and enums with out of bound values

Returns
String
A string containing the name of the enumerated constant in enumType whose value is value; or null if no such constant is found.

These unnamed enum values however, have a valid ToString() representation that can be parsed back.

using System;

Print(E.A | E.B);             // -> Name: ''; ToString: 'A, B'
Print(Enum.Parse<E>("A, B")); // -> Name: ''; ToString: 'A, B'

Print((E) 100);               // -> Name: ''; ToString: '100'
Print(Enum.Parse<E>("100"));  // -> Name: ''; ToString: '100'

void Print(E e) => Console.WriteLine($"Name: '{Enum.GetName(e.GetType(), e)}'; ToString: '{e.ToString()}'");

[Flags]
enum E
{
	A = 1 << 0,
	B = 1 << 1,
	C = 1 << 2
}

So, with the proposed solution, it is possible to have the value lost in transit, and this is NOT a desirable outcome.

I personally do not want to add a workaround for clearly invalid mapping. However, I'm open to accept the special treatment for the enums in a form of ToString(...) call if @nhibernate/core agrees:

if (value is Enum e)
      value = e.ToString(); // EDIT: removed CultureInfo.InvariantCulture parameter because method is obsolete