Merge branch 'main' into Unflaking

Author: AndriySvyryd

src/EFCore.SqlServer/Storage/Internal/SqlServerStringTypeMapping.cs

@@ -2,9 +2,7 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 
 using System.Data;
-using System.Data.SqlTypes;
 using System.Text;
-using System.Xml;
 using Microsoft.Data.SqlClient;
 using Microsoft.EntityFrameworkCore.Storage.Json;
 
@@ -21,15 +19,6 @@ public class SqlServerStringTypeMapping : StringTypeMapping
     private const int UnicodeMax = 4000;
     private const int AnsiMax = 8000;
 
-    // DTD processing is explicitly prohibited and no external resolver is used so that a malicious
-    // payload is rejected rather than processed.
-    private static readonly XmlReaderSettings XmlFragmentSettings = new()
-    {
-        ConformanceLevel = ConformanceLevel.Fragment,
-        DtdProcessing = DtdProcessing.Prohibit,
-        XmlResolver = null
-    };
-
     private static readonly CaseInsensitiveValueComparer CaseInsensitiveValueComparer = new();
 
     private readonly bool _isUtf16;
@@ -169,15 +158,17 @@ protected override void ConfigureParameter(DbParameter parameter)
 
         if (_sqlDbType == SqlDbType.Xml)
         {
-            // SqlClient only sends a parameter as 'xml' (rather than 'nvarchar(max)') when its value is a SqlXml.
-            // Sending it as 'xml' lets SQL Server honor any encoding declared in the XML prolog (e.g. utf-8), which
-            // otherwise fails with "unable to switch the encoding". A fragment-conformant reader is used so that the
-            // content forms that the 'xml' store type accepts - empty strings, text, and multiple top-level nodes -
-            // continue to round-trip.
+            // xml is a max-length type; pin the size to unbounded so SqlClient does not infer it from the
+            // value length (which would otherwise vary per value in logs and the query cache).
+            parameter.Size = -1;
+
+            // SqlClient sends a string parameter as 'nvarchar(max)' even when its SqlDbType is 'xml', which makes
+            // SQL Server reject an XML prolog that declares a non-UTF-16 encoding (e.g. utf-8) with "unable to switch
+            // the encoding". Only the XML declaration conflicts, and it can only appear at the very start, so it is
+            // sliced off and the rest of the value is sent verbatim.
             if (value is string xml)
             {
-                using var reader = XmlReader.Create(new StringReader(xml), XmlFragmentSettings);
-                parameter.Value = new SqlXml(reader);
+                parameter.Value = RemoveXmlProlog(xml);
             }
 
             return;
@@ -220,6 +211,30 @@ protected override void ConfigureParameter(DbParameter parameter)
         }
     }
 
+    private static string RemoveXmlProlog(string xml)
+    {
+        var start = 0;
+        while (start < xml.Length && char.IsWhiteSpace(xml[start]))
+        {
+            start++;
+        }
+
+        // An XML declaration starts with "<?xml" followed by mandatory whitespace. If present, remove everything
+        // up to and including the closing "?>" and return the remainder verbatim.
+        if (start + 5 < xml.Length
+            && string.CompareOrdinal(xml, start, "<?xml", 0, 5) == 0
+            && char.IsWhiteSpace(xml[start + 5]))
+        {
+            var end = xml.IndexOf("?>", start + 6, StringComparison.Ordinal);
+            if (end >= 0)
+            {
+                return xml[(end + 2)..];
+            }
+        }
+
+        return xml;
+    }
+
     /// <summary>
     ///     This is an internal API that supports the Entity Framework Core infrastructure and not subject to
     ///     the same compatibility standards as public APIs. It may be changed or removed without notice in

src/EFCore/Query/Internal/NavigationExpandingExpressionVisitor.SingleResultSubqueryLifting.cs

@@ -0,0 +1,108 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+namespace Microsoft.EntityFrameworkCore.Query.Internal;
+
+public partial class NavigationExpandingExpressionVisitor
+{
+    private NavigationExpansionExpression LiftSingleResultSubqueries(NavigationExpansionExpression source)
+    {
+        var selectorBody = source.PendingSelector;
+
+        var collector = new SingleResultMemberAccessCollector();
+        collector.Visit(selectorBody);
+
+        foreach (var subquery in collector.Liftable)
+        {
+            var collection = BuildSingleResultCollection(subquery);
+            var innerParameter = Expression.Parameter(collection.Type.GetSequenceType(), "e");
+            var rewrittenBody = new ReplacingExpressionVisitor([subquery], [innerParameter]).Visit(selectorBody);
+
+            // The collection already references the outer element via source.PendingSelector; this parameter is unused.
+            source = ProcessSelectMany(
+                source,
+                Expression.Lambda(collection, Expression.Parameter(source.SourceElementType, "o")),
+                Expression.Lambda(rewrittenBody, Expression.Parameter(source.SourceElementType, "o"), innerParameter));
+            selectorBody = source.PendingSelector;
+        }
+
+        return source;
+    }
+
+    private static Expression BuildSingleResultCollection(MethodCallExpression subqueryMethod)
+    {
+        var method = subqueryMethod.Method.GetGenericMethodDefinition();
+        var source = subqueryMethod.Arguments[0];
+        var elementType = source.Type.GetSequenceType();
+
+        Expression Where(Expression c)
+            => Expression.Call(QueryableMethods.Where.MakeGenericMethod(elementType), c, subqueryMethod.Arguments[1]);
+        Expression Skip(Expression c)
+            => Expression.Call(QueryableMethods.Skip.MakeGenericMethod(elementType), c, subqueryMethod.Arguments[1]);
+        Expression Reverse(Expression c)
+            => Expression.Call(QueryableMethods.Reverse.MakeGenericMethod(elementType), c);
+
+        var oneRow = method switch
+        {
+            _ when method == QueryableMethods.FirstWithPredicate || method == QueryableMethods.FirstOrDefaultWithPredicate
+                || method == QueryableMethods.SingleWithPredicate || method == QueryableMethods.SingleOrDefaultWithPredicate
+                => Where(source),
+            _ when method == QueryableMethods.LastWithPredicate || method == QueryableMethods.LastOrDefaultWithPredicate
+                => Reverse(Where(source)),
+            _ when method == QueryableMethods.LastWithoutPredicate || method == QueryableMethods.LastOrDefaultWithoutPredicate
+                => Reverse(source),
+            _ when method == QueryableMethods.ElementAt || method == QueryableMethods.ElementAtOrDefault
+                => Skip(source),
+            _ => source
+        };
+
+        var firstRow = Expression.Call(QueryableMethods.Take.MakeGenericMethod(elementType), oneRow, Expression.Constant(1));
+
+        return Expression.Call(QueryableMethods.DefaultIfEmptyWithoutArgument.MakeGenericMethod(elementType), firstRow);
+    }
+
+    private sealed class SingleResultMemberAccessCollector : ExpressionVisitor
+    {
+        private static readonly HashSet<MethodInfo> SingleResultMethods =
+        [
+            QueryableMethods.FirstWithPredicate, QueryableMethods.FirstWithoutPredicate,
+            QueryableMethods.FirstOrDefaultWithPredicate, QueryableMethods.FirstOrDefaultWithoutPredicate,
+            QueryableMethods.SingleWithPredicate, QueryableMethods.SingleWithoutPredicate,
+            QueryableMethods.SingleOrDefaultWithPredicate, QueryableMethods.SingleOrDefaultWithoutPredicate,
+            QueryableMethods.LastWithPredicate, QueryableMethods.LastWithoutPredicate,
+            QueryableMethods.LastOrDefaultWithPredicate, QueryableMethods.LastOrDefaultWithoutPredicate,
+            QueryableMethods.ElementAt, QueryableMethods.ElementAtOrDefault
+        ];
+
+        private readonly Dictionary<MethodCallExpression, int> _memberAccessCount = new(ReferenceEqualityComparer.Instance);
+
+        public IEnumerable<MethodCallExpression> Liftable
+            => _memberAccessCount.Where(e => e.Value > 1).Select(e => e.Key);
+
+        protected override Expression VisitMember(MemberExpression memberExpression)
+        {
+            if (memberExpression.Expression is MethodCallExpression { Method.IsGenericMethod: true } subquery
+                && SingleResultMethods.Contains(subquery.Method.GetGenericMethodDefinition()))
+            {
+                if (_memberAccessCount.TryGetValue(subquery, out var count))
+                {
+                    _memberAccessCount[subquery] = count + 1;
+                }
+                else
+                {
+                    // First encounter: count it and visit its arguments once (a later access duplicates the count)
+                    _memberAccessCount[subquery] = 1;
+
+                    foreach (var argument in subquery.Arguments)
+                    {
+                        Visit(argument);
+                    }
+                }
+
+                return memberExpression;
+            }
+
+            return base.VisitMember(memberExpression);
+        }
+    }
+}

src/EFCore/Query/Internal/NavigationExpandingExpressionVisitor.cs

@@ -671,9 +671,10 @@ when QueryableMethods.IsSumWithSelector(method):
 
                     case nameof(Queryable.Select)
                         when genericMethod == QueryableMethods.Select:
-                        return ProcessSelect(
-                            source,
-                            methodCallExpression.Arguments[1].UnwrapLambdaFromQuote());
+                        return LiftSingleResultSubqueries(
+                            ProcessSelect(
+                                source,
+                                methodCallExpression.Arguments[1].UnwrapLambdaFromQuote()));
 
                     case nameof(Queryable.Where)
                         when genericMethod == QueryableMethods.Where:

test/EFCore.Cosmos.FunctionalTests/Query/NorthwindSelectQueryCosmosTest.cs

@@ -695,6 +695,13 @@ public override async Task Projection_in_a_subquery_should_be_liftable(bool asyn
     public override Task Projection_containing_DateTime_subtraction(bool async)
         => Assert.ThrowsAsync<InvalidOperationException>(() => base.Projection_containing_DateTime_subtraction(async));
 
+    public override async Task Multiple_members_of_correlated_single_result_subquery_lift_to_single_join(bool async, string method)
+    {
+        await AssertTranslationFailed(() => base.Multiple_members_of_correlated_single_result_subquery_lift_to_single_join(async, method));
+
+        AssertSql();
+    }
+
     public override async Task Project_single_element_from_collection_with_OrderBy_Take_and_FirstOrDefault(bool async)
     {
         // Cosmos client evaluation. Issue #17246.

test/EFCore.Specification.Tests/Query/NorthwindSelectQueryTestBase.cs

@@ -2303,4 +2303,55 @@ from o2 in ss.Set<Order>()
                 }).Take(5),
             assertOrder: true,
             elementAsserter: (e, a) => AssertCollection(e.OrderIds, a.OrderIds, elementSorter: ee => ee));
+
+    public static TheoryData<bool, string> SingleResultMethodData()
+        => new(
+            from async in new[] { true, false }
+            from method in new[]
+            {
+                nameof(Queryable.First), nameof(Queryable.FirstOrDefault),
+                nameof(Queryable.Single), nameof(Queryable.SingleOrDefault),
+                nameof(Queryable.Last), nameof(Queryable.LastOrDefault),
+                nameof(Queryable.ElementAt), nameof(Queryable.ElementAtOrDefault)
+            }
+            select (async, method));
+
+    [Theory, MemberData(nameof(SingleResultMethodData))]
+    public virtual Task Multiple_members_of_correlated_single_result_subquery_lift_to_single_join(bool async, string method)
+        => AssertQuery(
+            async,
+            ss =>
+            {
+                var customers = ss.Set<Customer>();
+                var orders = ss.Set<Order>().Where(o => o.CustomerID != null);
+                return method switch
+                {
+                    nameof(Queryable.First) => from o in orders
+                        let c = customers.First(c => c.CustomerID == o.CustomerID)
+                        select new { o.OrderID, c.City, c.Country, c.ContactName },
+                    nameof(Queryable.FirstOrDefault) => from o in orders
+                        let c = customers.FirstOrDefault(c => c.CustomerID == o.CustomerID)
+                        select new { o.OrderID, c.City, c.Country, c.ContactName },
+                    nameof(Queryable.Single) => from o in orders
+                        let c = customers.Single(c => c.CustomerID == o.CustomerID)
+                        select new { o.OrderID, c.City, c.Country, c.ContactName },
+                    nameof(Queryable.SingleOrDefault) => from o in orders
+                        let c = customers.SingleOrDefault(c => c.CustomerID == o.CustomerID)
+                        select new { o.OrderID, c.City, c.Country, c.ContactName },
+                    nameof(Queryable.Last) => from o in orders
+                        let c = customers.OrderBy(c => c.CustomerID).Last(c => c.CustomerID == o.CustomerID)
+                        select new { o.OrderID, c.City, c.Country, c.ContactName },
+                    nameof(Queryable.LastOrDefault) => from o in orders
+                        let c = customers.OrderBy(c => c.CustomerID).LastOrDefault(c => c.CustomerID == o.CustomerID)
+                        select new { o.OrderID, c.City, c.Country, c.ContactName },
+                    nameof(Queryable.ElementAt) => from o in orders
+                        let c = customers.Where(c => c.CustomerID == o.CustomerID).OrderBy(c => c.CustomerID).ElementAt(0)
+                        select new { o.OrderID, c.City, c.Country, c.ContactName },
+                    nameof(Queryable.ElementAtOrDefault) => from o in orders
+                        let c = customers.Where(c => c.CustomerID == o.CustomerID).OrderBy(c => c.CustomerID).ElementAtOrDefault(0)
+                        select new { o.OrderID, c.City, c.Country, c.ContactName },
+                    _ => throw new InvalidOperationException(method)
+                };
+            },
+            elementSorter: e => e.OrderID);
 }

test/EFCore.SqlServer.FunctionalTests/BuiltInDataTypesSqlServerTest.cs

@@ -3820,20 +3820,31 @@ FROM INFORMATION_SCHEMA.COLUMNS
     // The grinning-face emoji is outside the BMP (a UTF-16 surrogate pair, four UTF-8 bytes) and the euro sign
     // is a single UTF-16 code unit but three UTF-8 bytes; both are represented differently in UTF-16 than in
     // UTF-8 and are lost when an xml value is sent to the server as a non-Unicode string, which makes them good
-    // probes for the SqlXml/SqlDbType.Xml parameter path.
+    // probes for the SqlDbType.Xml parameter path.
     private const string XmlEmoji = "\U0001F600";
     private const string XmlEuro = "\u20AC";
 
     [Theory]
-    [InlineData("<root>" + XmlEmoji + XmlEuro + "</root>", "<root>" + XmlEmoji + XmlEuro + "</root>")]
-    // An explicit non-UTF-16 prolog is accepted because the value is sent as 'xml', not 'nvarchar(max)'.
-    [InlineData("<?xml version=\"1.0\" encoding=\"utf-8\"?><root>" + XmlEmoji + "</root>", "<root>" + XmlEmoji + "</root>")]
-    [InlineData("<?xml version=\"1.0\" encoding=\"utf-16\"?><root>" + XmlEuro + "</root>", "<root>" + XmlEuro + "</root>")]
+    [InlineData(
+        "<root>" + XmlEmoji + XmlEuro + "</root>",
+        "<root>" + XmlEmoji + XmlEuro + "</root>",
+        "<root>" + XmlEmoji + XmlEuro + "</root>")]
+    // Only the XML declaration is removed; a following stylesheet PI and the rest of the value are sent verbatim.
+    [InlineData(
+        "<?xml version=\"1.0\" encoding=\"utf-8\" standalone='yes' ?> <?xml-stylesheet href=\"style.xsl\" type=\"text/xml\"?> <root>" + XmlEmoji + "</root>",
+        " <?xml-stylesheet href=\"style.xsl\" type=\"text/xml\"?> <root>" + XmlEmoji + "</root>",
+        "<?xml-stylesheet href=\"style.xsl\" type=\"text/xml\"?><root>" + XmlEmoji + "</root>")]
+    // The leading whitespace and the declaration are removed when the value is sent.
+    [InlineData(
+        " <?xml version=\"1.1\" encoding=\"utf-16\"?> <root>" + XmlEuro + "</root>",
+        " <root>" + XmlEuro + "</root>",
+        "<root>" + XmlEuro + "</root>")]
     // Content forms that the 'xml' store type accepts beyond a single well-formed document.
-    [InlineData("", "")]
-    [InlineData("text fragment", "text fragment")]
-    [InlineData("<a/><b/>", "<a /><b />")]
-    public async Task Xml_value_round_trips(string value, string expected)
+    [InlineData("", "", "")]
+    [InlineData("text fragment", "text fragment", "text fragment")]
+    // The content is sent verbatim, but the server expands self-closing tags when the xml column is read back.
+    [InlineData("<a/><b/>", "<a/><b/>", "<a /><b />")]
+    public async Task Xml_value_round_trips(string value, string expected, string roundTripped)
     {
         await using var context = CreateContext();
 
@@ -3845,7 +3856,7 @@ public async Task Xml_value_round_trips(string value, string expected)
         context.ChangeTracker.Clear();
 
         // xml columns cannot be compared directly in a WHERE clause, so the row is fetched by its key. Coalescing
-        // the column with the original value sends that value as an 'xml' parameter, exercising the SqlXml
+        // the column with the original value sends that value as an 'xml' parameter, exercising the prolog-removal
         // parameter path in a query in addition to the insert above.
         var query = context.Set<XmlTestDocument>()
             .Where(d => d.Id == id)
@@ -3859,15 +3870,16 @@ public async Task Xml_value_round_trips(string value, string expected)
 SELECT COALESCE([x].[Content], @value)
 FROM [XmlTestDocument] AS [x]
 WHERE [x].[Id] = @id
-""".ReplaceLineEndings(),
-            query.ToQueryString().ReplaceLineEndings());
+""",
+            query.ToQueryString(),
+            ignoreLineEndingDifferences: true);
 
-        var roundTripped = await query.SingleAsync();
-        Assert.Equal(expected, roundTripped);
+        var actual = await query.SingleAsync();
+        Assert.Equal(roundTripped, actual);
 
         AssertSql(
             $"""
-@p0='{expected}' (DbType = Xml)
+@p0='{expected}' (Size = -1) (DbType = Xml)
 
 SET IMPLICIT_TRANSACTIONS OFF;
 SET NOCOUNT ON;
@@ -3877,7 +3889,7 @@ OUTPUT INSERTED.[Id]
 """,
             //
             $"""
-@value='{expected}' (DbType = Xml)
+@value='{expected}' (Size = -1) (DbType = Xml)
 @id='{id}'
 
 SELECT TOP(2) COALESCE([x].[Content], @value)
@@ -3886,40 +3898,6 @@ FROM [XmlTestDocument] AS [x]
 """);
     }
 
-    [Fact]
-    public async Task Xml_value_with_dtd_payload_is_rejected()
-    {
-        await using var context = CreateContext();
-
-        // A "billion laughs" entity-expansion payload: the reader must reject the DTD rather than expand it.
-        const string maliciousXml =
-            "<?xml version=\"1.0\"?>"
-            + "<!DOCTYPE lolz [<!ENTITY lol \"lol\">"
-            + "<!ENTITY lol2 \"&lol;&lol;&lol;&lol;&lol;&lol;&lol;&lol;&lol;&lol;\">"
-            + "<!ENTITY lol3 \"&lol2;&lol2;&lol2;&lol2;&lol2;&lol2;&lol2;&lol2;&lol2;&lol2;\">]>"
-            + "<lolz>&lol3;</lolz>";
-
-        context.Add(new XmlTestDocument { Content = maliciousXml });
-
-        var exception = await Assert.ThrowsAnyAsync<Exception>(() => context.SaveChangesAsync());
-        Assert.True(
-            HasXmlException(exception),
-            $"Expected an {nameof(XmlException)} in the exception chain but found: {exception}");
-
-        static bool HasXmlException(Exception exception)
-        {
-            for (var current = exception; current is not null; current = current.InnerException)
-            {
-                if (current is XmlException)
-                {
-                    return true;
-                }
-            }
-
-            return false;
-        }
-    }
-
     private class XmlTestDocument
     {
         public int Id { get; set; }