Skip to content

#3129: detect collection adders using Iterable setter/getter #3130

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
Closed
sranka wants to merge 2 commits into from
Closed

#3129: detect collection adders using Iterable setter/getter #3130

sranka wants to merge 2 commits into from

Conversation

@sranka
Copy link

@sranka sranka commented Jan 2, 2023 •
edited
Loading

Closes #3129

The code herein fixes the detection of collection adders to use a more generic Iterable as a setter argument (or a getter return value). As consequence, a lombok generated builder setter (fluent) method:

public LombokImmutablePathBuilder edges(Iterable<? extends String> edges) {
  // ...
  return this;
}

is now also considered as a candidate for searching of an associated adder:

public LombokImmutablePathBuilder addEdge(String edge) {
  // ...
  return this;
}

Using the adders directly has a performance benefit when using lombok builders. The mapper implementation then does not need to create intermediary ArrayList values to map collection properties. For example, in place of this code (before this PR):

...
ActionRule.ActionRuleBuilder actionRule = ActionRule.builder();
if (actionRules.getCostList() != null) {
    actionRule.costs(this.costListToCostIterable(actionRules.getCostList()));
}
...
protected Iterable<Cost> costListToCostIterable(List<b.Cost> list) {
    if (list == null) {
        return null;
    } else {
        ArrayList<Cost> iterable = new ArrayList(list.size());
        Iterator var3 = list.iterator();

        while(var3.hasNext()) {
            com.goeuro.search2.model.proto.Cost cost = (com.goeuro.search2.model.proto.Cost)var3.next();
            iterable.add(this.costMapper.map(cost));
        }

        return iterable;
    }
}

, the new mapper implementation is then optimal (this PR):

ActionRule.ActionRuleBuilder actionRule = ActionRule.builder();
if (actionRules.getCostList() != null) {
    Iterator var3 = actionRules.getCostList().iterator();

    while(var3.hasNext()) {
        Cost costList = (Cost)var3.next();
        actionRule.addCost(this.costMapper.map(costList));
    }
}

@sranka sranka marked this pull request as ready for review January 2, 2023 09:08
@sranka sranka changed the title fix: detect collection adders using Iterable setter/getter #3129: detect collection adders using Iterable setter/getter Jan 2, 2023
filiphr
filiphr requested changes Mar 17, 2023
View reviewed changes
Copy link
Member

@filiphr filiphr left a comment

Choose a reason for hiding this comment

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

I've left a comment on the LombokBuilderAccessorNamingStrategy. I think that the solution for this is fine (i.e. support adder methods for iterables). However, I would like to decouple this from Lombok entirely.

Can you also simplify the test classes a bit. This can be easily reproduced by having a class that looks like:

public class ImnutablePath {
    private final List<String> edges = new ArrayList<>();

    public Iterable<String> getEdges() {
        return edges;
    }

    public void setEdges(Iterable<String> edges) {
        this.edges.clear();
        for(String edge: edges) {
            this.edges.add(edge.toUpperCase());
        }
    }

    public void addEdge(String edge) {
        this.edges.add(edge);
    }
}

The tests should also be moved in the org.mapstruct.ap.test.collection.adder.AdderTest

.../src/test/java/org/mapstruct/ap/test/builder/lombok/LombokBuilderAccessorNamingStrategy.java
Copy link
Member

@filiphr filiphr Mar 17, 2023

Choose a reason for hiding this comment

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

I don't understand the use of this class. There is an easy workaround for this if you use @Singular("add<singular"). If we are going in the territory of supporting @Singular properly then we need to also support something like @Singular("custom").

I would prefer that we do not go in that direction and only perhaps support adders for iterables as well.

processor/src/test/java/org/mapstruct/ap/test/builder/lombok/PathMapper.java
Comment on lines +16 to +18
@Mappings({
@Mapping(target = "edges", source = "edges"),
})
Copy link
Member

@filiphr filiphr Mar 17, 2023

Choose a reason for hiding this comment

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

This is obsolete. MapStruct is going to map this implicitly

@filiphr filiphr mentioned this pull request Apr 22, 2023
Merged
@filiphr
Copy link
Member

filiphr commented Apr 22, 2023

@sranka you don't need to do anything on this PR. I believe that PR #3166 is going to solve the problem

@sranka
Copy link
Author

sranka commented Apr 23, 2023

@filiphr yeah, it looks similar, closing this PR

@sranka sranka closed this Apr 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@filiphr filiphr filiphr requested changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

CollectionMappingStrategy.ADDER_PREFERRED is not supported for Iterables

2 participants

@sranka @filiphr