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

[JENKINS-38048] Relax nullability on Item/ItemGroup context parameters #68

Merged
merged 3 commits into from Oct 28, 2016

Conversation

Projects
None yet
3 participants
@jglick
Copy link
Member

commented Sep 8, 2016

[JENKINS-38048] Relax nullability on Item/ItemGroup context parameters.
The underlying checks already handle null values.
Makes it easier to provide credentials dropdowns in global configuration.
@reviewbybees

This comment has been minimized.

Copy link

commented Sep 8, 2016

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

@stephenc
Copy link
Member

left a comment

My initial feeling is that this is a direction I do not want the API to take

@jglick

This comment has been minimized.

Copy link
Member Author

commented Sep 21, 2016

Can you explain why?

These newly introduced methods take a @Nonnull parameter, then do nothing with it except delegate to longstanding methods accepting a @Nullable parameter (with an IMO sensible meaning of looking only in global scope).

@stephenc

This comment has been minimized.

Copy link
Member

commented Sep 21, 2016

Well if we allow null then we will get people passing null and then the null needs to be typecast... plus I now believe it was a mistake to allow the previous APIs to accept null rather than forcing people to be explicit.

I am open to debate on this subject. Just my initial push-back

@jglick

This comment has been minimized.

Copy link
Member Author

commented Sep 21, 2016

forcing people to be explicit

Well instead of

return new StandardListBoxModel()
        .includeEmptyValue()
        .includeMatchingAs(
                project instanceof Queue.Task
                        ? Tasks.getAuthenticationOf((Queue.Task) project)
                        : ACL.SYSTEM,
                project,
                StandardUsernameCredentials.class,
                GitURIRequirementsBuilder.fromUri(url).build(),
                GitClient.CREDENTIALS_MATCHER)
        .includeCurrentValue(credentialsId);

you could inline the implementation of includeMatchingAs to satisfy the existing nullability signatures:

StandardListBoxModel model = new StandardListBoxModel().includeEmptyValue();
model.addMissing(CredentialsProvider.listCredentials(
    StandardUsernameCredentials.class,
    project,
    project instanceof Queue.Task
        ? Tasks.getAuthenticationOf((Queue.Task) project)
        : ACL.SYSTEM,
    GitURIRequirementsBuilder.fromUri(url).build(),
    GitClient.CREDENTIALS_MATCHER)));
return model.includeCurrentValue(credentialsId);

and then to satisfy proposed stricter nullability you would have to say

StandardListBoxModel model = new StandardListBoxModel().includeEmptyValue();
Authentication authentication = project instanceof Queue.Task
        ? Tasks.getAuthenticationOf((Queue.Task) project)
        : ACL.SYSTEM;
List<DomainRequirement> domainRequirements = GitURIRequirementsBuilder.fromUri(url).build();
CredentialsMatcher matcher = GitClient.CREDENTIALS_MATCHER);
model.addMissing(project != null ?
    CredentialsProvider.listCredentials(
        StandardUsernameCredentials.class,
        project,
        authentication,
        domainRequirements,
        matcher) :
    CredentialsProvider.listCredentials(
        StandardUsernameCredentials.class,
        Jenkins.getInstance(),
        authentication,
        domainRequirements,
        matcher));
return model.includeCurrentValue(credentialsId);

which does not feel like an improvement to me, much as I also like explicit APIs.

@stephenc

This comment has been minimized.

Copy link
Member

commented Oct 10, 2016

Compromise suggestion... make the ItemGroup variants can be @Nullable the Item cannot... I am open to vice-versa if you feel that makes more sense...

@jglick

This comment has been minimized.

Copy link
Member Author

commented Oct 10, 2016

make the ItemGroup variants can be @Nullable the Item cannot

Well the motivating case (as shown above) involves a null Item.

I am open to vice-versa

Why would it make sense to allow null in some calls but not others? That sort of inconsistency is the whole point of this PR.

jglick added a commit to jglick/git-plugin that referenced this pull request Oct 24, 2016

@jglick

This comment has been minimized.

Copy link
Member Author

commented Oct 27, 2016

Conclusion with @stephenc:

  • for the short term, let Item overloads be @Nullable, but ItemGroup overloads remains @Nonnull
  • file an issue to provide a better system:
    • new methods (not overloads) taking @Nonnull ModelObject
    • for non-Item/ItemGroup, currently just fall back to Jenkins, but later could handle User, Node, etc.
    • introduce a helper method for Authentication based on item with possible defaults:
project instanceof Queue.Task ? Tasks.getAuthenticationOf((Queue.Task) project): ACL.SYSTEM
@@ -467,7 +468,7 @@
* @since 2.1.0
*/
public AbstractIdCredentialsListBoxModel<T, C> includeMatchingAs(@NonNull Authentication authentication,
@NonNull Item context,
@Nullable Item context,

This comment has been minimized.

Copy link
@jglick

jglick Oct 27, 2016

Author Member

All other variants patched here wind up delegating to this one, which uses context only to call CredentialsProvider.listCredentials, which accepts a @Nullable Item.

@jglick

This comment has been minimized.

Copy link
Member Author

commented Oct 27, 2016

Made the requested short-term change and filed JENKINS-39324 for the rest.

@stephenc stephenc merged commit 408765b into jenkinsci:master Oct 28, 2016

1 check passed

Jenkins This pull request looks good
Details

@jglick jglick deleted the jglick:global-credentialsId-JENKINS-38048 branch Oct 28, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.