Efcore: Consider using different namespaces for provider internal code

[ajcvickers]

And making this guidance for provider writers so that it is much easier to tell if a provider is accidentally using EF internal code. See https://github.com/PomeloFoundation/Pomelo.EntityFrameworkCore.MySql/issues/497#issuecomment-369929292

/cc @roji @ErikEJ @caleblloyd

[ErikEJ]

Excellent proposal, I use the same convention

[roji]

Agree, I do use the same namespaces as EF Core and it's easy to see how a reference to an internal namespace could slip into there.

[caleblloyd]

I made this change, and can now see that we have a lot of references to upstream internal classes. How are you guys dealing with this @ErikEJ @roji? Copying these files into your provider?

https://github.com/PomeloFoundation/Pomelo.EntityFrameworkCore.MySql/issues/497#issuecomment-372128480

[roji]

I haven't made the change, but I wouldn't be surprised to find out I have some references to EF Core internal classes. I work hard to keep the provider in sync with EF Core relatively quickly, so I'm sure that has prevented breakage as I adapt to changes in internal EF Core code...

[roji]

Coming to think of it, aside from very specific extension classes that should automatically light up for users, why shouldn't all provider classes be in their own namespace? What's the gain in having even non-public classes in Microsoft.EntityFrameworkCore?

[ErikEJ]

I have the following problematic uses of .Internal namespaces, that I cannot see how to avoid, but maybe I am doing something wrong? @ajcvickers

@caleblloyd I would not say I have "a lot" of uses, though...

https://github.com/ErikEJ/EntityFramework.SqlServerCompact/blob/master/src/Provider40/Extensions/SqlCeServiceCollectionExtensions.cs

https://github.com/ErikEJ/EntityFramework.SqlServerCompact/tree/master/src/Provider40/MetaData/Conventions/Internal

https://github.com/ErikEJ/EntityFramework.SqlServerCompact/blob/master/src/Provider40/MetaData/Internal/SqlCeInternalMetaDataBuilderExtensions.cs

https://github.com/ErikEJ/EntityFramework.SqlServerCompact/blob/master/src/Provider40/Query/Internal/SqlCeQueryCompilationContext.cs

https://github.com/ErikEJ/EntityFramework.SqlServerCompact/blob/master/src/Provider40/Query/Internal/SqlCeQueryCompilationContextFactory.cs

[smitpatel]

I looked at MySQL solution to figure out what kind of references to EFCore Internal classes are.

1. Conventions are Internal, when provider has to add more conventions or extend one of the core ones, they are required to use .Internal
2. ProviderMetadataAnnotations/BuilderExtensions classes are using InternalMetadataBuilders/ConfigurationSource which requires .Internal
3. IRelationalCommandBuilder implementes an generic interface with IndentedStringBuilder which is .Internal
4. Implementing IRelationalCommandBuilder requires implementation of IRelationalParameterBuilder too since RelationalParameterBuilder is .Internal.
5. CoreStrings/RelationalStrings references from exception throwing.
6. Logging using EF core methods from provider code.
7. TableSelectionSet class in RevEng. perhaps providers are free to choose how they want to do it. (We removed this class in 2.1)
8. Utilities like DisplayName/ShortDisplayName

Some of above are actually used in our providers too.
Some of them are easy to fix in a way that provider may need to copy the code (like utilities/TableSelectionSet).
CommandBuilding can be partially fixed using implementing all related interfaces.
Conventions/Annotations framework has no other way and it is like that in SqlServer provider too.

[smitpatel]

From SqlCe, apart from 1-2 from last post about Convention/Annotations,

QueryCompilationContext requires ILinqOperatorProvider. That interface & implementation both are .Internal. So overrideing QueryCompilationFactory forces use of .Internal.

[ErikEJ]

@smitpatel But must I override QueryCompilationContext at all??

[smitpatel]

@ErikEJ - Yes. The interfaces are public so providers can override it. Perhaps one of the parameter type left out from public by error.

[ajcvickers]

@caleblloyd @ErikEJ @smitpatel @roji This is great data! We will look through these and consider what to do about each case. The goal here is not to force providers to copy "a lot" of code to avoid using internals, but instead to identify and fix holes in the public surface that send providers down the internal code path. However, that being said, there likely is some places that a small amount of utility code will need to be copied. If it turns out to be too tempting to use this code anyway, then we might make it really internal like we have for a bunch of the Reflection helpers--so that we are free to add to and remove these as needed.

For conventions, we know that these are internal but must be used by providers in this way until we do the work to make them public. We take the responsibility to not break these APIs (without coordination with providers) even though they are technically internal.

[ajcvickers]

@roji I have no problem with changing the namespaces of the public provider code if you want to do so, although it will likely technically result in a breaking change at this point, since these are public types that have been shipped. I honestly can't quite remember why we choose this namespace approach for our providers. I think it made some things look "cleaner" in some way, but I don't think it was for any specific functional reason.

[roji]

OK.

At least for my side (with the somewhat-reduced backwards compatibility constraints :)), I think I'll be changing the namespace for the entire provider, aside from extension classes which are obviously meant to be publicly-accessible (e.g.UseNpgsql()). I'll report back here on any Internal dependencies on EF Core that I find.

[ErikEJ]

FYI: I have file copied Check.cs and friends: https://github.com/ErikEJ/EntityFramework.SqlServerCompact/tree/master/src/Provider40/Utilities

[roji]

OK, that was fun...

Here are some places where Npgsql is referencing Internal components in EF Core. I went a bit quickly so there's probably quite a bit of duplication in the following list. There are also probably cases where I'm simply doing things wrong, let me know.

Conventions

• Following the SqlServer model, NpgsqlPropertyBuilderExtensions has GetNpgsqlInternalBuilder, which references InternalPropertyBuilder and ConfigurationSource which are internal. I don't know enough about this to comment.
• The provider has its own convention set builder, in order to set RelationalMaxIdentifierLengthConvention etc., but all the related types are are internal (IConventionSetBuilder, RelationalConventionSetBuilder).
• Similar for NpgsqlValueGenerationStrategyConvention, which references InternalModelBuilder, ConfigurationSource and IModelInitializedConvention.

Misc

• RelationalQueryCompilationContext is public but accepts internal types in ctor (filed #11247)
• Npgsql has NpgsqlSqlTranslatingExpressionVisitor for supporting arrays, but it needs to call MemberAccessBindingExpressionVisitor.GetPropertyPath() which is internal. I guess I could just copy it into the provider (although if you make changes/fixes...), let me know what you think.
• I have an expression translator which makes use of RelationalExpressionExtensions.FindProperty() to get the configured store type of a property, which is important in how translation will happen. Looking at it, I'm not sure it's OK (is this data included in cache keys???), some guidance would be appreciated. If needed RelationalExpressionExtensions.FindProperty() can be copied across as above.
• I have some logging code making use of TypeExtensions (ShortDisplayName() and the like. This could be a good candidate for copying into the provider, let me know.
• Following the SqlServer model, I have a NpgsqlModelBuilderAnnotations and NpgsqlPropertyBuilderAnnotations which needs to pass RelationalAnnotationsBuilder to the base constructor, but that class is internal.
• NpgsqlInternalMetadataBuilderExtensions is filled with references to RelationalEntityTypeBuilderAnnotations, RelationalIndexBuilderAnnotations etc.
• Like @ErikEJ, I just copied Check.cs, that seems fine.

Strings

• I have cases everywhere where CoreStrings and RelationalStrings are used for cases which seem pretty cross-provider. As these strings are general, does it make sense to expose CoreStrings, or create another public provider-facing *Strings class? If not I can copy the strings into my provider, but it seems like needless duplication.

[ErikEJ]

It did not mention Strings (last paragraph above), but I have the same issue, just did not mention it.

[ajcvickers]

@ErikEJ @roji @caleblloyd I filed #11266 to capture the work we plan to do in 2.1 with regard to provider use of internal types. Can we continue any discussion there?

[smitpatel]

We discussed in design meeting and arrived at follow conclusions:
For our providers, we will use the package name as root namespace. So for SqlServer, it becomes Microsoft.EntityFrameworkCore.SqlServer.
The sub namespaces will append after the root following same hierarchy as EFCore.

The changes we have made as following

| Provider | Root Namespace |
|----------|---------------------|
| SqlServer | Microsoft.EntityFrameworkCore.SqlServer |
| Sqlite | Microsoft.EntityFrameworkCore.Sqlite |
| InMemory | Microsoft.EntityFrameworkCore.InMemory |
| OracleProvider | Microsoft.EntityFrameworkCore.Oracle |