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

Uploaded image asset preview is incorrect if an image with same name got uploaded and deleted before #3295

Open
sumtsui opened this issue Dec 25, 2024 · 3 comments
Labels
design 📐 This issue deals with high-level design of a feature P3: minor Non-critical, no workarounds exist type: bug 🐛 Something isn't working @vendure/asset-server-plugin

Comments

@sumtsui
Copy link

sumtsui commented Dec 25, 2024

Describe the bug
When upload an image asset in admin page, and then delete it. Upload a different image with the same name as the one deleted. the deleted one is shown instead of the new image.

To Reproduce
Steps to reproduce the behavior:

  1. Go to admin page
  2. Click on Assets
  3. Click Upload assets
  4. Upload image called XXX.png
  5. Delete the uploaded image from admin page
  6. Upload a different image also called XXX.png
  7. See old image instead of new one

Expected behavior
Should see correct imgae in preview

Environment (please complete the following information):

  • @vendure/core version: 3.1.1
  • Nodejs version 20
  • Database (mysql/postgres etc): postgres

Additional context

@sumtsui sumtsui added the type: bug 🐛 Something isn't working label Dec 25, 2024
@sumtsui
Copy link
Author

sumtsui commented Dec 30, 2024

the asset preview cache is saved:

try {
const parameters = await this.getImageTransformParameters(req);
const image = await transformImage(file, parameters);
const imageBuffer = await image.toBuffer();
const cachedFileName = this.getFileNameFromParameters(req.path, parameters);
if (!req.query.cache || req.query.cache === 'true') {
await this.assetStorageStrategy.writeFileFromBuffer(cachedFileName, imageBuffer);
Logger.debug(`Saved cached asset: ${cachedFileName}`, loggerCtx);
}

but it is not being deleted along with the assets when removed:

private async deleteUnconditional(ctx: RequestContext, assets: Asset[]): Promise<DeletionResponse> {
for (const asset of assets) {
// Create a new asset so that the id is still available
// after deletion (the .remove() method sets it to undefined)
const deletedAsset = new Asset(asset);
await this.connection.getRepository(ctx, Asset).remove(asset);
try {
await this.configService.assetOptions.assetStorageStrategy.deleteFile(asset.source);
await this.configService.assetOptions.assetStorageStrategy.deleteFile(asset.preview);
} catch (e: any) {
Logger.error('error.could-not-delete-asset-file', undefined, e.stack);
}
await this.eventBus.publish(new AssetEvent(ctx, deletedAsset, 'deleted', deletedAsset.id));
}
return {
result: DeletionResult.DELETED,
};
}

@sumtsui
Copy link
Author

sumtsui commented Dec 30, 2024

hmm, but manually removing the cached preview file still will have this issue.

@michaelbromley
Copy link
Member

I think the main challenge here is that the cached images have a hash appended so it is not as simple as just calling "delete" on a specific file.

Let's say we have my-image.jpg. When we have created a bunch of cached previews we'll end up with something like:

my-image__preview_12a34bcde56f7890abcd1234e567890a.jpg
my-image__preview_fa56b7890cd12e34ab567890cdef1234.jpg
my-image__preview_90bcde12a34f5678abcd90ef1234a567.jpg
my-image__preview_78abcd90e1234f56b90cde12a34f5678.jpg
my-image__preview_e1234b56fa78cd90a34bcde12f567890.jpg
my-image__preview_34fa78bc90de1234ab56cd90e12f5678.jpg

The AssetStorageStrategy only has a deleteFile method, which means we need to know the precise file name to delete. We can't simple do "list all files that start with my-image__preview plus hash, and delete". This would work on a local file system but not eg on S3. At least, not without a new method available on the AssetStorageStrategy interface.

Therefore I think this is only solvable by extending the AssetStorageStrategy interface in some way to allow bulk deletion of matching files.

Whatever API we come up with would need to be implementable on local file system (trivial) and s3 (needs research) at a minimum.

@michaelbromley michaelbromley added design 📐 This issue deals with high-level design of a feature @vendure/asset-server-plugin P3: minor Non-critical, no workarounds exist labels Dec 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design 📐 This issue deals with high-level design of a feature P3: minor Non-critical, no workarounds exist type: bug 🐛 Something isn't working @vendure/asset-server-plugin
Projects
None yet
Development

No branches or pull requests

2 participants