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

Database Rollback not working M2.3.0 #21151

Merged
merged 2 commits into from
Mar 2, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 24 additions & 3 deletions app/code/Magento/Backup/Model/Db.php
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@
namespace Magento\Backup\Model;

use Magento\Backup\Helper\Data as Helper;
use Magento\Backup\Model\ResourceModel\Table\GetListTables;
use Magento\Backup\Model\ResourceModel\View\GetViewsBackup;
use Magento\Framework\App\ObjectManager;
use Magento\Framework\Exception\RuntimeException;

Expand Down Expand Up @@ -44,18 +46,35 @@ class Db implements \Magento\Framework\Backup\Db\BackupDbInterface
private $helper;

/**
* @param \Magento\Backup\Model\ResourceModel\Db $resourceDb
* @var GetListTables
*/
private $getListTables;

/**
* @var GetViewsBackup
*/
private $getViewsBackup;

/**
* Db constructor.
* @param ResourceModel\Db $resourceDb
* @param \Magento\Framework\App\ResourceConnection $resource
* @param Helper|null $helper
* @param GetListTables|null $getListTables
* @param GetViewsBackup|null $getViewsBackup
*/
public function __construct(
\Magento\Backup\Model\ResourceModel\Db $resourceDb,
\Magento\Framework\App\ResourceConnection $resource,
?Helper $helper = null
?Helper $helper = null,
GetListTables $getListTables = null,
Copy link
Member

@osrecio osrecio Feb 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be added to end of the construct Backward Compatibility (Adding a constructor parameter)

GetViewsBackup $getViewsBackup = null
) {
$this->_resourceDb = $resourceDb;
$this->_resource = $resource;
$this->helper = $helper ?? ObjectManager::getInstance()->get(Helper::class);
$this->getListTables = $getListTables ?: ObjectManager::getInstance()->get(GetListTables::class);
$this->getViewsBackup = $getViewsBackup ?: ObjectManager::getInstance()->get(GetViewsBackup::class);
}

/**
Expand Down Expand Up @@ -161,7 +180,7 @@ public function createBackup(\Magento\Framework\Backup\Db\BackupInterface $backu

$this->getResource()->beginTransaction();

$tables = $this->getResource()->getTables();
$tables = $this->getListTables->execute();

$backup->write($this->getResource()->getHeader());

Expand Down Expand Up @@ -198,6 +217,8 @@ public function createBackup(\Magento\Framework\Backup\Db\BackupInterface $backu
$backup->write($this->getResource()->getTableDataAfterSql($table));
}
}
$this->getViewsBackup->execute($backup);

$backup->write($this->getResource()->getTableForeignKeysSql());
$backup->write($this->getResource()->getTableTriggersSql());
$backup->write($this->getResource()->getFooter());
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
<?php
/**
* Copyright © Magento, Inc. All rights reserved.
* See COPYING.txt for license details.
*/
declare(strict_types=1);

namespace Magento\Backup\Model\ResourceModel\Table;

use Magento\Framework\App\ResourceConnection;

/**
* Class GetListTables
*/
class GetListTables
{
private const TABLE_TYPE = 'BASE TABLE';

/**
* @var ResourceConnection
*/
private $resource;

/**
* @param ResourceConnection $resource
*/
public function __construct(ResourceConnection $resource)
{
$this->resource = $resource;
}

/**
* Get base tables
*
* @return array
*/
public function execute()
{
return $this->resource->getConnection('backup')->fetchCol(
"SHOW FULL TABLES WHERE `Table_type` = ?",
self::TABLE_TYPE
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicate constant... Delete one please

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like github's suggested change doesn't work properly...

Copy link
Member

@osrecio osrecio Feb 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is a BETA feature. Maybe sometimes doesn't work properly. Sorry

);
}
}
43 changes: 43 additions & 0 deletions app/code/Magento/Backup/Model/ResourceModel/View/GetListViews.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
<?php
/**
* Copyright © Magento, Inc. All rights reserved.
* See COPYING.txt for license details.
*/
declare(strict_types=1);

namespace Magento\Backup\Model\ResourceModel\View;

use Magento\Framework\App\ResourceConnection;

/**
* Class GetListViews
*/
class GetListViews
{
private const TABLE_TYPE = 'VIEW';
/**
* @var ResourceConnection
*/
private $resource;

/**
* @param ResourceConnection $resource
*/
public function __construct(ResourceConnection $resource)
{
$this->resource = $resource;
}

/**
* Get view tables
*
* @return array
*/
public function execute()
{
return $this->resource->getConnection('backup')->fetchCol(
"SHOW FULL TABLES WHERE `Table_type` = ?",
self::TABLE_TYPE
);
}
}
116 changes: 116 additions & 0 deletions app/code/Magento/Backup/Model/ResourceModel/View/GetViewsBackup.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,116 @@
<?php
/**
* Copyright © Magento, Inc. All rights reserved.
* See COPYING.txt for license details.
*/
declare(strict_types=1);

namespace Magento\Backup\Model\ResourceModel\View;

use Magento\Framework\App\ResourceConnection;
use Magento\Framework\Backup\Db\BackupInterface;
use Magento\Framework\DB\Adapter\AdapterInterface;

/**
* Class GetViewsBackup
*/
class GetViewsBackup
{
/**
* @var GetListViews
*/
private $getListViews;

/**
* @var ResourceConnection
*/
private $resourceConnection;

/**
* @var AdapterInterface
*/
private $connection;

/**
* @param GetListViews $getListViews
* @param ResourceConnection $resourceConnection
*/
public function __construct(
GetListViews $getListViews,
ResourceConnection $resourceConnection
) {
$this->getListViews = $getListViews;
$this->resourceConnection = $resourceConnection;
}

/**
* Backup
*
* @param BackupInterface $backup
*/
public function execute(BackupInterface $backup)
{
$views = $this->getListViews->execute();

foreach ($views as $view) {
$backup->write($this->getViewHeader($view));
$backup->write($this->getDropViewSql($view));
$backup->write($this->getShowCreateView($view));
}
}

/**
* Get connection
*
* @return AdapterInterface
*/
private function getConnection()
{
if (!$this->connection) {
$this->connection = $this->resourceConnection->getConnection('backup');
}

return $this->connection;
}

/**
* Get show create view
*
* @param string $viewName
* @return string
*/
private function getShowCreateView($viewName)
{
$quotedViewName = $this->getConnection()->quoteIdentifier($viewName);
$query = 'SHOW CREATE VIEW ' . $quotedViewName;
Copy link
Contributor

@maghamed maghamed Feb 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

creating View we have to use this syntax

                SQL SECURITY INVOKER

the same we do in MSI - https://github.com/magento-engcom/msi/blob/2.3-develop/app/code/Magento/InventoryCatalog/Setup/Patch/Schema/CreateLegacyStockStatusView.php#L69

https://dev.mysql.com/doc/refman/5.6/en/stored-programs-security.html

For a stored routine or view, use SQL SECURITY INVOKER in the object definition when possible so that it can be used only by users with permissions appropriate for the operations performed by the object.

In opposite case, MySQL user working with Magento would not be possible to invoke the View after creation.
This is especially affect our Cloud deployment

Copy link
Contributor Author

@Stepa4man Stepa4man Feb 27, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@maghamed

  1. If this VIEW was created with SQL SECURITY INVOKER declaration - SHOW CREATE VIEW will show SQL statement with this declaration also.
    image

  2. If no - then with SQL SECURITY DEFINER (in this example inventory_stock_2 is also VIEW and was created without SQL SECURITY INVOKER declaration)
    image

So it's ok with the current implementation

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, we already use SQL SECURITY INVOKER at the time of View creation on MSI, so that backup with just pick this setting and at the time of DB restore we keep that setting

$row = $this->getConnection()->fetchRow($query);
$regExp = '/\sDEFINER\=\`([^`]*)\`\@\`([^`]*)\`/';
$sql = preg_replace($regExp, '', $row['Create View']);

return $sql . ';' . "\n";
}

/**
* Get view header
*
* @param string $viewName
* @return string
*/
public function getViewHeader($viewName)
{
$quotedViewName = $this->getConnection()->quoteIdentifier($viewName);
return "\n--\n" . "-- Structure for view {$quotedViewName}\n" . "--\n\n";
}

/**
* Get drop view SQL
*
* @param string $viewName
* @return string
*/
public function getDropViewSql($viewName)
{
$quotedViewName = $this->getConnection()->quoteIdentifier($viewName);
return sprintf('DROP VIEW IF EXISTS %s;' . "\n", $quotedViewName);
}
}