Repository navigation
Microsoft.Data.Sqlite and enum mapping #467
Description
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.
Activity
mgravell commentedon Feb 22, 2016
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 commentedon Feb 22, 2016
Thanks, I read through the linked issue. That is the biggest mystery right now, when does RC2 or RTW release? :)
Add SQLITE to test builds; add tests for issues #466 and #467; fix #466…
mgravell commentedon Feb 22, 2016
@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 commentedon Feb 22, 2016
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.
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 commentedon Feb 23, 2016
Firstly: what is the error message you are seeing now? The message in the original post is
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 astring; it is almost certainly returning a boxed primitive marked with the enum type, which theToString()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 commentedon Feb 23, 2016
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 commentedon Feb 23, 2016
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 commentedon Feb 23, 2016
First, thanks for the thorough replies.
To your first question, I get the same message
For your second question,
prop.GetGetMethod().Invoke(Parameters, null);is returning the enum valueString = 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
SanitizeParameterValueand as far as I can tell, it wasn't called. I imagine when you saidIt 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 commentedon Feb 23, 2016
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:
AdamDotNet commentedon Feb 23, 2016
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 theswitchstatement in the non-nullable code branch starting at line 2315.AdamDotNet commentedon Feb 23, 2016
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 commentedon Feb 23, 2016
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:
AdamDotNet commentedon Feb 23, 2016
Hmmm, would you mind sharing your SQL query?
mgravell commentedon Feb 23, 2016
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:
AdamDotNet commentedon Feb 23, 2016
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 commentedon Feb 23, 2016
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:
Regards,
Marc