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

[FIXED JENKINS-34691] Add the ability for ItemListeners to veto copy operations #2782

Merged
merged 2 commits into from Mar 15, 2017

Conversation

5 participants
@stephenc
Member

stephenc commented Mar 8, 2017

@reviewbybees

This comment has been minimized.

Show comment
Hide comment
@reviewbybees

reviewbybees Mar 8, 2017

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.

reviewbybees commented Mar 8, 2017

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.

@daniel-beck

This comment has been minimized.

Show comment
Hide comment
@daniel-beck

daniel-beck Mar 8, 2017

Member

Do we really need to reuse a Listener for this?

Member

daniel-beck commented Mar 8, 2017

Do we really need to reuse a Listener for this?

@jglick

jglick approved these changes Mar 8, 2017

Fine so far as it goes but it is not a great solution to the originally stated problem.

@stephenc

This comment has been minimized.

Show comment
Hide comment
@stephenc

stephenc Mar 8, 2017

Member

@daniel-beck I envision other use cases where you may want to do processing before the copy and after, in which case it would make sense to put the functionality in the ItemListener as that already has the onCopied() method.

I was originally calling it beforeCopy but decided to make the check functionality explicit instead

Member

stephenc commented Mar 8, 2017

@daniel-beck I envision other use cases where you may want to do processing before the copy and after, in which case it would make sense to put the functionality in the ItemListener as that already has the onCopied() method.

I was originally calling it beforeCopy but decided to make the check functionality explicit instead

@daniel-beck

This comment has been minimized.

Show comment
Hide comment
@daniel-beck

daniel-beck Mar 8, 2017

Member

@stephenc I understand that, but Listener should be reserved to informing about things, rather than being able to influence them?

I mean, I abused SecurityListener to implement login rate throttling by throwing SecurityExceptions, but that's not what it was meant for (and IIRC @jglick told me it's a bad idea). This seems similar.

Member

daniel-beck commented Mar 8, 2017

@stephenc I understand that, but Listener should be reserved to informing about things, rather than being able to influence them?

I mean, I abused SecurityListener to implement login rate throttling by throwing SecurityExceptions, but that's not what it was meant for (and IIRC @jglick told me it's a bad idea). This seems similar.

@jglick

This comment has been minimized.

Show comment
Hide comment
@jglick

jglick Mar 8, 2017

Member

I abused SecurityListener to implement login rate throttling by throwing SecurityExceptions, but that's not what it was meant for (and IIRC @jglick told me it's a bad idea)

That was a different case—you were throwing unchecked exceptions out of an SPI method which was not documented to allow them and hoping that the firing implementation was not already improved to just log them and continue. This case is about adding a documented contract, and the pattern is at least as old as VetoableChangeListener.

Member

jglick commented Mar 8, 2017

I abused SecurityListener to implement login rate throttling by throwing SecurityExceptions, but that's not what it was meant for (and IIRC @jglick told me it's a bad idea)

That was a different case—you were throwing unchecked exceptions out of an SPI method which was not documented to allow them and hoping that the firing implementation was not already improved to just log them and continue. This case is about adding a documented contract, and the pattern is at least as old as VetoableChangeListener.

@oleg-nenashev

🐛 Since I feel that the Failure class should not be used in this listener, see the comment in the codeline

Show outdated Hide outdated core/src/main/java/hudson/model/listeners/ItemListener.java
for (ItemListener l : all()) {
try {
l.onCheckCopy(src, parent);
} catch (Failure e) {

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Mar 9, 2017

Member

Maybe Error as well, NIT

@oleg-nenashev

oleg-nenashev Mar 9, 2017

Member

Maybe Error as well, NIT

This comment has been minimized.

@stephenc

stephenc Mar 9, 2017

Member

Error does not extend RuntimeException, so that will be propagated as it

@stephenc

stephenc Mar 9, 2017

Member

Error does not extend RuntimeException, so that will be propagated as it

* @throws Failure to veto the operation.
* @since TODO
*/
public void onCheckCopy(Item src, ItemGroup parent) throws Failure {

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Mar 9, 2017

Member

🐛 Failure is the wrong exception class. Javadoc says that it "Represents an error induced by user, encountered during HTTP request processing. ", but from what I see it is not the only use-case for this listener. E.g. it may be used in CLI commands for copying jobs

@oleg-nenashev

oleg-nenashev Mar 9, 2017

Member

🐛 Failure is the wrong exception class. Javadoc says that it "Represents an error induced by user, encountered during HTTP request processing. ", but from what I see it is not the only use-case for this listener. E.g. it may be used in CLI commands for copying jobs

This comment has been minimized.

@stephenc

stephenc Mar 9, 2017

Member

Failure knows how to render the HTML response. Would you be ok if I update the jacadoc of Dailure to reflect its current usage

@stephenc

stephenc Mar 9, 2017

Member

Failure knows how to render the HTML response. Would you be ok if I update the jacadoc of Dailure to reflect its current usage

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Mar 9, 2017

Member

I am not a big fan of it since it is a RuntimeException, hence we potentially get into the error propagation fun. But I will be fine if you create an equivalent non-Runtime exception with support of the error rendering

@oleg-nenashev

oleg-nenashev Mar 9, 2017

Member

I am not a big fan of it since it is a RuntimeException, hence we potentially get into the error propagation fun. But I will be fine if you create an equivalent non-Runtime exception with support of the error rendering

This comment has been minimized.

@stephenc

stephenc Mar 9, 2017

Member

"Represents an error induced by user, encountered during HTTP request processing."

translation, this represents an error - resulting from a user action - that occurred while the HTTP request of the user was being processed.

Here we have a user request - to copy something - and we cannot complete the HTTP request.

Further, the other checks in the call path all throw a Failure, e.g.

throw new Failure(Messages.Hudson_JobAlreadyExists(name));

So we are left with your only objecting being that it is a RuntimeException

Seems overly reaching to require code duplication just to make the exception non-runtime exception

@stephenc

stephenc Mar 9, 2017

Member

"Represents an error induced by user, encountered during HTTP request processing."

translation, this represents an error - resulting from a user action - that occurred while the HTTP request of the user was being processed.

Here we have a user request - to copy something - and we cannot complete the HTTP request.

Further, the other checks in the call path all throw a Failure, e.g.

throw new Failure(Messages.Hudson_JobAlreadyExists(name));

So we are left with your only objecting being that it is a RuntimeException

Seems overly reaching to require code duplication just to make the exception non-runtime exception

This comment has been minimized.

@stephenc

stephenc Mar 9, 2017

Member

There is another reason to use Failure and that is if we used another class we would not be able to pick up usage of the class until the baseline has this new class, whereas with Failure we can just implement the method on ItemListener and it will work on newer jenkins versions: https://github.com/jenkinsci/branch-api-plugin/pull/95/files#diff-b347159a8a8075d4e700065f8668b804R2244

@stephenc

stephenc Mar 9, 2017

Member

There is another reason to use Failure and that is if we used another class we would not be able to pick up usage of the class until the baseline has this new class, whereas with Failure we can just implement the method on ItemListener and it will work on newer jenkins versions: https://github.com/jenkinsci/branch-api-plugin/pull/95/files#diff-b347159a8a8075d4e700065f8668b804R2244

@stephenc

This comment has been minimized.

Show comment
Hide comment
@stephenc

stephenc Mar 9, 2017

Member

@oleg-nenashev I claim I have addressed your code review comments

Member

stephenc commented Mar 9, 2017

@oleg-nenashev I claim I have addressed your code review comments

@daniel-beck

This comment has been minimized.

Show comment
Hide comment
@daniel-beck

daniel-beck Mar 11, 2017

Member

No objections from me.

Member

daniel-beck commented Mar 11, 2017

No objections from me.

@oleg-nenashev oleg-nenashev self-requested a review Mar 11, 2017

@oleg-nenashev

🐝 since due to the plugin compatibility requirements, ideally we need a better API for a longer term perspective.

@stephenc stephenc merged commit 80aa2c8 into jenkinsci:master Mar 15, 2017

1 of 2 checks passed

continuous-integration/jenkins/pr-head This commit cannot be built
Details
Jenkins This pull request looks good
Details

@stephenc stephenc deleted the stephenc:jenkins-34691 branch Mar 15, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment