Microsoft.Data.Sqlite and enum mapping

[AdamDotNet]

I have a POCO:

public partial class ColumnMap
    {
        public long Id { get; set; }
        public string ColumnName { get; set; }
        public DataType DataType { get; set; }
        public long Ordinal { get; set; }
        public VariableType VariableType { get; set; }
    }

And some enums:

public enum DataType : long
    {
        Byte = 1,
        Boolean = 2,
        Int16 = 3,
        Int32 = 4,
        Int64 = 5,
        Single = 6,
        Double = 7,
        Decimal = 8,
        String = 9,
        Blob = 10
    }

public enum VariableType : long
    {
        Collected = 0,
        AutoField = 1,
        Merged = 2,
        Segment = 3,
        Weight = 4
    }

This is part of my class declaration that uses dapper:

public class DapperSqliteDataHelper : IDataHelper
    {
        private bool isInitialized = false;
        private readonly SqliteConnection connection;
        private List<ColumnMap> columnMaps;

        public DapperSqliteDataHelper(SqliteConnection connection)
        {
            this.connection = connection;
        }

        public async Task AddColumnMappingAsync(string columnName, DataType dataType, VariableType variableType)
        {
            await InitializeAsync().ConfigureAwait(false);
            long nextId = columnMaps.Max(c => c.Id) + 1;
            long nextOrdinal = columnMaps.Max(c => c.Ordinal) + 1;

            ColumnMap newMap = new ColumnMap
            {
                ColumnName = columnName,
                DataType = dataType,
                Id = nextId,
                Ordinal = nextOrdinal,
                VariableType = variableType
            };

            await connection.QueryAsync("INSERT INTO column_map (column_id, column_name, ordinal, data_type, variable_type) VALUES (@Id, @ColumnName, @Ordinal, @DataType, @VariableType);", newMap).ConfigureAwait(false);

            columnMaps.Add(newMap);
        }
//...
}

Upon running QueryAsync, the following exception is thrown (I added white space after the @ to avoid tagging people):
Must add values for the following parameters: @ Id, @ ColumnName, @ Ordinal, @ DataType, @ VariableType

at Microsoft.Data.Sqlite.SqliteCommand.ExecuteReader(CommandBehavior behavior)
at Microsoft.Data.Sqlite.SqliteCommand.ExecuteReaderAsync(CommandBehavior behavior, CancellationToken cancellationToken)
at Microsoft.Data.Sqlite.SqliteCommand.d__52.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
at System.Runtime.CompilerServices.ConfiguredTaskAwaitable1.ConfiguredTaskAwaiter.GetResult() at Dapper.SqlMapper.<QueryAsync>d__221.MoveNext() in D:\GitHub\dapper-dot-net\Dapper\SqlMapper.Async.cs:line 210

With close inspection in the debugger, I found that after invoking the paramReader (https://github.com/StackExchange/dapper-dot-net/blob/master/Dapper/CommandDefinition.cs#L134), my parameters for enum types are essentially represented as their enum string values instead of long values:

The long value should be 9, not "String". Notice the DbType is correct, however.

[mgravell]

This is a duplicate of #375.

I believe this is fixed in the next sqlite core-clr deploy, but I'll continue to track it.

[AdamDotNet]

Thanks, I read through the linked issue. That is the biggest mystery right now, when does RC2 or RTW release? :)

[mgravell]

@AdamDotNet I'm aftaid the best I can refer you to there is the update by sebastienros a few minutes ago (on the linked issue)

[AdamDotNet]

Sorry, for a little more clarification, I added myget builds for the December Rc2 SQLite release and I'm seeing the same error. Enums aren't turning into numbers in the command parameters. It's entirely possible that MyGet didn't quite have the fix mentioned, however.

I looked at it further, and line SqlMapper:2295 writes IL that calls the get method of a property of the class I passed in. In the case of the property being an enum, the get method returns the name of the enum instead of the numeric value, seems like what I would expect.

While the debugger was stopped in CommandDefinition.SetupCommand, I used the immediate window to "simulate" what I believe the IL was trying to do.

var prop = System.Linq.Enumerable.Single(Parameters.GetType().GetProperties(), p => p.Name == "DataType");
prop.GetGetMethod().Invoke(Parameters, null);
// Returns
// String

So just so I'm clear, the SQLite driver is in charge of changing enum CommandParameters to the underlying type, such as long, and it simply isn't?

[mgravell]

Firstly: what is the error message you are seeing now? The message in the original post is

Must add values for the following parameters: @ Id, @ ColumnName, @ Ordinal, @ DataType, @ VariableType

which is the linked provider bug. Next, "names" are not the same as typed enum values; I very much doubt that prop.GetGetMethod().Invoke(Parameters, null); actually returns a string; it is almost certainly returning a boxed primitive marked with the enum type, which the ToString() happens to unwrap nicely. The IL here gets the unboxed primitive then boxes it, which is slightly different.

As for who is in charge of changing enum parameters - historically, the ADO.NET providers used to deal with this, but it looks like they are failing hard; see also: https://github.com/dotnet/corefx/issues/1613

Even though this isn't actually a dapper issue, I'm fed up with it being a problem. I'll see about doing the work to make this problem go away.

Fun related fact: boxed primitives / enums are unique in that they can actually be unboxed incorrectly successfully; you can box an int and unbox it as an int-typed enum, for example.

I'll create a new issue about the enum problem.

[mgravell]

Hmm.... thinking about it, I thought we'd already fixed this by adding the workaround! I'll have to yank down the rc2 bits to try it; do you happen to have the feed / etc details that you used to get this, so I can repro?

The AdoNetEnumValue() in the test suite only looks at raw ADO.NET; pretty sure dapper actually deals with it correctly already.

[mgravell]

Note: the magic here happens in SanitizeParameterValue, but this wasn't being used (or an equivalent) in some cases (specifically: CreateParamInfoGenerator). This is now rectified.

[AdamDotNet]

First, thanks for the thorough replies.

To your first question, I get the same message

Must add values for the following parameters: @ Id, @ ColumnName, @ Ordinal, @ DataType, @ VariableType

For your second question, prop.GetGetMethod().Invoke(Parameters, null); is returning the enum value String = 9, not a "string". Sorry for the confusion, I should have been more clear. See enum definition in my initial post. So your points about boxing and enum.ToString are all clear to me and are indeed likely what is happening.

The RC2 bits I used were from adding a new package source for NuGet
aspnetvnext
https://www.myget.org/F/aspnetvnext/api/v3/index.json

Finally, I put a break point in the method SanitizeParameterValue and as far as I can tell, it wasn't called. I imagine when you said

Note: the magic here happens in SanitizeParameterValue , but this wasn't being used (or an equivalent) in some cases (specifically: CreateParamInfoGenerator ). This is now rectified.

It simply hasn't been merged into master yet, so I wouldn't see it happen yet.

Perhaps all of this was a bad assumption on my part. I guess I assumed that the enum was to blame, but really the driver isn't happy about the parameter names not starting with @, which is a provider bug as you say?

[mgravell]

The overwhelming problem is the names bug fixed in the provider. I have
fixed the "primitive vs typed" thing - that should have made it into master
now.
On 23 Feb 2016 4:01 pm, "AdamDotNet" [email protected] wrote:

First, thanks for the thorough replies.

To your first question, I get the same message

Must add values for the following parameters: @ Id, @ ColumnName, @
Ordinal, @ DataType, @ VariableType

For your second question, prop.GetGetMethod().Invoke(Parameters, null);
is returning the enum value String = 9, not a "string". Sorry for the
confusion, I should have been more clear. See enum definition in my initial
post. So your points about boxing and enum.ToString are all clear to me and
are indeed likely what is happening.

The RC2 bits I used were from adding a new package source for NuGet
aspnetvnext
https://www.myget.org/F/aspnetvnext/api/v3/index.json

Finally, I put a break point in the method SanitizeParameterValue and as
far as I can tell, it wasn't called. I imagine when you said

Note: the magic here happens in SanitizeParameterValue , but this wasn't
being used (or an equivalent) in some cases (specifically:
CreateParamInfoGenerator ). This is now rectified.

It simply hasn't been merged into master yet, so I wouldn't see it happen
yet.

Perhaps all of this was a bad assumption on my part. I guess I assumed
that the enum was to blame, but really the driver isn't happy about the
parameter names not starting with @, which is a provider bug as you say?

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

[AdamDotNet]

Yes I see the enum code now. I am fairly new to git syntax so I used git checkout . instead of git pull, and thus didn't see new code.

I think the solution is great, it works, and puts the long 9 instead of String as I expect now. One comment I would make is that it seems to drop into the if (checkForNull) branch after it already determined the enum was not nullable.

See SqlMapper Line 2315 for the code branch that knows it isn't nullable, and then the check for nulls branch anyway at SqlMapper Line 2341

I suggest setting checkForNull = false; just after the switch statement in the non-nullable code branch starting at line 2315.

[AdamDotNet]

At the end of the day, I'm just waiting on SQLite working correctly now it seems. I understand why it is complaining, you removed the @ sign, but it sounds like that is perfectly normal behavior to expect the drivers to handle.

[mgravell]

Good spot on the null check. Will fix. The parameter name thing seems to be
fixed in rc2 from my testing.
On 23 Feb 2016 5:47 p.m., "AdamDotNet" [email protected] wrote:

Yes I see the enum code now. I am fairly new to git syntax so I used git
checkout . instead of git pull, and thus didn't see new code.

I think the solution is great, it works, and puts the long 9 instead of
String as I expect now. One comment I would make is that it seems to drop
into the if (checkForNull) branch after it already determined the enum
was not nullable.

See SqlMapper Line 2315
https://github.com/StackExchange/dapper-dot-net/blob/master/Dapper/SqlMapper.cs#L2315
for the code branch that knows it isn't nullable, and then the check for
nulls branch anyway at SqlMapper Line 2341
https://github.com/StackExchange/dapper-dot-net/blob/master/Dapper/SqlMapper.cs#L2341

I suggest setting checkForNull = false; just after the switch statement
in the non-nullable code branch starting at line 2315.

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

[AdamDotNet]

Hmmm, would you mind sharing your SQL query?

[mgravell]

At the moment I only have a very simple minimal query - see the
DapperEnumValue method. It is also possible that my test rig ran against
the wrong platform. I will check later. I would also happily accept fully
runnable failing repro examples either here as comments, or via a pull
request (don't worry if it fails the appveyor build, if it is a known
failing test)
On 23 Feb 2016 6:58 pm, "AdamDotNet" [email protected] wrote:

Hmmm, would you mind sharing your SQL query?

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

[AdamDotNet]

Ok two things, and I'm showing a little humility here because I made silly mistakes.

One, my unit test library referenced RC1 when my service library referenced RC2, so that's why the same error came back. I now am making sure RC2 outputs to the bin folder correctly, and the parameters error goes away, like you observed.

Two, I then got SqlMapper.GetColumnHash() errors, but that was due to the fact that I used `connection.QueryAsync' on an INSERT/UPDATE query instead of 'connection.ExecuteAsync'!

Ah I feel so bad! I'm sorry to have put you through this trouble. Hopefully some good came to the Dapper project out of my folly, however.

[mgravell]

Absolutely! In fact, I've just pushed the fix for the null-check on enums,
so: thanks for spotting that.

On 23 February 2016 at 21:08, AdamDotNet [email protected] wrote:

Ok two things, and I'm showing a little humility here because I made silly
mistakes.

One, my unit test library referenced RC1 when my service library
referenced RC2, so that's why the same error came back. I now am making
sure RC2 outputs to the bin folder correctly, and the parameters error goes
away, like you observed.

Two, I then got SqlMapper.GetColumnHash() errors, but that was due to the
fact that I used `connection.QueryAsync' on an INSERT/UPDATE query instead
of 'connection.ExecuteAsync'!

Ah I feel so bad! I'm sorry to have put you through this trouble.
Hopefully some good came to the Dapper project out of my folly, however.

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

Regards,

Marc