Dapper icon indicating copy to clipboard operation
Dapper copied to clipboard

V2: [Column] and [Table] Attributes

Open NickCraver opened this issue 7 years ago • 80 comments

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:

[Table("Users")]
public class User
{
    public int Id { get; set; }
    public string Name { get; set; }
    [Column("Address")]
    public string AddressSomethingCrazy { get; set; }
}

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:

namespace Dapper
{
    public partial 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; }
    }
}

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:

SqlMapper.TableNameMapper = type =>
{
	if (type == typeof(User)) return "Users";
	return SqlMapper.DefaultTableNameMapper(type);
};

Note: these work together, the Options.DefaultTableNameMapper and Options.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. Perhaps Column has a CaseSensitive 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 or Func 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

NickCraver avatar Mar 14 '17 03:03 NickCraver

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.

phillip-haydon avatar Mar 14 '17 03:03 phillip-haydon

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 tb_TableName or the columns are chrColumnName 🤢

I prefer my database identity columns to be in the form of TableNameID, and my c# classes as just ID. I would like the flexibility to setup a convention to map ID to [ClassName]Id and just reuse it for all of my queries and stored procs.

Thanks!

Gonkers avatar Mar 14 '17 04:03 Gonkers

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.

uliian avatar Mar 14 '17 04:03 uliian

A suggestion for the Attributes vs Config discussion:

Go with the Config option, and supply an implementation (in Dapper.Contrib?) which would do attribute mapping. That way by default Dapper would remain small, but if Attribute mapping is required, add a reference to Dapper.Contrib. You could even supply a .UseAttributeMapping extension which would apply all of the Func<Type...> required in one go if needs be.

Personal bias is towards Config rather than Attributes, but I could live with either. Config feels more flexible however.

Pondidum avatar Mar 14 '17 06:03 Pondidum

+1 for @Pondidum. The Config is a must for the Dapper Core, the Attributes are for Dapper.Contrib only (optional). Since I use Dapper.Contrib I will use the Attributes - and I would like to have an option to change/enhance the Attributes by the Config.

Just a minor note: you should decide, what will happen, when TableNameMapper, ColumnNameMapper return null or empty string - either fall back to the default mappers or throw an exception.

xmedeko avatar Mar 14 '17 06:03 xmedeko

@Gonkers another option there is a virtual method on the attribute and subclass to change default behavior; worth consideration

mgravell avatar Mar 14 '17 07:03 mgravell

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, SqlMapper. DefaultTableNameMapper would look for the [Table] attribute and use it if present, otherwise it'd do what we do today. The same holds for SqlMapper. DefaultColumnNameMapper, it'd look for ColumnAttribute if present, and default to the current behavior if not.

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?

NickCraver avatar Mar 14 '17 11:03 NickCraver

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 Caliburn.Micro project does config the same way. Dapper.Contrib may just plug in own mappers to provide Attribute mappings.

xmedeko avatar Mar 14 '17 11:03 xmedeko

@nickcraver - For conn.Query<Users>("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.

philn5d avatar Mar 14 '17 11:03 philn5d

@xmedeko I think so many people have asked for this type of mapping, it simply no longer belongs in .Contrib, it's something we have to take on and maintain. It also means it can work by default, which is easier for the user. Without that, I think we get a lot of people that forget Options.TableNameMapper = .... and Options.ColumnNameMapper = ... and wonder why their attributes aren't working everywhere.

It also means core things like type mapping conversions could never be on those attributes (e.g. handling GUID <> byte[]) and such across providers. There so much extensibility we can do if core is aware of these. That's the rationale for splitting them into .Contrib, given they'd be completely optional to use?

@philn5d That's done via the generic args on the multi-map overloads, e.g. .Query<Blog, Post, Comment, User, Blog>(), so I'm not sure where string names would come into play, can you clarify a bit? Or do we just suck at making people aware the multi-map functionality exists? I want to do some Jekyll docs with v2, so please, let us have it on where the docs suck.

NickCraver avatar Mar 14 '17 12:03 NickCraver

I think I misunderstood the 1/both/many part - suggested implementation so they work together is fine. I would only be marginally concerned with bloat, but it seems like it'll be pretty minor either way.

Pondidum avatar Mar 14 '17 12:03 Pondidum

I just updated the issue to clarify the confusing points, hopefully a bit better for getting on the same page.

NickCraver avatar Mar 14 '17 12:03 NickCraver

@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:

  • A migration tool for old and new database schemes.
  • Storing customer data on one database and global application on another.
  • Converting a server database to SQLite to be sent to mobile users.

Flash3001 avatar Mar 14 '17 12:03 Flash3001

@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.

NickCraver avatar Mar 14 '17 12:03 NickCraver

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.

mgasparel avatar Mar 14 '17 13:03 mgasparel

@NickCraver I haven't seen the big picture. All alternatives I could think of after your explanation would do more harm than good.

Flash3001 avatar Mar 14 '17 13:03 Flash3001

@nickcraver that's on me - wasn't thinking clearly about that.

philn5d avatar Mar 14 '17 14:03 philn5d

I really like the attributes, they are unsurprising and smooth and cover the 95% use case quite nicely. Not so much the Options class. Here are two reasons for it and some alternatives.

  1. As a matter of taste I don't like the fact that we have almost duplicate properties with unclear semantics e.g a TableNameMapper and a DefaultTableNameMapper. I understand what you are trying to achieve, but I don't think it's a clean solution because it sort-of forces the client to use methods in-order. A possible alternative that would get us most of the way there would be to have only one set and a MappingBehavior enum telling Dapper whether to chain the default behavior on null or empty response from the Funcs.
public static class Options
{
    public Func<Type, string> TableNameMapper { get; set; }
    public Func<Type, PropertyInfo, string> ColumnNameMapper { get; set; }
    public MappingBehavior Behavior { get; set; }
 } 
  1. While having a runtime version of the mapping options and a compile time version as well is a very good idea, I think we can take this a notch further. I'm thinking of use cases where some plug in or dll is loaded dynamically at run time, bringing in its own mappings. It would be difficult to support multiple Dapper Options being set with the current proposal. We could solve this by treating Options a bit like an event handler
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

sklivvz avatar Mar 14 '17 14:03 sklivvz

@NickCraver Sorry I misunderstood. I'm all for both optional config and attribute.

Gonkers avatar Mar 14 '17 14:03 Gonkers

@sklivvz an event handler model is interesting, but ultimately there's a fundamental race to attach rather than explicit control. For example: " it sort-of forces the client to use methods in-order" ...that's true with either approach 1 or 2. But with a setter, you get explicit control.

If the defaults are confusing, what if we put the default behaviors all in their own namespace? Like this:

namespace Dapper
{
    public static class Options
    {
        public Func<Type, string> TableNameMapper { get; set; }
        public Func<Type, PropertyInfo, string> ColumnNameMapper { get; set; }

        public static class Defaults
        {
            public Func<Type, string> TableNameMapper { get; }
            public Func<Type, PropertyInfo, string> ColumnNameMapper { get; }
        }
    }   
}

Note that Dapper has many more options - this proposes moving them all under the same scheme to be consistent. Users wouldn't need to type SqlMapper for almost all use cases, which is also more consistent with the extension nature of Dapper. The defaults don't have to be exposed at all, we could force people to get then set - but I think that's not very user friendly, so IMO they should be available.

NickCraver avatar Mar 14 '17 16:03 NickCraver

Just want to throw this out there if you had not seen it before... https://github.com/henkmollema/Dapper-FluentMap I have used it before and it was pretty good for many of my needs.

Gonkers avatar Mar 14 '17 17:03 Gonkers

@NickCraver @Flash3001 note you can go without static Options - you may create some kind of Context or Session and do session.Query(....). E.g. that's what Java Hibernate does. AFAIK it goes beyond the scope of this issue, and maybe is not welcome for simple ORM like Dapper is.

xmedeko avatar Mar 14 '17 17:03 xmedeko

@Gonkers Yep, if that style suits you then by all means. In terms of performance it's hard to do that without allocations and analysis overhead at many points along the way, so an awesome task for a separate library to tackle.

@xmedeko That brings a big dependency injection dependency ;) I'd definitely say not something that's in the goals of Dapper, IMO.

NickCraver avatar Mar 14 '17 20:03 NickCraver

I'm using Dapper exclusively (for the moment) on .NET Core projects, and at least for one of those projects, I haven't decided between PostgreSQL and MSSQL for production--the semantics may be slightly different for each one.

So, my preference would be a configuration object (which may or may not utilize attributes if available) that I can instantiate and inject. IMHO, the configuration object should be responsible for falling back to the default behavior. This would, for example, make it child's play to obtain the default value and then optionally decorate it -- adding a table prefix, lowercasing, etc. -- or to "disable" a field so it isn't read.

Some minor thoughts:

  • If the configuration responds with null/empty for a given property, Dapper should ignore the property.

  • The configuration object may need access to the POCO, not just its type, to determine the correct field name.

  • If Insert and Update are planned for the future (I'm using SimpleCRUD now, so I hope this is the case!), the interface for obtaining the table/field name should include a flag for the action at hand. For example, a dev may need to read from a view but insert/update against a table, or certain fields may be computed or otherwise managed in the background for reading but should not be part of an INSERT/UPDATE.

  • 10+ years ago, I had my own .NET ORM that used attributes for the field name and a few other odds and ends (default value for null, max length for string fields, etc.). Back then there was a performance hit for enumerating the attributes, enough so that I believe I ended up caching them. Is that still an issue? (If so, the default configuration implementation could cache them. Maybe JSON.NET would be a good library to check to see if they've had to work around any perf issues with attributes.)

richardtallent avatar Mar 15 '17 01:03 richardtallent

@richardtallent thanks for chiming in, follow-ups with some more context:

If the configuration responds with null/empty for a given property, Dapper should ignore the property.

I'm not sure exactly how this will work in both directions, but yes - probably this behavior.

The configuration object may need access to the POCO, not just its type, to determine the correct field name.

This won't be possible, Dapper only figures out these bits once and caches the (de)serializer, via writing IL. So it's not per-object...we can't do that in a performant way. This functionality is what makes Dapper so fast.

If Insert and Update are planned for the future...

I think these will remain in .Contrib, and will be able to access fields, but differing view vs. table read/writes, etc. likely will never be a target use case. If it's something that works then great, but I think it'd be hard to justify complication for them.

Back then there was a performance hit for enumerating the attributes

See point 2 above - this isn't a tremendous concern since we cache the deserializer (and we'd cache this mapping, mental note: clearing it on a Func<> set). We want it to be fast, but there is an overall limited impact given the nature of Dapper here. @mgravell also maintains protobuf-net, which we can steal a few tricks from here.

NickCraver avatar Mar 15 '17 02:03 NickCraver

@NickCraver - no you didn't botch anything. I'm just giving my opinion :) my concern is ok so you're adding support for configuring mismatch between classes and database naming. But you're adding support for 2 groups of people. Those for attributes, and those against attributes. So what happens in the future when another group of people want another feature. I'm worried that over time all these 'little' things will just begin to bloat and slow Dapper.

I just hope I can still have a toggle for supporting underscores, since I use PostgreSQL, it's nice to be able to just toggle that on and be done with it.

phillip-haydon avatar Mar 15 '17 03:03 phillip-haydon

@NickCraver just about non-static Options (Context, Session). It does not mean big dependency injection dependency for everyone, just for those, who need to use more contexts. For the most of the users there may be a static default instance, so they may use Default.Options and they may keep using connection.Query() since the Query would use the Default context. Anyway, IMO it a little bit off-topic. I think @Flash3001 should create a new issue, if he needs this feature.

xmedeko avatar Mar 15 '17 07:03 xmedeko

@NickCraver I would love this set up. It captures the possibility to map everything using your own conventions in one function. Just the way my PR https://github.com/StackExchange/Dapper/pull/653 was about.

This could also return a null as an alternative for the [Ignore] attribute. Is that a possibility also?

frankhommers avatar Mar 17 '17 09:03 frankhommers

Agree with the approach - flexible to allow anyone to implement their own conventions.

@phillip-haydon : you talked about toggling a postgres convention. I think a standard set of conventions should be made available as part of the official project... however not in the core Dapper library.

for example: a Dapper.PostgresConventions library that allows easy application of this convention. Similarly, a Dapper.AttributeMapping library .. etc etc.

mjashanks avatar Mar 23 '17 12:03 mjashanks

A bit late to the discussion, but I thought I'd chime in anyway since we extensively use both Dapper and Dapper.Contrib

Majority of our Dapper projects connects to a PostgreSQL database so the column sensitivity when using Dapper.Contrib was an issue. For this purpose, we forked Dapper.Contrib and disabled quoting by default.

The upcoming changes are exciting and feels like a great step forward.

I'm +1 on the configuration/convention and attributes change and bringing [Table] and [Column] to Core Dapper.

One prime example here is [Column(PrimaryKey = true)] (of which there could be many), and possibly some others along those lines.

@NickCraver: Could you please elaborate where something like this would be useful in Dapper Core? I can see its use more in Contrib for Get, Update but a little hazy on where Dapper Core can make use of it.

for example: a Dapper.PostgresConventions library that allows easy application of this convention

I really like how the approach would allow for things like this. :+1:


Additionally, I think that we have to spend some extra time with documentation. I remember recently having to probe the code base and look around on StackOverflow (tsk tsk) for quite a while before I could understand how to express the JsonB type in C# for me to use it via Dapper.

I'm all up for a documentation overhaul and would be more than happy to contribute to it. Have in the past as well.


Thanks for the efforts @NickCraver and @mgravell :tada:

shrayasr avatar Apr 09 '17 11:04 shrayasr