MapsterMapper / Mapster

A fast, fun and stimulating object to object Mapper
MIT License
4.25k stars 326 forks source link

New Record detection - Adapt To Class not create new instance #634

Closed DocSvartz closed 9 months ago

DocSvartz commented 9 months ago

Partial Fix Bug from issue #537

Not Fix: 1) Collection bug - #430 2) Modification from Object To Type #524

Changes: 1) Adapt (Class or Record) To Class not create new instance
2) Adapt Record To Record always Create new Instanse (before only RecordStruct and PositionalRecord) possible regression

Regress: 1) Required configuration Constructor from Class that does not have a public constructor without parameters and contains Properties with private setters From .Compile() configuration.

2) Current spec of record description at the end of the page Temporarily not supported.

Problems: 1) This Bug was involved in the mechanisms of .Fork() and .Clone() configuration

andrerav commented 9 months ago

Nice work! I added some comments on typos/symbol names that should be fixed.

DocSvartz commented 9 months ago

@andrerav Ok, I'll try to fix it within a couple of days.

DocSvartz commented 9 months ago

@andrerav
I also did some testing in public builds available in Nuget (7.2.0;7.0.0; 6.5.1; 5.3.2)

Collections had the required behavior in build 5.3.2

Modification from Object To Type was not supported in any of them

Classes were completely degraded in version 7.0.0; in version 7.2.0, classes with a public constructor without parameters returned to their previous behavior.

I hope this helps in resolving other issues

DocSvartz commented 9 months ago

@andrerav be Current record definitions From https://github.com/MapsterMapper/Mapster/wiki/Data-types#mappable-objects and identification problems described

Perhaps you should add new Attribute From Types, Field and Property MapsterRecord (AdaptAsRecord) - forces processing as a record MapsterNotRecord (AdaptAsNotRecord)- forces not to processing as a record

How do you feel about this decision?

andrerav commented 9 months ago

@andrerav be Current record definitions From https://github.com/MapsterMapper/Mapster/wiki/Data-types#mappable-objects and identification problems described

Perhaps you should add new Attribute From Types, Field and Property MapsterRecord (AdaptAsRecord) - forces processing as a record MapsterNotRecord (AdaptAsNotRecord)- forces not to processing as a record

How do you feel about this decision?

Hm. Given the circumstances this could be an okay compromise to help users that end up in this situation. Maybe something like:

[AdaptAs(Source.Record)]

To get a cleaner API. I think we should open a separate issue on that though. What you've done in this PR should already fix the problem for a lot of users :)

DocSvartz commented 9 months ago

@andrerav Sorry, I reread the record specification again https://learn.microsoft.com/ru-ru/dotnet/csharp/language-reference/proposals/csharp-9.0/records#copy-and-clone-members and it seems that he did not take differences in constructor attributes for sealed versions of the record. Can I add a correction here or should I open a new PR?

andrerav commented 9 months ago

Feel free to correct this one :)

DocSvartz commented 9 months ago

@andrerav I checked collection and object adapt bug. These are independent bugs. Unrelated to record detect - class problems. Adapt Record To Record always Create new Instanse - In terms of creating a new instance, it corresponds to the specification for the modification of the record

This equal _destination with {_sourse.field ..}, although it happens differently :)

DocSvartz commented 9 months ago

Don,t check. I try fixing fix :) on next week. To minimize the false-positive detection of Fake Records.

andrerav commented 9 months ago

@DocSvartz Is this PR ready to be merged? Or do you still have any work in progress that you want to push before I merge it?

DocSvartz commented 9 months ago

Yes, I've made the final edits. 1) I will add Attribute to (separate PR). 2) I'll try to revert the behavior to the old specification Record if it doesn't cause bugs (separate PR).

Attribute Most likely it will be like this [AdaptAs(Destination.Record)]

This Bug occurred when Fake Record was in the role TDestination :)

andrerav commented 9 months ago

Thank you!