magento / magento2

Prior to making any Submission(s), you must sign an Adobe Contributor License Agreement, available here at: https://opensource.adobe.com/cla.html. All Submissions you make to Adobe Inc. and its affiliates, assigns and subsidiaries (collectively “Adobe”) are subject to the terms of the Adobe Contributor License Agreement.
http://www.magento.com
Open Software License 3.0
11.56k stars 9.32k forks source link

SELECT from media_storage_file_storage when media DB storage is not in use or disabled #38116

Open ilnytskyi opened 1 year ago

ilnytskyi commented 1 year ago

Preconditions and environment

Steps to reproduce

  1. Disable and never use DB media storage
  2. Make sure the table media_storage_file_storage does not exists image
  3. Allow nginx to fallback to pub/get.php to resolve image
  4. Access non existent image e.g. https://magento.local/media/wysiwyg/someImage.jpg

Expected result

  1. The code execution does not trigger queries to media_storage_file_storage table
  2. The user gets placeholder image without triggering media_storage_file_storage table
  3. Mysql service does not contain warning logs about accessing the table media_storage_file_storage
  4. Exceptions are logged in and not silently suppressed Magento\MediaStorage\Model\File\Storage\Synchronization::synchronize
  5. Magento\MediaStorage\Model\File\Storage\Synchronization::synchronize does not use disabled DB media storage

Actual result

  1. The request triggers the queries to media_storage_file_storage table
  2. Mysql service contains warning logs about accessing the table media_storage_file_storage that does not exists
  3. Exceptions are not logged and silently suppressed in Magento\MediaStorage\Model\File\Storage\Synchronization::synchronize
  4. Magento\MediaStorage\Model\File\Storage\Synchronization::synchronize does not uses disabled DB media storage

Additional information

The execution path according blackfire

PDOStatement::execute(select e.* from media_storage_file_storage as e where (filename = ?) and (directory = ?))
 Magento\MediaStorage\Model\File\Storage\Synchronization::synchronize
 Magento\MediaStorage\App\Media\Interceptor::createLocalCopy
 Magento\MediaStorage\App\Media\Interceptor::launch
 Magento\Framework\App\Bootstrap::run
 run_init::pub/get.php

https://github.com/magento/magento2/blob/2.4-develop/app/code/Magento/MediaStorage/Model/File/Storage/Synchronization.php#L57 The Magento\MediaStorage\Model\File\Storage\Synchronization::synchronize executes a this code:

 /** @var $storage Database */
        $storage = $this->storageFactory->create();
        try {
            $storage->loadByFilename($relativeFileName);
        } catch (\Exception $e) {
        }

The workaround can be to adjust nginx config, but still the code has to be refactored, as no warnings logged, and query is fired to non-existent table.

Release note

During accessing images via pub/get.php the select queries to media_storage_file_storage are not fired when DB media storage is disabled.

Triage and priority

m2-assistant[bot] commented 1 year ago

Hi @ilnytskyi. Thank you for your report. To speed up processing of this issue, make sure that the issue is reproducible on the vanilla Magento instance following Steps to reproduce. To deploy vanilla Magento instance on our environment, Add a comment to the issue:

m2-assistant[bot] commented 1 year ago

Hi @engcom-November. Thank you for working on this issue. In order to make sure that issue has enough information and ready for development, please read and check the following instruction: :point_down:

engcom-November commented 1 year ago

Hello @ilnytskyi,

Thank you for the report and collaboration!

Can you please elaborate the 3rd step in Steps to reproduce, and it would be great if you could provide code examples and use case for the issue. This will help us to reproduce the issue more effectively.

Thank you.

ilnytskyi commented 1 year ago

@engcom-November You can check the message with xdebug

 /** @var $storage Database */
        $storage = $this->storageFactory->create();
        try {
            $storage->loadByFilename($relativeFileName);
        } catch (\Exception $e) {
           //here or just
          var_dump($e);
        }

On the other hand you can enable DB query logging bin/magento dev:query-log:enable or directly on DB and see that magento sends selects to media_storage_file_storage table.

engcom-November commented 11 months ago

Hello @ilnytskyi,

Thank you for the quick reply!

We were able to reproduce this issue on 2.4-develop. Please take a look at the screenshot below: image Here we can see a query to the non-existent table media_storage_file_storage. Hence confirming the issue.

Thank you.

github-jira-sync-bot commented 11 months ago

:white_check_mark: Jira issue https://jira.corp.adobe.com/browse/AC-10586 is successfully created for this GitHub issue.

m2-assistant[bot] commented 11 months ago

:white_check_mark: Confirmed by @engcom-November. Thank you for verifying the issue.
Issue Available: @engcom-November, You will be automatically unassigned. Contributors/Maintainers can claim this issue to continue. To reclaim and continue work, reassign the ticket to yourself.

github-jira-sync-bot commented 11 months ago

:x: You don't have permission to export this issue.

leonhelmus commented 4 months ago

Is there already a patch available from Adobe commerce?

CC. @engcom-November

leonhelmus commented 4 months ago

`` For now this patch of a collegue seems to resolve it:

diff --git a/Model/ResourceModel/File/Storage/Database.php b/Model/ResourceModel/File/Storage/Database.php
--- a/Model/ResourceModel/File/Storage/Database.php
+++ b/Model/ResourceModel/File/Storage/Database.php
@@ -238,6 +238,7 @@
      */
     public function loadByFilename(\Magento\MediaStorage\Model\File\Storage\Database $object, $filename, $path)
     {
+        return $this;
         $connection = $this->getConnection();

         $select = $connection->select()->from(

At the moment we get more than 19.2k requests in a week which generates this error and pollutes our monitoring.

or

diff --git a/Model/ResourceModel/File/Storage/Database.php b/Model/ResourceModel/File/Storage/Database.php
--- a/Model/ResourceModel/File/Storage/Database.php
+++ b/Model/ResourceModel/File/Storage/Database.php
@@ -238,6 +238,8 @@
      */
     public function loadByFilename(\Magento\MediaStorage\Model\File\Storage\Database $object, $filename, $path)
     {
+        return $this;
+
         $connection = $this->getConnection();

         $select = $connection->select()->from(
vanpear commented 2 months ago

I created a Composer Patch: magento/module-media-storage

diff --git a/Model/File/Storage/Synchronization.php b/Model/File/Storage/Synchronization.php
index 3ee2rd..8349152 111644
--- a/Model/File/Storage/Synchronization.php
+++ b/Model/File/Storage/Synchronization.php
@@ -10,6 +10,7 @@
 use Magento\Framework\Exception\FileSystemException;
 use Magento\Framework\Filesystem\Directory\WriteInterface as DirectoryWrite;
 use Magento\Framework\Filesystem\File\WriteInterface;
+use Magento\MediaStorage\Helper\File\Storage\Database as DatabaseStorageHelper;

 /**
  * Synchronize files from Db storage to local file system
@@ -31,15 +32,23 @@
     protected $mediaDirectory;

     /**
+     * @var DatabaseStorageHelper $databaseStorageHelper
+     */
+    protected $databaseStorageHelper;
+
+    /**
      * @param DatabaseFactory $storageFactory
      * @param DirectoryWrite $directory
+     * @param DatabaseStorageHelper $databaseStorageHelper
      */
     public function __construct(
         DatabaseFactory $storageFactory,
-        DirectoryWrite $directory
+        DirectoryWrite $directory,
+        DatabaseStorageHelper $databaseStorageHelper
     ) {
         $this->storageFactory = $storageFactory;
         $this->mediaDirectory = $directory;
+        $this->databaseStorageHelper = $databaseStorageHelper;
     }

     /**
@@ -51,6 +60,10 @@
      */
     public function synchronize($relativeFileName)
     {
+        if (!$this->databaseStorageHelper->checkDbUsage()) {
+            return;
+        }
+        
         /** @var $storage Database */
         $storage = $this->storageFactory->create();
         try {