DapperLib / Dapper

Dapper - a simple object mapper for .Net
https://www.learndapper.com/
Other
17.51k stars 3.68k forks source link

Custom type handlers for enums are ignored #259

Open chilversc opened 9 years ago

chilversc commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 notifications@github.com 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 https://github.com/StackExchange/dapper-dot-net/issues/259#issuecomment-91655115 .

Regards,

Marc

vdaron commented 9 years ago

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 commented 9 years ago

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

https://github.com/otac0n/dapper-dot-net/commit/817fa9593397b2fc5d2482159f21315e9f43a662

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 commented 9 years ago

@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 commented 9 years ago

+1

drusellers commented 9 years ago

I've run into this as well. :(

kbilsted commented 8 years ago

What is the status of this issue?

peterthomastlc commented 8 years ago

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 commented 8 years ago

:+1

wmate commented 8 years ago

+1 This. Is. The. Feature.

mgravell commented 8 years ago

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 notifications@github.com wrote:

+1 This. Is. The. Feature.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/StackExchange/dapper-dot-net/issues/259#issuecomment-250268121, or mute the thread https://github.com/notifications/unsubscribe-auth/AABDsPNsoeweGYdz7U35qT_-Bm1txKMWks5qursUgaJpZM4D0geM .

Regards,

Marc

kbilsted commented 8 years ago

Wouldn't integration tests take away some those fears?

chilversc commented 8 years ago

@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 commented 8 years ago

@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 ?

jeremycook commented 6 years ago

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.

ghost commented 6 years ago

Is there any update on this request?

SammyROCK commented 5 years ago

I'm impressed this is still an Issue, I had it last year and just ran into it once again...

jboarman commented 5 years ago

Yeah .. this seems crazy not to have some sort of support

Maximys commented 5 years ago

Let's get my fork with fixes of this bug. I don't know why owner of Dapper not yet reviewed and merged it.

kevingoos commented 5 years ago

No fix yet? Or a workaround?

OsvaldasRubavicius commented 5 years ago

Still no update? Stumbled on this as well.

QtRoS commented 5 years ago

Any news on this? It's a kind of disappointment that enums can't be mapped even with my own type handler.

ayatim commented 4 years ago

:+1

Xenetics commented 4 years ago

:+1

miminno commented 4 years ago

We'll soon be celebrating 5 Years of this issue being ignored. Yay!

davidvmckay commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 3 years ago

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

banderberg commented 3 years ago

Good job, team!

phillip-haydon commented 3 years ago

6 years and no fix :/

joost00719 commented 3 years ago

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.

JeroenHo commented 2 years ago

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

gandhis1 commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

@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 commented 2 years ago

@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 commented 2 years ago

fits the situation image

heku commented 2 years ago

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

nyan-cat commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

+1 for fixing this.

kotvytskyi commented 1 year ago

Please, fix the issue!

asrichesson commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

mgravell commented 1 year ago

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.