Skip to content

#3652 Inverse Inheritance possible for ignore-mappings without source #3675

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

Merged
filiphr merged 4 commits into from
Aug 18, 2024
Merged

#3652 Inverse Inheritance possible for ignore-mappings without source #3675

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
30c395b
#3652 Inverse Inheritance possible for ignore mappings without source
Hypnagokali Aug 17, 2024
8667098
#3652 Remove relics of previous version
Hypnagokali Aug 17, 2024
bab2963
#3652 Adjust Javadoc comment
Hypnagokali Aug 18, 2024
8296405
Add changelog
filiphr Aug 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions NEXT_RELEASE_CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@

### Bugs

* Inverse Inheritance Strategy not working for ignored mappings only with target (#3652)

### Documentation

### Build
Expand Down
5 changes: 2 additions & 3 deletions processor/src/main/java/org/mapstruct/ap/internal/model/source/MappingOptions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -479,13 +479,12 @@ public MappingControl getMappingControl(ElementUtils elementUtils) {
}

/**
* mapping can only be inversed if the source was not a constant nor an expression nor a nested property
* and the mapping is not a 'target-source-ignore' mapping

@zyberzebra zyberzebra Aug 18, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm still curious why this was explicitly stated here. I tried looking at the history of this method, but did not find a hint

@filiphr filiphr Aug 18, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I was curious too @zyberzebra. This looks like an ancient thing. The MappingOptions was called Mapping at some point. The rename was done in 7bee121.

The line isIgnored && sourceName == null was added first in 4d8bc29. And looking at that commit it seems like the logic was not ported properly, because prior to that we had

        // mapping can only be reversed if the source was not a constant nor an expression nor a nested property
        if ( constant != null || javaExpression != null ) {
            return null;
        }

        // should only ignore a property when 1) there is a sourceName defined or 2) there's a name match
        if ( isIgnored ) {
            if ( sourceName == null && !hasPropertyInReverseMethod( targetName, method ) ) {
                return null;
            }
        }

and after that we had

        // mapping can only be reversed if the source was not a constant nor an expression nor a nested property
        // and the mapping is not a 'target-source-ignore' mapping
        if ( constant != null || javaExpression != null || ( isIgnored && sourceName == null ) ) {
            return null;
        }

Things were quite different back then. We are now handling the target later and differently to back then, so the fix done by @Hypnagokali looks like spot on.

* Mapping can only be inversed if the source was not a constant nor an expression
*
* @return true when the above applies
*/
public boolean canInverse() {
return constant == null && javaExpression == null && !( isIgnored && sourceName == null );
return constant == null && javaExpression == null;
}

public MappingOptions copyForInverseInheritance(SourceMethod templateMethod,
Expand Down
30 changes: 30 additions & 0 deletions processor/src/test/java/org/mapstruct/ap/test/bugs/_3652/Bar.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
/*
* Copyright MapStruct Authors.
*
* Licensed under the Apache License version 2.0, available at http://www.apache.org/licenses/LICENSE-2.0
*/

package org.mapstruct.ap.test.bugs._3652;

public class Bar {

private int secret;
private int doesNotExistInFoo;

public int getSecret() {
return secret;
}

public void setSecret(int secret) {
this.secret = secret;
}

public int getDoesNotExistInFoo() {
return doesNotExistInFoo;
}

public void setDoesNotExistInFoo(int doesNotExistInFoo) {
this.doesNotExistInFoo = doesNotExistInFoo;
}

}
21 changes: 21 additions & 0 deletions processor/src/test/java/org/mapstruct/ap/test/bugs/_3652/Foo.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
/*
* Copyright MapStruct Authors.
*
* Licensed under the Apache License version 2.0, available at http://www.apache.org/licenses/LICENSE-2.0
*/

package org.mapstruct.ap.test.bugs._3652;

public class Foo {

private int secret;

public int getSecret() {
return secret;
}

public void setSecret(int secret) {
this.secret = secret;
}

}
24 changes: 24 additions & 0 deletions processor/src/test/java/org/mapstruct/ap/test/bugs/_3652/FooBarConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
/*
* Copyright MapStruct Authors.
*
* Licensed under the Apache License version 2.0, available at http://www.apache.org/licenses/LICENSE-2.0
*/

package org.mapstruct.ap.test.bugs._3652;

import org.mapstruct.InheritInverseConfiguration;
import org.mapstruct.MapperConfig;
import org.mapstruct.Mapping;
import org.mapstruct.MappingInheritanceStrategy;

@MapperConfig(mappingInheritanceStrategy = MappingInheritanceStrategy.AUTO_INHERIT_ALL_FROM_CONFIG)
public interface FooBarConfig {

@Mapping(target = "doesNotExistInFoo", ignore = true)
@Mapping(target = "secret", ignore = true)
Bar toBar(Foo foo);

@InheritInverseConfiguration(name = "toBar")
Foo toFoo(Bar bar);

}
21 changes: 21 additions & 0 deletions processor/src/test/java/org/mapstruct/ap/test/bugs/_3652/FooBarMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
/*
* Copyright MapStruct Authors.
*
* Licensed under the Apache License version 2.0, available at http://www.apache.org/licenses/LICENSE-2.0
*/

package org.mapstruct.ap.test.bugs._3652;

import org.mapstruct.Mapper;
import org.mapstruct.factory.Mappers;

@Mapper(config = FooBarConfig.class)
public interface FooBarMapper {

FooBarMapper INSTANCE = Mappers.getMapper( FooBarMapper.class );

Bar toBar(Foo foo);

Foo toFoo(Bar bar);

}
35 changes: 35 additions & 0 deletions processor/src/test/java/org/mapstruct/ap/test/bugs/_3652/Issue3652Test.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
/*
* Copyright MapStruct Authors.
*
* Licensed under the Apache License version 2.0, available at http://www.apache.org/licenses/LICENSE-2.0
*/

package org.mapstruct.ap.test.bugs._3652;

import org.mapstruct.ap.testutil.IssueKey;
import org.mapstruct.ap.testutil.ProcessorTest;
import org.mapstruct.ap.testutil.WithClasses;

import static org.assertj.core.api.Assertions.assertThat;

@IssueKey("3652")
public class Issue3652Test {

@WithClasses({
Bar.class,
Foo.class,
FooBarConfig.class,
FooBarMapper.class,
})
@ProcessorTest
void ignoreMappingsWithoutSourceShouldBeInvertible() {
Bar bar = new Bar();
bar.setSecret( 123 );
bar.setDoesNotExistInFoo( 6 );

Foo foo = FooBarMapper.INSTANCE.toFoo( bar );

assertThat( foo.getSecret() ).isEqualTo( 0 );
}

}