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

Feature Request: Adding Support for Tuples in DbContext.Database.SqlQuery<T>() #35163

Open
fedpo2 opened this issue Nov 20, 2024 · 23 comments
Open

Comments

@fedpo2
Copy link

fedpo2 commented Nov 20, 2024

Currently, Entity Framework does not support using tuples as the generic type in the DbContext.Database.SqlQuery<T>() method. This limitation makes it challenging to execute SQL queries that return columns that do not directly map to entities or predefined models in the code.

I would like Entity Framework to support tuples as the generic type in DbContext.Database.SqlQuery(). For example:

var results = context.Database.SqlQuery<(int Id, string Name)>("SELECT Id, Name FROM TableName");

This would allow developers to work directly with strongly-typed tuple results, simplifying development and eliminating the need for additional classes to handle temporary or specific query results.

@roji
Copy link
Member

roji commented Nov 20, 2024

One limitation of value tuples, is that the field names (Id, Name) aren't preserved, so it wouldn't be possible to match the tuple fields to the columns coming back from the database. We could match positionally though.

@cincuranet cincuranet changed the title Feature Request: Adding Support for Touples in DbContext.Database.SqlQuery<T>() Feature Request: Adding Support for Tuples in DbContext.Database.SqlQuery<T>() Nov 20, 2024
@cincuranet
Copy link
Contributor

I personally think records are better fit for this type of querying.

@roji
Copy link
Member

roji commented Nov 21, 2024

@cincuranet I agree records are a good match here, though they do require an explicit type definition, which is somewhat annoying for an ad-hoc SQL query...

On the other hand, positional result binding - which is the only thing we can do with tuples - carries a certain degree of confusion that may make it a pit of failure. For example:

var results = context.Database.SqlQuery<(string Bar, string Foo)>("SELECT Foo, Bar FROM TableName");

In the code above, the natural expectation is for Foo on the returned tuple to contain the Foo value from the database, but it will not: since we don't have field names on the tuple, we can only bind positionally, meaning that Bar gets Foo, and Foo gets Bar.

Let's discuss this briefly in design. I'm not sure this would be a good idea.

@cincuranet
Copy link
Contributor

FWIW there's TupleElementNames attribute produced by compiler. But also, tuple elements are usually lowercase. 🤷‍♂️

@roji
Copy link
Member

roji commented Nov 21, 2024

FWIW there's TupleElementNames attribute produced by compiler. But also, tuple elements are usually lowercase. 🤷‍♂️

I don't think that works for the generic invocation usage above - just like we can't know about reference nullability of generic parameters... There's nowhere for the compiler to emit [TupleElementNames]...

@cincuranet
Copy link
Contributor

Right. We're not in a place where it is declared. Pity. More points for records! :D

@roji
Copy link
Member

roji commented Nov 25, 2024

Design decision: we won't be supporting this, because of the risk above with positional results binding.

Note that it's possible to just project out an anonymous type (or even possibly a tuple), which seems like almost what you want.

@roji roji closed this as not planned Won't fix, can't repro, duplicate, stale Nov 25, 2024
@roji roji added closed-no-further-action The issue is closed and no further action is planned. and removed needs-design labels Nov 25, 2024
@ahdung
Copy link

ahdung commented Dec 4, 2024

@roji I think developers should and are willing to take this risk, since they decide write raw sql, why we need SqlQuery because we need flexibility, not safety.

@stevendarby
Copy link
Contributor

stevendarby commented Dec 4, 2024

I was interesting to know if (non-Value) Tuple would work, and got this:

_ = context.Database.SqlQuery<Tuple<string ,string>>($"Select 'A' as Item1, 'B' as Item2").ToList();

System.InvalidOperationException: 'No suitable constructor was found for entity type 'Tuple<string, string>'. The following constructors had parameters that could not be bound to properties of the entity type: 
    Cannot bind 'item1', 'item2' in 'Tuple<string, string>(string item1, string item2)'
Note that only mapped properties can be bound to constructor parameters. Navigations to related entities, including references to owned types, cannot be bound.'

This isn't specific to Tuple, as got the same in a quick Test class:

public class Test
{
    public string Item1 { get; }
    public string Item2 { get; }

    public Test(string item1, string item2)
    {
        Item1 = item1;
        Item2 = item2;
    }
}

I don't know much about constructor binding as usually just work with properties with public setters, but EF does seem to try to allow this, and I'd be interested to know why it doesn't work. I suspect something to do with it being ad-hoc rather than entity types. If it works for entity types but not ad-hoc types, maybe that's something that could be looked at...?

@roji
Copy link
Member

roji commented Dec 5, 2024

I think developers should and are willing to take this risk, since they decide write raw sql, why we need SqlQuery because we need flexibility, not safety.

First, using SQL doesn't mean that the things shouldn't be safe or be a pit of failure. But the important point is that in absolutely all other cases, EF binds result columns by name - this would be the only case where it does so by position, which is why it's risky: it would be very reasonable for users to expect named binding like everywhere else, and then get badly bit by incorrect data.

What's your issue with simply projecting out an anonymous type, rather than a tuple?

@stevendarby
Copy link
Contributor

@roji how would you do that with SqlQuery without going via another type, which defeats the point?

@roji
Copy link
Member

roji commented Dec 5, 2024

You're right, I confused APIs there and forgot the shape of SqlQuery. If it's OK to start from a mapped entity type, then FromSql can do this quite well:

_ = await context.Blogs.FromSql($"SELECT * FROM Blogs").Select(b => new { ... }).ToListAsync();

Otherwise I'm still quite apprehensive about seeing users do this kind of thing:

var results = context.Database.SqlQuery<(string Bar, string Foo)>("SELECT Foo, Bar FROM TableName");

Any thoughts on all this @stevendarby?

(regarding constructor binding for SqlQuery specifically, we might not do that currently for that API - open a separate issue?)

@ahdung
Copy link

ahdung commented Dec 6, 2024

@roji The question is SqlQuery can't be used with anonymous type.

I understood your point, I agree there is inconsistency here. but IMO, developers not babies, they don’t need to be taken care of so carefully, they are obliged to understand some basic knowledge, ValueTuple is position-dependent, which is the one thing of them. In addition, we also have intellisense, lots explanations is not a strange thing in EF Core:

Image

@roji
Copy link
Member

roji commented Dec 6, 2024

I'll reopen to bring this back to the team for discussion, but I'm still quite skeptical here. As you say, developers are not babies, but our job here is to design APIs which are not pits of failure which lead to unexpected (and silent!) data issues, which is what I'd expect a reasonable developer to fall into (I'm pretty sure I would). Documentation can help to a certain extent, but still doesn't remove the need to design good APIs.

@roji roji reopened this Dec 6, 2024
@roji
Copy link
Member

roji commented Dec 6, 2024

BTW am noting that Dapper does support mapping tuples/value tuples, although the exact same issue exists there. That could be an indication for us to do it too.

@ErikEJ
Copy link
Contributor

ErikEJ commented Dec 6, 2024

Don't do this, it will cause data corruption

@roji
Copy link
Member

roji commented Dec 6, 2024

@ErikEJ the way I see it, on of the main value propositions of SqlQuery is to allow one-off ad-hoc queries with arbitrary result shapes, i.e. without having a return type definition. Requiring users to define a type (e.g. record) every time they want to do an ad-hoc SQL query seems like quite the annoyance, and exactly the kind of thing this API should solve; and besides, if you're OK with defining a type, you may as well just include it in the model and use FromSql, and don't really need SqlQuery.

That, coupled with the fact that Dapper has supported just this thing for quite a long time - and without tons of complaints as far as I can see - makes me think we should consider it...

@stevendarby
Copy link
Contributor

stevendarby commented Dec 6, 2024

(regarding constructor binding for SqlQuery specifically, we might not do that currently for that API - open a separate issue?)

Ignore that, same issue with true entity types too when the property types don't have setters.

Regarding positional mapping, could you perhaps instead insist on Item1, Item2, etc. naming in the SQL? These are 'stable' properties on ValueType

@roji
Copy link
Member

roji commented Dec 6, 2024

@stevendarby interesting idea, it might be a good compromise... I'll point out that people regularly have trouble with naming their projection Value when composing on top of SqlQuery (docs), it's apparently hard to understand that EF requires certain naming for results coming out of raw SQL queries. But it may be better for people to have to work through that and silently getting bad data...

@ahdung
Copy link

ahdung commented Dec 7, 2024

But it may be better for people to have to work through that and silently getting bad data...

I did meet this value issue first time I used SqlQuery, and I checked the generated sql instead of docs, fine, just append as value, no complaints, happy to do this, but I really mind defining a class/record for just once used.

@stevendarby
Copy link
Contributor

stevendarby commented Dec 7, 2024

Just to zoom out a little on why exactly ValueTuple isn't supported currently.

Firstly it throws this:

The specified type 'System.ValueTuple`2[System.String,System.String]' must be a non-interface reference type to be used as an entity type.

So firstly there's a problem using any value types, not just ValueTuple. Do we know why this is? Without addressing this, supporting ValueType is probably going to involve special-casing that type, which doesn't seem like a great solution. (1)

As an experiment, I took a copy of ValueTuple<T1, T2>, renamed it and made it a class instead of struct. (I had to make a few small tweaks - e.g. remove an internal interface and its implementations, but I don't think that significantly changed things). Using this in a query then exposed a second problem, it throws:

No suitable constructor was found for entity type 'ValueTupleX<string, string>'. The following constructors had parameters that could not be bound to properties of the entity type:
Cannot bind 'item1', 'item2' in 'ValueTupleX<string, string>(string item1, string item2)'
Note that only mapped properties can be bound to constructor parameters. Navigations to related entities, including references to owned types, cannot be bound.

I could workaround this by changing the Item1 and Item2 fields into properties. Then the constructor binding works and I'm able to query with it, using Item1 and Item2 as the SQL columns names.

So it'd be interesting to understand why it doesn't work with fields - maybe it's not a big ask to get that working in general?

To summarise, if it's at all possible to 'fix'...

  • Not being able to use value types
  • Not being able to use constructor binding when fields are involved

... then ValueTuple might 'just work'.

edit: (1) On second thought, types like int must already be special-cased, otherwise they'd get that first exception too I think? So ValueType would just be another :)

@roji
Copy link
Member

roji commented Dec 7, 2024

Thanks for diving into this - let us discuss this in the team and see what's what.

@roji
Copy link
Member

roji commented Dec 9, 2024

Design decisions:

  • We'll introduce positional binding as proposed above, when the return type implements ITuple (so Tuple and ValueTuple). Because of time constraints we may start by enabling Tuple first and leave ValueTuple for later.
  • Since tuples have up to 7 items, and beyond that start nesting other tuples, we'll likely support only up to 7 values. We also believe that beyond this, it's a better idea to use an explicit type with clear property names anyway, to avoid confusion.

Additional context: we considered @stevendarby's proposal of allowing binding via Item1 and Item2. While this is indeed a bit safer than positional binding (less chance of binding incorrectly), the requirement is very undiscoverable (no new user would guess this and would fail, needing to go to the documentation etc.), the required SQL is quite ugly, plus the Dapper example seems to show that the risk of mix-up isn't that problematic (and/or that users discover the error relatively quickly via testing).

var results = context.SqlQuery<(int, string)>("SELECT Id AS Item1, Name AS Item2 FROM Foo");

@roji roji removed the needs-design label Dec 9, 2024
@roji roji added this to the Backlog milestone Dec 9, 2024
@roji roji added consider-for-next-release and removed closed-no-further-action The issue is closed and no further action is planned. labels Dec 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants