skinny85 / jilt

Java annotation processor library for auto-generating Builder (including Staged Builder) pattern classes
Other
240 stars 12 forks source link

Functional Builders #17

Closed tschuehly closed 5 months ago

tschuehly commented 8 months ago

Hello there. First thank you for creating this library! The staged builder are so awesome! I was wondering if you are up to adding a BuilderStyle.Functional to this library that implements a Builder that is described here: https://glaforge.dev/posts/2024/01/16/java-functional-builder-approach/

Do you think that would be feasible, I might take a crack at it if I find the time. Where would I need to start? Greetings Thomas

skinny85 commented 8 months ago

Hi @tschuehly,

thanks for opening the issue! I'm glad you found the library useful 😊.

As for Functional Builders, I think it's an interesting idea, and I would have no objections against adding this style of Builders to Jilt πŸ™‚. I think the main challenge will be in figuring out how to implement this pattern without modifying the built class itself, which you can't do in a well-behaved annotation processor like Jilt - the fact that the static mutator methods are part of the class is crucial in the blog article you linked to, since that allows them to change the fields, which are private. But there might be a way to simulate something similar with a separate Builder class.

As for where to start on implementing this, it would probably involve adding a new value to the BuilderStyle enum, and then handling that new value in BuilderGeneratorFactory by implementing a new BuilderGenerator (you can extend your new implementation from AbstractBuilderGenerator, which contains a lot of common logic shared by all current BuilderGenerators - in fact, looking at how the other BuilderGenerators work should give you a good idea of how to implement this new one).

Let me know if this makes sense, and if you have any questions!

Thanks, Adam

skinny85 commented 8 months ago

@tschuehly any updates on this? πŸ™‚

tschuehly commented 8 months ago

@skinny85 No I didn't have the time to take a look

skinny85 commented 8 months ago

Do you want me to investigate this area, and maybe propose some potential solutions?

tschuehly commented 8 months ago

Sure! Take a crack at it, that would be awesome!

skinny85 commented 7 months ago

So, I think a basic version of a functional builder is pretty straightforward.

Using the example value class from the above article:

package org.jilt.test.data.functional;

public final class SomeModel {
    public final String modelName;
    public final Float temperature;
    public final Integer maxOutputTokens;

    public SomeModel(String modelName, Float temperature, Integer maxOutputTokens) {
        this.modelName = modelName;
        this.temperature = temperature;
        this.maxOutputTokens = maxOutputTokens;
    }
}

With a separate Builder class that Jilt would generate, we can make the fields of the value class final, which the original article couldn't do.

A SomeModelBuilder would look like this:

package org.jilt.test.data.functional;

public class SomeModelBuilder {
    public static interface Setter extends Consumer<SomeModelBuilder> {}

    public static Setter modelName(final String modelName) {
        return builder -> { builder.modelName = modelName; };
    }

    public static Setter temperature(final Float temperature) {
        return builder -> { builder.temperature = temperature; };
    }

    public static Setter maxOutputTokens(final Integer maxOutputTokens) {
        return builder -> { builder.maxOutputTokens = maxOutputTokens; };
    }

    public static SomeModel someModel(Setter... setters) {
        SomeModelBuilder builder = new SomeModelBuilder();
        for (Setter setter : setters) {
            setter.accept(builder);
        }
        return new SomeModel(
                builder.modelName,
                builder.temperature,
                builder.maxOutputTokens
        );
    }

    private String modelName;
    private Float temperature;
    private Integer maxOutputTokens;

    private SomeModelBuilder() {
    }
}

And you would use it like this:

import static org.assertj.core.api.Assertions.assertThat;

import static org.jilt.test.data.functional.SomeModelBuilder.maxOutputTokens;
import static org.jilt.test.data.functional.SomeModelBuilder.modelName;
import static org.jilt.test.data.functional.SomeModelBuilder.temperature;

SomeModel someModel = SomeModelBuilder.someModel(
        maxOutputTokens(13),
        temperature(36.6F),
        modelName("my model")
);

assertThat(someModel.modelName).isEqualTo("my model");
assertThat(someModel.temperature).isEqualTo(36.6F);
assertThat(someModel.maxOutputTokens).isEqualTo(13);

The interesting part is if we can make it support Staged Builders?

I think we can do required properties with types pretty easily:

package org.jilt.test.data.functional;

public class SomeModelBuilderStaged {
    public static interface SetterModelName extends Consumer<SomeModelBuilderStaged> {}
    public static SetterModelName modelName(final String modelName) {
        return builder -> { builder.modelName = modelName; };
    }

    public static interface SetterTemperature extends Consumer<SomeModelBuilderStaged> {}
    public static SetterTemperature temperature(final Float temperature) {
        return builder -> { builder.temperature = temperature; };
    }

    public static interface SetterMaxOutputTokens extends Consumer<SomeModelBuilderStaged> {}
    public static SetterMaxOutputTokens maxOutputTokens(final Integer maxOutputTokens) {
        return builder -> { builder.maxOutputTokens = maxOutputTokens; };
    }

    public static SomeModel someModel(
        SetterModelName setterModelName,
        SetterTemperature setterTemperature,
        SetterMaxOutputTokens setterMaxOutputTokens
    ) {
        SomeModelBuilderStaged builder = new SomeModelBuilderStaged();
        setterModelName.accept(builder);
        setterTemperature.accept(builder);
        setterMaxOutputTokens.accept(builder);
        return new SomeModel(
                builder.modelName,
                builder.temperature,
                builder.maxOutputTokens
        );
    }

    private String modelName;
    private Float temperature;
    private Integer maxOutputTokens;

    private SomeModelBuilderStaged() {
    }
}

And it can be used only like this:

import static org.jilt.test.data.functional.SomeModelBuilder.maxOutputTokens;
import static org.jilt.test.data.functional.SomeModelBuilder.modelName;
import static org.jilt.test.data.functional.SomeModelBuilder.temperature;

SomeModel someModel = SomeModelBuilderStaged.someModel(
        modelName("my model"),
        temperature(36.6F),
        maxOutputTokens(13)
);

But optional properties seem really tricky - I'm not sure how to handle those...

Thoughts on this @tschuehly?

skinny85 commented 7 months ago

OK, I figured out how to do optional properties πŸ˜€. The crucial insight is combining the two above approaches into one.

For example, let's say that we want to make the temperature and maxOutputTokens of SomeModel optional, like they were originally.

So, with Jilt, it would look something like:

package org.jilt.test.data.functional;

import org.jilt.Builder;
import org.jilt.BuilderStyle;
import org.jilt.Opt;

@Builder(style = BuilderStyle.FUNCTIONAL)
public final class SomeModel {
    public final String modelName;

    @Opt
    public final Float temperature;

    @Opt
    public final Integer maxOutputTokens;

    public SomeModel(String modelName, Float temperature, Integer maxOutputTokens) {
        this.modelName = modelName;
        this.temperature = temperature == null ? 0.3F : temperature;
        this.maxOutputTokens = maxOutputTokens == null ? 100 : maxOutputTokens;
    }
}

So, for the required properties (only modelName in this case), we generate a separate setter interface for each, and we use those property-specific interface types in the static creation method as parameters. But for all optional properties, we share the same interface, like Setter above in the first example, and we add that type as the last, variadic, argument to the creation method.

So, bringing it all together, the Builder for the above SomeModel that Jilt would generate would be:

package org.jilt.test.data.functional;

public class SomeModelBuilder {
    public static interface SetterModelName extends Consumer<SomeModelBuilder> {}
    public static SetterModelName modelName(final String modelName) {
        return builder -> { builder.modelName = modelName; };
    }

    // shared interface for optional properties
    public static interface Setter extends Consumer<SomeModelBuilder> {}

    public static Setter temperature(final Float temperature) {
        return builder -> { builder.temperature = temperature; };
    }

    public static Setter maxOutputTokens(final Integer maxOutputTokens) {
        return builder -> { builder.maxOutputTokens = maxOutputTokens; };
    }

    public static SomeModel someModel(
        SetterModelName setterModelName,
        Setter... optionals
    ) {
        SomeModelBuilderStaged builder = new SomeModelBuilderStaged();
        setterModelName.accept(builder);
        for (Setter setter : optionals) {
            setter.accept(builder);
        }
        return new SomeModel(
                builder.modelName,
                builder.temperature,
                builder.maxOutputTokens
        );
    }

    private String modelName;
    private Float temperature;
    private Integer maxOutputTokens;

    private SomeModelBuilderStaged() {
    }
}

And the Builder can be used in the following ways:

import static org.jilt.test.data.functional.SomeModelBuilder.maxOutputTokens;
import static org.jilt.test.data.functional.SomeModelBuilder.modelName;
import static org.jilt.test.data.functional.SomeModelBuilder.temperature;

// temperature and maxOutputTokens are both default
SomeModel someModel1 = SomeModelBuilder.someModel(
        modelName("my model")
);

// only maxOutputTokens is default
SomeModel someModel2 = SomeModelBuilder.someModel(
        modelName("my model"),
        temperature(36.6F)
);

// only temperature is default
SomeModel someModel3 = SomeModelBuilder.someModel(
        modelName("my model"),
        maxOutputTokens(13)
);

// no defaults
SomeModel someModel4 = SomeModelBuilder.someModel(
        modelName("my model"),
        maxOutputTokens(13),
        temperature(36.6F)
);

This actually looks really, really nice, in my opinion.

@tschuehly thoughts on this?

tschuehly commented 7 months ago

@skinny85 Looks very good!

glaforge commented 7 months ago

Thanks @tschuehly for bringing this functional builder idea to Jilt, and thank you @skinny85 for reaching out to me. I replied over Twitter DMs to Adam's ping, and I'm copying my thoughts down below:

When I wrote that article, someone pointed me at your library, that I didn't know, and thought it was a massive improvement over what Lombok does, for example. And I particularly liked the "staged" builder.

So, what I liked about my approach was that I could use again constructors. But I understand that this could be problematic for a (well-behaved) annotation processor as you can't change the class itself.

I also liked that the design was quite simple to make, without necessitating a library, but that's only true for the simple approach (without optional params, etc)

The questions I've received were about the fact that the fields were not final. Some people also wondered about serializability (with Jackson and friends). The topic of required/optional values came up too. Others complained about discoverability (but IDEs are smart and suggest the methods because they return the right type)

I had a quick look at your implementation idea, and I really like your design! It ticks pretty much all the boxes. Of course you can't use the SomeModel constructor directly, but that's logical. Fields are final, that's good. You handle optional vs required values.

There's one thing I'm wondering is about staged builders here. I thought the idea was to return the next setter, so you can chain them? But here, we can't really chain them. So we can't really do a real staged builder with that approach, I guess? That would be weird to have builder(temperature(123).maxTokens(23)) with a mixed of chained methods and single methods.

Overall, again, I really like your approach and implementation! That's neat! Looking forward to trying it in Jilt!

skinny85 commented 7 months ago

Thank you for the detailed response @glaforge! I'm so glad you think this approach captures your original intent ☺️.

There's one thing I'm wondering is about staged builders here. I thought the idea was to return the next setter, so you can chain them? But here, we can't really chain them. So we can't really do a real staged builder with that approach, I guess? That would be weird to have builder(temperature(123).maxTokens(23)) with a mixed of chained methods and single methods.

Yes, the Staged Builder is implemented very differently here - there is no chaining. The required properties are forced by the per-property types of the parameters of the static factory method of the Builder (SomeModelBuilder.someModel() in the above example, taking SomeModelBuilder.SetterModelName as the first argument), while the optional properties are done by sharing the same type (SomeModelBuilder.Setter above), and making the last argument to the static factory method a variadic argument of that type. So, while the idea behind Staged Builders is preserved (forcing users of the Builder, at compile time, to provide all required properties when constructing an instance of the target class), the implementation is wildly different.

Because of this, I don't think I would even use the term "Staged" to refer to these Builders - I think these are Functional Builders, a completely different kind. This happens to work out beautifully with the BuilderStyle enum, as we can add a new value to it, BuilderStyle.FUNCTIONAL, and everything clicks into place. While, if we could have Builders that are both Functional and Staged, I guess the solution would be to instead create a new annotation, called something like @FunctionalBuilder, which you used alongside the existing @Builder, and combined it with BuilderStyle.CLASSIC or BuilderStyle.STAGED. I think that's more complicated, so I like the additional BuilderStyle enum value solution a lot more.

I hope this makes sense, please let me know if it doesn't πŸ™‚.

skinny85 commented 7 months ago

@tschuehly @glaforge I was also curious about your opinion on another aspect that I was thinking about. I wonder whether it makes sense to group all of the static methods for the optional properties of the target class under an additional nested class of the Builder.

So, continuing the example from above, do something like this:


public class SomeModelBuilder {
    public static interface SetterModelName extends Consumer<SomeModelBuilder> {}
    public static SetterModelName modelName(final String modelName) {
        return builder -> { builder.modelName = modelName; };
    }

    // shared interface for optional properties
    public static interface Setter extends Consumer<SomeModelBuilder> {}

    public static class Optionals {
        public static Setter temperature(final Float temperature) {
            return builder -> { builder.temperature = temperature; };
        }

        public static Setter maxOutputTokens(final Integer maxOutputTokens) {
            return builder -> { builder.maxOutputTokens = maxOutputTokens; };
        }
    }

    public static SomeModel someModel(
        SetterModelName modelName,
        Setter... optionals
    ) {
        SomeModelBuilder builder = new SomeModelBuilder();
        modelName.accept(builder);
        for (Setter setter : optionals) {
            setter.accept(builder);
        }
        return new SomeModel(
                builder.modelName,
                builder.temperature,
                builder.maxOutputTokens
        );
    }

    private String modelName;
    private Float temperature;
    private Integer maxOutputTokens;

    private SomeModelBuilder() {
    }
}

The reason I'm thinking about this is that I'm a little worried about the static methods for the optional properties being potentially a little difficult to discover if there are many of them. I think the required properties are pretty easy, since the name of the parameter in the static factory method of the Builder will always match the name of the static method for that property exactly - but that's not the case for optional properties, which share the same, variadic, parameter to the static factory method.

I'm thinking that grouping them might be helpful - you could just type SomeModelBuilder.Optionals., and the IDE would autocomplete the possible static methods for you, making them more discoverable (putting them all directly on the SomeModelBuilder level would mix the required ones in there, which makes the optional ones stand out less).

Thoughts on this idea?

glaforge commented 7 months ago

Because of this, I don't think I would even use the term "Staged" to refer to these Builders - I think these are Functional Builders, a completely different kind.

The thing I had in mind was when some setters are inter-related. It's probably a bit of a design smell though, but for example, let's say I have a location field, and an endpoint field. The endpoint may be a concatenation of location and something else. In that case endpoint depends on location, so being able to set location first and endpoint next could be useful.

Another example would be maybe coordinates, like polar(angle).length(len) vs rectX(x).rectY(y) but maybe you'd just pass an object like coord(polarCoord) and coord(rectCoord). Or imagine combining polar(angle, len) and rect(x, y)?

But I guess it's not a great design when setters are dependent upon each other, and having a setter that takes 2+ params is weird and uncommon. And it's usually better to pass an aggregated object anyway.

glaforge commented 7 months ago

IDE would autocomplete the possible static methods for you, making them more discoverable

Discoverability is indeed important. If regrouping them helps IDEs, then that's clearly a +1 in my book.

skinny85 commented 7 months ago

The thing I had in mind was when some setters are inter-related. It's probably a bit of a design smell though, but for example, let's say I have a location field, and an endpoint field. The endpoint may be a concatenation of location and something else. In that case endpoint depends on location, so being able to set location first and endpoint next could be useful.

Right, so in Jilt, that would be handled in the constructor of the value class, where you can initialize the endpoint field by referring to the location property. The Jilt main ReadMe file actually contains an example of this (in fact, even two examples), with username and displayName properties:

public final class User {
    public final String email, username, firstName, lastName, displayName;

    @Builder(style = BuilderStyle.STAGED)
    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;
    }
}

Another example would be maybe coordinates, like polar(angle).length(len) vs rectX(x).rectY(y) but maybe you'd just pass an object like coord(polarCoord) and coord(rectCoord). Or imagine combining polar(angle, len) and rect(x, y)?

Yes, I think this would be handled by two different constructors of the value class, and two different Builders generated by Jilt. So, something like:

public class Point {
    @Builder(className = "PolarBuilder", factoryMethod = "polar")
    public Point(double angle, double length) {
        // ...
    }

    @Builder(className = "RectBuilder", factoryMethod = "rect")
    public static Point rect(double x, double y) {
        // ...
    }

    // ...
}

Which can be used like so:

Point p1 = PolarBuilder.polar()
    .angle(anAngle)
    .length(aLength)
    .build();

Point p2 = RectBuilder.rect()
    .x(anX)
    .y(aY)
    .build();

But I guess it's not a great design when setters are dependent upon each other, and having a setter that takes 2+ params is weird and uncommon. And it's usually better to pass an aggregated object anyway.

Yes. In Jilt, there is not even a concept of having a setter that takes 2+ parameters - it's not something the library supports (at least at this time). So, the only way to make that happen would be to have a property of a composite type.

glaforge commented 6 months ago

Hi, I'm curious what is the status of this idea?

skinny85 commented 6 months ago

Hi, I'm curious what is the status of this idea?

Hi Guillaume! I'm just finishing up something in another project I own, and I plan to work on this next. I hope to have it done this June.

glaforge commented 6 months ago

Cool, great news! Thank you :-)

skinny85 commented 6 months ago

Of course! I'll keep this issue updated with the progress.

skinny85 commented 5 months ago

@glaforge OK, I've implemented the Function Builder feature, pushed it to the develop branch, and published a Release Candidate of version 1.6 of Jilt, 1.6.rc1, which means you can easily try it out in your own project if you want to!

Some other resources:

  1. The documentation for the feature is here.
  2. Here are two examples of value classes using this feature ([1 (only required properties)], [2 (both required and optional properties)]).
  3. And here is an example test using the Function Builders for the above value classes.

Let me know if this implementation captures your intent for this feature, and if you have any feedback on it!

skinny85 commented 5 months ago

This feature has been released in Jilt version 1.6.

Let me know if you run into any problems related to this, and I'd be happy to reopen this issue.