Add a helper to use FormattableString for text and parameters.

[ddunkin]

This allows using FormattableString to encapsulate command text and parameters, reducing redundancy and increasing readability.

Example:

var item1 = "one";
var item2 = "two";
connector.Command(Sql($"insert into Items (Name) values ({item1}); insert into Items (Name) values ({item2});")).Execute();

Here,

connector.Command(Sql($"insert into Items (Name) values ({item1}); insert into Items (Name) values ({item2});"))

is the equivalent of

connector.Command("insert into Items (Name) values (@p0); insert into Items (Name) values (@p1);", ("p0", item1), ("p1", "two"))

Using an explicit static method to convert the FormattableString to text and parameters makes it easier to see in code review that it's not a SQL injection vulnerability.

Command does not get an overload that takes a FormattableString, and Sql doesn't take a string, which avoids some of the potential problems, e.g. being able to pull the command argument into a local with var.

// textAndParameters is a tuple
var textAndParameters = Sql($"insert into Items (Name) values ({item1}); insert into Items (Name) values ({item2});");
connector.Command(textAndParameters).Execute();

// textAndParameters is a string
var textAndParameters = $"insert into Items (Name) values ({item1}); insert into Items (Name) values ({item2});");
// this will fail to compile because Sql doesn't take a string
connector.Command(Sql(textAndParameters)).Execute();

[ejball]

Cool idea! I played with similar ideas here but didn't think about creating actual parameters.

How would we feel about CommandFormat and/or requiring a param format specifier?

var name = "hello";
connector.CommandFormat($"insert into Items (Name) values ({name:param});").Execute();

[ddunkin]

How would we feel about CommandFormat and/or requiring a param format specifier?

My first version did use a different method from Command, like that. I went with the static method to address cases where one might want to move the command text to a local variable before creating the command. It doesn't have to be either/or, though.

What's the purpose of the param format specifier?

[ejball]

The param format specifier makes it even more obvious that it's not a SQL injection vulnerability.

It also opens up the possibility of a raw format specifier, e.g. to actually inject SQL, which for better or for worse is fairly common, e.g. to inject a where clause only in certain circumstances. See my "similar ideas" link above for other possible (and possibly controversial) ideas.

[ejball]

It doesn't have to be either/or, though.

True. A static DbCommand.FormatText (FormatSql?) method to create the tuple? Or a separate class might be nice, e.g. for static fields, perhaps DbCommandSql (no, it binds actual parameter values, so that doesn't make sense)?

[ddunkin]

The param format specifier makes it even more obvious that it's not a SQL injection vulnerability.

It also opens up the possibility of a raw format specifier, e.g. to actually inject SQL, which for better or for worse is fairly common, e.g. to inject a where clause only in certain circumstances. See my "similar ideas" link above for other possible (and possibly controversial) ideas.

Making the default behavior (no format specifier) treat the argument as a parameter makes sense to me because I expect that to be the vast majority of use cases. It doesn't preclude other format specifiers, e.g. raw. E.g.

Sql($"select {columns:raw} from items where name = {name}")

The literal from your ideas is interesting. I don't recall having a use case for it, at least in a way that isn't handled just as well by a parameter. When would one want to use that?

[ejball]

I expect that to be the vast majority of use cases

No doubt. If we used it, it would be to make code review easier. With it, you don't need to look at context to determine whether it is a string injection or a SQL parameter.

When would one want to use that?

Efficiency, mostly. If you have lots of values, literals can be a lot faster than parameters. But it might not be wise to encourage it.

[ejball]

Also, without the format specifier, it just reads like injection. I think I would find it dissonant to have the exact same syntax mean different things depending on context.

[ejball]

Also, different databases have different escaping rules for literals, which would complicate matters, especially in this context.

[ddunkin]

I'm not sure the format specifier itself helps that much. Can you tell at a glance which one is an injection vulnerability?

connector.CommandFormat($"insert into Items (Name) values ({name:param});").Execute();

connector.Command($"insert into Items (Name) values ({name:param});").Execute();

The param specifier is just ignored in the injection case.

This whole approach may not be good without an accompanying analyzer that flags misuse.

[ddunkin]

Also, different databases have different escaping rules for literals, which would complicate matters, especially in this context.

Yeah, that was actually my first thought of why it could be problematic.

[ejball]

Can you tell at a glance which one is an injection vulnerability?

No, and I do wish invalid format specifiers caused runtime errors.

But I can tell that it was the author's intention to inject a parameter rather than a raw string. That makes correct code easier to read (IMHO), and a reviewer (or an analyzer) can confirm that CommandFormat is being used when they see param without having to first puzzle the author's intent.

I agree that Faithlife.Data could use some analyzers. Another potentially common mistake is forgetting to call Execute().