Missing import when using generic on generic element #1164

Closed
influence160 opened this Issue Mar 30, 2017 · 21 comments

Comments

Projects
None yet
3 participants
@influence160

My class SearchResultDto contains this attribute
private List<GenericRessource<CriteriaDto>> criterias;

i have a mapper like this

@Mapper(componentModel = "spring", uses = {CriteriaWsMapper.class, .....(other classes)})
public abstract class SearchResultWsMapper extends GenericWsMapper<SearchResult, SearchResultDto> {
....
}

this mapper works fine with mapstructure 1.0.0.Final and generates this file

package mydomain.ws.mappers;

import mydomain.domain.entities.SearchResult;
import mydomain.ws.dto.SearchResultDto;
import javax.annotation.Generated;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.stereotype.Component;

@Generated(
    value = "org.mapstruct.ap.MappingProcessor",
    date = "2017-03-30T16:27:59+0200",
    comments = "version: 1.0.0.Final, compiler: javac, environment: Java 1.8.0_121 (Oracle Corporation)"
)
@Component
public class SearchResultWsMapperImpl extends SearchResultWsMapper {

    @Autowired
    private PieceWsMapper pieceWsMapper;
    @Autowired
    private CapaciteWsMapper capaciteWsMapper;
    @Autowired
    private ArgumentaireHuileWsMapper argumentaireHuileWsMapper;

    @Override
    public SearchResultDto toDto(SearchResult entity) {
        if ( entity == null ) {
            return null;
        }

        SearchResultDto searchResultDto = new SearchResultDto();

        searchResultDto.setCriterias( mapCriterias( entity.getCriterias() ) );
        searchResultDto.setGroupePieces( mapGroup( entity.getGroupePieces() ) );
        searchResultDto.setAfficherAvertissement( entity.isAfficherAvertissement() );
        searchResultDto.setPiecesAssociees( pieceWsMapper.toDtos( entity.getPiecesAssociees() ) );
        searchResultDto.setCapaciteVariantes( capaciteWsMapper.toDtos( entity.getCapaciteVariantes() ) );
        searchResultDto.setArgumentairesHuiles( argumentaireHuileWsMapper.toDtos( entity.getArgumentairesHuiles() ) );

        return searchResultDto;
    }
}

when i try to migrate to 1.1.0.Final the generated file is :

package mydomain.ws.mappers;

import mydomain.domain.entities.SearchResult;
import mydomain.ws.GenericRessource;
import mydomain.ws.dto.ArgumentaireHuileDto;
import mydomain.ws.dto.CapaciteVarianteDto;
import mydomain.ws.dto.GroupePieceDto;
import mydomain.ws.dto.PieceDto;
import mydomain.ws.dto.SearchResultDto;
import java.util.List;
import javax.annotation.Generated;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.stereotype.Component;

@Generated(
    value = "org.mapstruct.ap.MappingProcessor",
    date = "2017-03-30T16:29:18+0200",
    comments = "version: 1.1.0.Final, compiler: javac, environment: Java 1.8.0_121 (Oracle Corporation)"
)
@Component
public class SearchResultWsMapperImpl extends SearchResultWsMapper {

    @Autowired
    private PieceWsMapper pieceWsMapper;
    @Autowired
    private CapaciteWsMapper capaciteWsMapper;
    @Autowired
    private ArgumentaireHuileWsMapper argumentaireHuileWsMapper;

    @Override
    public SearchResultDto toDto(SearchResult entity) {
        if ( entity == null ) {
            return null;
        }

        SearchResultDto searchResultDto = new SearchResultDto();

        List<GenericRessource<CriteriaDto>> list = mapCriterias( entity.getCriterias() );
        if ( list != null ) {
            searchResultDto.setCriterias( list );
        }
        List<GroupePieceDto> list_ = mapGroup( entity.getGroupePieces() );
        if ( list_ != null ) {
            searchResultDto.setGroupePieces( list_ );
        }
        searchResultDto.setAfficherAvertissement( entity.isAfficherAvertissement() );
        List<PieceDto> list__ = pieceWsMapper.toDtos( entity.getPiecesAssociees() );
        if ( list__ != null ) {
            searchResultDto.setPiecesAssociees( list__ );
        }
        List<CapaciteVarianteDto> list___ = capaciteWsMapper.toDtos( entity.getCapaciteVariantes() );
        if ( list___ != null ) {
            searchResultDto.setCapaciteVariantes( list___ );
        }
        List<ArgumentaireHuileDto> list____ = argumentaireHuileWsMapper.toDtos( entity.getArgumentairesHuiles() );
        if ( list____ != null ) {
            searchResultDto.setArgumentairesHuiles( list____ );
        }

        return searchResultDto;
    }
}

it does not compile and there is a missing import for the class CriteriaDto

@filiphr

This comment has been minimized.

Show comment
Hide comment
@filiphr

filiphr Mar 30, 2017

Member

@influence160 Thanks for your report. Just to make sure we are on the same page the mapCriterias method is not an abstract method it is a handwritten method in your inheritance tree? I am asking because if it is a handwritten method, I think that we should not do a null check at all. @sjaakd?

I'll look into the issue with the import

Member

filiphr commented Mar 30, 2017

@influence160 Thanks for your report. Just to make sure we are on the same page the mapCriterias method is not an abstract method it is a handwritten method in your inheritance tree? I am asking because if it is a handwritten method, I think that we should not do a null check at all. @sjaakd?

I'll look into the issue with the import

@filiphr

This comment has been minimized.

Show comment
Hide comment
@filiphr

filiphr Mar 30, 2017

Member

I forgot to mention, a workaround would be to add the missing types to the @Mapper(imports) property.

Member

filiphr commented Mar 30, 2017

I forgot to mention, a workaround would be to add the missing types to the @Mapper(imports) property.

@influence160

This comment has been minimized.

Show comment
Hide comment
@influence160

influence160 Mar 31, 2017

@filiphr yes the method mapCriterias is handwritten in the class SearchResultWsMapper and all the code compile and works fine with 1.0.0.Final
this is the method

public List<GenericRessource> mapCriterias(List criterias) {
List<GenericRessource> criteriaDto = Collections.EMPTY_LIST;
criteriaDto = criteriaMapper.toDtoRessource(
criterias,
(d) -> d.addLink(new Link(this.buildURI(d.getRessource())).withRel("next"))
);
return criteriaDto;
}

@filiphr yes the method mapCriterias is handwritten in the class SearchResultWsMapper and all the code compile and works fine with 1.0.0.Final
this is the method

public List<GenericRessource> mapCriterias(List criterias) {
List<GenericRessource> criteriaDto = Collections.EMPTY_LIST;
criteriaDto = criteriaMapper.toDtoRessource(
criterias,
(d) -> d.addLink(new Link(this.buildURI(d.getRessource())).withRel("next"))
);
return criteriaDto;
}

filiphr added a commit to filiphr/mapstruct that referenced this issue Mar 31, 2017

@filiphr

This comment has been minimized.

Show comment
Hide comment
@filiphr

filiphr Mar 31, 2017

Member

@influence160 Yes it works in 1.0.0.Final because the way we set the code there has been changed. That is why I am asking @sjaakd if this code is correct and it is like it was planned.

I am not sure if you managed to solve it with the workaround. I was talking about this. I created a PR #1165 that should fix the problem. Can you perhaps try it out?

Member

filiphr commented Mar 31, 2017

@influence160 Yes it works in 1.0.0.Final because the way we set the code there has been changed. That is why I am asking @sjaakd if this code is correct and it is like it was planned.

I am not sure if you managed to solve it with the workaround. I was talking about this. I created a PR #1165 that should fix the problem. Can you perhaps try it out?

@filiphr filiphr closed this in #1165 Mar 31, 2017

filiphr added a commit that referenced this issue Mar 31, 2017

@filiphr

This comment has been minimized.

Show comment
Hide comment
@filiphr

filiphr Mar 31, 2017

Member

@influence160 the fix has been merged into master. If you want you can try it out directly.

Member

filiphr commented Mar 31, 2017

@influence160 the fix has been merged into master. If you want you can try it out directly.

@sjaakd

This comment has been minimized.

Show comment
Hide comment
@sjaakd

sjaakd Apr 1, 2017

Contributor

@influence160 : can you check the latest version (Beta2). There have been some changes in this area (and lots of discussions). I need to dig deeper to figure out when / why this was introduced.

Contributor

sjaakd commented Apr 1, 2017

@influence160 : can you check the latest version (Beta2). There have been some changes in this area (and lots of discussions). I need to dig deeper to figure out when / why this was introduced.

@sjaakd

This comment has been minimized.

Show comment
Hide comment
@sjaakd

sjaakd Apr 1, 2017

Contributor

@filiphr .. I checked and it seems we do add a null check also in Beta2. I'm still investigating why. Why did you close this issue?

Contributor

sjaakd commented Apr 1, 2017

@filiphr .. I checked and it seems we do add a null check also in Beta2. I'm still investigating why. Why did you close this issue?

@filiphr

This comment has been minimized.

Show comment
Hide comment
@filiphr

filiphr Apr 1, 2017

Member

It was closed via the PR. I think that there are 2 problems with this one:

  • A missing import was missing for aub generic typeas (Fixed with #1165)
  • The null check was strange to me so I commented that as well.

@sjaakd We can either open a new one or reopen this one foe the null check. I am fine with both

Member

filiphr commented Apr 1, 2017

It was closed via the PR. I think that there are 2 problems with this one:

  • A missing import was missing for aub generic typeas (Fixed with #1165)
  • The null check was strange to me so I commented that as well.

@sjaakd We can either open a new one or reopen this one foe the null check. I am fine with both

@sjaakd sjaakd reopened this Apr 1, 2017

@sjaakd

This comment has been minimized.

Show comment
Hide comment
@sjaakd

sjaakd Apr 1, 2017

Contributor

Lets reopen it 😄

Contributor

sjaakd commented Apr 1, 2017

Lets reopen it 😄

@sjaakd

This comment has been minimized.

Show comment
Hide comment
@sjaakd

sjaakd Apr 1, 2017

Contributor

@filiphr the issue is introduced when SetterWrapperForCollectionsAndMaps was refactored. I think I somehow decided to always do a null check after the introduction of nullCheckLocalVarName in the WrapperForCollectionsAndMaps whereas in the past we did this only for conversions or direct and left it up to the designer of the mapping method to do this correctly.

Perhaps its not always needed to create a local variable for the result before we assign in case of collections. So we should refactor the nullCheckLocalVarName into something like intermediateLocalVarName and intermediateLocalVarType to express this. Also the handleLocalVarNullCheck needs to be refactored into handleIntermediateLocalVar and a branch could be added here if there is no intermediateLocalVar.

Note the base class is shared between 2 implementors: SetterWrapperForCollections, GetterWrapperForCollections. In the SetterWrapperForCollections. So we should be careful changing base behavior. Perhaps we should split the SetterWrapperForCollections into an update variant and a create variant. That might make life more easy.

Then we should go through all scenarios and see whether we need a null check or not. In the past, we decided to leave the result of a mapping method up to the implementer of that method (handwritten or generated). Note: you might in that case end up with problems in addAll.

WDYT?

Contributor

sjaakd commented Apr 1, 2017

@filiphr the issue is introduced when SetterWrapperForCollectionsAndMaps was refactored. I think I somehow decided to always do a null check after the introduction of nullCheckLocalVarName in the WrapperForCollectionsAndMaps whereas in the past we did this only for conversions or direct and left it up to the designer of the mapping method to do this correctly.

Perhaps its not always needed to create a local variable for the result before we assign in case of collections. So we should refactor the nullCheckLocalVarName into something like intermediateLocalVarName and intermediateLocalVarType to express this. Also the handleLocalVarNullCheck needs to be refactored into handleIntermediateLocalVar and a branch could be added here if there is no intermediateLocalVar.

Note the base class is shared between 2 implementors: SetterWrapperForCollections, GetterWrapperForCollections. In the SetterWrapperForCollections. So we should be careful changing base behavior. Perhaps we should split the SetterWrapperForCollections into an update variant and a create variant. That might make life more easy.

Then we should go through all scenarios and see whether we need a null check or not. In the past, we decided to leave the result of a mapping method up to the implementer of that method (handwritten or generated). Note: you might in that case end up with problems in addAll.

WDYT?

@filiphr

This comment has been minimized.

Show comment
Hide comment
@filiphr

filiphr Apr 1, 2017

Member

@sjaakd I agree with you. Splitting the SetterWrapperForCollections into 2 model variants one for update and another one for normal is fine for me. This would make things a bit easier.

Regarding the problem with addAll, I think that for GettersWrapperForCollection we only need to do a null check before the addAll and always call clear. I remember that there was one issue where someone was having a problem with the direct setting of null and the JPA proxies not being cleared correctly. This problem only occurs with GetterWrapper(s) right?

Member

filiphr commented Apr 1, 2017

@sjaakd I agree with you. Splitting the SetterWrapperForCollections into 2 model variants one for update and another one for normal is fine for me. This would make things a bit easier.

Regarding the problem with addAll, I think that for GettersWrapperForCollection we only need to do a null check before the addAll and always call clear. I remember that there was one issue where someone was having a problem with the direct setting of null and the JPA proxies not being cleared correctly. This problem only occurs with GetterWrapper(s) right?

@sjaakd

This comment has been minimized.

Show comment
Hide comment
@sjaakd

sjaakd Apr 2, 2017

Contributor

This problem only occurs with GetterWrapper(s) right?

No. its a bit confusing, but the GettersWrapperForCollections is used when we have only a getter available to do the job. The SettersWrapperForCollection is a create scenario and in an update scenario. The update in the SettersWrapperForCollection scenario looks very similar to the scenario where only a getter is available (in the GettersWrapperForCollections. This has grown over time, but perhaps its nicer to split it along functional lines.

I spent a lot of time on it when handling #913. There are a lot of test cases available there. Can you take a look at it?

Contributor

sjaakd commented Apr 2, 2017

This problem only occurs with GetterWrapper(s) right?

No. its a bit confusing, but the GettersWrapperForCollections is used when we have only a getter available to do the job. The SettersWrapperForCollection is a create scenario and in an update scenario. The update in the SettersWrapperForCollection scenario looks very similar to the scenario where only a getter is available (in the GettersWrapperForCollections. This has grown over time, but perhaps its nicer to split it along functional lines.

I spent a lot of time on it when handling #913. There are a lot of test cases available there. Can you take a look at it?

@filiphr

This comment has been minimized.

Show comment
Hide comment
@filiphr

filiphr Apr 2, 2017

Member

I spent a lot of time on it when handling #913. There are a lot of test cases available there. Can you take a look at it?

Yes sure, I can have a look at it.

I have an idea how the generated code should look like. However, I think that we will have to agree on a strategy here. I would even propose that we always return a default Iterable, Array or Map in our internal methods and we expect the users to return a default value in their methods and then we won't need to have a null check before calling addAll. Basically the nullValueMappingStrategy will not apply to Iterables, Arrays or Maps (this might be too extreme though).

Member

filiphr commented Apr 2, 2017

I spent a lot of time on it when handling #913. There are a lot of test cases available there. Can you take a look at it?

Yes sure, I can have a look at it.

I have an idea how the generated code should look like. However, I think that we will have to agree on a strategy here. I would even propose that we always return a default Iterable, Array or Map in our internal methods and we expect the users to return a default value in their methods and then we won't need to have a null check before calling addAll. Basically the nullValueMappingStrategy will not apply to Iterables, Arrays or Maps (this might be too extreme though).

@sjaakd

This comment has been minimized.

Show comment
Hide comment
@sjaakd

sjaakd Apr 2, 2017

Contributor

The current strategy is:

  1. Do not check when the input for the target collections is the result of a mapping method
  2. NullCheck if the input for the target collection is the result of a conversion
  3. Always check when the target accessor is a getter.
  4. I probably forget one-or-two rules.

I think rule 1. is violated in this problem. IHMO I think we should stick our solution to that and do not distinguish between calling hand-written or forged methods.

Now, solution wise: I would split the behavior clearly into a situation that we use the getter as target accessor and in a situation that we use the setter as accessor. That means that that specific piece of the functionality can be removed (with care) from SetterWrapperForCollections and be only present in GetterWrapperForCollections. This means we need to bring some checks to the java PropertyMapping code that are now in the templates.

I would take the #913 test cases as base for departure and only fix this issue (leave the remainder untouched). There have been a lot of discussions on this in the past on when to do a:

  1. source null check
  2. intermediate null check (result of conversion and mapping, see my explanation above)
  3. target null check.

Its a bit of a mine field 😄. So some fixtures would be good to add to the test cases. Also, we do need some design documentation on our decisions. They keep on popping up.

Having said that. I do think its time that a fresh mind has a look at this (and understands what I've been doing with this in the past 😄 ).. I can help out if you want.

Contributor

sjaakd commented Apr 2, 2017

The current strategy is:

  1. Do not check when the input for the target collections is the result of a mapping method
  2. NullCheck if the input for the target collection is the result of a conversion
  3. Always check when the target accessor is a getter.
  4. I probably forget one-or-two rules.

I think rule 1. is violated in this problem. IHMO I think we should stick our solution to that and do not distinguish between calling hand-written or forged methods.

Now, solution wise: I would split the behavior clearly into a situation that we use the getter as target accessor and in a situation that we use the setter as accessor. That means that that specific piece of the functionality can be removed (with care) from SetterWrapperForCollections and be only present in GetterWrapperForCollections. This means we need to bring some checks to the java PropertyMapping code that are now in the templates.

I would take the #913 test cases as base for departure and only fix this issue (leave the remainder untouched). There have been a lot of discussions on this in the past on when to do a:

  1. source null check
  2. intermediate null check (result of conversion and mapping, see my explanation above)
  3. target null check.

Its a bit of a mine field 😄. So some fixtures would be good to add to the test cases. Also, we do need some design documentation on our decisions. They keep on popping up.

Having said that. I do think its time that a fresh mind has a look at this (and understands what I've been doing with this in the past 😄 ).. I can help out if you want.

@filiphr

This comment has been minimized.

Show comment
Hide comment
@filiphr

filiphr Apr 10, 2017

Member

@sjaakd I had a look at the SetterWrapperForCollections and the GetterWrapperForCollections, everywhere it is correctly generated with the exception of the mapped assignment.

There is nothing we need to do for the GetterWrapperForCollections.

As for the SetterWrapperForCollections we do the following:

  1. Existing instance mapping and not CollectionMappingStrategy#TARGET_IMMUTABLE - everything is correctly done and we do clear correctly
  2. For the rest we always do a null check. I think that this can be split into 2 parts:
    2.1 Direct assignment (we need to do new ArrayList<>(source.getList()) for example) or NullValueCheckStrategy#ALWAYS - we do a null / presence check
    2.2 Assignment via internal mapping method or user defined method - I don't think we need to care whether we set null or not.

I have already done something in that implements my last 2 points. However, I am stuck with the imports. The decision whether or not we do existingInstanceMapping is passed to the wrapper during the writing of the generated code by FreeMarker. This either leads to a Checkstyle error because an import is not used or a compilation failure because an import was not added. I have 2 suggestions to solve this:

  • Add a boolean to the getImportTypes() in the Assignment so we can correctly add the imports
  • Pass the information whether the assignment is in an update method (existingInstanceMapping) or not to the SetterWrapperForCollections

In my opinion the 2nd approach is better and will have less changes. WDYT?

Member

filiphr commented Apr 10, 2017

@sjaakd I had a look at the SetterWrapperForCollections and the GetterWrapperForCollections, everywhere it is correctly generated with the exception of the mapped assignment.

There is nothing we need to do for the GetterWrapperForCollections.

As for the SetterWrapperForCollections we do the following:

  1. Existing instance mapping and not CollectionMappingStrategy#TARGET_IMMUTABLE - everything is correctly done and we do clear correctly
  2. For the rest we always do a null check. I think that this can be split into 2 parts:
    2.1 Direct assignment (we need to do new ArrayList<>(source.getList()) for example) or NullValueCheckStrategy#ALWAYS - we do a null / presence check
    2.2 Assignment via internal mapping method or user defined method - I don't think we need to care whether we set null or not.

I have already done something in that implements my last 2 points. However, I am stuck with the imports. The decision whether or not we do existingInstanceMapping is passed to the wrapper during the writing of the generated code by FreeMarker. This either leads to a Checkstyle error because an import is not used or a compilation failure because an import was not added. I have 2 suggestions to solve this:

  • Add a boolean to the getImportTypes() in the Assignment so we can correctly add the imports
  • Pass the information whether the assignment is in an update method (existingInstanceMapping) or not to the SetterWrapperForCollections

In my opinion the 2nd approach is better and will have less changes. WDYT?

@filiphr

This comment has been minimized.

Show comment
Hide comment
@filiphr

filiphr Apr 10, 2017

Member

@sjaakd btw, is existingInstanceMapping needs only for the SetterWrapperForCollections and the GetterWrapperForCollections? From what I can understand from the code it seems like it

Member

filiphr commented Apr 10, 2017

@sjaakd btw, is existingInstanceMapping needs only for the SetterWrapperForCollections and the GetterWrapperForCollections? From what I can understand from the code it seems like it

filiphr added a commit to filiphr/mapstruct that referenced this issue Apr 10, 2017

#1164 When using set in the SetterWrapperForCollections do null chec…
…k only

for direct assignment or when the NullValueCheckStrategy is ALWAYS
@sjaakd

This comment has been minimized.

Show comment
Hide comment
@sjaakd

sjaakd Apr 10, 2017

Contributor

@filiphr we can fix it in the SetterWrapperForCollections as you did.But to be honest, I'm not in favor of any one of the options. We should select the proper Wrapper based on what we have available on the java side and not bring to much knowledge (and also via 2 paths) to the templates.

So perhaps we should go for a more definite solution. Our unit test is good (you've extended on that a bit more in the PR.). So it should be possible to refactor. BTW: Did you checkout my last comment (8 days ago)?

Anyway.. I can also understand if you want to go for a quick solution and just fix the problem. We can also do it later. In that case, take the boolean.

Contributor

sjaakd commented Apr 10, 2017

@filiphr we can fix it in the SetterWrapperForCollections as you did.But to be honest, I'm not in favor of any one of the options. We should select the proper Wrapper based on what we have available on the java side and not bring to much knowledge (and also via 2 paths) to the templates.

So perhaps we should go for a more definite solution. Our unit test is good (you've extended on that a bit more in the PR.). So it should be possible to refactor. BTW: Did you checkout my last comment (8 days ago)?

Anyway.. I can also understand if you want to go for a quick solution and just fix the problem. We can also do it later. In that case, take the boolean.

@sjaakd

This comment has been minimized.

Show comment
Hide comment
@sjaakd

sjaakd Apr 10, 2017

Contributor

Anyway.. I can also understand if you want to go for a quick solution and just fix the problem. We can also do it later. In that case, take the boolean.

Or alternatively, I can push also some of my ideas to your branch later this week

Contributor

sjaakd commented Apr 10, 2017

Anyway.. I can also understand if you want to go for a quick solution and just fix the problem. We can also do it later. In that case, take the boolean.

Or alternatively, I can push also some of my ideas to your branch later this week

@filiphr

This comment has been minimized.

Show comment
Hide comment
@filiphr

filiphr Apr 11, 2017

Member

Or alternatively, I can push also some of my ideas to your branch later this week

By all means, I am not in a hurry for any solution. I just didn't want to change a lot of things.

So perhaps we should go for a more definite solution. Our unit test is good (you've extended on that a bit more in the PR.). So it should be possible to refactor. BTW: Did you checkout my last comment (8 days ago)?

Yes I had a look at it.
We can also create a wrapper for each of the branches in the template, one for existing instance mapping and not target immutable, one for direct assignment or always null check and one that does the default. Is this what you are suggesting?

I also thing that separating the models will make it easier to do it correctly and also will move the logic of selecting a wrapper to the java code. I am going away for the weekend, and I am not sure how much time I'll have. In any case, I am back Monday early and will have more time to make changes then.

Member

filiphr commented Apr 11, 2017

Or alternatively, I can push also some of my ideas to your branch later this week

By all means, I am not in a hurry for any solution. I just didn't want to change a lot of things.

So perhaps we should go for a more definite solution. Our unit test is good (you've extended on that a bit more in the PR.). So it should be possible to refactor. BTW: Did you checkout my last comment (8 days ago)?

Yes I had a look at it.
We can also create a wrapper for each of the branches in the template, one for existing instance mapping and not target immutable, one for direct assignment or always null check and one that does the default. Is this what you are suggesting?

I also thing that separating the models will make it easier to do it correctly and also will move the logic of selecting a wrapper to the java code. I am going away for the weekend, and I am not sure how much time I'll have. In any case, I am back Monday early and will have more time to make changes then.

@sjaakd

This comment has been minimized.

Show comment
Hide comment
@sjaakd

sjaakd Apr 12, 2017

Contributor

Is this what you are suggesting?

That's more or less the idea.. I'm a bit constraint in time myself (busy at work). But I can have a shot at this myself as well. First I've got to do my tax reporting this weekend 😭

Contributor

sjaakd commented Apr 12, 2017

Is this what you are suggesting?

That's more or less the idea.. I'm a bit constraint in time myself (busy at work). But I can have a shot at this myself as well. First I've got to do my tax reporting this weekend 😭

filiphr added a commit to filiphr/mapstruct that referenced this issue Apr 18, 2017

#1164 Split the SetterWrapperForCollections into multiple models:
* SetterWrapperForCollectionsAndMaps - Does a simple assignment without doing any null checks
* SetterWrapperForCollectionsAndMapsWithNullCheck - Does an assignment that does a null check before assignment and takes direct assignment into consideration
* ExistingInstanceSetterWrapperForCollectionsAndMaps - Used for wrapping an assignment when the method is an update method

filiphr added a commit to filiphr/mapstruct that referenced this issue Apr 25, 2017

#1164 Split the SetterWrapperForCollections into multiple models:
* SetterWrapperForCollectionsAndMaps - Does a simple assignment without doing any null checks
* SetterWrapperForCollectionsAndMapsWithNullCheck - Does an assignment that does a null check before assignment and takes direct assignment into consideration
* ExistingInstanceSetterWrapperForCollectionsAndMaps - Used for wrapping an assignment when the method is an update method

Additionally don't do local var assignment if there are presence checkers

filiphr added a commit to filiphr/mapstruct that referenced this issue Apr 25, 2017

#1164 Split the SetterWrapperForCollections into multiple models:
* SetterWrapperForCollectionsAndMaps - Does a simple assignment without doing any null checks
* SetterWrapperForCollectionsAndMapsWithNullCheck - Does an assignment that does a null check before assignment and takes direct assignment into consideration
* ExistingInstanceSetterWrapperForCollectionsAndMaps - Used for wrapping an assignment when the method is an update method

Additionally don't do local var assignment if there are presence checkers

filiphr added a commit that referenced this issue Apr 25, 2017

#1164 Split the SetterWrapperForCollections into multiple models:
* SetterWrapperForCollectionsAndMaps - Does a simple assignment without doing any null checks
* SetterWrapperForCollectionsAndMapsWithNullCheck - Does an assignment that does a null check before assignment and takes direct assignment into consideration
* ExistingInstanceSetterWrapperForCollectionsAndMaps - Used for wrapping an assignment when the method is an update method

Additionally don't do local var assignment if there are presence checkers
@filiphr

This comment has been minimized.

Show comment
Hide comment
@filiphr

filiphr Apr 25, 2017

Member

The second part of this issue has been fixed as part of the #1175 PR

Member

filiphr commented Apr 25, 2017

The second part of this issue has been fixed as part of the #1175 PR

@filiphr filiphr closed this Apr 25, 2017

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