dotnet / efcore

EF Core is a modern object-database mapper for .NET. It supports LINQ queries, change tracking, updates, and schema migrations.
https://docs.microsoft.com/ef/
MIT License
13.79k stars 3.19k forks source link

`Optimize-DbContext` Does Not Fully Qualify Types, Resulting in Compile Errors #25523

Closed Mike-E-angelo closed 3 years ago

Mike-E-angelo commented 3 years ago

File a bug

When using Optimize-DbContext, the generated types are not fully qualified and/or do not fully resolve the generated type, resulting in compile errors.

I have identified 3 scenarios:

  1. The model type extends a class which has the same name of the class it extends (e.g. IdentityUser )
  2. The model type uses the same name as a type found in the System namespace (e.g. Index)
  3. The model type has the same name as the namespace in which it is located (e.g. Definition.Definition)

Include your code

I have committed a SLN with the generated compile errors here for your review: https://github.com/Mike-E-angelo/Stash/blob/master/EfCore6.Model/EfCore6.Model.sln

Include stack traces

NA

Include verbose output

PM> Optimize-DbContext -OutputDir Optimized -Verbose
Using project 'EfCore6.Model'.
Using startup project 'EfCore6.Model'.
Build started...
Build succeeded.
C:\Program Files\dotnet\dotnet.exe exec --depsfile ...\Mike-E-angelo\Stash\EfCore6.Model\EfCore6.Model\bin\Debug\net6.0\EfCore6.Model.deps.json --additionalprobingpath C:\Users\micha\.nuget\packages --additionalprobingpath "C:\Program Files\dotnet\sdk\NuGetFallbackFolder" --runtimeconfig ...\Mike-E-angelo\Stash\EfCore6.Model\EfCore6.Model\bin\Debug\net6.0\EfCore6.Model.runtimeconfig.json C:\Users\micha\.nuget\packages\microsoft.entityframeworkcore.tools\6.0.0-preview.7.21378.4\tools\netcoreapp2.0\any\ef.dll dbcontext optimize --output-dir Optimized --verbose --no-color --prefix-output --assembly ...\Mike-E-angelo\Stash\EfCore6.Model\EfCore6.Model\bin\Debug\net6.0\EfCore6.Model.dll --project ...\Mike-E-angelo\Stash\EfCore6.Model\EfCore6.Model\EfCore6.Model.csproj --startup-assembly ...\Mike-E-angelo\Stash\EfCore6.Model\EfCore6.Model\bin\Debug\net6.0\EfCore6.Model.dll --startup-project ...\Mike-E-angelo\Stash\EfCore6.Model\EfCore6.Model\EfCore6.Model.csproj --project-dir ...\Mike-E-angelo\Stash\EfCore6.Model\EfCore6.Model\ --language C# --working-dir ...\Mike-E-angelo\Stash\EfCore6.Model --root-namespace EfCore6.Model --nullable
Using assembly 'EfCore6.Model'.
Using startup assembly 'EfCore6.Model'.
Using application base '...\Mike-E-angelo\Stash\EfCore6.Model\EfCore6.Model\bin\Debug\net6.0'.
Using working directory '...\Mike-E-angelo\Stash\EfCore6.Model\EfCore6.Model'.
Using root namespace 'EfCore6.Model'.
Using project directory '...\Mike-E-angelo\Stash\EfCore6.Model\EfCore6.Model\'.
Remaining arguments: .
Finding DbContext classes...
Finding IDesignTimeDbContextFactory implementations...
Found IDesignTimeDbContextFactory implementation 'StorageBuilder'.
Found DbContext 'ApplicationState'.
Finding application service provider in assembly 'EfCore6.Model'...
Finding Microsoft.Extensions.Hosting service provider...
No static method 'CreateHostBuilder(string[])' was found on class 'Program'.
No application service provider was found.
Finding DbContext classes in the project...
Using DbContext factory 'StorageBuilder'.
Using context 'ApplicationState'.
Finding design-time services referenced by assembly 'EfCore6.Model'...
Finding design-time services referenced by assembly 'EfCore6.Model'...
No referenced design-time services were found.
Finding design-time services for provider 'Microsoft.EntityFrameworkCore.SqlServer'...
Using design-time services from provider 'Microsoft.EntityFrameworkCore.SqlServer'.
Finding IDesignTimeServices implementations in assembly 'EfCore6.Model'...
No design-time services were found.
The index {'UserId'} was not created on entity type 'IdentityUserRole<int>' as the properties are already covered by the index {'UserId', 'RoleId'}.
The index {'UserId'} was not created on entity type 'IdentityUserToken<int>' as the properties are already covered by the index {'UserId', 'LoginProvider', 'Name'}.
The property 'Index.UserId' was created in shadow state because there are no eligible CLR members with a matching name.
The property 'User.DefinitionId' was created in shadow state because there are no eligible CLR members with a matching name.
Successfully generated a compiled model, to use it call 'options.UseModel(ApplicationStateModel.Instance)'. Run this command again when the model is modified.
'ApplicationState' disposed.

Include provider and version information

EF Core version: 6.0.0-preview.7.21378.4 Database provider: Microsoft.EntityFrameworkCore.SqlServer Target framework: net6.0 Operating system: Windows 10 IDE: Visual Studio 2022 Preview 3.0

AndriySvyryd commented 3 years ago

This is something we can't fix automatically without drastically changing the infrastructure to use Roslyn. This is also that should be rather rare as all the listed cases are anti-patterns.

Workaround: Starting with 6.0.0-rc1 there is a way of achieving this by creating a class deriving from CSharpHelper and overriding ShouldUseFullName. Then registering the implementation as ICSharpHelper, see Design-time services.

Mike-E-angelo commented 3 years ago

Thank you very much for looking into this, @AndriySvyryd!

This is also that should be rather rare as all the listed cases are anti-patterns.

Source for this, please? It would seem if that were the case, warnings would be emitted by either Roslyn or R# during design/compile time, neither of which do so.

Either way, one developer's anti-pattern is another's strong use of the .NET type system. :P

Then register the implementation as ICSharpHelper, see Design-time services.

Cool, thank you for letting me know. 👍

AndriySvyryd commented 3 years ago

Source for this, please? It would seem if that were the case, warnings would be emitted by either Roslyn or R# during design/compile time, neither of which do so.

https://docs.microsoft.com/en-us/dotnet/standard/design-guidelines/names-of-namespaces

❌ DO NOT give the same name to types in namespaces within a single application model. ❌ DO NOT introduce generic type names such as Element, Node, Log, and Message. ❌ DO NOT use the same name for a namespace and a type in that namespace.

AndriySvyryd commented 3 years ago

BTW about https://devblogs.microsoft.com/dotnet/announcing-entity-framework-core-6-0-preview-5-compiled-models/#comment-10060

I couldn't answer because comments are now closed. You can use this to get your model "size":

var model = context.Model;

Console.WriteLine("Model has:");
Console.WriteLine($"  {model.GetEntityTypes().Count()} entity types");
Console.WriteLine($"  {model.GetEntityTypes().SelectMany(e => e.GetDeclaredProperties()).Count()} properties");
Console.WriteLine($"  {model.GetEntityTypes().SelectMany(e => e.GetDeclaredForeignKeys()).Count()} relationships");

But the benefit you get from a compiled model also depends on the shape of your model. In general if the startup time wasn't bothering you that much previously you probably won't notice a difference with a compiled model.

Mike-E-angelo commented 3 years ago

I appreciate you providing this information, @AndriySvyryd. I see now that you are talking guidelines and not analysis rules/warnings, which I would expect to be applied in the case of anti-patterns (although who knows).

DO NOT give the same name to types in namespaces within a single application model.

I suppose we'd have to really discern what is considered an application model here. In the case above, IdentityUser is inheriting another IdentityUser located in another referenced assembly. I can see the case here for not having two `IdentityUser' within the same assembly/project, but IMO that is not applicable here as an application-specific class is inheriting from a generalized framework class located in another assembly altogether.

DO NOT introduce generic type names such as Element, Node, Log, and Message.

So System.Index violates this guideline? 🙃

DO NOT use the same name for a namespace and a type in that namespace.

Yeah, TBH I am not a fan of this one. It actually bothers me a bit. 😬 It seems both redundant and lazy. It also makes me want to rename it just by looking at it. Nonetheless, it's still technically/syntactically correct so thought it would be a good use case to report.

I couldn't answer because comments are now closed. You can use this to get your model "size":

That is very cool! Thank you very much. I was interested in seeing how many relationships are found within the # of properties.

I am in a bit of a refactor ATM (trying to un-scope my DbContexts and replace with IDbContextFactory), but will try to get that together and see what the results are.

Thank you for taking the time to assist. 👍

Mike-E-angelo commented 3 years ago

Not to be a stickler here. 😁

There's also this one, which I think better applies to Index:

❌ DO NOT give types names that would conflict with any type in the Core namespaces.

I'm not a fan of this rule as there are SO many core namespaces/types and it's so very easy to meet a type name that already exists. Also, what happens when there's a new "Core" type that is introduced at some point later that conflicts with a type you've already created? This is the whole point of namespaces.

Along those lines, my feeling is that a lot of these guidelines are made to assist the tooling, which has been very deficient around these types of issues over the years. One of the many reasons I use ReSharper. :)

AndriySvyryd commented 3 years ago

Along those lines, my feeling is that a lot of these guidelines are made to assist the tooling

They are mostly for the benefit of the ones reading the code. I'd add a more general guideline - avoid type names that will need to be namespace-qualified. But as you said these are guidelines, not rules.

Mike-E-angelo commented 3 years ago

They are mostly for the benefit of the ones reading the code

Nodding along, @AndriySvyryd. I appreciate the discussion and hearing your viewpoint.

FWIW my stats are as follows:

Model has:
  126 entity types
  451 properties
  117 relationships
  568 total compilation targets

So at only 568 targets, it would seem I would not benefit much from a compiled model. This aligns with my observations when attempting to utilize a compiled model from my context. This would only yield ~114ms of startup time savings, if I understand correctly.

AndriySvyryd commented 3 years ago

This would only yield ~114ms of startup time savings, if I understand correctly.

This sounds about right.

ghosttie commented 2 years ago

Are there downsides to overriding CSharpHelper.ShouldUseFullName? Is there other generated code that's going to be wrong if we do that?

AndriySvyryd commented 2 years ago

Are there downsides to overriding CSharpHelper.ShouldUseFullName?

Not really, just potentially uglier code for compiled models and migration snapshots.

ghosttie commented 2 years ago

Is there a way we could make it smarter, so it only applies to Optimize-DbContext?

AndriySvyryd commented 2 years ago

Is there a way we could make it smarter, so it only applies to Optimize-DbContext?

No.

HakanL commented 2 years ago

@AndriySvyryd I'm facing an issue where I want to use this feature, but with an existing database that I can't modify, so even if there may be some anti-patterns, it is what it is but the Optimize-DbContext generates code that doesn't compile (due to not having full names in the references). I'm not sure I would create CSharpHelper.ShouldUseFullName when calling Optimize-DbContext? IMHO it would be nice with a parameter to Optimize-DbContext that specifies that full names should be used, is that feasible?

Mike-E-angelo commented 2 years ago

Workaround: Starting with 6.0.0-rc1 there is a way of achieving this by creating a class deriving from CSharpHelper

Hi @AndriySvyryd I am investigating this again and cannot seem to find this class. Can you point me in the direction of where it's located?

ErikEJ commented 2 years ago

In EFCore.Design

Mike-E-angelo commented 2 years ago

I see ICSharpHelper but not CSharperHelper. Do you have a full path and/or URL for reference @ErikEJ?

Mike-E-angelo commented 2 years ago

Ah, here we go: https://github.com/dotnet/efcore/blob/main/src/EFCore.Design/Design/Internal/CSharpHelper.cs

I was looking here: https://github.com/dotnet/efcore/tree/main/src/EFCore/Design/Internal

HakanL commented 2 years ago

I'm now on EF Core 6.0.6 and this part works great, but there's still one issue. I have a table named DayOfWeek (which in hindsight wasn't great, but I can't change it now). The problem is that the optimized CompiledModule causes a conflict between my own data model class for DayOfWeek and System.DayOfWeek in the auto-generated file. I have to manually add this line into the file to work around the issue. It would be good if this generated file from the Optimize-DbContext would always fully-qualify all types, at least as an option if not default. using DayOfWeek = Skumberg.DataModel.Models.DayOfWeek;

AndriySvyryd commented 2 years ago

This tracks adding a simple commandline switch - https://github.com/dotnet/efcore/issues/27203

dharmeshtailor commented 1 year ago

Ah, here we go: https://github.com/dotnet/efcore/blob/main/src/EFCore.Design/Design/Internal/CSharpHelper.cs

I was looking here: https://github.com/dotnet/efcore/tree/main/src/EFCore/Design/Internal

How do you reference this in the code? I am not able to find the CSharpHelper in "Microsoft.EntityFrameworkCore.Design.Internal". Am I missing something?

AndriySvyryd commented 1 year ago

How do you reference this in the code? I am not able to find the CSharpHelper in "Microsoft.EntityFrameworkCore.Design.Internal". Am I missing something?

Add a reference to Microsoft.EntityFrameworkCore.Design

dharmeshtailor commented 1 year ago

How do you reference this in the code? I am not able to find the CSharpHelper in "Microsoft.EntityFrameworkCore.Design.Internal". Am I missing something?

Add a reference to Microsoft.EntityFrameworkCore.Design

Here are the references:

image

Here's the error:

image

AndriySvyryd commented 1 year ago

@dharmeshtailor Did you try recompiling? If you are getting the EF1001 warning then the type is referenced correctly.

dharmeshtailor commented 1 year ago

@AndriySvyryd this is what I got on recompiling. It seems reference is correct.

I have .net 7.0 class library.

image

dharmeshtailor commented 1 year ago

@AndriySvyryd I have created a sample solution having same issue.

WebApplication4.zip

AndriySvyryd commented 1 year ago

Oh, it's because Microsoft.EntityFrameworkCore.Design is referenced as a build-only dependency. Edit the .csproj and replace

    <PackageReference Include="Microsoft.EntityFrameworkCore.Design" Version="7.0.5">
      <PrivateAssets>all</PrivateAssets>
      <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
    </PackageReference>

with

<PackageReference Include="Microsoft.EntityFrameworkCore.Design" Version="7.0.5" />
dharmeshtailor commented 1 year ago

@AndriySvyryd Works perfectly. Thanks. :-)