Separate methods for raw SQL that leverage interpolated strings

[divega]

Currently the behavior is different if you call FromSql or ExecuteSqlCommand with an inline interpolated string (parameterization happens) or if you assign the interpolated string to a variable and then pass it to the method (the string is formatted with the arguments).

In some cases customers expect parameterization but it doesn't happen: #10080

In other cases customers expect parameterization not to happen, but it does happen: #10956

This is very confusing and hard to explain. Interpolated strings in C# were simply not designed in a way that works well with having two overloads of the same method in which one works with interpolated strings and the other one with a regular string.

I think we should separate the methods, but we can only do this in major release because it is a breaking change.

This would also imply removing the RawSqlString type.

[NickCraver]

Absolutely agree with this move. If it helps, I wrote up some more test cases on how this is confusing (and sometimes dangerous) when it first came up here: https://github.com/NickCraver/EFCoreInjectionSample/blob/master/Program.cs

[divega]

@NickCraver thanks. We are going to try to address #10080 in the shorter term with a Roslyn analyzer. I think we can probably leverage your tests as tests cases for the analyzer.

cc @anpete

[ajcvickers]

And remove or modify the analyzer?

[divega]

I believe modify. I haven’t thought through all the details but I believe it can still provide value on the string overload.

[xaviergxf]

Please take into consideration cases like this:

var tablename = dbcontext.Model.FindEntityType(typeof(ABC)).Relational().TableName;
delete from {tableName} where a=123;

How to create a raw sql in such case? Is there currently any workaround?

[jeremycook]

I'm glad this is to be improved. One request is being able to opt-into raw strings when using the interpolated string arguments/methods, something like:

var tablename = dbcontext.Model.FindEntityType(typeof(ABC)).Relational().TableName;
$"select * from {Sql.Raw(tablename)} where date between {from} and {to}"

from and to would be parameterized and tablename would not. This idea could be reused if Sql.Raw returns an implementation of IRawSql and if something implements IRawSql then its IRawSql.Value will be inserted and not parameterized. Taken further, the above example could be shortened to the following C# if IRelationalEntityTypeAnnotations extends IRawSql which when evaluated would return a string like "[someSchema].[SomeTable]".

var table = dbcontext.Model.FindEntityType(typeof(ABC)).Relational();
$"select * from {table} where date between {from} and {to}"

IRelationalPropertyAnnotations could also extend IRawSql and its IRawSql.Value in this example could return "[date]":

var table = dbcontext.Model.FindEntityType(typeof(ABC)).Relational();
var column = dbcontext.Model.FindEntityType(typeof(ABC)).FindProperty("Date").Relational();
$"select * from {table} where {column} between {from} and {to}"

Implementations of IRawSql for entity types and properties would need to be specific to each database provider so that identifiers are properly escaped with brackets, double-quotes, back ticks, etc.

[jeremycook]

I would also suggest providing arguments that take an IEnumerable<FormattableStrings> to enable composing interpolated strings without losing parameterization. Here's a slightly contrived example:

public IQueryable<Post> PublishedPosts(FormattableString filter = null)
{
    return this.Posts.InterpolatedSql(new FormattableString[] { 
        $"select * from dbo.Posts where Published = 1 and ", 
        filter ?? $"1=1"
    });
}

You could also imagine building a List<FormattableString> and passing that in.

But perhaps there is a better way I am not thinking of.

[divega]

To elaborate on what @smitpatel wrote above:

We discussed this extensively in the EF design meeting today. For a while I have been inclined to adopt option A, which would keep FromSql as the version that takes FormattableString and would require a new name that conveys danger for the version that works directly with strings.

But after reading many of the comments on this thread, I began to realize that this approach would not address a couple of important concerns:

1. What the interpolated string version of the method is doing is actually very special and can be pretty surprising if you come with the expectation that you are using the language feature just to build a string

2. There are actually many ways to use the string version of the API that are perfectly safe. In other words, this version of the method is not really a more dangerous API than DbCommand.CommandText (for which there is an off-the-shelf fxcop rule) or similar APIs on Dapper. Things only became dangerous when we conflated the interpolated string version and the compiler could switch between the two versions without you realizing it

So, after discussing, we agreed to go with option B:

1. Deprecate both flavors of the APIs
2. Come up with the new naming pattern. As @smitpatel mentioned, we are going with FromRawSql, FromInterpolatedSql, ExecuteRawSql, ExecuteInterpolatedSql.

The (relatively) big disadvantage of this approach is that we can no longer use the simplest name, "FromSql", but we believe we can live with this.

[NickCraver]

I can't think of a use of FormattableString being unsafe, generating invalid SQL that will error, yes, but being unsafe, no

I just wanted to point out here for anyone else finding later: this isn't accurate...it's still unsafe. Strings are very dangerous and as long as they are in play anything that gets turned into a string in fair game for input exploitation. I added some ways this is exploitable in the repo at the start of the issue: https://github.com/NickCraver/EFCoreInjectionSample/blob/master/Program.cs

[divega]

@NickCraver can you point to which example you are talking about here?

I think @jeremycook was referring to the method that only accepts FormattableString.

From the examples in https://github.com/NickCraver/EFCoreInjectionSample/blob/17fa99c2d1a46933755ba477f65090b6b4805e6d/Program.cs#L20-L50, I think the only one that would compile against such method is the first one. All the other ones use strings.

BTW, I am not pushing back on the fact that there is always going to be some way to do something unsafe with interpolated strings or without them. I just want to understand if you are referring to something that separating the methods doesn't address.

[roji]

Agree with @divega, I'd be interested in seeing a case where a method accepting only a FormattableString could be used in an unsafe way. No casts are possible between string and FormattableString, you can't concatenate FormattableStrings... It basically seems like the only way to integrate user input is to go through parameterization, which is safe...

[jeremycook]

@NickCraver I can understand how future onlookers could misunderstand my point. So I reworded my comment, replacing "FormattableString" with "FromInterpolatedSql".

[NickCraver]

@jeremycook my apologies - you wrote FormattableString and my brain read it as IFormattableString even after a few passes. I completely agree on the concrete type: there's no way I can think of to break that approach. If we allowed the interface anywhere...oh yeah, lots of fun. Sorry about that!

[ajcvickers]

Closing since this is now done and the break is documented.