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

1.2.0-Beta1 Automapping Feature: Doesn't work with "Unmapped target property" on the child Bean. #1104

Closed
brabenetz opened this Issue Feb 26, 2017 · 9 comments

Comments

Projects
None yet
5 participants
@brabenetz

brabenetz commented Feb 26, 2017

An "Unmapped target property" on the child Bean shows the error Consider to declare/implement a mapping method: ...:
MapStructWithHirarchyMapper.java.txt

The annotation @Mapping(ignore = true, target = "child.id") solves that issue.
But, ...

  1. There should not be an Error. A warning like Unmapped target property: "child.id" would be more appropriate.

  2. If unmappedTargetPolicy = ReportingPolicy.IGNORE then there should not even be a warning.

Background:
This was the root error I searched for: My Domain Objects all extends a MappedSuperClass which contains the Database Id.
This MappedSuperClass should not be relevant for the mapping, because the DB-Id is not exposed to the DTOs.

@filiphr

This comment has been minimized.

Show comment
Hide comment
@filiphr

filiphr Feb 27, 2017

Member

@brabenetz thanks a lot for trying out and providing so good examples for the new feature. Regarding the reporting, we explicitly added an error there and are not taking the ReportingPolicy into consideration. I am not 100% sure if it is correct to ignore or warn here.

I have to agree, for your use case it makes perfect sense to ignore it. However, imagine that we start to generate a mapping method for arbitrary objects, only because they have the same name, there you could end up with a completely empty method that just creates the target object and returns it.

@brabenetz maybe having #1086 and explicitly using MapperConfig would be a better way to go. WDYT?

@gunnarmorling, @agudian, @sjaakd WDYT?

Member

filiphr commented Feb 27, 2017

@brabenetz thanks a lot for trying out and providing so good examples for the new feature. Regarding the reporting, we explicitly added an error there and are not taking the ReportingPolicy into consideration. I am not 100% sure if it is correct to ignore or warn here.

I have to agree, for your use case it makes perfect sense to ignore it. However, imagine that we start to generate a mapping method for arbitrary objects, only because they have the same name, there you could end up with a completely empty method that just creates the target object and returns it.

@brabenetz maybe having #1086 and explicitly using MapperConfig would be a better way to go. WDYT?

@gunnarmorling, @agudian, @sjaakd WDYT?

@agudian

This comment has been minimized.

Show comment
Hide comment
@agudian

agudian Feb 28, 2017

Member

Hmm, I would have expected that the unmappedTargetPolicy is taken into account as it is defined and that no error is created.

But I get how we want to avoid just instanciating an empty object... although it's kinda the same with our other mapping methods in combination with unmappedTargetPolicy = IGNORE - the user wouldn't be informed in case the properties in the target class are refactored and nothing matches anymore.
So IMHO the IGNORE policy is for daredevils only anyway 😉, and we're not making it that much worse with the name-based forged methods.

Member

agudian commented Feb 28, 2017

Hmm, I would have expected that the unmappedTargetPolicy is taken into account as it is defined and that no error is created.

But I get how we want to avoid just instanciating an empty object... although it's kinda the same with our other mapping methods in combination with unmappedTargetPolicy = IGNORE - the user wouldn't be informed in case the properties in the target class are refactored and nothing matches anymore.
So IMHO the IGNORE policy is for daredevils only anyway 😉, and we're not making it that much worse with the name-based forged methods.

@brabenetz

This comment has been minimized.

Show comment
Hide comment
@brabenetz

brabenetz Feb 28, 2017

I prefer Tests which informs me if something is not mapped any more.
But at the moment (for this Case) there is only an Error that basically says that Automapping Feature doesn't work, without any hint what the real problem is. Even without unmappedTargetPolicy = IGNORE.

brabenetz commented Feb 28, 2017

I prefer Tests which informs me if something is not mapped any more.
But at the moment (for this Case) there is only an Error that basically says that Automapping Feature doesn't work, without any hint what the real problem is. Even without unmappedTargetPolicy = IGNORE.

@filiphr

This comment has been minimized.

Show comment
Hide comment
@filiphr

filiphr Feb 28, 2017

Member

@brabenetz you are most probably right. Maybe we also need to display the properties that could not have been mapped.

@agudian do you think that we should use the warning policy per default? I am asking, because if we do this, then the mappers would be generated with only a warning.

Member

filiphr commented Feb 28, 2017

@brabenetz you are most probably right. Maybe we also need to display the properties that could not have been mapped.

@agudian do you think that we should use the warning policy per default? I am asking, because if we do this, then the mappers would be generated with only a warning.

@agudian

This comment has been minimized.

Show comment
Hide comment
@agudian

agudian Mar 1, 2017

Member

I think a warning (or whatever fits the configured policy) is just fine. The IDE will show it right away and you'll know what's going on. 🙂

Member

agudian commented Mar 1, 2017

I think a warning (or whatever fits the configured policy) is just fine. The IDE will show it right away and you'll know what's going on. 🙂

@filiphr filiphr added this to the 1.2-next milestone Mar 1, 2017

@filiphr

This comment has been minimized.

Show comment
Hide comment
@filiphr

filiphr Mar 1, 2017

Member

@agudian I think that it is agreed to use the unmappedTargetPolicy for the forged methods as well. I have assigned the issue to the 1.2-next milestone. Please remove it if you don't agree. This means that name based forged methods will be as all other mappings, i.e. there won't be an error per default :)

Member

filiphr commented Mar 1, 2017

@agudian I think that it is agreed to use the unmappedTargetPolicy for the forged methods as well. I have assigned the issue to the 1.2-next milestone. Please remove it if you don't agree. This means that name based forged methods will be as all other mappings, i.e. there won't be an error per default :)

@gunnarmorling

This comment has been minimized.

Show comment
Hide comment
@gunnarmorling

gunnarmorling Mar 2, 2017

Member

+1 for raising a warning by default and honor the effectively applying unmappedTargetPolicy. It should make no difference in terms of handling/reporting whether there is an unmapped target property on the root level or on the level of an automatically generated ("forged") sub node mapping method.

Member

gunnarmorling commented Mar 2, 2017

+1 for raising a warning by default and honor the effectively applying unmappedTargetPolicy. It should make no difference in terms of handling/reporting whether there is an unmapped target property on the root level or on the level of an automatically generated ("forged") sub node mapping method.

@gunnarmorling gunnarmorling added the bug label Mar 2, 2017

@filiphr

This comment has been minimized.

Show comment
Hide comment
@filiphr

filiphr Mar 2, 2017

Member

Before we change this, I have one question for @navpil, do you remember why we are throwing an error when the targetProperties at BeanMappingMethod#L681 are empty?

Member

filiphr commented Mar 2, 2017

Before we change this, I have one question for @navpil, do you remember why we are throwing an error when the targetProperties at BeanMappingMethod#L681 are empty?

@navpil

This comment has been minimized.

Show comment
Hide comment
@navpil

navpil Mar 3, 2017

Contributor

@filiphr There was a problem with Issue590Test. In particular - automapping tried to generate mapping between String and XMLFormatter. Since none of them have any properties an empty method got generated which was wrong. It was decided to add the check that there is at least one property we can map into.
You can read more in the mapstruct google group in thread "Automatically create mapping methods to map nested properties" https://groups.google.com/forum/#!topic/mapstruct-users/8MiKbs-W5_c

Contributor

navpil commented Mar 3, 2017

@filiphr There was a problem with Issue590Test. In particular - automapping tried to generate mapping between String and XMLFormatter. Since none of them have any properties an empty method got generated which was wrong. It was decided to add the check that there is at least one property we can map into.
You can read more in the mapstruct google group in thread "Automatically create mapping methods to map nested properties" https://groups.google.com/forum/#!topic/mapstruct-users/8MiKbs-W5_c

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