Skip to content
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 - Column mapping #623

Closed
wants to merge 14 commits into from

Conversation

mjashanks
Copy link

@mjashanks mjashanks commented Sep 28, 2016

This allows us to apply Property -> Column mapping in a pluggable way.

I originally made this pull request, which was too opinionated: #376

That previous PR was closed, and batched into: #385

I believe that issue 385 is not the correct place for this, as it is talking specifically about making the mapper more pluggable (i.e. Mapping the data from SqlDataReader to a POCO) - which would only cover SELECTs.

My PR, is more about the generated SQL that is executed, thus covers INSERT, UPDATE, DELETE & SELECT - i.e. the responsibility of Dapper.Contrib

I have implemented in a similar way to TableNameMapper .. where the user must override the following delegates:

GetAllColumns - returns all mappings for a type
GetPersistentColumns - returns writeable (i.e. Insert & update) mappings for a type
GetKeyColumns - returns key mappings for a type

This would be a breaking change for anybody who has implemented their own ISqlAdapter, but that is all

I have only tested this for SQL Server

@NickCraver
Copy link
Member

At a cursory glance, this also appears to be a breaking change for anyone with non-aliased columns, inherited classes, etc. The Select * => column list change is quite a huge breaking change. What's the goal here? I don't want to dismiss the ideas off-hand, but this implementation (as-is) would break far too many use cases.

@mjashanks
Copy link
Author

The goal overall is to be able to override the default Column Name To Property Name mapping. So, I believe it would be necessary to generate a column list for Get, in order to do this.

However, I don't understand how this breaks anything. The default behaviour here is to generate a column names, equal to the POCO's property names. Is this not already the assumption with select * ? I.e. select * will produce a datareader that has items named by column names. These items then get mapped directly to Property names. Am I misunderstanding anything ?

@NickCraver
Copy link
Member

@mjashanks There's a matching problem in either direction here. Let me see if I can clear it up a bit:

We have a choice at the first level: do we enumerate all of the columns for the SELECT list, or just the mapped ones with properties? If only the mapped, that's a breaking change. If we don't do only mapped and do all properties, then we hit another issues: properties that aren't in the database at all, generating an invalid SELECT fo columns that don't exist.

In general, Dapper is a "get everything, map what's there", because that's the only safe scenario to do such. Contrib could behave differently in generation forcing mapping of all columns with attributes that that's hard for 2 reasons: a) it's a breaking change, and b) you (currently) have no way to control classes that aren't yours, e.g. from a shared library or something else.

@mjashanks
Copy link
Author

@NickCraver - appreciate you taking the time to explain.

I have modified this so that it uses select *- unless the behaviour is overridden. Thus this aspect should no longer be a breaking change.

@frankhommers
Copy link

I have made a PR with the same functionality, only a different implementation. I believe it's a bit more compatible. #653

@NickCraver NickCraver added the v3.0 Changes awaiting the next breaking release label Jan 28, 2017
@NickCraver NickCraver modified the milestone: v2.0 Jan 30, 2017
@NickCraver
Copy link
Member

@mjashanks I'm consolidating all of the discussion here into #722, would you please weigh in and help us design this feature to work for as many cases as we reasonably can?

@NickCraver NickCraver closed this Mar 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v3.0 Changes awaiting the next breaking release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants