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

To vs From methods: Proposal to rationalize ToTable, ToQuery, ToView, FromSql, and other related methods #17270

Closed
Tracked by #827
divega opened this issue Aug 19, 2019 · 6 comments · Fixed by #20054
Closed
Tracked by #827
Labels
area-query closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-enhancement
Milestone

Comments

@divega
Copy link
Contributor

divega commented Aug 19, 2019

Problems

  1. Predicting what the outcome for the query, update and DDL pipelines is when a combination of ToTable, ToView and ToQuery is used requires understanding a growing set of arbitrary rules.
  2. There are scenarios that we want to support with the current combinations that actually aren't possible. For example:
    1. We want to re-enable using defining query with keyed entity types which brings back the question of what happens with the update pipeline by default
    2. We want to allow configuring a table to be used for CUD operations and as the root of the defining query (e.g. the defining query only adds a filter and sort order), and have the table still created by the DDL pipeline.
    3. Allow a table to be used for CUD operations but a different object, for example a view, to be used for queries (Support separation of query and update mapping #15671), and still get the table created by migrations or EnsureCreated.
  3. We also have a desire to extend EF Core to allow mapping entities in new ways, many of which are going to require new API surface that is also going to interact with the existing one, resulting in even more combinations and more rules. For example:
    1. Support for mapping to stored procedures for updates and possible query (Map inserts, updates, and deletes (CUD operations) to stored procedures #245)
    2. A more terse API for creating defining queries based on raw SQL (Relational: Add EntityTypeBuilder.ToQuerySql() sugar method #17063)
    3. A way to specify a null mapping, so that the query pipeline throws unless the user provides ad-hoc SQL in the LINQ query (Support for defining query in query types #3932 (comment)).
  4. There is currently a mismatch between the naming patterns used to specify mappings in the model (most methods start with To) and ad-hoc mappings in queries (methods start with From). This obviously affects the existing methods but it would be good to have a coherent story if we want to add more, like a FromTable method you can use directly in queries.

Examples

Right now what needs to happen when you call two methods like ToTable and ToQuery on the same entity type isn't clear.

In general, it is desirable that when and entity is configured with ToTable, it affects what the query pipeline, the update pipeline and the DDL pipeline for that entity. But when ToQuery is also applied, the simplest and most useful behavior seems to be that the latter will only override what happens with the query pipeline.

Here the defining query references People which is the DbSet<Person> just to add an OrderBy call:

modelBuilder.Entity<Person>().HasNoKey();
modelBuilder.Entity<Person>().ToTable("Guests");
modelBuilder.Entity<Person>().ToQuery(() => this.People.OrderBy(p => p.Name));

Ideally this should mean that:

  1. For the query pipeline, when context.People.ToList() is executed, this will generate the following SQL:
SELECT [g]
FROM [Guests] AS [g]
ORDER BY [g].[Name]
  1. For the update pipeline, CUD operations will be generated against Guests.

  2. In the DDL pipeline (Migrations and EnsureCreated) will still create the table Guests. But currently any entity that has a defining query configured is ignored by the DDL pipeline. That is, the table won't be created, and there is no way to override that.

There have been similar request from customers to be able to compose ToTable and ToView, so that if both are used together, ToView only applies to the query pipeline, and ToTable to the update pipeline and DDL.

Proposal

a. We slightly bend the semantics of To as used in APIs like ToTable to specify:

  1. Primarily, the database object to which instances of that entity type will be persisted.
  2. By convention (e.g. unless it is overridden by configuration), the database object from which instances of that type should be retrieved from.
  3. Also by convention, the database object that the DDL pipeline should take care of creating.

b. We start adding From*, e.g. FromSql, FromTable, FromQuery, FromView that are used only to configure the way instances of a type should be retrieved (e.g. the mapping used by the query pipeline), without affecting the other two aspects.

c. We add the ability to configuring a null mapping, either through a new API or through calling ToTable(null) .

d. We add public API to ignore parts of the model in the DDL pipeline (#2725). That is, so that elements can be configured as declared but not defined in the EF Core model. E.g. IsExtern() or whatever name we choose.

e. We decide what to do with the other existing To* methods on a case-by-case basis:

  1. Some of them may become obsolete if they are incoherent and not useful.
  2. Some of them may be kept if they can be expressed using the building blocks listed above. For example, the current version of ToView which doesn't allow a definition of the view to be passed, implies by convention to use the same object for the update pipeline and to ignore the object for the purpose of migrations (e.g. IsExtern(true)).

f. For store procedure mapping, also consider how we can leverage the building blocks above. In the past we have got a lot of feedback on the value of having fine-grained mappings, for instance, be able to configure that UPDATEs should go trough a stored procedures while other operations still go through other conventional or explicitly configured mappings. While at the same time, there is value in being able to in a single call configure all CUD operations to stored procedures with by-convention names. But should this override the mapping for the query pipeline, or should we still go by the default table name unless explicitly configured with FromQuery or FromSql?.

@ajcvickers
Copy link
Member

Note from team: see case in #19708

@alugili
Copy link

alugili commented Feb 6, 2020

Here the defining query references People which is the DbSet just to add an OrderBy call:

modelBuilder.Entity().HasNoKey();
modelBuilder.Entity().ToTable("Guests");
modelBuilder.Entity().ToQuery(() => this.People.OrderBy(p => p.Name));
Ideally this should mean that:

For the query pipeline, when context.People.ToList() is executed, this will generate the following SQL:
SELECT [g].[Name]
FROM [Guests] AS [g]
ORDER BY [g].[Name]

For me, it does not make sense that: context.People.ToList() will return a collection of string?
It makes sense for me this will generate the following SQL:

 SELECT [g]
 FROM [Guests] AS [g]
 ORDER BY [g].[Name]

Did I miss something?

@smitpatel
Copy link
Member

@alugili - It was mistake. Thanks for pointing out. I updated first post.

AndriySvyryd added a commit that referenced this issue Feb 12, 2020
Use the new model in migrations and update pipeline

Part of #12846, #2725, #8258, #15671, #17270
AndriySvyryd added a commit that referenced this issue Feb 12, 2020
Use the new model in migrations and update pipeline

Part of #12846, #2725, #8258, #15671, #17270
AndriySvyryd added a commit that referenced this issue Feb 13, 2020
Use the new model in migrations and update pipeline

Part of #12846, #2725, #8258, #15671, #17270
AndriySvyryd added a commit that referenced this issue Feb 13, 2020
Use the new model in migrations and update pipeline

Part of #12846, #2725, #8258, #15671, #17270
AndriySvyryd added a commit that referenced this issue Feb 13, 2020
Use the new model in migrations and update pipeline

Part of #12846, #2725, #8258, #15671, #17270
@AndriySvyryd
Copy link
Member

AndriySvyryd commented Feb 19, 2020

I don't think there's a need to have separate From* methods. As I've shown for the relational model we can establish a clear priority order that allows to have different mappings for migrations, query and updates simultaneously. The only case it wouldn't allow is to have an entity type mapped to a table for one aspect and not having any mapping at all for other aspects.

Here's a counter-proposal that just builds on existing API to add more mapping functionality:

modelBuilder
  .Entity<Blog>()
  .ToTable("Blogs", t => t.ExcludeFromMigrations())
  .ToView("BlogsView", v =>
        {
            v.HasDefinitionSql("select * from blogs");
            v.IsUpdatable();
        })
  .FromSql("select * from Blogs")
  .ToFunction("GetBlogs", f => f.HasName("get_blogs"))
    .UpdateUsingStoredProcedure(u => u.HasName("modify_cust")
        .HasParameter(b => b.Id, pb => pb.HasName("customer_id"))
        .HasParameter(b => b.Name, pb => pb.IsOutput())
        .HasParameter(b => b.AlternateKey)
        .HasRowsAffectedParameter(pb => pb.HasName("rows_affected")))
    .DeleteUsingStoredProcedure(d => d.HasName("delete_cust")
        .HasParameter(b => b.Id, pb => pb.HasName("customer_id")))
    .InsertUsingStoredProcedure(i => i.HasName("insert_cust")
        .HasParameter(b => b.Id, pb => pb.HasName("customer_id"))
        .HasParameter(b => b.Name)
        .HasParameter(b => b.AlternateKey)
        .HasResultColumn(b => b.Id, rb => rb.HasName("customer_out_id"))
        .HasResultColumn(b => b.Name)
        .HasRowsAffectedParameter(pb => pb.HasName("rows_affected")));

TPH:

    modelBuilder.Entity<Blog>()
        .ToTable("Blogs")
        .HasDiscriminator<string>("blog_type")
        .HasValue<Blog>("blog_base")
        .HasValue<RssBlog>(null)
        .HasNotNull<OtherBlog>();

TPT:

    modelBuilder.Entity<Blog>().ToTable("Blogs");  
    modelBuilder.Entity<RssBlog>().ToTable("RssBlogs");

TPC:

    modelBuilder.Entity<Blog>().ToTable("Blogs")
        .InheritPropertyMapping(false)
        .Property(b => b.BlogId).HasColumnName("ID");  
    modelBuilder.Entity<RssBlog>().ToTable("RssBlogs")
        .ToTable("RssBlogs", bt =>
        {
            bt.Property(b => b.BlogId).HasColumnName("id");
        });

Table splitting:

    modelBuilder.Entity<Blog>(b =>
    {
        b.ToTable("Blogs");
    });

    modelBuilder.Entity<RssBlog>(b =>
    {
        b.ToTable("Blogs");
        b.HasOne(o => o.Blog).WithOneRequired()
            .HasForeignKey<RssBlog>(o => o.BlogId);
    });

Entity splitting:

    modelBuilder.Entity<Blog>(bb =>
    {
        bb.ToTable("Blogs")
        bb.Property(b => b.BlogId).HasColumnName("ID");
        bb.Property(b => b.Name).HasColumnName("NAME");

        bb.SplitToTable("RssBlogs", m =>
        {
            m.Property(b => b.BlogId).HasColumnName("Id");
            m.Property(b => b.Url);
        })
    };

@ajcvickers
Copy link
Member

@AndriySvyryd Similar question here--how much can I do end-to-end in preview 2?
/cc @JeremyLikness

@AndriySvyryd
Copy link
Member

@ajcvickers Same, only model building and migrations are done so far

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-query 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.

7 participants