[JENKINS-53675] Add the ability to disable checks for certain classes only

[agentgonzo]

https://issues.jenkins-ci.org/browse/JENKINS-53675

Add an @DisableRestriction annotation that can be used at either the class or method level to disable @Restricted checks on a per-class basis.

Example use:

@DisableRestriction(RestrictedApi.class)
public class Caller extends ApiWithRestrictedMethodAndField { // This is fine, ApiWithRestrictedMethodAndField itself is not restricted

    private RestrictedApi restrictedApi;

    @DisableRestriction(ApiWithRestrictedMethodAndField.class)
    public Caller() {
        ApiWithRestrictedMethodAndField.notReallyPublic(); // illegal but check disabled at the method level
    }

    @DisableRestriction({RestrictedApi.class, ApiWithRestrictedMethodAndField.class}) // Duplicates class-level annotation, but included as an example.
    private void invalidFieldUse() {
        restrictedApi.field = null;
        super.field = null;
    }

For more use, see it/disable-restrictions integration test

@jenkinsci/code-reviewers @amuniz @jtnord

[jtnord]

you probably want to change this :)

[jtnord]

looks ok apart from the javadoc which looks wrong to me.

[jtnord]

this looks wrong

[jtnord]

as does this.

[jtnord]

easier to review the pre-refactored diff

[agentgonzo]

@jtnord - fixed the javadoc/headers and changed a bit because it wasn't quite working

[amuniz]

LGTM 👍

[agentgonzo]

@jglick @ndeloof @kohsuke - any chance you could take a look at this please?

[kohsuke]

JIRA appears unresponsive so I can't see the upstream discussion, so my comment is without the context.

The central design idea of access modifier annotation like @Restricted is that it's an extension and generalization of standard access modifiers like public and private -- it's an intent declared by the code author what other people should or should not be able to do.

Also, when I think about other programming languages that have more complicated access modifiers, like friend in C++ or Java module systems, they all still maintain the idea that the discretion of determining the access is on the side of the callee, not on the caller.

So from that perspective, this mechanism of allowing arbitrary caller to ignore that intent seems to defeat the purpose, and I don't see any reference in code/documentation that explains how/why this should be used, so that makes me feel uneasy.

With all that said, I'm coming in late, and like I said I don't have the context of JENKINS-53675 so I'm blind to the problem that led to this solution. And because of that, I can't offer more constructive suggestion, and I hate not being able to do that. This feedback questions the approach itself, and it's too late for that.

So if others with more context see the logic in this, don't let my -1 stand in the way.

[ndeloof]

As a plugin maintainer, while I've annotated my code with @Restricted I would assume it's never used without my control, and as such I can make backward-incompatible changes without having to worry. (I'd like I could mark my whole plugin as "NoExternalUse" and only opt-in for "None" restriction for what I consider a public API)

JENKINS-53675 unfortunately focus on the technical requirement, not the use-case behind this.

[agentgonzo]

Whilst I agree with the comments above, we have hit the issue where we need to use some @Restricted classes.
There are always exceptions, and in the case of private/protected members there are mechanisms to circumvent the protection when needed (ReflectionUtils, subclassing to expose protected members etc). Whilst these are not desirable, they are commonly used as the only way to achieve the functionality needed.

In our specific case, we want to:

• Being able to call InstallUtil.proceedToNextStateFrom() and Jenkins.getInstallState() to proceed to the next state when hooking into the SetupWizard
• Being able to use DownloadSettings so that we can use the same settings on multiple masters that are kept 'in sync'

When using an @Restricted class, we are acknowledging that it comes with the risks that it may change at any point and break our software (which is why we have firmer tests around these APIs we are not supposed to be using). This is a risk we acknowledge and accept, but have taken the decision to use the Restricted classes as it is best alternative.

At the moment, we are just disabling the access-modifier-checker entirely (via <access-modifier-checker.failOnError>false</access-modifier-checker.failOnError>). This proposal offers a less-sledgehammer approach to saying "I accept that I am violating the contract in this specific case and will bear the consequences should something change upstream" without the danger of having to turn the checker off entirely (which is what we are forced to do at the moment).

[ndeloof]

bypassing @Restricted as well as relying on Field.setAccessible is unfortunately a hack that for sure offers a quick-and-dirty solution, but should come with a PR to ensure the target element is made accessible in a future release, so you can bring a better implementation and limit the risk to suffer from some internal changes.

[amuniz]

Sure, here it is: jenkinsci/jenkins#3617 (with objections too...)

[jtnord]

The important thing here is that we do not want to disable the entire annotation checker. Sure we could have no violations by changing the code to purely use reflection to call the code, but that gives us even less comfort, or we could disable the checker entirely. Given a project using this may be consuming multiple libraries we may only want to accept the risk of using a Restricted api for a library that we ourselves maintain (for example).

bypassing @restricted as well as relying on Field.setAccessible is unfortunately a hack that for sure offers a quick-and-dirty solution, but should come with a PR to ensure the target element is made accessible in a future release, so you can bring a better implementation and limit the risk to suffer from some internal changes.

Using reflection and loosing compile time guarantees is a really bad thing to do, and one that may not be possible in future JVMs.

This is not always possible. for example an upstream has rejected our attempts to remove the Restricted annotation on at least one occaision.

[ndeloof]

The important thing here is that we do not want to disable the entire annotation checker

I get your point. I would suggest to rely on @SuppressWarning for consistency with comparable "break the rule but I know about it" scenario.