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 - Implemented ColumnNameMapping #653

Closed
wants to merge 2 commits into from

Conversation

frankhommers
Copy link

@frankhommers frankhommers commented Dec 1, 2016

I would love to see this pulled!

SqlMapperExtensions.ColumnNameMapper = (propertyInfo) =>
{
  Regex idAtEndRegex = new Regex("[a-z]Id$", RegexOptions.CultureInvariant);
  return idAtEndRegex.Replace(
      propertyInfo.Name, 
      x => { return x.Value[0] + "_" + x.Value.Substring(1); }
    ).ToLower();
};

This will allow you to write a custom Property.Name to column mapping.

The default behaviour does not change.

The example code changes the mapping from property PersonId to database field person_id.

@frankhommers frankhommers changed the title Implemented ColumnNameMapping Dapper.Contrib - Implemented ColumnNameMapping Dec 2, 2016
@frankhommers
Copy link
Author

This is duplicate in terms of functionality, not in implementation: #623

@NickCraver
Copy link
Member

Note: there are quite a few other changes in here which may or may not be intentional effects:

  • Method signatures (breaking)
  • Column names (e.g. quotes in Postgres) (breaking)

@frankhommers
Copy link
Author

@NickCraver:

Method signatures: I don't really see how this is breaking? The behavior from the "outside" is the same.

Column names for Postgres: Column names quoting in Postgres is just fixed and not breaking, the old behaviour was somewhat incorrect. That is fixed, but it's certainly not a breaking in change.

@NickCraver
Copy link
Member

@frankhommers

  • Methods: you're changing a non-private interface others may have implemented. That's a hard break.
  • Columns: You're adding quotes, that suddenly means casing must match. In Postgres non-quoted are effectively case insensitive due to a lower fold. This means if my properly was MyColumn and the database had mycolumn or MYCOLUMN or MyCoLuMn, it previously worked as it matched any. When quoted, those people with any mismatch are instantly broken. That is absolutely a breaking change.

Hope the above clarifies. I'm not against quoting in v2, but that has to be a major version change and we'll have to be very explicit about it in a breaking changes section.

@frankhommers
Copy link
Author

@NickCraver

Methods:
I can add seperate interfaces for this new functionality so it won't break existing interfaces. But are you willing to pull it then? Otherwise it's a lot of work for me for nothing ;)

Columns:
The AppendColumnName for Postgres does add quotes in the original code for selects, but the insert does not. I just thought that was very inconsistent. I think that's a bug in my opinion, because the select might get other fields than the insert. Maybe I should've put that in a seperate pull request.

@NickCraver
Copy link
Member

@frankhommers Nope, I wouldn't pull it then. This is simply ill-suited for a non-breaking-changes release. We need to revisit column quoting entirely in V2. And it's not simple a Postgres problem. I also don't know how columns will end up, this is one of over a dozen proposals on columns. The thing is, to be blunt, a damn mess.

We'll likely have a [Column] attribute and some way to map types to which you can't add attributes. If there's a hook point for all mapping conversions (likely), it wouldn't live on SqlMapperExtensions. This simple needs far more design and we have to find something that works for all providers. Putting this in would be a temporary addition to further complicate and make life harder for anyone that does use it, since we'll absolutely break it in V2.

We need a full API discussion around Column mapping for .Contrib, which will also affect Dapper core, as that has to read the columns as well. And of course there are performance concerns there.

@frankhommers
Copy link
Author

OK. I really hope you can put the idea into V2 then. Writing a single function that converts all names in your POCO's to the database fields is just awesome. Just one place to capture your naming conventions! Instead of littering the whole solution with attributes and mappers.

@NickCraver
Copy link
Member

@frankhommers Sorry I've been so busy, but I wrote up an issue for discussion in #722, I'd love your thoughts - can you please chime in?

I'm closing out all of the existing PRs on the issue because of the reasons above...this needs to be a far deeper change/feature...and nothing yet would be near usable for that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants