mikependon / RepoDB

A hybrid ORM library for .NET.
Apache License 2.0
1.7k stars 126 forks source link

Bug: RepoDb.SqlServer v1.1.3: Failing to map ENUM to INT #779

Open Freysev opened 3 years ago

Freysev commented 3 years ago

Bug Description

Hello. I'm encountering an issue when im trying to insert or merge a class with an enum in it. I've tried adding a [TypeMap(DbType.Int32)] attribute, but it doesnt change anything.

Exception Message:

Compiler.Entity/Object.Property: Failed to automatically convert the value expression 
for property 'Resolution (XXX.YYY.PeriodResolution)'. 
ClassProperty :: Name = Resolution (XXX.YYY.PeriodResolution), DeclaringType = 
XXX.YYY.Models.Entities.MarketPrice
InnerException: Expression of type 'XXX.YYY.PeriodResolution' 
cannot be used for parameter of type 'System.Int32' of method 'Int32 ToInt32(Int32)' (Parameter 'arg0')
InnerException.StackTrace: 
   at System.Dynamic.Utils.ExpressionUtils.ValidateOneArgument(MethodBase method, ExpressionType nodeKind, Expression arguments, ParameterInfo pi, String methodParamName, String argumentParamName, Int32 index)
   at System.Linq.Expressions.Expression.Call(MethodInfo method, Expression arg0)
   at RepoDb.Reflection.Compiler.ConvertExpressionToSystemConvertExpression(Expression expression, Type toType)
   at RepoDb.Reflection.Compiler.ConvertExpressionWithAutomaticConversion(Expression expression, Type toType)
   at RepoDb.Reflection.Compiler.GetEntityInstancePropertyValueExpression(Expression entityInstanceExpression, ClassProperty classProperty, DbField dbField)

Schema and Model:

CREATE TABLE [dbo].[MarketPrices]
(
    [Product] VARCHAR(50) NOT NULL, 
    [Resolution] INT NOT NULL, 
    [Period] INT NOT NULL, 
    [Value ] DECIMAL(18, 3) NOT NULL, 
    Primary key ([Product], [Resolution], [Period])
)
    [Map("[dbo].[MarketPrices]")]
    public class MarketPrice
    {
        public string Product { get; set; }
        [TypeMap(DbType.Int32)]
        public PeriodResolution Resolution { get; set; }
        public int Period{ get; set; }
        public decimal Value { get; set; }
    }
    public enum PeriodResolution
    {
        [Description("Uke")]
        Week,
        [Description("Måned")]
        Month,
        [Description("Kvartal")]
        Quarter,
        [Description("År")]
        Year,
        [Description("Egendefinert")]
        Custom
    }

Library Version:

RepoDb.SqlServer v1.1.3 .NET Core 3.1

mikependon commented 3 years ago

RepoDB should automatically map your PeriodResolution enum type on property Resolution to an INT type without you even defining such TypeMap attribute only if use the model-based operations (i.e.: Insert, Merge, Update etc).

Did you use the raw SQL approach when inserting and/or merging the data?

mikependon commented 3 years ago

Only if use the raw-SQL operations (i.e: ExecuteNonQuery and other Execute<Methods>), then this section from the documentation is relevant to you.

Freysev commented 3 years ago

I'm using MergeAllAsync, also tested with InsertAsync, same issue there

mikependon commented 3 years ago

Interesting as this is well covered here. Let me play around with it.

mikependon commented 3 years ago

A bit weird how is the integration test is passing, but I can replicate this problem. I will revisit all of them. When do you need the fix?

Project: RepoDbEnumProject.zip

Freysev commented 3 years ago

Of cource it would be nice to have it ASAP, but it is not to difficult to work around the issue for now

mikependon commented 3 years ago

I found your problem after playing around with it further. It is because you are using the composite keys in which RepoDB is not working well.

Would it be possible for you to create a dedicated Id for your table like below?

CREATE TABLE [dbo].[MarketPrices]
(
    [Id] INT IDENTITY PRIMARY KEY,
    [Product] VARCHAR(50) NOT NULL, 
    [Resolution] INT NOT NULL, 
    [Period] INT NOT NULL, 
    [Value] DECIMAL(18, 3) NOT NULL
)
mikependon commented 3 years ago

I also place this note on our disclaimer in relation to the limitations.

image

Your scenario is one of them.

Freysev commented 3 years ago

Ok, I'll see if i can avoid the comp key. Thank you for the quick responses :)

Just FYI, as it might be helpful when you are working on comp key issues in the future; While trying to work around the issue i found some interesting things, i'm guessing, also related to composite key.

I tried adding this little get/set hack:

    [Map("[dbo].[MarketPrices]")]
    public class MarketPrice
    {
        public string Product { get; set; }
        public int Resolution { get; set; }
        public PeriodResolution PeriodResolution  //Code updated to ref this prop instead of 'Resolution'
        {
            get { return (PeriodResolution)Resolution; }
            set { Resolution = (int)value; }
        }
        public int Period{ get; set; }
        public decimal Value { get; set; }
    }

But the following method results in only a single row insertion

        public async Task UpsertTest()
        {
            using var connection = new SqlConnection(options.Value.ConnectionString);

            await connection.MergeAllAsync(new List<MarketPrice> {
                    new MarketPrice
                    {
                        Product = "A",
                        PeriodResolution = PeriodResolution.Month,
                        Period = 1,
                        Value = 1
                    },
                    new MarketPrice
                    {
                        Product = "A",
                        PeriodResolution = PeriodResolution.Quarter,
                        Period = 1,
                        Value = 1
                    }
            });
        }

Product | Resolution | Period | Value A | 2 | 1 | 1.000

If i use InsertAsync individually for the two objects it works, i get two rows inserted, but if i try to use UpdateAsync on the inserted rows i get an insert (?) error Violation of PRIMARY KEY constraint 'PK__MarketPr__6988E071DC4E9FB6'. Cannot insert duplicate key in object 'dbo.MarketPrices'. The duplicate key value is (A, 1, 1). The statement has been terminated.

mikependon commented 3 years ago

Yes, that is because RepoDB will use either 1 of the column from your composite key as the qualifier. Of course, unless otherwise you explicitly specify the qualifier fields in the qualifiers argument, this kind of issue is always prevalent.

This is known issue from our limitation page. In short, this is expected if you are using a composite key as the key as also reported by others.

mikependon commented 3 years ago

UpdateAsync? Throwing an Insert exception? As I know, SQL Server is deleting and reinserting a row if you update, that is visible if you are using the CDC, but this seems to me a problem in messaging.

Can you confirm that the insert exception is triggered by update?

Freysev commented 3 years ago

Hi, sorry for late answer. Yes, the error message from my last post came after a call to UpdateAsync.

mikependon commented 3 years ago

I am trying to replicate your issue in relation to the duplicate key constraint when calling the update operation. Unfortunately, I could not replicate it.

In addition to that, you mentioned that you are success in calling the InsertAsync, whereas on my side, I could not make the InsertAsync works unless you remove the Composite Key and replace it with a single column PK.

Note: Again, this is because, the Composite Key is a limitation to the library.

mikependon commented 3 years ago

I had attached a very small project solution above. Can you replicate your issue using that project and attached it back here? I will start the investigation from there. Many thanks!

mikependon commented 3 years ago

I could not replicate this issue once more. Maybe I need a help from your side. I will flag this one as wontfix until you replicate the issue on a small project attached. Please do not hesitate to revert always.

mikependon commented 3 years ago

FYI, in relation to the composite key, it is not fully supported as of writing this. But, there is always alternative, so please do not hesitate to revert if you still have more questions in relation to that.

Freysev commented 3 years ago

Hey, i'm quite busy with other stuff atm, but hopefully i'll get around to replicating with your project later this week or next weekend

mikependon commented 3 years ago

No worries, just feel free to revert always and I can do the fix for your anytime.