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

ImportAction cannot be used in combination with an async queue driver #10002

Closed
maartenpaauw opened this issue Dec 1, 2023 · 27 comments · Fixed by #10024
Closed

ImportAction cannot be used in combination with an async queue driver #10002

maartenpaauw opened this issue Dec 1, 2023 · 27 comments · Fixed by #10024
Labels
bug Something isn't working low priority unconfirmed

Comments

@maartenpaauw
Copy link
Contributor

maartenpaauw commented Dec 1, 2023

Package

filament/filament

Package Version

v3.1.0

Laravel Version

10.34.2

Livewire Version

3.2.1

PHP Version

8.2.12

Problem description

When using the new CSV import action in combination with an async queue driver (in the example repository I'm using Laravel Horizon), the import action fails / enters an infinite loop.

The problem is app(Authenticatable::class) revolves to NULL using an async driver. Thereby the import user could not be resolved.

A workaround for now is to add the following code to a service provider. It would be better if the importer uses the defined authGuard at panel level. I'm not sure if this is possible.

$this->app->bind(Authenticatable::class, User::class);

Expected behavior

It should be possible to use the CSV import action with an async queue driver.

Steps to reproduce

Clone example repo (it's based on the demo) and try to import a CSV. See the job failing and entering a infinite loop.

Reproduction repository

https://github.com/maartenpaauw/filament-demo/tree/bug/csv-import-with-horizon-queue

Relevant log output

[2023-12-01 14:58:27] local.ERROR: Cannot use "::class" on value of type null {"exception":"[object] (TypeError(code: 0): Cannot use \"::class\" on value of type null at /Users/mpaauw/Personal/filament-demo/vendor/filament/actions/src/Imports/Models/Import.php:43)
[stacktrace]
#0 /Users/mpaauw/Personal/filament-demo/vendor/laravel/framework/src/Illuminate/Database/Eloquent/Builder.php(802): Filament\\Actions\\Imports\\Models\\Import->user()
#1 /Users/mpaauw/Personal/filament-demo/vendor/laravel/framework/src/Illuminate/Database/Eloquent/Relations/Relation.php(110): Illuminate\\Database\\Eloquent\\Builder->Illuminate\\Database\\Eloquent\\{closure}()
#2 /Users/mpaauw/Personal/filament-demo/vendor/laravel/framework/src/Illuminate/Database/Eloquent/Builder.php(800): Illuminate\\Database\\Eloquent\\Relations\\Relation::noConstraints(Object(Closure))
#3 /Users/mpaauw/Personal/filament-demo/vendor/laravel/framework/src/Illuminate/Database/Eloquent/Builder.php(774): Illuminate\\Database\\Eloquent\\Builder->getRelation('user')
#4 /Users/mpaauw/Personal/filament-demo/vendor/laravel/framework/src/Illuminate/Database/Eloquent/Builder.php(754): Illuminate\\Database\\Eloquent\\Builder->eagerLoadRelation(Array, 'user', Object(Closure))
#5 /Users/mpaauw/Personal/filament-demo/vendor/laravel/framework/src/Illuminate/Database/Eloquent/Model.php(704): Illuminate\\Database\\Eloquent\\Builder->eagerLoadRelations(Array)
#6 /Users/mpaauw/Personal/filament-demo/vendor/laravel/framework/src/Illuminate/Queue/SerializesAndRestoresModelIdentifiers.php(109): Illuminate\\Database\\Eloquent\\Model->load(Array)
#7 /Users/mpaauw/Personal/filament-demo/vendor/laravel/framework/src/Illuminate/Queue/SerializesAndRestoresModelIdentifiers.php(62): Filament\\Actions\\Imports\\Jobs\\ImportCsv->restoreModel(Object(Illuminate\\Contracts\\Database\\ModelIdentifier))
#8 /Users/mpaauw/Personal/filament-demo/vendor/laravel/framework/src/Illuminate/Queue/SerializesModels.php(93): Filament\\Actions\\Imports\\Jobs\\ImportCsv->getRestoredPropertyValue(Object(Illuminate\\Contracts\\Database\\ModelIdentifier))
#9 [internal function]: Filament\\Actions\\Imports\\Jobs\\ImportCsv->__unserialize(Array)
#10 /Users/mpaauw/Personal/filament-demo/vendor/laravel/framework/src/Illuminate/Queue/CallQueuedHandler.php(97): unserialize('O:39:\"Filament\\\\...')
#11 /Users/mpaauw/Personal/filament-demo/vendor/laravel/framework/src/Illuminate/Queue/CallQueuedHandler.php(60): Illuminate\\Queue\\CallQueuedHandler->getCommand(Array)
#12 /Users/mpaauw/Personal/filament-demo/vendor/laravel/framework/src/Illuminate/Queue/Jobs/Job.php(102): Illuminate\\Queue\\CallQueuedHandler->call(Object(Illuminate\\Queue\\Jobs\\RedisJob), Array)
#13 /Users/mpaauw/Personal/filament-demo/vendor/laravel/framework/src/Illuminate/Queue/Worker.php(439): Illuminate\\Queue\\Jobs\\Job->fire()
#14 /Users/mpaauw/Personal/filament-demo/vendor/laravel/framework/src/Illuminate/Queue/Worker.php(389): Illuminate\\Queue\\Worker->process('redis', Object(Illuminate\\Queue\\Jobs\\RedisJob), Object(Illuminate\\Queue\\WorkerOptions))
#15 /Users/mpaauw/Personal/filament-demo/vendor/laravel/framework/src/Illuminate/Queue/Worker.php(176): Illuminate\\Queue\\Worker->runJob(Object(Illuminate\\Queue\\Jobs\\RedisJob), 'redis', Object(Illuminate\\Queue\\WorkerOptions))
#16 /Users/mpaauw/Personal/filament-demo/vendor/laravel/framework/src/Illuminate/Queue/Console/WorkCommand.php(137): Illuminate\\Queue\\Worker->daemon('redis', 'default', Object(Illuminate\\Queue\\WorkerOptions))
#17 /Users/mpaauw/Personal/filament-demo/vendor/laravel/framework/src/Illuminate/Queue/Console/WorkCommand.php(120): Illuminate\\Queue\\Console\\WorkCommand->runWorker('redis', 'default')
#18 /Users/mpaauw/Personal/filament-demo/vendor/laravel/horizon/src/Console/WorkCommand.php(51): Illuminate\\Queue\\Console\\WorkCommand->handle()
#19 /Users/mpaauw/Personal/filament-demo/vendor/laravel/framework/src/Illuminate/Container/BoundMethod.php(36): Laravel\\Horizon\\Console\\WorkCommand->handle()
#20 /Users/mpaauw/Personal/filament-demo/vendor/laravel/framework/src/Illuminate/Container/Util.php(41): Illuminate\\Container\\BoundMethod::Illuminate\\Container\\{closure}()
#21 /Users/mpaauw/Personal/filament-demo/vendor/laravel/framework/src/Illuminate/Container/BoundMethod.php(93): Illuminate\\Container\\Util::unwrapIfClosure(Object(Closure))
#22 /Users/mpaauw/Personal/filament-demo/vendor/laravel/framework/src/Illuminate/Container/BoundMethod.php(35): Illuminate\\Container\\BoundMethod::callBoundMethod(Object(Illuminate\\Foundation\\Application), Array, Object(Closure))
#23 /Users/mpaauw/Personal/filament-demo/vendor/laravel/framework/src/Illuminate/Container/Container.php(662): Illuminate\\Container\\BoundMethod::call(Object(Illuminate\\Foundation\\Application), Array, Array, NULL)
#24 /Users/mpaauw/Personal/filament-demo/vendor/laravel/framework/src/Illuminate/Console/Command.php(211): Illuminate\\Container\\Container->call(Array)
#25 /Users/mpaauw/Personal/filament-demo/vendor/symfony/console/Command/Command.php(326): Illuminate\\Console\\Command->execute(Object(Symfony\\Component\\Console\\Input\\ArgvInput), Object(Illuminate\\Console\\OutputStyle))
#26 /Users/mpaauw/Personal/filament-demo/vendor/laravel/framework/src/Illuminate/Console/Command.php(180): Symfony\\Component\\Console\\Command\\Command->run(Object(Symfony\\Component\\Console\\Input\\ArgvInput), Object(Illuminate\\Console\\OutputStyle))
#27 /Users/mpaauw/Personal/filament-demo/vendor/symfony/console/Application.php(1096): Illuminate\\Console\\Command->run(Object(Symfony\\Component\\Console\\Input\\ArgvInput), Object(Symfony\\Component\\Console\\Output\\ConsoleOutput))
#28 /Users/mpaauw/Personal/filament-demo/vendor/symfony/console/Application.php(324): Symfony\\Component\\Console\\Application->doRunCommand(Object(Laravel\\Horizon\\Console\\WorkCommand), Object(Symfony\\Component\\Console\\Input\\ArgvInput), Object(Symfony\\Component\\Console\\Output\\ConsoleOutput))
#29 /Users/mpaauw/Personal/filament-demo/vendor/symfony/console/Application.php(175): Symfony\\Component\\Console\\Application->doRun(Object(Symfony\\Component\\Console\\Input\\ArgvInput), Object(Symfony\\Component\\Console\\Output\\ConsoleOutput))
#30 /Users/mpaauw/Personal/filament-demo/vendor/laravel/framework/src/Illuminate/Foundation/Console/Kernel.php(201): Symfony\\Component\\Console\\Application->run(Object(Symfony\\Component\\Console\\Input\\ArgvInput), Object(Symfony\\Component\\Console\\Output\\ConsoleOutput))
#31 /Users/mpaauw/Personal/filament-demo/artisan(35): Illuminate\\Foundation\\Console\\Kernel->handle(Object(Symfony\\Component\\Console\\Input\\ArgvInput), Object(Symfony\\Component\\Console\\Output\\ConsoleOutput))
#32 {main}
"}
@maartenpaauw maartenpaauw added bug Something isn't working low priority unconfirmed labels Dec 1, 2023
@maartenpaauw
Copy link
Contributor Author

A screenshot of Laravel Horizon log output:

Scherm­afbeelding 2023-12-01 om 15 59 29

@ccc-developer
Copy link

@maartenpaauw Thanks for creating this bug report, documenting the issue, and highlighting how the issue intersects with redis and horizon.

@danharrin danharrin mentioned this issue Dec 2, 2023
3 tasks
@maartenpaauw
Copy link
Contributor Author

maartenpaauw commented Dec 3, 2023

Hi @danharrin! Thank you for picking up this issue so quickly. I've upgraded my Filament project and swapped out $table->foreignId('user_id')->constrained()->cascadeOnDelete(); with $table->morphs('user');, as stated in the newly added docs. I'd also add Import::polymorphicUserRelationship(); to the app service provider.

The imports table is perfectly using a morph when I'm importing a CSV file (the record gets created), but I think the importer itself isn't executed. At the moment I don't know what's going wrong. The changes seems okay. I've updated the reproduction repo / branch with the changes and it's not working either. I think we need to reopen this issue.

Changes based on the new documentation: maartenpaauw/filament-demo@5b6b021

@maartenpaauw
Copy link
Contributor Author

Donnell Wyche shared this on the Filament Discord:

While the error is gone, so is my ability to import anything. It doesn't trigger a job any longer, so nothing happens on import. I will try to replicate on a fresh installation.

@danharrin
Copy link
Member

That's incredibly weird...

@maartenpaauw
Copy link
Contributor Author

maartenpaauw commented Dec 3, 2023

Did some further debugging:

I noticed the league/csv package, used for the import CSV action, got updated from v9.11.0 to v.9.12.0. After rolling back to v9.11.0 the CSV import action, using Redis queue driver, works. I'm not entirely sure what change broke the import.

@danharrin
Copy link
Member

Thanks, that would explain why my testing was ok 😁

@danharrin
Copy link
Member

Do you think I should lock the package to 9.11 in the meantime?

@danharrin
Copy link
Member

Locked, if you could work out the exact problem with 9.12 and report to league/csv that would be very helpful

@maartenpaauw
Copy link
Contributor Author

I think we should lock the package in the meantime until someone finds out what the importer broke. Will do some more debugging later on.

@maartenpaauw
Copy link
Contributor Author

maartenpaauw commented Dec 3, 2023

I think I found something, but I'm not entirely sure; v9.11 yields from combineHeader method and v9.12 returns it. Filament has a custom ChunkIterator, which also uses yields. I think this change broke the ChunkIterator. I never used yield before, so that's the reason why I'm not entirely sure this is the causing issue.

thephpleague/csv@3e0c482#diff-63e150e70c3f2253f8ab94c5a8fe06190bbfee264c842f2c7fb51f9920dd0f2eL219

@maartenpaauw
Copy link
Contributor Author

Adding a quick dd(iterator_to_array($importChunkIterator->get())); after this line: https://github.com/filamentphp/filament/blob/3.x/packages/actions/src/Concerns/CanImportRecords.php#L183

v9.11.0 output is a filled array.
v9.12.0 output is an empty array.

@maartenpaauw
Copy link
Contributor Author

@danharrin I'm kinda stuck a this point. Maybe you know what needs to be changed to the ChunkIterator to support v9.12.0?

@danharrin
Copy link
Member

Should we report the change instead of fixing it on our end?

@maartenpaauw
Copy link
Contributor Author

maartenpaauw commented Dec 3, 2023

I think we could. A return type changed from Generator to MapIterator. I'm not completely familiar with the CSV package, if ->getRecords() is not an internal method then this feels like a breaking change.

Never mind, I'm not sure about it because the return type stayed the same; Iterator. I'm not sure whether we should open a bug report at The PHP League package or fix it at our end. This because I don't have experiences with generators.

@ccc-developer
Copy link

ccc-developer commented Dec 3, 2023

I can confirm that using filament 3.1.8 and downloading and requiring

composer require league/csv:9.11.0

Works for my tests. I am now able to use the ImportAction using Redis queue driver without error.

@indygill
Copy link

I can confirm that using filament 3.1.8 and downloading and requiring

composer require league/csv:9.11.0

Works for my tests. I am now able to use the ImportAction using Redis queue driver without error.

Can confirm this is a valid fix for things on my end too.

@rossbearman
Copy link
Contributor

rossbearman commented Jan 22, 2024

I found this issue after noticing league/csv was being prevented from updating by Filament. Following on from discussion in 829e288:

Based on @maartenpaauw's research the issue was introduced by this change in league/csv, a quick fix was then applied based on this issue, then later rolled back due to the reasoning in this comment. This suggests that the current behaviour is what is expected and if this is still an issue, Filament's ChunkIterator should be updated to work with that behaviour.

Can I suggest that either this issue be re-opened, or a new issue created to track this until league/csv can be unpinned? Pinning a patch version like this has security implications, if there were an upstream issue detected in league/csv that required an urgent fix, Filament would block it.

@maartenpaauw
Copy link
Contributor Author

Adding a quick dd(iterator_to_array($importChunkIterator->get())); after this line: https://github.com/filamentphp/filament/blob/3.x/packages/actions/src/Concerns/CanImportRecords.php#L183

v9.11.0 output is a filled array. v9.12.0 output is an empty array.

I quickly did the same dd at version v.9.14.0 (using redis + Horizon as queue) and the array is not empty anymore. The latest league/csv version seems to fix the problem and we could lock the version at ^9.14.

@maartenpaauw
Copy link
Contributor Author

@rossbearman I've opened up a PR #10990

@danharrin
Copy link
Member

@maartenpaauw I've had a report that it's still broken with 9.14 but 9.11 works, can you do another test please?

@danharrin
Copy link
Member

I'm releasing a fix to the behaviour in the next version, but I'm annoyed that the breaking change was made on their end in the first place

@maartenpaauw
Copy link
Contributor Author

I see you've figured it out what was going wrong 💪

Reference: c74af1f

@meliani
Copy link

meliani commented Feb 7, 2024

The whole day I’ve been trying to make filament native importer to work at the end I discovered this 😍 I waiting for the next patch tonight hhhh

@danharrin
Copy link
Member

It's released

@meliani
Copy link

meliani commented Feb 8, 2024

Thank you, I’ll figure out how to help in this amazing project ! Really thank you @danharrin and thanks to every contributor <3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working low priority unconfirmed
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

6 participants