Skip to content

#1821 nullpointer due to @BeanMapping via inheritance#1822

Merged
sjaakd merged 3 commits into
mapstruct:masterfrom
sjaakd:1821
Sep 22, 2019
Merged

#1821 nullpointer due to @BeanMapping via inheritance#1822
sjaakd merged 3 commits into
mapstruct:masterfrom
sjaakd:1821

Conversation

@sjaakd

@sjaakd sjaakd commented May 16, 2019

Copy link
Copy Markdown
Contributor

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

filiphr commented Sep 15, 2019

Copy link
Copy Markdown
Member

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

@sjaakd

sjaakd commented Sep 15, 2019

Copy link
Copy Markdown
Contributor Author

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

@filiphr filiphr left a comment

Copy link
Copy Markdown
Member

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

sjaakd commented Sep 22, 2019

Copy link
Copy Markdown
Contributor Author

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

sjaakd commented Sep 22, 2019

Copy link
Copy Markdown
Contributor Author

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

filiphr commented Sep 22, 2019

Copy link
Copy Markdown
Member

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

sjaakd commented Sep 22, 2019

Copy link
Copy Markdown
Contributor Author

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

filiphr commented Sep 22, 2019

Copy link
Copy Markdown
Member

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.

2 participants