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

Consolidate EntityType and QueryType #14194

Closed
ajcvickers opened this issue Dec 17, 2018 · 19 comments
Closed

Consolidate EntityType and QueryType #14194

ajcvickers opened this issue Dec 17, 2018 · 19 comments
Labels
breaking-change closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-enhancement
Milestone

Comments

@ajcvickers
Copy link
Member

We introduced "query types" in 2.1 as types that don't have a primary key defined. However, more and more it is looking that these should just be entity types without primary keys defined. This doesn't mean that they will behave differently, just that they will be represented by and called entity types. This has the following advantages:

  • Removes the decision point between query types and entity types. We have seen a number of issues where people inappropriately choose one over the other.
  • Removes all the extra code added for model-building APIs, etc.
  • Reduces the chance that we will add code/support to one place and not the other when it really should be in both

One thing to be careful of is that we don't treat something that should have a PK as something that doesn't have a PK by convention, since this will result in limited functionality (i.e. query type functionality) on something that is expected to have entity type functionality, which could be hard to debug. We might want to consider requiring something explicit like HasNoKey or an appropriate annotation to make one of these types.

@ajcvickers
Copy link
Member Author

Note about query types. After further discussion, all navigations to query types will be disallowed. This is because:

  • There is a point of view which says references to query types is not needed. That is, in a well-defined model query types should only ever have references to entity types. Query types should never reference other query types, and should never reference entity types, even if the query type has an FK property and there is an entity type with a PK that matches that FK.
  • This means that even though query types cannot be tracked, they could be used to pull in entity types that are tracked without ambiguity. (Note that this is not implemented yet.)

@ajcvickers
Copy link
Member Author

Note from planning: we will obsolete the current APIs since a hard break in 3.0 will be too painful.

AndriySvyryd added a commit that referenced this issue Feb 22, 2019
Mark QUery methods and types as obsolete
Throw when ToTable is called on derived types

Fixes #14194
Fixes #11811
@AndriySvyryd AndriySvyryd added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Feb 22, 2019
@AndriySvyryd AndriySvyryd reopened this Feb 22, 2019
@AndriySvyryd AndriySvyryd modified the milestones: 3.0.0, 3.0.0-preview3 Feb 26, 2019
@NetTecture
Copy link

@ajcvickers There is also a point of view that the world is flat. Makes no logical sense.

Here are scenarios of query types that are reachable from entity types. Views.

  • I have an entity X. I have a query type with a similar PK (except I can not set a PK on a view, so code generation, you know, does not see it) that analyzes the object and returns particular data (markers mostly, true false). Query type in navigation property. Mapping the properties into the base entity is bad (separation of concerns, they really belong to a subsystem).

  • I have an entity. There is a view as above "pk can not be entered), pk is entity pk + user pk) with rights (can edit, can delete). I really WANT to be able to use that relationship + filter by user) to fast-get the user rights on an entity.

I have a lot of view based query types, mostly because the DB is not made by "code first, ignore all SQL level features" style. The ability to reference query types (i.e. views) from an entity type is critical in this scenario. And no, you can not express advanced view and sql thingies in LINQ. No way.

@ajcvickers
Copy link
Member Author

@NetTecture If the views have unique column(s) that can act as a key, then map them to entity types.

@AndriySvyryd AndriySvyryd removed their assignment Feb 28, 2019
@Suchiman
Copy link
Contributor

Suchiman commented Mar 7, 2019

@ajcvickers

That is, in a well-defined model query types should only ever have references to entity types.

Query types […] should never reference entity types, even if the query type has an FK property and there is an entity type with a PK that matches that FK.

Only one of these statements can be true, and i hope it's the first because i don't see why Query types should be constrained the second way. Makes them less useful and you now have to write manual joins...

roji added a commit to roji/efcore.pg that referenced this issue Mar 7, 2019
roji added a commit to roji/efcore.pg that referenced this issue Mar 7, 2019
@AndriySvyryd
Copy link
Member

@Suchiman

That is, in a well-defined model query types should only ever have references to entity types.

Query types […] should never reference entity types, even if the query type has an FK property and there is an entity type with a PK that matches that FK.

Only one of these statements can be true, and i hope it's the first because i don't see why Query types should be constrained the second way. Makes them less useful and you now have to write manual joins...

I think @ajcvickers meant to say "Entity types should never reference query types, even if the query type has an FK property and there is an entity type with a PK that matches that FK."

@NetTecture
Copy link

And the reason is why?

I have a ton of entities that have query types - totally read only types - that are coming from a view attached.

  • Rights (pk of entity, pk of user, can read ,can delete etc.)

  • KPI analysis (entity pk, is overdue, is over budget etc.).

Not entities by definition, referenced from an entity.

@ajcvickers
Copy link
Member Author

@NetTecture Why can't those be mapped as entity types?

roji added a commit to roji/efcore.pg that referenced this issue Mar 8, 2019
@lightel
Copy link

lightel commented Apr 5, 2019

@NetTecture Why can't those be mapped as entity types?

Maybe I'm missing something so let me explain my case a bit more in details so that you could understand my confusion. We are building the API using an existing legacy database. So basically we use the Database First Approach. In a legacy database we have many views which returns ids of entities (some of them contain a column which can serve as PK). We used QueryType with EF Core 2.1 and mapped such entities to corresponding views. We also added navigation properties so that we could make joins to related entities. With EF Core 2.2 this doesn't work anymore. My understanding is that you are suggesting to pretend that these views are tables and map them to entities (with current implementation an entity can be mapped to table only). With the Code First Approach, I guess, this would work just fine because a migration would create a table instead of a view in a database. But with the Database First Approach once I mapped a view to an entity nothing prevents me (or any other person who sees my code for the first time) from assuming that it's a regular entity which is mapped to a regular table therefore it is pretty safe to insert a new record what will obviously cause a crash.
Can you advise on this? I will appreciate any input.

@ajcvickers
Copy link
Member Author

@lightel Just because the mapping is to a view doesn't necessarily mean that the returned objects don't have keys or are read-only. Likewise, just because the mapping is to a table doesn't necessarily mean that the returned objects must have keys and be updatable. Query types have one feature that defines them--they don't have keys defined. This doesn't mean they have to be used with views. Likewise entity types with keys defined don't have to be used with tables.

Scaffolding to views is covered here: #1679
Excluding types from migrations is covered here: #2725
Read-only entity types are covered here: #7586

@JonPSmith
Copy link

Can I add some thoughts here.

I agree that having a DbSet<T> and a DbQuery<T> had some down sides, especially in libraries where you needed to check if an entity was a DbSet<T> or a DbQuery<T>.

However I think the concept of a "Read-Only" entity class is a good one. I have build systems where I use multiple DBContext to create DDD type "bounded contexts" in one database. I use a DbQuery to enforce that one bounded context has access to, but cannot write to, a particular table. I can also see a technical reason for having a read-only entity class when a view is not updatable.

I looked at the Read-only entity types issue #7586, but its very old and doesn't seem to go anywhere. Is there something like add a fluent API method like .IsReadOnly a possibility?

@roji
Copy link
Member

roji commented May 3, 2019

@JonPSmith agreed that a read-only entity type can definitely be useful, but note that as explained in #7586 it's relatively easy to achieve that without any special support from EF Core (or at least some version of that).

@JonPSmith
Copy link

Hi @roji, I saw that. It works but its not automatic. Also overriding SaveChanges and using ChangeTracker require some finesse to do properly and being efficient, i.e. not calling ChangeTracker twice. But I accept that might be the way I need to go if I want to enforce read-only.

@ajcvickers
Copy link
Member Author

@JonPSmith Read-only types is something we intend to implement. The point is that query types were not ever intended to be used because they are read-only. That's just a side-effect of not having a key. This doesn't change when making them entity types without keys; they will still be read-only out of necessity.

@kpejr
Copy link

kpejr commented May 23, 2019

Is there a trick to getting this to work? Latest preview I get the key required message.

@AndriySvyryd
Copy link
Member

@kpejr You need to call ModelBuilder.Entity<MyEntity>().HasNoKey()
See breaking changes

@kpejr
Copy link

kpejr commented May 23, 2019

@AndriySvyryd Thanks, missed that working great now.

@lbogdanov
Copy link

@ajcvickers one of the query type's benefits - you can pre-define queries using raw sql in your code, no need to create a views in the database. Will this feature be retained in some form and shape in v3.0?
modelBuilder.Query<MyQueryType>().ToQuery( () => Query<MyQueryType>().FromSql("select * from mytable"));
Thank you

@AndriySvyryd
Copy link
Member

@ibogdanov Yes, all functionality remains the same. You just need to call 'modelBuilder.Entity().HasNoKey()' instead of 'modelBuilder.Query()'

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. type-enhancement
Projects
None yet
Development

No branches or pull requests

9 participants