Bug 27434 - Background Jobs should use classes as job types
Summary: Background Jobs should use classes as job types
Status: In Discussion
Alias: None
Product: Koha
Classification: Unclassified
Component: Task Scheduler (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Bugs List
QA Contact: Testopia
URL:
Keywords:
Depends on: 22417
Blocks: 35092
  Show dependency treegraph
 
Reported: 2021-01-14 15:30 UTC by Nick Clemens
Modified: 2023-10-18 14:46 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 27434: POC (2.78 KB, patch)
2021-01-14 15:30 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2021-01-14 15:30:22 UTC
Rather than maintaining a list of types and going through each case we should use the class as the job type so we can call it directly
Comment 1 Nick Clemens 2021-01-14 15:30:58 UTC
Created attachment 115164 [details] [review]
Bug 27434: POC
Comment 2 David Cook 2021-01-14 23:40:40 UTC
Comment on attachment 115164 [details] [review]
Bug 27434: POC

Review of attachment 115164 [details] [review]:
-----------------------------------------------------------------

::: Koha/BackgroundJob.pm
@@ +137,4 @@
>      my ( $self, $args ) = @_;
>  
>      my $job_type = $self->type;
> +    my $class = "Koha::BackgroundJob::${job_type}";

I'd suggest $job_type actually contain the whole "Koha::BackgroundJob::BatchUpdateBiblio". I think that would be more powerful/flexible. It could also make for fewer typos.

::: Koha/BackgroundJob/BatchUpdateAuthority.pm
@@ +43,4 @@
>  =cut
>  
>  sub job_type {
> +    return 'BatchUpdateAuthority';

If we actually use the entire Koha::BackgroundJob::BatchUpdateAuthority string for the job_type, then we could replace this with 'return __PACKAGE__;'

::: misc/background_jobs_worker.pl
@@ +28,5 @@
>      warn sprintf "Cannot connect to the message broker, the jobs will be processed anyway (%s)", $_;
>  };
>  
> +# FIXME Job types should be stored in a table so that plugins can register new job types as well
> +my @job_types = qw( BatchUpdateBiblio BatchUpdateAuthority );

Honestly, I don't think that we actually even need to store job_types. Something that could be interesting though is specifying what queues the background_jobs_worker.pl will subscribe to.

This could be useful in terms of future scaling of workers. For instance, you might want only 1 worker for a lot of background tasks, but you might want a few for particularly busy queues.
Comment 3 David Cook 2021-01-14 23:41:52 UTC
Overall, I'm very pro this idea.

In fact, class names are what I have already been using for my local Koha::MQ stuff prior to to pushing of Bug 22417.
Comment 4 Jonathan Druart 2021-01-15 09:19:44 UTC
To be clear, I implemented it like that to provide an "allow list" of tasks we can executed. Otherwise we will potentially execute anything we don't have the hand on it.

However we may want to assume that if the DB is corrupted, it's too late already...
Comment 5 David Cook 2021-01-18 01:43:08 UTC
(In reply to Jonathan Druart from comment #4)
> To be clear, I implemented it like that to provide an "allow list" of tasks
> we can executed. Otherwise we will potentially execute anything we don't
> have the hand on it.
> 
> However we may want to assume that if the DB is corrupted, it's too late
> already...

I suppose that's a case of security vs convenience. Having an allow list would be more secure, but less convenient - especially for plugins. 

What are the potential threats? Malicious Perl plugins? Other parts of the system sending malicious messages to RabbitMQ?

Malicious Perl plugins are already a problem. Malicious messages would have to be extremely well crafted and even then... unlikely to do any harm especially if we used a hook like "run_koha_background_task".
Comment 6 Martin Renvoize 2021-02-24 19:41:01 UTC
Totally onboard with this.. thanks for opening the bug
Comment 7 Tomás Cohen Arazi 2022-03-25 15:07:02 UTC
I filed bug 30350, which implements one line that was present on this bug (I didn't know about). The one removing the codes allow-list from background_job_worker.pl and reusing Koha::BackgroundJob::type_to_class_mapping instead.

I think, overall, having an intermediate method that provides an allow-list is good. Inside that method I expect to query for plugin-implemented tasks to be added on this 'core' mapping we are building for core-defined tasks.

The last few days I've filed several bugs for polishing the area. Maybe we can delay this enhancement until it is clear how much we gain/loose.