Cache job result URLs, narrow caught exceptions, and accept model labels in register_job_result_url()

docs/plugins/development/background-jobs.md

@@ -95,7 +95,7 @@ class MyModel(JobsMixin, NetBoxModel):
 
 ### Result URLs
 
-By default, links to a job (such as those in the jobs list and in job completion notifications) lead to its generic view. A plugin which provides its own page for presenting the results of jobs associated with one of its models can register a function which returns the URL of that page, using the `register_job_result_url()` decorator from `core.utils`. The function accepts a Job instance and returns either a URL, or `None` to decline.
+By default, links to a job (such as those in the jobs list and in job completion notifications) lead to its generic view. A plugin which provides its own page for presenting the results of jobs associated with one of its models can register a function which returns the URL of that page, using the `register_job_result_url()` decorator from `core.utils`. The model may be passed as a class or as a string in the form `<app_label>.<model_name>`. The function accepts a Job instance and returns either a URL, or `None` to decline. If the function raises `NoReverseMatch`, `ObjectDoesNotExist`, or `AttributeError`, a warning is logged and it is treated as having declined.
 
 Optionally, a job name may be specified to restrict the function to jobs having that name. A function registered for a specific job name takes precedence over one registered for the model alone, which is consulted if the former returns `None`. The generic view is used if no function returns a URL. Only one function may be registered for each combination of model and job name.
 

netbox/core/models/jobs.py

@@ -1,7 +1,7 @@
 import logging
 import uuid
 from dataclasses import asdict
-from functools import partial
+from functools import cached_property, partial
 
 import django_rq
 from django.conf import settings
@@ -156,9 +156,12 @@ class Job(models.Model):
         return self.name
 
     def get_absolute_url(self):
-        if url := get_job_result_url(self):
-            return url
-        return reverse('core:job', args=[self.pk])
+        return self._absolute_url
+
+    @cached_property
+    def _absolute_url(self):
+        # Cached because job tables link multiple columns, and a registered result URL function may query the database
+        return get_job_result_url(self) or reverse('core:job', args=[self.pk])
 
     def get_status_color(self):
         return JobStatusChoices.colors.get(self.status)

netbox/core/tests/test_models.py

@@ -512,6 +512,5 @@ class JobTestCase(TestCase):
         self.assertEqual(job.get_absolute_url(), reverse('core:job', args=[123]))
 
         # Jobs for custom scripts link to the script result view
-        job.object_type = ObjectType.objects.get_for_model(Script)
-        job.object_id = 1
+        job = Job(pk=123, name='Test Job', object_type=ObjectType.objects.get_for_model(Script), object_id=1)
         self.assertEqual(job.get_absolute_url(), reverse('extras:script_result', kwargs={'job_pk': 123}))

netbox/core/tests/test_utils.py

@@ -1,4 +1,4 @@
-from unittest.mock import patch
+from unittest.mock import Mock, patch
 
 from django.contrib.contenttypes.models import ContentType
 from django.core.exceptions import ImproperlyConfigured
@@ -29,15 +29,17 @@ class JobResultURLTestCase(TestCase):
 
         self.assertIs(register_job_result_url(DataSource)(func), func)
         self.assertIs(register_job_result_url(DataSource, job_name='Sync')(func), func)
+        self.assertIs(register_job_result_url('dcim.Site')(func), func)
         self.assertEqual(registry['job_result_urls'], {
             ('core.datasource', None): func,
             ('core.datasource', 'Sync'): func,
+            ('dcim.site', None): func,
         })
 
     def test_register_duplicate(self):
         register_job_result_url(DataSource, job_name='Sync')(lambda job: None)
         with self.assertRaises(ImproperlyConfigured):
-            register_job_result_url(DataSource, job_name='Sync')(lambda job: None)
+            register_job_result_url('core.datasource', job_name='Sync')(lambda job: None)
 
     def test_no_object(self):
         register_job_result_url(DataSource)(lambda job: '/foo/')
@@ -75,9 +77,26 @@ class JobResultURLTestCase(TestCase):
         def func(job):
             return reverse('plugins:invalid')
 
-        with self.assertLogs('netbox.jobs', level='ERROR'):
+        with self.assertLogs('netbox.jobs', level='WARNING'):
             self.assertEqual(self.get_job(DataSource).get_absolute_url(), reverse('core:job', args=[123]))
 
+    def test_unexpected_exception(self):
+        @register_job_result_url(DataSource)
+        def func(job):
+            raise ValueError
+
+        with self.assertRaises(ValueError):
+            self.get_job(DataSource).get_absolute_url()
+
+    def test_url_cached(self):
+        func = Mock(return_value='/foo/')
+        register_job_result_url(DataSource)(func)
+        job = self.get_job(DataSource)
+
+        self.assertEqual(job.get_absolute_url(), '/foo/')
+        self.assertEqual(job.get_absolute_url(), '/foo/')
+        func.assert_called_once_with(job)
+
     def test_unresolvable_model(self):
         register_job_result_url(DataSource)(lambda job: '/foo/')
 

netbox/core/utils.py

@@ -1,8 +1,9 @@
 import logging
 
-from django.core.exceptions import ImproperlyConfigured
+from django.core.exceptions import ImproperlyConfigured, ObjectDoesNotExist
 from django.db import DatabaseError, connection
 from django.http import Http404
+from django.urls import NoReverseMatch
 from django.utils.translation import gettext_lazy as _
 from django_rq.queues import get_queue, get_queue_by_index, get_redis_connection
 from django_rq.settings import get_queues_list, get_queues_map
@@ -33,6 +34,8 @@ __all__ = (
     'stop_rq_job',
 )
 
+logger = logging.getLogger('netbox.jobs')
+
 
 def get_rq_jobs():
     """
@@ -215,7 +218,8 @@ def get_db_schema():
 def register_job_result_url(model, job_name=None):
     """
     Decorator for registering a function which returns the URL of the results page for background jobs associated
-    with objects of the given model (and optionally, having the given name).
+    with objects of the given model (and optionally, having the given name). The model may be passed as a class or as
+    a string in the form "<app_label>.<model_name>".
 
     The function must accept a Job instance and return either a URL, or None to defer to the next applicable function
     (and ultimately to the generic job view). A function registered for a specific job name takes precedence over one
@@ -225,13 +229,13 @@ def register_job_result_url(model, job_name=None):
         def my_job_result_url(job):
             return reverse('plugins:myplugin:mymodel_results', args=[job.object_id])
     """
-    key = (model._meta.label_lower, job_name)
+    label = model.lower() if isinstance(model, str) else model._meta.label_lower
+    key = (label, job_name)
 
     def _wrapper(func):
         if key in registry['job_result_urls']:
             raise ImproperlyConfigured(
-                f"A job result URL function is already registered for model {model._meta.label} and job name "
-                f"{job_name!r}."
+                f"A job result URL function is already registered for model {label} and job name {job_name!r}."
             )
         registry['job_result_urls'][key] = func
         return func
@@ -251,8 +255,10 @@ def get_job_result_url(job):
             try:
                 if url := func(job):
                     return url
-            except Exception:
+            except (NoReverseMatch, ObjectDoesNotExist, AttributeError) as e:
                 # A failing function must not break the rendering of jobs lists or notifications.
-                logger = logging.getLogger('netbox.jobs')
-                logger.exception("Failed to resolve the result URL for job %s using %r", job.pk, func)
+                logger.warning(
+                    "Failed to resolve the result URL for job %s using %r: %s", job.pk, func, e,
+                    exc_info=logger.isEnabledFor(logging.DEBUG)
+                )
     return None