Java "step" builders are not future-compatible

[eladb]

Brought up by @kiiadi - our beautiful Java "step" builder, which enforces required fields before optional fields will cause backwards compatibility issues when a field is changed from required to optional (which is not supposed to be a breaking change).

Furthermore, the current approach will also cause a breaking change if the order of properties change across revisions, which is also unacceptable.

[skinny85]

Hey @eladb ,

there is actually a way to do this with "step" Builders.

Example: we have a class Target with 2 properties, first and second, both of which are required:

public final class Target {
     public final int first;
     public final String second;

     public Target(int first, String second) {
          this.first = first;
          this.second = second;
     }
}

So, the interfaces that the Builders implement look something like:

public interface TargetBuilders {
    interface FirstB {
         SecondB first(int first);
    }

    interface SecondB {
         Build second(String second);
    }

    interface Build {
        Target build();
    }
}

and the Builder itself:

public class TargetBuilder implements FirstB, SecondB, Build {
    public static FirstB target() {
        return new TargetBuilder();
    }

    private int first;
    private String second;

    private TargetBuilder() {
    }

    // setters here...
}

used like so:

Target target = TargetBuilder.target().first(1).second("").build();

Now, if any of the properties are changed from required to optional, the only thing that has to change are the interfaces - we need to add the setters from the subsequent interfaces, up to the first required interface, into the interfaces for the optional properties.

This might sound a little abstract, so concrete examples:

a) Let's say first is changed to be optional (second is still required). The interfaces will now be:

public interface TargetBuilders {
    interface FirstB {
         SecondB first(int first);
         Build second(String second);
    }

    interface SecondB {
         Build second(String second);
    }

    interface Build {
         Target build();
    }
}

b) The reverse - second is now optional (first is still required):

public interface TargetBuilders {
    interface FirstB {
         SecondB first(int first);
    }

    interface SecondB {
         Build second(String second);
         Target build();
    }

    interface Build {
         Target build();
    }
}

c) Both are now optional:

public interface TargetBuilders {
    interface FirstB {
         SecondB first(int first);
         Build second(String second);
         Target build();
    }

    interface SecondB {
         Build second(String second);
         Target build();
    }

    interface Build {
         Target build();
    }
}

This maintains the relative ordering between the properties, and so is backwards compatible.

I've actually implemented this pattern in a library I own, which is an annotation processor for generating "step" Builders, called Jilt - it's the TYPE_SAFE_UNGROUPED_OPTIONALS style.

[eladb]

@skinny85 thanks! Makes total sense. However, I think that won't work in the jsii case, since we are generating these builders from the jsii spec. We don't have historical context about the API, so we can't tell that this was once a required field, only that it's now optional. Does that make sense?

[skinny85]

Does that make sense?

Sorry, but it doesn't ;p.

Why would you need historical context for this generation?

[kiiadi]

@skinny85 how do you know which 'additional' properties to generate on the interfaces based on what was previously required? To put it another way - how do you know what was previously required?

In your scenario a) when I create the initial builder - what interface gets returned?

Target target = TargetBuilder.target().first(1).second("").build();

In scenario a) where second is required but first is optional - if this were the first iteration of that object (ie - we'd never seen it before), one would have to create it like this:

Target target = TargetBuilder.target().second("").first(1).build();

So without keeping historical information about how the model has mutated over time, how will we know to allow the first code sample?

[skinny85]

@skinny85 how do you know which 'additional' properties to generate on the interfaces based on what was previously required? To put it another way - how do you know what was previously required?

It doesn't matter what was previously required - it only matters what is required currently.

In your scenario a) when I create the initial builder - what interface gets returned?

Target target = TargetBuilder.target().first(1).second("").build();

TargetBuilder.target() returns FirstB - always, in all 3 variants, regardless of whether first is required or not (because first is the first property).

In scenario a) where second is required but first is optional - if this were the first iteration of that object (ie - we'd never seen it before), one would have to create it like this:

Target target = TargetBuilder.target().second("").first(1).build();

Nope. The creation would still be the same as before (when both first and second were optional):

Target target = TargetBuilder.target().first(1).second("").build();

In variant a), if you want to construct a Target without first, it looks like this:

Target target = TargetBuilder.target().second("").build();

Does this make it more clear?

[kiiadi]

I think that makes sense - how do we know what is the "first" property?

[eladb]

🤕my head is exploding... I need to read this a few more times...

[eladb]

By the way, code that emits the builders under jsii-pacmak/lib/generators/java.ts#L332.

[kiiadi]

I guess the crux of it is, that the "step" approach programmatically enforces an order that customers can call methods in. (Which is a great customer experience, it makes it easier to use the API if it's telling you what is legal to call next) However in order to do it well we need to ensure that the order remains stable - otherwise we'll introduce a backwards compatibility issue.

[skinny85]

I guess the crux of it is, that the "step" approach programmatically enforces an order that customers can call methods in. (Which is a great customer experience, it makes it easier to use the API if it's telling you what is legal to call next) However in order to do it well we need to ensure that the order remains stable - otherwise we'll introduce a backwards compatibility issue.

The way I've shown above indeed fixes the order of the properties, and changing it is a backwards-incompatible change. That's usually not a huge deal (the order doesn't matter that much), but if we're worried about this being too rigid, there is a slight variation that allows you to code-generate your way out of even this problem :).

The basic idea is to generate interfaces for each possible ordering of assigning properties to your Builder (and, by extension, your target class). Let me show you what that looks like, again on the same Target class example shown above (I'll only be showing the interfaces, the rest is the same - in particular, TargetBuilder.target() always returns FirstB):

Starting situation (both first and second are required)

interface TargetBuilders {
    interface FirstB {
        FirstThenSecond first(int first);
        SecondThenFirst second(String second);
    }

    interface FirstThenSecond {
        Build second(String second);
    }

    interface SecondThenFirst {
        Build first(int first);
    }

    interface Build {
        Target build();
    }
}

So, you can do:

TargetBuilder.target().first(1).second("").build(); // fine
TargetBuilder.target().second("").first(1).build(); // also fine

but not:

TargetBuilder.target().first(1).second("").first(2).build(); // does not compile

Variant a) (first turned from required to optional, second is still required)

interface TargetBuilders {
    interface FirstB {
        FirstThenSecond first(int first);
        SecondThenFirst second(String second);
    }

    interface FirstThenSecond {
        Build second(String second);
    }

    interface SecondThenFirst {
        Build first(int first);
        Target build();
    }

    interface Build {
        Target build();
    }
}

So, this is still fine:

TargetBuilder.target().first(1).second("").build(); // still fine
TargetBuilder.target().second("").first(1).build(); // also still fine

but now you can also do:

TargetBuilder.target().second("").build(); // no first given

Variants b) and c) are very similar - I won't bother showing them.

This requires generating on the order of n! interfaces, where n is the number of properties of the target class, but it does make the "step" Builder immune to property order changes.

[kiiadi]

My question was more when we get a model how will we know what the 'first' property is? Are we relying on the order of the properties in the collection? Are we doing it alphabetically?

The above works OK when we have 1 or two properties - but what about when we have 20? Are we going to generate an interface for every permutation of every property? I think that's going to lead to very complex looking, large classes.

[skinny85]

My question was more when we get a model how will we know what the 'first' property is? Are we relying in the order of the properties in the collection? Are we doing it alphabetically?

I believe we would be relying on the order the properties are declared in in TypeScript code.

The above works OK when we have 1 or two properties - but what about when we have 20? Are we going to generate an interface for every permutation of every property? I think that's going to lead to very complex looking, large classes.

Yes, like I said, for a class with 20 properties, we would generate 20! interfaces (note however that the Builder class will still be simple).

[kiiadi]

I believe we would be relying on the order the properties are declared in in TypeScript code.

Whilst it looks to be modeled as an Array (which should be ordered) I'm not sure how this array is populated in JSII. This ordering requirement may add additional complexity at several layers of Typescript code. Where does it come from?

[eladb]

I don't think we can rely on ordering since it can arbitrarily be changed by authors, and they don't expect that to be a breaking change.

I also don't think it's scalable to generate N! interfaces for every builder (we have many builders with many properties).

Back to the drawing board? (or just do flat builders with runtime validation)

@rix0rrr had some magical generic incantation, can you share?

[kiiadi]

@eladb that's kinda what I suspected. I think you're right. I love the idea of compile-time validation and getting the IDE to help me "do it right" but I'm just not sure how we can do it in a backwards compatible way.

Unless @rix0rrr's generic magic comes to the rescue! I'm also keen to take a look at that!

[skinny85]

@eladb can you explain how does the current Builder look like? Let's say we have something like this in TypeScript:

interface SomeProps {
    str: string; // required
    i: number; // required
    o?: string; // optional
}

What does the Java Builder API look like for SomeProps?

[eladb]

The interface MyFirstStruct from our regression tests looks like this:

export interface MyFirstStruct {
    astring: string
    anumber: number
    firstOptional?: string[]
}

Here's the generated builder for it (full source):

static Builder builder() {
    return new Builder();
}

class Builder {
    public AnumberStep withAstring(final java.lang.String value) {
        return new FullBuilder().withAstring(value);
    }

    public interface AnumberStep {
        Build withAnumber(final java.lang.Number value);
    }

    public interface Build {
        MyFirstStruct build();
        Build withFirstOptional(final java.util.List<java.lang.String> value);
    }

    class FullBuilder implements AnumberStep, Build {
        private Jsii$Pojo instance = new Jsii$Pojo();

        public AnumberStep withAstring(final java.lang.String value) {
            java.util.Objects.requireNonNull(value, "_astring is required");
            this.instance._astring = value;
            return this;
        }

        public Build withAnumber(final java.lang.Number value) {
            java.util.Objects.requireNonNull(value, "_anumber is required");
            this.instance._anumber = value;
            return this;
        }

        public Build withFirstOptional(final java.util.List<java.lang.String> value) {
            this.instance._firstOptional = value;
            return this;
        }

        public MyFirstStruct build() {
            MyFirstStruct result = this.instance;
            this.instance = new Jsii$Pojo();
            return result;
        }
    }
}

This discussion actually brings up another issue with this which is ordering. If the order of the required properties change, it's a breaking change again.

I am leaning towards a simple flat, runtime validation, solution.

[rix0rrr]

I did have a magical generic incantation, but unfortunately it has similar lack of robustness against changing properties. The trick is to have a generic parameter for every required property:

abstract class TRUE {}
abstract class FALSE {}

class MyProps<P1, P2> {
  // Meat of the solution
  MyProps<TRUE, P2> withRequiredParam1(...) { }
  MyProps<P1, TRUE> withPequiredParam2(...) { }
  MyProps<P1, P2> withOptionalParam(...) { }

  // To make it so you must go through initialization
  private MyProps() { ... }
  static MyProps<FALSE, FALSE> newInstance();
}

// This is where we consume the initialized object
void useProps(MyProps<TRUE, TRUE> props) { ... }

Will make it so that you can only call useProps() once you've called both withRequiredParam1() and withRequiredParam2(), and the order in which you do so doesn't matter.

However, this solution is really only acceptable if users never have to write out the type... and we've seen people very much writing the following source code:

MyProps myProps = new MyProps();
myProps.withThis();
myProps.withThat();
MyConstruct myConstruct = new MyConstruct(myProps);

Which makes it so you have to type it as MyProps<?, ?> (ouch), and even worse have to change it to MyProps<?, ?, ?> if the number of required properties changes.

Only acceptable in case Java can do type deduction, and even then only possibly.

[skinny85]

@eladb thanks for the explanation! Yeah, so it seems like the ordering already needs to be stable for the current Builders to work. Given that, I see 2 possible solutions:

a) use the method I outlined above to make future transitions of a property from being required to optional backwards-compatible (it won't handle changing the property order, but that's the same as the current Builders), or

b) get rid of "step" Builders altogether in favor of "unsafe" Builders

I wouldn't fault you for going with b). Personally, I'm a huge fan of the "step" Builder pattern, and I think it provides a great experience in Java, so I'm a fan of a). Considering the order is already fixed for the Builders, and it seems to be working fine, maybe it's not such a big deal...? We can always document it in some L2-writing guidelines, right (and perhaps have auto-generated Java unit tests checking that the order wasn't broken)?

[kiiadi]

Could we make this something that construct creators opt into? I assume the method ordering comes from how we generate our JSII meta-data - if the construct author wants to commit to a stable method order they can opt-in to using the step-builder....

[kiiadi]

@skinny85 I love the step-builders too for all your same reasons - I would have loved to have done something similar in the Java SDK. However due to the problems with backwards compatibility it creates (and we have no way to enforce service teams to introduce new structure members in a specific order) we didn't go that route.

[eladb]

I don't think guidance around keeping the order is a sustainable approach. We can't expect that to be preserved. I think we will have to not use step builders 😢

[skinny85]

😭

[skinny85]

I've given this topic a lot of thought, and I believe I came up with a possible solution.

I won't detail on how I arrived at it (it will make a good blog post, or possibly a made-for-TV movie), but it looks something like the following.

Imagine the target class is called Example. It has 4 properties, named first, second, third and fourth. Only the first 2 (first and second) are required, third and fourth are optional:

public final class Example {
    public final String first;
    public final int second;
    public final boolean third;
    public final char fourth;

    public Example(String first, int second, boolean third, char fourth) {
        this.first = first;
        this.second = second;
        this.third = third;
        this.fourth = fourth;
    }
}

We need the following 2 interfaces for type safety:

public interface ExampleBuilderInterfaces {
    interface BuilderInterf<R1, R2> {
        R1 first(String first);
        R2 second(int second);
        BuilderInterf<R1, R2> third(boolean third);
        BuilderInterf<R1, R2> fourth(char fourth);
    }

    interface FinalBuilderInterf extends BuilderInterf<FinalBuilderInterf, FinalBuilderInterf> {
        FinalBuilderInterf first(String first);
        FinalBuilderInterf second(int second);
        FinalBuilderInterf third(boolean third);
        FinalBuilderInterf fourth(char fourth);
        Example build();
    }
}

And finally the Builder itself:

public final class ExampleBuilder implements FinalBuilderInterf {
    @SuppressWarnings("unchecked")
    public static
    BuilderInterf<
        BuilderInterf<?, FinalBuilderInterf>,
        BuilderInterf<FinalBuilderInterf, ?>
    > example() {
        return (BuilderInterf) new ExampleBuilder();
    }

    private String first;
    // remaining 3 fields from Example...

    @Override
    public FinalBuilderInterf first(String first) {
        this.first = first;
        return this;
    }

    // other setters analogous...

    @Override
    public Example build() {
        return new Example(first, second, third, fourth);
    }
}

Now, you can use it like this:

ExampleBuilder.example().first("").second(1).build() // fine
ExampleBuilder.example().second(1).first("").build() // also fine

and it's not possible to skip providing a required property:

ExampleBuilder.example().second(3).build() // does not compile!

If, in the future, first changes from required to optional, the newly-generated interfaces for type-safety will be:

public interface ExampleBuilderInterfaces {
    interface BuilderInterf<R> {
        BuilderInterf<R> first(String first);
        R second(int second);
        BuilderInterf<R> third(boolean third);
        BuilderInterf<R> fourth(char fourth);
    }

    interface FinalBuilderInterf extends BuilderInterf<FinalBuilderInterf> {
        FinalBuilderInterf first(String first);
        FinalBuilderInterf second(int second);
        FinalBuilderInterf third(boolean third);
        FinalBuilderInterf fourth(char fourth);
        Example build();
    }
}

and the Builder itself:

public final class ExampleBuilder implements FinalBuilderInterf {
    public static
    BuilderInterf<
            FinalBuilderInterf
    > example() {
        return new ExampleBuilder();
    }
     // rest of the class exactly the same as above...
}

All of the previous usages of ExampleBuilder (when first was still required) will remain valid after first becomes optional, and now this will compile:

ExampleBuilder.example().second(3).build() // fine now

I understand this is a ton of code to look at, so I put it in a GitHub repo, feel free to take a look here: https://github.com/skinny85/future-proof-step-builder

Pros of this solution:

• A constant (2 to be exact) number of interfaces required, regardless of the number of properties the target class has
• Only one Builder class, regardless of the number of properties the target class has
• Handles providing the properties in an arbitrary order
• Handles evolving the API (changing a property from required to optional)

Cons:

• The signature of the builder static factory method (ExampleBuilder.example() above) is complex (and becomes more complex as the number of required properties grow), and it's visible to our customers
• If a customer provides a required property twice before providing all of the required properties (so: ExampleBuilder.example().first("").first("")), they will not be able to proceed from there (that last first returns java.lang.Object).

@eladb and @kiiadi , I'd love your take on this - does any of this make sense, or am I just going crazy here?

[skinny85]

I've worked on this some more. Turns out, there are 2 ways of achieving what we want here, that is a Builder that:

• Guarantees, at compile time, that all of the required properties of the class are provided before constructing an instance.
• Uses a single Builder class, in-place (does not require nested classes, or extra allocations).
• Does not care about the order of the properties.
• Maintains backwards compatibility when changing a property from required to optional (without "historic" context).

The method I outlined above, which I call the 'static factory method' variant (OK, I agree it's not the greatest name, but it's not like there's a wealth of existing vocabulary I can draw from in this case), is one way. The one big pro it has is that it generates a constant number of interfaces (just 2), regardless of how many properties does the built class have. However, it has 2 downsides: the complicated type of the client-facing static factory method, and the weird behavior when providing the same required property twice, before all of the required properties have been provided.

I've tried to correct these faults, but I was not able to do it. What I did do, however, is discover an alternative way of formulating the Builder. I call it the 'interfaces' variant. It doesn't have any of the faults of the 'static factory method' variant - the types are all simple, and you can specify any property multiple times, and nothing weird happens. It has one disadvantage though - it requires generating 2^m interfaces, where m is the number of required properties of the built class. However, I don't think I recall a CDK type with more than 3 required properties (the number of optional properties does not increase the interface count at all), so I think it's acceptable for our use case.

I won't paste the 'interfaces' variant code here, but it's all in the example repo: https://github.com/skinny85/future-proof-step-builder

[eladb]

Wow 😮 impressive solutions!

I wish the 2-interface approach wouldn’t have that issue with double-required... I don’t think the complexity of the returned type is a blocker, but the double-required issue is.

I do think that you should at least add another required property to your prototypes: two is a tricky number for tests... especially due to the exponential implications of these solutions (perhaps you can generate the code...).

The CDK does and will have types with a large number of required properties, so the 2^m number of interfaces in the 2nd approach does worry me...

[skinny85]

I added an example for a 3 required property class here - right now, only in the 'interfaces' variant, I'll push the 'static factory method' variant soon.

I'm banging my head against the wall trying to fix the double required property problem in the 'static factory method' variant. The best I could come up with right now was a little cheat: we can "balloon" the returned type to accommodate specifying the same field i times, for any i > 0. For example, if we take the return type above:

BuilderInterf<
        BuilderInterf<?, FinalBuilderInterf>,
        BuilderInterf<FinalBuilderInterf, ?>
> 

and change it to this:

BuilderInterf<
        BuilderInterf<
           BuilderInterf<?, FinalBuilderInterf>,
           FinalBuilderInterf
       >,
       BuilderInterf<
           FinalBuilderInterf,
           BuilderInterf<FinalBuilderInterf, ?>
       >
> 

our Builder will now handle specifying the same property twice, but will fail in the same way as before if specifying it 3 times. We could then change it in a similar way to accommodate the 3 case, but would fail for 4, etc.

I'm yet to figure out a solution that would handle repeating the same property an arbitrary number of times.

[eladb]

@kiiadi what do you think?

[skinny85]

After thinking about this problem even more, I'm starting to doubt whether we'll achieve what we want here.

Even assuming we will solve the above problem with the 'static factory method' variant, there are 2 things that concern me.

The first is the client experience of using these Builders. The main advantage of the Step builders was always the fact that the required properties were encoded by the type system - so, the clients of the class always knew the minimum set of properties they had to provide in order to construct an instance of the class. With this approach, this advantage is gone: if a class has 10 properties, but only 2 required ones, the client has no way of knowing which 2 of the 10 are required, short of looking at the documentation - and if we're making clients rely on the documentation, instead of the type system, we might as well provide a "dumb" Builder at that point.

The second concern is that the intermediate expressions in the Builder change types as we make required properties optional. This is true for both the 'static factory method' variant as well as the 'interfaces' one. What I mean is, if we have code like this:

SomeType intermediate = ExampleBuilde.example().first("");
Example instance = intermediate.second(2).build();

The type returned from ExampleBuilde.example().first("") will change if we release a new version of the CDK with first changed from required to optional - it will no longer be SomeType, and the above code will no longer compile.

Of course, that code is kind of weird, but we can't guarantee our Builders will not be used in weird ways.

Because of these concerns, even if we did have a future-proof Step Builder implementation that required generating a small number of extra interfaces, I'm not sure if we should use it.

[kiiadi]

I like where the thinking is going, and the implementation is a lot cleaner with only two interfaces to generate.

I share some of your concerns around changes being compilable in future - we need to ensure that our builders are backwards compatible - we have examples in the Java SDK of customers passing builders around (so that they can build up mutations over time).

I also have concerns about the generic typing leaking out - especially for large classes. Sure it's not a problem in languages (e.g. like Kotlin / Scala) which do type inference, and even in Java 10 it's less of a problem because of the introduction of var but the vast majority of customers are going to be using Java 8 or lower, that means their assignment of the builder variable includes all of the generics:

BuilderInterf<String, Integer, String, Double, Integer, String, String> builder = ExampleBulde.example();

This gets pretty ugly.

[kiiadi]

The other comment I'd make is about compatibility - we need to ensure not just compile-time backwards compatibility but also binary compatibility. These constructs are going to be used in a hierarchy of dependencies - with 3rd party libraries also depending on the CDK/JSII. We can't assume that each intermediate dependency is going to be recompiled against a new version of the library.

That means things like changing return types (i.e. introducing/removing generic parameters), even if the methods available remains the same - cannot be permitted.

[skinny85]

Hah. Interesting. I've actually solved the "required property provided multiple times before all required properties" problem of the 'static factory method' variant. It actually turned out to be much simpler than I thought. And the resulting type of the static factory method is much cleaner and shorter than what was there before.

I've pushed the examples to the test repo ([example with 2 properties], [example with 3 properties]), both with some tests confirming the behavior. I'm repeating the return type for the 3-variant below, as it's probably the craziest Java method signature I will write in my career:

    public static
    <
            T_1_2 extends BuilderInterf<T_1_2, T_1_2, FinalBuilderInterf>,
            T_1_3 extends BuilderInterf<T_1_3, FinalBuilderInterf, T_1_3>,
            T_2_3 extends BuilderInterf<FinalBuilderInterf, T_2_3, T_2_3>,
            T_1 extends BuilderInterf<T_1, T_1_2, T_1_3>,
            T_2 extends BuilderInterf<T_1_2, T_2, T_2_3>,
            T_3 extends BuilderInterf<T_1_3, T_2_3, T_3>,
            T extends BuilderInterf<T_1, T_2, T_3>
    > T user() {
        return (T) new UserBuilder();
    }

It doesn't invalidate any of the concerns I voiced above, but at the very least it's interesting that it was possible to express in Java using only 2 interfaces.