Fixes #22750: Converts raw IDs to ObjectVars/MultiObjectVars

[mburggraf]

Closes: #22750

Raw IDs from API calls are now converted to Objects, using the scripts form.

Note, that this may also taint execution as event handlers, so I like to point to feature request #21619.

[pheus]

Thanks for opening this PR!

Could you please take a look at Claude's review comments and either address the points it raised or reply with a short explanation where you think a comment doesn’t apply? That context would be really helpful for the review. Thanks!

[mburggraf]

@pheus Taking Claude's review into consideration, I recrafted the code to do the conversion in the REST API view (as suggested), reverting the changes to ScriptJob.run

[arthanson]

Thanks @mburggraf. It looks like you are on the right track here, but note the CI tests are failing.

Moving the conversion into the API view looks correct. The problem is how the execution parameters (commit, schedule_at, interval, notifications) are read: pulling them out of the form instead of the request means the form's defaults win, so commit is always False. This means scripts silently do nothing and scheduling is dropped. An existing test already catches this, which is why CI is failing.

Several suggested things to fix it: I think you can read those four parameters from validated_data instead of the form

Also clean up the CodeQL/error-format items. As noted in by the code scans - the two new tests in test_jobs.py don't execute any changed code - they re-implement the view's form-building/pop/merge logic inline and call runner.run(). Nothing imports ScriptViewSet, so they pass regardless of what views.py

[arthanson]

@claude /review Only report on issues of potential concern; we don't need commentary on anything that doesn't require further consideration.

[github-actions]

Claude finished @arthanson's task in 5m 10s —— View job

---

Review: #22861 — Convert raw IDs to ObjectVars/MultiObjectVars

• Fetch PR diff
• Analyze extras/api/views.py
• Analyze extras/scripts.py helper + UI path
• Check ScriptJob / enqueue data handling
• Review test coverage
• Post review

---

Overview

The API POST /api/extras/scripts/<pk>/ handler now binds the submitted data payload to the script's own ScriptForm and enqueues form.cleaned_data instead of the raw JSON, so ObjectVar/MultiObjectVar reach Script.run() as model instances/querysets. The default-backfill logic is extracted from ScriptView.post() into a shared prepare_script_form() helper. Execution parameters (commit, schedule_at, interval, notifications) are read from validated_data, not the form — this correctly resolves @arthanson's commit is always False regression. CI and CodeQL are both green on the current head, so the earlier failures are addressed.

The approach is right. The items below are the ones I think still need consideration.

---

Issues

1. Non-field form errors are silently swallowed → empty-body 400 — netbox/extras/api/views.py:388-390

errors = {k: v for k, v in form.errors.items() if not k.startswith('_')}
raise ValidationError(errors)

Django keys non-field errors under NON_FIELD_ERRORS = '__all__', which starts with _ and is therefore stripped. ScriptForm.clean() raises exactly such an error ("Scheduled time must be in the future."). Concretely: POST {"data": {"_schedule_at": "2020-01-01T00:00:00Z", "site": 1}, "commit": true} → form is invalid, only __all__ is populated, errors filters down to {}, and the client gets 400 with an empty body and no indication of what went wrong. The prefix test is also wrong for any script var legitimately declared with a leading underscore.

The explicit tuple two lines below is the correct filter — reuse it rather than a prefix heuristic:

EXEC_PARAMS = ('_commit', '_schedule_at', '_interval', '_notifications')
errors = {k: v for k, v in form.errors.items() if k not in EXEC_PARAMS}
if not errors:
    errors = {'data': form.errors.get(NON_FIELD_ERRORS, [_('Invalid script input.')])}

Fix this →

2. Undeclared payload keys are now silently dropped — backwards-incompatible — netbox/extras/api/views.py:392

form.cleaned_data contains only declared script variables. Any key a client sends today that isn't a declared ScriptVariable previously reached Script.run() verbatim and now vanishes with no error. Scripts doing data.get('extra_thing') will start silently receiving None after an upgrade. Value types also change (a DateVar now yields datetime.date instead of the raw string, etc.) — that's the intended fix, but it's still a contract change for existing API consumers.

Two things worth deciding explicitly: (a) should unknown keys 400 rather than be dropped silently, and (b) this needs a note in the release notes / docs/customization/custom-scripts.md flagged as a breaking change for API callers.

3. Form errors are returned as top-level response keys, colliding with execution parameters — netbox/extras/api/views.py:390

Serializer errors (schedule_at, interval, commit, notifications, data) and script-variable errors now share one flat namespace. A script declaring interval = IntegerVar() produces {"interval": ["..."]} that a client cannot distinguish from a rejected execution-parameter. Nesting under the field the values actually came from is unambiguous and matches the request shape:

raise ValidationError({'data': errors})

Fix this →

4. Test gaps on the exact regression that was flagged — netbox/extras/tests/test_api.py:1447+

The new tests cover commit and notifications forwarding, but not schedule_at / interval, which were half of @arthanson's finding. There's also no test that an invalid/nonexistent object ID returns 400 — the primary new failure mode this PR introduces. Both are cheap additions to the existing mocked-enqueue pattern.

Fix this →

5. Script.python_class is monkey-patched without restore — netbox/extras/tests/test_api.py:1490

Script.python_class = self.TestScriptClass

This permanently replaces the model's python_class property on the class object for the remainder of the test process — no addCleanup. ScriptTestCase above does the same thing (pre-existing), but this PR duplicates it, so any later test in the same worker that relies on the real property gets TestScriptClass. patch.object(Script, 'python_class', self.TestScriptClass) + self.addCleanup(...) is a one-line fix and is already the pattern used for the worker patch immediately below it.

Fix this →

---

Minor

• netbox/extras/api/views.py:383 — _('Script class could not be loaded; cannot determine job timeout.') is misleading; the job timeout is incidental. Something like _('Unable to load the script class.') is clearer, and this is arguably a 500-class condition rather than a client error.
• netbox/extras/scripts.py:44 — prepare_script_form is added to __all__, which is the namespace script authors get from from extras.scripts import *. It's an internal plumbing helper; consider leaving it out of __all__ or moving it to extras/utils.py.
• netbox/extras/scripts.py:663-664 — the docstring claims the two entry points "can't drift apart again", but there are four ScriptJob.enqueue call sites. extras/management/commands/runscript.py:76 calls as_form() without the default backfill (and forgets to pop _notifications, so it leaks into script data), and extras/events.py:263 still enqueues raw event_data. Either narrow the docstring or route runscript through the new helper too.
• netbox/extras/tests/test_api.py:1448-1455 — the class docstring references "PR Fixes #22750: Converts raw IDs to ObjectVars/MultiObjectVars #22861" and its review history. Test docstrings outlive the PR; referencing issue Custom script execution via REST API passes ObjectVar values as raw IDs #22750 and the behaviour under test reads better long-term.
• netbox/extras/api/views.py:371 vs 377/383/390 — mixes return Response(..., 400) and raise ValidationError(...) for the same outcome. Picking one is tidier.
  • Branch: 22750-convert-raw-IDs-to-ObjectVars

[arthanson]

@mburggraf looks like it needs a few more fixes from the last changes:

Item 1 the 400 issue looks like the biggest issue - get_vars() (extras/scripts.py:451-473) applies no name filtering, so startswith('') swallows both NON_FIELD_ERRORS ('all') and any script variable legitimately named _foo. raise ValidationError({}) gives DRF self.detail = {} → 400 with an empty body.

2 and 3 I think are more minor, you can check. 4 can do with another couple tests for completeness. Also 5 looks like an easy one-liner.

[mburggraf]

@arthanson: Fixed the mentioned things. About 2: I'd say the backwards incompatibility is intentional and should be mentioned in the changelog.

[arthanson]

Closing this as opening #23119 off of these changes to resolve merge conflicts and add doc string.