Repository navigation
Sqlite support is broken in DNX #375
Description
Parameter names are cleaned using this code:
static string Clean(string name)
{
if (!string.IsNullOrEmpty(name))
{
switch (name[0])
{
case '@':
case ':':
case '?':
return name.Substring(1);
}
}
return name;
}
Meaning if I specifically add an @ in front on my parameter names, for instance in a dictionary, they are automatically removed. The issue is that it is provider specific that DbParameter should be called or not with a char prefix. For instance SqlServer is expecting @ in the name, but the provider will cope with it not being defined, the Sqlite one since DNX though will just pass whatever is set in the DbParameter instance, and with the current code it fails as sqlite expects one (which can be $, @, and others).
I'd suggest to remove this Clean() method altogether, or let adapters handle it.
Activity
[-]DynamicParameters.Clean should unclean sometimes [/-][+]Sqlite support is broken in DNX[/+]sebastienros commentedon Nov 2, 2015
And in case you say the driver should add it, this issue explains the reason not to, as the driver can't guess what char was used in the query.
aspnet/Microsoft.Data.Sqlite#80
NickCraver commentedon Nov 6, 2015
Before I dig into this...are you saying it's broken only in DNX, and not on the full CLR?
sebastienros commentedon Nov 6, 2015
Right, the issue is that the Sqlite driver for DNX was expecting a parameter prefix ('@', '$', ':') because sqlite makes it mandatory. The other providers are more lose in this context.
I got the EF team to fix it 10 minutes ago actually ;) by detecting when the name doesn't have a prefix and getting one from the query itself if there are no ambiguities. So it will be solved with next RC2 release.
The issue in Dapper lays in the
Clean()call which is removing any prefix before creating theDbCommandParameter. Well not the method itself but as it's called from too many places I assume. This is done because you need to map it with dynamic parameters at some point, so@ageshould find it's way fromnew { age = 1 }, for in and out parameters. And then the prefixes are lost for theDbCommandParameter. It works if the Sql provider has explicit support for that. Sqlite would break.mgravell commentedon Nov 6, 2015
It is really hard to fix this in a clean way, especially since the metadata
API is more reduced in DNX (not that we currently use it). We can't
reliably even infer from usage, as in some SQL variants @foo, :foo and $foo
(or whatever) could all be present meaning different things. And for stored
procedures we don't even get that.
I don't like hard coding and special-casing connection types. It kinda
feels like this can should actually be kicked back to the sqlite folks:
"don't do that, it really hurts provider-agnostic tools"...
Thoughts?
On 6 Nov 2015 10:30, "Sébastien Ros" [email protected] wrote:
mgravell commentedon Nov 6, 2015
The other way of thinking about this is to rephrase it slightly: "sqlite
support is broken in a wide range of user/application code and
tools/library code that has used a long-present feature of the sqlite
provider". Fixing one tool is a drop in the ocean compared to the
underlying issue that is going to bite lots and lots of people unless fixed
at source.
On 6 Nov 2015 11:41 am, "Marc Gravell" [email protected] wrote:
sebastienros commentedon Nov 6, 2015
Feel free to close it as "by design" since the "Sqlite folks", a.k.a Entity Framework team, have implemented prefix inference in the driver since then. They agreed that it could just not hurt more to inject it if there were none.
mgravell commentedon Nov 6, 2015
Ah, when you mentioned EF I pictured EF having the same problem when
talking to sqlite. If the EF team owns and has already fixed the sqlite
driver, then there's nothing to do here IMO.
On Fri, 6 Nov 2015 13:12 Sébastien Ros [email protected] wrote:
A full CoreCLR/DNX friendly conversion
A full CoreCLR/DNX friendly conversion
mgravell commentedon Feb 22, 2016
@sebastienros this came up again today; any idea when the next public (nuget) drop of Microsoft.Data.SQLite can be expected (i.e. with the fix)?
sebastienros commentedon Feb 22, 2016
It has been fixed in the rc2 branch, in December.
sebastienros commentedon Feb 22, 2016
So to actually answer the question, it will ship when rc2 ships. Though it's already available in the rc2 feeds.
mgravell commentedon Feb 22, 2016
Awesome; thanks for the update