projectlombok / lombok

Very spicy additions to the Java programming language.
https://projectlombok.org/
Other
12.93k stars 2.4k forks source link

[FEATURE] "simple" @PostBuild annotation #2477

Open MichelMunoz opened 4 years ago

MichelMunoz commented 4 years ago

Description The idea is to have a basic, simple, ability to validate (or post-process) an object created by a builder without polluting lombok @Builder with non-building problems ; to do so :

Precisions/Rationale/Benefits

Background

Stupid-proofing To detect invalid usage at compile-time :

assafmlr commented 4 years ago

Is this feature still open? Can I take a look as a new contributor?

Rawi01 commented 4 years ago

If I have not missed anything important the @PostGeneratedConstructor feature can be used for this one. The PR (https://github.com/rzwitserloot/lombok/pull/2522) is there, it just needs to be merged. To be honest I have not tried if it works together with @Builder.

MichelMunoz commented 4 years ago

As far as I understand @PostGeneratedConstructor, you simply annotate the method to call, and it will be called from any generated constructor.

My proposal had several simplifying constraints (eg. no checked exceptions, etc.), but most importantly the behavior is guaranteed and does not change upon subsequent modification of non-lombok related code (the same way @Builder tolerates auto generated constructors or developper-provided ones without changing the behavior of the code and without hidden side-effect).

With a PostGeneratedConstructor or a #1207 approach you have one real-life case of silent chnange in behavior : a class has initially only one generated all args + PostGeneratedConstructor (or #1207) then, later, a custom constructor is added by another developper : no warning, no error, but at runtime the validation (or method to call) is called or not depending on which constructor is called ; aka a subtle regression, that can comme up very late in testing or worse, after delivery of product.

This predictability and stability of behavior is (one of the reasons) why I framed my description as I did ; the Builder annotation is one of those cases where Lombok can guarantee that the validation will always be called, whatever the type of constructor used by lombok. Other Lombok annotations have this same nice quality of "behavior stability" against modification of code that is non-lombok related (+ edit/compil time warning in IDE if something is off, eg. you manually overwrite an auto-gen getter/setter)

To transpose that to your approach : I don't know if there's a nice way to at least add compile/edit time warnings to @PostGeneratedConstructor, so that the situation underlined above can be made apparent to the developpers. Eg. something like

If I'm not clear, I apologize, do not hesitate to ask for clarifications.

Rawi01 commented 4 years ago

The idea of @PostGeneratedConstructor is to handle the most common cases that right now can only be handled by replacing the generated constructor. The provided example is one of these cases where the annotation would solve the problem.

You are right that a custom constructor might behave different but thats actually the reason why Generated is part of the annotation name. Lombok cannot know if you want to call the annotated method or not, what it does is throwing an error if there is no generated constructor at all.

I think that in most @Builder related cases the annotation should work. If you use the default constructor generated by @Builder everything should be fine. If you add a custom constructor later on, lombok will show a compilation error.

It is also possible to override the build() method so it can not be guranteed that it calls @PostBuild

MichelMunoz commented 4 years ago

It is also possible to override the build() method so it can not be guranteed that it calls @PostBuild.

Well, TIL, damn.

Lombok cannot know if you want to call the annotated method or not,

It was more the following, the annotation being the marker of the "intent", ex.:

Then you have 3 cases:

But, if its not technically possible, that settles it, then.

@PostGeneratedConstructor it is.

phiferd commented 3 years ago

Here's one workaround that doesn't require too much additional effort and is compile time compatible with existing code.

@Builder(buildMethodName = "unsafeBuild")
public class Person {
    private final String firstName;
    private final String lastName;
    @Builder.Default private final int age = -1;

    private Person validate() {
        if (this.age <= 0) {
            throw new IllegalStateException("Age cannot be less than zero");
        }

        return this;
    }

    static class PersonBuilder {
        public Person build() {
            return unsafeBuild().validate();
        }
    }
}

This also allows for enforcing that primitives are set (as long as there is some invalid default value). https://github.com/rzwitserloot/lombok/issues/2291

You can also add @AllArgsConstructor(access = AccessLevel.PRIVATE) to prevent direct access to the constructor.

phiferd commented 3 years ago

Actually, wouldn't this be a simple change in the BuilderHandler to support validated build()?

Option 1 (simplest): Private Build Method Allow the access modifier of the build method to be specified as private. This would force the user to create their own public build method, where any sort of pre/post build validation could be done. In this case, Lombok doesn't need to make any new guarantees about validation, but instead provides a way for users to customize the Builder class in a safe way (i.e. only exposing the build method that is safe).

@Builder(buildMethodName = "unsafeBuild", buildMethodAccess = AccessLevel.PRIVATE)
public class Person {
   // ...
   static class PersonBuilder {
        // Example method coded by user, not auto-generated.
        public Person build() {
            Person person = unsafeBuild();
            // do whatever validation you want, however you want.
            return person;
        }
    }
}

Option 2 (more user friendly): Lombok handles validation call If there is a validation target method (either @PostBuild or by adding @Builder(buildValidationMethodName="validate"), then the BuilderHandler generates a private unsafeBuild method as well as a public build method:

// This is generated in the Builder if a validation method is specified. 
// If there is already a build method, lombok should generate a compile time error. 
// Users control unsafeBuild and the validate method, but not the auto-generated build method, 
// since lombok would not be able to guarantee validation otherwise
public Person build() { /* this is whatever the builderMethodName is */
    Person unsafe = unsafeBuild();
    unsafe.validate(); // throw RuntimeException if validation fails
    // Person.validate(unsafe); // if the validation method is static
    return unsafe;
 }

// Auto generated, unless specified by the user.
private Person unsafeBuild() {   
   // normal auto-generated build method
}
rzwitserloot commented 2 years ago

@phiferd

Hack 1: buildMethodName = unsafeBuild()

Having any method named 'unsafe' anywhere seems crazy to me. This feature needs to make sense, not to scream 'HACK!!!!' right from the get go. Naming the builder method unsafeBuild is a non-starter, so, I'm open for more obvious/direct support for builder validation.

Hack 2: Handroll the build() method

No can do. For example, our build method does some fairly crazy shenanigans in order to provide you with optimally 'packed' maps (once you involve @Singular, it should be obvious why we cant let you write it yourself.

I don't like either of these directions. A method that does validation seems simplest to me, but, there are many ways to dice that particular pineapple:

This allows transformations in addition to validation, and keeps methods in that sense simpler - they just deal with 1 parameter at a time. But it then makes it impossible to do inter-param validation, i.e. imagine that you have:

class Person {
  LocalDate birthDate;
  Long socialSecurityNumber;
}

Where the rule is that all persons must have a valid SoSec unless they are underage at the time of creation of this object, in which case it must in fact be null. This, too, seems simple enough:

@Validator
static void validateSocialSec(LocalDate birthDate, Long socialSecurityNumber) {
  if (not valid) throw something
}

By treating the parameter names as magic (they must match your field names, or if you stuck @Builder on parameters, the param names), we know what to pass. Transformation (where the method may return something else in addition to the usual functionality, which is to silently do nothing if input is okay, and throw something if it is not) is allowed only one single arg validators.

We can even make a rule that you could have a validator method that takes in the very object you're creating:

@Validator
static void validate(Person self) {}

// or even
void validate() {}

Where the non-static form must have no args, and the static forms must have at least one, whose name and type either matches one of the 'params/fields', or, is of type "self" and then the name is mostly irrelevant - it just can't match any of your field names, or we'd interpret it as that (and then it'll probably have the wrong type).

I'd want a clear outlay of ideas and a plan that seems to lead to the best combination of flexibility, ease of use, ease of 'understanding' (if Jane Q. random coder looks at it, how high are the odds that she can figure out without much hassle what the code is probably intending to do, even if Jane is confused about how it would work, which presumably she would be if she isn't aware Lombok exists), and ease of writing (lets not have a boilerplatebusting tool demand you write rather a lot of boilerplate!)

Once we have that down, we can think of stuff to build. I dislike just getting on with the first thing you can think of / the thing that would work for your projects – because once you release something you set the tone and it may have closed doors (by forcing a backwards incompatible chance or a complete rewrite which is very much backwards incompatible as well of course).

Rawi01 commented 2 years ago

I still think that @PostConstructor cover most of the usecases. Field level validation/transformation before object creation is the only thing that is not possible this way. Did I miss something?

I also think that it is pretty easy to understand that a methode annotated with @PostConstructor gets called after the object creation. With a proper name (e.g. validate) almost everyone should be able to understand what the method does.

rzwitserloot commented 2 years ago

The idea of @PostConstructor being that you can also do other stuff, say, log something perhaps. I see that.

We're back to the core issue though (and I recall discussing this earlier - maybe there's another issue floating around?) - we cannot actually do any of this, at least, not if the intent is: "Ensure this is called for any and all constructors". We can't easily inject a call to this stuff in your constructor. @PostGeneratedConstructor would in turn solve that issue, though, right?

Biggest feel-bad is naming this @PostGeneratedConstructor now and regretting that choice because later we instead would have wanted to just do @PostConstructor and put in the effort to find a way to always invoke.

So let's explore always-invoke (that is, find all non-lombok-generated constructors and add there as well):

That seems simple enough.

public HandWrittenCtor(int param) {
  try {
    handWrittenCode(); // all other lines in the body are lombok-generated.
  } finally {
    postConstructor();
  }
}

But this would invoke the post constructor even if you abort-via-exception, and any exceptions occurring in the postConstructor would then replace the exception thrown in your hand written code, which sounds like a really, really bad idea. We can go hunt for every return statement and set a boolean flag, but we can't set the boolean flag 'at the end of the natural block'. After all, imagine:

public MyConstructor() {
  switch (a) {
    case 1:
      stuff();
      return;
    default:
      return;
  }
}

If we generate $normalExit = true; at the end of this, it's a compiler error - unreachable code. Which means we need to either find a way:

All of which sounds extremely complicated.

If only you could figure out what the 'pending' throwable is in a finally block. But you can't, as far as I know.

dstango commented 2 years ago

If only you could figure out what the 'pending' throwable is in a finally block. But you can't, as far as I know.

How about:

public HandWrittenCtor(int param) {
  Throwable pendingThrowable = null;
  try {
    handWrittenCode(); // all other lines in the body are lombok-generated.
  } catch (Throwable t) {
    pendingThrowable = t;
    throw t;
  } finally {
    if (pendingThrowable == null) {
      postConstructor();
    }
  }
}
rzwitserloot commented 2 years ago

Oh, interesting. Uh.. yeah that works. Oof that's ugly. But it works. Should we... go for that? Presumably the number of times you add @PostConstructor whilst also having your own constructors is not all that common. It will pile that cruft into your code (and might trigger linter tools), and will pile that cruft into your delomboked code as well, which is a bit of a downside.

Rawi01 commented 2 years ago

@rzwitserloot Thats what my PR does if it detects a return. In all other cases it adds the method calls to the end.

rzwitserloot commented 2 years ago

Ah, there's a PR, 🤦 - that's why all this sounds so familiar.

phiferd commented 2 years ago

Having any method named 'unsafe' anywhere seems crazy to me.

Fair enough.

There are two things that need to be done: build and validate. If the expectation is that the build() method will return a validated object, then it needs to validate before it returns. That's why I suggested breaking it into two pieces:

  1. A private method that does what the build method does today (just constructs an object). Call it what you want, but it has to be different than the public method.
  2. A public method that calls the private method, validates the result, and then returns it.

I don't see any fundamental problem with the approach and it works pretty well for me already. Currently, however, there is no way to make the build method private, which means I'm left with an extra public method on my builder class that isn't supposed to be called. Having the option to set buildMethodAccess = AccessLevel.PRIVATE seemed like a low risk, backward compatible change that would allow for a clean pattern for inserting build validation logic. Maybe I'm missing something here.

If Lombok implements the @PostBuild feature, sure -- no need for an extra method if you want to just add the call to the PostBuild method at the end of the build method. Not an approach I can apply today, though.

As for @PostConstructor ... it sounds like a nice feature, but it also sounds a lot more complex than the original @PostBuild request. What will Lombok guarantee?

e.g.

Guarantees about some sort of @PostBuild annotation seem more manageable. e.g. "If the class annotated with @ Builder has a method annotated with @PostBuild, the auto-generated build method will guarantee the PostBuild method is called exactly once before returning the new instance to the caller".

trumpetinc commented 2 years ago

I agree with phiferd on this.

There is a large population of Lombok users today who are really struggling to properly implement the builder pattern when there is anything beyond the most simple of cases, and I think it is a very good idea to put the focus on solving those problems - especially if the solution is simple and not controversial.

Having buildMethodAccess = AccessLevel.PRIVATE added to @Builder would be more than sufficient, and seems like it would be simple to implement, forwards and backwards compatible, and very low risk.

Our code has so many places where we are having to hand-write build() when Lombok could absolutely be doing it for us - if we could just make the generated method private. Adding a field to our builder classes requires changes in multiple places. If the order of the fields change (and therefore the order of the fields in the constructor changes) it's an even bigger nightmare. We are making design tradeoffs between "do we take the risk of things silently breaking when we add or adjust fields? or do we try to do code review to prevent anyone from calling the wrong build() method?". As an architect, these are not tradeoffs that I am happy having to make.

FWIW - I believe that the buildMethodAccess idea was proposed in October 2017 (https://github.com/projectlombok/lombok/issues/1489 ).

A @PostBuild (or @PreBuild) annotation would be a little cleaner as it would mean we wouldn't have to actually declare the builder class. But the real need is to make the build() method private so we can add our own public method that validates/transforms then delegates to the private Lombok generated build() method.

Long and short: @PostBuild (and similar) are nice-to-haves. Controlling access level of the build method is a game changer that will make @Builder actually usable for tons of cases where it is not being helpful today.

On Sun, Apr 10, 2022 at 1:39 PM phiferd @.***> wrote:

Having any method named 'unsafe' anywhere seems crazy to me.

Fair enough.

There are two things that need to be done: build and validate. If the expectation is that the build() method will return a validated object, then it needs to validate before it returns. That's why I suggested breaking it into two pieces:

  1. A private method that does what the build method does today (just constructs an object). Call it what you want, but it has to be different than the public method.
  2. A public method that calls the private method, validates the result, and then returns it.

I don't see any fundamental problem with the approach and it works pretty well for me already. Currently, however, there is no way to make the build method private, which means I'm left with an extra public method on my builder class that isn't supposed to be called. Having the option to set buildMethodAccess = AccessLevel.PRIVATE seemed like a low risk, backward compatible change that would allow for a clean pattern for inserting build validation logic. Maybe I'm missing something here.

If Lombok implements the @PostBuild feature, sure -- no need for an extra method if you want to just add the call to the PostBuild method at the end of the build method. Not an approach I apply today, though.

As for @PostConstructor ... it sounds like a nice feature, but it also sounds a lot more complex than the original @PostBuild request. What will Lombok guarantee?

e.g.

  • what if one constructor calls another -- how many times will the validation method be called? Remember that constructors can call other constructors and change state afterward.
    • what if the validation method has a side-effect, such as logging metrics?
    • what if validation is expensive or requires a call to an external system?
  • Lombok will not be able to prevent other classes from getting references to instances that haven't had their @PostConstructor method invoked. Will that be the expectation of the average developer?

Guarantees about some sort of @PostBuild annotation seem more manageable. e.g. "If the class annotated with @ Builder has a method annotated with @PostBuild, the auto-generated build method will guarantee the PostBuild method is called exactly once before returning the new instance to the caller".

— Reply to this email directly, view it on GitHub https://github.com/projectlombok/lombok/issues/2477#issuecomment-1094364428, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABSP46AV4B6UZUTK2XXN24TVEM36RANCNFSM4NLGRXJQ . You are receiving this because you are subscribed to this thread.Message ID: @.***>

rzwitserloot commented 2 years ago

I don't think this is a good idea to try to smash in there for the 1.18.24 release, but especially given there's a PR, I'm aiming for this to be addressed in the version after that.

MarkHesk commented 1 year ago

Any news when this will be implemented?

fabiencelier commented 1 year ago

Long and short: @PostBuild (and similar) are nice-to-haves. Controlling access level of the build method is a game changer that will make @builder actually usable for tons of cases where it is not being helpful today.

Totally agree with this conclusion !

matt-isaman commented 1 year ago

Chiming in here after looking to convert some classes I have from the Immutables library to Lombok. In Immutables they have a @Check annotation that functions a lot like the suggested @PostBuild annotation above.

The code they end up generating when that annotation is present does more or less the following:

public MyClass {
    // ...
    public Builder {
        // ...
        public MyClass build() {
            var instance = new MyClass(this);
            instance.validate();
            return instance;
        }
    }
}

For added context, in my case I am trying to have built in Bean Validation for these classes so the validate method is this:

@Check
default void validate() throws ConstraintViolationException {
    try (var validatorFactory = Validation.buildDefaultValidatorFactory()) {
        var violations = validatorFactory.getValidator().validate(this);
        if (!violations.isEmpty()) { 
            throw new ConstraintViolationException(violations);
        }
    }
}

I can write the Builder::build() method more or less as I have it listed above and everything works with Lombok. But I think it would be really nice to also be able to replace that @Check annotation with some Lombok equivalent and remove even more boilerplate.

Even better if this also plays nice with the @Jacksonized annotation and does the validation after the class is deserialized and built as well.

MichelMunoz commented 1 year ago

@trumpetinc said:

Long and short: @PostBuild (and similar) are nice-to-haves. Controlling access level of the build method is a game changer that will make @builder actually usable for tons of cases where it is not being helpful today.

I agree, and, more than that, it does cover the @PostBuild (and similar) needs / benefits as well.

Proof 1, the simple case, one class ; my initial example would become something like :

@AllArgsConstructor(access = AccessLevel.PRIVATE)
@Builder(buildMethodName = "rawBuild", buildMethodAccess = AccessLevel.PRIVATE)
public MyClass {
    private String someField;
    ...

    public static class MyClassBuilder {
        public MyClass build() {
            MyClass someInstance = rawBuild();

            // do validation, etc. stuff here

            return someInstance;
        }
    }
}

Which, except for the buildMethodAccess option, is already possible in current lombok.

And that covers most of the expected benefits initially expressed:

Proof 2 sub-classing ie. having a sub class adding field leveraging on parent class builder and validation, aka @SuperBuilder. Some capabilities seems necessary to apply the same pattern:

The solution, in the case of subclassing, with just the new buildMethodAccess. The parent :

@SuperBuilder(buildMethodName = "rawBuild", buildMethodAccess = AccessLevel.PRIVATE)
public class MyClass {
    private String someField;

    public static abstract class MyClassBuilder<C extends MyClass, B extends MyClassBuilder<C, B>> {
        public MyClass build() {
            MyClass someInstance = rawBuild();
            validate(someInstance);
            return someInstance;
        }

        protected void validate(MyClass someInstance) {
            // do validation, etc. stuff here
        }
    }
}

and the child :

@SuperBuilder(buildMethodName = "rawBuild", buildMethodAccess = AccessLevel.PRIVATE)
public class MySubClass extends MyClass {

    private String someOtherField;

    public static abstract class MySubClassBuilder<C extends MySubClass, B extends MySubClassBuilder<C, B>> extends MyClassBuilder<C, B> {
        public MySubClass build() {
            MySubClass someInstance = rawBuild();
            validate(someInstance);
            return someInstance;
        }

        protected void validate(MyClass someInstance) {
            super.validate(someInstance); // here call super validation
            // do specific validation, etc. stuff here
        }
    }
}

Overall, just adding buildMethodAccess also covers the needs for post-build processing, for the simple case and for the subclassing case