Throw when same-named DbParameters are hard-coded in FromSql/SqlQuery SQL

src/EFCore.Relational/EFCore.Relational.baseline.json

@@ -15592,6 +15592,9 @@
         {
           "Member": "static string ExplicitDefaultConstraintNamesNotSupportedForTpc(object? explicitDefaultConstraintName);"
         },
+        {
+          "Member": "static string FromSqlDuplicateParameterName(object? parameterName);"
+        },
         {
           "Member": "static string FromSqlMissingColumn(object? column);"
         },

src/EFCore.Relational/Properties/RelationalStrings.resx

@@ -565,6 +565,9 @@
   <data name="ExplicitDefaultConstraintNamesNotSupportedForTpc" xml:space="preserve">
     <value>Explicitly named default constraints cannot be used with TPC inheritance or entity splitting. Constraint name: '{explicitDefaultConstraintName}'.</value>
   </data>
+  <data name="FromSqlDuplicateParameterName" xml:space="preserve">
+    <value>The DbParameter name '{parameterName}' is used by more than one parameter in the same query, and the SQL passed to 'FromSqlRaw' or 'SqlQueryRaw' references it by name. EF cannot rename a parameter whose name is written directly in the SQL, even if the parameters have the same value. Use a distinct name for each parameter, or reference the parameters positionally (for example '{{0}}') so that EF can assign unique names.</value>
+  </data>
   <data name="FromSqlMissingColumn" xml:space="preserve">
     <value>The required column '{column}' was not present in the results of a 'FromSql' operation.</value>
   </data>

src/EFCore.Relational/Query/Internal/RelationalParameterProcessor.cs

@@ -1,6 +1,7 @@
 // Licensed to the .NET Foundation under one or more agreements.
 // The .NET Foundation licenses this file to you under the MIT license.
 
+using System.Globalization;
 using Microsoft.EntityFrameworkCore.Query.SqlExpressions;
 using Microsoft.EntityFrameworkCore.Storage.Internal;
 
@@ -152,7 +153,7 @@ private FromSqlExpression VisitFromSql(FromSqlExpression fromSql)
                 {
                     if (parameterValues[i] is DbParameter dbParameter)
                     {
-                        subParameters.Add(ProcessDbParameter(dbParameter));
+                        subParameters.Add(ProcessDbParameter(dbParameter, i));
                     }
                     else
                     {
@@ -174,7 +175,7 @@ private FromSqlExpression VisitFromSql(FromSqlExpression fromSql)
 
                 for (var i = 0; i < existingValues.Length; i++)
                 {
-                    constantValues[i] = ProcessConstantValue(existingValues[i]);
+                    constantValues[i] = ProcessConstantValue(existingValues[i], i);
                 }
 
                 return fromSql.Update(Expression.Constant(constantValues, typeof(object[])));
@@ -191,7 +192,7 @@ private FromSqlExpression VisitFromSql(FromSqlExpression fromSql)
                         throw new InvalidOperationException();
                     }
 
-                    constantValues[i] = ProcessConstantValue(existingValue);
+                    constantValues[i] = ProcessConstantValue(existingValue, i);
                 }
 
                 return fromSql.Update(Expression.Constant(constantValues, typeof(object[])));
@@ -201,15 +202,15 @@ private FromSqlExpression VisitFromSql(FromSqlExpression fromSql)
                 throw new UnreachableException("FromSql.Arguments must be Constant/QueryParameterExpression");
         }
 
-        object ProcessConstantValue(object? existingConstantValue)
+        object ProcessConstantValue(object? existingConstantValue, int index)
             => existingConstantValue is DbParameter dbParameter
-                ? ProcessDbParameter(dbParameter)
+                ? ProcessDbParameter(dbParameter, index)
                 : _sqlExpressionFactory.Constant(
                     existingConstantValue,
                     existingConstantValue?.GetType() ?? typeof(object),
                     _typeMappingSource.GetMappingForValue(existingConstantValue));
 
-        RawRelationalParameter ProcessDbParameter(DbParameter dbParameter)
+        RawRelationalParameter ProcessDbParameter(DbParameter dbParameter, int index)
         {
             _processedDbParameters ??= [];
 
@@ -222,14 +223,86 @@ RawRelationalParameter ProcessDbParameter(DbParameter dbParameter)
                 return existingParameter;
             }
 
-            dbParameter.ParameterName = string.IsNullOrEmpty(dbParameter.ParameterName)
-                ? GenerateNewParameterName()
-                : UniquifyParameterName(dbParameter.ParameterName);
+            if (string.IsNullOrEmpty(dbParameter.ParameterName))
+            {
+                dbParameter.ParameterName = GenerateNewParameterName();
+            }
+            else
+            {
+                var originalName = dbParameter.ParameterName;
+                var uniquifiedName = UniquifyParameterName(originalName);
+
+                // If the name had to be changed, this is only safe if EF is the one writing the parameter's name into the SQL (i.e. the
+                // SQL references the argument positionally via {n}). If the SQL hard-codes the original name, renaming the DbParameter
+                // would silently leave the SQL pointing at a different parameter with the same name. See #32932.
+                // Note that this doesn't detect SQL which references the same argument both positionally and by name; the by-name
+                // reference would still be left pointing at the other parameter.
+                if (uniquifiedName != originalName && !ReferencesArgumentByPosition(fromSql.Sql, index))
+                {
+                    throw new InvalidOperationException(RelationalStrings.FromSqlDuplicateParameterName(originalName));
+                }
+
+                dbParameter.ParameterName = uniquifiedName;
+            }
 
             return _processedDbParameters[dbParameter] = new RawRelationalParameter(dbParameter.ParameterName, dbParameter);
         }
     }
 
+    /// <summary>
+    ///     Returns whether the given composite format string (as passed to <see cref="string.Format(string, object[])" />) contains a
+    ///     placeholder for the argument at the given index, e.g. <c>{0}</c>, <c>{0,5}</c> or <c>{0:x}</c>. Escaped braces
+    ///     (<c>{{0}}</c>) are not placeholders.
+    /// </summary>
+    internal static bool ReferencesArgumentByPosition(string sql, int index)
+    {
+        for (var i = 0; i < sql.Length; i++)
+        {
+            if (sql[i] != '{')
+            {
+                continue;
+            }
+
+            // '{{' is an escaped brace
+            if (i + 1 < sql.Length && sql[i + 1] == '{')
+            {
+                i++;
+                continue;
+            }
+
+            var j = i + 1;
+            while (j < sql.Length && char.IsWhiteSpace(sql[j]))
+            {
+                j++;
+            }
+
+            var digitsStart = j;
+            while (j < sql.Length && sql[j] is >= '0' and <= '9')
+            {
+                j++;
+            }
+
+            if (j == digitsStart
+                || !int.TryParse(sql.AsSpan(digitsStart, j - digitsStart), NumberStyles.None, CultureInfo.InvariantCulture, out var value)
+                || value != index)
+            {
+                continue;
+            }
+
+            while (j < sql.Length && char.IsWhiteSpace(sql[j]))
+            {
+                j++;
+            }
+
+            if (j < sql.Length && sql[j] is ',' or ':' or '}')
+            {
+                return true;
+            }
+        }
+
+        return false;
+    }
+
     private string GenerateNewParameterName()
     {
         string name, prefixedName;

test/EFCore.Relational.Specification.Tests/Query/FromSqlQueryTestBase.cs

@@ -1330,6 +1330,191 @@ public virtual async Task Multiple_occurrences_of_FromSql_with_db_parameter_adds
         Assert.Empty(actual);
     }
 
+    // DbParameters with the same name in the same query need to be renamed to be unique. That's only possible when the SQL references
+    // the parameters positionally ({0}), since EF can't rewrite a name which is hard-coded in the SQL. Previously the second parameter
+    // was renamed, but the SQL kept referencing the first one, silently using the wrong value. See #32932.
+    [Theory, MemberData(nameof(IsAsyncData))]
+    public virtual async Task Union_of_FromSql_with_same_named_db_parameters_hardcoded_in_sql_throws(bool async)
+    {
+        using var context = CreateContext();
+
+        var query = FromSqlCity(context, "@city", CreateDbParameter("city", "Seattle"))
+            .Union(FromSqlCity(context, "@city", CreateDbParameter("city", "London")));
+
+        await AssertDuplicateParameterNameThrows(query, "city", async);
+    }
+
+    // Even if the values are the same, we don't try to detect that the parameters are equivalent and silently share them.
+    [Theory, MemberData(nameof(IsAsyncData))]
+    public virtual async Task Union_of_FromSql_with_same_named_same_valued_db_parameters_hardcoded_in_sql_throws(bool async)
+    {
+        using var context = CreateContext();
+
+        var query = FromSqlCity(context, "@city", CreateDbParameter("city", "Seattle"))
+            .Union(FromSqlCity(context, "@city", CreateDbParameter("city", "Seattle")));
+
+        await AssertDuplicateParameterNameThrows(query, "city", async);
+    }
+
+    [Theory, MemberData(nameof(IsAsyncData))]
+    public virtual async Task Union_of_FromSql_with_differently_named_db_parameters_hardcoded_in_sql(bool async)
+    {
+        using var context = CreateContext();
+
+        var query = FromSqlCity(context, "@city1", CreateDbParameter("city1", "Seattle"))
+            .Union(FromSqlCity(context, "@city2", CreateDbParameter("city2", "London")));
+
+        await AssertSeattleAndLondon(query, async);
+    }
+
+    // The first parameter keeps its name, which is hard-coded in the SQL. The second is renamed, which is fine since it's referenced
+    // positionally.
+    [Theory, MemberData(nameof(IsAsyncData))]
+    public virtual async Task Union_of_FromSql_with_same_named_db_parameters_hardcoded_then_positional(bool async)
+    {
+        using var context = CreateContext();
+
+        var query = FromSqlCity(context, "@city", CreateDbParameter("city", "Seattle"))
+            .Union(FromSqlCity(context, "{0}", CreateDbParameter("city", "London")));
+
+        await AssertSeattleAndLondon(query, async);
+    }
+
+    // The second parameter needs to be renamed, but it's hard-coded in the SQL.
+    [Theory, MemberData(nameof(IsAsyncData))]
+    public virtual async Task Union_of_FromSql_with_same_named_db_parameters_positional_then_hardcoded_throws(bool async)
+    {
+        using var context = CreateContext();
+
+        var query = FromSqlCity(context, "{0}", CreateDbParameter("city", "Seattle"))
+            .Union(FromSqlCity(context, "@city", CreateDbParameter("city", "London")));
+
+        await AssertDuplicateParameterNameThrows(query, "city", async);
+    }
+
+    [Theory, MemberData(nameof(IsAsyncData))]
+    public virtual async Task Union_of_FromSql_with_same_named_db_parameters_one_prefixed_hardcoded_in_sql_throws(bool async)
+    {
+        using var context = CreateContext();
+
+        var query = FromSqlCity(context, "@city", CreateDbParameter("@city", "Seattle"))
+            .Union(FromSqlCity(context, "@city", CreateDbParameter("city", "London")));
+
+        await AssertDuplicateParameterNameThrows(query, "city", async);
+    }
+
+    [Theory, MemberData(nameof(IsAsyncData))]
+    public virtual async Task Union_of_FromSql_with_db_parameters_differing_by_case_hardcoded_in_sql_throws(bool async)
+    {
+        using var context = CreateContext();
+
+        var query = FromSqlCity(context, "@city", CreateDbParameter("city", "Seattle"))
+            .Union(FromSqlCity(context, "@City", CreateDbParameter("City", "London")));
+
+        await AssertDuplicateParameterNameThrows(query, "City", async);
+    }
+
+    // A parameter which isn't a DbParameter gets a generated name (p0), which then conflicts with the hard-coded DbParameter name.
+    [Theory, MemberData(nameof(IsAsyncData))]
+    public virtual async Task Union_of_FromSql_with_db_parameter_name_conflicting_with_generated_name_hardcoded_in_sql_throws(bool async)
+    {
+        using var context = CreateContext();
+
+        var seattle = "Seattle";
+        var query = context.Set<Customer>()
+            .FromSqlRaw(NormalizeDelimitersInRawString("SELECT * FROM [Customers] WHERE [City] = {0}"), seattle)
+            .Union(FromSqlCity(context, "@p0", CreateDbParameter("p0", "London")));
+
+        await AssertDuplicateParameterNameThrows(query, "p0", async);
+    }
+
+    [Theory, MemberData(nameof(IsAsyncData))]
+    public virtual async Task FromSqlRaw_compiled_with_same_named_db_parameters_hardcoded_in_sql_throws(bool async)
+    {
+        var sql = NormalizeDelimitersInRawString("SELECT * FROM [Customers] WHERE [City] = @city");
+        var expectedMessage = RelationalStrings.FromSqlDuplicateParameterName("city");
+
+        // Throws on every execution, not just the first
+        for (var i = 0; i < 2; i++)
+        {
+            if (async)
+            {
+                var query = EF.CompileAsyncQuery((NorthwindContext context) => context.Set<Customer>()
+                    .FromSqlRaw(sql, CreateDbParameter("city", "Seattle"))
+                    .Union(context.Set<Customer>().FromSqlRaw(sql, CreateDbParameter("city", "London"))));
+
+                using var context = CreateContext();
+                var exception = await Assert.ThrowsAsync<InvalidOperationException>(async () => await query(context).ToListAsync());
+                Assert.Equal(expectedMessage, exception.Message);
+            }
+            else
+            {
+                var query = EF.CompileQuery((NorthwindContext context) => context.Set<Customer>()
+                    .FromSqlRaw(sql, CreateDbParameter("city", "Seattle"))
+                    .Union(context.Set<Customer>().FromSqlRaw(sql, CreateDbParameter("city", "London"))));
+
+                using var context = CreateContext();
+                var exception = Assert.Throws<InvalidOperationException>(() => query(context).ToList());
+                Assert.Equal(expectedMessage, exception.Message);
+            }
+        }
+    }
+
+    // Executing the same compiled query repeatedly, where the DbParameter names have to be uniquified, works every time.
+    [Theory, MemberData(nameof(IsAsyncData))]
+    public virtual async Task FromSqlRaw_compiled_with_same_named_db_parameters_positional_in_sql(bool async)
+    {
+        var sql = NormalizeDelimitersInRawString("SELECT * FROM [Customers] WHERE [City] = {0}");
+
+        for (var i = 0; i < 2; i++)
+        {
+            using var context = CreateContext();
+
+            if (async)
+            {
+                var query = EF.CompileAsyncQuery((NorthwindContext context) => context.Set<Customer>()
+                    .FromSqlRaw(sql, CreateDbParameter("city", "Seattle"))
+                    .Union(context.Set<Customer>().FromSqlRaw(sql, CreateDbParameter("city", "London"))));
+
+                Assert.Equal(SeattleAndLondonCustomerCount, (await query(context).ToListAsync()).Count);
+            }
+            else
+            {
+                var query = EF.CompileQuery((NorthwindContext context) => context.Set<Customer>()
+                    .FromSqlRaw(sql, CreateDbParameter("city", "Seattle"))
+                    .Union(context.Set<Customer>().FromSqlRaw(sql, CreateDbParameter("city", "London"))));
+
+                Assert.Equal(SeattleAndLondonCustomerCount, query(context).ToList().Count);
+            }
+        }
+    }
+
+    private IQueryable<Customer> FromSqlCity(NorthwindContext context, string cityReference, DbParameter parameter)
+        => context.Set<Customer>().FromSqlRaw(
+            NormalizeDelimitersInRawString("SELECT * FROM [Customers] WHERE [City] = " + cityReference),
+            parameter);
+
+    private static async Task AssertDuplicateParameterNameThrows(IQueryable<Customer> query, string parameterName, bool async)
+    {
+        var exception = async
+            ? await Assert.ThrowsAsync<InvalidOperationException>(() => query.ToListAsync())
+            : Assert.Throws<InvalidOperationException>(() => query.ToList());
+
+        Assert.Equal(RelationalStrings.FromSqlDuplicateParameterName(parameterName), exception.Message);
+    }
+
+    // Northwind has 1 customer in Seattle and 6 in London. A constant is used rather than a query to keep the logged SQL minimal.
+    private const int SeattleAndLondonCustomerCount = 7;
+
+    private static async Task AssertSeattleAndLondon(IQueryable<Customer> query, bool async)
+    {
+        var actual = async
+            ? await query.ToListAsync()
+            : query.ToList();
+
+        Assert.Equal(SeattleAndLondonCustomerCount, actual.Count);
+    }
+
     // The GroupBy followed by a non-reducing Select causes the base FromSql to get duplicated in the SQL, and so the same DbParameter
     // to get referenced from multiple FromSqlExpressions. Ensure we only process the DbParameter once. See #37409.
     [Theory, MemberData(nameof(IsAsyncData))]

test/EFCore.Relational.Specification.Tests/Query/SqlQueryTestBase.cs

@@ -1113,6 +1113,23 @@ WITH [Customers2] AS (
             elementSorter: e => e.CustomerID,
             elementAsserter: AssertUnmappedCustomers);
 
+    // See #32932.
+    [Theory, MemberData(nameof(IsAsyncData))]
+    public virtual async Task Union_of_SqlQuery_with_same_named_db_parameters_hardcoded_in_sql_throws(bool async)
+    {
+        using var context = CreateContext();
+
+        var sql = NormalizeDelimitersInRawString("SELECT * FROM [Customers] WHERE [City] = @city");
+        var query = context.Database.SqlQueryRaw<UnmappedCustomer>(sql, CreateDbParameter("city", "Seattle"))
+            .Union(context.Database.SqlQueryRaw<UnmappedCustomer>(sql, CreateDbParameter("city", "London")));
+
+        var exception = async
+            ? await Assert.ThrowsAsync<InvalidOperationException>(() => query.ToListAsync())
+            : Assert.Throws<InvalidOperationException>(() => query.ToList());
+
+        Assert.Equal(RelationalStrings.FromSqlDuplicateParameterName("city"), exception.Message);
+    }
+
     [Theory, MemberData(nameof(IsAsyncData))]
     public virtual async Task Multiple_occurrences_of_SqlQuery_with_db_parameter_adds_two_parameters(bool async)
     {