Skip to content

Commit

Permalink
Make original value parameter check for sproc into a warning
Browse files Browse the repository at this point in the history
Fixes #28602
  • Loading branch information
ajcvickers committed Sep 15, 2022
1 parent 5d0d937 commit 287654b
Show file tree
Hide file tree
Showing 11 changed files with 196 additions and 24 deletions.
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

0 comments on commit 287654b

Please sign in to comment.