Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow builder methods to be protected #1078

Open
cslee00 opened this issue Aug 30, 2019 · 1 comment
Open

Allow builder methods to be protected #1078

cslee00 opened this issue Aug 30, 2019 · 1 comment

Comments

@cslee00
Copy link

cslee00 commented Aug 30, 2019

Generated builders expose public methods to set attributes; in cases where the Builder is sub-classed (in combination with some other flag), make those methods protected such that only the subclass builder can access them. This allow the subclass builder to normalize data, enforce various rules & expose a use-case / domain-specific / higher-level API - while still generating the same immutable object.

Example:

This immutable template:

@Value.Immutable
public abstract class AbstractPolicyStatement {
    public abstract List<String> actions();

    public abstract List<String> notActions();

    public abstract List<String> resources();

    public abstract String effect();

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

    @SuppressWarnings("ResultOfMethodCallIgnored")
    public static class Builder extends ImmutablePolicyStatement.Builder {
        public Builder anyAction() {
            return resource("*");
        }

        public Builder anyResource() {
            return resource("*");
        }

        // addResources( ... ) remains publicly available
        public Builder resource( String resource ) {
            return addResources(resource );
        }

        // effect( String ) remains publicly available
        public Builder allow() {
            return effect("Allow");
        }

        // effect( String ) remains publicly available
        public Builder deny() {
            return effect("Deny");
        }

        public Builder denyAll() {
            anyAction();
            anyResource();
            return deny();
        }
    }
}

Used this way:

void example() {
        // issues:
        //  type safety (partially fixed by using enums in some cases)
        //  inconventient: attributes set one by one;
        //  lower-level, not necessarily reflective of domain (as attributes can only be set one-by-one, can't model
        //      higher-level combinations/use-cases
        var usingRawBuilder = ImmutablePolicyStatement.builder()
                .effect("Deny")
                .addResources("*")
                .addActions("*")
                .build();

        // simpler, reflects domain, addresses type safety & contextual requirements
        var usingFriendlyBuilder = ImmutablePolicyStatement.builder()
                .denyAll()
                .build();

        // however... base builder methods still exposed
        var usingFriendlyBuilder2 = ImmutablePolicyStatement.builder()
                .denyAll()
                .addActions("someAction")
                .build();
    }

...almost gets us there but exposes the raw builder methods to all consumers, which may not be desirable in all cases. It gets increasingly worse for more complex object, such as ones that have Multimap attributes, etc (required to match domain model / support JSON marshalling, but not a small/safe builder api)

Perhaps something like this on the subclass builder:

@Builder.AttributeVisibility(PROTECTED)
    public static class Builder extends ImmutablePolicyStatement.Builder {
        public Builder anyAction() {
            return resource("*");
        }
@asereda-gs
Copy link
Member

I'm thinking if it makes more sense to define visibility by overriding the method directly on the builder:

 public static class Builder extends ImmutablePolicyStatement.Builder {
        @Override // copy visibility to parent (not-yet-generated Builder)
        Builder resource(String resource) {
             return super.resource(resource);
        }

        public Builder anyAction() {
            return resource("*");
        }

This way you can selectively modify visibility of each attribute in the builder.

Another thought I have is if it makes more sense to define two builders: public and private. Composition is usually better than inheritance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants