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

#1821 nullpointer due to @BeanMapping via inheritance #1822

Merged
merged 3 commits into from Sep 22, 2019

Conversation

sjaakd
Copy link
Contributor

@sjaakd sjaakd commented May 16, 2019

mapExtended does not have the @BeanMapping, but still it tries to resolve the prism.

  • I fixed this by copying the SelectionParameters for inheritance too and leaving out the resultType.
  • Additionally I introduced a builder for BeanMapping.
  • Also, I don't resort to the prims in BeanMappingMethod, but use BeanMapping

@filiphr
Copy link
Member

filiphr commented Sep 15, 2019

@sjaakd what is the status of this one? I see that #1811 has been merged already

@sjaakd
Copy link
Contributor Author

sjaakd commented Sep 15, 2019

still an issue.. I know that @Captain1653 also looked at it.. Just need to find some time to think about this one.

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.

Where exactly was the NPE before? There are a lot of changes to be able to determine what fixes the problem.

Do we really need a builder for the BeanMapping, why can't we keep the old methods? Looking at the builder it seems that it is the same as the methods before.

@sjaakd
Copy link
Contributor Author

sjaakd commented Sep 22, 2019

Do we really need a builder for the BeanMapping, why can't we keep the old methods? Looking at the builder it seems that it is the same as the methods before.

Yes.. but isn't that always the case..? For me I start to use builder when the number of constructor args starts to approach 10 args. Slowly we are moving all our classes to builders, so its unclear to me why we would apply that here.

btw: I added the mirror as constructor arg.

@sjaakd
Copy link
Contributor Author

sjaakd commented Sep 22, 2019

Where exactly was the NPE before? There are a lot of changes to be able to determine what fixes the problem.

The problem was caused because we inherited the resultType. Check the selection parameters, it has a method forInheritance as well. Here the resultType is set to null.

I probably should have split stuff in different commits.

@filiphr
Copy link
Member

filiphr commented Sep 22, 2019

We are passing the utils all over the place. Perhaps we should just pass the ProcessorContext everywhere and have a single parameter instead of 3 or 4, that is for some other day. I only mentioned it because it was difficult to review with so many changes 😄.

One think which is now not clear to me. If before we had an NPE, why isn't some test failing due to reporting?

@sjaakd
Copy link
Contributor Author

sjaakd commented Sep 22, 2019

One think which is now not clear to me. If before we had an NPE, why isn't some test failing due to reporting?

I added a failing test :).

@filiphr
Copy link
Member

filiphr commented Sep 22, 2019

Yes that test confused me. I thought that it was failing when we were reporting something. Anyways LGTM, feel free to merge

@sjaakd sjaakd merged commit ade4f4d into mapstruct:master Sep 22, 2019
sjaakd added a commit to sjaakd/mapstruct that referenced this pull request Sep 29, 2019
filiphr pushed a commit that referenced this pull request Sep 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants