Consider moving log messages/warnings for requiredness of navigation properties to model validation

[divega]

We currently log messages or warning for some conditions while executing conventions (see discussion in #15499). For example:

• If we switch principal-dependent ends because we find [Required] or non-nullable on a navigation,
• If [Required] or non-nullability appears on navigations on both sides of a relationship

However the checks are limited to metadata that the conventions reason about and fail to detect some ambiguous scenarios, e.g.:

• The convention detects [Required] (or non-nullability) on a navigation and makes the target of the navigation the principal, but that principal contains a good FK candidate property
• The convention detects [Required] (or non-nullability) on a navigation that is later made to point to the dependent via explicit configuration

Discussing this with @AndriySvyryd and @smitpatel one of the conclusions was if we postpone the analysis to model validation we could implement more robust detection of ambiguous configurations.

[jzabroski]

[Priority Low] Sorry if only tangential, but why do some InvalidOperationException occur when calling db.Database.EnsureDeleted();? I read the docs for EnsureDeleted, but they don't really explain this behavior. I would expect my exceptions to be raised in db.Database.EnsureCreated();

From: https://docs.microsoft.com/en-us/dotnet/api/microsoft.entityframeworkcore.storage.idatabasecreator.ensuredeleted?view=efcore-2.1

Warning: The entire database is deleted an no effort is made to remove just the database objects that are used by the model for this context.

Similarly, searching GitHub issues, I don't see questions being asked about this. - I found one weird scenario where a user was calling EnsureCreated without EnsureDeleted, but that's it.

[smitpatel]

[Priority Low] Sorry if only tangential, but why do some InvalidOperationException occur when calling db.Database.EnsureDeleted();?

What does the exception message say? Without that it is hard to find exact cause. Though I believe you can get error (not necessarily IOE) if your connection string is incorrect or services are not registered properly.

[jzabroski]

Hi @smitpatel - I have just submitted a bug #15553 which demonstrates the behavior. The bottom of the stack trace is very explicit that EFCore is eagerly loading things maybe it shouldn't. For me, delete is a drop database command, albeit for SQL Server, this can be agonizingly difficult.

[smitpatel]

It is model building. Service wiring causes it. Same service provides functionality for create/delete database. Create depends on the model so delete is also depending on model right now. In what case do you think, it would be helpful that EnsureDeleted worked successfully when nothing else can work in EF Core?

[jzabroski]

When you phrase it that way, perhaps nothing. But my thought process was, when developing locally, I do not want a database to exist at all if I'm prototyping something. I want Groundhog's Day.

As an analogy: If I've changed something and my program won't compile, Visual Studio asks me if I want to explicitly run the executable from the last reliable bin output.

I suppose phrased as an analogy, this is more of a feature request, but I was expecting EnsureDeleted to really mean EnsureDeleted, so that I have a Groundhog's Day environment for each test.

[smitpatel]

I do not want a database to exist at all if I'm prototyping something. I want Groundhog's Day.

• If it is very first iteration then database does not exist at all.
• EnsureDeleted/Created are used in pair. You want to drop the database before you create so that you can change schema. No harm in EnsureDeleted failing because Created would fail anyway.
• If you don't want the database to exist when program finishes then it should be in cleanup phase of program.
  So there are ways to achieve it with does not impact usability in any way.

I would analogy is incorrect in this case: VS suggests that because it still has the older executable (which was supposed to be replaced by the newer but compilation failed). If you want to use EF Core that way then EF Core has to store the model on disk, when model building fails, it can suggest do you want to use the older model we have available. I don't think that would help any user. (Honestly I never ran executable from last out in VS. Never found it useful. I feel that dialogue box shouldn't be there. But my personal opinion)

[smitpatel]

There is nothing wrong in your expectation. But it would require factoring EnsureDeleted out in different service which does not depend on model. It would increase complexity of codebase (and for provider writers). The cost may not worth the value.

[jzabroski]

Hmm. I guess the question is if there is a side effect. I was not thinking of the fix being a breaking change. Originally my curiosity was how you could build part of the model while deleting the models target. I don't know what the Internals are doing but if you're matching up with the database, why would you want to take a dependency on that. Even if there is a logical sequence, that sequence may not be threadsafe. In the extreme it would be helpful to dump the model to disk at any "check point" in the process. I once built an ORM that was recursively implemented by a micro-ORM where all the features were stored as Relations and relvars, and it had such a checkpoint feature. The ORM was proprietary, but the anecdote is merely to suggest it's doable.

…

On Wed, May 1, 2019, 6:09 PM Smit Patel ***@***.***> wrote: There is nothing wrong in your expectation. But it would require factoring EnsureDeleted out in different service which does not depend on model. It would increase complexity of codebase (and for provider writers). The cost may not worth the value. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#15539 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADNH7N3ORT5SU7DDK36ZHLPTIIK3ANCNFSM4HJQUECA> .

[jzabroski]

To clarify, in my mind, the non-breaking change would be adding an optional parameter to EnsureDeleted(bool validateModel = true), and callers like me would pass in EnsureDeleted(false). I don't love this, since, as I mentioned, it is not 100% clean, but it would theoretically address concerns with loading the model prematurely and/or in non-thread safe ways. Either way, this is not the end of the world, it just means my tests are more complicated.

[smitpatel]

EnsureDeleted(bool validateModel = true)

Cannot. DatabaseCreator is the service which execute EnsureDeleted. And it takes IModel as service through DI. Regardless of what method you call (even if you don't call any method), the moment you ask for DatabaseCreator service, model will build and throw exception if any. Hence it requires to put EnsureDeleted in a different service altogether which does not depend on IModel.

[jzabroski]

Ew. Well, not much to be said then. If log messages/warnings could be accompanied with a dump snapshot feature, that would probably be helpful. For example, say I create my database with FluentMigrator and map it with EF Code-First migrations and make a silly mistake like passing in "PropertyName" instead of nameof(ClassName.PropertyName). It would be nice to be able to log snapshot, similar to how Serilog and Its.Log provide rich logging. Even if it only exists as a Sample outside the main code path.

To be honest, given a set of classes and a set of tables, there are only so many options possible. I'm surprised nobody has invented Wix for ORMs.

[ajcvickers]

Notes from triage: we need to have a design meeting about what [Required] on a navigation property actually means.