Sqlite support is broken in DNX

[sebastienros]

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.

[sebastienros]

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]

Before I dig into this...are you saying it's broken only in DNX, and not on the full CLR?

[sebastienros]

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 the DbCommandParameter. 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 @age should find it's way from new { age = 1 }, for in and out parameters. And then the prefixes are lost for the DbCommandParameter. It works if the Sql provider has explicit support for that. Sqlite would break.

[mgravell]

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:

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 the DbCommandParameter. 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 @Age should find it's
way from new { age = 1 }, for in and out parameters. And then the
prefixes are lost for the DbCommandParameter. It works if the Sql
provider has explicit support for that. Sqlite would break.

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

[mgravell]

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:

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:

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 the DbCommandParameter. 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 @Age
should find it's way from new { age = 1 }, for in and out parameters.
And then the prefixes are lost for the DbCommandParameter. It works if
the Sql provider has explicit support for that. Sqlite would break.

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

[sebastienros]

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]

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:

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.

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

[mgravell]

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

It has been fixed in the rc2 branch, in December.

[sebastienros]

So to actually answer the question, it will ship when rc2 ships. Though it's already available in the rc2 feeds.

[mgravell]

Awesome; thanks for the update