Skip to content

#1104 use unmapped target policy for forged name based mappings #1137

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
Merged
filiphr merged 1 commit into from
Mar 15, 2017
Merged

#1104 use unmapped target policy for forged name based mappings #1137

filiphr merged 1 commit into from
Mar 15, 2017

Conversation

@filiphr
Copy link
Member

@filiphr filiphr commented Mar 12, 2017

Fixes #1104.

This PR is a starter for supporting unmapped target policy for the forged named based mappings.

There are some tests that are failing and I wanted to discuss with you about the approach to "fix" them or remove them.

In my opinion most of the tests (the ones that are failing because the compilation succeeds) need to be updated and have the unmapped target property message with warning.

One main change is that an error is no longer reported if the target properties for a forged method are empty. I think that we should behave the same way as the user has written the method.

For example for the Issue590Test. One can define a method like:

void map(String source, @MappingTarget xmlFormatter);

and we are going to generate an implementation for it. Yes it'll be empty and it'll look like:

@Override
public void map(String source, XMLFormatter xmlFormatter) {
    if ( source == null ) {
        return;
    }
}

Maybe I should add the generated Sources for those tests to the source code?

WDYT?

@gunnarmorling
Copy link
Member

gunnarmorling commented Mar 13, 2017

@filiphr What is your suggestion to do for the String - XMLFormatter case?

We could raise a warning when generating a (forged) method without any property mappings at all, as it indicates that something is fishy. But otherwise I'm not sure how we could detect that something is wrong.

It's this kind of situation which is why I'm still skeptical about the auto-mapping feature (and why we should have an option to turn it off).

@filiphr
Copy link
Member Author

filiphr commented Mar 13, 2017

@gunnarmorling In my opinion I don't see a difference between a mapping like String - XMLFormatter and Source - Target where the target is empty (makes no sense to have empty target, but still :)).

I was thinking to just mark the test as it is a success. I agree with you having a target that is empty is fishy. Therefore, I think that we should always emit a warning when there are no target properties, not only for (forged) name based mappings. We can even suggest the method that needs to be created.

A user can still write a method like

XMLFormatter map(String source);

and MapStruct will generate it empty.

Regarding the turning off. I also think that it'll be nice to have the option. Maybe it'll be best to do #1118 (or at least move the forged mapping in the MappingResolver). If we do that then we can more easily apply the turning of. Moreover, this PR can benefit from turned of auto mappings (all those tests will not fail).

@agudian
Copy link
Member

agudian commented Mar 13, 2017 •
edited
Loading

We could raise a warning when generating a (forged) method without any property mappings at all, as it indicates that something is fishy. But otherwise I'm not sure how we could detect that something is wrong.

I don't know... then the only way to remove the warning would be to manually add a method declaration for that source/target type pair... But that would be funny as well. How about having another option in an annotation to actively acknowledge and suppress the warning? It could be applied to manually declared methods for that type pair as well. And one should be able to put that annotation to a prototype method...

@agudian
Copy link
Member

agudian commented Mar 13, 2017

Btw, my suggestion would be to let this PR only handle the unmapped target policy thing, and let's discuss the warning and a proper suppression mechanism in a seperate issue.

@filiphr
Copy link
Member Author

filiphr commented Mar 13, 2017

Btw, my suggestion would be to let this PR only handle the unmapped target policy thing, and let's discuss the warning and a proper suppression mechanism in a seperate issue.

If you agree then I will add back the targetProperties.isEmpty() check and for forged methods we will output an error like we were doing before. Only then we can move the discussion for that in a new issue, it doesn't make sense to fix the tests when we know that it is not a good approach to leave it like that :)

@gunnarmorling
Copy link
Member

gunnarmorling commented Mar 13, 2017

I don't know... then the only way to remove the warning would be to manually add a method declaration for that source/target type pair...

Right, but that's the most reasonable thing to do, no? In which case would you have a mapping without any properties?

But +1 for discussing this separately.

I don't know... then the only way to remove the warning would be to manually add a method declaration for that source/target type pair...

+1

@filiphr filiphr mentioned this pull request Mar 13, 2017
Closed
@filiphr filiphr force-pushed the 1104-unmapped-policy-for-forging branch from 8c14b98 to 4c2c8bc Compare March 13, 2017 22:51
@filiphr
Copy link
Member Author

filiphr commented Mar 13, 2017

I have created a new issue for when the target is empty.

Just to add to the discussion, the user has to implement a method, if he/she just declares one then it would be the same.

I updated the PR and left the empty target properties. Please have a look at the modified tests and let me know if you agree with them

@agudian
Copy link
Member

agudian commented Mar 14, 2017

Just checking on my phone here on the subway, but does unmappedTargetProperty=IGNORE work as well?
And could you rename the Erroneous* mappers that now only report warnings?

@filiphr
Copy link
Member Author

filiphr commented Mar 14, 2017

@agudian Yes unmappedTargetProperty=IGNORE works. Have a look at the UnmappableIgnoreXXXMapper(s).

For the Erroneous* mappers. Should I rename them so they can run in the integration tests or just the naming? I am asking, because they are still in a erroneous package.

@agudian
Copy link
Member

agudian commented Mar 14, 2017

Yes unmappedTargetProperty=IGNORE works. Have a look at the UnmappableIgnoreXXXMapper(s).

Cool 👍

For the Erroneous* mappers. Should I rename them so they can run in the integration tests or just the naming? I am asking, because they are still in a erroneous package.

Yes, please rename and move out of that package... :)

@agudian
Copy link
Member

agudian commented Mar 14, 2017

Looks good otherwise, feel free to merge when you're done 🎉

@filiphr filiphr force-pushed the 1104-unmapped-policy-for-forging branch from 4c2c8bc to f37320c Compare March 14, 2017 22:12
@filiphr filiphr merged commit 9881a88 into mapstruct:master Mar 15, 2017
@filiphr
Copy link
Member Author

filiphr commented Mar 15, 2017

Merged. Thanks a lot for the review @gunnarmorling, @sjaakd and @agudian :)

@filiphr filiphr deleted the 1104-unmapped-policy-for-forging branch March 15, 2017 21:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

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

3 participants

@filiphr @gunnarmorling @agudian