From 0334a23c193a9689b03d7b35deb5fd167caec4c8 Mon Sep 17 00:00:00 2001 From: Arthur Vickers Date: Mon, 17 Aug 2020 12:48:28 -0700 Subject: [PATCH] Updated per code review comments Also added message for split query, as discussed in triage. --- .../Properties/CosmosStrings.Designer.cs | 8 +- .../Properties/CosmosStrings.resx | 2 +- ...ssionVisitor.ReadItemQueryingEnumerable.cs | 213 +++++++++--------- .../Properties/RelationalStrings.Designer.cs | 8 +- .../Properties/RelationalStrings.resx | 57 ++--- .../Query/Internal/SplitQueryingEnumerable.cs | 2 +- .../NorthwindIncludeQuerySqlServerTest.cs | 14 ++ ...NorthwindSplitIncludeQuerySqlServerTest.cs | 15 +- 8 files changed, 182 insertions(+), 137 deletions(-) diff --git a/src/EFCore.Cosmos/Properties/CosmosStrings.Designer.cs b/src/EFCore.Cosmos/Properties/CosmosStrings.Designer.cs index e65fa606f13..5db2c84bfc3 100644 --- a/src/EFCore.Cosmos/Properties/CosmosStrings.Designer.cs +++ b/src/EFCore.Cosmos/Properties/CosmosStrings.Designer.cs @@ -25,10 +25,12 @@ public static string CosmosNotInUse => GetString("CosmosNotInUse"); /// - /// There is no string-based representation of this query. + /// There is no string-based representation of this query as it's executed using 'ReadItemQueryAsync({resourceId}, {partitionKey})'. /// - public static string NoReadItemQueryString - => GetString("NoReadItemQueryString"); + public static string NoReadItemQueryString([CanBeNull] object resourceId, [CanBeNull] object partitionKey) + => string.Format( + GetString("DuplicateDiscriminatorValue", nameof(resourceId), nameof(partitionKey)), + resourceId, partitionKey); /// /// The discriminator value for '{entityType1}' is '{discriminatorValue}' which is the same for '{entityType2}'. Every concrete entity type mapped to the container '{container}' needs to have a unique discriminator value. diff --git a/src/EFCore.Cosmos/Properties/CosmosStrings.resx b/src/EFCore.Cosmos/Properties/CosmosStrings.resx index f366e28d0af..983b08e45c5 100644 --- a/src/EFCore.Cosmos/Properties/CosmosStrings.resx +++ b/src/EFCore.Cosmos/Properties/CosmosStrings.resx @@ -121,7 +121,7 @@ Cosmos-specific methods can only be used when the context is using the Cosmos provider. - There is no string-based representation of this query. + There is no string-based representation of this query as it's executed using 'ReadItemQueryAsync({resourceId}, {partitionKey})'. The discriminator value for '{entityType1}' is '{discriminatorValue}' which is the same for '{entityType2}'. Every concrete entity type mapped to the container '{container}' needs to have a unique discriminator value. diff --git a/src/EFCore.Cosmos/Query/Internal/CosmosShapedQueryCompilingExpressionVisitor.ReadItemQueryingEnumerable.cs b/src/EFCore.Cosmos/Query/Internal/CosmosShapedQueryCompilingExpressionVisitor.ReadItemQueryingEnumerable.cs index 2847a9c78be..eb9864d64e2 100644 --- a/src/EFCore.Cosmos/Query/Internal/CosmosShapedQueryCompilingExpressionVisitor.ReadItemQueryingEnumerable.cs +++ b/src/EFCore.Cosmos/Query/Internal/CosmosShapedQueryCompilingExpressionVisitor.ReadItemQueryingEnumerable.cs @@ -59,7 +59,110 @@ public IAsyncEnumerator GetAsyncEnumerator(CancellationToken cancellationToke IEnumerator IEnumerable.GetEnumerator() => GetEnumerator(); public string ToQueryString() - => CosmosStrings.NoReadItemQueryString; + { + TryGetResourceId(out var resourceId); + TryGetPartitionId(out var partitionKey); + return CosmosStrings.NoReadItemQueryString(resourceId, partitionKey); + } + + private bool TryGetPartitionId(out string partitionKey) + { + partitionKey = null; + + var partitionKeyPropertyName = _readItemExpression.EntityType.GetPartitionKeyPropertyName(); + if (partitionKeyPropertyName == null) + { + return true; + } + + var partitionKeyProperty = _readItemExpression.EntityType.FindProperty(partitionKeyPropertyName); + + if (TryGetParameterValue(partitionKeyProperty, out var value)) + { + partitionKey = GetString(partitionKeyProperty, value); + + return !string.IsNullOrEmpty(partitionKey); + } + + return false; + } + + private bool TryGetResourceId(out string resourceId) + { + var idProperty = _readItemExpression.EntityType.GetProperties() + .FirstOrDefault(p => p.GetJsonPropertyName() == StoreKeyConvention.IdPropertyJsonName); + + if (TryGetParameterValue(idProperty, out var value)) + { + resourceId = GetString(idProperty, value); + + if (string.IsNullOrEmpty(resourceId)) + { + throw new InvalidOperationException(CosmosStrings.InvalidResourceId); + } + + return true; + } + + if (TryGenerateIdFromKeys(idProperty, out var generatedValue)) + { + resourceId = GetString(idProperty, generatedValue); + + return true; + } + + resourceId = null; + return false; + } + + private bool TryGetParameterValue(IProperty property, out object value) + { + value = null; + return _readItemExpression.PropertyParameters.TryGetValue(property, out var parameterName) + && _cosmosQueryContext.ParameterValues.TryGetValue(parameterName, out value); + } + + private static string GetString(IProperty property, object value) + { + var converter = property.GetTypeMapping().Converter; + + return converter is null + ? (string)value + : (string)converter.ConvertToProvider(value); + } + + private bool TryGenerateIdFromKeys(IProperty idProperty, out object value) + { + var entityEntry = Activator.CreateInstance(_readItemExpression.EntityType.ClrType); + +#pragma warning disable EF1001 // Internal EF Core API usage. + var internalEntityEntry = new InternalEntityEntryFactory().Create( + _cosmosQueryContext.Context.GetDependencies().StateManager, _readItemExpression.EntityType, entityEntry); +#pragma warning restore EF1001 // Internal EF Core API usage. + + foreach (var keyProperty in _readItemExpression.EntityType.FindPrimaryKey().Properties) + { + var property = _readItemExpression.EntityType.FindProperty(keyProperty.Name); + + if (TryGetParameterValue(property, out var parameterValue)) + { +#pragma warning disable EF1001 // Internal EF Core API usage. + internalEntityEntry[property] = parameterValue; +#pragma warning restore EF1001 // Internal EF Core API usage. + } + } + +#pragma warning disable EF1001 // Internal EF Core API usage. + internalEntityEntry.SetEntityState(EntityState.Added); + + value = internalEntityEntry[idProperty]; + + internalEntityEntry.SetEntityState(EntityState.Detached); +#pragma warning restore EF1001 // Internal EF Core API usage. + + return value != null; + } + private sealed class Enumerator : IEnumerator, IAsyncEnumerator { @@ -69,6 +172,7 @@ private sealed class Enumerator : IEnumerator, IAsyncEnumerator private readonly Type _contextType; private readonly IDiagnosticsLogger _queryLogger; private readonly bool _standAloneStateManager; + private readonly ReadItemQueryingEnumerable _readItemEnumerable; private readonly CancellationToken _cancellationToken; private JObject _item; @@ -82,6 +186,7 @@ public Enumerator(ReadItemQueryingEnumerable readItemEnumerable, Cancellation _contextType = readItemEnumerable._contextType; _queryLogger = readItemEnumerable._queryLogger; _standAloneStateManager = readItemEnumerable._standAloneStateManager; + _readItemEnumerable = readItemEnumerable; _cancellationToken = cancellationToken; } @@ -97,12 +202,12 @@ public bool MoveNext() { if (!_hasExecuted) { - if (!TryGetResourceId(out var resourceId)) + if (!_readItemEnumerable.TryGetResourceId(out var resourceId)) { throw new InvalidOperationException(CosmosStrings.ResourceIdMissing); } - if (!TryGetPartitionId(out var partitionKey)) + if (!_readItemEnumerable.TryGetPartitionId(out var partitionKey)) { throw new InvalidOperationException(CosmosStrings.ParitionKeyMissing); } @@ -137,12 +242,12 @@ public async ValueTask MoveNextAsync() if (!_hasExecuted) { - if (!TryGetResourceId(out var resourceId)) + if (!_readItemEnumerable.TryGetResourceId(out var resourceId)) { throw new InvalidOperationException(CosmosStrings.ResourceIdMissing); } - if (!TryGetPartitionId(out var partitionKey)) + if (!_readItemEnumerable.TryGetPartitionId(out var partitionKey)) { throw new InvalidOperationException(CosmosStrings.ParitionKeyMissing); } @@ -200,104 +305,6 @@ private bool ShapeResult() return hasNext; } - - private bool TryGetPartitionId(out string partitionKey) - { - partitionKey = null; - - var partitionKeyPropertyName = _readItemExpression.EntityType.GetPartitionKeyPropertyName(); - if (partitionKeyPropertyName == null) - { - return true; - } - - var partitionKeyProperty = _readItemExpression.EntityType.FindProperty(partitionKeyPropertyName); - - if (TryGetParameterValue(partitionKeyProperty, out var value)) - { - partitionKey = GetString(partitionKeyProperty, value); - - return !string.IsNullOrEmpty(partitionKey); - } - - return false; - } - - private bool TryGetResourceId(out string resourceId) - { - var idProperty = _readItemExpression.EntityType.GetProperties() - .FirstOrDefault(p => p.GetJsonPropertyName() == StoreKeyConvention.IdPropertyJsonName); - - if (TryGetParameterValue(idProperty, out var value)) - { - resourceId = GetString(idProperty, value); - - if (string.IsNullOrEmpty(resourceId)) - { - throw new InvalidOperationException(CosmosStrings.InvalidResourceId); - } - - return true; - } - - if (TryGenerateIdFromKeys(idProperty, out var generatedValue)) - { - resourceId = GetString(idProperty, generatedValue); - - return true; - } - - resourceId = null; - return false; - } - - private bool TryGenerateIdFromKeys(IProperty idProperty, out object value) - { - var entityEntry = Activator.CreateInstance(_readItemExpression.EntityType.ClrType); - -#pragma warning disable EF1001 // Internal EF Core API usage. - var internalEntityEntry = new InternalEntityEntryFactory().Create( - _cosmosQueryContext.Context.GetDependencies().StateManager, _readItemExpression.EntityType, entityEntry); -#pragma warning restore EF1001 // Internal EF Core API usage. - - foreach (var keyProperty in _readItemExpression.EntityType.FindPrimaryKey().Properties) - { - var property = _readItemExpression.EntityType.FindProperty(keyProperty.Name); - - if (TryGetParameterValue(property, out var parameterValue)) - { -#pragma warning disable EF1001 // Internal EF Core API usage. - internalEntityEntry[property] = parameterValue; -#pragma warning restore EF1001 // Internal EF Core API usage. - } - } - -#pragma warning disable EF1001 // Internal EF Core API usage. - internalEntityEntry.SetEntityState(EntityState.Added); - - value = internalEntityEntry[idProperty]; - - internalEntityEntry.SetEntityState(EntityState.Detached); -#pragma warning restore EF1001 // Internal EF Core API usage. - - return value != null; - } - - private bool TryGetParameterValue(IProperty property, out object value) - { - value = null; - return _readItemExpression.PropertyParameters.TryGetValue(property, out var parameterName) - && _cosmosQueryContext.ParameterValues.TryGetValue(parameterName, out value); - } - - private static string GetString(IProperty property, object value) - { - var converter = property.GetTypeMapping().Converter; - - return converter is null - ? (string)value - : (string)converter.ConvertToProvider(value); - } } } } diff --git a/src/EFCore.Relational/Properties/RelationalStrings.Designer.cs b/src/EFCore.Relational/Properties/RelationalStrings.Designer.cs index dc7a49639d4..ecebe72ecc0 100644 --- a/src/EFCore.Relational/Properties/RelationalStrings.Designer.cs +++ b/src/EFCore.Relational/Properties/RelationalStrings.Designer.cs @@ -1,4 +1,4 @@ -// +// using System; using System.Reflection; @@ -38,6 +38,12 @@ public static string ModificationCommandInvalidEntityState([CanBeNull] object en public static string NoDbCommand => GetString("NoDbCommand"); + /// + /// This LINQ query is being executed in split-query mode. The SQL shown is for the first query to be executed. Additional queries may also be executed depending on the results of the first query. + /// + public static string SplitQueryString + => GetString("SplitQueryString"); + /// /// The 'DbConnection' is currently in use. The connection can only be changed when the existing connection is not being used. /// diff --git a/src/EFCore.Relational/Properties/RelationalStrings.resx b/src/EFCore.Relational/Properties/RelationalStrings.resx index 8fbec4c83e3..2cd266244fd 100644 --- a/src/EFCore.Relational/Properties/RelationalStrings.resx +++ b/src/EFCore.Relational/Properties/RelationalStrings.resx @@ -1,17 +1,17 @@  - @@ -123,6 +123,9 @@ Cannot create a 'DbCommand' for a non-relational query. + + This LINQ query is being executed in split-query mode. The SQL shown is for the first query to be executed. Additional queries may also be executed depending on the results of the first query. + The 'DbConnection' is currently in use. The connection can only be changed when the existing connection is not being used. diff --git a/src/EFCore.Relational/Query/Internal/SplitQueryingEnumerable.cs b/src/EFCore.Relational/Query/Internal/SplitQueryingEnumerable.cs index 114e2ade21f..3016e71cf55 100644 --- a/src/EFCore.Relational/Query/Internal/SplitQueryingEnumerable.cs +++ b/src/EFCore.Relational/Query/Internal/SplitQueryingEnumerable.cs @@ -111,7 +111,7 @@ public virtual DbCommand CreateDbCommand() /// doing so can result in application failures when updating to a new Entity Framework Core release. /// public virtual string ToQueryString() - => _relationalQueryContext.RelationalQueryStringFactory.Create(CreateDbCommand()); + => $"{_relationalQueryContext.RelationalQueryStringFactory.Create(CreateDbCommand())}{Environment.NewLine}{Environment.NewLine}{RelationalStrings.SplitQueryString}"; private sealed class Enumerator : IEnumerator { diff --git a/test/EFCore.SqlServer.FunctionalTests/Query/NorthwindIncludeQuerySqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/Query/NorthwindIncludeQuerySqlServerTest.cs index 387159e1d6c..36168657b0f 100644 --- a/test/EFCore.SqlServer.FunctionalTests/Query/NorthwindIncludeQuerySqlServerTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/Query/NorthwindIncludeQuerySqlServerTest.cs @@ -2,7 +2,9 @@ // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using System.Threading.Tasks; +using Microsoft.EntityFrameworkCore.TestModels.Northwind; using Microsoft.EntityFrameworkCore.TestUtilities; +using Xunit; using Xunit.Abstractions; namespace Microsoft.EntityFrameworkCore.Query @@ -146,6 +148,18 @@ FROM [Orders] AS [o] LEFT JOIN [Customers] AS [c] ON [o].[CustomerID] = [c].[CustomerID] LEFT JOIN [Order Details] AS [o0] ON [o].[OrderID] = [o0].[OrderID] ORDER BY [o].[OrderID], [c].[CustomerID], [o0].[OrderID], [o0].[ProductID]"); + + using var context = CreateContext(); + + Assert.Equal( + @"SELECT [o].[OrderID], [o].[CustomerID], [o].[EmployeeID], [o].[OrderDate], [c].[CustomerID], [c].[Address], [c].[City], [c].[CompanyName], [c].[ContactName], [c].[ContactTitle], [c].[Country], [c].[Fax], [c].[Phone], [c].[PostalCode], [c].[Region], [o0].[OrderID], [o0].[ProductID], [o0].[Discount], [o0].[Quantity], [o0].[UnitPrice] +FROM [Orders] AS [o] +LEFT JOIN [Customers] AS [c] ON [o].[CustomerID] = [c].[CustomerID] +LEFT JOIN [Order Details] AS [o0] ON [o].[OrderID] = [o0].[OrderID] +ORDER BY [o].[OrderID], [c].[CustomerID], [o0].[OrderID], [o0].[ProductID]", + context.Set().Include(o => o.Customer).Include(o => o.OrderDetails).ToQueryString(), + ignoreLineEndingDifferences: true, + ignoreWhiteSpaceDifferences: true); } public override async Task Include_references_multi_level(bool async) diff --git a/test/EFCore.SqlServer.FunctionalTests/Query/NorthwindSplitIncludeQuerySqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/Query/NorthwindSplitIncludeQuerySqlServerTest.cs index 30fc4f0d87c..92bbef47ec6 100644 --- a/test/EFCore.SqlServer.FunctionalTests/Query/NorthwindSplitIncludeQuerySqlServerTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/Query/NorthwindSplitIncludeQuerySqlServerTest.cs @@ -180,6 +180,19 @@ FROM [Orders] AS [o] LEFT JOIN [Customers] AS [c] ON [o].[CustomerID] = [c].[CustomerID] INNER JOIN [Order Details] AS [o0] ON [o].[OrderID] = [o0].[OrderID] ORDER BY [o].[OrderID], [c].[CustomerID]"); + + using var context = CreateContext(); + + Assert.Equal( + @"SELECT [o].[OrderID], [o].[CustomerID], [o].[EmployeeID], [o].[OrderDate], [c].[CustomerID], [c].[Address], [c].[City], [c].[CompanyName], [c].[ContactName], [c].[ContactTitle], [c].[Country], [c].[Fax], [c].[Phone], [c].[PostalCode], [c].[Region] +FROM [Orders] AS [o] +LEFT JOIN [Customers] AS [c] ON [o].[CustomerID] = [c].[CustomerID] +ORDER BY [o].[OrderID], [c].[CustomerID] + +This LINQ query is being executed in split-query mode. The SQL shown is for the first query to be executed. Additional queries may also be executed depending on the results of the first query.", + context.Set().Include(o => o.Customer).Include(o => o.OrderDetails).AsSplitQuery().ToQueryString(), + ignoreLineEndingDifferences: true, + ignoreWhiteSpaceDifferences: true); } public override async Task Include_references_multi_level(bool async) @@ -1768,7 +1781,7 @@ public override Task Include_collection_with_last_no_orderby(bool async) async, ss => ss.Set().Include(c => c.Orders), entryCount: 8 - ), RelationalStrings.MissingOrderingInSqlExpression); + ), RelationalStrings.MissingOrderingInSqlExpression); } private void AssertSql(params string[] expected)