Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Job.clean() not called #17685

Closed
alehaa opened this issue Oct 7, 2024 · 8 comments · Fixed by #17847
Closed

Job.clean() not called #17685

alehaa opened this issue Oct 7, 2024 · 8 comments · Fixed by #17847
Assignees
Labels
netbox severity: low Does not significantly disrupt application functionality, or a workaround is available status: accepted This issue has been accepted for implementation type: bug A confirmed report of unexpected behavior in the application

Comments

@alehaa
Copy link
Contributor

alehaa commented Oct 7, 2024

Deployment Type

Self-hosted

NetBox Version

v4.1.3

Python Version

3.11

Steps to Reproduce

  1. Create a new JobRunner.
  2. Schedule the job runner without with enqueue(interval=0).

Expected Behavior

Getting a ValidationError:

django.core.exceptions.ValidationError: {
  '__all__': [
    'Jobs cannot be assigned to this object type (None).'
  ]
}

Observed Behavior

Nothing happens, as neither Job.enqueue(), Job.start() nor Job.terminate() trigger Job.clean().

@alehaa alehaa added status: needs triage This issue is awaiting triage by a maintainer type: bug A confirmed report of unexpected behavior in the application labels Oct 7, 2024
@alehaa
Copy link
Contributor Author

alehaa commented Oct 7, 2024

This error occured during testing of similar issues, as Job.clean() (like #17682, #17086 and #17648) assumes the object_type field is not None. If its decided Job should call clean(), this should be fixed as well (could happen in conjunction with #17682). Otherwise the method could be removed IMHO.

@jeremystretch
Copy link
Member

@alehaa could you please elaborate on your steps to reproduce above? It is not clear exactly what you're doing. Example code would be helpful.

@jeremystretch jeremystretch added status: revisions needed This issue requires additional information to be actionable and removed status: needs triage This issue is awaiting triage by a maintainer labels Oct 7, 2024
@alehaa
Copy link
Contributor Author

alehaa commented Oct 7, 2024

During the implementation of #16971, I tested whether NetBox would raise exceptions on incorrectly configured jobs, and whether separate tests needed to be added to the system job registration. I added this (abbreviated) code to NetBox:

class SampleJob(JobRunner):
    def run(self, *args, **kwargs):
        pass

SampleJob.enqueue_once(interval=0)

The job can be scheduled without error. However, this should fail for two reasons:

  1. interval=0 violates the field's MinValueValidator(1).
  2. As no instance is set, clean() would raise an exception as it doesn't take into account self.object_type can be None. That's a bug related to Exception for job list if object not set #17682, Exception for job details if object not set #17086 and When a background job is scheduled without an instance, delete() on the job model will throw exceptions #17648, but didn't attract attention before, as clean() isn't called by any of the other methods in Job.

As this is not a cause of system jobs (#16971), but the Job model itself, I see this as a separate issue / bug.

Copy link
Contributor

This is a reminder that additional information is needed in order to further triage this issue. If the requested details are not provided, the issue will soon be closed automatically.

@github-actions github-actions bot added the pending closure Requires immediate attention to avoid being closed for inactivity label Oct 15, 2024
@alehaa
Copy link
Contributor Author

alehaa commented Oct 15, 2024

@jeremystretch Did you have a chance to review the additional information above?

@jeremystretch
Copy link
Member

@alehaa pk, I'll flag this for an owner, unless you'd like to take it?

@jeremystretch jeremystretch added status: needs owner This issue is tentatively accepted pending a volunteer committed to its implementation severity: low Does not significantly disrupt application functionality, or a workaround is available and removed status: revisions needed This issue requires additional information to be actionable pending closure Requires immediate attention to avoid being closed for inactivity labels Oct 22, 2024
@alehaa
Copy link
Contributor Author

alehaa commented Oct 22, 2024

I can provide a PR. As this is a fix for existing functionality I assume develop is the right branch?

@jeremystretch
Copy link
Member

@alehaa great, thanks! And yeah, develop please.

@jeremystretch jeremystretch added status: accepted This issue has been accepted for implementation and removed status: needs owner This issue is tentatively accepted pending a volunteer committed to its implementation labels Oct 22, 2024
@jeremystretch jeremystretch added the netbox label Nov 1, 2024 — with Linear
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
netbox severity: low Does not significantly disrupt application functionality, or a workaround is available status: accepted This issue has been accepted for implementation type: bug A confirmed report of unexpected behavior in the application
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants