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

Specify property name in exception message on nested type build failure #411

Open
petringo opened this issue Mar 28, 2019 · 2 comments
Open

Comments

@petringo
Copy link

petringo commented Mar 28, 2019

When a nested buildable type fails to build, exception is passed as is, giving no hint in exception message of failed property.

For example

@FreeBuilder
public abstract class TopLevel {
    public static void main(final String[] args) {
        new TopLevel.Builder()
                .mutateNested1(builder -> builder.requiredProperty(true))
                // leave nested2.requiredProperty unset
                .build();
    }

    public abstract Nested nested1();
    public abstract Nested nested2();

    public static class Builder extends TopLevel_Builder {}

    @FreeBuilder
    public abstract static class Nested {
        public abstract boolean requiredProperty();

        public static class Builder extends TopLevel_Nested_Builder {}
    }
}

outputs when generated by FreeBuilder 2.3.0:

Exception in thread "main" java.lang.IllegalStateException: Not set: [requiredProperty]
	at example.TopLevel_Nested_Builder.build(TopLevel_Nested_Builder.java:135)
	at example.TopLevel$Nested$Builder.build(TopLevel.java:23)
	at example.TopLevel_Builder$Value.<init>(TopLevel_Builder.java:254)
	at example.TopLevel_Builder$Value.<init>(TopLevel_Builder.java:240)
	at example.TopLevel_Builder.build(TopLevel_Builder.java:217)
	at example.TopLevel$Builder.build(TopLevel.java:17)
	at example.TopLevel.main(TopLevel.java:11)

To resolve which of the nested properties failed one must go through stacktrace and look at the generated code.

Instead the generated top level class should wrap the exception adding name of failed property, e.g.:

  private static final class Value extends Rebuildable {
    private final Nested nested1;
    private final Nested nested2;

    private Value(TopLevel_Builder builder) {
        // ...
        try {
          this.nested2 = nested2Builder.build();
        } catch (RuntimeException e) {
          throw new IllegalStateException("nested2: " + e.getMessage(), e);
        }
    // ...
@alicederyn
Copy link
Collaborator

Nice idea, though we risk breaking enclosing error-handling code by throwing away the exception type.

@petringo
Copy link
Author

petringo commented Apr 2, 2019

That's true, it's a breaking change. But I would consider an unbuildable nested property as an illegal state of top-level builder (caused by an illegal state of the nested builder), just like an unset property. And in some (typical?) cases it conceptually is unset, like nested2 above. From this perspective just passing nested builder errors is inconsistent to handling of concretely unset properties, making this issue look like a bug rather than an enhancement.

In cases where retaining compatibility with enclosing error-handling code is required, I think all build() methods might be overridden to catch the exception and rethrow the cause (recursively if nested builders are out of own control). To identify such exceptions, the message could (and probably should anyway) be formatted something like Unbuildable: <property-name>; <cause-message>.

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

No branches or pull requests

2 participants