-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Dapper.Contrib Get should select list of columns #482
Comments
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 |
The folks who use What about some pluggable query generator public static Func<Type, string> GetQueryColumnsGenerator = (type) => "*"; so everyone may change it. |
@xmedeko I think you're assuming people who use 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. |
@NickCraver your assumption about my assumption is wrong. I am aware about people who use 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 The |
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 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. |
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 But I think it's pity you do not want to open Other thing is, that almost all functionality is already included in 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. |
Taking a look in v2 to how we'd possibly support this scenario. |
#722 seem perfect for me, thanks. So, I am closing this issue. |
I have a SQLite table
Where
data
is a large clob and do not want to read it in every query. I have a classAnd read a
Car
by the Dapper.Contrib queryIt generates the SQL query
It's is not optimal, since the
data
column (large) is fetched too, and never used. I would expect the queryI.e specify the columns in the
select
clause. TheGetAll()
method should has to be changed, too. (IMHO no ORM should useselect *
in it's core functions.)Note: the
[Computed]
columns has to be omitted, too. With[Computed]
theDapper.Contrib
Get
query reads the column, but Update, Insert wont change the[Computed]
column.The text was updated successfully, but these errors were encountered: