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

Support raw SQL Queries for basic types like Guid, DateTime and int #11624

Closed
Tracked by #24106
Eirenarch opened this issue Apr 11, 2018 · 40 comments · Fixed by #28665
Closed
Tracked by #24106

Support raw SQL Queries for basic types like Guid, DateTime and int #11624

Eirenarch opened this issue Apr 11, 2018 · 40 comments · Fixed by #28665
Assignees
Labels
area-query closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. ef6-parity punted-for-6.0 type-enhancement
Milestone

Comments

@Eirenarch
Copy link

I read all the issues related to the newly introduced Query Types but I still can't find a good way to do a query for a simple value type. In EF6 I did

Database.SqlQuery<Guid>(sql, parameters).SingleOrDefaultAsync()

and I got back a Guid? from my single column, single row SQL query that returned a Guid or no rows at all. I tried porting that functionality to the new Query Types in EF Core 2.1 preview 1 and it looks like a mess. Even if #10753 is implemented and registering the type in advance is not required I will still have to create a wrapper class for the Guid because value types are not allowed in the Query method. Also if I register the type all my SQL should follow the same convention and I can't just SELECT ProductID or SELECT CommentID because the name of the column must match the name of the property in the type.

Am I missing some other way to query for these types? If not please consider adding a way to execute such queries.

@ajcvickers
Copy link
Member

Triage: adding this to the backlog to consider how to implement this scenario.

@ajcvickers ajcvickers added this to the Backlog milestone Apr 11, 2018
@Eirenarch
Copy link
Author

One possible way is to make the Query method accept value types as well as reference types and inspect its generic argument for known types (Guid, DateTime, String, int, etc.)

@Vasim-DigitalNexus
Copy link

I too would appreciate the EF 6 Database.SqlQuery for my scenario, I feel this simple query/mapping to types is missing in EF Core

@roji
Copy link
Member

roji commented Jul 22, 2018

At least for your example above, why not just drop down to ADO.NET and execute the query directly? After all you're providing your own SQL, so what added value is there for doing this via EF? If you read back a query type, then you're using EF's O/RM capabilities to at least map the results back to a class, but in the example above there seems to be nothing EF is doing for you...

@Eirenarch
Copy link
Author

@roji this is what I did but it is ugly. I had to create my own connection which I don't want to and bolt the method on top of the EF context so that I don't have to inject another object in my services.

@roji
Copy link
Member

roji commented Jul 23, 2018

You can easily use an existing EF Core DbContext to get an ADO.NET connection, no need to inject an additional object via DbContext.Database.GetDbConnection(), which you can then use to execute any command, without bolting anything on top etc.

@Eirenarch
Copy link
Author

Yeah, I remember trying that but there was some problem. It was a couple of months ago so I don't remember what it was but there was probably something tricky so I downgraded to creating my own connection. I don't doubt that it is possible but I didn't feel like investigating. In any case since many projects will have this requirement why force users to add their own method to the context instead of providing it. After all EF6 does in fact provide this method because it was determined useful.

@roji
Copy link
Member

roji commented Jul 24, 2018

No method needs to be added anywhere - you're simply provided with the standard .NET API for executing SQL on a database...

This is only my personal opinion, but EF Core should do what it's there to do - be an O/RM - and not provide wrappers over ADO.NET (which add almost no value at all). That's simply not pay off it's job description.

It's true ADO.NET could be improved to be a better API, but that's an unrelated issue.

@Vasim-DigitalNexus
Copy link

Vasim-DigitalNexus commented Jul 24, 2018

However, EF 6 Database.SqlQuery<T> provided this function that worked with any types <guid> <int>, <List<T>>, it was very useful for arbitrary types not rooted in DbSet/DbQuery, thus, why not bring this functionality back?

I used this function for read-only views/stored procedures that did not need tracking

I am aware of EF Core Query<T>.FromSql() and used it. However, this still requires registering the type in the DdContext in my case 200+ of these types will have to be added for no useful reason since I only need the mapping of columns and LINQ functionality for the read-only cases; therefore EF 6 Database.SqlQuery<T> would be useful in EF Core (my opinion of course)

@roji
Copy link
Member

roji commented Jul 25, 2018

There seems to be a disconnect in the conversation...

  1. FromSql() is suitable for using raw SQL but allowing EF Core to map the results back to objects (query types or others). It is indeed not suitable for reading primitives, and it is not what I suggested above. The suggestion isn't to use FromSql().
  2. EF Core is an O/RM built on top of .NET's database access layer, which is called ADO.NET. If you want to send arbitrary SQL to the database and get primitive results back, ADO.NET is there for that. EF Core already allows you easy access to ADO.NET via DbContext.Database.GetDbConnection(), as I wrote above.
  3. EF Core can't provide any added value for this task - reading primitive type results of an arbitrary SQL query - since that's exactly what ADO.NET exists. For people who think ADO.NET's API is not ideal, I do agree but think the problem should be fixed there (i.e. modernizing ADO.NET) and not tacking on a nicer API at the unrelated EF Core layer. For one thing, modernizing ADO.NET would allow everyone to benefit from a better API, not just EF Core users.

The fact that something existed in EF6 doesn't mean it needs to exist in EF Core - many things have changed and some adaptations are necessary. To make sure everyone understands, here's a code sample for using ADO.NET starting from an EF Core DbContext:

using (var ctx = new MyContext())
{
    using (var conn = ctx.Database.GetDbConnection())
    using (var cmd = conn.CreateCommand())
    {
        cmd.CommandText = "SELECT * FROM some_table";
        using (var reader = cmd.ExecuteReader())
        {
            while (reader.Read())
                Console.WriteLine(reader.GetString(0));
        }
    }
}

Dapper can also be used to make the above code much simpler.

@divega
Copy link
Contributor

divega commented Jul 25, 2018

I am actually not against adding this capability in EF Core for convenience at some point. It is only lower priority than a large number of other issues we are tracking because, as @roji explained, there are multiple workarounds and this is not central to the goals of EF Core. On the other hand, have you tried this?

public class GuidRow
{
    public Guid Guid { get; set }
}
...
modelBuilder.Query<GuidRow>();
...
var manyGuids = context.Query<GuidRow>().FromSql(sql, params).Select(r => r.Guid);
var singleGuid = manyGuids.SingleOrDefault));

I am typing this on my phone so I didn’t check for errors and I could be missing something, but I’d expect this to work. Keep in mind that FromSql being limited to entity types and query types only affects how you bootstrap the query. You can still later project something else.

Another variation would be to call AsEnumerable() before the select to avoid query composition.

For sure it is more ceremony than what would be desirable.

@Vasim-DigitalNexus
Copy link

Vasim-DigitalNexus commented Jul 25, 2018

@roji no disconnect I understood what you said, I just wanted to point out explicitly that Database.SqlQuery<T> would be useful for both cases (primitives, or for arbitrary types not rooted in DbSet/DbQuery)

RE: "the fact that something existed in EF6 doesn't mean it needs to exist in EF Core", the same is true the other way, maybe it should exist

@Eirenarch
Copy link
Author

Eirenarch commented Jul 25, 2018

@roji how is mapping the result of an SQL query to a Guid not mapping? Also if you point out the term ORM as a motivation for not including this API then EF should not generate SQL at all. After all it should only do mapping like Dapper

@divega I opted out of this solution and went for sticking an ADO.NET based method in the context itself. This seems cleaner approach from API perspective and also much more similar to the EF6 code I was porting which reminds me that this is another reason to allow this - make porting code from EF6 easier. To be fair if I knew how painful this (porting as a whole not this specific method) is I wouldn't be porting to .NET Core at all, I'd stay with EF6 and ASP.NET Core on top of the .NET Framework. While I did complete the migration I regard the decision to port to .NET Core as a mistake because it took too much time and resulted in some ugly workarounds in the codebase and EF Core was my sole pain point. Adding this API is something that will reduce the pain for people migrating to .NET Core in the future.

@roji
Copy link
Member

roji commented Jul 25, 2018

@roji how is mapping the result of an SQL query to a Guid not mapping? Also if you point out the term ORM as a motivation for not including this API then EF should not generate SQL at all. After all it should only do mapping like Dapper

AFAIK the term ORM usually (always?) applies to components which generate SQL for you, taking care of saving/loading/querying objects in your languages to relational database tables. In fact, unless I'm mistaken Dapper calls itself a "micro-ORM" precisely because it doesn't generate SQL but executes your own SQL, mapping results to your objects (a bit like how EF Core's FromSql() works). Loading GUID results from an SQL query which you provide isn't usually referred to as "mapping", at least in the way I'm familiar with the term. But this terminology discussion isn't very important.

At the end of the day, I'm just in general in favor of each component concentrating on its mission, and doing that really well. We have ADO.NET (low-level, SQL-only, no mapping), Dapper (higher-level, SQL-only, with mapping) and EF Core (higher-level still, SQL not required, with mapping). It doesn't mean there can't be any overlap etc., but IMHO each component should stick to its task. Once again, with Dapper you can write something like the following (untested):

using (var ctx = new MyContext())
{
    using (var conn = ctx.Database.GetDbConnection())
        return conn.Query<Guid>("SELECT something FROM somewhere");
}

I'm really not sure why anyone would want anything more :)

@Eirenarch
Copy link
Author

As you pointed out Dapper does call itself an ORM (even if micro) because it does mapping. This is however irrelevant as EF aims to be quite complete data access layer. As pointed out in various places it actually implements the repository (dbsets) and unit of work (context) patterns. It also includes methods like FromSql and many other things which are needed for data access. In my opinion the goal should be to make EF the most practical tool not some exercise in software engineering. EF shouldn't require that I pull Dapper as a dependency in my project in addition to EF just to make a couple of queries or roll a screen of ADO.NET Code. It results in inferior experience for people using EF. What is more EF already provides Query types (i.e. something that covers 90% of what Dapper does) it just doesn't support this particular case.

@roji
Copy link
Member

roji commented Jul 27, 2018

First, Dapper calls itself a micro-ORM precisely because you still have to write SQL - so one could say it doesn't pretend to be a full ORM.

Second, FromSql() in its current form does indeed contain ORM functionality because it maps results back to your entity types (i.e. objects, not primitive types). Crucially, it also allows you to compose on top of your results, i.e. specify LINQ on top of raw SQL. That's a very different thing from what ADO.NET or Dapper do.

I think this discussion has probably run its course... It's of course up to the EF Core team to decide whether they want to be a one-stop shop for any sort of imaginable data access pattern in .NET. I personally believe it's a bit of a dangerous path, even though I agree that the primitive access proposed here isn't that bad and could make sense. But it would be good to think what should happen the next time someone comes up with a request to reimplement or wrap some ADO.NET / Dapper functionality - should EF Core incorporate everything that Dapper provides, because it would provide a nicer experience? Does that really make sense?

Finally, ADO.NET needs to improve - I think everyone agrees with that. That doesn't mean that friendly wrappers for raw SQL access should be introduced in EF Core.

@Vasim-DigitalNexus
Copy link

@roji I understand where you are coming from. However, advertising Dapper as a solution does not make sense to me, it is reasonable and I see no harm, danger, or contradiction in bringing back the Database.SqlQuery<T> functionality which also happens to handle primitive types. The RawQuery would not conflict with FromSql since it would be a separate function.

@roji
Copy link
Member

roji commented Jul 28, 2018

The RawQuery would not conflict with FromSql since it would be a separate function.

It's certainly possible to add something like this alongside FromSql(), but care must be taken not to create API confusion:

  • We'd have the existing DbSet.FromSql() which returns IQueryable<T> and works only on entities/query types (no primitive support).
  • We'd have a new Database.SqlQuery<T> (or similar) which supports primitives, but does it also support objects?
  • If so, would this include full object mapping support for entities (e.g. would entities loaded with this API be tracked)? This seems like a bad direction - it would mean that the method does very different things if invoked on arbitrary types vs. types that happen to be entities (always a bad idea). It would also be a bad idea to put such a method on Database, which currently contains lower-level database functionality (so where would it live?).
  • If objects are supported but not "full mapping", then we now have two different O/RM mapping schemes in EF Core (e.g. one tracks and the other doesn't). That doesn't seem like a good way forward to me.

So it's simple enough to tack on a primitive-only Database.SqlQuery<T> - this would be a trivial wrapper over ADO.NET with no (object) mapping capabilities, and would satisfy the original request (I still don't believe this really belongs in EF Core). Anything with object support becomes more complex and probably requires careful consideration...

@Vasim-DigitalNexus
Copy link

If so, would this include full object mapping support for entities

Yes - with no tracking (e.g., Stored Procedure, read-only views)

(e.g. would entities loaded with this API be tracked)?

No, it should not be tracked

DbSet/DbQuery already support AsNoTracking but are rooted in DbSet that means if you have 500 read-only types you would have to register them with DbSet/DbQuery for no reason (in the case of read-only with no tracking e.g., dc.Query<T>().AsNoTracking().FromSql("storedProcedure",parameters).ToListAsync())

@roji I do understand and appreciate your concerns; our views are shaped by our development experiences and the complexities of projects that we work on. However, sometimes it is hard to see the other perspective because the experiences we had were entirely different.

I can tell you while I am migrating from EF to EF Core, I keep running into runtime errors because I forgot to register a type for a "complicated multi-join read-only stored procedure".

Alas, I do not agree that this functionality does not belong in EF Core and I want to make sure that my voice is heard, ultimately it is up to the EF Core team.

@roji
Copy link
Member

roji commented Jul 28, 2018

DbSet/DbQuery already support AsNoTracking but are rooted in DbSet that means if you have 500 read-only types you would have to register them with DbSet/DbQuery for no reason (in the case of read-only with no tracking e.g., dc.Query().AsNoTracking().FromSql("storedProcedure",parameters).ToListAsync())

That's a very valid concern. It's also possible it could be addressed without necessarily introducing a new, parallel raw SQL query API. Perhaps the requirement to register query types could be relaxed instead, for example. One small note: IIRC for query types you don't have to specify AsNoTracking() as they aren't tracked in any case (but someone from the team should confirm this).

Alas, I do not agree that this functionality does not belong in EF Core and I want to make sure that my voice is heard, ultimately it is up to the EF Core team.

Of course, and I think this is a good and useful conversation regardless of what ends up being decided.

@Paul-Dempsey
Copy link

Meanwhile, falling back to ADO with an example as in this thread should be covered in the topic on Raw Sql Queries for EF core, as well as the limitations and prerequisites (registration) better documented in the docs for Query

@vovikdrg
Copy link

"However, sometimes it is hard to see the other perspective because the experiences we had were entirely different."
Same problem here. I have some queries where all i want to run as a select and map to object. I understand that core team has vision, but sometime you need to go and see how people are using your product.

@weitzhandler
Copy link
Contributor

weitzhandler commented Dec 24, 2018

Anyway it can be achieved using DbComand:

public static class DbContextCommandExtensions
{
  public static async Task<int> ExecuteNonQueryAsync(this DbContext context, string rawSql,
    params object[] parameters)
  {
    var conn = context.Database.GetDbConnection();
    using (var command = conn.CreateCommand())
    {
      command.CommandText = rawSql;
      if (parameters != null)
        foreach (var p in parameters)
          command.Parameters.Add(p);
      await conn.OpenAsync();
      return await command.ExecuteNonQueryAsync();
    }
  }

  public static async Task<T> ExecuteScalarAsync<T>(this DbContext context, string rawSql,
    params object[] parameters)
  {
    var conn = context.Database.GetDbConnection();
    using (var command = conn.CreateCommand())
    {
      command.CommandText = rawSql;
      if (parameters != null)
        foreach (var p in parameters)
          command.Parameters.Add(p);
      await conn.OpenAsync();
      return (T)await command.ExecuteScalarAsync();
    }
  }
}

@yzorg
Copy link

yzorg commented Jun 22, 2022

Can I request this now support the new DateOnly and TimeOnly single-value types in .net6+?

@roji
Copy link
Member

roji commented Jun 22, 2022

@yzorg support for specific types (e.g. DateOnly) will generally depend on support in the underlying ADO.NET provider. If you're using SQL Server, that's SqlClient, which doesn't yet support these two types (see dotnet/SqlClient#1009).

@smitpatel
Copy link
Member

smitpatel commented Jul 22, 2022

Design decision - We will make the API composable. So SqlQuery will return IQueryable<T> which can be composed or combined with other EF queries.
type of T for this issue must be mappable by type mapping source. Users can use ModelConfigurationBuilder API to map additional type with value converters which are not supported by provider by default. We would use those mapped type (so is types coming from plugins like NTS).

If the SQL is being composed afterwards the projection has to use a fixed name for the column. The current default name is Value. If it is not composed then the column name doesn't matter. We will see in future if we need to provide an API to change this default name.

@jjxtra

This comment was marked as off-topic.

@roji

This comment was marked as off-topic.

@jjxtra

This comment was marked as off-topic.

smitpatel added a commit that referenced this issue Aug 11, 2022
smitpatel added a commit that referenced this issue Aug 11, 2022
smitpatel added a commit that referenced this issue Aug 13, 2022
@smitpatel smitpatel added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Aug 14, 2022
smitpatel added a commit that referenced this issue Aug 15, 2022
smitpatel added a commit that referenced this issue Aug 15, 2022
@ghost ghost closed this as completed in #28665 Aug 15, 2022
ghost pushed a commit that referenced this issue Aug 15, 2022
@smitpatel smitpatel modified the milestones: 7.0.0, 7.0.0-rc1 Aug 15, 2022
@ajcvickers ajcvickers modified the milestones: 7.0.0-rc1, 7.0.0 Nov 5, 2022
This issue was closed.
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. ef6-parity punted-for-6.0 type-enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.