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
Created attachment 115164 [details] [review] Bug 27434: POC
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.
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.
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...
(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".
Totally onboard with this.. thanks for opening the bug
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.