Fields of type `java.util.Optional` should be treated as optional

[maciejwalkowiak]

Fields of type Optional (one from Java 8) could be automatically treated as if they would be annotated with @Opt. Additionally, actual type could be unwrapped from Optional in generated builder API.

For example, for given class:

@Builder(style = BuilderStyle.TYPE_SAFE)
public class Name {
    private String firstName;
    private String lastName;
    private Optional<String> middleName;

    Name(String firstName, String lastName, Optional<String> middleName) {
        this.firstName = firstName;
        this.lastName = lastName;
        this.middleName = middleName;
    }
}

The builder usage:

            NameBuilder.name()
                .firstName("first")
                .lastName("last")
                .middleName("middle")
                .build();

.. and generated builders:

@Generated("Jilt-1.2")
public interface NameBuilders {
  interface FirstName {
    LastName firstName(String firstName);
  }

  interface LastName {
    Optionals lastName(String lastName);
  }

  interface Optionals {
    Optionals middleName(String middleName);

    Name build();
  }
}

@Generated("Jilt-1.2")
public class NameBuilder implements NameBuilders.FirstName, NameBuilders.LastName, NameBuilders.Optionals {
  private String firstName;

  private String lastName;

  private Optional<String> middleName;

  private NameBuilder() {
  }

  public static NameBuilders.FirstName name() {
    return new NameBuilder();
  }

  public NameBuilders.LastName firstName(String firstName) {
    this.firstName = firstName;
    return this;
  }

  public NameBuilders.Optionals lastName(String lastName) {
    this.lastName = lastName;
    return this;
  }

  public NameBuilders.Optionals middleName(String middleName) {
    this.middleName = Optional.ofNullable(middleName);
    return this;
  }

  public Name build() {
    return new Name(firstName, lastName, middleName);
  }
}

[skinny85]

Hi @maciejwalkowiak, thanks for opening the issue!

The problem I see with this is that this goes against the idiomatic way of using Optional, as only the return type of methods, and not as the type of parameters or fields (more info here).

So, I think a better way to model that would be:

public class Name {
    private String firstName;
    private String lastName;
    @Nullable private String middleName;

    @Builder(style = BuilderStyle.TYPE_SAFE)
    Name(String firstName, String lastName, @Nullable String middleName) {
        this.firstName = firstName;
        this.lastName = lastName;
        this.middleName = middleName;
    }

    public Optional<String> getMiddleName() {
        return Optional.ofNullable(this.middleName);
    }

    // ...
}

Which would make it covered by #11.

In addition, making this change would actually be backwards-incompatible for existing users of Jilt, unless we generate two setter methods for each field (one for the Optional<T> type, and one for the T type), which would increase the complexity of the solution.

Let me know your thoughts on the matter!

Thanks, Adam

[maciejwalkowiak]

I think the idiomatic way is just a one way of using optionals and i've seen lots of projects where it is used in non-idiomatic way.

Generating two methods to keep project backward compatible would be the way to go - I agree it will increase the complexity, so perhaps at this stage is not worth it. I believe implementing #11 is much more important. I may spend some time to implement support for optionals, and we can then asses is the complexity worth it, but can't promise if/when.

[skinny85]

That makes sense! And thanks a lot for offering to help with this 😊.

I currently have my hands full with #5 and #11, but I'd be happy to investigate what it would take to support this after I'm done with those two. I'll be sure to update this issue if I start working on it.

[skinny85]

OK, #5, #10 and #11 are now all complete, and pushed to develop. I'm looking into this feature.

[skinny85]

Well, this was hell 😀. But it's finally done on the develop branch (here's an example test, here are the docs for it). I've also added the overloaded setters that take the unwrapped type from the Optional, to maintain backwards compatibility.

Assuming everything looks good, I'll include this in the next release (alongside #5, #10, and #11) that I'll do this week.

[skinny85]

@maciejwalkowiak now that I've implemented this feature, and had a chance to play with it, I'm having serious second thoughts about it. I don't think we actually want to do this.

To illustrate, let me use your class as an example:

@Builder(style = BuilderStyle.STAGED)
public final class FullName {
    private final String firstName, lastName;
    private final Optional<String> middleName;

    // ...
}

The problem here is that you still have to handle null in the FullName class's constructor, even though the middleName property has type Optional - because null is the default value of all reference type properties in Builders generated by Jilt. So, if the user of the Builder doesn't provide middleName before calling build(), null will be passed to the class' constructor in the middleName parameter.

So, the constructor is forced to have this ugly shape:

@Builder(style = BuilderStyle.STAGED)
public final class FullName {
    FullName(String firstName, String lastName, Optional<String> middleName) {
        this.middleName = middleName == null ? Optional.empty() : middleName;
        // ...

So, how could we solve it? Well, we could special-case the initialization of properties of type Optional in the Builder to initialize them with Optional.empty(), instead of null like we do for other reference types. So, if the consumer of the Builder doesn't provide that property before calling build(), Optional.empty() will be passed to the constructor of the class instead.

But that doesn't actually help, because we want to overload the setters to have one which takes an Optional directly, to preserve backwards compatibility! So, a consumer can always do this:

FullNameBuilder.middleName((Optional<String>) null).build();

(BTW, that cast to Optional<String> is required because of the setter overload, and is another downside of this solution)

And you still have the same problem.

OK, so you say. Let's not worry about backwards compatibility. Let's forget about generating the overloaded setter that takes an Optional, and just generate one with the unwrapped type (so, String for Optional<String>). This way, we can use Optional.ofNullable() in the body of the setter, and, combined with the initialization changes for Optional fields in the Builder mentioned above, we would guarantee that a value of type Optional is never null in the Builder.

But here's the thing - you can already do this with Jilt! Without any special support for this feature:

public final class FullName {
    private final String firstName, lastName;
    private final Optional<String> middleName;

    private FullName(String firstName, String lastName, Optional<String> middleName) {
        this.firstName = firstName;
        this.lastName = lastName;
        this.middleName = middleName;
    }

    @Builder(style = BuilderStyle.STAGED)
    FullName(String firstName, String lastName, @Nullable String middleName) {
        this(firstName, lastName, Optional.ofNullable(middleName));
    }

    // ...
}

So, I'm thinking of actually reverting the changes I've pushed to develop, and removing this feature altogether. I don't think it's worth the additional complexity, especially considering that this is not the idiomatic way of using the Optional type anyway, so I don't think we should promote its usage in this fashion.

Let me know if you agree.

[skinny85]

I'm closing this issue as "Not Planned". Please leave a comment if you'd still like to see this implemented, and I'll reopen the issue.