Case-sensitive parameter names

[zabolotnev]

I'm used to all IDbCommand implementations have case insensitive parameter names. This is true for the SqlClient namespace and the System.Data.SQLite package. This feature is very useful, but when I started using the Microsoft.Data.Sqlite package, I got an error:
System.InvalidOperationException: 'Must add values for the following parameters'

To Reproduce

    class TestClass
    {
        public static void Test()
        {
            var f = "testdb.sqlite";
            if (File.Exists(f))
            {
                File.Delete(f);
            }
            using (var conn = new SqliteConnection("Data source = " + f))
            {
                conn.Open();
                var cmd = conn.CreateCommand();
                cmd.CommandText = "Create table test(f1 integer)";
                cmd.ExecuteNonQuery();

                cmd.CommandText = "select * from test where f1 = @PRM";

                var p = cmd.CreateParameter();
                p.ParameterName = "@prm";
                p.Value = 1;
                cmd.Parameters.Add(p);

                using (cmd.ExecuteReader()) { }

            }
        }
    }

Result:

System.InvalidOperationException
  HResult=0x80131509
  Message=Must add values for the following parameters: @PRM
  Source=Microsoft.Data.Sqlite
  StackTrace:
   at Microsoft.Data.Sqlite.SqliteCommand.<GetStatements>d__54.MoveNext()
   at Microsoft.Data.Sqlite.SqliteDataReader.NextResult()
   at Microsoft.Data.Sqlite.SqliteCommand.ExecuteReader(CommandBehavior behavior)
   at Microsoft.Data.Sqlite.SqliteCommand.ExecuteReader()
   at ConsoleApp14.TestMS.Test() in D:\Projects\ConsoleApp14\TestMS.cs:line 34

Additional context

I found the reason of case-sensitivity here: https://github.com/aspnet/EntityFrameworkCore/blob/master/src/Microsoft.Data.Sqlite.Core/SqliteCommand.cs
Line 336:

if (_parameters.IsValueCreated
&& !_parameters.Value.Cast<SqliteParameter>().Any(p => p.ParameterName == name))

Equality operator == is case sensitive.

For example, System.Data.SQLite uses this method of compare:
https://github.com/OpenDataSpace/System.Data.SQLite/blob/master/System.Data.SQLite/SQLiteStatement.cs
Line 209:

if (String.Compare(_paramNames[n], startAt, s, 0, Math.Max(_paramNames[n].Length - startAt, s.Length), StringComparison.OrdinalIgnoreCase) == 0)

Microsoft.Data.Sqlite version: 3.1.0-preview2.19525.5
Target framework: .NET 4.8, Xamarin
Operating system: Windows, Android

[ajcvickers]

@zabolotnev Thanks for filing this issue. We will discuss in our next triage meeting.

/cc @bricelam

[bricelam]

Parameters are case-sensitive in SQLite:

command.CommandText = "SELECT $A, $a, @A, @a, :A, :a";
command.Parameters.AddWithValue("$A", 1L);
command.Parameters.AddWithValue("$a", 2L);
command.Parameters.AddWithValue("@A", 3L);
command.Parameters.AddWithValue("@a", 4L);
command.Parameters.AddWithValue(":A", 5L);
command.Parameters.AddWithValue(":a", 6L);

It would be a nice enhancement, however, to be case-forgiving when the name is not ambiguous.

[thiagokoster]

Hi, I want to contribute to an open source project for the first time, and since this issue has a "good first issue" tag I would like to try this one. I already downloaded the project and saw where I could do the change. I can already start?
Thanks in advance!

[bricelam]

@thiagokoster Feel free to submit a PR. We can work through any implementation details there that come up.

[KevRitchie]

Hi @bricelam, is this still up for grabs? @thiagokoster, do you still want to take this one?

[bricelam]

Yep, go for it

[roji]

Sorry for not commenting earlier on this issue, but I think we shouldn't do this change for performance reasons.

In a nutshell, it originally seemed reasonable perf-wise to only attempt insensitive matching if sensitive matching fails - this way perf isn't affected for the case-sensitive scenario. However, it turns out the it's common for code to first check if a parameter name is already present in the collection, and only then add it; Dapper notably does this. This means two dictionary lookups just for that check, and things get worse as lots of parameters are added.

FYI in Npgsql we've done case-insensitive parameter name matching up to now (because SqlClient does them), but are changing that behavior by default for 6.0 (with an AppContext switch for the legacy case-insensitive behavior). See the full details (and Dapper info) in npgsql/npgsql#4027 (plus an additional user complaint in npgsql/npgsql#3978). I wouldn't be thrilled to see Sqlite going in the exact opposite direction...

[KevRitchie]

@roji that's a fair comment. @bricelam, do you think it's worth closing this issue then?

[roji]

@KevRitchie thanks for understanding and sorry again you already invested time in the PR. But let us first discuss this as a team to decide on what we want to do here - the above is only my opinion.

[KevRitchie]

@roji no problem at all. It's all experience 😄 Do you want me keep the PR open for now or close it?

[roji]

@KevRitchie let's keep it open for now until we make a decision.

[thiagokoster]

You can take it!

…

On Wed, 6 Oct 2021 at 16:07 Kev Ritchie ***@***.***> wrote: Hi @bricelam <https://github.com/bricelam>, is this still up for grabs? @thiagokoster <https://github.com/ThiagoKoster>, do you still want to take this one? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#18861 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AEXK3PTR6FAMMLGFUFUIOADUFSNA3ANCNFSM4JMAY3CA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

[roji]

One more note for completeness... Just doing a single insensitive list pass is problematic also because an earlier parameter may match insensitively where there's a later parameter that could match sensitively.