A component to ask a job question from an event listener?

Development
1

Hello devs,

XClassDeletingListener (xwiki-platform-refactoring-default) and DocumentsDeletingListener (xwiki-platform-extension-handler-xar) share this block:

} catch (InterruptedException e) {
    this.logger.warn("Confirm question has been interrupted.");
    cancelableEvent.cancel("Question has been interrupted.");
}
// we always want the event and the CancelableJobStatus to be consistent
if (jobStatus instanceof CancelableJobStatus cancelableJobStatus) {
    if (cancelableJobStatus.isCanceled()) {
        cancelableEvent.cancel();
    }
    if (cancelableEvent.isCanceled()) {
        cancelableJobStatus.cancel();
    }
}

I noticed it because [Misc] Fix SonarQube issues by restoring the thread interrupt status by claude[bot] · Pull Request #6248 · xwiki/xwiki-platform · GitHub fails its SonarCloud quality gate on it (new_duplicated_lines_density at 5%, threshold 3%): that PR adds a single Thread.currentThread().interrupt(); line to each of the two files, and both land inside the duplicated block.

Every type involved is in Commons: Job, JobStatus and CancelableJobStatus in xwiki-commons-job-api, CancelableEvent in xwiki-commons-observation-api. And a base listener class wouldn’t be right, since the shared code never touches onEvent(), the event list or the listener name: the two classes have nothing in common as listeners, they just need the same helper. So I’d rather have a component in xwiki-commons-job-api, which already has org.xwiki.observation on its compile classpath:

@Role
public interface JobQuestionAsker
{
    /**
     * Ask a question to the user of the passed job, and cancel both the event and the job status when no answer
     * is received before the timeout expires or when the wait is interrupted.
     *
     * @param job the job asking the question
     * @param event the cancelable event being handled
     * @param question the question to ask
     * @param timeout how long to wait for an answer
     * @param noAnswerMessage the message to log and to cancel the event with when no answer is received
     * @since 18.8.0RC1
     */
    void ask(Job job, CancelableEvent event, Object question, Duration timeout, String noAnswerMessage);
}

The default implementation in xwiki-commons-job-default is just the two blocks moved out of the listeners:

@Component
@Singleton
public class DefaultJobQuestionAsker implements JobQuestionAsker
{
    @Inject
    private Logger logger;

    @Override
    public void ask(Job job, CancelableEvent event, Object question, Duration timeout, String noAnswerMessage)
    {
        JobStatus jobStatus = job.getStatus();

        try {
            if (!jobStatus.ask(question, timeout.toMillis(), TimeUnit.MILLISECONDS)) {
                // Without any answer we must cancel the operation.
                this.logger.warn(noAnswerMessage);
                event.cancel(noAnswerMessage);
            }
        } catch (InterruptedException e) {
            this.logger.warn("Confirm question has been interrupted.");
            Thread.currentThread().interrupt();
            event.cancel("Question has been interrupted.");
        }

        // We always want the event and the CancelableJobStatus to be consistent.
        if (jobStatus instanceof CancelableJobStatus cancelableJobStatus) {
            if (cancelableJobStatus.isCanceled()) {
                event.cancel();
            }
            if (event.isCanceled()) {
                cancelableJobStatus.cancel();
            }
        }
    }
}

Note that this is also where the Thread.currentThread().interrupt(); that started all this would live, once, instead of in each listener.

XClassDeletingListener

Today:

@Override
public void onEvent(Event event, Object source, Object data)
{
    Job job = (Job) source;
    CancelableEvent cancelableEvent = (CancelableEvent) event;
    XClassBreakingQuestion question = this.buildQuestion(job, cancelableEvent, data);

    if (question == null) {
        return;
    }
    // Ask a confirmation to the user if some pages contain used XClass
    if (!question.getImpactedObjects().isEmpty()) {
        JobStatus jobStatus = job.getStatus();
        try {
            if (this.documentAccessBridge.isAdvancedUser(job.getRequest().getProperty("user.reference"))) {
                // Conservative choice: we let the user enable the pages to delete.
                question.unselectAll();
                // The user can modify the question so it could disable some EntitySelection.
                // We add a timeout because when a refactoring job is running, it prevents others to run.
                // 5 minutes is probably enough for the user to decide if the process should go on.
                boolean ack = job.getStatus().ask(question, 5, TimeUnit.MINUTES);
                if (!ack) {
                    // Without any confirmation, we must cancel the operation.
                    String message = "The question has been asked, however no answer has been received.";
                    this.logger.warn(message);
                    cancelableEvent.cancel(message);
                }
            } else {
                question.setRefactoringForbidden(true);
                // we don't want the user to answer the question,
                // but we want to display that his action is forbidden.
                boolean ack = jobStatus.ask(question, 1, TimeUnit.MINUTES);
                if (!ack) {
                    String message = "The question has been canceled because this refactoring is forbidden.";
                    cancelableEvent.cancel(message);
                }
            }
        } catch (InterruptedException e) {
            this.logger.warn("Confirm question has been interrupted.");
            cancelableEvent.cancel("Question has been interrupted.");
        }
        // we always want the event and the CancelableJobStatus to be consistent
        if (jobStatus instanceof CancelableJobStatus cancelableJobStatus) {
            if (cancelableJobStatus.isCanceled()) {
                cancelableEvent.cancel();
            }
            if (cancelableEvent.isCanceled()) {
                cancelableJobStatus.cancel();
            }
        }
    }
}

With the component:

@Override
public void onEvent(Event event, Object source, Object data)
{
    Job job = (Job) source;
    CancelableEvent cancelableEvent = (CancelableEvent) event;
    XClassBreakingQuestion question = this.buildQuestion(job, cancelableEvent, data);

    // Ask a confirmation to the user if some pages contain used XClass
    if (question == null || question.getImpactedObjects().isEmpty()) {
        return;
    }

    if (this.documentAccessBridge.isAdvancedUser(job.getRequest().getProperty("user.reference"))) {
        // Conservative choice: we let the user enable the pages to delete.
        question.unselectAll();
        // The user can modify the question so it could disable some EntitySelection.
        // We add a timeout because when a refactoring job is running, it prevents others to run.
        // 5 minutes is probably enough for the user to decide if the process should go on.
        this.questionAsker.ask(job, cancelableEvent, question, Duration.ofMinutes(5),
            "The question has been asked, however no answer has been received.");
    } else {
        // We don't want the user to answer the question, but we want to display that this action is forbidden.
        question.setRefactoringForbidden(true);
        this.questionAsker.ask(job, cancelableEvent, question, Duration.ofMinutes(1),
            "The question has been canceled because this refactoring is forbidden.");
    }
}

45 lines down to 24, two nesting levels less. One behaviour change to note: the “refactoring is forbidden” branch currently cancels without logging, and would now log the message as a warning.

DocumentsDeletingListener

Today (the tail of onEvent(), the rest is unchanged):

    JobStatus jobStatus = job.getStatus();
    // Ask a confirmation to the user if some pages belong to extensions
    if (!question.getExtensions().isEmpty()) {
        // Conservative choice: we let the user enable the pages to delete.
        question.unselectAll();
        try {
            // The user can modify the question so it could disable some EntitySelection.
            // We add a timeout because when a refactoring job is running, it prevents others to run.
            // 5 minutes is probably enough for the user to decide if the process should go on.
            boolean ack = jobStatus.ask(question, 5, TimeUnit.MINUTES);
            if (!ack) {
                // Without any confirmation, we must cancel the operation.
                String message = "The question has been asked, however no answer has been received.";
                this.logger.warn(message);
                cancelableEvent.cancel(message);
            }
        } catch (InterruptedException e) {
            this.logger.warn("Confirm question has been interrupted.");
            cancelableEvent.cancel("Question has been interrupted.");
        }
        // we always want the event and the CancelableJobStatus to be consistent
        if (jobStatus instanceof CancelableJobStatus cancelableJobStatus) {
            if (cancelableJobStatus.isCanceled()) {
                cancelableEvent.cancel();
            }
            if (cancelableEvent.isCanceled()) {
                cancelableJobStatus.cancel();
            }
        }
    }

With the component:

    // Ask a confirmation to the user if some pages belong to extensions
    if (!question.getExtensions().isEmpty()) {
        // Conservative choice: we let the user enable the pages to delete.
        question.unselectAll();
        // The user can modify the question so it could disable some EntitySelection.
        // We add a timeout because when a refactoring job is running, it prevents others to run.
        // 5 minutes is probably enough for the user to decide if the process should go on.
        this.questionAsker.ask(job, cancelableEvent, question, Duration.ofMinutes(5),
            "The question has been asked, however no answer has been received.");
    }

29 lines down to 10, no behaviour change.

Open questions

  • Is a component the right shape, or is a static utility enough? The argument for a component is testability: XClassDeletingListenerTest and DocumentsDeletingListenerTest both stub status.ask(...) and re-verify the interruption behaviour today, whereas with a component they’d verify one interaction and the behaviour would be tested once, in Commons.
  • Naming: JobQuestionAsker, or JobQuestionManager to stay in line with our other component names?
  • Signature: Duration, or long timeout, TimeUnit unit to stay consistent with the existing JobStatus.ask()? And is passing noAnswerMessage as a String acceptable? Returning the boolean and letting the caller cancel would put the duplication straight back.
  • The two listeners also share their event.isCanceled() / !job.getRequest().isInteractive() pre-checks (Sonar doesn’t flag those only because the log messages differ). Should a canAsk() method cover them too?
  • And the real question: is ~15 lines duplicated in exactly two places worth a new API in Commons? Doing nothing and accepting the duplication on that PR is a perfectly valid answer.

In practice this would mean an XCOMMONS issue and PR landing first, then the Platform side, with @since 18.8.0RC1.

Honestly I’m not sure it’s worth it, hence the brainstorming tag. Curious to have your POV.

Thanks

2

+1 to provide a helper for that use case

The order of parameters is a bit strange, different concepts are mixed together. Also, I feel the method name should be a little less generic, as cancelling an event is really not the only way to react to a user not answering to a job question (for most other use cases, like extension questions for which we need to add timeouts, we would simply keep the default answer).

For example:

    void askOrCancel(Job job, Object question, Duration timeout, CancelableEvent event, String noAnswerMessage);

or askCancelableEvent, or some better name expressing the same concept.

I know it would be a pretty strange use case (but anyway anything lower than a few seconds is a strange use case), but your implementation does not support the theoretically possible use case where someone would pass a < 1ms duration. So it would be more accurate to go through nanoseconds I guess.

Honestly using a static because “we don’t need it to be a component” often come back to haunt us. Experienced that a few times…

Not a fan of “asker”, sounds a bit weird. In any case, I think I would prefer something more generic related to job question in general (which might also be about something else than wrapping JobStatus#ask).

I guess I would tend to prefer something more consistent, but it’s not critical either.

Probably not in itself. It could also be an internal static helper, for now.

But I suspect this is a common use case so it does have value if you want to embark on this adventure :slight_smile:

3

Thanks Thomas, all taken. Revised proposal:

@Role
public interface JobQuestionManager
{
    void askOrCancel(Job job, Object question, Duration timeout, CancelableEvent event, String noAnswerMessage);
}
  • Generic role name, cancel semantics in the method name, and your parameter order.
  • Implementation goes through jobStatus.ask(question, timeout.toNanos(), TimeUnit.NANOSECONDS) so sub-millisecond durations aren’t truncated to 0.
  • Keeping Duration since you said consistency with JobStatus.ask() isn’t critical: it reads better at the call sites (Duration.ofMinutes(5)) and the conversion is lossless.
  • Component, not static.

Still open, if anyone has an opinion:

  • Should the helper also cover the event.isCanceled() / !job.getRequest().isInteractive() pre-checks the two listeners share, e.g. via a canAsk() method?
  • Is passing noAnswerMessage as a String acceptable? Returning the boolean and letting the caller cancel would put the duplication straight back.

The one behaviour change (the “refactoring is forbidden” branch now logging its message as a warning) looks fine to me, so I’ll keep it unless someone objects.