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.

Sign up for GitHub

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

Jump to bottom

5.0 API Reviews #20409

Closed
95 tasks done
bricelam opened this issue Mar 25, 2020 · 16 comments · Fixed by #21064
Closed
95 tasks done

5.0 API Reviews #20409

bricelam opened this issue Mar 25, 2020 · 16 comments · Fixed by #21064
Assignees
@roji
Labels
area-global closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-cleanup
Milestone
5.0.0

Comments

@bricelam
Copy link
Contributor

bricelam commented Mar 25, 2020 •
edited by ajcvickers
Loading

January 22

Done

  • @ajcvickers Hide ValueComparer.HasDefaultBehavior using a sentinal value and an extension method
  • @ajcvickers Convert DiagnosticsLoggerExtensions to default interface implementations
  • @roji Revert breaking changes in d1cd9c7 (API Review: revert scaffolding nullability breaking changes #20824)
  • @smitpatel Make FromSqlParameterApplyingExpressionVisitor internal (Query: API Cleanup #20767 )
  • @smitpatel Review NullabilityBasedSqlProcessingExpressionVisitor (Moved to Query: revisit extensibility of null semantics visitor #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
  • @ajcvickers Review IRelationalConnection.GetCheckedConnectionString()
    • Should it be an extension method?
    • Use Validated instead of Checked?
    • Swap behavior with the property?
    • Design meeting?
  • @AndriySvyryd Should SqlitePropertyBuilderExtensions.CanSetGeometricDimension and others take an IPropertyBuilder?
    • The missing methods are in SqliteNetTopologySuitePropertyBuilderExtensions

March 25

Done

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

April NaN

Done

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

May 20

Done

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

July 22

Done

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

August 5

Done

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

August 12 (full)

Done
@bricelam bricelam mentioned this issue Mar 25, 2020
Merged
@AndriySvyryd AndriySvyryd removed their assignment Mar 26, 2020
@ajcvickers ajcvickers added this to the 5.0.0 milestone Mar 27, 2020
ajcvickers added a commit that referenced this issue Mar 28, 2020
@ajcvickers
API review changes
6813c6b 
Issue #20409

* ValueComparer.HasDefaultBehavior => IsDefault extension method
* Convert DiagnosticsLoggerExtensions to default interface implementations
* GetCheckedConnectionString removed from interface and changed to protected GetValidatedConnectionString
@ajcvickers ajcvickers mentioned this issue Mar 28, 2020
Merged
@ajcvickers ajcvickers removed their assignment Mar 28, 2020
ajcvickers added a commit that referenced this issue Mar 29, 2020
@ajcvickers
API review changes
759375c 
Issue #20409

* ValueComparer.HasDefaultBehavior => IsDefault extension method
* Convert DiagnosticsLoggerExtensions to default interface implementations
* GetCheckedConnectionString removed from interface and changed to protected GetValidatedConnectionString
@ajcvickers
Copy link
Member

@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
Copy link
Member

@ajcvickers Just rename the string overload

@AndriySvyryd AndriySvyryd self-assigned this Apr 16, 2020
@ajcvickers
Copy link
Member

@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
Copy link
Member

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

ajcvickers added a commit that referenced this issue Apr 18, 2020
@ajcvickers
API review changes
04d9704 
Part of #20409

- Use Field instead of FieldInfo in model building
- Change entityTypeName for `IDbSetCache.GetOrAddSet` and similar
- Rename CreateKeyValueReadExpression to CreateKeyValuesExpression
- Make IsQueryableType real internal
- Rename IndexedProperty to IndexerProperty and related
- Seal ExpressionEqualityComparer
- Change ReplacingExpressionVisitor to use `IReadOnlyList<Expression>`
- Use `ETag` instead of `Etag`
- Remove `GetViewOrTableColumnMappings`
- Rename IColumnBase.Type to StoreType
@ajcvickers ajcvickers mentioned this issue Apr 18, 2020
Merged
ajcvickers added a commit that referenced this issue Apr 18, 2020
@ajcvickers
Obsolete key comparers on properties
c63e625 
Part of #20409

Type mappings need to be able to provide different comparers for key and non-key property. However, when a key comparer is being set on a specific property, then a key comparer can be set for key properties and a regular comparer can be set for non-key properties. We don't need to support setting both on the same property.

Also remove the fallback parameter, per API review.
@ajcvickers ajcvickers mentioned this issue Apr 18, 2020
Merged
ajcvickers added a commit that referenced this issue Apr 18, 2020
@ajcvickers
Obsolete key comparers on properties
8fde7ba 
Part of #20409

Type mappings need to be able to provide different comparers for key and non-key property. However, when a key comparer is being set on a specific property, then a key comparer can be set for key properties and a regular comparer can be set for non-key properties. We don't need to support setting both on the same property.

Also remove the fallback parameter, per API review.
ajcvickers added a commit that referenced this issue Apr 18, 2020
@ajcvickers
Obsolete key comparers on properties (#20680)
0471f9b 
Part of #20409

Type mappings need to be able to provide different comparers for key and non-key property. However, when a key comparer is being set on a specific property, then a key comparer can be set for key properties and a regular comparer can be set for non-key properties. We don't need to support setting both on the same property.

Also remove the fallback parameter, per API review.
ajcvickers added a commit that referenced this issue Apr 18, 2020
@ajcvickers
Make ToDebugString methods public and DebugViews easily obtainable
bd625fb 
Part of #20409
@ajcvickers ajcvickers mentioned this issue Apr 18, 2020
Merged
ajcvickers added a commit that referenced this issue Apr 18, 2020
@ajcvickers
API review changes
5d85022 
Part of #20409

- Use Field instead of FieldInfo in model building
- Change entityTypeName for `IDbSetCache.GetOrAddSet` and similar
- Rename CreateKeyValueReadExpression to CreateKeyValuesExpression
- Make IsQueryableType real internal
- Rename IndexedProperty to IndexerProperty and related
- Seal ExpressionEqualityComparer
- Change ReplacingExpressionVisitor to use `IReadOnlyList<Expression>`
- Use `ETag` instead of `Etag`
- Remove `GetViewOrTableColumnMappings`
- Rename IColumnBase.Type to StoreType
ajcvickers added a commit that referenced this issue Apr 19, 2020
@ajcvickers
Run dependency object tests automatically for all dependency objects
d46f7cc 
Part of #20409
@smitpatel smitpatel removed their assignment Aug 7, 2020
@bricelam bricelam removed their assignment Aug 7, 2020
ajcvickers added a commit that referenced this issue Aug 12, 2020
@ajcvickers
Type mapping fixes from API reviews
3fb7e75 
* Removed `StoreTypeNameBaseUsesPrecision`
* Removed large GetMapping overload that was added in 5.0 but is never called
* Fixed issue when reverse engineering where different casing for type name was being ignored

Part of #20409
@ajcvickers ajcvickers mentioned this issue Aug 12, 2020
Merged
ajcvickers added a commit that referenced this issue Aug 12, 2020
@ajcvickers
Type mapping fixes from API reviews (#22032)
dc540f1 
* Removed `StoreTypeNameBaseUsesPrecision`
* Removed large GetMapping overload that was added in 5.0 but is never called
* Fixed issue when reverse engineering where different casing for type name was being ignored

Part of #20409
ajcvickers added a commit that referenced this issue Aug 12, 2020
@ajcvickers
API review changes
3815a0d 
Part of #20409

* Use FullName instead of DisplayName in the in-memory database
* Remove builtIn parameter from DbFunctionAttribute
* Make IndexAttribute.PropertyNames an IReadOnlyList
* Change IndexAttribute.GetIsUnique to IsUniqueHasValue
@ajcvickers ajcvickers mentioned this issue Aug 12, 2020
Merged
bricelam added a commit to bricelam/efcore that referenced this issue Aug 12, 2020
@bricelam
API Review Changes
fb29107 
Part of dotnet#20409
@bricelam bricelam mentioned this issue Aug 12, 2020
Merged
ghost pushed a commit that referenced this issue Aug 12, 2020
@ajcvickers
API review changes (#22037)
33ad238 
* API review changes

Part of #20409

* Use FullName instead of DisplayName in the in-memory database
* Remove builtIn parameter from DbFunctionAttribute
* Make IndexAttribute.PropertyNames an IReadOnlyList
* Change IndexAttribute.GetIsUnique to IsUniqueHasValue

* Tweak DbFunctionAttribute
ajcvickers added a commit that referenced this issue Aug 12, 2020
@ajcvickers
Stop qualifying Type parameters with CLR
975c506 
Part of #20409
@ajcvickers ajcvickers mentioned this issue Aug 12, 2020
Closed
@ajcvickers ajcvickers removed their assignment Aug 12, 2020
bricelam added a commit to bricelam/efcore that referenced this issue Aug 14, 2020
@bricelam
API Review Changes
1602b46 
Part of dotnet#20409
ghost pushed a commit that referenced this issue Aug 14, 2020
@bricelam
API Review Changes (#22038)
4497b71 
Part of #20409
@bricelam
Copy link
Contributor Author

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 added a commit that referenced this issue Aug 14, 2020
@ajcvickers
Stop qualifying Type parameters with CLR
6958821 
Part of #20409
ajcvickers added a commit that referenced this issue Aug 14, 2020
@ajcvickers
Stop qualifying Type parameters with CLR
9f6ad1a 
Part of #20409
@ajcvickers ajcvickers mentioned this issue Aug 14, 2020
Merged
ghost pushed a commit that referenced this issue Aug 14, 2020
@ajcvickers
Stop qualifying Type parameters with CLR (#22069)
d8cd61e 
* Stop qualifying Type parameters with CLR

Part of #20409

* Updated API test

* Fixed typo
@ajcvickers
Copy link
Member

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

@AndriySvyryd
Copy link
Member

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

@ajcvickers
Copy link
Member

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

@ajcvickers ajcvickers added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Aug 25, 2020
@ajcvickers ajcvickers modified the milestones: 5.0.0, 5.0.0-rc1 Aug 25, 2020
@smitpatel
Copy link
Contributor

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

@ajcvickers ajcvickers modified the milestones: 5.0.0-rc1, 5.0.0 Nov 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@roji roji

Labels
area-global closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-cleanup
Projects
None yet
Milestone
5.0.0
Development

Successfully merging a pull request may close this issue.

API review: Rename ISqlExpressionFactory.Function to NiladicFunction dotnet/efcore
7 participants
@bricelam @ajcvickers @smitpatel @maumar @roji @lajones @AndriySvyryd