abpframework / abp

Open Source Web Application Framework for ASP.NET Core. Offers an opinionated architecture to build enterprise software solutions with best practices on top of the .NET and the ASP.NET Core platforms. Provides the fundamental infrastructure, production-ready startup templates, application modules, UI themes, tooling, guides and documentation.
https://abp.io
GNU Lesser General Public License v3.0
12.31k stars 3.32k forks source link

How do I set my primary key ID? #2308

Closed 344089386 closed 4 years ago

344089386 commented 4 years ago

image

Set accessors are not accessible. If new is an object, then I need to write every attribute manually, which is very troublesome.

344089386 commented 4 years ago

@maliming Are there any other methods besides modifying the constructor? Where else can I ask questions besides here.

javiercampos commented 4 years ago

Make it settable on your entity?

public new Guid Id { get => base.Id; set => base.Id = value; }

Just be careful with this approach... Hash codes for an entity should derive only from immutable properties, or things may get tricky (this is one of the reasons why the Id stopped being settable). Setting it on the constructor should be the way to go.

If your problem is using automapper, you can actually create the entity first and then map an object over it:

var myDto = new MyDtoTypeWithIdOnTheConstructor(myIdHere);
ObjectMapper.Map(input, myDto);
gdlcf88 commented 4 years ago

https://github.com/abpframework/abp/issues/1931#issuecomment-546584008 I have the same question.

344089386 commented 4 years ago

Make it settable on your entity?

public new Guid Id { get => base.Id; set => base.Id = value; }

Just be careful with this approach... Hash codes for an entity should derive only from immutable properties, or things may get tricky (this is one of the reasons why the Id stopped being settable). Setting it on the constructor should be the way to go.

If your problem is using automapper, you can actually create the entity first and then map an object over it:

var myDto = new MyDtoTypeWithIdOnTheConstructor(myIdHere);
ObjectMapper.Map(input, myDto);

The CreateAsync method does not automatically assign the Id when adding data, so the Id is assigned manually, but the Id is not accessible. I think it is ok according to your method, but I don't think it is a good method.I need to override the Id attribute for the rest of my business, right?

344089386 commented 4 years ago

@javiercampos

maliming commented 4 years ago

Mapping from dto to entity is not suggested. You need to use constructor.

https://github.com/abpframework/abp/issues/1931#issuecomment-547744013

344089386 commented 4 years ago

Mapping from dto to entity is not suggested. You need to use constructor.

Wouldn't it be a hassle if I had too many attributes. @maliming

maliming commented 4 years ago

This is the recommended best practice.

https://github.com/abpframework/abp/blob/dev/modules/blogging/src/Volo.Blogging.Application/Volo/Blogging/Blogs/BlogAppService.cs#L64

https://github.com/abpframework/abp/blob/dev/modules/docs/src/Volo.Docs.Admin.Application/Volo/Docs/Admin/Projects/ProjectAdminAppService.cs#L50

https://github.com/abpframework/abp/blob/dev/modules/blogging/src/Volo.Blogging.Application/Volo/Blogging/Posts/PostAppService.cs#L154

https://github.com/abpframework/abp/blob/dev/modules/blogging/src/Volo.Blogging.Application/Volo/Blogging/Comments/CommentAppService.cs#L85

cuibty commented 4 years ago

when is object-to-object mapping used? @maliming

maliming commented 4 years ago

@cuibty

gdlcf88 commented 4 years ago

I will change my application service code for creating entity. But it seems tedious if the entity is complex.

javiercampos commented 4 years ago

@cuibty

  • Don’t use DTO to entity auto-mapping, use entity constructor.

Why so? we do use automapper profiles for complex entities (including checkings, and pre/post-conditions) and it works just fine for us. What's the reasoning for not using automapper on a DTO -> Entity mapping?

gdlcf88 commented 4 years ago

Automapper could be used for creating entity if IGuidGenerator will automatic create a guid for empty Id property in repository.

maliming commented 4 years ago

@javiercampos

Aggregating roots and entities may not be simple POCO classes. This is the recommended practice.

https://docs.abp.io/en/abp/latest/Entities

javiercampos commented 4 years ago

@javiercampos

Aggregating roots and entities may not be simple POCO classes. This is the recommended practice.

https://docs.abp.io/en/abp/latest/Entities

I don't see how that's incompatible with Automapper... you can map DTO properties to the target entity methods (using ConstructUsing() for the constructor, using AfterMap(), conditional mappings, etc.). I personally like the simplicity of having all possible code that maps an Entity to the multiple possible DTOs and reverses on a single place (which would be an automapper profile in this case).

Yes, of course you could do manual converters and have them on a single place too, but nothing prevents you to do that with Automapper -plus- having its simple property-property mappings where it makes sense.

I agree with you that a simple convention-based automapper mapping won't do for complex entities, but automapper is flexible enough to use on complex scenarios, and it's a great way to organize (IMO)

javiercampos commented 4 years ago

@344089386 by the way, to give a concise answer to your initial question, if you have a constructor which allows you to write the Id (which is a good idea to have, if anything, for seeding data), you can use that in your mapping... something like:

CreateMap<MyDto, MyEntity>()
   .ConstructUsing(dto => new MyEntity(dto.Id))
   // Rest of your mapping profile
   ;
344089386 commented 4 years ago

@344089386 by the way, to give a concise answer to your initial question, if you have a constructor which allows you to write the Id (which is a good idea to have, if anything, for seeding data), you can use that in your mapping... something like:

CreateMap<MyDto, MyEntity>()
   .ConstructUsing(dto => new MyEntity(dto.Id))
   // Rest of your mapping profile
   ;

Thanks, I've solved this problem with the constructor.

gdlcf88 commented 4 years ago

cool. @javiercampos @344089386

gdlcf88 commented 4 years ago

@344089386 by the way, to give a concise answer to your initial question, if you have a constructor which allows you to write the Id (which is a good idea to have, if anything, for seeding data), you can use that in your mapping... something like:

CreateMap<MyDto, MyEntity>()
   .ConstructUsing(dto => new MyEntity(dto.Id))
   // Rest of your mapping profile
   ;

What if mapping form OrderDto which has property List<OrderItem> OrderItems? How can we set Id for each OrderItem?

javiercampos commented 4 years ago

@344089386 by the way, to give a concise answer to your initial question, if you have a constructor which allows you to write the Id (which is a good idea to have, if anything, for seeding data), you can use that in your mapping... something like:


CreateMap<MyDto, MyEntity>()

What if mapping form OrderDto which has property List<OrderItem> OrderItems? How can we set Id for each OrderItem?

You'd need a mapping defined for the dto class of the orderitems to your entity class

gdlcf88 commented 4 years ago

In this way, CreateEntityDto should have a Id property?

gdlcf88 commented 4 years ago

Found something: https://github.com/abpframework/abp/blob/00f4171af94a285c3525297a3a726e48303fe352/framework/src/Volo.Abp.Ddd.Domain/Volo/Abp/Domain/Entities/EntityHelper.cs#L86