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

Inconsistent use of clearing collections in case of update methods. #1142

Closed
sjaakd opened this issue Mar 15, 2017 · 10 comments
Closed

Inconsistent use of clearing collections in case of update methods. #1142

sjaakd opened this issue Mar 15, 2017 · 10 comments

Comments

@sjaakd
Copy link
Contributor

sjaakd commented Mar 15, 2017

when using ADDER_PREFERRED the following update method is generated:

    @Override
    public Car updateCar(CarDto dto, Car car) {
        ...
        if ( dto.getPersons() != null ) {
            for ( PersonDto person : dto.getPersons() ) {
                car.addPerson( dtoToPerson( person ) );
            }
        }
       ...

It doesn't clear the collection. This is not consistent with other CollectionMappingStrategie's
icw update methods.

@jklapwijk
Copy link

Will the collection.clear() be added to the generated update method implementation for the ADDER_PREFERRED strategy?

@sjaakd
Copy link
Contributor Author

sjaakd commented Feb 10, 2019

I'm reiterating some issues.. I see there's a difference. Although I am thinking whether we should access a getter when handling an adder method... @filiphr : WDYT?

@filiphr
Copy link
Member

filiphr commented Feb 10, 2019

This is a tough one @sjaakd. I also had the same question last week. I would say if you are using an adder strategy we should not touch the getter. It might even be possible that the adder returns an immutable collection that will throw an exception if we clear it, so I wouldn't go there 😄

@sjaakd
Copy link
Contributor Author

sjaakd commented Feb 10, 2019

Ok.. I'll close this one.. But we clear collections also in the regular case. Should we leave that as is?

@sjaakd sjaakd closed this as completed Feb 10, 2019
@filiphr
Copy link
Member

filiphr commented Feb 10, 2019

I think that in the regular case it is OK. The reason is that we are using the getter to add to the collection, so we can clear it. If we can't clear it we also can't add to it (at least that is my reasoning)

@sjaakd
Copy link
Contributor Author

sjaakd commented Feb 10, 2019

Agreed. But do we add this reasoning somewhere..? FAQ?

@filiphr
Copy link
Member

filiphr commented Feb 10, 2019

Perhaps in the documentation?

@sjaakd
Copy link
Contributor Author

sjaakd commented Feb 10, 2019

Good idea.. I'll open op a pr for that..

@filiphr
Copy link
Member

filiphr commented Feb 10, 2019

In Update existing bean instances

we currently have

Collection- or map-typed properties of the target bean to be updated will be cleared and then populated with the values from the corresponding source collection or map.

Maybe we need to add about the adders here

sjaakd pushed a commit to sjaakd/mapstruct that referenced this issue Feb 10, 2019
sjaakd pushed a commit to sjaakd/mapstruct that referenced this issue Feb 10, 2019
sjaakd added a commit that referenced this issue Feb 10, 2019
* #1142 update documentation

* #1142 comment
@sjaakd
Copy link
Contributor Author

sjaakd commented Feb 10, 2019

Maybe we need to add about the adders here

Hmmm. Immutable? I don't think we have adders for maps..

@filiphr filiphr added this to the 1.4.0 milestone Sep 22, 2019
filiphr pushed a commit to filiphr/mapstruct that referenced this issue Sep 22, 2019
@filiphr filiphr modified the milestones: 1.4.0, 1.3.1 Sep 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants