Repository navigation
Dapper.Contrib Get should select list of columns #482
Description
I have a SQLite table
create table cars (
id integer primary key,
name text,
data text);Where data is a large clob and do not want to read it in every query. I have a class
public class Car
{
[Key]
public long Id { get; set; }
public string Name { get; set; }
}And read a Car by the Dapper.Contrib query
connection.Get<Car>(id);It generates the SQL query
select * from cars where Id = @idIt's is not optimal, since the data column (large) is fetched too, and never used. I would expect the query
select t.id, t.name from cars t where Id = @idI.e specify the columns in the select clause. The GetAll() method should has to be changed, too. (IMHO no ORM should use select * in it's core functions.)
Note: the [Computed] columns has to be omitted, too. With [Computed] the Dapper.Contrib Get query reads the column, but Update, Insert wont change the [Computed] column.
Activity
NickCraver commentedon Mar 18, 2016
There are a number of problems with generating the select statement in a more complicated manner. For example, Dapper does case insensitive matching as well, so this would be a breaking case for case sensitive databases where the model doesn't match (a breaking change). It also breaks when a column disappears from the database making the select invalid, rather than the property being default or null on the model - this breaks deployments for some people.
The last point goes back to doing it at all - anyone with extra properties on their model for any reason would break. That's likely a very large percentage of users. People have extra properties not matching 1:1 with the database for any number of reasons.
The recommendation fpr such cases were you want to customize the query would simply to to not use
.Get<T>(). Instead, use.QueryFirstOrDefault<T>()from core dapper and write the select elements you want.xmedeko commentedon Mar 18, 2016
The folks who use
UpdateandInserthas to have class properties in sync with DB columns, hasn't they? And they must have case sensitivity solved, too. So the breaking change would be only for the users who userDapper.Contribjust for querying (Get,GetAll).What about some pluggable query generator
so everyone may change it.
NickCraver commentedon Mar 18, 2016
@xmedeko I think you're assuming people who use
.Get()also must be using other methods - that's definitely not the case for many applications. For example, reporting applications. I'm not sure I see the need (in Dapper) for such a generator...we let you plug in whatever SQL you want already. Dapper is very much meant to be a Micro-ORM, and not in the venue of generating complicated queries.I'd step back to the root of your issue here: very rarely is there something so big on a row in the database that you want to exclude it, you need only generate a more complicated query (and only do so once) in that very narrow case. Right now, I disagree with more complication and breaking changes for potentially every single user to support this very narrow case which already has alternatives.
xmedeko commentedon Mar 18, 2016
@NickCraver your assumption about my assumption is wrong. I am aware about people who use
Get()only and for them it may be a breaking change. I just assume many (maybe most)Dapper.Contribusers use update methods, too, and I assume for them if would be no breaking change.First, we should think about the ideal behaviour, then about the backward compatibility and how to join these two things together. Think about it for a minute, would you use
select *if you would start writing a new Micro-ORM library? If you agree with me, that it would be better to useselect id, name ...then now we may try to figure out the way how to change the Dapper while keeping the backward compatibility.The
public static Funcfields are excellent for configurable behaviour and they are used by many other brilliant C# libraries. It keeps the core library small but allows users to extend the behaviour as they like.NickCraver commentedon Mar 19, 2016
That's totally fair, I don't think I read your third sentence correctly on first pass - my fault!
Yep, I would. There are many advantages to doing so. We use
SELECT *for simplicity, plan cache efficiency and the fact that in a 1:1 model environment it's the same behavior as something vastly more complicated 99.99% of the time. We also do this by choice for a great many things at Stack Overflow; it's rarely an efficiency issue.It's really just a non-starter to change it. It's a breaking change. You'd have to introduce a new method or new overload to have such behavior and have a decent fork of many code paths. Or, you could write 1 SQL query in 1 place today. Do you see why I'm so inclined towards the options already available right now?
Dapper is first and foremost simple and efficient. Contrib also tries to go along these same lines. If we're getting into query generation for anything complicated I would, with no ill will, suggest that Dapper may not be the right library to use anymore - we simply differ in goals at that point.
xmedeko commentedon Mar 20, 2016
OK, I understand this improvement is not a starter to do a breaking change.
Note: Currently, I am developing a small app where we have a just few tables where we cannot go with
select *. But a few years ago I have been participating on an app with a huge legacy Oracle DB (with plenty of views). Theselect *in the code has been an unacceptable in that app, since it could have a big, unpredictable, impact on performance (with views usually). (Unpredictable - when someone will add columns to table/view later.)But I think it's pity you do not want to open
Dapper.Contribto user extensions. E.g. even issues like #385 (particularly #376 changing the mapping using a [Column] attribute) need not to be inDapper.ContribIMHO. It would be enough ifDapper.Contribwould provide a possibility to plug in a userFuncto convert a property to the column name. Same with this issue.Other thing is, that almost all functionality is already included in
Dapper.Contrib, because the list of columns has to be generated by theUpdateandInsertmethods. See my HACK. (In fact just a 6 lines of code, the rest is a reflection and methods headers).I can make a short PR to show you it would not have a big impact on the current code and keeps the backward compatibility, if you are interested. Otherwise feel free to close this issue.
NickCraver commentedon Jan 28, 2017
Taking a look in v2 to how we'd possibly support this scenario.
NickCraver commentedon Mar 14, 2017
@xmedeko can you please take a look at #722 and chime in?
xmedeko commentedon Mar 14, 2017
#722 seem perfect for me, thanks. So, I am closing this issue.