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

Make original value parameter check for sproc into a warning #29061

Merged
merged 1 commit into from
Sep 15, 2022
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.Relational/Diagnostics/RelationalEventId.cs
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,7 @@ private enum Id
ForeignKeyTpcPrincipalWarning,
TpcStoreGeneratedIdentityWarning,
KeyPropertiesNotMappedToTable,
StoredProcedureConcurrencyTokenNotMapped,

// Update events
BatchReadyForExecution = CoreEventId.RelationalBaseId + 700,
Expand Down Expand Up @@ -872,6 +873,20 @@ private static EventId MakeValidationId(Id id)
public static readonly EventId KeyPropertiesNotMappedToTable =
MakeValidationId(Id.KeyPropertiesNotMappedToTable);

/// <summary>
/// An entity type is mapped to the stored procedure with a concurrency token not mapped to any original value parameter.
/// </summary>
/// <remarks>
/// <para>
/// This event is in the <see cref="DbLoggerCategory.Model.Validation" /> category.
/// </para>
/// <para>
/// This event uses the <see cref="StoredProcedurePropertyEventData" /> payload when used with a <see cref="DiagnosticSource" />.
/// </para>
/// </remarks>
public static readonly EventId StoredProcedureConcurrencyTokenNotMapped =
MakeValidationId(Id.StoredProcedureConcurrencyTokenNotMapped);

/// <summary>
/// A foreign key specifies properties which don't map to the related tables.
/// </summary>
Expand Down
40 changes: 40 additions & 0 deletions src/EFCore.Relational/Diagnostics/RelationalLoggerExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -3122,6 +3122,46 @@ private static string OptionalDependentWithoutIdentifyingPropertyWarning(EventDe
return d.GenerateMessage(p.EntityType.DisplayName());
}

/// <summary>
/// Logs the <see cref="RelationalEventId.StoredProcedureConcurrencyTokenNotMapped" /> event.
/// </summary>
/// <param name="diagnostics">The diagnostics logger to use.</param>
/// <param name="entityType">The entity type that the stored procedure is mapped to.</param>
/// <param name="concurrencyProperty">The property which represents the concurrency token.</param>
/// <param name="storedProcedureName">The stored procedure name.</param>
public static void StoredProcedureConcurrencyTokenNotMapped(
this IDiagnosticsLogger<DbLoggerCategory.Model.Validation> diagnostics,
IEntityType entityType,
IProperty concurrencyProperty,
string storedProcedureName)
{
var definition = RelationalResources.LogStoredProcedureConcurrencyTokenNotMapped(diagnostics);

if (diagnostics.ShouldLog(definition))
{
definition.Log(diagnostics, entityType.DisplayName(), storedProcedureName, concurrencyProperty.Name);
}

if (diagnostics.NeedsEventData(definition, out var diagnosticSourceEnabled, out var simpleLogEnabled))
{
var eventData = new StoredProcedurePropertyEventData(
definition,
StoredProcedureConcurrencyTokenNotMapped,
entityType,
concurrencyProperty,
storedProcedureName);

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

private static string StoredProcedureConcurrencyTokenNotMapped(EventDefinitionBase definition, EventData payload)
{
var d = (EventDefinition<string, string, string>)definition;
var p = (StoredProcedurePropertyEventData)payload;
return d.GenerateMessage(p.EntityType.DisplayName(), p.StoredProcedureName, p.Property.Name);
}

/// <summary>
/// Logs for the <see cref="RelationalEventId.BatchExecutorFailedToRollbackToSavepoint" /> event.
/// </summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -286,6 +286,15 @@ public abstract class RelationalLoggingDefinitions : LoggingDefinitions
[EntityFrameworkInternal]
public EventDefinitionBase? LogPossibleUnintendedUseOfEquals;

/// <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? LogStoredProcedureConcurrencyTokenNotMapped;

/// <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
17 changes: 8 additions & 9 deletions src/EFCore.Relational/Infrastructure/RelationalModelValidator.cs
Original file line number Diff line number Diff line change
Expand Up @@ -221,23 +221,23 @@ protected virtual void ValidateStoredProcedures(
if (deleteStoredProcedure != null)
{
AddSproc(StoreObjectType.DeleteStoredProcedure, entityType, storedProcedures);
ValidateSproc(deleteStoredProcedure, mappingStrategy);
ValidateSproc(deleteStoredProcedure, mappingStrategy, logger);
sprocCount++;
}

var insertStoredProcedure = entityType.GetInsertStoredProcedure();
if (insertStoredProcedure != null)
{
AddSproc(StoreObjectType.InsertStoredProcedure, entityType, storedProcedures);
ValidateSproc(insertStoredProcedure, mappingStrategy);
ValidateSproc(insertStoredProcedure, mappingStrategy, logger);
sprocCount++;
}

var updateStoredProcedure = entityType.GetUpdateStoredProcedure();
if (updateStoredProcedure != null)
{
AddSproc(StoreObjectType.UpdateStoredProcedure, entityType, storedProcedures);
ValidateSproc(updateStoredProcedure, mappingStrategy);
ValidateSproc(updateStoredProcedure, mappingStrategy, logger);
sprocCount++;
}

Expand Down Expand Up @@ -288,7 +288,10 @@ static void AddSproc(
}
}

private static void ValidateSproc(IStoredProcedure sproc, string mappingStrategy)
private static void ValidateSproc(
IStoredProcedure sproc,
string mappingStrategy,
IDiagnosticsLogger<DbLoggerCategory.Model.Validation> logger)
{
var entityType = sproc.EntityType;
var storeObjectIdentifier = sproc.GetStoreIdentifier();
Expand Down Expand Up @@ -646,11 +649,7 @@ private static void ValidateSproc(IStoredProcedure sproc, string mappingStrategy

if (originalValueProperties.Values.FirstOrDefault(p => p.IsConcurrencyToken) is { } missedConcurrencyToken)
{
throw new InvalidOperationException(
RelationalStrings.StoredProcedureConcurrencyTokenNotMapped(
entityType.DisplayName(),
storeObjectIdentifier.DisplayName(),
missedConcurrencyToken.Name));
logger.StoredProcedureConcurrencyTokenNotMapped(entityType, missedConcurrencyToken, storeObjectIdentifier.DisplayName());
}

if (sproc.ResultColumns.Any(c => c != sproc.FindRowsAffectedResultColumn()))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -386,7 +386,8 @@ public static CoreOptionsExtension WithDefaultWarningConfiguration(CoreOptionsEx
.TryWithExplicit(RelationalEventId.AmbientTransactionWarning, WarningBehavior.Throw)
.TryWithExplicit(RelationalEventId.IndexPropertiesBothMappedAndNotMappedToTable, WarningBehavior.Throw)
.TryWithExplicit(RelationalEventId.IndexPropertiesMappedToNonOverlappingTables, WarningBehavior.Throw)
.TryWithExplicit(RelationalEventId.ForeignKeyPropertiesMappedToUnrelatedTables, WarningBehavior.Throw));
.TryWithExplicit(RelationalEventId.ForeignKeyPropertiesMappedToUnrelatedTables, WarningBehavior.Throw)
.TryWithExplicit(RelationalEventId.StoredProcedureConcurrencyTokenNotMapped, WarningBehavior.Throw));

/// <summary>
/// Information/metadata for a <see cref="RelationalOptionsExtension" />.
Expand Down
33 changes: 25 additions & 8 deletions src/EFCore.Relational/Properties/RelationalStrings.Designer.cs

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

7 changes: 4 additions & 3 deletions src/EFCore.Relational/Properties/RelationalStrings.resx
Original file line number Diff line number Diff line change
Expand Up @@ -769,6 +769,10 @@
<value>The entity type '{entityType}' is an optional dependent using table sharing without any required non shared property that could be used to identify whether the entity exists. If all nullable properties contain a null value in database then an object instance won't be created in the query. Add a required property to create instances with null values for other properties or mark the incoming navigation as required to always create an instance.</value>
<comment>Warning RelationalEventId.OptionalDependentWithoutIdentifyingPropertyWarning string</comment>
</data>
<data name="LogStoredProcedureConcurrencyTokenNotMapped" xml:space="preserve">
<value>The entity type '{entityType}' is mapped to the stored procedure '{sproc}', but the concurrency token '{token}' is not mapped to any original value parameter.</value>
<comment>Warning RelationalEventId.StoredProcedureConcurrencyTokenNotMapped string string string</comment>
</data>
<data name="LogPossibleUnintendedUseOfEquals" xml:space="preserve">
<value>Possible unintended use of method 'Equals' for arguments '{left}' and '{right}' of different types in a query. This comparison will always return false.</value>
<comment>Warning RelationalEventId.QueryPossibleUnintendedUseOfEqualsWarning string string</comment>
Expand Down Expand Up @@ -976,9 +980,6 @@
<data name="SqlQueryUnmappedType" xml:space="preserve">
<value>The element type '{elementType}' used in 'SqlQuery' method is not natively supported by your database provider. Either use a supported element type, or use ModelConfigurationBuilder.DefaultTypeMapping to define a mapping for your type.</value>
</data>
<data name="StoredProcedureConcurrencyTokenNotMapped" xml:space="preserve">
<value>The entity type '{entityType}' is mapped to the stored procedure '{sproc}', but the concurrency token '{token}' is not mapped to any original value parameter.</value>
</data>
<data name="StoredProcedureCurrentValueParameterOnDelete" xml:space="preserve">
<value>Current value parameter '{parameter}' is not allowed on delete stored procedure '{sproc}'. Use HasOriginalValueParameter() instead.</value>
</data>
Expand Down
43 changes: 43 additions & 0 deletions src/EFCore/Diagnostics/StoredProcedurePropertyEventData.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
// 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.Diagnostics;

/// <summary>
/// A <see cref="DiagnosticSource" /> event payload class for events that have involving mapping of a property to a stored procedure.
/// </summary>
/// <remarks>
/// See <see href="https://aka.ms/efcore-docs-diagnostics">Logging, events, and diagnostics</see> for more information and examples.
/// </remarks>
public class StoredProcedurePropertyEventData : PropertyEventData
{
/// <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="entityType">The entity type that the stored procedure is mapped to.</param>
/// <param name="property">The property.</param>
/// <param name="storedProcedureName">The stored procedure name.</param>
public StoredProcedurePropertyEventData(
EventDefinitionBase eventDefinition,
Func<EventDefinitionBase, EventData, string> messageGenerator,
IEntityType entityType,
IProperty property,
string storedProcedureName)
: base(eventDefinition, messageGenerator, property)
{
EntityType = entityType;
StoredProcedureName = storedProcedureName;
}

/// <summary>
/// The entity type that the stored procedure is mapped to.
/// </summary>
public virtual IEntityType EntityType { get; }

/// <summary>
/// The stored procedure name.
/// </summary>
public virtual string StoredProcedureName { get; }
}
Original file line number Diff line number Diff line change
Expand Up @@ -2960,9 +2960,9 @@ public virtual void Detects_unmapped_concurrency_token()
.HasRowsAffectedReturnValue())
.Property(a => a.Name).IsRowVersion();

VerifyError(
RelationalStrings.StoredProcedureConcurrencyTokenNotMapped(nameof(Animal), "Animal_Update", nameof(Animal.Name)),
modelBuilder);
VerifyWarning(
RelationalResources.LogStoredProcedureConcurrencyTokenNotMapped(new TestLogger<TestRelationalLoggingDefinitions>())
.GenerateMessage(nameof(Animal), "Animal_Update", nameof(Animal.Name)), modelBuilder);
}

[ConditionalFact]
Expand Down
39 changes: 39 additions & 0 deletions test/EFCore.SqlServer.FunctionalTests/LoggingSqlServerTest.cs
Original file line number Diff line number Diff line change
@@ -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 Microsoft.EntityFrameworkCore.Diagnostics.Internal;
using Microsoft.EntityFrameworkCore.SqlServer.Diagnostics.Internal;
using Microsoft.EntityFrameworkCore.SqlServer.Infrastructure.Internal;

Expand All @@ -9,6 +10,44 @@ namespace Microsoft.EntityFrameworkCore;

public class LoggingSqlServerTest : LoggingRelationalTestBase<SqlServerDbContextOptionsBuilder, SqlServerOptionsExtension>
{
[ConditionalFact]
public virtual void StoredProcedureConcurrencyTokenNotMapped_throws_by_default()
{
using var context = new StoredProcedureConcurrencyTokenNotMappedContext(CreateOptionsBuilder(new ServiceCollection()));

var definition = RelationalResources.LogStoredProcedureConcurrencyTokenNotMapped(CreateTestLogger());
Assert.Equal(
CoreStrings.WarningAsErrorTemplate(
RelationalEventId.StoredProcedureConcurrencyTokenNotMapped.ToString(),
definition.GenerateMessage(nameof(Animal), "Animal_Update", nameof(Animal.Name)),
"RelationalEventId.StoredProcedureConcurrencyTokenNotMapped"),
Assert.Throws<InvalidOperationException>(
() => context.Model).Message);
}

protected class StoredProcedureConcurrencyTokenNotMappedContext : DbContext
{
public StoredProcedureConcurrencyTokenNotMappedContext(DbContextOptionsBuilder optionsBuilder)
: base(optionsBuilder.Options)
{
}

protected override void OnModelCreating(ModelBuilder modelBuilder)
=> modelBuilder.Entity<Animal>(
b =>
{
b.Ignore(a => a.FavoritePerson);
b.Property(e => e.Name).IsRowVersion();
b.UpdateUsingStoredProcedure(
b =>
{
b.HasOriginalValueParameter(e => e.Id);
b.HasParameter(e => e.Name, p => p.IsOutput());
b.HasRowsAffectedReturnValue();
});
});
}

protected override DbContextOptionsBuilder CreateOptionsBuilder(
IServiceCollection services,
Action<RelationalDbContextOptionsBuilder<SqlServerDbContextOptionsBuilder, SqlServerOptionsExtension>> relationalAction)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,14 @@ public override void Passes_on_derived_entity_type_not_mapped_to_a_stored_proced
Assert.Equal(SqliteStrings.StoredProceduresNotSupported(nameof(Animal)), exception.Message);
}

public override void Detects_unmapped_concurrency_token()
{
var exception =
Assert.Throws<InvalidOperationException>(() => base.Detects_unmapped_concurrency_token());

Assert.Equal(SqliteStrings.StoredProceduresNotSupported(nameof(Animal)), exception.Message);
}

public override void Store_generated_in_composite_key()
{
var modelBuilder = CreateConventionModelBuilder();
Expand Down