Closed blackfoxoh closed 4 years ago
That's not a builder, it's an updater :) PB doesn't support update(T t)
though it may be possible to trick it into doing so by annotating a factory method that contains some bad code to return the existing T instead of a new one.
private static Thing temp; // NOT thread safe, use ThreadLocal if required
@GeneratePojoBuilder public static Thing factoryMethod() {
return temp == null ? new Thing() : temp;
}
This is an evil builder but it's worth noting you can generate multiple builders so this can live alongside whatever you have currently.
Thank you for your feedback.
That sounds possible but I agree with you it is really evil. I would have to write a factory method for every entity, generate a seperate builder and even the "api" to use it would require to first set temp before applying the builder... really complex in contrast to what would be necessary if PB would support update(T t)
.
I agree that "Updater" is the correct term for what I have described. Thinking about this I have to correct my description above: What I want to achieve is a builder, not an updater. In fact the existing copy(T t)
method is nearly what I need with the only problem that I need the other values already set in the builder to have precedence. My usecase doesn't allow to call copy(T t)
before defining the other data. So a copyNotSetFields(T t)
(name of the method might be optimizable) which initializes the builders fields only for not already initialized ones.
I stumbled upon PB in an article in iX magazine by M. Karneim and O. Kraeft (German) which promotes PB for the usecase of providing testdata. I really like this idea but to be really helpful for us this functionality would be necessary for efficently handle our testcases. Maybe there is a chance to add this (as a configurable option).
I don't think copyNotSetFields
is a good API addition in general though I understand your usecase. What counts as "unset"? null might be an intent, what about primitives, is 0 "unset"? If you look inside the PB builder, it explicitly tracks what has been set so on the builder has that knowledge (not the bean).
OPTION 1 Merge Builder
A generic api might be ThingBuilder merge(ThingBuilder builder)
where the client call would be new ThingBuilder().copy(instance).merge(template).build()
Implementing this to the point of production-worthy code is non trivial though as PB has struggled for a long time figure out builder interfaces and builder inheritance (merge(Builder<? super T> builder)
). Features like this only add to the problem :)
OPTION 2 Update Pojo
Your first request void update(T item)
isn't terrible. Client call is template.update(new ThingBuilder().copy(instance).build())
This is easy to implement but the builder might hold constructor properties that cannot be set
OPTION 3 Use a lambda
Use Consumer<Thing>
as your template, nothing to do with PB. Client call is template.accept(new ThingBuilder().copy(instance).build())
let's assume we hav a pojo Article
with field nr
. If we create a pojobuilder for this it has the following two fields and could have the following method:
protected int value$nr$int;
protected boolean isSet$nr$int;
...
public ArticleBuilder copyNotSetFields(Article pojo) {
if (!isSet$nr$int) {
withNr(pojo.getNr());
}
...
}
Result of new ArticleBuilder().withNr(123).copyNotSetFields(someArticle)
would be the same as if you do new ArticleBuilder().copy(someArticle).withNr(123)
but with the first (new) solution the copy call could happen later.
Maybe a better API would be
public ArticleBuilder copy(Article pojo) {
copy(pojo, true);
}
public ArticleBuilder copy(Article pojo, boolean overwrite) {
if (overwrite || !isSet$nr$int) {
withNr(pojo.getNr());
}
...
}
your OPTION 3: I don't really got it so far. your example client call passes a 1:1 copy of the instance to the accept method. So far I haven't won much, the builder in this scenario is just used to produce a clone but nothing of the functionality (merge/update) is actually done here...!? Or please open my eyes if I missed something...
Hi @blackfoxoh.
I am not sure if I understand your use case completely, but maybe you can help yourself by using PB's Factory Method feature?
Hi I'm not sure if I understand your suggestion with the factory method - I had a look at that but at the moment I don't know how this should solve my problem easily. I can try to describe my usecase again:
copy()
inside the application then all the data defined in the incoming PB-instance is overwritten.I am sorry, but using the copy
method does not help you here since it does not copy values from the builder to the pojo but vice versa. I guess it is valid to say that this is more than just a "slight difference".
Anyway, for your use case I have two things to say.
1) Like @drekbour said, a builder is not well suited for modifying existing objects. This would require a mutator. In can see that at a first glance both operators look alike but in fact aren't. Nevertheless you could use the factory method to redefine the builder's semantics. To do this, just declare your own factory method and annotate it with @GeneratePojoBuilder
. Then implement it like you described it yourself (find a suitable object in the database by using the method parameters, or create one if you don't find one, and finally return it).
2) But I'd like to suggest that you rethink your test design. Instead of providing a database full of data before each individual test starts, just clear it completely. Then, as a first step of each individual test, inside the "given" block, insert the minimal set of data that is required to run the test. It's best practice to use a combination of the builder pattern and a builder factory to do this without bloating the test with irrelevant information. In this approach you don't need a mutator at all.
I think a good solution is Option 1 earlier (merge builders). This doesn't break the basic contract of PojoBuilder and opens up new avenues of flexibility. It is however notably complex and probably unlikely to be implemented!
Thank you for your feedbacks. I will have a look if I can solve my issue with your suggestions or if I will use a completly different approach.
Nevertheless you could use the factory method to redefine the builder's semantics. To do this, just declare your own factory method and annotate it with
@GeneratePojoBuilder
. Then implement it like you described it yourself (find a suitable object in the database by using the method parameters, or create one if you don't find one, and finally return it).
To clarify what I meant with "redefining the builder's semantics" I created this little example:
The following code defines the semantics of the ContactBuilder
as described above.
Here is the generated ContactBuilder
(click to expand):
Below you find the other classes used in this example (click to expand):
The following program demonstrates how you could use the generated ContactBuilder
:
When runnning this program you get the following output:
1 = Contact [id=1, name=Albert Einstein, dateOfBirth=1879-03-14, profession=Patent examiner]
2 = Contact [id=null, name=dummyName, dateOfBirth=2000-11-30, profession=null]
Hi, I would like to see something similar to copy() but with a slight difference. Let me describe the usecase: I want to define some testdata so I use a builder on my jpa entitys. I would like to keep my testdata-definition separat and just want to define the attributes currently necessary for the testcase, e.g. for an article I maybe just want to specify its articlenumber (key) and measurements but the weight is not needed. In the testcase I'd like to define a builder with desired values, then my testframework should lookup the entity by it's key (if present) and just modify the values set in the builder (example above: keep weigth of the article as is). If no article is found a new one will be created and weight is empty then. Of course the lookup of JPA-entitys and persisting them is not part of my request to pojobuilder, just part of usecasedescription.
Best way to achieve this might be to have a
buildWithBase(T t)
or something like this which uses the passed in object (if not null) instead a new instance of T to set the builder-values. What do you think about that? Or is there already a way to achive this I didn't notice?best regards Ralf