Skip to content

#3617 support for target this enum mapping. #3616

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.

Sign up for GitHub

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

Jump to bottom
Open
Zegveld wants to merge 1 commit into
base: main
Choose a base branch
from
Open

#3617 support for target this enum mapping. #3616

Zegveld wants to merge 1 commit into from

Conversation

@Zegveld
Copy link
Contributor

@Zegveld Zegveld commented May 28, 2024 •
edited by thunderhook
Loading

fixes #3617

@thunderhook
Copy link
Contributor

thunderhook commented May 29, 2024

Thanks for your work on this @Zegveld!

I checked it out and played with it a bit, and yes, this solves the problem when an enum is used as a target via target = ".".
My mistake, I thought it was the same problem as described in #2421. It just has the same error message but a different cause.

The reproducer with an inherit inverse configuration provided by @filiphr is still not solved.
See 488d951 and just comment out this line:

So I have created another issue for your fix, which then solves #3437: #3617

I have relinked the PR with the new issue.

@thunderhook thunderhook changed the title #2421 support for target this enum mapping. #3617 support for target this enum mapping. May 29, 2024
thunderhook
thunderhook reviewed May 29, 2024
View reviewed changes
Copy link
Contributor

@thunderhook thunderhook left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have enough experience yet and would like @filiphr to take another look at it, I just have a few comments from my side.

processor/src/main/java/org/mapstruct/ap/internal/model/BeanMappingMethod.java
for ( MappingReference targetThis : this.targetThisReferences ) {
PropertyMapping propertyMapping = new PropertyMappingBuilder().mappingContext( ctx )
.sourceMethod( method )
.sourcePropertyName( "test" )
Copy link
Contributor

@thunderhook thunderhook May 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to have no effect in this case, however this seems fishy - maybe just not set sourcePropertyName at all?

processor/src/main/java/org/mapstruct/ap/internal/model/BeanMappingMethod.java
}

public boolean shouldDirectlyReturnOnlyMappingResult() {
return this.returnTypeToConstruct.isEnumType() && this.propertyMappings.size() == 1;
Copy link
Contributor

@thunderhook thunderhook May 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we could come up with more tests. I have no idea what scenarios could be tested, but after checking the coverage report we could provide a test where this.propertyMappings.size() is != 1

Not quite sure though, my coverage report seems to be a bit buggy.

processor/src/test/java/org/mapstruct/ap/test/bugs/_2421/Issue2421Test.java
import org.mapstruct.ap.testutil.ProcessorTest;
import org.mapstruct.ap.testutil.WithClasses;

public class Issue2421Test {
Copy link
Contributor

@thunderhook thunderhook May 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, we would need to change the issue number to 3617 - might aswell add @IssueKey("3617") here.

@filiphr
Copy link
Member

filiphr commented Jun 1, 2024

I need to have a better look at this. However, I don't like the fact that this is done using the BeanMappingMethod. What might make more sense is to do this as part of the enum mapping and support source in @EnumMapping. This way people can still use @ValueMapping and basically it would be a shortcut to writing it by hand.

We currently support string to enum. The solution for this would be to convert the EnumMapping#source (or whatever we name that) to a method that just delegates to another method (which we already have and would be able to handle everything that we already have).

@Zegveld
Copy link
Contributor Author

Zegveld commented Jun 7, 2024

I need to have a better look at this. However, I don't like the fact that this is done using the BeanMappingMethod. What might make more sense is to do this as part of the enum mapping and support source in @EnumMapping. This way people can still use @ValueMapping and basically it would be a shortcut to writing it by hand.

You can actually use @ValueMapping with this solution. This is even present in the current test example.

We currently support string to enum. The solution for this would be to convert the EnumMapping#source (or whatever we name that) to a method that just delegates to another method (which we already have and would be able to handle everything that we already have).

It would indeed be better to introduce a source property on the EnumMapping method, and perhaps throw an error on the current situation which refers to that route. It does feel like the better solution, then what I've currently implemented.

@filiphr
Copy link
Member

filiphr commented Jun 10, 2024

You can actually use @ValueMapping with this solution. This is even present in the current test example.

Indeed, I missed that.

It would indeed be better to introduce a source property on the EnumMapping method, and perhaps throw an error on the current situation which refers to that route. It does feel like the better solution, then what I've currently implemented.

I think that we arleady have an error when using @Mapping with EnumMapping. It used to be supported, but I believe we removed it. Have a look at Message.ENUMMAPPING_REMOVED.

ronodhirSoumik
ronodhirSoumik reviewed Sep 3, 2025
View reviewed changes
processor/src/main/java/org/mapstruct/ap/internal/model/BeanMappingMethod.java
Comment on lines 517 to 518
return !method.getOptions().getSubclassMappings().isEmpty()
&& ( method.getOptions().getBeanMapping().getSubclassExhaustiveStrategy().isAbstractReturnTypeAllowed()
Copy link

@ronodhirSoumik ronodhirSoumik Sep 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just to know-

( method.getOptions().getBeanMapping().getSubclassExhaustiveStrategy().isAbstractReturnTypeAllowed()
                    || isCorrectlySealed() 
                || isCorrectlyDefinedTargetThisForEnumTarget() ); ??

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

Reviewers

@thunderhook thunderhook thunderhook left review comments

+1 more reviewer

@ronodhirSoumik ronodhirSoumik ronodhirSoumik left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Support enum as target this

4 participants

@Zegveld @thunderhook @filiphr @ronodhirSoumik