Framework: [5.1] Jobs fail to run when a dependent model is deleted

[EvanDarwin]

If a Model is serialized in let's say Job A, and it is deleted in Job B before this job can run, it throws an exception during the __wakeup() call, throwing it back in queue infinitely.

The problems with the current Job system in this case:

• You're unable to catch any Exceptions during the __wakeup() or deserialization phase
• The serialization of the model is called with findOrFail()

https://github.com/laravel/framework/blob/5.1/src/Illuminate/Queue/SerializesModels.php#L65

[2015-06-20 18:03:22] production.ERROR: exception 'Illuminate\Database\Eloquent\ModelNotFoundException' with message 'No query results for model [KikFinder\Submission].' in /var/www/kikfinder.com/vendor/laravel/framework/src/Illuminate/Database/Eloquent/Builder.php:127
Stack trace:
#0 [internal function]: Illuminate\Database\Eloquent\Builder->findOrFail('127')
#1 /.../bootstrap/cache/compiled.php(11013): call_user_func_array(Array, Array)
#2 /.../vendor/laravel/framework/src/Illuminate/Queue/SerializesModels.php(65): Illuminate\Database\Eloquent\Model->__call('findOrFail', Array)
#3 /.../vendor/laravel/framework/src/Illuminate/Queue/SerializesModels.php(65): KikFinder\Submission->findOrFail('127')
#4 /.../vendor/laravel/framework/src/Illuminate/Queue/SerializesModels.php(40): KikFinder\Jobs\Admin\DeleteSubmission->getRestoredPropertyValue(Object(Illuminate\Contracts\Database\ModelIdentifier))
#5 [internal function]: KikFinder\Jobs\Admin\DeleteSubmission->__wakeup()
#6 /.../vendor/laravel/framework/src/Illuminate/Queue/CallQueuedHandler.php(38): unserialize('O:37:"KikFinder...')

[GrahamCampbell]

I don't see what the issue is. They only go back in the queue if they haven't been retried enough times?

[GrahamCampbell]

Not throwing an exception here would be incorrect. We need to indicate that waking up the object has failed.

[GrahamCampbell]

throwing it back in queue infinitely.

Looks like a configuration issue on your side to me?

[EvanDarwin]

I still believe that there should be a way to either handle the exception, or to pass it as null.

[EvanDarwin]

Since it will continue to always be null, retrying isn't going to do anything in this case.

[GrahamCampbell]

or to pass it as null.

We definitely don't want that. That's why it was changed to findOrFail.

[GrahamCampbell]

retrying isn't going to do anything in this case.

That's not our logic though. You configure the retrys.

[valorin]

@GrahamCampbell @EvanDarwin This is actually an issue with you're dealing with _Soft Deletes_. If you try to pass a _soft-deleted_ model into a queued job, it will fail because findOrFail() won't find it - but the model is a perfectly valid model and is still usable.

For example, we have a common class that handles marking models as deleted on the form submission. It then fires a ModelWasDeleted event. I am trying to set up a listener for that event that will queue a job with the deleted event to clean up extra data in the background - but it's failing because I am passing the deleted model into it.

A simple work-around is to pass in the ID, rather than the model itself. However, this breaks our usual pattern and the change in behaviour when using SerializesModels isn't really expected.

[EvanDarwin]

@valorin the workaround I used was to override the getRestoredPropertyValue() function and including the ::withTrashed() method when finding the object.

[EvanDarwin]

Or you can use this snippet for anyone who needs to access deleted models:

    /**
     * Override this to catch in the case that the model is already deleted
     */
    protected function getRestoredPropertyValue($value)
    {
        if($value instanceof ModelIdentifier) {
          $model = (new $value->class)->find($value->id);
          return ($model) ? $model : null;
        } else {
          return $value;
        }
    }

[leitom]

Or remove SerializeModels for deleted resources?

[InstanceOfMichael]

Hey thanks @EvanDarwin for that snippet, it was really helpful. This would be really helpful for large transactions in some scenarios, but IMO the framework's default behavior is fine as long as my extended SerializesModelsNullableTrait continues to work.

[reeslo]

@EvanDarwin I've tried override getRestoredPropertyValue this generate an error :

[2015-10-08 10:47:57] local.ERROR: exception 'ErrorException' with message 'Undefined property: Illuminate\Contracts\Database\ModelIdentifier::$user' 

The model returned is empty nut its not deleted :'(

[kaiserlos]

Thanks @EvanDarwin

[johnpaulmedina]

@EvanDarwin where are you overwriting that method? Does it get placed inside the job class? or on the model? or were you referring to something else?

I have jobs that run and work with softDeleted data but it is failing because it can not findOrFail with trashed on the job. I would like to have certain jobs include trashed items.

What are your thoughts?

[EvanDarwin]

@johnpaulmedina I no longer use Laravel but I believe the code went into the actual Job class.

[valorin]

I made a new trait and overrode the method:

use SerializesModels, SerialisesDeletedModels {
    SerialisesDeletedModels::getRestoredPropertyValue insteadof SerializesModels;
}

use Illuminate\Contracts\Database\ModelIdentifier;

trait SerialisesDeletedModels
{
    protected function getRestoredPropertyValue($value)
    {
        if ($value instanceof ModelIdentifier) {
            $model = new $value->class;

            if (method_exists($model, 'withTrashed')) {
                return $model->withTrashed()->findOrFail($value->id);
            }

            return $model->findOrFail($value->id);
        } else {
            return $value;
        }
    }
}

[emielmolenaar]

Thanks @valorin for the 5.1 example. This works for me, while running Laravel 5.2:

namespace App\Traits;

use Illuminate\Contracts\Database\ModelIdentifier;

/**
 * Class SerialisesDeletedModels
 * @see https://github.com/laravel/framework/issues/9347#issuecomment-165647596
 */
trait SerialisesDeletedModels
{
    /**
     * @param $value
     * @return mixed
     */
    protected function getRestoredPropertyValue($value)
    {
        if (!$value instanceof ModelIdentifier) {
            return $value;
        }

        if (is_array($value->id)) {
            return $this->restoreCollection($value);
        }

        $instance = new $value->class;
        $query = $instance->newQuery()->useWritePdo();

        if (property_exists($instance, 'forceDeleting')) {
            return $query->withTrashed()->find($value->id);
        }

        return $query->findOrFail($value->id);
    }
}

[crabmusket]

@emielmolenaar is there a particular reason you used find on the withTrashed query, but findOrFail on the final query?

    if (property_exists($instance, 'forceDeleting')) {
        return $query->withTrashed()->find($value->id);
    }

    return $query->findOrFail($value->id);

[emielmolenaar]

I guess because findOrFail will throw an exception on models that have been soft-deleted. The withTrashed()->find($value->id) "fixes" this by including soft-deleted models.

[crabmusket]

Right, but could you not have done withTrashed()->findOrFail to throw an exception if the model truly does not exist (not even in the 'trashed' part of the collection)? Just want to make sure I understand!

[emielmolenaar]

Good one. I guess I could have used withTrashed()->findOrFail(), but in this case I only used soft-deleting models so a record would always exist.

When you use soft-delete models and a job gets queued with a non-existing (i.e. no database record) soft-deleted model in it then there is a serious problem: that is a situation that theoretically should not exist :smile:

[EvanDarwin]

@emielmolenaar I don't believe that to be true, there's are plenty of perfectly valid use cases for retrieving a soft-deleted model such as offloading analysis of the model to the task queue. (An example would be analyzing a deleted Submission, etc.)

[valorin]

@emielmolenaar sorry, ignore my last. I miss-read your comment.