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

Enhancement: Use lambda to define property handlers within DTO constructor. #421

Closed
cwaldron opened this issue Apr 20, 2020 · 8 comments
Closed
Assignees
Labels
deployed Feature or bug is deployed at the current release request A request from the community.

Comments

@cwaldron
Copy link
Contributor

cwaldron commented Apr 20, 2020

I agree with request #415 because as @guy-lud mention, attributes on DTOs are extremely awkward. I opened this as a separate issue because I think this may not work within the existing ClassMap scheme. I want to define are property handlers within the DTO constructor. This makes it easier to organize and keep the logic tied to the DTO.

I would like to define the property handlers like this.

[Map("CompleteTable")]
private class StringToDateClass : BaseEntity<StringToDateClass>
{
    [Primary]
    public Guid SessionId { get; set; }
    public string ColumnDate { get; set; }

    public StringToDateClass()
    {
        Map(p => p.ColumnDate).Convert<DateTime, string>(o => o.ToString("M'/'d'/'yyyy h:mm:ss tt"));
        Map(p => p.ColumnDate).Convert<string>(o => DateTime.Parse(o, CultureInfo.InvariantCulture));
    }
}

This modification fixed the three unit tests that were failing:

TestSqlConnectionCrudConversionFromStringToDateTime2
TestSqlConnectionCrudConversionFromStringToDateTime
TestSqlConnectionCrudConversionFromStringToDate
I modified the function factories (FunctionFactory.cs) to recognize BaseEntity as well as adding two helper classes to enable calling and caching these converters: EntityPropertyConverter, EntityPropertyMap

The current manner by which the property handlers are defined is quite awkward because you have to create a separate disjoined class for the handler operators (get, set). This way the handlers are defined as lambda directly in the class.

I think this could be added as part of the @guy-lud ClassMap request.

@mikependon mikependon self-assigned this Apr 20, 2020
@mikependon mikependon added request A request from the community. under assessment The feature request is under assessment labels Apr 20, 2020
@mikependon
Copy link
Owner

mikependon commented Apr 20, 2020

Historically, RepoDb understand the same base class named BaseEntity, but I removed it to clear some ground working before the development.

Anyway, with regards to your classes, please allow me to review your changes and let us discuss if that is feasible for PR.

I am thinking that once @guy-lud issue a PR, such request will be covered as well.

Not to mention, in the latest Beta release, the classes named PropertyHandlerMapper and PropertyHandlerCache is provided to cater the expression-based property handling mapping.

I am dreaming of having these:

var mapper = new ClassMap<Customer> // or FluentMap<Customer>
     .Name("[dbo].[TableName]")
     .Primary(e => e.Id)
     .Identity(e => e.Id)
     .TypeMap(e => e.DateOfBirth, DbType.DateTime2)
     .ProperHandler<AddressPropertyHandler>(e => e.Address);

The underlying calls would simply call the existing mappers.

public class ClassMap<TEntity> where TEntity : class
{
     public ClassMap<TEntity> ProperyHandler<TPropertyHandler>(Expression<Func<TEntity, object>> expression)
     {
          PropertyHandlerMapper.Add<TEntity, TPropertyHandler>(expression);
          return this;
     }

     ...

}

TBH, the changes would be small and less impact as new class will be introduced that calls the mappers. The full control would be 100% on the the PR issuer side. Of course, the Unit and Integration Tests must be written, and I can help here.

In the operational POV and maintainability, it is easy as we are only looking at the Cachers and Mappers.

Are you interested on PR?

@cwaldron
Copy link
Contributor Author

cwaldron commented Apr 20, 2020

I'll need your help setting up a pull request. I forked your tree and did the work in that fork. Would setting up a pull request mean cloning your tree and creating a branch? I also think that this request aligns with issue #413. Your guidance is greatly appreciated. I've updated the initial comment as the markdown format was incorrect.

EDIT: I figure out how to create a pull request from another fork. Here's the link ...
Pull Request: #422

Cheers.

@mikependon
Copy link
Owner

I wonder why I could not see the PR directly to RepoDb master branch.

@mikependon
Copy link
Owner

There you go, I can now see it. Please allow me to review it.

@cwaldron
Copy link
Contributor Author

Thanks for taking the time to assess. RepoDB is a looking really good. I love what you did with your use of Linq Expressions.

Cheers.

@mikependon
Copy link
Owner

@cwaldron - I liked that you dive down in the core compiler implementation of RepoDb (the FunctionFactory.cs) - that's a bit difficult. Thanks for that and cheers!

Anyway, TBH, your changes seems to be a lot and it is a bit risky to do some acceptance of PR as big as this. Also, some of it is seems against to architectural decisions made historically. I would be very happy if you had contacted me if you had plan to do a PR like this so we had collaborate specific area to be changed. Apology but I think I am not approving these PR.

On the other hand, I liked to merged some of your extension methods. Also, I am opening the implicit class/fluent mapping to the public, and if you try looking at it, it seems to be solving your problem as well - with much more simplier and better approach consolidated to a single class. You are free to take it if you would like, we can just notify @guy-lud.

I hope this would be okay to you.

@cwaldron
Copy link
Contributor Author

@mikependon I really appreciated the feedback. It was one of those "what-if" things that I wanted to try out before contacting you. I wouldn't have made sense if I didn't have a working version. I didn't expect you to accept the PR but just to take a look at it and see if the approach would be useful. It really didn't take long to put together and I really marvel at your use of Linq Expression. There really is not much thorough documentation on using Linq Expression and I learned a lot by going through your code.

After reading @guy-lud request, I realized that implementing BaseEntity class , despite being optional, is problematic because it makes the DTO dependent on the ORM. So I clearly understand your decision to reject the PR but it was fun anyway.

Cheers,
Chris.

@mikependon mikependon added deployed Feature or bug is deployed at the current release and removed under assessment The feature request is under assessment labels May 10, 2020
@mikependon
Copy link
Owner

This request of yours can be addressed by FluentMapper class.

FluentMapper
	.Entity<Customer>() // Which Entity Model?
	.Table("[sales].[Customer]") // Table/View Mapping
	.Primary(e => e.Id) // Primary Mapping
	.Identity(e => e.Id) // Identity Mapping
	.Column(e => e.Id, "CustomerId") // Column Mapping
	.Column(e => e.Name, "CustomerName") // Column Mapping
	.DbType(e => e.DateOfBirth, DbType.DateTime2) // DbType Mapping
	.PropertyHandler<CustomerAddressPropertyHandler>(e => e.Address); // PropertyHandler Mapping

The feature is available at RepoDb (v1.11.0).

> Install-Package RepoDb -version 1.11.0

Please visit the Implicit Mapping to learn more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deployed Feature or bug is deployed at the current release request A request from the community.
Projects
None yet
Development

No branches or pull requests

2 participants