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 12, 2022
1 parent fbc10ec commit 63c3ffc
Show file tree
Hide file tree
Showing 8 changed files with 112 additions and 23 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="PropertyEventData" /> 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="concurrencyProperty">The property which represents the concurrency token.</param>
public static void StoredProcedureConcurrencyTokenNotMapped(
this IDiagnosticsLogger<DbLoggerCategory.Model.Validation> diagnostics,
IProperty concurrencyProperty)
{
var definition = RelationalResources.LogStoredProcedureConcurrencyTokenNotMapped(diagnostics);

if (diagnostics.ShouldLog(definition))
{
var entityType = concurrencyProperty.DeclaringEntityType;
definition.Log(diagnostics,
entityType.DisplayName(),
entityType.GetUpdateStoredProcedure()?.Name ?? "unnamed",
concurrencyProperty.Name);
}

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

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

private static string StoredProcedureConcurrencyTokenNotMapped(EventDefinitionBase definition, EventData payload)
{
var d = (EventDefinition<string, string, string>)definition;
var p = (PropertyEventData)payload;
var property = p.Property;
var entityType = property.DeclaringEntityType;
return d.GenerateMessage(entityType.DisplayName(), entityType.GetUpdateStoredProcedure()?.Name ?? "unnamed", 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(missedConcurrencyToken);
}

if (sproc.ResultColumns.Any(c => c != sproc.FindRowsAffectedResultColumn()))
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>Error 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
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, LogLevel.Error);
}

[ConditionalFact]
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 63c3ffc

Please sign in to comment.