From d47464b060c063b81bfaf1c1c72a6b4814aca56e Mon Sep 17 00:00:00 2001 From: "martijn.vanhaagen" Date: Fri, 6 Aug 2021 20:48:55 +0200 Subject: [PATCH 1/5] [FEATURE][AOM2-172] Started using count in reports --- Api/Data/ReportInterface.php | 12 ++++++ Model/Data/Report.php | 19 ++++++++++ Model/ReportRepository.php | 72 +++++++++++++++++++++++++++--------- etc/db_schema.xml | 1 + 4 files changed, 86 insertions(+), 18 deletions(-) diff --git a/Api/Data/ReportInterface.php b/Api/Data/ReportInterface.php index 865747a..934f916 100644 --- a/Api/Data/ReportInterface.php +++ b/Api/Data/ReportInterface.php @@ -124,5 +124,17 @@ public function getDate(); */ public function setDate($date); + /** + * Get Count + * @return string|null + */ + public function getCount(); + + /** + * @param $count + * @return \Experius\Csp\Api\Data\ReportInterface + */ + public function setCount($count); + } diff --git a/Model/Data/Report.php b/Model/Data/Report.php index ee47d4a..b7b8f33 100644 --- a/Model/Data/Report.php +++ b/Model/Data/Report.php @@ -165,5 +165,24 @@ public function setDate($date) return $this->setData(self::DATE, $date); } + /** + * Get count + * @return string|null + */ + public function getCount() + { + return $this->_get(self::COUNT); + } + + /** + * Set count + * @param string $count + * @return \Experius\Csp\Api\Data\ReportInterface + */ + public function setCount($count) + { + return $this->setData(self::COUNT, $count); + } + } diff --git a/Model/ReportRepository.php b/Model/ReportRepository.php index 944ffb1..743eae3 100644 --- a/Model/ReportRepository.php +++ b/Model/ReportRepository.php @@ -16,11 +16,13 @@ use Magento\Framework\Api\ExtensibleDataObjectConverter; use Magento\Framework\Api\ExtensionAttribute\JoinProcessorInterface; use Magento\Framework\Api\SearchCriteria\CollectionProcessorInterface; +use Magento\Framework\Exception\AbstractAggregateException; use Magento\Framework\Exception\CouldNotDeleteException; use Magento\Framework\Exception\CouldNotSaveException; use Magento\Framework\Exception\NoSuchEntityException; use Magento\Framework\Reflection\DataObjectProcessor; use Magento\Store\Model\StoreManagerInterface; +use Magento\Framework\Api\SearchCriteriaBuilder; class ReportRepository implements ReportRepositoryInterface { @@ -46,8 +48,10 @@ class ReportRepository implements ReportRepositoryInterface private $collectionProcessor; + protected $searchCriteriaBuilder; /** + * ReportRepository constructor. * @param ResourceReport $resource * @param ReportFactory $reportFactory * @param ReportInterfaceFactory $dataReportFactory @@ -59,6 +63,7 @@ class ReportRepository implements ReportRepositoryInterface * @param CollectionProcessorInterface $collectionProcessor * @param JoinProcessorInterface $extensionAttributesJoinProcessor * @param ExtensibleDataObjectConverter $extensibleDataObjectConverter + * @param SearchCriteriaBuilder $searchCriteriaBuilder */ public function __construct( ResourceReport $resource, @@ -71,7 +76,8 @@ public function __construct( StoreManagerInterface $storeManager, CollectionProcessorInterface $collectionProcessor, JoinProcessorInterface $extensionAttributesJoinProcessor, - ExtensibleDataObjectConverter $extensibleDataObjectConverter + ExtensibleDataObjectConverter $extensibleDataObjectConverter, + SearchCriteriaBuilder $searchCriteriaBuilder ) { $this->resource = $resource; $this->reportFactory = $reportFactory; @@ -84,6 +90,7 @@ public function __construct( $this->collectionProcessor = $collectionProcessor; $this->extensionAttributesJoinProcessor = $extensionAttributesJoinProcessor; $this->extensibleDataObjectConverter = $extensibleDataObjectConverter; + $this->searchCriteriaBuilder = $searchCriteriaBuilder; } /** @@ -96,24 +103,25 @@ public function save( $storeId = $this->storeManager->getStore()->getId(); $report->setStoreId($storeId); } */ - - $reportData = $this->extensibleDataObjectConverter->toNestedArray( - $report, - [], - \Experius\Csp\Api\Data\ReportInterface::class - ); - - $reportModel = $this->reportFactory->create()->setData($reportData); - - try { - $this->resource->save($reportModel); - } catch (\Exception $exception) { - throw new CouldNotSaveException(__( - 'Could not save the report: %1', - $exception->getMessage() - )); + + $existingReport = $this->checkReportExist($report); + + if(!$existingReport) { + try { + $this->resource->save($this->createReportModel($report)); + } catch (\Exception $exception) { + throw new CouldNotSaveException(__( + 'Could not save the report: %1', + $exception->getMessage() + )); + } + return $report->getDataModel(); + } else { + $existingReport->setCount($existingReport->getCount() + 1); + $this->resource->save($this->createReportModel($existingReport)); + + return $existingReport->getDataModel(); } - return $reportModel->getDataModel(); } /** @@ -183,5 +191,33 @@ public function deleteById($reportId) { return $this->delete($this->get($reportId)); } + + /** + * @param $reportData + * @return \Experius\Csp\Api\Data\ReportInterface|false|null + * @throws \Magento\Framework\Exception\LocalizedException + */ + public function checkReportExist($report) + { + if ($report) { + $searchCriteria = $this->searchCriteriaBuilder->addFilter('violated_directive', $report->getViolatedDirective(), 'eq')->create(); + $searchCriteria = $this->searchCriteriaBuilder->addFilter('blocked_uri', $report->getBlockedUri(), 'eq')->create(); + $searchCriteria = $this->searchCriteriaBuilder->addFilter('document_uri', $report->getDocumentUri(), 'eq')->create(); + $report = reset($this->getList($searchCriteria)->getItems()); + } + + return $report; + } + + public function createReportModel($report) + { + $reportData = $this->extensibleDataObjectConverter->toNestedArray( + $report, + [], + \Experius\Csp\Api\Data\ReportInterface::class + ); + + return $this->reportFactory->create()->setData($reportData); + } } diff --git a/etc/db_schema.xml b/etc/db_schema.xml index 1405c0f..1749f75 100644 --- a/etc/db_schema.xml +++ b/etc/db_schema.xml @@ -23,5 +23,6 @@ + From 4179e5309be7a173fc50f86bf1181757171e1ed2 Mon Sep 17 00:00:00 2001 From: "martijn.vanhaagen" Date: Mon, 9 Aug 2021 11:34:12 +0200 Subject: [PATCH 2/5] [FEATURE][DONS-156] Added delay + fixed filters --- Model/ReportRepository.php | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/Model/ReportRepository.php b/Model/ReportRepository.php index 743eae3..b9acad2 100644 --- a/Model/ReportRepository.php +++ b/Model/ReportRepository.php @@ -104,6 +104,10 @@ public function save( $report->setStoreId($storeId); } */ + // random microsecond to prevent double saves + $sleep = rand(1, 1000000); + usleep($sleep); + // check for existing report $existingReport = $this->checkReportExist($report); if(!$existingReport) { @@ -193,16 +197,18 @@ public function deleteById($reportId) } /** - * @param $reportData - * @return \Experius\Csp\Api\Data\ReportInterface|false|null - * @throws \Magento\Framework\Exception\LocalizedException + * @param $report + * @return \Experius\Csp\Api\Data\ReportInterface|false|mixed */ public function checkReportExist($report) { if ($report) { - $searchCriteria = $this->searchCriteriaBuilder->addFilter('violated_directive', $report->getViolatedDirective(), 'eq')->create(); - $searchCriteria = $this->searchCriteriaBuilder->addFilter('blocked_uri', $report->getBlockedUri(), 'eq')->create(); - $searchCriteria = $this->searchCriteriaBuilder->addFilter('document_uri', $report->getDocumentUri(), 'eq')->create(); + $searchCriteria = $this->searchCriteriaBuilder + ->addFilter('violated_directive', $report->getViolatedDirective(), 'eq') + ->addFilter('blocked_uri', $report->getBlockedUri(), 'eq') + ->addFilter('document_uri', $report->getDocumentUri(), 'eq') + ->create(); + $report = reset($this->getList($searchCriteria)->getItems()); } From 14409747f656c88f1be3a99d4030a1f54dcbae2f Mon Sep 17 00:00:00 2001 From: Boris van Katwijk Date: Thu, 12 Aug 2021 11:45:50 +0200 Subject: [PATCH 3/5] [BUGFIX][AOM2-172] Fatal error: Uncaught Error: Undefined class constant 'COUNT' in ReportInterface instances. --- Api/Data/ReportInterface.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Api/Data/ReportInterface.php b/Api/Data/ReportInterface.php index 934f916..23e378f 100644 --- a/Api/Data/ReportInterface.php +++ b/Api/Data/ReportInterface.php @@ -9,7 +9,6 @@ interface ReportInterface extends \Magento\Framework\Api\ExtensibleDataInterface { - const VIOLATED_DIRECTIVE = 'violated_directive'; const REPORT_ID = 'report_id'; const DOCUMENT_URI = 'document_uri'; @@ -17,6 +16,7 @@ interface ReportInterface extends \Magento\Framework\Api\ExtensibleDataInterface const ORIGINAL_POLICY = 'original_policy'; const DATE = 'date'; const BLOCKED_URI = 'blocked_uri'; + const COUNT = 'count'; /** * Get report_id From e6114c12d0312f70e67f003797b6130059c81ac9 Mon Sep 17 00:00:00 2001 From: Boris van Katwijk Date: Thu, 12 Aug 2021 11:46:38 +0200 Subject: [PATCH 4/5] [BUGFIX][AOM2-172] Refactor report existance in save() function to properly function without any reports being active. Small simplification/cleanup for ease of reading the code. --- Model/ReportRepository.php | 61 ++++++++++++++++++++++---------------- 1 file changed, 35 insertions(+), 26 deletions(-) diff --git a/Model/ReportRepository.php b/Model/ReportRepository.php index b9acad2..b63a32b 100644 --- a/Model/ReportRepository.php +++ b/Model/ReportRepository.php @@ -7,6 +7,7 @@ namespace Experius\Csp\Model; +use Experius\Csp\Api\Data\ReportInterface; use Experius\Csp\Api\Data\ReportInterfaceFactory; use Experius\Csp\Api\Data\ReportSearchResultsInterfaceFactory; use Experius\Csp\Api\ReportRepositoryInterface; @@ -97,34 +98,32 @@ public function __construct( * {@inheritdoc} */ public function save( - \Experius\Csp\Api\Data\ReportInterface $report + ReportInterface $report ) { - /* if (empty($report->getStoreId())) { - $storeId = $this->storeManager->getStore()->getId(); - $report->setStoreId($storeId); - } */ + // @TODO: [Should have] Move detection if already exists outside of the save() function + // @TODO: [Nice to have] Find the reason csp reports are sent directly at the same time, to prevent having to + // to use the usleep() mechanic to de-synchronise saving. - // random microsecond to prevent double saves + // Sleep for a random microsecond to prevent double saves $sleep = rand(1, 1000000); usleep($sleep); - // check for existing report - $existingReport = $this->checkReportExist($report); + $existingReport = $this->doesReportExistAlready($report); if(!$existingReport) { try { - $this->resource->save($this->createReportModel($report)); + $report = $this->resource->save($this->createReportModel($report)); } catch (\Exception $exception) { throw new CouldNotSaveException(__( 'Could not save the report: %1', $exception->getMessage() )); } - return $report->getDataModel(); + return $report; } else { $existingReport->setCount($existingReport->getCount() + 1); - $this->resource->save($this->createReportModel($existingReport)); + $existingReport = $this->resource->save($this->createReportModel($existingReport)); - return $existingReport->getDataModel(); + return $existingReport; } } @@ -151,7 +150,7 @@ public function getList( $this->extensionAttributesJoinProcessor->process( $collection, - \Experius\Csp\Api\Data\ReportInterface::class + ReportInterface::class ); $this->collectionProcessor->process($criteria, $collection); @@ -173,7 +172,7 @@ public function getList( * {@inheritdoc} */ public function delete( - \Experius\Csp\Api\Data\ReportInterface $report + ReportInterface $report ) { try { $reportModel = $this->reportFactory->create(); @@ -197,22 +196,32 @@ public function deleteById($reportId) } /** + * Does report exist already? + * * @param $report - * @return \Experius\Csp\Api\Data\ReportInterface|false|mixed + * @return ReportInterface|false */ - public function checkReportExist($report) + public function doesReportExistAlready($report) { - if ($report) { - $searchCriteria = $this->searchCriteriaBuilder - ->addFilter('violated_directive', $report->getViolatedDirective(), 'eq') - ->addFilter('blocked_uri', $report->getBlockedUri(), 'eq') - ->addFilter('document_uri', $report->getDocumentUri(), 'eq') - ->create(); - - $report = reset($this->getList($searchCriteria)->getItems()); + if (!$report || $report instanceof ReportInterface == false) { + return false; } - return $report; + $searchCriteria = $this->searchCriteriaBuilder + ->addFilter(ReportInterface::VIOLATED_DIRECTIVE, $report->getViolatedDirective()) + ->addFilter(ReportInterface::BLOCKED_URI, $report->getBlockedUri()) + ->addFilter(ReportInterface::DOCUMENT_URI, $report->getDocumentUri()) + ->create(); + + if ($this->getList($searchCriteria)->getTotalCount() < 1) { + return false; + } + + // Return first match if found + foreach ($this->getList($searchCriteria)->getItems() as $item) { + return $item; + } + return false; } public function createReportModel($report) @@ -220,7 +229,7 @@ public function createReportModel($report) $reportData = $this->extensibleDataObjectConverter->toNestedArray( $report, [], - \Experius\Csp\Api\Data\ReportInterface::class + ReportInterface::class ); return $this->reportFactory->create()->setData($reportData); From a3c6e13e2f7921e4dda817052563d0fe36f46df6 Mon Sep 17 00:00:00 2001 From: Boris van Katwijk Date: Thu, 12 Aug 2021 11:53:16 +0200 Subject: [PATCH 5/5] [REFACTOR][AOM2-172] Sleep for a random millisecond instead of microsecond to avoid the difference being smaller than the save time, which would result in duplicate CSP reports. --- Model/ReportRepository.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Model/ReportRepository.php b/Model/ReportRepository.php index b63a32b..908c9cf 100644 --- a/Model/ReportRepository.php +++ b/Model/ReportRepository.php @@ -104,8 +104,8 @@ public function save( // @TODO: [Nice to have] Find the reason csp reports are sent directly at the same time, to prevent having to // to use the usleep() mechanic to de-synchronise saving. - // Sleep for a random microsecond to prevent double saves - $sleep = rand(1, 1000000); + // Sleep for a random millisecond to prevent double saves + $sleep = rand(1000, 1000000); usleep($sleep); $existingReport = $this->doesReportExistAlready($report);