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

EntityTypeBuilder ToView or not ToView #14787

Closed
AndriySvyryd opened this issue Feb 22, 2019 · 13 comments · Fixed by #17101
Closed

EntityTypeBuilder ToView or not ToView #14787

AndriySvyryd opened this issue Feb 22, 2019 · 13 comments · Fixed by #17101
Assignees
Labels
closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-enhancement
Milestone

Comments

@AndriySvyryd
Copy link
Member

Should it remain obsoleted or become a permanent synonym for ToTable?

@ajcvickers
Copy link
Contributor

ajcvickers commented Feb 25, 2019

Notes from triage: we will not obsolete ToView, but instead set it up as the means to indicate to migrations that a table should not be created for this entity type. For now, this means migrations will do nothing. In the future, overloads of ToView could allow the view to be specified so that it can be used by migrations to create it. (see #465)

@bricelam
Copy link
Contributor

bricelam commented May 3, 2019

See also #2725

@divega
Copy link
Contributor

divega commented May 6, 2019

Extract from #1679 (comment):

Tables and views are database concepts. Distinction between those two, IMHO, does not make sense in EF.

I think it is fair to say that EF Core tries to abstract common aspects of databases to enable customers to write code that queries and persists data in terms of entities, typically using a DbContext.
This abstraction is achieved with help from two-way mappings between database objects and entities, which are associated with a DbContext. I think it is natural and expected that the APIs that are used (typically in code that is part of the DbContext) to define those mappings often refer to database concepts.

My point is in modern relational databases functional distinction between tables and views is blurry.

I think it is true that at the database level tables and views can be used pretty much interchangeably (views may be more often read-only or have no keys, but we know that is not universally true, and I think we are all mostly in agreement that conflating these concepts was a bad idea).

The actual difference between tables and views is in how they are defined: while tables represent named storage areas with a specific schema, views are named virtual tables based on queries over other tables or views.

In alignment with all of this, our plan for EF Core 3.0 is that entities configured to map to views will have the exact same query and persistence capabilities as entities mapped to tables. The only difference between ToView and ToTable will be in what migrations and EnsureCreated will do with them:

  • If you use ToTable, a table will be created

  • If you use ToView, a table will not be created.

Ideally EF Core migrations and EnsureCreated should have the capability to create views instead, but for now that is not the case. We are missing migrations operations that represent DDL for views, and an API to associate a defining query with the view. Given this limitation, migrations and EnsureCreated will have to simply ignore (not try to create any DDL) for any database object introduced to the model with ToView.

And this is where I have my actual concerns with ToView:

If someday we want to add the capability to create database views, an argument providing a defining query for the view would be required, but is it ok to just implicitly ignore the object if it isn’t provided?

I think the connection between the ToView method and the behavior (database object is to be ignored by migrations and EnsureCreated) becomes too implicit and obscure. E.g.:

  • The behavior isn't conveyed at all by the name of the API, so it is not discoverable.

  • Once customers learn how it works, they could start calling ToView for tables in the database for which they don’t want EF Core to ever produce any DDL.

That might work, but would it really be ok? It reminds me of the kind of thing that happens when we make it too easy to conflate orthogonal concepts, e.g. when customers started trying to use the Query<T> API with tables that they wanted to treat as read-only, even if they had primary keys defined.

@ajcvickers
Copy link
Contributor

Discussed this again and per @divega's comments we are not going to make this change. We may bring .ToView back later when it actually has some semantics directly related to views, as opposed to just skipping inclusion in migrations.

@ajcvickers ajcvickers removed this from the Backlog milestone May 11, 2019
@ajcvickers ajcvickers added closed-out-of-scope This is not something that will be fixed/implemented and the issue is closed. and removed needs-design labels May 11, 2019
@divega
Copy link
Contributor

divega commented May 15, 2019

I had another idea that I like more because I believe it helps us keep a simple version of ToView() now, but in a way that doesn’t introduce confusion and can evolve gracefully as we add more capabilities. The principles would be:

  1. A call toToView() in the model is equivalent to ToTable() for query and update pipeline.

  2. For migrations and EnsureCreated, a call to ToView() in the model is supposed to lead to the creation of a database view, but since we still don’t have the mechanism that will allow passing the view definition, it will cause an exception for now. The exception should indicate that creating a view isn’t supported and that it can be suppressed by calling the API to suppress DDL on the entity, described in Ability to exclude/skip/ignore parts of the model from migrations so that a table is not created (for overlapping bounded contexts) #2725.

Hence the simple version of ToView() can now also be scaffolded by reverse engineering, with the caveat that it won’t round trip just yet.

Reopening so that we can discuss this again. Sorry about the randomization.

@divega divega reopened this May 15, 2019
@divega divega removed closed-out-of-scope This is not something that will be fixed/implemented and the issue is closed. punted-for-3.0 labels May 15, 2019
@ajcvickers
Copy link
Contributor

Triage: will discuss with @bricelam

@ajcvickers
Copy link
Contributor

After lengthy discussion on whether or not this would make the experience better we are going to punt for 3.0, and leave ToView obsoleted for now so we can still do whatever we decide in the future.

@bricelam
Copy link
Contributor

bricelam commented Aug 2, 2019

Open questions: What should migrations do with keyless entity types? Should calling ToView (obsolete) affect that behavior?

@bricelam bricelam removed this from the Backlog milestone Aug 2, 2019
@ajcvickers
Copy link
Contributor

For 3.0:

  • ToView - un-obsolete it for 3.0
    • Keyed entity types: no-op in Migrations (for now)
    • Keyless entity types: no-op in Migrations (for now)
  • ToTable ->
    • Keyed entity types: create table
    • Keyless entity types: create table

We also need to follow up on reverse engineering:

  • Views go to ToView
  • Keyless tables go to ToTable

@ajcvickers ajcvickers added this to the 3.0.0 milestone Aug 5, 2019
@ajcvickers ajcvickers assigned bricelam and unassigned divega and AndriySvyryd Aug 5, 2019
@ErikEJ
Copy link
Contributor

ErikEJ commented Aug 5, 2019

@bricelam does this mean adding a bool IsView to the Table class in DatabaseModel then?

@bricelam
Copy link
Contributor

bricelam commented Aug 5, 2019

Yep, or at least some way to tell the difference

@bricelam
Copy link
Contributor

bricelam commented Aug 5, 2019

Also worth thinking about where we’ll eventually put the view definition SQL

@bricelam
Copy link
Contributor

bricelam commented Aug 6, 2019

@ErikEJ I'm adding a DatabaseTable sub-type--DatabaseView--instead, so we can add things like ViewDefinition and IsUpdatable later.

@bricelam bricelam added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Aug 12, 2019
@ajcvickers ajcvickers modified the milestones: 3.0.0, 3.0.0-preview9 Aug 21, 2019
@ajcvickers ajcvickers modified the milestones: 3.0.0-preview9, 3.0.0 Nov 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants