-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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.
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
Inconsistent behavior related to relationship cycle #32422
Comments
I've found this happening to several of our code bases, this worked in EF6 and EF7. It's trivial to reproduce having an entity like this:
|
/cc @AndriySvyryd |
Same case here. The issue is preventing us from upgrading to .Net 8. |
I just tried debugging this in EF 7. It turns out the only reason it works is because |
The first part was a deliberate breaking change, the second part (when generating the next migration) wasn't |
@AndriySvyryd It seems that the current algorithm breaks the loop when we get back to the property we've originally started with or the next property would be the same as the current one. Wouldn't it be better to maintain a set of the properties we've already traversed and break when we get to a property already in the set? Here you can see exactly what I mean: I tried it out and it seemed to solve the problem presented in the original post. I know the extra allocation wouldn't do any good for performance but it can be optimized so that the allocation only gets done if it's actually needed. |
It would be more accurate, but would have a measurable negative perf impact. We can improve the cycle breaking logic without tracking more than 1 property. |
…hen not starting on one. Additionally, detect multiple relationship chains that end with incompatible conversions. Fixes #32422
…hen not starting on one. Additionally, detect multiple relationship chains that end with incompatible conversions. Fixes #32422
…hen not starting on one. Additionally, detect multiple relationship chains that end with incompatible conversions. Fixes #32422
…tor. Don't allocate Queue unless necessary. Fix an issue with configuration of an FK to the base type. Part of #32422
…tor. Don't allocate Queue unless necessary. Fix an issue with configuration of an FK to the base type. Part of #32422
…tor. Don't allocate Queue unless necessary. Fix an issue with configuration of an FK to the base type. Part of #32422
…hen not starting on one. Additionally, detect multiple relationship chains that end with incompatible conversions. Fixes #32422
This issue also surfaces during scaffolding. I try to scaffold an Oracle database created with EF (not-core) 6. Scaffolding works with Microsoft.EntityFrameworkCore.Relational 7.0.14 and Oracle.EntityFrameworkCore 7.21.12, but fails with Microsoft.EntityFrameworkCore.Relational 8.0.0 and Oracle.EntityFrameworkCore 8.21.121. This is quite inconvenient, because there is not much I can do about the existing database structure (which works fine, by the way). So this new behavior de facto breaks scaffolding for my database. When I create a DbContext with EF 7 and then upgrade to EF 8 I get a runtime error, but at least I can now try to adapt the structure to get a consistent EF 8 model. |
@Peter-B- I think this will be fixed in 8.0.2 - and how is it "failing"? |
Hi @ErikEJ, When I run
No models or db context are created. I saw that there are PRs for 8.0.2. I just wanted to bring to your attention, that this also affects scaffolding. Thanks for your prompt reply and the fast fix |
Hello,
Take the following
DbContext
:When I want to add a migration using
dotnet ef migrations add Init --verbose
, it works fine. Let's add another entity:A new migration can no longer be added, with the following error message:
The new entity did not introduce any new relationship cycles, yet EF was able to handle the previous one just fine. So I started digging. The exception is thrown here:
https://github.com/dotnet/efcore/blob/45673126512a0fe99ef73bf5c1e5701012fd9c26/src/EFCore/Metadata/Internal/Property.cs#L812C22-L812C22
There is this condition:
In the first case, on the second cycle, this condition is triggered with
principalProperty == this
, and the function returns. In the second case, with the additional entity, this does not happen, and an exception is thrown. I am not sure why this behaves like this. If the goal was to detect cycles and throw, it fails to throw on the trivial cycle in the first example. If the goal was to break cycles, it throws anyways if there is a "leaf", as in a related entity that is not included in the cycle.So let's try following the recommendation in the exception, and add value converters to the properties involved:
This works, and the migration is generated as expected. We are not out of the woods yet, however, as
dotnet ef database update --verbose
will fail with the following error:Looks familiar, but the stack is very different. It turns out that the generated migration conveniently forgot about the value converters:
And when this is loaded, the annotation is once again missing. We could work around this by overriding the implementation of AnnotationCodeGenerator:
This will include a null-valued annotation in the generated code for ValueConverter and ProviderClrType, and the migration will run:
Once again, I don't know whether this is intentional or not. The recommended workaround does not survive code generation, and overriding methods in AnnotationCodeGenerator seems hacky at best. To me, this whole thing smells like a bug somewhere, but I am not familiar enough with the codebase to pinpoint exactly where.
Provider and version information
EF Core version: 8.0
Database provider: Npgsql.EntityFrameworkCore.PostgreSQL
Target framework: .NET 8.0
Operating system: Ubuntu 22.04
IDE: JetBrains Rider 2023.2.3
The text was updated successfully, but these errors were encountered: