-
-
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
V2: [Column] and [Table] Attributes #722
Comments
Attributes Vs Config... I really think Dapper should only support 1 convention, even if there's a group of people who don't like Attributes or prefer attributes and don't like Config. Dapper is a beautiful tiny lightweight ORM that does the bare minimum. Yes it needs some mapping to get around legacy and poorly named tables/columns in databases, and vice versa. But not at the sacrifice of bloating Dapper. /my 2 cents I'm impartial to Attributes vs Config. As long as Dapper doesn't lose focus and become bloat. |
I appreciate your attention to this @NickCraver! Hopefully there is a good solution to cover the vast majority of issues/preferences. I highly prefer config (not attributes) based setup because I may not own the class/model, the other big ones are conventions. Working with legacy schema (I can't change) where all the tables are prefixed like I prefer my database identity columns to be in the form of Thanks! |
I hope that the Dapper is supported in two ways, and Attribute is simple and intuitive. But I think Dapper in complex scenes require enhanced Mapper function, especially in the one-to-many, many-to-many scenario, EF-like configuration method based on Lambda expression expression ability. |
A suggestion for the Attributes vs Config discussion: Go with the Config option, and supply an implementation (in Personal bias is towards Config rather than Attributes, but I could live with either. Config feels more flexible however. |
+1 for @Pondidum. The Config is a must for the Just a minor note: you should decide, what will happen, when |
@Gonkers another option there is a virtual method on the attribute and subclass to change default behavior; worth consideration |
I'm seeing a lot of surprising replies, that's awesome. Let me make sure we're on the same page when discussing first: The proposal isn't for an either/or, it's for both. But they wouldn't be 2 separate approaches, they work together. For example, You'd free to use the attributes or not. You'd be free to use none of this and get the exact same behavior we have today. Is that what everyone understood from the proposal? If not I'll do some editing to clarify. @phillip-haydon I'm not sure I understand the bloat comments here - what we're talking about is very minimal and removes several one-off options both in the project and proposed today - they could instead be included mappers...or the other mappers could be in another project, or something else. Can you explain a bit more? Or did I botch mentioning the totally optional part? @Gonkers same thing - did you read this as either/or - or would you only want to see 1 for some reason? @uliian I don't think we'll be tackling 1:many here, that's a very different generation and consumption problem, likely in Dapper.Contrib for the foreseeable future...but all together another feature entirely. @Pondidum Same question - was the either/or and not both confusing, or does your opinion stay the same regardless? @xmedeko Both are optional...same question, confusing on the proposal? It looks like I need to edit the copy to clarify some things, will wait for responses there. Also related proposal: I think we should move all of these static settings to another place, probably: namespace Dapper
{
public static class Options
{
public Func<Type, string> TableNameMapper { get; set; }
public Func<Type, string> DefaultTableNameMapper { get; }
public Func<Type, PropertyInfo, string> ColumnNameMapper { get; set; }
public Func<Type, PropertyInfo, string> DefaultColumnNameMapper { get; }
}
} ...along with all other options in V2, so they're contained in an intuitive spot. This would make access more intuitive, like this: Dapper.Options.TableNameMapper = ...; Thoughts? |
Yep, we are on the same page. Yes, all is optional for the user. By optional I was thinking something else, never mind about it. You can make just: public Func<Type, string> TableNameMapper { get; set; }
public Func<Type, PropertyInfo, string> ColumnNameMapper { get; set; } And the user should do the mapper chaining: var origMapper = Options.TableNameMapper;
Options.TableNameMapper = (t, s) => {
if (type == typeof(User)) return "Users";
return origMapper(type);
} The |
@NickCraver - For conn.Query("select ...", ...), is it possible to name the "tables" in the split? spliton: "commentid, followerid", splitto: "Comment, User". Also, with conn.QueryMultiple, maybe a way to map result sets to "Table" using ordinal in the set - "Blog,Post,Comment,User" where each is a table in the result set. Maybe getting closer to other features, but related to table mapping nonetheless. |
@xmedeko I think so many people have asked for this type of mapping, it simply no longer belongs in It also means core things like type mapping conversions could never be on those attributes (e.g. handling GUID <> @philn5d That's done via the generic args on the multi-map overloads, e.g. |
I think I misunderstood the 1/both/many part - suggested implementation so they work together is fine. |
I just updated the issue to clarify the confusing points, hopefully a bit better for getting on the same page. |
@NickCraver I don't think using Options as static is a good thing. Someone might need to connect to two different databases at the same time using two different mapping schemes, with static that would require using AppDomains. Examples:
|
@Flash3001 - What's the alternative, passing the options as an overload to every single mapper? Note this affects the deserializer cache, etc. Not making them static (as all options are today) would be a huge burden to take on, for a very small subset of cases. As an example, none of the examples you're mentioning would be practically supported today, since type mappers and such are global/static today. Unless there's a way I can't think of to share this without passing it around everywhere, forking the cache, etc. - I just can't see us supporting those use cases in Dapper, just as we don't really today. |
I really like it. I've been using other libraries on top of Dapper that provide column mapping, and this is a much cleaner solution. I love the decision to implement the attributes using the config, keeping everything flexible and extensible, and up to the user to choose how they want to use it. As for your question about controlling other things from attributes... I personally like the idea of defining the keys via attributes. This lets me keep all my table-level configuration in one place. |
@NickCraver I haven't seen the big picture. All alternatives I could think of after your explanation would do more harm than good. |
@NickCraver that's on me - wasn't thinking clearly about that. |
I really like the attributes, they are unsurprising and smooth and cover the 95% use case quite nicely. Not so much the
public static class Options
{
public Func<Type, string> TableNameMapper { get; set; }
public Func<Type, PropertyInfo, string> ColumnNameMapper { get; set; }
public MappingBehavior Behavior { get; set; }
}
Options.AddMapping(Func<Type, string> mapping);
Options.AddMapping(Func<Type, PropertyInfo, string> mapping); Dapper would run all the user mappings last to first until one returns a non-empty, non-null string. It would eventually run the default mapper as a last option. If there are no plugins I think this would have a minimal perf impact, but of course it's O(N) on the number of plugins. All in all, great job, and a good step forward @NickCraver & @mgravell |
@NickCraver Sorry I misunderstood. I'm all for both optional config and attribute. |
Is this new version with some of these great new features/changes happening at all, I ask as the first post was 2017 so I am guessing perhaps not? |
It seems to me there is no plan or time table for these features to be implemented because dapper works well for StackOverflow and (i think) they don't want to make breaking changes or any performance hit caused by changes or maybe even bother themselves changing their codes. |
@yekanchi This issue, and inability to get pass the shortcomings related to naming without modifying Dapper Contrib source code, inspired me to write a replacement for Dapper Contrib SQL generation part. There are similar solutions but I needed multiple database support using a same codebase - Dapper.SqlGenerator allows it as well as a different schema for different connection strings although only supports SQL Server and PostgreSQL for now - other Dapper Contrib adapters can be relatively easily implemented if one has quick access to these databases and is willing to test it. Dapper.SqlGenerator uses EF Core compatible schema definition, so one can scaffold schema from an existing db using EF Core tools and should handle generated schema without changes, it just ignores definitions not related to SqlGenerator i.e. indexes, using dummy extension methods. Apologies for self-advertisement but hope someone will find this library useful and if you do please report any suggestions. |
If you are looking for Attributes to control the generated queries check my project Generated SQL will fetch back identity and generated columns into the POCO. Also includes attributes to mark columns as readonly/update/insert only. |
None of this got into Dapper v2 right? |
The "Dapper.DefaultTypeMap.MatchNamesWithUnderscores = true" is already working for insert and update in Dapper.Contrib? |
So there is no way to map properties to their columns using |
I‘m using Dapper's native Insert method, but the [Write(false)] attribute under Dapper.Contrib is not correctly recognized. If the author wants to update the attribute tag, it is recommended to use [NotMapped], because many projects are compatible with this tag, and the namespace is :System.ComponentModel.DataAnnotations.Schema. |
It is insane that it's going on 5 years that Dapper.Contrib hasn't implemented a It's also annoying that Dapper doesn't support or respect I guess Dapper's real goal is to make the framework so coupling that you'd never think of moving away from it. |
It is open source, the original developers clearly do not need it, consider offering a commit! |
Mike, I understand the point you're making. But we haven't even gotten a signal of consensus from the maintainers as to what they'd accept in a PR, and they're now working on V3 when this would be a PR for V1. Of course the devs don't need it - this is their own DB ORM so they're fine with any coupling that matches their exact DB object naming conventions. I think it was a pretty significant oversight all things considered from the tool, but as the thread has seen others have basically remade .contrib functionality to work without this coupling. Fingers crossed something along these lines makes it into V3. |
@Ryanman my thoughts exactly. I have to get some code done and in prod, but once I catch a slight break I'll probably be looking for another ORM that would allow that kind of decoupling. It's a shame because Dapper is a decent, lightweight framework, but I can't make that type of coupling choice in an enterprise application. |
@mmulhearn , I have an ORM that uses attributes from DataAnnotations that allows for the decoupling you desire. https://github.com/dallasbeek/Dapper.Database |
Maybe not ideal for your situation, but you could leverage a tool like AutoMapper to map your domain classes to your data classes. Obviously this is not quite as efficient because it involves an intermediate copy, but it would get the job done. |
@pchasco I do use AutoMapper for class to class translations/mapping. However, the issue is pulling the data directly from the database into a class |
What I mean is to construct your classes to exactly model your database schema, without any column remapping. Then use AutoMapper between your data and domain layer to perform this mapping. As stated, not ideal because this is actually exactly one of the responsibilities of ORM, but it would work. Then again, employing a second mapping phase with AutoMapper with the wasted cycles during copy and wasted allocations may moot any benefit to using a lightweight ORM such as dapper to begin with... |
If all you need is aliasing a property so Dapper.Contrib's
I know this is ugly, and works best if the class in question is a DTO to be discarded after the INSERT but it works. |
Here is a small implementation code that is working with v2 for both "properties" and "fields" column mappings:
This can be sticked to any your custom attributes "ColumnAttribute" and "TableAttribute" or create those 2 attributes on your own. To enable the support just register it: |
This would be part of the overall Dapper v2 project (#688)
There's obviously been a lot of interest in controlling column and table name mappings in a coherent and fully featured way. But all of the PRs we've seen are all over the map as to how to go about this. They're also Dapper.Contrib restricted, whereas we need something to cover things overall. For example calling
.Insert<T>()
and then calling.Query<T>
afterwards and that not working isn't expected or awesome.Let me preface this. Both of these are optional. They work together, but are 100% optional - if you use neither then Dapper's existing behavior works as-is with mappings. The proposal is also for both, not an either/or. See below for how they work together:
Basically, this can't be
Dapper.Contrib
, it needs to be global. This type, this member goes to this table, this column...in a controllable way. I'm picturing an API like this:Now, this doesn't work for everyone. You can't add attributes everywhere and some people don't want to. They want conventions, or covering types they don't control - so we need something else. I'm not sure what these are called, but basically we make the type and member to table and column mappings pluggable. For example, something like this:
So if your convention is init caps, underscores, emoji, or whatever really, you could define this in one place. You could call the default mapper as a fallback in your own function as well, if you just want to handle a few cases and fall back for the rest. Or, you could make your own attributes, and entirely base all mappings on whatever scheme you could come up with. As a simple example:
Note: these work together, the
Options.DefaultTableNameMapper
andOptions.DefaultColumnNameMapper
would look at the attributes in their implementation. So this isn't an either/or proposal, the two approaches collaborate to handle all the cases we've seen thus far, while being totally optional all together.All of these would apply across the board, to
.Query<T>
,.Insert<T, TKey>
,.Update<T>
,.Get<T>
, and...well, you get the idea. They need to work everywhere, that's why it's needs to be in Dapper core and consumed all the way through.Contrib
, etc.Now, that deals with naming (I hope) - but there's more we can do here too. For example, is there interest in controlling defaults or something else in attributes? Naming is the biggest thing I've seen by far, but there may be some other areas we can solve along the same lines.
One prime example here is
[Column(PrimaryKey = true)]
(of which there could be many), and possibly some others along those lines.Some other problems the above potentially solves (or not) would be case sensitivity (or not) and being something that the mutation methods (like
.Insert()
) can use. For example, whether to quote a field or not across.Contrib
provider implementations when generating T-SQL would be important. PerhapsColumn
has aCaseSensitive
property...or something else. The two-way nature on the deserialization of that is a thing, but generally handled by fallbacks today. Or it could be handled by exact names in the column attribute orFunc
overrides...or any of the combination of those things. Lots of options here - what makes sense to expose in a simple way for lots of use cases?Related Info
Related issues: #447, #463, #515, #605, #623, #670, #681, #572
Potentially covered: #482, #571
Thoughts? Suggestions? Things you'd like to throw in my general direction?
cc @mgravell
The text was updated successfully, but these errors were encountered: