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

Remove Lombok Delegate from WebTargetHelper #859

Closed
sleberknight opened this issue Jan 29, 2023 · 0 comments · Fixed by #860
Closed

Remove Lombok Delegate from WebTargetHelper #859

sleberknight opened this issue Jan 29, 2023 · 0 comments · Fixed by #860
Assignees
Labels
enhancement A request for change or improvement to an existing feature
Milestone

Comments

@sleberknight
Copy link
Member

sleberknight commented Jan 29, 2023

WebTargetHelper uses Lombok's experimental @Delegate to delegate to a WebTarget instance. This causes various usage limitations as describes in the Javadoc, with the most significant one being the class does not implement WebTarget, which in turns means you cannot chain methods except those defined in WebTargetHelper. Once a method from WebTarget is called, then the return type becomes WebTarget, and you can no longer chain WebTargetHelper methods.

The impetus for finally removing these restrictions is that CodeQL (at least when using the advanced-security/delombok action as of this writing) does not properly delombok code that uses @Delegate which causes compilation errors. The delegated methods are - for some reason - not in the delomboked source code. This is not the case when running Delombok via the IntelliJ plugin, via the Lombok Maven plugin, or running it manually via the Lombok JAR file.

If we remove the @Delegate we gain a few benefits. First, if they remove it in a future version (which the docs say might happen, since it is experimental and not likely to move to stable due to difficulty maintaining it), then we are not impacted. Second, the CodeQL action can work since the code will correctly compile. And third, we can change WebTargetHelper to implement WebTarget and thus remove all the usage restrictions since we'll be able to return WebTargetHelper from the methods that return WebTarget, e.g.

public class WebTargetHelper implements WebTarget {

    // ...

    // This delegates to the wrapped/decorated WebTarget, and returns WebTargetHelper
    @Override
    public WebTargetHelper path(String path) {
        this.webTarget.path(path);
        return this;
    }

    // ...
}

Thus all the chaining restrictions "magically" go away. So this third benefit is probably, in reality, the most important.

The main downside is that if WebTarget adds new methods or changes existing ones, including removal, our code won't compile anymore. But this might be a Good Thing, since then there are no surprises at runtime.

Another, more annoying, downside, is that we'll need to test each of the delegated methods. Again, though, this is probably good in the long run.

For now, leave the @Beta annotation on this class because we will want to verify it works as expected and there as no "gotchas" that I'm not thinking about.

@sleberknight sleberknight added the enhancement A request for change or improvement to an existing feature label Jan 29, 2023
@sleberknight sleberknight added this to the 2.5.0 milestone Jan 29, 2023
@sleberknight sleberknight self-assigned this Jan 29, 2023
@sleberknight sleberknight added the in progress A task that is actively being worked on label Jan 29, 2023
sleberknight added a commit that referenced this issue Jan 29, 2023
* Replace Lombok Delegate on the wrapped WebTarget with "real" code
* Make this class implement WebTarget
* Update class-level Javadocs to remove the "Limitations" section
* Fix method javadocs that incorrectly stated "this" is returned
* Fix a few minor grammatical errors in comments

Closes #859
@sleberknight sleberknight removed the in progress A task that is actively being worked on label Jan 29, 2023
sleberknight added a commit that referenced this issue Jan 30, 2023
* Replace Lombok Delegate on the wrapped WebTarget with "real" code
* Make this class implement WebTarget
* Update class-level Javadocs to remove the "Limitations" section
* Fix method javadocs that incorrectly stated "this" is returned
* Fix a few minor grammatical errors in comments

Closes #859
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A request for change or improvement to an existing feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant