Skip to content
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

Re-consider One Model to Rule Them All #7166

Closed
ajcvickers opened this issue Nov 30, 2016 · 2 comments
Closed

Re-consider One Model to Rule Them All #7166

ajcvickers opened this issue Nov 30, 2016 · 2 comments
Labels
breaking-change closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. providers-beware type-cleanup
Milestone

Comments

@ajcvickers
Copy link
Member

Many moons ago we decided to follow the path of One Model to Rule Them All. That is, the ability to create a single model that can be used by all providers. To this end:

  • Annotations and the APIs around them were implemented such that any provider-specific annotations could co-exist on the model with provider-specific annotations for a different provider.
  • Provider-specific conventions would not structurally change the model.

However, several aspects of where we have ended up mean that we may want to reconsider this:

  • The TPH implementation introduces a discriminator property, thereby structurally changing the model.
  • By default, we create and cache one model per provider, so even though it is possible to create one model for many providers, the usual path is not this.
  • We have backed off on automatic Migration generation/scaffolding for all registered providers.
  • We have not implemented running conventions for all registered providers, so we currently only run the conventions for the current provider.
  • The switch to an internal service provider by default means we are considering changing the provider model to allow only one provider per internal service provider.

Switching to one model per provider would have the following advantages:

  • Provider-specific conventions could structurally change the model without issues. (We have, as far as we know, worked around the discriminator issues so that, at least for the in-memory provider, it is okay to have the discriminator property present. However, it really still doesn't make sense that it is there, and beyond the discriminator issue there could be future conventions that generate similar problems.)
  • Provider-specific behaviors could be built into the model more explicitly. For example, rather than adding abstract information indicating that some form of provider-specific value generation is needed, the model could have a specific value generator set at model building time, removing the need for value generator selection.
  • We would not need to implement the code to run all provider conventions, and we would not need to deal with any conflicts resulting from this.
  • We could simplify the annotation story, especially for relational annotations, by only ever putting in the "Relational:" annotation. If SQLite needs a different annotation value than SQL Server, then this would be automatically handled by having two different models. The big advantage of this is that relational-level consuming code does not need to indirect through the provider to read relational values.
  • If we wanted, we could remove the ForSqlServerColumnType methods and instead have a general way of testing what the current provider is in OnModelCreating. This would also allow core things like MaxLength to vary by provider in a natural way.

Note that provider-agnostic migrations would still be possible in the same way they are today--by adding code to switch on the current provider in an edited migration.

@rowanmiller rowanmiller added this to the 1.2.0 milestone Dec 7, 2016
@ajcvickers ajcvickers modified the milestones: 2.0.0-preview1, 2.0.0 Apr 19, 2017
@ajcvickers ajcvickers modified the milestones: 2.0.0-preview2, 2.0.0 May 15, 2017
ajcvickers added a commit that referenced this issue May 18, 2017
Part of #7166

Since we now build one model per database provider there is no need to allow relational and provider-specific overrides to exist together in the same model. This means that any relational annotation will always be prefixed with "Relational:" and never with, for example, "SqlServer:". Provider-specific annotations still use the provider prefix.

This particularly advantageous for the consumer, since to read, for example, the table name, a consumer only ever needs to read `entityType.Relational().TableName;` instead of having to handle the case where it has been overridden for the current provider.

Note: there is other API (e.g. ForSqlServerTableName) than can also be removed; that will be part of another PR.
ajcvickers added a commit that referenced this issue May 19, 2017
Part of #7166

Since we now build one model per database provider there is no need to allow relational and provider-specific overrides to exist together in the same model. This means that any relational annotation will always be prefixed with "Relational:" and never with, for example, "SqlServer:". Provider-specific annotations still use the provider prefix.

This particularly advantageous for the consumer, since to read, for example, the table name, a consumer only ever needs to read `entityType.Relational().TableName;` instead of having to handle the case where it has been overridden for the current provider.

Note: there is other API (e.g. ForSqlServerTableName) than can also be removed; that will be part of another PR.
ajcvickers added a commit that referenced this issue May 22, 2017
Part of #7166

Main scenario here is conditional behavior in OnModelCreating now that we build a different model per provider.
ajcvickers added a commit that referenced this issue May 23, 2017
Part of #7166

Main scenario here is conditional behavior in OnModelCreating now that we build a different model per provider.
ajcvickers added a commit that referenced this issue May 24, 2017
Part of #7166

Main scenario here is conditional behavior in OnModelCreating now that we build a different model per provider.
ajcvickers added a commit that referenced this issue May 25, 2017
Part of #7166

Code can instead use
```C#
if (Database.IsSqlServer())
{
    modelBuilder.HasColumnName("MySqlServerName");
}
```
ajcvickers added a commit that referenced this issue May 26, 2017
…itly

Issue #8034, possible because of #7166

The idea here is to generate type mappings at model building type and then store them in the model. This allows `property.RelationalAnnotationNames().ColumnType` to always return the store type.

The solution is not perfect:
- A convention runs when the model is built, which means that ColumnType will remain null during OnModelCreating. This could be changed by lots of conventions that run whenever something that could change the mapping is changed.
- Because this is done by convention, and because conventions are not always run (e.g. ModelSnapshot, HistoryTable, using the ModelBuilder directly with an empty convention set) it means sometimes type mappings are not available. However, in the normal application scenario, they will always be available.

I will file an issue to look at these things post 2.0.
ajcvickers added a commit that referenced this issue May 26, 2017
…itly

Issue #8034, possible because of #7166

The idea here is to generate type mappings at model building type and then store them in the model. This allows `property.RelationalAnnotationNames().ColumnType` to always return the store type.

The solution is not perfect:
- A convention runs when the model is built, which means that ColumnType will remain null during OnModelCreating. This could be changed by lots of conventions that run whenever something that could change the mapping is changed.
- Because this is done by convention, and because conventions are not always run (e.g. ModelSnapshot, HistoryTable, using the ModelBuilder directly with an empty convention set) it means sometimes type mappings are not available. However, in the normal application scenario, they will always be available.

I will file an issue to look at these things post 2.0.
ajcvickers added a commit that referenced this issue May 26, 2017
…itly

Issue #8034, possible because of #7166

The idea here is to generate type mappings at model building type and then store them in the model. This allows `property.RelationalAnnotationNames().ColumnType` to always return the store type.

The solution is not perfect:
- A convention runs when the model is built, which means that ColumnType will remain null during OnModelCreating. This could be changed by lots of conventions that run whenever something that could change the mapping is changed.
- Because this is done by convention, and because conventions are not always run (e.g. ModelSnapshot, HistoryTable, using the ModelBuilder directly with an empty convention set) it means sometimes type mappings are not available. However, in the normal application scenario, they will always be available.

I will file an issue to look at these things post 2.0.
ajcvickers added a commit that referenced this issue May 26, 2017
Part of #7166

Code can instead use
```C#
if (Database.IsSqlServer())
{
    modelBuilder.HasColumnName("MySqlServerName");
}
```
@ajcvickers
Copy link
Member Author

Done enough for preview2, moving out for any remaining work.

@ajcvickers ajcvickers modified the milestones: 2.0.0, 2.0.0-preview2 May 26, 2017
ajcvickers added a commit that referenced this issue May 26, 2017
…itly

Issue #8034, possible because of #7166

The idea here is to generate type mappings at model building type and then store them in the model. This allows `property.RelationalAnnotationNames().ColumnType` to always return the store type.

The solution is not perfect:
- A convention runs when the model is built, which means that ColumnType will remain null during OnModelCreating. This could be changed by lots of conventions that run whenever something that could change the mapping is changed.
- Because this is done by convention, and because conventions are not always run (e.g. ModelSnapshot, HistoryTable, using the ModelBuilder directly with an empty convention set) it means sometimes type mappings are not available. However, in the normal application scenario, they will always be available.

I will file an issue to look at these things post 2.0.
@ajcvickers
Copy link
Member Author

Looked at the value generator stuff and decided to not change it for now. Some generators need to access scoped services, which can't be done in an obvious way if they are configured directly on the model. Maybe come 3.0 we'll revisit... ;-)

@ajcvickers ajcvickers added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label May 27, 2017
@ajcvickers ajcvickers modified the milestones: 2.0.0-preview2, 2.0.0 May 27, 2017
@ajcvickers ajcvickers modified the milestones: 2.0.0-preview2, 2.0.0 Oct 15, 2022
@ajcvickers ajcvickers removed their assignment Sep 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. providers-beware type-cleanup
Projects
None yet
Development

No branches or pull requests

2 participants