[9.x] Scout sensitive attributes

[juampi92]

This small addition will prevent firing engine updates if sensitive attributes were not changed.

Problem

We noticed in our project that we were performing many small updates on attributes that are irrelevant for the search, yet every change was still firing an Algolia update job.

Even though update events are free in Algolia, our models have a toSearchableArray structure that requires many relationships to be loaded, so we have a lot to gain from not firing a job when possible.

Solution

This PR lets developers define the method scoutSensitiveAttributes to return an array of the sensitive attributes that only when changed, the engine should be updated.

Deleted and restored events

When restoring/soft-deleting a model, the model itself doesn't have any dirty attributes, so the sensitive attributes have to be ignored. For this scenario, we use the forceSaving (similar to the SoftDeletes forceDelete logic) where we skip the sensitive attributes check entirely.

Notes

I added a new test to test the soft delete flow in the ModelObserver, which wasn't being tested before.

[taylorotwell]

Is it possible to take an approach with a lighter touch that just gives you the hook for searchShouldUpdate and within that method you can do whatever you want. That way we don't have to introduce an entirely new concept of "Scout sensitive attributes".

[juampi92]

What about something like this?

    /**
     * Given a list of updated attribute keys, this method determines
     * if we should perform a search engine update or not.
     *
     * @param  array  $updatedAttributeKeys
     * @return bool
     */
    public function searchShouldUpdate(array $updatedAttributeKeys): bool
    {
        return true;
    }

And then to solve the problem of the sensitive attributes, this would be an example overwriting the hook:

    /**
     * Given a list of updated attribute keys, this method determines
     * if we should perform an engine update or not.
     *
     * @param  array  $updatedAttributeKeys
     * @return bool
     */
    public function searchShouldUpdate(array $updatedAttributeKeys): bool
    {
        $sensitiveAttributeKeys = ['published_at', 'title', 'content'];

        return count(array_intersect($updatedAttributeKeys, $sensitiveAttributeKeys)) > 0;
    }

(or you can even do it with collections as I did on the implementation)

Observations

I understand that being more generic and extensible allows devs to be creative, and this hook might help solve other problems too, which is great.

But I also can't stop thinking that it's up to the dev now to figure out how to use this method for something.
Having an out-of-the-box feature that prevents unnecessary updates based on which attributes were modified is something that won't make sense in all scenarios, but now devs will be aware of this problem and know that the package provides a solution for it (that is entirely optional).

I guess this will make more sense when writing the documentation. We could easily tell the dev that they can use searchShouldUpdate for this exact purpose.

[taylorotwell]

We can document this use case as a possible use of this method in the documentation so that it spurs the idea in their head.

[juampi92]

Do you think that we should send the array $updatedAttributeKeys or let them do array_keys($model->getDirty()) by themselves ?

Perhaps, by sending the updatedAttributeKeys, we're forcing them into using the method a specific way, but then if they only use it to check which keys are being updated, it could save a bit of repetitive code

(I decided to not send the updated attributes, let me know what you think. I can add them if you consider them useful)

[taylorotwell]

I would also get rid of this concept of "force saved".

[taylorotwell]

Please also fix the conflicts on this PR and mark as ready for review when done.

[juampi92]

Sure thing.

But I still have a concern regarding the soft-delete events:

When we create/update a model, the saved event will be fired before the attributes are synced with the originals, so we'll have them as dirty and we can make the check for the sensitive.
But when we delete/restore, the deleted_at attribute is not dirty. I guess the attributes get synced before the event is fired, so we never see any change (just like when forceDeleting).

I introduced the force saving concept to skip the searchShouldUpdate check when deleting/restoring, and I wanna find an alternative to it now that you said that you don't want it.

Do you think it makes sense to assume that if the dirty attributes are empty, it means that it's being deleted/restored?

I added that here

[taylorotwell]

I see what you're saying - but there should not be a "forceSaved" method... it should all be internal. Add it back in and I can try to massage it how I want it to be.

[juampi92]

Done!

Curious to see what you'll end up doing. Thanks for the help man! Love your work

[taylorotwell]

Made my changes and merged.