Address review comments

dotnet · Jun 21, 2022 · 85d3262 · 85d3262
1 parent 9389247
commit 85d3262
Show file tree

Hide file tree

Showing 8 changed files with 32 additions and 19 deletions.
diff --git a/src/EFCore.Relational/Design/Internal/RelationalCSharpRuntimeAnnotationCodeGenerator.cs b/src/EFCore.Relational/Design/Internal/RelationalCSharpRuntimeAnnotationCodeGenerator.cs
@@ -610,7 +610,7 @@ private static void AppendLiteral(StoreObjectIdentifier storeObject, IndentedStr
                     .Append("DbFunction(").Append(code.Literal(storeObject.Name)).Append(")");
                 break;
             default:
-                Check.DebugAssert(false, "Unexpected StoreObjectType: " + storeObject.StoreObjectType);
+                Check.DebugFail("Unexpected StoreObjectType: " + storeObject.StoreObjectType);
                 break;
         }
     }

diff --git a/src/EFCore.Relational/Migrations/Internal/MigrationsModelDiffer.cs b/src/EFCore.Relational/Migrations/Internal/MigrationsModelDiffer.cs
@@ -237,7 +237,7 @@ protected virtual IReadOnlyList<MigrationOperation> Sort(
             else
             {
                 leftovers.Add(operation);
-                Check.DebugAssert(false, "Unexpected operation type: " + operation.GetType());
+                Check.DebugFail("Unexpected operation type: " + operation.GetType());
             }
         }
 
@@ -275,7 +275,7 @@ protected virtual IReadOnlyList<MigrationOperation> Sort(
                     }
                     else
                     {
-                        Check.DebugAssert(false, "Operation removed twice: " + cyclicAddForeignKeyOperation);
+                        Check.DebugFail("Operation removed twice: " + cyclicAddForeignKeyOperation);
                     }
                 }
 
@@ -2189,7 +2189,7 @@ private IEnumerable<MigrationOperation> GetDataOperations(
 
                         if (forSource)
                         {
-                            Check.DebugAssert(false, "Insert using the source model");
+                            Check.DebugFail("Insert using the source model");
                             break;
                         }
 
@@ -2212,7 +2212,7 @@ private IEnumerable<MigrationOperation> GetDataOperations(
 
                         if (forSource)
                         {
-                            Check.DebugAssert(false, "Update using the source model");
+                            Check.DebugFail("Update using the source model");
                             break;
                         }
 

diff --git a/src/EFCore.Relational/Query/Internal/FromSqlParameterExpandingExpressionVisitor.cs b/src/EFCore.Relational/Query/Internal/FromSqlParameterExpandingExpressionVisitor.cs
@@ -160,7 +160,7 @@ public virtual Expression Expand(
                 return _visitedFromSqlExpressions[fromSql] = fromSql.Update(Expression.Constant(constantValues, typeof(object[])));
 
             default:
-                Check.DebugAssert(false, "FromSql.Arguments must be Constant/ParameterExpression");
+                Check.DebugFail("FromSql.Arguments must be Constant/ParameterExpression");
                 return null;
         }
     }

diff --git a/src/EFCore.SqlServer.NTS/Query/Internal/SqlServerGeometryMethodTranslator.cs b/src/EFCore.SqlServer.NTS/Query/Internal/SqlServerGeometryMethodTranslator.cs
@@ -85,7 +85,12 @@ public SqlServerGeometryMethodTranslator(
                 arguments.Where(e => typeof(Geometry).IsAssignableFrom(e.Type)));
             var typeMapping = ExpressionExtensions.InferTypeMapping(geometryExpressions.ToArray());
 
-            Check.DebugAssert(typeMapping != null, "At least one argument must have typeMapping.");
+            if (typeMapping is null)
+            {
+                Check.DebugFail("At least one argument must have typeMapping.");
+                return null;
+            }
+
             var storeType = typeMapping.StoreType;
             var isGeography = string.Equals(storeType, "geography", StringComparison.OrdinalIgnoreCase);
 

diff --git a/...EFCore.SqlServer.NTS/Query/Internal/SqlServerNetTopologySuiteAggregateMethodTranslator.cs b/...EFCore.SqlServer.NTS/Query/Internal/SqlServerNetTopologySuiteAggregateMethodTranslator.cs
@@ -60,6 +60,12 @@ public SqlServerNetTopologySuiteAggregateMethodTranslator(
             return null;
         }
 
+        if (sqlExpression.TypeMapping is not { } typeMapping)
+        {
+            Check.DebugFail("SQL expression is missing a type mapping.");
+            return null;
+        }
+
         var functionName = method == GeometryCombineMethod
             ? "CollectionAggregate"
             : method == UnionMethod
@@ -85,15 +91,12 @@ public SqlServerNetTopologySuiteAggregateMethodTranslator(
             sqlExpression = new DistinctExpression(sqlExpression);
         }
 
-        var storeType = sqlExpression.TypeMapping?.StoreType ?? "geometry";
-
         return _sqlExpressionFactory.Function(
-            $"{storeType}::{functionName}",
-            // "geometry::" + functionName,
+            $"{typeMapping.StoreType}::{functionName}",
             new[] { sqlExpression },
             nullable: true,
             argumentsPropagateNullability: new[] { false },
-            typeof(Geometry),
-            _typeMappingSource.FindMapping(typeof(Geometry), storeType));
+            method.ReturnType,
+            _typeMappingSource.FindMapping(method.ReturnType, typeMapping.StoreType));
     }
 }
diff --git a/src/EFCore/Infrastructure/ModelValidator.cs b/src/EFCore/Infrastructure/ModelValidator.cs
@@ -330,7 +330,7 @@ protected virtual void ValidateIgnoredMembers(
                                 ignoredMember, entityType.DisplayName(), property.DeclaringEntityType.DisplayName()));
                     }
 
-                    Check.DebugAssert(false, "Should never get here...");
+                    Check.DebugFail("Should never get here...");
                 }
 
                 var navigation = entityType.FindNavigation(ignoredMember);
@@ -343,7 +343,7 @@ protected virtual void ValidateIgnoredMembers(
                                 ignoredMember, entityType.DisplayName(), navigation.DeclaringEntityType.DisplayName()));
                     }
 
-                    Check.DebugAssert(false, "Should never get here...");
+                    Check.DebugFail("Should never get here...");
                 }
 
                 var skipNavigation = entityType.FindSkipNavigation(ignoredMember);
@@ -356,7 +356,7 @@ protected virtual void ValidateIgnoredMembers(
                                 ignoredMember, entityType.DisplayName(), skipNavigation.DeclaringEntityType.DisplayName()));
                     }
 
-                    Check.DebugAssert(false, "Should never get here...");
+                    Check.DebugFail("Should never get here...");
                 }
 
                 var serviceProperty = entityType.FindServiceProperty(ignoredMember);
@@ -369,7 +369,7 @@ protected virtual void ValidateIgnoredMembers(
                                 ignoredMember, entityType.DisplayName(), serviceProperty.DeclaringEntityType.DisplayName()));
                     }
 
-                    Check.DebugAssert(false, "Should never get here...");
+                    Check.DebugFail("Should never get here...");
                 }
             }
         }

diff --git a/src/EFCore/Metadata/Conventions/Internal/ConventionDispatcher.ImmediateConventionScope.cs b/src/EFCore/Metadata/Conventions/Internal/ConventionDispatcher.ImmediateConventionScope.cs
@@ -59,7 +59,7 @@ public ImmediateConventionScope(ConventionSet conventionSet, ConventionDispatche
         }
 
         public override void Run(ConventionDispatcher dispatcher)
-            => Check.DebugAssert(false, "Immediate convention scope cannot be run again.");
+            => Check.DebugFail("Immediate convention scope cannot be run again.");
 
         public IConventionModelBuilder OnModelFinalizing(IConventionModelBuilder modelBuilder)
         {
@@ -716,7 +716,7 @@ public IConventionModelBuilder OnModelInitialized(IConventionModelBuilder modelB
                     if (navigationBuilder.Metadata.IsInModel
                         && navigationBuilder.Metadata.FindAnnotation(name) != annotation)
                     {
-                        Check.DebugAssert(false, "annotation removed");
+                        Check.DebugFail("annotation removed");
                         return null;
                     }
 

diff --git a/src/Shared/Check.cs b/src/Shared/Check.cs
@@ -120,4 +120,9 @@ public static void DebugAssert([DoesNotReturnIf(false)] bool condition, string m
             throw new Exception($"Check.DebugAssert failed: {message}");
         }
     }
+
+    [Conditional("DEBUG")]
+    [DoesNotReturn]
+    public static void DebugFail(string message)
+        => throw new Exception($"Check.DebugAssert failed: {message}");
 }