Names and default names for keys, constraints and indices

[roji]

While working on #12821, I noticed some that may merit some discussion.

When scaffolding, RelationalScaffoldingModelFactory checks the database name for keys, constraints and indices, comparing them against the default name EF Core would assign to this index. The idea is presumably to avoid useless scaffolding HasName() calls which would bloat OnModelCreating().

Now, at least in the PostgreSQL case (but I think also the others), a name is always returned for keys/constraints/indices. PostgreSQL has its own defaults: primary keys that haven't been explicitly named are called {table}_pkey, etc. This means that for a typical database that has been created without specifying names, HasName() will be scaffolded everywhere because the PostgreSQL defaults aren't the same as the EF Core defaults (PK_{table}).

There seem to be two options:

• Current behavior: always take the name reported by the database. This bloats OnModelCreating() with HasName() calls on every single key, constraint and index. But it does have the advantage of round-trippability (exact names are preserved from the scaffolded database).
• Alternative behavior: have the database model factories omit names when those names are the database defaults, leading to null names and less HasName() everywhere. Since EF Core must specify names when creating keys/indices, it would see nullls and default create indexes and keys with its own defaults.

Basically it's a choice between round-trippability (current behavior) and a leaner OnModelCreating(). I'm not sure to what extent the former is an actual goal.

What do you guys think?

[ajcvickers]

@roji If I remember rightly, I think the idea was that the names generated by EF by default for PostgreSQL would match the "most common convention" for names created in that database. It sounds like for PostgreSQL this would be to match the names that the database will use if no name is provided.

However, it would be a breaking change to start generating the names in this way now, in that a new migration would be generated to rename all the constraints to their new values. We will discuss in triage what options there might be here.

[ajcvickers]

@bricelam to link related issues.

[roji]

I see... If I read the code right, that isn't possible currently, is it? I mean, the default name generation seems to be static methods which the providers cannot override etc.

Regardless yeah, it would be a breaking change but it definitely seems like the most correct way to deal with this... Maybe for 3.0?

[ajcvickers]

@roji I looked into this code and we effectively broke the extensiblity model here when we simplified the service provider and model building process in 2.0--dumping "one model to rule them all". This is definitely something we should look at for 3.0.

We also discussed this in triage, and I believe there is a desire to be able to reverse engineer in two ways:

• To a model that contains all information needed to then continue managing that database with Migrations. This mode would require thinks like constraint names to be included.
• To a model that will only be used by the EF runtime and therefore can be smaller because it doesn't include things that EF doesn't care about.

@bricelam is going to link issues related to this. I don't think we have decided exactly how APIs, switches, etc will look, nor what the default will be.

[ErikEJ]

I only care about: "To a model that will only be used by the EF runtime and therefore can be smaller because it doesn't include things that EF doesn't care about."

[roji]

I guess this is the right discussion to also think about round-trippability, which is related but not exactly identical to the first reverse-engineering "mode".

Personally having two different reverse-engineering modes (command-line switch?) seems a bit complicated too me, it will probably be difficult for users to understand etc. My gut feeling is that a bit of bloat in the model (stuff like constraint and index names) isn't that bad, or at least not bad enough to warrant a whole new mode. After all, if users don't care about that stuff they can just manually delete it from the model (although if you're working on "iterative" reverse-engineering to continue to update models that might get tricky as false changes get detected).

Just my two cents...

[bricelam]

Some notes:

With #10890 we could make the default reverse engineering experience not include constrint names. Part of #2167 is to add a gesture to Scaffold-Database to indicate you want to start using Migrations with it; this would signal that we should reverse engineer things like constraint names. The update model from database feature probably doesn't need the names since it doesn't need to rename anything (it just re-scaffolds everything) and a structural comparison is probably sufficient.

[ajcvickers]

Triage: keeping this open for 3.0 to look at:

• Re-enabling extensibility point for provider default constraint names
• Considering then what to do with the PostgreSQL provider, and possibly others--i.e. should they take a breaking change in 3.0?

Other aspects discussed here are tracked by #10890, #2167.

[ajcvickers]

@roji to investigate what we should do here.

[JonPSmith]

Can I add something to this debate. In summary I would like the following two options added to reverse engineering:

1. Have an option that includes the constraint names in the reverse engineering process
2. Add a feature like @ErikEJ ExtraOnModelCreating method that can be overridden in another partial class.

My use case for asking for this is as follows:

I am reverse engineering a couple of my client's database which were created via SQL scripts. To ensure that any changes to the database (which happens often) the plan is to run a unit test in the build process that uses my CompareEfSql class to check that the EF Core's Model is still in step with the database it accesses. Because my CompareEfSql class checks everything it throws out errors because the primary key constraint names are not the same.

I could use @ErikEJ EF Core Power Tools and use his ExtraOnModelCreating method to alter the primary keys using the approach suggested by Andrew Lock (I tried that and it worked great - Andrew's suggestion opens up some really useful features). The problem is the client wants a single command that they can run, which rules out EF Core Power Tools. I also have a big list of tables that should be scaffolded, so a command they can copy/paste is better.

Mt CompareEfSql tool has a feature to suppress errors so I can get around this, but I would prefer to either get the Primary Key constraint name to be set from the database, or (and!) have @ErikEJ overridable ExtraOnModelCreating method added to the created OnModelCreating.

[roji]

@JonPSmith as @ajcvickers wrote above, we probably want to have EF generate default constraint names that match the database's default. In other words, if you don't explicitly set constraint names in a code model, a PostgreSQL database created from that model would have PostgreSQL default constraint names, while an SQL Server database would have SQL Server's default constraint names.

If we go this way, then reverse engineering would probably behave accordingly - a default constraint name wouldn't result in a HasName(). This would provide both round-trippability and a lean model.

Unless I'm mistaken, there's nothing in the above that prevents something like CompareEfSql from comparing an EF Core model to an existing database - as long as the tool interprets the lack of an explicit constraint name in the model as a database default. Providers would possibly expose logic for letting tools such as CompareEfSql know what the default name generation strategy is, allowing your tool to understand if a constraint name encountered in the database is a default name or not.

Note that all this isn't finalized, just my idea of where we may want to go.

[JonPSmith]

Hi @roji,

Thanks for the quick feedback. Adding the constraint name if its not the default sound like a good idea and it would also solve my problem.

You might also consider what @ErikEJ does in his EF Core Power Tools, i.e. providing a method at the end of the OnModelCreating which calls an overridable method (@ErikEJ - can you provide info. I only found it by using your tool and I couldn't find it in your docs). That just gives you lots of room for doing complex things.

One minor point. CompareEfSql uses the DbContext's Model property, with entityType.GetIndexes() to get the indexes and then ...Relational().Name to get the constraint name. This always returns a constraint name - either the default or one provided by HasName(). That means I don't need to do anything in CompareEfSql special - it just works.

[roji]

Thanks for the quick feedback. Adding the constraint name if its not the default sound like a good idea and it would also solve my problem.

Great, I hope I'll get around to looking at this issue soon.

You might also consider what @ErikEJ does in his EF Core Power Tools, i.e. providing a method at the end of the OnModelCreating which calls an overridable method (@ErikEJ - can you provide info. I only found it by using your tool and I couldn't find it in your docs). That just gives you lots of room for doing complex things.

I assume you're asking for this because you want to re-scaffoled an existing context from a database which has changed, and want to make sure some configuration isn't overwritten. If so, take a look at #12245 which seems to be the same, but more in general, at #831 which is a more general approach to updating the model. This would definitely be out of scope for this issue.

One minor point. CompareEfSql uses the DbContext's Model property, with entityType.GetIndexes() to get the indexes and then ...Relational().Name to get the constraint name. This always returns a constraint name - either the default or one provided by HasName(). That means I don't need to do anything in CompareEfSql special - it just works.

I understand, but I'm not sure if that's the right behavior for default names. Imagine a model scaffolded from one database (e.g. SQL Server), used to create another (e.g. PostgreSQL); is seems ideal if the default constraint names from SQL Server are not scaffolded in any way, allowing PostgreSQL's default names to be used. In other words, in my current mental model (which may change) you would see null as the constraint name, and would need to reconstruct the database's default name, possibly with the provider's help.

[smitpatel]

https://github.com/aspnet/EntityFrameworkCore/blob/34a6ae7e583f9ed0d8ea57354824b1938945891f/src/EFCore.Relational/Metadata/RelationalKeyAnnotations.cs#L50-L51

Current code makes sure that you always get a constraint name.

[roji]

@smitpatel yeah, this is one reason this would be a breaking change. I'll think about this more in-depth and then we can discuss.

[JonPSmith]

@roji,

Thanks for the link to #12245, with @ErikEJ OnModelCreatingPartial. I do think that is a useful idea.

I understand, but I'm not sure if that's the right behavior for default names. Imagine a model scaffolded from one database (e.g. SQL Server), used to create another (e.g. PostgreSQL); is seems ideal if the default constraint names from SQL Server are not scaffolded in any way, allowing PostgreSQL's default names to be used. In other words, in my current mental model (which may change) you would see null as the constraint name, and would need to reconstruct the database's default name, possibly with the provider's help.

From my point of view EF Core's Model has to be database specific to be any use. It contains things like the ColumnType, DefaultValueSql and many more that have to be for the current database. I can't speak for others but I use Model to find out things about the database I'm trying to access, so it needs to reflect that database's schema.

[ErikEJ]

@JonPSmith You may be interested in this: ErikEJ/EFCorePowerTools#196

[AndriySvyryd]

This could be handled by default value conventions