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

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

Merged
merged 11 commits into from
Oct 15, 2023

Conversation

DocSvartz
Copy link

@DocSvartz DocSvartz commented Sep 25, 2023

Partial Fix Bug from issue #537

Not Fix:

  1. Collection bug - Meaning of mapping to an existing object? #430
  2. Modification from Object To Type Invalid mapping to exists class if source is object #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

@DocSvartz DocSvartz marked this pull request as ready for review September 25, 2023 14:22
@andrerav andrerav self-requested a review September 25, 2023 21:34
@andrerav andrerav added the bug label Sep 25, 2023
src/Mapster.Tests/WhenRecordRegration.cs Outdated Show resolved Hide resolved
src/Mapster.Tests/WhenRecordRegration.cs Outdated Show resolved Hide resolved
src/Mapster.Tests/WhenRecordRegration.cs Outdated Show resolved Hide resolved
src/Mapster.Tests/WhenRecordRegration.cs Outdated Show resolved Hide resolved
src/Mapster.Tests/WhenRecordRegration.cs Outdated Show resolved Hide resolved
src/Mapster.Tests/WhenRecordRegration.cs Outdated Show resolved Hide resolved
src/Mapster.Tests/WhenRecordRegration.cs Outdated Show resolved Hide resolved
src/Mapster.Tests/WhenRecordRegration.cs Outdated Show resolved Hide resolved
src/Mapster.Tests/WhenRecordRegration.cs Outdated Show resolved Hide resolved
src/Mapster/Utils/ReflectionUtils.cs Outdated Show resolved Hide resolved
@andrerav
Copy link
Contributor

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

@andrerav andrerav changed the base branch from master to development September 25, 2023 21:47
@DocSvartz
Copy link
Author

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

@DocSvartz
Copy link
Author

DocSvartz commented Sep 26, 2023

@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
Copy link
Author

DocSvartz commented Sep 26, 2023

@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
Copy link
Contributor

andrerav commented Sep 26, 2023

@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
Copy link
Author

@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
Copy link
Contributor

Feel free to correct this one :)

@DocSvartz
Copy link
Author

DocSvartz commented Oct 3, 2023

@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
Copy link
Author

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

@andrerav
Copy link
Contributor

@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?

del empty lines
@DocSvartz
Copy link
Author

DocSvartz commented Oct 15, 2023

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 andrerav merged commit 09a0e92 into MapsterMapper:development Oct 15, 2023
1 check passed
@andrerav
Copy link
Contributor

Thank you!

boylec pushed a commit to True-Velocity/Mapster that referenced this pull request Jun 14, 2024
New Record detection - Adapt To Class not create new instance
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants