How to ensure that builder pattern is completed?

Question

EDIT: I am not worried about being called in the wrong order since this is enforced through using multiple interfaces, I am just worried about the terminal method getting called at all.

---

I am using a builder pattern to create permissions in our system. I chose a builder pattern because security is so important in our product (it involves minors so COPPA et al), I felt it was imperative that permissions be readable, and felt that the readability was of the utmost importance (i.e. use a fluent-style builder pattern rather than a single function with 6 values).

The code looks like this:

 permissionManager.grantUser( userId ).permissionTo( Right.READ ).item( docId ).asOf( new Date() );

The methods populate a private backing bean, that upon having the terminal method (i.e. asOf ) commit the permission to the database; if that method does not get called nothing happens. Occasionally developers will forget to call the terminal method, which does not cause a compiler error and is easy to miss on a quick reading/skimming of the code.

What could I do to prevent this problem? I would not like to return a Permission object that needs to get saved since that introduces more noise and makes permission code harder to read, follow, track, and understand.

I have thought about putting a flag on the backing which gets marked by the terminal command. Then, check the flag in the finalize method and write to the log if the object was created without persisting. (I know that finalize is not guaranteed to run, but it's the best I can think of.)

Hydrograph

isn't that what javadoc comments are for? also, usually the "terminal method" is called build(). – Baxy

The finalize approach (or the equivalent PhantomReference approach) should be a best-effort error detection mechanism only, if you implement it. As you said: it does not usually guarantee anything, but it can help you debug the problem. Also: you could keep grab a stack trace every time a non-terminal method is called and print that when the finalizer finds an un-applied builder. This way you'll know where the problem occured. – Osrock

I don't see any other non-compiler oriented approach to this other than separating the permission construction and registration. – Jarboe

@little bunny foo foo I didn't call it build() because it doesn't return an object, it persists the change directly to the database. – Hydrograph

@ArtB: So call it Save ? (And have AsOf perhaps just be another "property setting" function) – Mahon

@Marjan Venema I could, but that still doesn't help with my problem of guarantee that the terminal method (whatever it ends up being named) get called. – Hydrograph

@ArtB: that's why I commented and didn't answer. My thinking was that calling the final method "Save" would be a better indicator that you need to call it to actually save the data. "AsOf" as a name doesn't convey that message. – Mahon

@Marjan Ah, now I see your point, and I guess looking for the word "save" is very intuitive. I think the reason I didn't do that initially is that I wanted it to read like an sql-like command and additionally it adds an additional method call which contributes to line length. That said it is intuitive and should jump out more "wait! this isn't getting saved!". – Hydrograph

Answer 1

There is now an annotation processing based compiler plugin, that will check that for you, and throw compilation error, if the method is not present: Fluent API sentence end check.

You can either annotate your final method with @End annotation, or if you do not control the class, you can still provide a text file with fully qualified method name(s), and get the check working.

Then Maven can check during compilation.

It only works in Java 8, upwards, because it uses new compiler plugin mechanism, introduced there.

Elliellicott

How you stumbled on a 7 year old question is beyond me, but thank you for the answer! – Hydrograph

It's because I was facing similar problem and came across your question. So I wanted to let you know the solution too. – Magniloquent

Answer 2

Solution

A good way to structure this fluent API pattern is instead of just returning this from each method, return an instance of a Method Object Pattern that implements an Interface that only supports the method that should be next in the list and have the last method call return the actual object you need.

If that is the only way to get an instance of that object, the last method will always have to be called.

Q6613429.java

package com.stackoverflow;

import javax.annotation.Nonnull;
import java.util.Date;

public class Q6613429
{
    public static void main(final String[] args)
    {
        final Rights r = PermissionManager.grantUser("me").permissionTo("ALL").item("EVERYTHING").asOf(new Date());
        PermissionManager.apply(r);
    }

    public static class Rights
    {
        private String user;
        private String permission;
        private String item;
        private Date ofDate;

        private Rights() { /* intentionally blank */ }
    }

    public static class PermissionManager
    {
        public static PermissionManager.AssignPermission grantUser(@Nonnull final String user)
        {
            final Rights r = new Rights(); return new AssignPermission() {

                @Override
                public AssignItem permissionTo(@Nonnull String p) {
                    r.permission = p;
                    return new AssignItem() {
                    @Override
                    public SetDate item(String i) {
                        r.item = i;
                        return new SetDate()
                    {
                        @Override
                        public Rights asOf(Date d) {
                            r.ofDate = d;
                            return r;
                        }
                    };}
                };}
            };
        }

        public static void apply(@Nonnull final Rights r) { /* do the persistence here */ }

        public interface AssignPermission
        {
            public AssignItem permissionTo(@Nonnull final String p);
        }

        public interface AssignItem
        {
            public SetDate item(String i);
        }

        public interface SetDate
        {
            public Rights asOf(Date d);
        }
    }
}

This enforces the chain of construction calls, and is very friendly with code completion as it shows what the next interface is and it only method available.

Here is a more complete example with optional things in the middle:

UrlBuilder.java

This provides a foolproof checked exception free way to construct URL objects.

Mixing the persistence with the construction is mixing concerns:

Creating the object and storing it are different concerns and should not be mixed. Considering that .build() does not imply .store() and vice-versa and buildAndStore() points out the mixing of concerns immediately do the different things in different places and you get the guarantees you want.

Put your call to your persistence code in another method that only accepts a fully constructed instance of Rights.

Wheelock

Answer 3

You could write a rule for PMD or Findbugs if you really want to enforce it in the code. This would have the advantage that it is already available at compile time.

---

Runtime: If you only want to make sure the users call your builder in the correct order then use separate interfaces for each step.

grantUser() will return ISetPermission which has the method permissionTo(), which will return an IResourceSetter which has the method item()...

You can add all those interfaces to one builder, just make sure that the methods return the correct interface for the next step.

Hoplite

I was writing the exact same thing! – Glennaglennie

I do enforce correct order through interfaces, added comment about it. The PMD solution is a good one, but around here people get nervous about adding another tool to the build so I was looking for an in-Java solution. – Hydrograph

@ArtB I would generally recommend using PMD and Findbugs. If you don't want to include it in the normal build how about using Jenkins+Sonar to only let it run on the server side? – Uriiah

Answer 4

public class MyClass {
    private final String first;
    private final String second;
    private final String third;

    public static class False {}
    public static class True {}

    public static class Builder<Has1,Has2,Has3> {
        private String first;
        private String second;
        private String third;

        private Builder() {}

        public static Builder<False,False,False> create() {
            return new Builder<>();
        }

        public Builder<True,Has2,Has3> setFirst(String first) {
            this.first = first;
            return (Builder<True,Has2,Has3>)this;
        }

        public Builder<Has1,True,Has3> setSecond(String second) {
            this.second = second;
            return (Builder<Has1,True,Has3>)this;
        }

        public Builder<Has1,Has2,True> setThird(String third) {
            this.third = third;
            return (Builder<Has1,Has2,True>)this;
        }
    }

    public MyClass(Builder<True,True,True> builder) {
        first = builder.first;
        second = builder.second;
        third = builder.third;
    }

    public static void test() {
        // Compile Error!
        MyClass c1 = new MyClass(MyClass.Builder.create().setFirst("1").setSecond("2"));

        // Compile Error!
        MyClass c2 = new MyClass(MyClass.Builder.create().setFirst("1").setThird("3"));

        // Works!, all params supplied.
        MyClass c3 = new MyClass(MyClass.Builder.create().setFirst("1").setSecond("2").setThird("3"));
    }
}

Siddra

Clever. +1 for a novel idea, though I would prefer less clever solutions. – Mayor

Very clever, I like it, but unfortunately it exposes the constructor and loses one of the benefits of the builder pattern that I required in this scenario. That said, it's still a great solution for a DSL. – Hydrograph

Answer 5

There is the step builder pattern that does exactly what you needed : http://rdafbn.blogspot.co.uk/2012/07/step-builder-pattern_28.html

Stent

Answer 6

Apply the new permission in a separate step that first validates that the Builder was constructed correctly:

PermissionBuilder builder = permissionManager.grantUser( userId ).permissionTo( Right.READ ).item( docId ).asOf( new Date() );
permissionManager.applyPermission(builder); // validates the PermissionBuilder (ie, was asOf actually called...whatever other business rules)

Barramunda

this doesn't guarantee that the intermediate steps are called in the right order or at all at compile time, which is what the question is about how to enforce. – Wheelock

@Jarrod: I don't think so. The original poster does not talk about intermediate steps being called in the right order. He wants a way to guarantee that the final build step is called. – Bost

not calling the final step is not calling them in order, it is just a sub case. and calling the last step without the middle ones is of no use as well. – Wheelock

I enforce the prpoer order through the use of interfaces, I am just worried about calling the final method. Also, this is the method I specifically mentioned avoiding in my question: """I would not like to return a Permission object that needs to get saved since that introduces more noise and makes permissioning code harder to read, follow, track, and understand.""" – Hydrograph

Part of the point of the Builder pattern is to be nonlinear, so one assumes permissionManager.grantUser(id).permissionTo(Right.READ).asOf(date) gives read access to all items. If the steps really must go so linear, using a Builder pattern for this domain looks like overfitting. The issue is that OP wants to "overload" a particular Builder method to not just add to what is being built, but also persist the "built" result. Now, granted, he states in the original question (that I overlooked) that he doesn't want to add any "noise", so my solution is invalid for his (atypical seeming) use case. – Barramunda

Answer 7

Apart from using Diezel to generate the whole set of interfaces, is to force them getting the "token" object:

    Grant.permissionTo( permissionManager.User( userId ).permissionTo( Right.READ ).item( docId ).asOf( new Date() ) );

users won't be able to finish the statement until the last/exit method returns the right type. The Grant.permissionTo can be a static method, statically imported, a simple constructor. It will get all it needs to actually register the permission into the permissionManager, so it doesn't need to be configured, or obtained via configuration.

Folks at Guice uses another pattern. They define a "callable", that is used to configure permission (in Guice it's all about binding instead).

    public class MyPermissions extends Permission{

    public void configure(){
    grantUser( userId ).permissionTo( Right.READ ).item( docId ).asOf( new Date() );
    }

    }

    permissionManager.add(new MyPermissions() );

grantUser is a protected method. permissionManager can ensure that MyPermissions only contains fully qualified permissions.

For a single permission this is worst than the first solution, but for a bunch of permission it's cleaner.

Banting