Efcore: 5.0 API Reviews

[bricelam]

January 22

Done

• [x] @ajcvickers Hide ValueComparer.HasDefaultBehavior using a sentinal value and an extension method
• [x] @ajcvickers Convert DiagnosticsLoggerExtensions to default interface implementations
• [x] @roji Revert breaking changes in d1cd9c76 (#20824)
• [x] @smitpatel Make FromSqlParameterApplyingExpressionVisitor internal (#20767 )
• [x] @smitpatel Review NullabilityBasedSqlProcessingExpressionVisitor (Moved to #20204)
  
  
  
  • Rename to NullabilityProcessingSqlExpressionVisitor
  
  
  • Use a method (DoNotCache) instead of a setter on CanCache
  
  
  • Hide NonNullableColumns, add an Add method
  
  
  • Just return SelectExpression from Process (no bool)
  
  
  • Just return TResult from VisitInternal and use an output parameter for the bool
  
  
• [x] @ajcvickers Review IRelationalConnection.GetCheckedConnectionString()
  
  
  
  • Should it be an extension method?
  
  
  • Use Validated instead of Checked?
  
  
  • Swap behavior with the property?
  
  
  • Design meeting?
  
  
• [x] @AndriySvyryd ~Should SqlitePropertyBuilderExtensions.CanSetGeometricDimension and others take an IPropertyBuilder?~
  
  
  
  • The missing methods are in SqliteNetTopologySuitePropertyBuilderExtensions
  
  

March 25

Done

• [x] @AndriySvyryd Consider removing GetTypesInHierarchy (Used 11 times), IsSameHierarchy (Used 10 times), IsIntraHierarchical (Used 17 times)
• [x] @ajcvickers Use Field instead of FieldInfo in model building
• [x] @ajcvickers Change entityTypeName for IDbSetCache.GetOrAddSet and similar
• [x] @ajcvickers Remove fallback from GetKeyValueComparer
• [x] @ajcvickers Make ToDebugString, MetadataDebugStringOptions, etc. public and look at naming of AnnotationsToDebugString
• [x] @ajcvickers Rename CreateKeyValueReadExpression to CreateKeyValuesExpression
• [x] @ajcvickers Make IsQueryableType real internal
• [x] @AndriySvyryd Obsolete IConventionEntityType.HasNoKey, HasDependentToPrincipal, RemoveAnnotation, CanSetDiscriminator, HasNoDeclaredDiscriminator and related methods that were removed
• [x] @AndriySvyryd Consider making WithLeftManyNavigation and related methods internal
• [x] @ajcvickers Rename IndexedProperty to IndexerProperty and related
• [x] @smitpatel @roji Do we need both EvaluatableExpressionFilter and EvaluatableExpressionFilterBase, or should they be merged?
• [x] @ajcvickers Ensure all dependency objects are tested
• [x] @ajcvickers Seal ExpressionEqualityComparer
• [x] @ajcvickers Change ReplacingExpressionVisitor to use IReadOnlyList<Expression>
• [x] @ajcvickers Use ETag instead of Etag
• [x] @AndriySvyryd Consider removing GetViewOrTableMappings
• [x] @ajcvickers Remove GetViewOrTableColumnMappings
• [x] @AndriySvyryd Add API consistency tests for IColumn.GetDefaultValue, etc.
• [x] @ajcvickers Rename IColumnBase.Type to StoreType
• [x] @smitpatel Revisit name of of IAnnotatable.IsQueryable when we do aggregate UDF

April NaN

Done

• [x] @ajcvickers ITable.IsMigratable -> IsExcludedFromMigrations
• [x] @AndriySvyryd Two interfaces for IPrimaryKeyConstraint and IUniqueConstraint
• [x] @ajcvickers ITable.IsSplit -> IsShared
• [x] @ajcvickers InternalForeignKeys -> RowInternalForeignKeys
• [x] @AndriySvyryd Consider removing IUniqueConstraint.IsPrimaryKey
• [x] @ajcvickers ViewDefinition -> ViewDefinitionSql
• [x] @smitpatel Rename ISqlExpressionFactory.Function overloads to indicate the type of function
• [x] @smitpatel QueryableFunctions -> TableValuedFunctions

May 20

Done

• [x] @smitpatel Make queryable.PerofrmIdentityResolution an overload of AsNoTracking.
  
  
  
  • Add QueryTrackingBehavior.NoTrackingWithIdentityResolution
  
  
• [x] @AndriySvyryd Should we obsolete ModelBuilder.ctor(IMutableModel) instead of removing it?
• [x] @smitpatel Merge CoreLoggerExtensions.InvalidIncludePathError into BadInclude
• [x] @AndriySvyryd Rename minimal to minimum in AnnotatableBuilder.MergeAnnotationsFrom
• [x] @roji Remove redundant savepoint in DatabaseFacade parameter names
• [x] @AndriySvyryd Review IDbSetFinder.CreateClrTypeDbSetMapping. Should it just be GetDbSet(Type)?
• [x] @lajones Review the XML docs on IndentedStringBuilder.AppendLines. Make sure the behavior around newlines and indenting is clear.
• [x] @ajcvickers Review the removal of MetadataDebugStringOptions. (Probably fine.) AV: It just moved namespaces.
• [x] @smitpatel Double-check that we meant to remove EntityMaterializerSource. Is it internal now? Did it just move somewhere else?
• [x] @ajcvickers Review why we removed IParameterValues AV: It is pubternal consumed by the QueryContext which is public
• [x] @smitpatel Review why we sealed ProjectionMember
• [x] @ajcvickers Bring back QueryableMethods
• [x] @smitpatel Document the braking change to QueryableMethodTranslatingExpressionVisitor.ctor
• [x] @smitpatel Review the removal of QueryContext.StateManager
• [x] @lajones Review breaking change to DbContextActivator.CreateInstance. Will it break scaffolding?
  
  
  
  • Rename parameter to args
  
  
• [x] @roji Rename ComputedColumnIsStored to IsStored
• [x] @bricelam Did renaming RelationalAnnotationNames.ViewDefinition to ViewDefinitionSql break Migrations?
• [x] @AndriySvyryd Move InternalDbFunctionBuilder, InternalDbFunctionParameterBuilder & InternalSequenceBuilder to Internal
• [x] @roji Fix breaking changes to MigrationBuilder and ColumnsBuilder parameter order for computedColumnIsStored
• [x] @lajones Why did CreateTableBuilder change from PropertyInfo to MemberInfo? It's not possible to create non-property members in anonymous objects
• [x] @smitpatel Can we move ExpressionExtensions.IsLogicalNot to SqlUnaryExpression? Or just remove it?
• [x] @smitpatel Document provider breaking change to take QueryCompilationContext instead of IModel
• [x] @maumar Rename RelationalSqlTranslatingExpressionVisitor.ProvideTranslationErrorDetails to AddTranslationErrorDetails
• [x] @smitpatel Review removal of SqlExpressionFactory.Case(SqlExpression, SqlExpression, params CaseWhenClause[]). We should continue allowing providers to generate this pattern, and try to not break them
• [x] @smitpatel Sanity check and review all the SqlFunction changes in a design meeting
• [x] @roji Follow up in a design meeting on DatabaseFacade.AreSavepointsSupported. If we keep it, remove Are
• [x] @roji Remove Are from IDbContextTransaction.AreSavepointsSupported. Make ADO.NET proposal match
• [x] @roji Should savepoint APIs be on IDbContextTransactionManager? If we keep them, remove redundant savepoint from parameter names, and Are from AreSavepointsSupported
• [x] @ajcvickers Make BinaryValueGenerator pubternal (string and temp ones too) - Kept public; allows reverting to pre 3.1 behavior.
• [x] @ajcvickers Review RelationalTypeMappingSource.StoreTypeNameBaseUsesPrecision. How should we handle time which only has a scale. Consider making ParseStoreTypeName lookup a type mapping for storeTypeNameBase and use its StoreTypePostfix
• [x] @ajcvickers Is RelationalTypeMappingSourceExtensions.GetMapping(insanely large overload) needed? If so, rename type to clrType
• [x] @roji Review generic use in DbFunctions.Collate alongside the overloads of data length in a design meeting.

July 22

Done

• [x] @AndriySvyryd Return void from ConventionModelExtensions.AddShared, AddOwned ~& RemoveOwned~
• [x] @smitpatel Rename EntityFrameworkQueryableExtensions.IgnoreEagerLoadedNavigations to IgnoreAutoIncludes
  
  
  
  • Rename related APIs too; Fluent API should be AutoInclude
  
  
• [x] @ajcvickers Remove ModelBuilder.SharedEntity overloads that only mark a CLR type as shared and don't actually define an entity type
• [x] @smitpatel Rename ModelBuilder.SharedEntity to SharedTypeEntity
  
  
  
  • Rename related APIs too
  
  
• [x] @smitpatel Update usages of Association to JoinEntityType
• [x] @roji Make private CSharpSnapshotGenerator.GenerateFluentApiForDefaultValue, IsUnicode, MaxLength & PrecisionAndScale
• [x] @smitpatel Rename InMemoryEntityTypeBuilderExtensions.ToQuery to ToInMemeroyQuery
• [x] @smitpatel Rename DefiningQuery metadata APIs to InMemoryQuery (or whatever matches Relational)
• [x] @smitpatel Rename RelationalEntityTypeBuilderExtensions.ToQuerySql to ToSqlQuery
• [x] @AndriySvyryd Change excludedFromMigrations parameter to a nested closure method
• [x] @AndriySvyryd Rename IndexInvalidPropertiesEventData and IndexInvalidPropertyEventData
• [x] @AndriySvyryd Make StoreObjectIdentifier a readonly struct (with private constructors)
• [x] @ajcvickers Update usages of DisplayName to FullName
• [x] @ajcvickers Remove builtIn parameter from DbFunctionAttribute
• [x] @ajcvickers Make IndexAttribute.PropertyNames an IReadOnlyList
• [x] @ajcvickers Change IndexAttribute.GetIsUnique to IsUniqueHasValue
• [x] @ajcvickers Review why we added UpdateEntryExtensions.GetCurrentProviderValue
• [x] @ajcvickers Review how AbstractionsStrings.CollectionArgumentHasEmptyElements is used.
  
  
  
  • Check for duplication in CoreStrings
  
  

August 5

Done

• [x] @smitpatel Create a single method for NavigationIncluded, use a new event data based on INavigationBase and use a new event ID.
• [x] @smitpatel Nonconfigured -> NonConfigured
• [x] @smitpatel Make CanHaveNavigationBase pubternal or inlined and add CanHaveSkipNavigation
• [x] @smitpatel Merge SkipNavigationBackingFieldAttributeConvention into non-skip code
• [x] @smitpatel TwoSqlExpressionEventData -> TwoSqlExpressionsEventData
• [x] @AndriySvyryd navigationToTarget -> navigation
• [x] @AndriySvyryd IsExcludedFromMigrations -> ExcludeFromMigrations
• [x] @smitpatel DistinctSqlExpression -> DistinctExpression
• [x] @ajcvickers Can we remove WithPrecisionAndScale?
• [x] @ajcvickers Change clrType to type when it is a Type parameter. (Also for xxxClrType parameters.)

August 12 (full)

Done

[ajcvickers]

@AndriySvyryd We took a note to change SetFieldInfo to SetField. However, this falls foul of the metadata API consistency tests because IProperty has a FieldInfo property. However, we previously used SetField methods with this property. So it will be a break either way we become consistent. Which way should we go, or should we just not be consistent here?

[AndriySvyryd]

@ajcvickers Just rename the string overload

[ajcvickers]

@AndriySvyryd So then we want two configuration sources as well? One for Field and one for FieldInfo? Currently we only have one, but then we get:

IConventionPropertyBase expected to have a GetFieldConfigurationSource()

or vice-versa.

[AndriySvyryd]

No, add an exception for this. The string overload is just sugar

[ajcvickers]

@smitpatel @roji @AndriySvyryd Just a ping that new items have been added here.

[ajcvickers]

@AndriySvyryd @smitpatel New items for you here.

[smitpatel]

Double-check that we meant to remove EntityMaterializerSource. Is it internal now? Did it just move somewhere else?

It is public internal now. It was mainly used for CreateReadValueExpression static method publicly which we moved to ExpressionExtensions.

[smitpatel]

@smitpatel Review the removal of QueryContext.StateManager

EntityFramework internal.

[smitpatel]

Document the braking change to QueryableMethodTranslatingExpressionVisitor.ctor

Where should I document that? We have breaking changes page on docs for our customers. Though this breaking change could confuse customers and should only be conveyed to provider writers.

[AndriySvyryd]

Where should I document that?

An issue labeled with providers-beware

[smitpatel]

Review removal of SqlExpressionFactory.Case(SqlExpression, SqlExpression, params CaseWhenClause[]). We should continue allowing providers to generate this pattern, and try to not break them

We practically did not break anyone since that overload was never part of the interface ISqlExpressionFactory which is registered in DI and query should be using everywhere. Adding overload on the interface for the pattern supported by the ctors.

[bricelam]

Review generic use in DbFunctions.Collate alongside the overloads of data length in a design meeting.

@roji Feel free to close this issue as fixed if you're tracking this elsewhere.

[ajcvickers]

@roji API review changes need to be in rc1. What's the status on this one?

[AndriySvyryd]

@smitpatel Looked at it. Was there anything we needed to change?

[ajcvickers]

Didn't realize Smit had looked--just saw that this was still assigned to @roji. I'll close.

[smitpatel]

I believe the APIs we have now is consistent with our strict usage so far. The major question we need to answer about relaxing constraint of generics in those APIs is how would user call those methods on properties which had value converter applied and database type matches function expectation but CLR type does not. It is "needs-design" which should be done post-5.0