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

Fix to #20609 - Includes() with string throws not finding navigation path exception when type T is derived #20630

Merged
merged 1 commit into from
Apr 17, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions src/EFCore/Diagnostics/CoreEventId.cs
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ private enum Id
QueryExecutionPlanned,
PossibleUnintendedCollectionNavigationNullComparisonWarning,
PossibleUnintendedReferenceComparisonWarning,
InvalidIncludePathError,

// Infrastructure events
SensitiveDataLoggingEnabledWarning = CoreBaseId + 400,
Expand Down Expand Up @@ -196,6 +197,20 @@ public static readonly EventId PossibleUnintendedCollectionNavigationNullCompari
public static readonly EventId PossibleUnintendedReferenceComparisonWarning
= MakeQueryId(Id.PossibleUnintendedReferenceComparisonWarning);

/// <summary>
/// <para>
/// Invalid include path '{navigationChain}', couldn't find navigation for '{navigationName}'.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"couldn't" -> "could not"?

/// </para>
/// <para>
/// This event is in the <see cref="DbLoggerCategory.Query" /> category.
/// </para>
/// <para>
/// This event uses the <see cref="InvalidIncludePathEventData" /> payload when used with a <see cref="DiagnosticSource" />.
/// </para>
/// </summary>
public static readonly EventId InvalidIncludePathError
= MakeQueryId(Id.InvalidIncludePathError);

private static readonly string _infraPrefix = DbLoggerCategory.Infrastructure.Name + ".";
private static EventId MakeInfraId(Id id) => new EventId((int)id, _infraPrefix + id);

Expand Down
33 changes: 32 additions & 1 deletion src/EFCore/Diagnostics/CoreLoggerExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -531,7 +531,7 @@ public static void PossibleUnintendedReferenceComparisonWarning(

if (diagnostics.ShouldLog(definition))
{
definition.Log(diagnostics,left, right);
definition.Log(diagnostics, left, right);
}

if (diagnostics.NeedsEventData(definition, out var diagnosticSourceEnabled, out var simpleLogEnabled))
Expand All @@ -553,6 +553,37 @@ private static string PossibleUnintendedReferenceComparisonWarning(EventDefiniti
return d.GenerateMessage(p.Left, p.Right);
}

public static void InvalidIncludePathError(
[NotNull] this IDiagnosticsLogger<DbLoggerCategory.Query> diagnostics,
[NotNull] string navigationChain,
[NotNull] string navigationName)
{
var definition = CoreResources.LogInvalidIncludePath(diagnostics);
if (diagnostics.ShouldLog(definition))
{
definition.Log(diagnostics, navigationChain, navigationName);
}

if (diagnostics.NeedsEventData(definition, out var diagnosticSourceEnabled, out var simpleLogEnabled))
{
var eventData = new InvalidIncludePathEventData(
definition,
InvalidIncludePathError,
navigationChain,
navigationName);

diagnostics.DispatchEventData(definition, eventData, diagnosticSourceEnabled, simpleLogEnabled);
}
}

private static string InvalidIncludePathError(EventDefinitionBase definition, EventData payload)
{
var d = (EventDefinition<object, object>)definition;
var p = (InvalidIncludePathEventData)payload;

return d.GenerateMessage(p.NavigationChain, p.NavigationName);
}

/// <summary>
/// Logs for the <see cref="CoreEventId.ServiceProviderCreated" /> event.
/// </summary>
Expand Down
44 changes: 44 additions & 0 deletions src/EFCore/Diagnostics/Internal/InvalidIncludePathEventData.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Diagnostics;
using JetBrains.Annotations;

namespace Microsoft.EntityFrameworkCore.Diagnostics
{
/// <summary>
/// A <see cref="DiagnosticSource" /> event payload class for events that have
/// invalid include path information.
/// </summary>
public class InvalidIncludePathEventData : EventData
{
/// <summary>
/// Constructs the event payload.
/// </summary>
/// <param name="eventDefinition"> The event definition. </param>
/// <param name="messageGenerator"> A delegate that generates a log message for this event. </param>
/// <param name="navigationChain"> Navigation chain included to this point. </param>
/// <param name="navigationName"> The name of the invalid navigation. </param>
public InvalidIncludePathEventData(
[NotNull] EventDefinitionBase eventDefinition,
[NotNull] Func<EventDefinitionBase, EventData, string> messageGenerator,
[NotNull] string navigationChain,
[NotNull] string navigationName)
: base(eventDefinition, messageGenerator)
{
NavigationChain = navigationChain;
NavigationName = navigationName;
}

/// <summary>
/// Navigation chain included to this point.
/// </summary>
public virtual string NavigationChain { get; }

/// <summary>
/// The name of the invalid navigation.
/// </summary>
public virtual string NavigationName { get; }
}
}
9 changes: 9 additions & 0 deletions src/EFCore/Diagnostics/LoggingDefinitions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -376,6 +376,15 @@ public abstract class LoggingDefinitions
[EntityFrameworkInternal]
public EventDefinitionBase LogPossibleUnintendedReferenceComparison;

/// <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
/// any release. You should only use it directly in your code with extreme caution and knowing that
/// doing so can result in application failures when updating to a new Entity Framework Core release.
/// </summary>
[EntityFrameworkInternal]
public EventDefinitionBase LogInvalidIncludePath;

/// <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
Expand Down
3 changes: 2 additions & 1 deletion src/EFCore/Infrastructure/CoreOptionsExtension.cs
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,8 @@ private WarningsConfiguration _warningsConfiguration
= new WarningsConfiguration()
.TryWithExplicit(CoreEventId.ManyServiceProvidersCreatedWarning, WarningBehavior.Throw)
.TryWithExplicit(CoreEventId.LazyLoadOnDisposedContextWarning, WarningBehavior.Throw)
.TryWithExplicit(CoreEventId.DetachedLazyLoadingWarning, WarningBehavior.Throw);
.TryWithExplicit(CoreEventId.DetachedLazyLoadingWarning, WarningBehavior.Throw)
.TryWithExplicit(CoreEventId.InvalidIncludePathError, WarningBehavior.Throw);

/// <summary>
/// Creates a new set of options with everything set to default values.
Expand Down
24 changes: 24 additions & 0 deletions src/EFCore/Properties/CoreStrings.Designer.cs

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -789,7 +789,7 @@ private NavigationExpansionExpression ProcessInclude(NavigationExpansionExpressi

if (includeTreeNodes.Count == 0)
{
throw new InvalidOperationException(CoreStrings.InvalidIncludePath(navigationChain, navigationName));
_queryCompilationContext.Logger.InvalidIncludePathError(navigationChain, navigationName);
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5864,5 +5864,13 @@ public virtual async Task Null_parameter_name_works(bool isAsync)

Assert.Empty(result);
}

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task String_include_on_incorrect_property_throws(bool async)
{
return Assert.ThrowsAsync<InvalidOperationException>(
async () => await AssertQuery(async, ss => ss.Set<Customer>().Include("OrderDetails")));
}
}
}
69 changes: 69 additions & 0 deletions test/EFCore.SqlServer.FunctionalTests/Query/QueryBugsTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -7207,6 +7207,75 @@ private class MyModel20097 : IHaveId20097

#endregion

#region Issue20609

[ConditionalFact]
public virtual void Can_ignore_invalid_include_path_error()
{
using var context = CreateContext20609();
var result = context.Set<ClassA>().Include("SubB").ToList();
}

public class BaseClass
{
public string Id { get; set; }
}

public class ClassA : BaseClass
{
public SubA SubA { get; set; }
}

public class ClassB : BaseClass
{
public SubB SubB { get; set; }
}

public class SubA
{
public int Id { get; set; }
}

public class SubB
{
public int Id { get; set; }
}

private BugContext20609 CreateContext20609()
{
var testStore = SqlServerTestStore.CreateInitialized("QueryBugsTest", multipleActiveResultSets: true);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this the right way to do custom warnings config in qbt? @AndriySvyryd

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, but it's fine for now. Tracked by #17588

var options = Fixture.AddOptions(testStore.AddProviderOptions(new DbContextOptionsBuilder()))
.EnableDetailedErrors()
.EnableServiceProviderCaching(false)
.ConfigureWarnings(x => x.Ignore(CoreEventId.InvalidIncludePathError))
.Options;

var context = new BugContext20609(options);
context.Database.EnsureCreatedResiliently();

return context;
}

private class BugContext20609 : DbContext
{
public BugContext20609(DbContextOptions options)
: base(options)
{
}

public DbSet<BaseClass> BaseClasses { get; set; }
public DbSet<SubA> SubAs { get; set; }
public DbSet<SubB> SubBs { get; set; }

protected override void OnModelCreating(ModelBuilder modelBuilder)
{
modelBuilder.Entity<ClassA>().HasBaseType<BaseClass>().HasOne(x => x.SubA).WithMany();
modelBuilder.Entity<ClassB>().HasBaseType<BaseClass>().HasOne(x => x.SubB).WithMany();
}
}

#endregion

private DbContextOptions _options;

private SqlServerTestStore CreateTestStore<TContext>(
Expand Down