Fixes #2621

[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.

[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.

[hazzik]

@fredericDelaporte what about changes as in e8c8a3b07e3a06c545c7bdf43472877220a04927 (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:

https://github.com/nhibernate/nhibernate-core/blob/7f60bc3a875a721d6ad4b76b530f2447da79454f/src/NHibernate/Type/Int32Type.cs#L59-L62

[csharper2010]

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

https://github.com/nhibernate/nhibernate-core/blob/7f60bc3a875a721d6ad4b76b530f2447da79454f/src/NHibernate/Type/Int32Type.cs#L59-L62

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();
}

[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

[hazzik]

PS: should have re-read the whole thread before posting my reply :-(