Case sensitive parameters 18861

[KevRitchie]

• I've read the guidelines for contributing and seen the walkthrough

• I've posted a comment on an issue with a detailed description of how I am planning to contribute and got approval from a member of the team

• The code builds and tests pass locally (also verified by our automated build checks)

• Commit messages follow this format:

    Summary of the changes
  
    - Check SqliteParameterCollection for parameters that already exist ignoring casing
    - Update Bind to check for parameters regardless of casing
    - Add test for case-insensitive parameter binding
  

Fixes #18861

• Tests for the changes have been added (for bug fixes / features)
• Code follows the same patterns and style as existing code in this repo

[dnfadmin]

All CLA requirements met.

[smitpatel]

release/6.0 branch is closed for commits unless critical ship blockers. Please rebase your branch on top main branch.

[roji]

@KevRitchie I've posted a general comment on the perf consequences of this change (apologies, I should have posted that earlier).

[KevRitchie]

@roji makes perfect sense. It would be best to keep everything moving in the same direction.

[bricelam]

Finally circling back to this...

@roji your comment about perf doesn't apply in this situation. The .NET parameter collection object would remain case-sensitive. You need to check for a parameter using the exact case it was added in.

This just adds a fallback during execution where, if the parameter fails to bind to the SQL, we'll try a case-insensitive match instead. Perf isn't affected for parameters specified using the exact case. This just allows what is currently a negative scenario to succeed if the SQL happens to contain a different case. This is similar to the code added in #24588 for data readers where, only if we fail to find an exact match for the column, we'll fallback to a case-insensitive match.

I think we should move forward with this change.

[bricelam]

This code shouldn't require any changes since parameter.Bind will return true if any match was found.

We do however have to update GetStatements.

[KevRitchie]

I've reverted the code back.

Is this the GetStatements method in SqliteCommand?

[bricelam]

Yes, for the error message that says which parameters weren't bound. Today, it just does a case-sensitive check:

efcore/src/Microsoft.Data.Sqlite.Core/SqliteCommand.cs

Lines 332 to 335 in b19c1e7

	var name = sqlite3_bind_parameter_name(stmt, i).utf8_to_string();
	if (_parameters != null
	&& !_parameters.Cast<SqliteParameter>().Any(p => p.ParameterName == name))

[bricelam]

We should also add a similar case-insensitive fallback for FindPrefixedParameter.

[bricelam]

Needs more coverage:

SQL	Parameter name(s)	Expected Result
SELECT $param	Param	Adds prefix and binds using a case-insensitive match
SELECT $param, $Param	$param and $Param	Both parameters are bound using case-sensitive matches
SELECT $param, $Param	$Param and then $param	Both parameters are bound using case-sensitive matches. Order doesn't matter
SELECT $param, $PARAM	$Param	Throws ambiguous parameter exception
SELECT $param, $PARAM	Param	Throws ambiguous parameter exception

[bricelam]

Also make sure the new GetStatements code gets covered.

[roji]

@bricelam if I'm understanding you correctly, you're saying that operations on SqliteParameterCollection would be case-sensitive only (so e.g. Contains for name foo would return false when there's a parameter with name FOO), but the matching of placeholders in the SQL (e.g. @FOO) would bind in a case-insensitive way (so @foo in the SQL would successfully bind to a parameter with FOO)?

If I've understood correctly, isn't this behavioral discrepancy problematic? I'd expect the same behavior to apply to both operations on SqliteParameterCollection and in placeholder binding (i.e. the name should either be case-sensitive or case-insensitive, but not sometimes one and sometime the other)?

[ajcvickers]

@bricelam Agree this seems reasonable to move forward with.

@roji Not sure why those things need to behave the same. SqliteParameterCollection doesn't have a behavior that works when the generated SQL does. However, the generated SQL is not limited by a limitation in the specific ways SqliteParameterCollection can be used.

[roji]

Not sure why those things need to behave the same.

Simply because both are about whether the parameter name is treated in a case-sensitive way or not? At the very least, it seems confusing to have two opposite behaviors around the same thing.

Put another way... Why exactly do we feel it's important to add the case-insensitive fallback for placeholder binding? I'm generally not a fan of lax/fuzzy/case-insensitive logic, but if do for some reason, why is that reason not equally valid for SqliteParameterCollection?

[bricelam]

To use Dapper as an example, I think a lot of people could conceivably write this code and expect it to just work:

var post = connection
    .Query<Post>(
        "select * from posts where id = @id",
        new { Id = 1 })
    .FirstOrDefault();

They aren't really thinking about how the case of the C# anonymous type's property and the SQL parameter need to match up.

[roji]

@bricelam yeah, I get that. I occasionally also get requests from people wanting to assign a string value to an NpgsqlDbType.Integer parameter, with Npgsql doing an implicit conversion - and tell them that I think it's a bad idea.

I guess I'm just a strictly-typed, case-sensitive kind of guy, just like C# :) I don't think it's a big deal for users to make their Dapper placeholders match the actual parameters; it also removes the ambiguity around what happens if there are two placeholders with the same case. I'd love for Npgsql to be in the same place as SQLite is now (fully case-sensitive), but an attempt to do that for 6.0 backfired pretty badly. I also specifically find it weird for the parameter collection to behave in one way, and for actual placeholder binding to behave in another way.

I've said my piece, if you guys still think it's important to do this (the issue only has two votes!), then that's fine by me...

[bricelam]

@KevRitchie I'm still happy to take this change. Are you still interested in addressing the comments from my review?

[KevRitchie]

@bricelam sorry for not picking this up sooner. Things have been a little hectic. I'm still interested. How soon do you need it turned around?

[bricelam]

No rush. I was just checking in. We usually try to be feature-complete for a release around mid-August. But even if it's not done by then, it'll just go into the next release it's ready for.

[ajcvickers]

Note from triage: closing this for now due to lack of activity. Happy to re-open if work resumes.