Lombok: @Delegate should preserve any annotation from the delegated method

[lombokissues]

_Migrated from Google Code (issue 568)_

[lombokissues]

:bust_in_silhouette: futuretelematics   :clock8: Aug 29, 2013 at 14:06 UTC

What steps will reproduce the problem?

1.Use @ Delegate to forward a method with a GUICE @ Transactional annotation (springs annotations should be an issue either)

2.Any delegated method annotated with @ Transactional at the delegated type looses this annotation and then is not transactional any more

The delegated type with a transactional method:

public class MyTransactionalMethodContainingType {
@ Transactional
public void myTransactionalMethod(...) {
// do something that should be transactional
}
}

The delegated type container:

public class MyContainer {
@ Delegate
private MyTransactionalMethodContainingType _transactionalType;
}

[lombokissues]

_End of migration_

[OrangeDog]

Another use case: building classes that should be validated (javax.validation).

@Data
class EmailRequest {
    private @Email @NotEmpty @Length(max = 254) String email;
}

@Data 
class AuthenticatedRequest {
    private @NotEmpty @Length(32) String userId;
    private @NotNull char[] password;
}

class ChangeEmailRequest {
    private final @Delegate EmailRequest emailRequest = new EmailRequest();
    private final @Delegate AuthenticatedRequest authRequest = new AuthenticatedRequest ();
}

[julien-may]

What's the state here?

[ben-pearson]

I need this too :'(

👍

[marcelhaerle]

I tested this issue with version 1.18.0. The compiled output contained the validation annotations.

import lombok.Data;

import javax.validation.constraints.Min;
import javax.validation.constraints.NotBlank;
import javax.validation.constraints.NotEmpty;

@Data
public class Customer {

    @NotEmpty
    @Min(2)
    private String firstName;

    @NotBlank
    private String lastName;

}

This issue seems to be solved. Or did I misunderstand anything?

[OrangeDog]

@marcelhaerle you're not using @Delegate...

[marcelhaerle]

@marcelhaerle you're not using @Delegate...

But the delegated method still holds the annotation, so it should still be applied when called. Do you expect that the annotations are 'pulled up' to the method that delegates? Could you explain to me, why the transaction won't work when applied to the delegated method? Thanks :-)

[OrangeDog]

@marcelhaerle yes, the annotations need to be pulled up for them to work, hence this issue. These JEE and related technologies check the annotations of declared beans. They do not check the annotations of objects in arbitrary private fields of declared beans.

Perhaps you're thinking of Python where annotations are functions that directly wrap something, instead of being simply typed markers.

[marcelhaerle]

@OrangeDog yes, you're right. Now I understand, that you want the delegating class to be a container-managed component/bean, while the (JEE transactional) annotations are on the delegate, which is not actually container-managed. Sorry, that I did not properly read and understand the issue :-)

My personal opinion is, that this is not how CDI should be used, I should inject CDI beans where I need them and not construct them myself with new, because I'd lose all the benefits of container-managed beans... It might also be confusing to others, when the field with the bean was not injected. What about all the proxied functionality on the not-injected delegate bean? The delegate annotation might not catch them, when there are class-level annotations.

We have never used lombok annotations on CDI beans, but if you do it like this, you're totally right, that the delegate annotation should be enhanced to do so.

[OrangeDog]

It applies to every annotation-driven system, not just those that involve managed beans. JAXB, JPA, @Nullable, etc.

[rzwitserloot]

We can't just arbitrarily 'lift' every annotation; not every annotation is meant to be used like this. We'd need to find a way to identify annotations as 'liftable' (fat chance of that ever happening), or we need to have a list of some sort so we can decide whether or not to do so based on this list.

This list could be a combination of a bunch of hardcoded annotations where we KNOW that lifting them is the right plan + the ability to extend it via lombok.config to add your own.

So, which annotations should be on the hardcoded list?

[negora]

I'm facing the same problem when using the Checker Framework. It uses lots of annotations here and there to make certain compilation-time checks. But @Delegate ignores all them.

So, which annotations should be on the hardcoded list?

@rzwitserloot What about adding a new parameter to @Delegate, named includeAnnotations, that accepts an array of annotation classes? This new feature would expose the annotations that are applied to the following elements:

• To the own methods (it includes the annotations applied to the return types).
• To their type parameters.
• To their parameters or arguments.

Another option would be to add one parameter that does just the opposite, called excludeAnnotations. If it weren't set, Lombok would lift all annotations by default. However, it would break the current behaviour of @Delegate, which is to ignore all annotations.

An advantage of the first proposal is that it would ignore @Override by default, which is an annotation that shouldn't be lifted by default, because the "delegator" class may not be overriding anything.

This proposal only covers the annotations of methods and their parameters. In my opinion, lifting the annotations of the class wouldn't make much sense, because you would need to write them in the includeAnnotations parameter, and that's something that you can do directly in the class declaration.

[negora]

Are there any news about this? Thank you!

[rzwitserloot]

@negora Defaulting to 'just copy em all' is just as bad as 'copy none of them'. These aren't solutions. We can always cover well known annotations such as @Override, that's not too much of a problem. Lombok already has support for listing the nature of annotations in config files, but 95% of users aren't going to bother. For what it's worth, lombok already knows about checkerframework, and already has a built in nature of 'oooh! I recognize that annotation! Better copy it' for various features, so 'make a big list of copyables' sounds like the way to go.

Even includeAnnos is not great; you're now boilerplating the boilerplate, forced to manually add a whole bunch of things to the includeAnnotations annoparam, at which point the effort involved is almost as bad as just manually writing wrappers, defeating the purpose. No, a lombok.config based feature would then be better, at least then you only have to write your list of copyables once.

One problem is that lombok has a generalized 'copyable annotations' concept but the problem is, it doesn't interact all that well with delegate; the copyable annotations concept is more about copying annotations from fields to setters and getters and constructors. In that sense, lombok's name for that feature ('copyableAnnotation') is perhaps poorly chosen.

That gets us to: The seemingly most useful way to do this that avoids backwards compatibility issues or other serious 'use of lombok breaks your stuff' (which is what would happen if lombok default-copies everything) is to use lombok.config, but without a healthy built-in list of well known types from commonly used libraries, we're relying on lombok users to manage such a list, which they wont.

I think this feature can happen... with that list. Which makes it a high-effort feature with a significant maintenance burden.

[negora]

Even includeAnnos is not great; you're now boilerplating the boilerplate, forced to manually add a whole bunch of things to the includeAnnotations annoparam, at which point the effort involved is almost as bad as just manually writing wrappers, defeating the purpose.

Thank you for your opinion. My goal with that annotation parameter was to have a fine-grained control over what annotations were lifted in each place. But you're right: it would be too much boilerplate.

I also considered something like that at the package level, with a new annotation placed in a package-info.java file. But I guess it would not make sense, since you can do the same with a lombok.config file.

That gets us to: The seemingly most useful way to do this that avoids backwards compatibility issues or other serious 'use of lombok breaks your stuff' (which is what would happen if lombok default-copies everything) is to use lombok.config, but without a healthy built-in list of well known types from commonly used libraries, we're relying on lombok users to manage such a list, which they wont.

I guess that the first step would be to enumerate some well-known projects that are frequently used by most users, and that make use of annotations. Then, we can dive in each of them to find the most used annotations.

A start point would be to enumerate those projects that have @Nullable and @NonNull -alike annotations. Because they're the perfect candidates for something like this.

[rzwitserloot]

Lombok already has nullable/nonnull annotation lists built-in. These are copied automatically in type-use, or 'wishes it was type-use' cases: When a field is annotated with e.g. @Nullable, then if lombok generates a setter, the param will get that annotation as well. This just happens; lombok ships with a long list already, but you can use lombok.config to add more if you want.

Here is the full list - ever since the checkerframework has added their annotations (which is a huge list), 'consider the performance implications' is on our todo list, for what it's worth :)

Some fairly exotic ones like org.jmlspecs.annotation.Nullable and androidx.annotation.Nullable are already in here.

Perhaps delegate needs to start looking at this list, but what delegate is doing is a very slightly different take on the notion of 'copyable'. Still, at the very least _these_ should all be copyable: Both if they occur on any parameter, as well as if they occur on the method itself, as well as any type-use anywhere in the signature.

So, just to confirm: Lombok doesn't currently do that, right?

[OrangeDog]

consider the performance implications

Making them sets instead of lists is probably a good idea.

[dborie]

Hello,

Just to be sure, there is still no workaround on this?

I'm using a lot of @Delegate on services to implement methods they have in implemented interface, but not carrying annotation like @org.eclipse.jdt.annotation.Nullable make it unusable in this interface and that's just sad :(

I'm a bit surprised as lombok.copyableAnnotations config seem to work great on many other annotation.

[negora]

@rzwitserloot I'm really sorry, but I thought that I had already answered to your last message, back in December :-S .

So, just to confirm: Lombok doesn't currently do that, right?

That's right. Consider this silly example:

import org.checkerframework.checker.nullness.qual.Nullable;


public interface Person {
    public @Nullable String getName ();
}


public final class BasicPerson
        implements Person {

    private final @Nullable String name;

    public BasicPerson (@Nullable String name) {
        this.name = name;
    }

    @Override
    public @Nullable String getName () {
        return name;
    }

}


public final class Student
        implements Person {

    @Delegate
    private final BasicPerson basic;

    public Student () {
        this.basic = new BasicPerson ("foo");
    }

}

The Student class should have a method with the signature public @Nullable String getName (), but it has this one instead: public String getName (). The @Nullable annotation is lost.

I agree about the idea that Lombok inspects the BASE_COPYABLE_ANNOTATIONS list when processing @Delegate. That way one can control which annotations are safe to be copied to the delegator methods. As long as you can customize such list per project, I think it's the best default behaviour.

@OrangeDog I agree too.

@dborie I think there isn't any yet.