-
-
Notifications
You must be signed in to change notification settings - Fork 939
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
Switching vom 1.2.0.Final to 1.3.0.Final leads to ignored ObjectFactory methods #1713
Comments
One interesting fact is that it took me hours to create a maven project from scratch, which reproduces the problem. As long as I put all together in one maven artifact (including the model, the mapper, and both dependencies to lombok and mapstruct), everything worked quite well. The problem does not occur then. So I splitted the project into two artifacts: one containing just the model and the dependency to lombok, and one just containing the mapper and the dependency to mapstruct. And then, the error occurs. Or in other words: The error does not occur as long as mapstruct seems to find lombok (or the lombok processor) on its classpath. I tried to have a look into the source code differences between mapstruct 1.2.0 and 1.3.0 to identify the possible root cause... but that's too much code to review for me right now. |
I'm providing now the example project, which proves the error. Please have a look into the following two branches: Example using 1.2.0.Final They only differ in the used mapstruct version. Output by maven when building the 1.2.0.Final version of the project:
Output by the 1.3.0.Final version of the project:
|
Please let me know if I you need any further information. Thanks for looking into this! Regards, |
Hey Michael.. I'll have a look asap. Thanks for your info. Passing default values as primitive values has been implemented by me as an optimization request. You could try |
@lazydays79 , I'm trying to read through your comments above.. And I'm a bit puzzled. Lets take them one by one:
Where is this shown in your example? Or was this an initial though?
The problem is not as such in Autoboxing. Thats fine. Its the Auto-unboxing which forms the root of the problem. So, the case were you have a wrapper-type that is potentially null and try to map it to a primitive. Vice versa there should be no problem.
I'll focus on that part.. Right? |
Your problem is above.. You are using Lombok with the builder. However, You are also requesting Mapstruct to create the object for your in an ObjectFactory method.. You should disable the builders if you don't want that.. Have a look in the documentation. on howto. EDIT You could also remove the |
@filiphr .. I tried to extend @lazydays79's example.. @lazydays79 : (your 1.3.0.Final example is actually 1.2.0.Final) with the noopbuilder. However it still creates the builder iso calling the object factory annotated method.. Think that that's an error.. Can you have a look? |
So, now you feel as confused as me the last days. 😁 Removing @builder is not an option, since this would lead to many other source code changes. The problem is indeed that mapstruct 1.3.0 ignores the object factory method, but 1.2.0 uses it. And this only occurs as long lombok is not a dependency in the same artifact where I'm using mapstruct. So I would say that this is definetly an issue here. Regards, |
@sjaakd maybe I missed up the links, I will check that tomorrow. But you should find two branches in my repository. One using 1.3.0 and one using 1.2.0. Just do a mvwn clean install in both, and you should see the difference. |
And I would say that object factories must be used by mapstruct if defined. Regardless whether the target class uses constructors, builders or whatsoever. Object factories should beat them all. |
I possibly see two problems.. yes.. you are right about objectfactories.. and.. it is strange that a builder is generated when with the noopbuilder option.. |
Yes, I know. But please don't blame me for that code. 😅 I just wanted to stick as near as possible to our commercial code, which in this case isn't made by me. And I don't want to change the code in one of our basic libraries, because it would be a breaking change (although it makes sence, I agree on that with you). Never the less, we still have the fact that mapstruct 1.3.0 ignores object factories under certain circumstances. |
There are 2 options, I think..
|
Hi @lazydays79 .. The @filiphr .. our documentation is not right. It suggests that the SPI can be in the project-at-hand itself. Thats not possible because you will not get the SPI file on your annotation processor class path.. Hence you need to define a small module that does so. |
I will have a look into it, thanks!
But it can take a few days.. Right now I'm fighting with a new release of
our software myself. ^_^
Thanks so far!
Am Mi., 13. Feb. 2019, 21:46 hat Sjaak Derksen <notifications@github.com>
geschrieben:
… Hi @lazydays79 <https://github.com/lazydays79> .. The NoOpBuilderProvider
works I could not directly push to your branch. But here's the diff in the
form of a PR
<sjaakd/mapstruct1.3.0issue@955f6be>
.
@filiphr <https://github.com/filiphr> .. our documentation is not right.
It suggests that the SPI can be in the project-at-hand itself. Thats not
possible because you will not get the SPI file on your annotation processor
class path.. Hence you need to define a small module that does so.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1713 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AQ8jGuPDdlSSZEftv-GH4DyniNKcczA8ks5vNHnCgaJpZM4a12GM>
.
|
@sjaakd the reason why the @lazydays79 builders have highest priority. Perhaps we need to iterate that and if there is an @sjaakd what do you think about #1661? I think that could be a solution for this, I would say that perhaps we even need to add a compiler option so that you can turn off the builders, this way it would be easier for people to do it. |
I guess we have to update our documentation for that and provide an example in our repo... |
I think we should have more fine grained control on when to use builders. This issue nicely demonstrates that.. Here setters are used (falsely IHMO) after the object has been build by means of an object factory in a default way: in my mind the builder is inextricably connected to immutable objects. But having said it's quite reasonable to assume that a bean consists out of properties that are immutable (builder) and mutable (setter). I don't think we can handle that out of the box. It would demand even mixed scenarios within one mapping method:
Having an object factory to initialise a bean using a builder, continuing with setters is also quite reasonable. |
Just for reference, the mix of immutability with (some) mutable fieldshas been also been asked for in #2155. |
This seems to have slipped through the cracks. Reading it now again we should perhaps give |
Yesterday I tried to switch from Mapstruct 1.2.0.Final to 1.3.0.Final. But this lead to some NullPointerExceptions in our unit tests.
After some investigation, I found out, that mapstruct now seems to ignore the ObjectFactory annotated method.
Here is some quick overview.
The target class, in which mapstruct will map:
The source class, from which mapstruct will map:
The mapper:
Let's have a look into the mapper implementation generated by 1.2.0.Final:
And now the one generated by mapstruct 1.3.0.Final:
Can you spot the difference? :)
The propblem here is, that this leads to uninitialized properties in the TargetClass object, in this case the two Boolean ones. And thanks to Autoboxing (which they should have never introduced in java IMHO), NullPointerExceptions occur in our unit tests.
I will provide more details in further comments to this issue...
Regards,
Michael
The text was updated successfully, but these errors were encountered: