Refactor ConventionDispatcher to allow delayed execution #7747
Conversation
|
This change adds a visitor to run conventions, so from this point on @anpete will be able to contribute to the model building code 😄 |
| { | ||
| return RunOnEntityTypeAdded(entityTypeBuilder); | ||
| } | ||
|
|
There was a problem hiding this comment.
Consider having a ConventionScope (or some sugar API) that doesn't batch to avoid having to do this conditional every place.
| { | ||
| Run(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Is it OK to Run if an exception is thrown and Dispose happens as part of unwinding the stack?
There was a problem hiding this comment.
The conventions should be robust enough to not throw even if the model is in a bad state due to an exception. All of our negative test cases work correctly. But in theory we should avoid running them after an exception, though I'd have to figure out how this can be detected.
There was a problem hiding this comment.
I think it's fine to leave as is unless we discover a real issue with it in practice.
| private void Run() | ||
| { | ||
| _ran = true; | ||
| while (true) |
There was a problem hiding this comment.
while (true) [](start = 15, length = 13)
Could we avoid an infinite loop here by stopping (or at least warning) if this goes on for a very large number of iterations?
There was a problem hiding this comment.
We can throw. I don't think a warning will do much of a difference.
Is 65536 a reasonable limit?
There was a problem hiding this comment.
The exception message would be
The convention invocations have reached the recursion limit. This is likely an issue in EF Core, please report it.
There was a problem hiding this comment.
Add a fwdlink for where to report it? @rowanmiller Maybe also suggest to include repro code and version of EF being used--we could include the version in this message.
| /// This API supports the Entity Framework Core infrastructure and is not intended to be used | ||
| /// directly from your code. This API may change or be removed in future releases. | ||
| /// </summary> | ||
| public Reference([CanBeNull] T @object, [CanBeNull] IReferenceRoot<T> root) |
There was a problem hiding this comment.
Is this CanBeNull true? Other constructor and setter has notnull
There was a problem hiding this comment.
Oops, the other one CanBeNull as well
| @@ -8,6 +8,7 @@ | |||
| using Microsoft.EntityFrameworkCore.Internal; | |||
| using Microsoft.EntityFrameworkCore.Metadata.Internal; | |||
| using Microsoft.EntityFrameworkCore.Utilities; | |||
| using Microsoft.EntityFrameworkCore.Metadata.Conventions.Internal; | |||
|
|
|
Now conventions are even more magic |
|
@smitpatel I think of it more as rule enforcement for conventions, so they will become regulated magic |
AndriySvyryd
force-pushed
the
ConventionsWorkflow
branch
from
037cd41 to
cc24376
Compare
…ns and tracking objects modified by conventions. When set on ConventionDispatcher ConventionScope captures the conventions to be executed. ConventionBatch sets a ConventionScope and executed the stored conventions when it's run or disposed. When conventions captured in a ConventionScope are executed any triggered conventions are captured in a new ConventionScope to avoid keeping all the convention chain on the stack. This changes the order of convention execution which exposed some issues in code and tests Allow specifying the dependend end without specifying the FK properties, Throw when the same property is specified several times when adding a key, foreign key or indexes. Remove Mock usage from ConventionDispatcherTest Part of #214
AndriySvyryd
force-pushed
the
ConventionsWorkflow
branch
from
cc24376 to
1243426
Compare
Part of #214