Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mapping-aware cusom SQL #34245

Closed
voroninp opened this issue Jul 18, 2024 · 8 comments
Closed

Mapping-aware cusom SQL #34245

voroninp opened this issue Jul 18, 2024 · 8 comments
Labels
closed-no-further-action The issue is closed and no further action is planned. customer-reported type-enhancement

Comments

@voroninp
Copy link

I'd like avoid writing custom sql query when I need to specify names of tables and columns.
Example:

WITH OrderedPeriod AS (
    SELECT
        Id,
        ROW_NUMBER() OVER (ORDER BY StartDate) AS SequenceNumber
    FROM
        SpotAggregationPeriod
    WHERE ReportId = {reportId.Value}
)
UPDATE SpotAggregationPeriod
SET SequenceNumber = OrderedPeriod.SequenceNumber
FROM OrderedPeriod
WHERE SpotAggregationPeriod.Id = OrderedPeriod.Id;

If there's a migration which changes the name of the table or the column, this query will break.

As ExecuteSqlAsync expects an instance of IFormattableString it should be possible to write query the following way:

First define tables and columns:

var periodTable = EF.Relational.Table<SpotAggregationPeriod>();
var idCol = EF.Relational.Column<SpotAggregationPeriod>(p => p.Id);
var startDateCol = EF.Relational.Column<SpotAggregationPeriod>(p => p.StartDate);
var reportIdCol = EF.Relational.Column<SpotAggregationPeriod>(p => p.ReportId);
var sequenceNumberCol = EF.Relational.Column<SpotAggreatePeriod>(p => p.SequenceNumber, tableQualified: false);

Next the query itself:

WITH OrderedPeriod AS (
    SELECT
        {idCol},
        ROW_NUMBER() OVER (ORDER BY {startDateCol}) AS SequenceNumber
    FROM
        {periodTable}
    WHERE {reportIdCol} = {reportId.Value}
)
UPDATE {periodTable}
SET {sequenceNumberCol} = OrderedPeriod.SequenceNumber
FROM OrderedPeriod
WHERE {idCol} = OrderedPeriod.Id;

Table and Column methods should return special type, so its values in IFormattableString can be proccessed by the framework to produce correct string values.

Yes, it does not protect me from specifying property from wrong table or putting parameter in the wrong place of SQL.
Yes, it's more verbose.
But it's still much safer.

@voroninp voroninp changed the title Mapping aware cusom SQL Mapping-aware cusom SQL Jul 18, 2024
@roji
Copy link
Member

roji commented Jul 18, 2024

This effectively seems to be a request for a type used to represent table/column names, i.e. a wrapper for string, just for the purpose of being able to use it with EF's FormattableString SQL query APIs. While I can see the theoretical argument here, I don't think we should introduce something like this to EF. My main problem is that it the FormattableString overloads have a very clem simple logic - anything that's interpolated becomes a DbParameter. Introducing something like the above feature starts blurring that, since some things would be doing (allegedly safe) string interpolation directly into the SQL, while other things would get parameterized as today.

My recommendation in general is to:

  1. Integrate table/column names into SQL. Users are expected to have some sort of test coverage of their SQL queries, otherwise any small typo (these are SQL strings unchecked at compile time!) would cause the query to fail at runtime. And such coverage would immediately catch any incorrect table/column names.
  2. If you really want to avoid integrating names into the SQL strings, use the non-FormattableString overloads to execute SQL queries. Yes, these do not protect against SQL injection, but if you're only interpolating table/column names coming from the model, you should be fine as these generally aren't user inputs.

@voroninp
Copy link
Author

voroninp commented Jul 19, 2024

While tests are obviously is a must, I won't need to fix the query every time migration affects naming. If I change property name in code, query will be changed automatically because property is referenced via expression.
If property is removed, I'll get a compiler error.
I can also lock property types to ensure I won't be comparing apples and oranges in my query:

EF.Relational.Column<SpotAggregationPeriod, int>(x => x.SequenceNumber);

Mixing parameters can be solved by using same used for structured logging:

await dbContext.ExecuteXyzAsync(
    $"""
    SELECT {idCol} 
    FROM {table}
    WHERE {reportIdCol} = @{{reportId}}
    """,
    reportId.Value);

Here formattable string is used only to interpolate db names, and parameters will be injected afterwards.

Could you give me an example for the second item? How do I get the name of the column or table? Maybe I can create my custom extension atop of ExecuteSqlAsync.

@roji
Copy link
Member

roji commented Jul 19, 2024

Mixing parameters can be solved by using same approach as exists in structured logging

Such an overload doesn't currently exist, and introducing a new one would create lots of confusion with the existing API, where once again interpolation (curlies) means DbParameter. In the area of SQL querying, everything needs to be kept crystal clear and simple to understand, since otherwise it's very easy to fall into SQL injection.

How do I get the name of the column or table?

You can easily get table and column names from the EF model, e.g.:

var columnName = context.Model.FindEntityType(typeof(Blog)).GetProperty(nameof(Blog.Name)).GetColumnName();

I'll go ahead and close this as I don't see us implementing this - there isn't a problem that's important enough to be solved, there are other satisfactory options (use non-FormattableString overloads), and anything else would create lots of API confusion in a security-critical area.

@roji roji closed this as not planned Won't fix, can't repro, duplicate, stale Jul 19, 2024
@roji roji added closed-no-further-action The issue is closed and no further action is planned. and removed needs-design propose-close labels Jul 19, 2024
@voroninp

This comment has been minimized.

@roji

This comment has been minimized.

@voroninp

This comment has been minimized.

@voroninp
Copy link
Author

var columnName = context.Model.FindEntityType(typeof(Blog)).GetProperty(nameof(Blog.Name)).GetColumnName();

However, it's not escaped. I assume it's the responsibility of data provider. Is it possible to access this functionality?

@roji
Copy link
Member

roji commented Jul 19, 2024

Tables/columns generally shouldn't need escaping - they're under the control of the application developer etc. But if you really want to, you can access the provider's quoting/escaping logic by resolving its ISqlGenerationHelper service.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-no-further-action The issue is closed and no further action is planned. customer-reported type-enhancement
Projects
None yet
Development

No branches or pull requests

3 participants