Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Custom type handlers for enums are ignored #259

Open
chilversc opened this issue Mar 25, 2015 · 55 comments · May be fixed by #458 or #1711
Open

Custom type handlers for enums are ignored #259

chilversc opened this issue Mar 25, 2015 · 55 comments · May be fixed by #458 or #1711
Labels
bug v3.0 Changes awaiting the next breaking release
Milestone

Comments

@chilversc
Copy link
Contributor

I have an enum that is serialized to the DB using a single letter code.
I tried adding a custom type handler for the enum to handle the conversion but dapper seems to ignore the custom type handler.

Given the example code I expected to see the values;
ID = 10, Type = B
ID = 1, Type = Foo

What I actually got was:
ID = 1, Type = 2
DataException, Error parsing column 1 (Type=F - String)


void Main()
{
    SqlMapper.AddTypeHandler(new ItemTypeHandler());
    using (var connection = new SqlConnection("Server=(local); Database=tempdb; Integrated Security=SSPI")) {
        connection.Open();
        connection.Query ("SELECT @ID AS ID, @Type AS Type", new Item {ID = 10, Type = ItemType.Bar}).Dump();
        connection.Query<Item> ("SELECT 1 AS ID, 'F' AS Type").Dump();
    }
}

class ItemTypeHandler : SqlMapper.TypeHandler<ItemType>
{
    public override ItemType Parse(object value)
    {
        var c = ((string) value) [0];
        switch (c) {
            case 'F': return ItemType.Foo;
            case 'B': return ItemType.Bar;
            default: throw new ArgumentOutOfRangeException();
        }
    }

    public override void SetValue(IDbDataParameter p, ItemType value)
    {
        p.DbType = DbType.AnsiStringFixedLength;
        p.Size = 1;

        if (value == ItemType.Foo)
            p.Value = "F";
        else if (value == ItemType.Bar)
            p.Value = "B";
        else
            throw new ArgumentOutOfRangeException();
    }
}

class Item
{
    public long ID { get; set; }
    public ItemType Type { get; set; }
}

enum ItemType
{
    Foo = 1,
    Bar = 2,
}
@wwarby
Copy link

wwarby commented Apr 2, 2015

I've just hit exactly the same problem. It seems like SqlMapper handles enums differently than every other type and tries to map them with Enum.Parse even if a custom type handler is specified. I'm just about to delve into the source and see if I can see an easy fix - I'll report back if I find anything useful.

@wwarby
Copy link

wwarby commented Apr 2, 2015

Urgh. I knew Dapper was optimised for performance but I didn't realise it literally emitted IL op codes to do value conversions. No chance I'm ever going to get to make enough sense of the source to offer up my own solution to this one so I'm hoping the devs will accept this as an enhancement.

@chilversc
Copy link
Contributor Author

I don't thing you need to touch the IL sections. It's mostly a case of changing the order of the type checks to look for a custom handler first. At the moment it checks if it's an enum before it checks for custom type handlers and thus never discovers the custom type handler.

My only concern is that I'm not sure what impact that will have on the rest of the system since similar type checks are in other places.

Backwards compatibility shouldn't be a concern since it has never worked in the past I doubt anyone will have custom handlers for primitive types such as enums.

@mgravell
Copy link
Member

Enums have quite a few nuances and special cases. I'm sympathetic to what
you want to do here, but it isn't trivial - I'll need to have a look at it
with a clear head.

On 10 April 2015 at 20:11, Chris Chilvers [email protected] wrote:

I don't thing you need to touch the IL sections. It's mostly a case of
changing the order of the type checks to look for a custom handler first.
At the moment it checks if it's an enum before it checks for custom type
handlers and thus never discovers the custom type handler.

My only concern is that I'm not sure what impact that will have on the
rest of the system since similar type checks are in other places.

Backwards compatibility shouldn't be a concern since it has never worked
in the past I doubt anyone will have custom handlers for primitive types
such as enums.


Reply to this email directly or view it on GitHub
#259 (comment)
.

Regards,

Marc

@vdaron
Copy link

vdaron commented May 7, 2015

I would like to be able the same for the generic System.Enum type to change the serialization behaviour of all Enums (I'm using attributes to specify the letter used to serialize an Enum)

First check for a

    SqlMapper.TypeHandler<ItemType>

then for a

    SqlMapper.TypeHandler<Enum> 

(the default Dapper behaviour could be specified that way too)
Thanks for your interest on this feature @mgravell

@otac0n
Copy link
Contributor

otac0n commented May 11, 2015

@mgravell I worked on this a bit yesterday, and came up with this:

otac0n@817fa95

It works for everything I've tested, but I haven't written any unit tests for it.

I was testing it with this Type Handler:

public class StringEnumTypeHandler<T> : SqlMapper.TypeHandler<T> where T : struct, IConvertible
{
    static StringEnumTypeHandler()
    {
        if (!typeof(T).IsEnum)
        {
            throw new ArgumentException("T must be an enumeration type");
        }
    }

    public override T Parse(object value)
    {
        return (T)Enum.Parse(typeof(T), Convert.ToString(value));
    }

    public override void SetValue(IDbDataParameter parameter, T value)
    {
        parameter.Value = value.ToString();
        parameter.DbType = DbType.AnsiString;
    }
}

This doesn't support @vdaron's use case, however.

@ShamsulAmry
Copy link

@vdaron, thanks for the reference. Didn't make an effort to check that this issue was already logged, my bad.

My use case is exactly the same as what @chilversc tried to achieve - single letter code in DB to represent an enum value. But it does not support @vdaron's "fall back" behavior request though.

@mgravell, my commit in issue #286 included relevant test code and "all green" tests.

@roji
Copy link

roji commented Aug 25, 2015

+1

@drusellers
Copy link

I've run into this as well. :(

@NickCraver NickCraver added the bug label Oct 11, 2015
@kbilsted
Copy link
Contributor

kbilsted commented Jan 5, 2016

What is the status of this issue?

@peterthomastlc
Copy link

peterthomastlc commented Sep 3, 2016

We submitted a pull request to fix this two years ago, but it was never accepted.
A one-line fix is all it takes:
In public static DbType LookupDbType, at the top of the function, change
handler = null
to
typeHandlers.TryGetValue(type, out handler);

Still it's not fixed, and no feedback why the pull request was rejected :-(
We'd love to be using the NuGet version of Dapper, but we need this fix because it's stopping TypeHandlers being used for any built-in type (and enums).

@kbilsted
Copy link
Contributor

kbilsted commented Sep 3, 2016

:+1

@wmate
Copy link

wmate commented Sep 28, 2016

+1
This. Is. The. Feature.

@mgravell
Copy link
Member

Ultimately, my reservation here is that it was not intended or designed
that type-handlers should subvert the expected behavior of primitives; I
haven't thought though the implication of every edge case of that; and I
haven't checked what the impact of that is on the IL emitting code - does
it still do "the right thing" (whatever the right thing is, which isn't
necessarily obvious), in all cases?

I think the elegance and convenience of a 1-line change is all well and
good, but it isn't necessarily that simple.

Have I been lax and tardy on bottoming out what that impact is? yes. that
is valid criticism, and I take it squarely on the chin. But when faced with
complex and nuanced unknowns, I tend to default towards the conservative.

On 28 September 2016 at 20:08, wmate [email protected] wrote:

+1
This. Is. The. Feature.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#259 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AABDsPNsoeweGYdz7U35qT_-Bm1txKMWks5qursUgaJpZM4D0geM
.

Regards,

Marc

@kbilsted
Copy link
Contributor

Wouldn't integration tests take away some those fears?

@chilversc
Copy link
Contributor Author

@kbilsted my main concern would be the external 3rd-party code that could get an unexpected change in behaviour, i.e. a type handler that was previously doing nothing and had been forgotten about could start running.

As such this change would have to be a major release under semantic versioning.

There were further comments on the pull request #458 (which probably also needs the breaking change label).

At the moment, the simplest way is to wrap the enum in a simple struct with implicit conversions, then you can add a type handler on the wrapping struct.

@kbilsted
Copy link
Contributor

@chilversc I don't think bumping the major version is an argument for not implementing the support.

More importantly, I did not realize there was a simple solution that did not require code changes in dapper. Would you kindly document the suggested approach in the readme.md file ?

@NickCraver NickCraver added the v3.0 Changes awaiting the next breaking release label Jan 28, 2017
@NickCraver NickCraver modified the milestone: v2.0 Jan 30, 2017
@jeremycook
Copy link

It would be nice to be able to opt-into support for the behavior requested by this issue instead of having to wait for v2.0.

Maximys pushed a commit to Maximys/Dapper that referenced this issue Jun 28, 2018
@davidvmckay
Copy link

Hello Dapper community; I encountered this same problem as well, dealing with SAGE accounting and LoadMaster transportation databases. I wish we could have the linked PR merged; writing types with implicit string conversions is tedious, and I've had to defend this bizarre enum alternative in code reviews on multiple dev teams over the last few years. I have a fork of Dapper I use in personal projects due to things like this Enum TypeHandler support that are hard to customize using the Dapper API. Then I can merge any PR I like into it. But I can't typically use my private fork in production because it doesn't have the official backing of the Dapper community.

What can I do to satisfy the apprehensions of the Dapper leadership, so we can merge the solution?

@chilversc
Copy link
Contributor Author

Could hide it behind a feature flag, or add a new interface such as SqlEnumConverter so that unless you explicitly use the new features it won't affect existing code. Only donwside is a slightly more confusing API as you have to explain the flag / type and have people trying to use the existing interface by mistake.

@ghost
Copy link

ghost commented Jun 21, 2020

Is there any chance this will be changed? If not then, unfortunately, I will have to look for an alternative because at the moment I have not found it possible to handle PostgreSQL Enum Types using Dapper in any form or workaround?

More importanly, I should add that I very much do appreciate the hard work that has gone into this library and am a bit disappointed I cannot use it, so this is not a criticism, just need to know if its on the timeline at all as its a very old issue.

@AlmostRemember
Copy link

I'd like to see this fixed. It's a frustrating and baffling limitation. Let me use my own handler please.

@banderberg
Copy link

Good job, team!

@phillip-haydon
Copy link
Contributor

6 years and no fix :/

@joost00719
Copy link

Why hasn't this been fixed yet? I've spend hours trying to figure out why this is not working only to find out that this has been an issue for over six years now.

@MaxShoshin MaxShoshin linked a pull request Oct 1, 2021 that will close this issue
@JeroenHo
Copy link

Still no progress on this issue? That's just to bad...

@gandhis1
Copy link

It's frustrating that such a common issue with such a trivial fix is not being considered...surely by now we have had enough time to rationalize the implications of this

@SimonCull
Copy link

@mgravell Any update since your last message? As others have said, this fix is trivial, there have been multiple PRs with unit tests provided. In your last update you mentioned that you didn't think type handlers shouldn't subvert the "expected behaviour of primitives" however given than System.Text.Json allows Enums to serialize as ints or strings I think we need to take a look at those expectations.

Those of us working with legacy databases would hugely appreciate some feedback here as to whether Dapper is ever going to be suitable for our needs, or whether we need to look for other solutions.

@phillip-haydon
Copy link
Contributor

@mgravell this is now 7 years old. Can the Dapper team either close the 453454 issues as 'wont fix' or make a decision on fixing this?

@NickCraver
Copy link
Member

@phillip-haydon Our decision on fixing this has been very consistent: we'd like to change this, if safe, in a breaking release. At the very least, it's a behavior change. At the most it could cause collateral damage to things and/or need an option (v3 has a plan for Options, but again: time). Overall a breaking major version is a very high bar and has a lot of work to do - and it's not just the work but follow-up on any issues, that's far worst than a known state. A change good for some can harm others. We're trying to button up StackExchange.Redis to current levels then give Dapper more maintenance love for things like this. It's a very non-trivial amount of work to do that release (minimum dedicated weeks) and this is 100% personal time so we have to choose ordering.

The world has shifted here in that Dapper emitting IL to do deserialization isn't the path we'd ideally take at this point. It's not friendly to some restricted and many high performance scenarios (e.g. AOT). A vNext, when we can find the time, is a huge undertaking - until then we're holding off on breaking changes. This is open source, if you want to branch and change the behavior for your scenario and any deal with any behavioral differences or unexpected things that arise: go for it!

@ghost
Copy link

ghost commented May 16, 2022

fits the situation
image

@heku
Copy link

heku commented May 19, 2022

Is there still no solution for this? It's 2022, 7 yeays since the issue opened, unbelievable.

@nyan-cat
Copy link

Is there still no solution for this? It's 2022, 7 yeays since the issue opened, unbelievable.

Hello from 2023 eve. Yet another frustrating enum issue opened for 8 years now. Just can't believe I've bumped into such an issue with such hyped ORM 😡

@jaredostermann
Copy link

Just ran into this problem when trying to map enums to PostgreSQL enums using npgsql. As a long-time dapper user, I really hope this gets addressed in v3.0

@CurlyBraceTT
Copy link

+1 for fixing this.

@kotvytskyi
Copy link

Please, fix the issue!

@asrichesson
Copy link

It's 2 am, and after scratching my head for several hours on why my type handler wasn't working with Enums, I find that it's been an open issue for the past 7 years. yay.

@BenjaBobs
Copy link

We ended up switching all our dapper code with linq2db as it supports custom handling for nearly everything. As a side bonus things become more typesafe, but it's a slightly different way to query.

@phillip-haydon
Copy link
Contributor

I'm slowly beginning to be convinced that Dapper is a dead project. Which is a shame cos I really love dapper.

@mgravell
Copy link
Member

mgravell commented Jul 7, 2023

The reports of my death have been greatly exaggerated

Hesitance to look at a specific feature doesn't mean we're not here; it means there might be more to it than you think, and it might not be as simple as one-fix-suits-all.

@agentGTAM
Copy link

@mgravell Dude, it's 2024, how many more years will this take? I get that you hesitate, that you are wary, cautious and all of that good stuff but at some point it becomes something else.

@bkothe
Copy link

bkothe commented Jan 18, 2024

I, too, have just spent hours trying to make this work and come to find it's been an issue for near 9 years. Any movement on this?

@thefex
Copy link

thefex commented Mar 23, 2024

I use ugly but generic workaround here as follows, just make sure you bind DB model into "MessageType" (property):

Data model class:

public ChatMessageType? ChatMessageType { get; set; }
// HACK, WORKAROUND -> Dapper Enum Limitations ... https://github.com/DapperLib/Dapper/issues/259 
[JsonIgnore] 
public string MessageType
{
    get => ChatMessageType.GetDescription();
    set => ChatMessageType = EnumExtensions<ChatMessageType>.BuildUsingDescription(value);
} 
 internal static class EnumExtensions
  {
      private static readonly ConcurrentDictionary<Enum, string> CachedDescriptionMap = new ConcurrentDictionary<Enum, string>();
      public static string GetDescription(this Enum value)
      {
          // Reflection will be called only N * Enum Values Count times - important as this might be called pretty often via Dapper/SQL mapping...
          if (CachedDescriptionMap.ContainsKey(value))
              return CachedDescriptionMap[value];
      
          Type type = value.GetType();
          string name = Enum.GetName(type, value);
          if (name != null)
          {
              FieldInfo field = type.GetField(name);
              if (field != null)
              {
                  if (Attribute.GetCustomAttribute(field, typeof(DescriptionAttribute)) is DescriptionAttribute attr)
                  {
                      CachedDescriptionMap.TryAdd(value, attr.Description);
                      return attr.Description;
                  }
              }
          }
      
          throw new InvalidOperationException("There is no description for " + value.ToString() + "," + type); 
      }

public static class EnumExtensions<TEnum> where TEnum : struct, Enum
{
    private static readonly ConcurrentDictionary<string, TEnum?> CachedDescriptionToEnumMap = new ConcurrentDictionary<string, TEnum?>();
    public static TEnum BuildUsingDescription(string enumDescription)
    {
        // Reflection will be called only N * Enum Values Count times - important as this might be called pretty often via Dapper/SQL mapping...
        if (CachedDescriptionToEnumMap.ContainsKey(enumDescription))
        {
            var cachedEnum = CachedDescriptionToEnumMap[enumDescription];
        
            // even if throws exception... make sure it's cached
            if (!cachedEnum.HasValue)
                throw new InvalidOperationException(enumDescription + " - " + typeof(TEnum) +
                                                    " - this enum does not have following value: " + enumDescription);
            else
                return cachedEnum.Value;
        }
    
        Type enumType = typeof(TEnum);

        var enumFields = enumType.GetFields(BindingFlags.Static | BindingFlags.Public);
        var enumDescriptionValueMap = enumFields.ToDictionary(x => x.GetCustomAttribute<DescriptionAttribute>()?.Description,
            x => x.Name);

        if (!enumDescriptionValueMap.Keys.Contains(enumDescription))
        {
            CachedDescriptionToEnumMap.TryAdd(enumDescription, null);
            throw new InvalidOperationException(enumDescription + " - " + typeof(TEnum) + " - this enum does not have following value: " + enumDescription);
        }

        var enumValueName = enumDescriptionValueMap[enumDescription];

        try
        {
            var parsedEnum = (TEnum)Enum.Parse(typeof(TEnum), enumValueName);
            CachedDescriptionToEnumMap.TryAdd(enumDescription, parsedEnum);
            return parsedEnum;
        }
        catch (Exception)
        {
            CachedDescriptionToEnumMap.TryAdd(enumDescription, null);
            throw;
        }
    } 
}

sample enum:

public enum ChatMessageType
{
    [Description("my_message")]
    MyMessage,
    [Description("other_message")]
    Other,
    [Description("system_message")]
    System
}

@phillip-haydon
Copy link
Contributor

Yearly update:

Project still appears dead. Issue is now 9 years old with no resolution in sight.

@djimmo
Copy link

djimmo commented Aug 24, 2024

Maybe this helps. I combined what I found online into something that works

https://gist.github.com/djimmo/b54a1643f15bd4e54303992ddce4a968

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug v3.0 Changes awaiting the next breaking release
Projects
None yet