Closed alexandrenavarro closed 7 months ago
Hi @alexandrenavarro,
thanks a lot for opening the issue! I think this is a sensible feature request. I'll try to work on it this week.
Thanks, Adam
No hurry, I just first know you are ok with the feature and it seems the case. I realized after also that the Optionals interface of the Stage builder should potential extends the different interfaces of mandatory fields if we want to override mandatory fields through the toBuilder method.
Thanks.
In this case, I think it’s fine to return the Builder class directly, and not the type-safe interface, since all fields will already be set (from the instance of the built class). So type safety doesn’t buy us anything here - in a way, all of the properties should be treated as optional here.
I agree.
OK, this is now done on the develop
branch, see the docs here.
I'll prepare a new release, 1.5
, including this feature, tomorrow.
The most interesting part of this feature was actually deciding how would the Builder get a value for the given property from an instance of the built class.
I've settled on the following algorithm:
record
s).public final
field case).Hopefully, this makes sense.
Your algorithm seems great.
I tested your develop branch but there is one problem. After retrieving the builder, if I set for instance the first mandatory field, after I have to set all following mandatory fields (I used the staged style), in this case, all fields should be optional because all mandatory fields are already set. It is linked to my second comment of the issue. I don't know what is the best way to solve but I see 2 potentials solutions
Thank you a lot for your work and reactivity, don't hesitate if you need more explanations.
@alexandrenavarro I guess you mean that, for the Person
example from the ReadMe:
public final class User {
public final String email, username, firstName, lastName, displayName;
@Builder(style = BuilderStyle.STAGED, toBuilder = "toBuilder")
public User(String email, @Opt String username, String firstName,
String lastName, @Opt String displayName) {
this.email = email;
this.username = username == null ? email : username;
this.firstName = firstName;
this.lastName = lastName;
this.displayName = displayName == null
? firstName + " " + lastName
: displayName;
}
}
When you just want to change the email
field of an existing User
, you can't do this:
UserBuilder.toBuilder(user)
.email("new@example.com")
.build(); // doesn't work!
That last call to .build()
doesn't compile, because email
in UserBuilder
returns the UserBuilders.FirstName
interface, which doesn't have a build()
method (only a firstName()
method that returns UserBuilders.LastName
)?
I've proposed a solution to the above problem, and pushed it to the develop
branch.
Can you try again, and let me know if that helps?
I checked quickly and it seems ok, It fixes my problem.
Nevertheless, I just realized linked to this feature, I need the possibility to annotate all the fields of the XBuilder (mandatory and optional fields) with a @Nullable to be able to use errorprone / nullaway / spotbugs on the XBuilder.
I need to use it on XBuilder classes notably to force someone not to give a null on a mandatory field and all the fields of the XBuilder must be annotated with @ Nullable because at first, all the fields are null and static tool checks it when thé builder is created.
I consider by default that a field / parameter / result must be notNull if it is not annotated with @ Nullable and if course, the contrary when it is annotated with @ Nullable.
My goal by using a staged jilt.builder is to be sure the object has no null value on all mandatory fields after .build() and check it with tools (errorprone / nullaway / spotbugs) at build time to reduce/remove (in my dream) the NullPointerException .
Tell me if it is not clear why I need it, it is not really easy to explain.
I will do test with errorprone / nullaway / spotbugs on the XBuilder to verify if I miss something else and keep in touch.
So, to sum up, you're saying you want every field (regardless whether it's required, or optional) of the generated Builder class to be annotated with @Nullable
?
I need to use it on XBuilder classes notably to force someone not to give a null on a mandatory field
That should already work today, right? Assuming you have "things are non-null by default" turned on, a required property setter won't have any annotations placed on its parameter, and so passing a @Nullable
value to it should cause a static analysis error.
So, to sum up, you're saying you want every field (regardless whether it's required, or optional) of the generated Builder class to be annotated with
@Nullable
?
Yes.
So, to sum up, you're saying you want every field (regardless whether it's required, or optional) of the generated Builder class to be annotated with
@Nullable
?Yes.
I'm not opposed to it, but this needs to be a separate feature then (something like @Builder(fieldsAnnotations = @Nullable)
).
I need to use it on XBuilder classes notably to force someone not to give a null on a mandatory field
That should already work today, right? Assuming you have "things are non-null by default" turned on, a required property setter won't have any annotations placed on its parameter, and so passing a
@Nullable
value to it should cause a static analysis error.
Yes, this part works because the method mandatoryField(X mandatoryField) or optionalField(@ Nullable optional) are well annotated on parameter as well on the result (always not null) but it will fail when we call new XBuilder() on the factoryMethod because the fields are not annotated with @ Nullable so there are considered to be always NotNull and as we call the empty constructor, just after the call, there are null, so the null checker will tell you it is no ok, your fields can be null after the constructor and there are not annotated with @ Nullable. All the fields not annotated by Nullable must be set in the constructor through a parameter or a defaulting in the constructor. I hope it is clearer, if not, tell me. I will do some tests to verify if I don't missed something else but I'm quite sure it will be necessary to have all fields annotated with a @ Nullable.
Yes, I think I get what you're saying, and I don't have any problems with supporting this - however, like I said in https://github.com/skinny85/jilt/issues/16#issuecomment-2004566812, I think this is a separate feature we're talking about here, I don't think it's related to toBuilder()
in any way.
Or, you can just exclude generated code from your static analysis tool 😉.
Or, you can just exclude generated code from your static analysis tool 😉.
No, because if you do that, you can set null in a mandatory field through the builder method, so you can miss a NullPointerException.
I don't think so. The call to a setter of a Builder will not be in generated code, it will be in the manually-written code, so it will still be checked.
Yes for the setter but I don't think it is not the case for the fields of the builder. It may work for the fields because the API can not permit some wrong things. I will do some tests on the different tool to see what is working or not and what is needed or not on the XBuilder. I will keep in touch.
I made some tests. You can see my experimentation on this repository https://github.com/alexandrenavarro/nullsafety-sample and mainly the class modified from the current develop branch generated jilt.Builder https://github.com/alexandrenavarro/nullsafety-sample/blob/main/src/main/java/com/github/alexandrenavarro/nullsafety/PersonBuilder.java
On nullaway, all checks from public API of XBuilder are checked on the caller and are correct. The checks inside XBuilder generates 2 errors :
On spotbugs, all checks from public API of XBuilder are checked on the caller and are correct. The checks inside XBuilder generates 1 error :
I did not check yet on checker framework but I supposed I will have same kind of results.
So finally, as the public API are correct and well checked by the checkers even if the XBuilder class checks are disabled because checks are made on the caller and as Staged style Builder prevents real problems inside the Builder even if the checkers see potential problems, I think the current develop branch version is ok at first in my user point of view using null checker on the client side.
You can modify your XBuider generated to in a second time to respect nullable checks if you want to have something really clean notably for classical style of builder by adding @ Nullable on Builder fields and check if the mandatory field are really not null but you will have to answer :
Tell me if you need more explanations or feedback if you want to implement clean null check builder.
Thank you for all, I will continue to do some tests on a real project now and I will report feedback/bugs if I see some problems in real project.
This has been fixed in Jilt release 1.5
.
I'm resolving this issue, please comment if you run into this problem again, and I'll be happy to reopen.
Possibilty to have a generated toBuilder method to map from a pojo to a builder (equivalent to
@lombok.Builder(toBuilder = true)
in the generated Builder class likeThis method is useful when your POJO is immutable or on a record and if you want to modify just 1-2 fields.
Thank you in advance, don't hesitate if it is not clear.