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

Bring back DbQuery<T> #18719

Closed
Neme12 opened this issue Nov 2, 2019 · 15 comments
Closed

Bring back DbQuery<T> #18719

Neme12 opened this issue Nov 2, 2019 · 15 comments
Labels
closed-no-further-action The issue is closed and no further action is planned. customer-reported

Comments

@Neme12
Copy link

Neme12 commented Nov 2, 2019

In EF Core 2.2, we used a query type similar to this (notice the Id property):

public sealed class Record
{
    public int Id { get; set; }
    public string Value { get; set; }
}

that wasn't mapped to anything - not a table, not a view. We only used it with FromSql(). In our DbContext we had:

public DbQuery<Record> Records { get; set; }

After upgrading to EF Core 3.0, I see that DbQuery is marked as obsolete, suggesting the use of DbSet instead. I was a little confused, because - how will EF Core then be able to tell that this isn't actually supposed to be an entity... only a query type? And of course after running this, EF Core suddenly treated it as an entity and wanted to create a table for Records.

I saw a suggestion to mitigate this by configuring the type using HasNoKey(). That didn't work. EF Core still wanted to create a table for it. So I tried a few other things - calling ToTable(null) since we don't want a table, but that threw an exception. Eventually I tried using ToView (even though we don't want to map this to any view) and calling ToView(null) finally did the trick.

My question is: why the deprecation of DbQuery<T>?

The docs says:

This change was made to reduce the confusion around the purpose of query types.

In my opinion, the previous behavior was really clear and unambiguous - in our DbContext, we had DbSet<T> and DbQuery<T> properties. You could immediately see what was an actual entity mapped to a table in the database (DbSet) and what was not (DbQuery).

Now, I can't tell by looking at the DbContext itself. Instead of a simple change in the type of a property, I have to add additional configuration for the type to prevent it from being treated as an entity and mapped to a table, in a way that's not intuitive at all. I find the current state much more confusing than before.

@ajcvickers
Copy link
Member

@Neme12 We appreciate the feedback, but I think we are going in the correct direction here.

A query type was simply an entity type without a key defined. It could be mapped to a table, or to a view, or not to anything at all. All three of these possible mappings are also valid for entity types with keys. So Migrations working differently just because a type doesn't have a key led to a lot of confusion about what query types really were. See #17270

The second part of this, which we unfortunately not able to get to for 3.0, is a general mechanism to avoid creating database artifacts for any part of the model.

in our DbContext, we had DbSet and DbQuery properties. You could immediately see what was an actual entity mapped to a table in the database (DbSet) and what was not (DbQuery)

What is the reason you need to DbSet properties for these types on your DbContext?

@loth0
Copy link

loth0 commented Nov 11, 2019

Want to add a scenario which worked before but broke when upgrading to EF Core 3.0:

We created a partial dbContext which is completely separated from the generated context (we use database first and scaffolding). We collect all DbQuery's for the raw sql queriers in the manually created context. DbSet forces us to add HasNoKey to them but we can't override OnModelCreating since it's allready overriden in the generated context. Is there any soulotion to this?

@ajcvickers
Copy link
Member

@loth0 Why doesn't the generated context use HasNoKey?

@ajcvickers ajcvickers added closed-no-further-action The issue is closed and no further action is planned. and removed type-enhancement labels Nov 11, 2019
@ErikEJ
Copy link
Contributor

ErikEJ commented Nov 11, 2019

@loth0 just implement the OnModelCreating partial method, and make any changes there.

@loth0
Copy link

loth0 commented Nov 12, 2019

@ajcvickers That would mean I'd have to modify the generated context each time I scaffold, which kinda defeats the purpose of a manually handled partial context, separated from the autogenerated context. I allready have to modify it (because of the default OnConfiguring generation) but the more modifications I have to do after each scaffolding the less convinient it gets.

@ErikEJ I don't follow, the OnModelCreating method is allready implemented in the generated context, I can't implement it in the manually created context too.

@loth0
Copy link

loth0 commented Nov 12, 2019

@ErikEJ Sorry my bad, now I found that a OnModelCreatingPartial method is generated, that one does the trick, thanks a lot!

@Neme12
Copy link
Author

Neme12 commented Nov 17, 2019

@ajcvickers

What is the reason you need to DbSet properties for these types on your DbContext?

We don't. I think the author wrote it that way to avoid having to use fluent configuration (which we now have to use anyway to prevent EF Core from trying to create a table for it but at the same time be able to use it in FromSql). So yes, now it's pointless, I'll remove them, thanks.

@Neme12
Copy link
Author

Neme12 commented Nov 17, 2019

I still think it would be useful to have a separate data type for a set of keyless entities as opposed to DbSet because the API should be different. For example Find doesn't make sense on keyless entity types. DbSet could then derive from this type and add Find and maybe other methods.

@Neme12
Copy link
Author

Neme12 commented Nov 17, 2019

@ajcvickers What about creating a new attribute then that would mark an entity as not being mapped to a table or anything in the database, similar to how there's an OwnedAttribute right now?

@ajcvickers
Copy link
Member

@Neme12 Thanks for the feedback.

I still think it would be useful to have a separate data type for a set of keyless entities as opposed to DbSet because the API should be different. For example Find doesn't make sense on keyless entity types. DbSet could then derive from this type and add Find and maybe other methods.

That's basically why we created the new type originally. However, it quickly became very apparent that understanding the difference between QueryType and EntityType (one has a key defined; the other does not) was very difficult for people. It also added code duplication which already meant we missed some API on QueryType that should have worked for both. On balance, it seemed better to use EntityType for both since the difference between them is only whether there is a key or not. The difference in behavior is still a good point, and I guess we'll see with time whether we made the right call.

I added a note to #2725 about the attribute.

@julielerman
Copy link

Hi there, I wanted to add a vote related to a point made in @Neme12 latest comment, which is to have some way for the compiler to understand that on a DbSet for a keyless entity, Find won't work. Currently that throws at run time. Is it technically possible? I didn't see an issue for it and am happy to add one explicitly if it's useful to track. Thanks

@ajcvickers
Copy link
Member

@julielerman Can you explain what you mean by, "the compiler to understand?"

@julielerman
Copy link

Oh sorry for lack of clarity.
If you have a DbSet that's been configured with HasNoKey, and try

_contextInstance.ThatKeylessSet.Find(1);

which doesn't make sense but let's say it's a mistake from habit...it will compile and run but throw an exception at runtime when EF Core tries to work out the query. Not in front of my computer so can't give an example. Hopefully that's enough.

@ajcvickers ajcvickers reopened this Dec 13, 2019
@ajcvickers
Copy link
Member

@julielerman We discussed this and we're wondering if just using IQueryable would work for this?

@cda-1
Copy link

cda-1 commented Jan 7, 2020

@ajcvickers

What is the reason you need to DbSet properties for these types on your DbContext?

We don't. I think the author wrote it that way to avoid having to use fluent configuration (which we now have to use anyway to prevent EF Core from trying to create a table for it but at the same time be able to use it in FromSql). So yes, now it's pointless, I'll remove them, thanks.

I'm following this thread and the exchange between @Neme12 and @ajcvickers, but I don't understand this part. Is the DbSet property still (in .net core 3.1) required if we are using .FromSql()?

I am still using FromSql, so I'll still need the DbSet item, and .HasNoKey() and .ToView(null), right?

I created a Stack Overflow Question about this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-no-further-action The issue is closed and no further action is planned. customer-reported
Projects
None yet
Development

No branches or pull requests

6 participants