Efcore: Identify and document internal code used by providers

Created on 14 Mar 2018  ·  25Comments  ·  Source: dotnet/efcore

Following up on the discussion from #11132, we have decided to take the following action for 2.1:

  • Identify the internal types that are used by the "big five" providers (Postgres, MySQL, SQLite, SQL Server, SQL Compact) and add a note to each of these types to avoid breaking in the 2.1 timeframe
  • File an issue for each class of internal code--e.g. one for resource strings, one for conventions, and so on. For the most part we will address these things post 2.1.

    • If there is already a way to do something without using internals, then file an issue on the relevant providers instead.

area-external area-test closed-fixed providers-beware
Source
picture ajcvickers
🎉1

Most helpful comment

FWIW as a provider writer I can say that there has been a very good side to exposing internal types as public (under an Internal namespace). It has allowed me to implement functionality even if it involved using internal types, rather than completely blocking on them until the EF team makes them truly public and releases a version (or use reflection). That's allowed a very flexible/fast workflow, as long as I was willing to accept some breakage on the way and be reactive. This seems to be a good fit especially for a new product like EF Core where things aren't (or weren't) yet completely nailed down.

It's worth reminding that unenforced encapsulation is how it works in some other languages (e.g. python has no public/internal/private), it's definitely a legitimate debate.

Regarding the comments (which I also received via email), @TechnikEmpire you seem to have really gone off the deep end here (FYI I'm not part of Microsoft).

picture roji on 31 Mar 2018
👍2 ❤1

All 25 comments

@ErikEJ @roji @caleblloyd Would appreciate your thoughts on this.

ajcvickers picture ajcvickers on 14 Mar 2018

@ajcvickers would you like me to open issues for the points raised in #11132?

roji picture roji on 14 Mar 2018

@roji That would be great, especially if you could include the actual internal types that are being used so we know where to go make annotations in our code.

ajcvickers picture ajcvickers on 14 Mar 2018

Pomelo.MySql

Annotations Framework
- InternalEntityTypeBuilder
- RelationalAnnotationsBuilder
- ConfigurationSource
- InternalIndexBuilder
- InternalModelBuilder
- InternalPropertyBuilder
- InternalKeyBuilder
- InternalRelationalBuilder
- RelationalForeignKeyBuilderAnnotations

Conventions
- DatabaseGeneratedAttributeConvention
- IModelInitializedConvention
- RelationalConventionSetbuilder
- RelationalConventionSetBuilderDependencies
- CoreConventionSetBuilder
- CoreConventionSetBuilderDependencies
- IConventionSetBuilder

RevEng
- TableSelectionSet

Logging
- RelationalStrings
- CoreStrings

Utility
- DisplayName()
- ShortDisplayName()

Command
- IndentedStringBuilder
- RelationalParameterBuilder
- RelationalCommand
- Logger.CommandExecuting/Executed/Error log mssages
- TransactionLogger.TransactionStarted/Used/Committed/Error/Rolledback

smitpatel picture smitpatel on 15 Mar 2018

Npgsql

Annotations Framework
- InternalEntityTypeBuilder
- ConfigurationSource
- InternalIndexBuilder
- InternalModelBuilder
- InternalPropertyBuilder
- InternalKeyBuilder
- InternalRelationalBuilder
- RelationalForeignKeyBuilderAnnotations
- RelationalIndexBuilderAnnotations
- RelationalKeyBuilderAnnotations
- RelationalEntityTypeBuilderAnnotations
- RelationalAnnotationsBuilder
- Property

Conventions
- IModelInitializedConvention
- RelationalConventionSetbuilder
- RelationalConventionSetBuilderDependencies
- RelationalMaxIdentifierLengthConvention
- ICoventionSetBuilder

Query
- FindProperty(this expression, type)
- MemberAccessBindingExpressionVisitor.GetPropertyPath
- QueryCompilationContextDependencies
- ILinqOperatorProvider
- LinqOperatorProvider
- AsyncLinqOperatorProvider

Logging
- RelationalStrings
- CoreStrings

Utility
- DisplayName()
- ShortDisplayName()

smitpatel picture smitpatel on 15 Mar 2018
👍1

SqlCe/SqlServer/SQLite is hard to figure out at present because they use the same namespace as EFCore hence no way to differentiate between EFCore type vs provider type.

Blocked on #11132

smitpatel picture smitpatel on 15 Mar 2018

Thanks for the legwork @smitpatel. Just to say that although this is important, it isn't incredibly urgent for me - in general I try to keep up with EF Core so Internal changes at your end are usually a minor annoyance more than anything else.

roji picture roji on 15 Mar 2018

@smitpatel Look in this branch, there I have renamed namespace: https://github.com/ErikEJ/EntityFramework.SqlServerCompact/tree/2-1-preview1-prep

ErikEJ picture ErikEJ on 15 Mar 2018
🎉1

@ajcvickers Sounds like a good plan, but agree on the urgency with @roji

ErikEJ picture ErikEJ on 15 Mar 2018

Thanks for compiling the list, @smitpatel. I agree with the others on the low urgency, we also try to do minor releases quickly that track upstream.

It would be really nice to get to a point where minor and patch releases don't require many changes, because it can be time consuming figuring out what has changed upstream.

caleblloyd picture caleblloyd on 15 Mar 2018

sqlserver

Annotations Framework
- InternalEntityTypeBuilder
- ConfigurationSource
- InternalIndexBuilder
- InternalModelBuilder
- InternalPropertyBuilder
- InternalKeyBuilder
- InternalRelationalBuilder
- RelationalForeignKeyBuilderAnnotations
- RelationalAnnotationsBuilder
- Property
- DbFunction
- Property
- EntityType
- CoreAnnotationNames
- ConventionalAnnotation
- GetAllBaseTypesInclusive
- FindPrincipal
- FindSharedTableRootPrimaryKeyProperty
- GetDeclaredProperties

Conventions
- IModelInitializedConvention
- RelationalConventionSetbuilder
- RelationalConventionSetBuilderDependencies
- RelationalMaxIdentifierLengthConvention
- ICoventionSetBuilder
- RelationalDbFunctionConvention
- IBaseTypeChangedConvention
- IIndexAddedConvention
- IIndexUniquenessChangedConvention
- IIndexAnnotationChangedConvention
- IPropertyNullabilityChangedConvention
- IPropertyAnnotationChangedConvention
- IEntityTypeAnnotationChangedConvention
- IKeyAddedConvention
- RelationalValueGeneratorConvention
- IModelInitializedConvention
- ValueGeneratorConvention
- RelationalDbFunctionConvention

Query
- FindProperty(this expression, type)
- MemberAccessBindingExpressionVisitor.GetPropertyPath
- QueryCompilationContextDependencies
- ILinqOperatorProvider
- LinqOperatorProvider
- AsyncLinqOperatorProvider
- LiftExpressionFromSubquery

Logging
- RelationalStrings
- CoreStrings
- AbstractionStrings

Utility
- DisplayName()
- ShortDisplayName()
- NonCapturingLazyInitializer
- IParameterBindingFactories (referenced in PropertyInfoExtensions)

Migrations/Scaffolding
- ScaffoldingAnnotationNames
- IndentedStringBuilder

Update
ISingletonUpdateSqlGenerator

smitpatel picture smitpatel on 23 Mar 2018

Sqlite

Annotations Framework
- InternalEntityTypeBuilder
- ConfigurationSource
- InternalIndexBuilder
- InternalModelBuilder
- InternalPropertyBuilder
- InternalKeyBuilder
- InternalRelationalBuilder
- RelationalForeignKeyBuilderAnnotations
- RelationalModelBuilderAnnotations
- RelationalPropertyBuilderAnnotations
- RelationalEntityTypeBuilderAnnotations
- RelationalKeyBuilderAnnotations
- RelationalIndexBuilderAnnotations
- GetDeclaredIndexes

Conventions
- RelationalConventionSetbuilder
- RelationalConventionSetBuilderDependencies
- ICoventionSetBuilder

Query
- FindProperty(this expression, type)

Logging
- AbstractionStrings

Utility
- DisplayName()
- ShortDisplayName()
- IParameterBindingFactories (referenced in PropertyInfoExtensions)

Update
- ISingletonUpdateSqlGenerator

smitpatel picture smitpatel on 23 Mar 2018

SqlServerCe

Annotations Framework
- InternalEntityTypeBuilder
- ConfigurationSource
- InternalIndexBuilder
- InternalModelBuilder
- InternalPropertyBuilder
- InternalKeyBuilder
- InternalRelationalBuilder
- RelationalForeignKeyBuilderAnnotations
- RelationalModelBuilderAnnotations
- RelationalPropertyBuilderAnnotations
- RelationalEntityTypeBuilderAnnotations
- RelationalKeyBuilderAnnotations
- RelationalIndexBuilderAnnotations

Conventions
- RelationalConventionSetbuilder
- RelationalConventionSetBuilderDependencies
- CoreConventionSetBuilder
- CoreConventionSetBuilderDependencies
- ICoventionSetBuilder

Query
- QueryCompilationContextDependencies
- ILinqOperatorProvider
- LinqOperatorProvider
- AsyncLinqOperatorProvider
- RemoveConvert

Logging
- CoreStrings
- RelationalStrings

Scaffolding
- TableSelectionSet
- ScaffoldingAnnotationNames

smitpatel picture smitpatel on 23 Mar 2018

Filed #11404, #11405, #11406, #11407, #11408, #11409

smitpatel picture smitpatel on 23 Mar 2018
👍1

@TechnikEmpire Not sure how your comment relates to this "clean-up" issue?

ErikEJ picture ErikEJ on 30 Mar 2018

@TechnikEmpire Reading your first and second comments, I understand your intention was to point out that the EF team (not the Pomelo project) has made what you believe are several mistakes, starting with using .Internal namespaces as a way to indicate that certain APIs should be considered internal rather than using the available internal keyword, and that you believe that this had the effect to encourage actions that we should have tried to discourage.

There were certainly reasons to do things the way we did them but I hope we can still take your feedback and use it to discuss if there are more things we can do around this area in the future (cc @ajcvickers).

On the other hand, although this issue is closed and the children issues (#11404, #11405, #11406, #11407, #11408, #11409) are in the backlog, we are still planning to mitigate breaks caused by changes in 2.1. This means in some cases we will revert changes, in other cases we will tweak changes to avoid breaking and in some other cases changes will be necessary on providers. We will continue working with provider writers on this.

Finally, in order to be able to count with your valuable feedback in the future, I need to ask you to follow the code of conduct at https://opensource.microsoft.com/codeofconduct/, in particular be respectful on your interactions with everyone in this repo. All comments you submit count, even if you delete or edit them later.

divega picture divega on 31 Mar 2018

@TechnikEmpire you are welcome to stay and continue participating of this conversation in a civil manner or to leave if you so desire.

By “all comments you submit count”, I was referring to the fact that in my email I found comments from you that I no longer see in this thread. Yes, the code of conduct applies, because everyone that subscribed to this thread will also receive them.

divega picture divega on 31 Mar 2018

FWIW as a provider writer I can say that there has been a very good side to exposing internal types as public (under an Internal namespace). It has allowed me to implement functionality even if it involved using internal types, rather than completely blocking on them until the EF team makes them truly public and releases a version (or use reflection). That's allowed a very flexible/fast workflow, as long as I was willing to accept some breakage on the way and be reactive. This seems to be a good fit especially for a new product like EF Core where things aren't (or weren't) yet completely nailed down.

It's worth reminding that unenforced encapsulation is how it works in some other languages (e.g. python has no public/internal/private), it's definitely a legitimate debate.

Regarding the comments (which I also received via email), @TechnikEmpire you seem to have really gone off the deep end here (FYI I'm not part of Microsoft).

roji picture roji on 31 Mar 2018
👍2 ❤1

@roji I agree re the public/internal surface. This is the main reason we did it. That said, it would have been great if you had to use more specific means to opt-in using an internal type in specific blocks of your code without getting a warning or error. The way things have been, it is apparently too easy for it to happen accidentally, and for us to miss the fact that an API that we consider internal is required to implement a provider. Changing namespaces can help a bit, but perhaps a feature more deeply integrated in the language could help more.

divega picture divega on 31 Mar 2018
👍1

@divega the namespace change is definitely important as we've seen. Maybe simply an analyzer check that produces warnings about usage of EF Core internal code? This way you'd be well-informed and can suppress the warnings if that's what you want. This could be useful for both providers and applications (which can also replace services etc.)

roji picture roji on 31 Mar 2018
👍1

Yes, an analyzer would be great.

divega picture divega on 31 Mar 2018

Enhance API consistency check and add it to the spec tests?

ErikEJ picture ErikEJ on 31 Mar 2018

@ErikEJ I think the problem with doing it in the spec tests is that it's actually OK to use internal code (after all that's why it's public), the main goal is to make sure people are aware that they're doing it. So there needs to be a way to say on specific usages "OK, I'm aware, I want to do it anyway". I'm not sure how such suppression could happen on a per-site basis outside of an analyzer-generated warning.

roji picture roji on 31 Mar 2018

@roji Analyzer suppressions ftw!

ErikEJ picture ErikEJ on 31 Mar 2018

Consolidated list of remaining work. (Will update individual issues as well, but wanted to have this in one place for me.)

Annotations Framework

  • InternalEntityTypeBuilder
  • RelationalAnnotationsBuilder
  • ~ConfigurationSource~ Public
  • InternalIndexBuilder
  • InternalModelBuilder
  • InternalPropertyBuilder
  • InternalKeyBuilder
  • InternalRelationalBuilder
  • RelationalForeignKeyBuilderAnnotations
  • RelationalPropertyBuilderAnnotations
  • RelationalIndexBuilderAnnotations
  • RelationalKeyBuilderAnnotations
  • RelationalEntityTypeBuilderAnnotations
  • RelationalAnnotationsBuilder
  • Property
  • DbFunction
  • Property
  • EntityType
  • ~CoreAnnotationNames~ Public
  • ConventionalAnnotation
  • ~GetAllBaseTypesInclusive~ Public
  • ~FindPrincipal~ Public
  • ~FindSharedTableRootPrimaryKeyProperty~ Public
  • ~GetDeclaredProperties~ Public
  • ~GetDeclaredIndexes~ Public

Conventions

  • DatabaseGeneratedAttributeConvention
  • IModelInitializedConvention
  • RelationalConventionSetbuilder
  • RelationalConventionSetBuilderDependencies
  • CoreConventionSetBuilder
  • CoreConventionSetBuilderDependencies
  • IConventionSetBuilder
  • RelationalMaxIdentifierLengthConvention
  • RelationalMaxIdentifierLengthConvention
  • RelationalDbFunctionConvention
  • IBaseTypeChangedConvention
  • IIndexAddedConvention
  • IIndexUniquenessChangedConvention
  • IIndexAnnotationChangedConvention
  • IPropertyNullabilityChangedConvention
  • IPropertyAnnotationChangedConvention
  • IEntityTypeAnnotationChangedConvention
  • IKeyAddedConvention
  • RelationalValueGeneratorConvention
  • IModelInitializedConvention
  • ValueGeneratorConvention
  • RelationalDbFunctionConvention

RevEng

  • ~TableSelectionSet~ Removed
  • ~ScaffoldingAnnotationNames~ Public

Logging

  • ~RelationalStrings~ Public
  • ~CoreStrings~ Public
  • ~AbstractionStrings~ Public

Utility

  • ~DisplayName()~ Public
  • ~ShortDisplayName()~ Public
  • ~NonCapturingLazyInitializer~ Use LazyInitializer instead

Command

  • ~IndentedStringBuilder~ No longer needed
  • ~RelationalParameterBuilder~ Removed
  • ~RelationalCommand~ Public
  • ~Logger.CommandExecuting/Executed/Error log mssages~ Public
  • ~TransactionLogger.TransactionStarted/Used/Committed/Error/Rolledback~ Public

Query

  • ~FindProperty(this expression, type)~ Public
  • ~MemberAccessBindingExpressionVisitor.GetPropertyPath~
  • QueryCompilationContextDependencies
  • ILinqOperatorProvider
  • LinqOperatorProvider
  • AsyncLinqOperatorProvider
  • LiftExpressionFromSubquery
  • ~RemoveConvert~ Public
  • IParameterBindingFactories (referenced in PropertyInfoExtensions)

Update

  • ~ISingletonUpdateSqlGenerator~ Removed

InMemory stuff

  • StateManager (Used by seeding)
  • IPrincipalKeyValueFactory (Used by in-memory table)
  • Property.GetIndex()
  • InternalEntityEntry
  • IdentityMapFactoryFactoryBase
  • GetKeyType
ajcvickers picture ajcvickers on 21 Mar 2019
Was this page helpful?
0 / 5 - 0 ratings

Related issues

MontyGvMC picture MontyGvMC  ·  3Comments
yukozh picture yukozh  ·  3Comments
TPJ11 picture TPJ11  ·  3Comments
mohsin91 picture mohsin91  ·  3Comments
ryanwinter picture ryanwinter  ·  3Comments