dotnet / efcore

EF Core is a modern object-database mapper for .NET. It supports LINQ queries, change tracking, updates, and schema migrations.
https://docs.microsoft.com/ef/
MIT License
13.82k stars 3.2k forks source link

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

Closed Eirenarch closed 2 years ago

Eirenarch commented 6 years ago

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 commented 6 years ago

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

Eirenarch commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

@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 commented 6 years ago

@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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

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:

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

"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 commented 5 years ago

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();
    }
  }
}
ajcvickers commented 5 years ago

Note from triage: consider also the scenario in #9882.

divega commented 5 years ago

Some notes from a brief conversation @smitpatel and I had:

  1. We can add the ability to create IQueryable<T> query roots for any T that is supported by the provider's type mapper (or custom type mapping/global conversions added by the user once we allow this)

  2. Unlike DbSet for regular and keyless entities, these will come with a null default database mapping. That is, they won't be assumed to be mapped to some database object, and the user will have to specify either FromSql*, defining query or FromData or a runtime exception will occur.

  3. With this design, we can provide a superset of the functionality of Dbontext.Database.SqlQuery in EF6. In addition to everything that API could do, this becomes a regular composable LINQ query root that can be joined with other query roots.

  4. This wouldn't address everything about #9882 which is about execute scalar scenarios. Part of it can be addressed with just FirstOrDefault & friends, but I think for something like invoking a stored procedure that returns a scalar we might need to come up with a separate API, for example ExecuteScalar or ExecuteFunction. I would like suggest that we reopen #9882 to consider it separately.

  5. What is the API for creating these query roots? I am personally leaning towards just reusing DbSet for this (after renaming the type parameter and removing the class constraint). My arguments for this are:

    • It's already a synonym with query root in EF Core
    • We just need to be a bit open minded about replacing any preconception we have on DbSets being only entities. There isn't anything in the name that would make it surprising for a new EF Core user that you can create DbSet<int>
    • We already crossed the bridge that not everything that you can type with DbSet<T> will be valid at runtime. For example, the Add,Remove/Update methods are dead for keyless entity types.
TehWardy commented 4 years ago

We have to stop thinking that workarounds are acceptable and thus not important to fix compared to new features as often new features sit on top of existing ones shaping their results too.

Thinking of DbSets as tables is a good thing IMO, unless we plan to replace them with Table View and so on to reflect what actually exists and thus what behaviour we can use on them. I say this because they have more in common with tables than anything else in the DB.

@divega The fact that an ad-hoc usage of the DB through a context that happens to wrap the known structure requires a composition root at all is weird. It's an arbitrary architecture choice made by the EF Core team and not actually needed is it?

If that's what you mean by points 1 and 3 I agree. That somewhat makes points 2 (with the exception of that FromData bit) and 5 redundant then doesn't it ?

As for point 4 The "resulting reader" from the execution and the "value of the result" are different things are they not? (thinking about this from a SQL server point of view) How does ADO.Net determine the value of an execute scalar call, presumably that can just be a pass through call in EF.

Your previous comment highlights the "hack style" solution we as consumers are forced in to implementing because the API isn't as fluent as it could be.

In your own words ...

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

The ideal IMO is regardless of the "model" defined it should be possible to do something like ...

var results = context.Query<SomeDto>(sql, params).ToArray();

... not applying the ToArray() would give us an IQueryable on to which we could optionally compose.

The current implication is that the model needs to know all possible result types you may or may not ask the context for both now and in the future regardless of the db structure which seems odd.

Why must I tell the model about the type "SomeDto" in the above example? Forcing a composition root leads to the conclusion that @Vasimovic came to above ... it makes no sense and thus doesn't feel like something of value to implement as we aren't gaining anything new.

Using the mapper as I describe though should come with an implicit acceptance that because the data didn't come from a set unless it contains model known types the result set cannot be tracked. But could we return a collection of Dto types that are unknown, and then Include children that are known from a given DbSet and those be tracked? Or is that technically too complex to achieve?

At this point i'm in the realm of complex types from ad-hoc queries not scalar types but i'm told on other tickets that this is the same issue, so hopefully i'm not off topic here.

smitpatel commented 4 years ago

This issue is triaged as stretch goal in 5.0 release.

TehWardy commented 4 years ago

Whilst we wait what's the current way to acheive this in EF Core 3.1 since DbQuery was removed since EF Core 2.x ?

ErikEJ commented 4 years ago

@TehWardy You can do this: https://github.com/dotnet/efcore/issues/1862#issuecomment-597022290

TehWardy commented 4 years ago

That's interesting. So random thought ...

If I don't have a DbSet defined as part of my model it looks like I can still use ...

class MyContext : DbContext
{
      public IQueryable<X> GetX(params object[] args) => Set<X>().FromRawSql("some sql", args);
}

... presumably this ticket is about making this work for say a collection of Guid as this appears to only be definable for reference types at the moment, and i've not run it on a set not in the model.

This would actually solve a ton of weird edge case stuff I have.

ghost commented 4 years ago

This issue is triaged as stretch goal in 5.0 release.

Any news on this? Still planned for 5.0? Haven't found it.

ErikEJ commented 4 years ago

@tsproesser It is in the backlog, and 5.0 is locked down now. In the meantime, you can do this: https://github.com/ErikEJ/EFCorePowerTools/blob/master/src/GUI/RevEng.Core/DbContextExtensions

ghost commented 4 years ago

Ah, ok. Thank you. Thought it would be part of 5.0 and just couldn't find it - but there's the reason. Thank you.

ErikEJ commented 3 years ago

I have published a NuGet package, that gives you this:

var guidList = await dbContext.SqlQueryValueAsync<Guid>("SELECT Id FROM Person");

https://www.nuget.org/packages/ErikEJ.EntityFrameworkCore.SqlServer.SqlQuery

yzorg commented 2 years ago

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

roji commented 2 years ago

@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 https://github.com/dotnet/SqlClient/issues/1009).

smitpatel commented 2 years ago

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 commented 2 years ago
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));
        }
    }

How to do parameters without caring about the underlying connection type?

roji commented 2 years ago

@jjxtra are you asking how to create instances of DbParameter? I'm not sure how that's relevant to this issue, but you can use DbCommand.CreateParameter() for that.

jjxtra commented 2 years ago

@jjxtra are you asking how to create instances of DbParameter? I'm not sure how that's relevant to this issue, but you can use DbCommand.CreateParameter() for that.

Saw that about 5 minutes after asking the question :) Thanks!