Renaming base64 added image using S3 driver, causes crash when using media conversions

[SlyDave]

As discussed in #3089 because adding a base64 media via addMediaFromBase64() doesn't correctly set the file extension, and using usingFileName() isn't an option when you don't know the source mimetype. I have taken to renaming the filename of the Media after it has been added to the collection (it is always added as .tmp)

This was working fine, however, (I believe) since upgrading from version 9 from 10 of medialibrary it's crashing with an Exception from the AWS S3 Adapter because renameConversionFiles() attempts to move a conversion with the same name to the same location, Something not permitted by S3 - The conversions have the correct extension the first place, as they work out the correct extension when they're generated...

Unable to copy file from f665fb39-8312-49a9-a68a-aa6b182a068d/conversions/0104a278-3b95-43ff-af85-7564446a7fd8-thumb.jpg to f665fb39-8312-49a9-a68a-aa6b182a068d/conversions/0104a278-3b95-43ff-af85-7564446a7fd8-thumb.jpg

[previous exception] [object] (Aws\\S3\\Exception\\S3Exception(code: 0): Error executing \"CopyObject\" on \"https://workrest-public.s3.eu-west-2.amazonaws.com/avatars/f665fb39-8312-49a9-a68a-aa6b182a068d/conversions/0104a278-3b95-43ff-af85-7564446a7fd8-thumb.jpg\"; AWS HTTP error: Client error: `PUT https://workrest-public.s3.eu-west-2.amazonaws.com/avatars/f665fb39-8312-49a9-a68a-aa6b182a068d/conversions/0104a278-3b95-43ff-af85-7564446a7fd8-thumb.jpg` resulted in a `400 Bad Request` response:
<Error><Code>InvalidRequest</Code><Message>This copy request is illegal because it is trying to copy an object to itself (truncated...)
 InvalidRequest (client): This copy request is illegal because it is trying to copy an object to itself without changing the object's metadata, storage class, website redirect location or encryption attributes. - <Error><Code>InvalidRequest</Code><Message>This copy request is illegal because it is trying to copy an object to itself without changing the object's metadata, storage class, website redirect location or encryption attributes.</Message><RequestId>4ZA3W6RZ2PZ55E4H</RequestId><HostId>B5PnYm0arCfGUHV+0GRz1g0PBlx8eoeYkBGo1OL6V+k6MQebxTcSk+4cR5nvHE11TuAQhetAF9OXvLqzDTYbug==</HostId></Error> at /var/app/current/vendor/aws/aws-sdk-php/src/WrappedHttpHandler.php:195)

/var/app/current/vendor/league/flysystem-aws-s3-v3/AwsS3V3Adapter.php:411
/var/app/current/vendor/league/flysystem/src/Filesystem.php:125
/var/app/current/vendor/laravel/framework/src/Illuminate/Filesystem/FilesystemAdapter.php:537
/var/app/current/vendor/spatie/laravel-medialibrary/src/MediaCollections/Filesystem.php:309
/var/app/current/vendor/spatie/laravel-medialibrary/src/MediaCollections/Filesystem.php:257
/var/app/current/vendor/spatie/laravel-medialibrary/src/MediaCollections/Models/Observers/MediaObserver.php:31
/var/app/current/vendor/laravel/framework/src/Illuminate/Events/Dispatcher.php:441
/var/app/current/vendor/laravel/framework/src/Illuminate/Events/Dispatcher.php:249
/var/app/current/vendor/laravel/framework/src/Illuminate/Events/Dispatcher.php:222
/var/app/current/vendor/laravel/framework/src/Illuminate/Database/Eloquent/Concerns/HasEvents.php:189
/var/app/current/vendor/laravel/framework/src/Illuminate/Database/Eloquent/Model.php:1197
/var/app/current/vendor/laravel/framework/src/Illuminate/Database/Eloquent/Model.php:1131
/var/app/current/app/Models/User.php:403

I believe in MedicaCollections\Filesystem::renameConversionFiles() if a check is added to see if $oldFile and $newFile do in fact differ in their filepath or filename, then do the move, otherwise there isn't a move to be done?

[SlyDave]

Confirmed that is what is going on

[SlyDave]

Another piece of the puzzle, this only occurs when a non-sync queue driver is used, and that the queue'd job for conversion runs faster than the MediaObserver can process the renameConversionFiles, otherwise a check in that function will skip moving the file (as it's not available on the $disk yet). So a little bit of a race condition as to whether or not you get this error, switching the QUEUE_CONNECTION to sync will cause the error to always occur.

[SlyDave]

As a work around for this:

in AppServicveProvider add:

use Spatie\MediaLibrary\MediaCollections\Filesystem as MediaLibraryFilesystem;
use App\Services\MediaLibrary\Filesystem;

in the AppServicveProvider::register() function add:
$this->app->bind(MediaLibraryFilesystem::class, Filesystem::class);

Create a new class (in my case I called it Fileystem under App\Services\MediaLibrary) and populate it as follows:

<?php
namespace App\Services\MediaLibrary;

use Spatie\MediaLibrary\Conversions\ConversionCollection;
use Spatie\MediaLibrary\MediaCollections\Filesystem as MediaLibraryFilesystem;
use Spatie\MediaLibrary\MediaCollections\Models\Media;

class Filesystem extends MediaLibraryFilesystem
{
    protected function renameConversionFiles(Media $media): void
    {
        $mediaWithOldFileName = config('media-library.media_model')::find($media->id);
        $mediaWithOldFileName->file_name = $mediaWithOldFileName->getOriginal('file_name');

        $conversionDirectory = $this->getConversionDirectory($media);

        $conversionCollection = ConversionCollection::createForMedia($media);

        foreach ($media->getMediaConversionNames() as $conversionName) {
            $conversion = $conversionCollection->getByName($conversionName);

            $oldFile = $conversionDirectory.$conversion->getConversionFile($mediaWithOldFileName);
            $newFile = $conversionDirectory.$conversion->getConversionFile($media);

            $disk = $this->filesystem->disk($media->conversions_disk);

            // No move required, old and new file filepath/filename are the same
            if ($oldFile === $newFile) {
                continue;
            }

            // A media conversion file might be missing, waiting to be generated, failed etc.
            if (! $disk->exists($oldFile)) {
                continue;
            }

            $disk->move($oldFile, $newFile);
        }
    }
}

This replaces renameConversionFiles from the MediaLibraryFilesystem with a patch to fix the issue, a 3 liner, that compares the $oldFile vs the $newFile and if they're the same, it doesn't do the move.

[fredsal]

Duplicate of #3089
PRs are welcome, feel free to make one to fix that if you think that there is a bug

[SlyDave]

Cheers @fredsal but as you may have noticed (and is literally the fist line of this issue), #3089 is a discussion, not an issue. not sure how a issue can duplicate a discussion, but here we are.

[fredsal]

you may have noticed

Of course i notice, but you should know that if you already opened a discussion and they already gave you an answer you don't need to open an issue

And still

PRs are welcome, feel free to make one to fix that if you think that there is a bug

[SlyDave]

You'll continue to notice then the time difference between the issue and the discussion and the discussion was elevated into an issue once an actual potential issue was identified, you'll also notice that the issue in question goes into great detail.

Frankly I'm not sure what your problem is, your not being constructive to the process, you're not providing insight into the issue or assisting in the development of a solution, you're not commenting on the potential fix or providing any kind of peer review or reason as to whether the suggested approach is viable, or if it's good for the project or heads in a direction that is suitable.

One thing you're correct about is PRs are indeed welcome. I invite you to be a constructive contributor

[fredsal]

S3 is a third party package, compatibility is not guaranteed, apparently you are the only one with this problem, it makes us think that you are doing something wrong (nobody wastes time on thos cases)

I invite you to be a constructive contributor

Same to you, If you don't do it, I guarantee that it will take years and you will not receive another answer than the one I already gave you, good luck.

#3148 (comment)
#3090 (comment)

[SlyDave]

Who hurt you in a past life Fredsal? I really don't understand what your beef with me is, You've got nothing to gain by being so condescending and deeming to other members of the community.

All I have done here is to log an issue and detail the situation under which it occurs, No one asked you to fix it, or to tell someone else to fix it. Yes it only occurs under S3, because as detailed S3 as the storage layer used in flysystem doesn't allow move commands from the same object id to the same object id (or rather, it did, but no longer does).

Maybe I am the only user with this problem -- does that mean it any less of an issue? (that's rhetorical by the way, the answer is no it doesn't), does it mean the issue shouldn't be logged? I don't think so - more so when I've even provided a way to work around the issue until an adequate solution has been found.

Maybe I am doing something wrong, you've been mightily helpful in trying to diagnosis the issue, reproduce, or other any form of functional constructive advice that might highlight the errors of my ways. (that by the way, was sarcasm, you've been quite quite pointless in all your posts, but thanks for trying?)

From the start you seem to be under the assumption that A: I expect someone else to fix this, B: that I wont. Not once have you stopped to consider I'm logging the issue, my finding, my possible solutions, with work arounds until a PR can be created, and merged. no, instead, you've gone on some personal crusade against me for some unknown reason. bizarre behaviour frankly.

And finally you've linked to comments from freek, someone you can't speak for. You'll also notice that freek has vetted those issues and tagged them accordingly.

Same to you, If you don't do it, I guarantee that it will take years

That it might, Not sure what that's got to do with you? sounds like my problem not yours, Although if you actually wanted to be helpful you might even look into the problem and say if the proposed solution is sufficient and looks like it's a good candidate for a PR. Heck, you might even try and reproduce the issue and confirm it and work with another member of the community (like say, me) and develop a solution, but I suggest that is far too much to ask for, so I wont.

Please go bother someone else, or provide actual useful discourse

[fredsal]

Ok, don't go beyond the first line, my time is more important than reading what I imagine are complaints
I don't understand why you suffer so much from the response that everyone else receives and doesn't suffer

So, PRs are welcome, feel free to make one to fix that if you think that there is a bug, have a good day

[SlyDave]

Given you can't even have the common courtesy of reading a response to your comments, you'd notice it wasn't complaints. But for someone that values their time so greatly you sure are invested in replying over and over, So I have to question just how much value you place on things, heh. good day indeed.