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

Scout indexes synced with stale data during transaction #1906

Closed
bakerkretzmar opened this issue Sep 9, 2019 · 9 comments
Closed

Scout indexes synced with stale data during transaction #1906

bakerkretzmar opened this issue Sep 9, 2019 · 9 comments

Comments

@bakerkretzmar
Copy link

With default Nova and Scout setups, model data is sometimes synced to the search index before it's done being saved, which results in the old data being indexed.

  • Laravel Version: 6.0.1
  • Nova Version: 2.2.0
  • PHP Version: 7.2
  • Database Driver and Version: MySQL 8.0.16
  • Operating System and Version: macOS 10.13

Description

Because Nova updates resources inside a transaction, the saved model event is sometimes fired before the new data has been committed and persisted to the database. Scout uses the saved event to trigger updating the search index, so when that update happens, if the new model data hasn't been persisted yet, the search index will be updated with the old data.

See #1759, laravel/scout#152, laravel/framework#8627, laravel/framework#29710, and laravel/ideas#1441.

I would think this is more of a Scout issue, since it would make sense to me to only bother persisting data to a search index once we're absolutely certain that that data is what's saved in our records (i.e. after all transactions are committed), cc @driesvints. Scout hooking updates into the saved event makes that kind of impossible if you use database transactions. Setting scout.queue to false doesn't help here either.

Is it really unreasonable to suggest not using a database transaction to update Nova resources?? Not sure how else we'd get around this.

Steps To Reproduce

  1. Fresh Laravel, Nova, and Scout installs.
  2. Set up Algolia so that Scout works and you can verify the data being synce (not necessary if you just want to see what order things happen in locally).
  3. Create a model observer and set up the saved method to dump something out.
  4. Open up Laravel\Scout\Searchable and dump something from within the queueMakeSearchable method (or from Laravel\Scout\Jobs\MakeSearchable).

Watch dumps: queueMakeSearchable is executed before your own model observer's saved event.

Ugly workaround

I don't know why this works, but if I register an observer to run only during Nova requests rather than globally, things execute in the order I want. For now, this workaround seems to be holding:

// in Nova-specific model observer

public function saving(Model $model)
{
    Model::disableSearchSyncing();
}

public function saved(Model $model)
{
    $model->searchable();
}

I'm turning off search syncing on that model during Nova requests, and then making the model searchable manually after the fact.

🙃

@driesvints
Copy link
Member

This is a tough one since there's no real way or knowing when a transaction has ended within scout. So we don't know when we should persist the data except for model events. I don't see any way we could save this in scout.

The workaround seems to do the trick though? Maybe not the most beautiful solution but when working with transactions it seems like the best approach to me.

@driesvints
Copy link
Member

There is a chance this could be done in Scout if we were to use events with the changed data which then could be synced to algolia in that way. But that would require a major refactoring of the Scout internals.

@fico7489
Copy link

laravel core should implement events for transactions and then scout could listed that events and act when the transaction is rolled back.

PS : I can do it with the external package like this one https://github.com/fntneves/laravel-transactional-events but I don't think that scout team will accept that PR....

@fntneves
Copy link

Hello, @driesvints.

This is an old issue that I've faced several times ago, not only with Nova but also with Laravel. Unfortunately, consistency between transactions and events/jobs and so on is not easy to achieve without additional logic.

In the past I created a package that holds events until the transaction commits. We could discuss a bit more on how to integrate this behavior in Laravel in order to make it more robust and predictable.

@driesvints
Copy link
Member

The source code is open source so anyone is free to attempt a PR.

@mfn
Copy link

mfn commented Jun 17, 2020

Hello everyone,

based on an existing package and with permission of its original author, I've drafted a possible solution to tackle the "transactional events" problem.

At the moment it's a draft PR in my Laravel forked repo to first gather feedback from the community before approaching to create an official PR, to give things time to unfold / develop.

I invite everyone to participate in mfn/laravel-framework#1 and share your feedback.

Thank you!

@bakerkretzmar
Copy link
Author

I think this is resolved for real now in Laravel v8.19.0. If an app has set scout.queue to true, adding 'after_commit' => true to the queue configuration should ensure that Scout only dispatches its sync jobs once transactions are committed. I haven't tested this yet to make sure.

See laravel/framework#35422 and https://divinglaravel.com/better-management-of-database-transactions-in-laravel-8.

@bakerkretzmar
Copy link
Author

As of v8.5.1 Scout now uses $afterCommit internally, so this issue should be completely resolved with no additional setup or configuration necessary.

@LiamKarlMitchell
Copy link

Does not seem to be using afterCommit automatically it does need to be configured.

config/scout.php

    /*
    |--------------------------------------------------------------------------
    | Database Transactions
    |--------------------------------------------------------------------------
    |
    | This configuration option determines if your data will only be synced
    | with your search indexes after every open database transaction has
    | been committed, thus preventing any discarded data from syncing.
    |
    */

    'after_commit' => true, /* Default was false */

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants