From 6d6d505ca3d1d1c2523e635108a5dc92d51267e5 Mon Sep 17 00:00:00 2001 From: Dan Garner Date: Wed, 2 Sep 2020 17:45:53 +0100 Subject: [PATCH] Removed null parameters from TimeSeriesStore setDependencies() - the interface should be concrete. Add missing displayGroupFactory parameter. Delayed instantiation of MongoDB client to allow for "setClient()" replacement if needed. This might also speed up XMDS because it won't create a Mongo client with each instantiation. Removed redundant error messages (which output the exact same content) --- lib/Factory/ContainerFactory.php | 14 ++--- lib/Storage/MongoDbTimeSeriesStore.php | 79 ++++++++++++++---------- lib/Storage/MySqlTimeSeriesStore.php | 6 +- lib/Storage/TimeSeriesStoreInterface.php | 15 ++++- 4 files changed, 66 insertions(+), 48 deletions(-) diff --git a/lib/Factory/ContainerFactory.php b/lib/Factory/ContainerFactory.php index 7d29a4fcbe..b711177621 100644 --- a/lib/Factory/ContainerFactory.php +++ b/lib/Factory/ContainerFactory.php @@ -122,16 +122,14 @@ public static function create() }, 'timeSeriesStore' => function(ContainerInterface $c) { if ($c->get('configService')->timeSeriesStore == null) { - return (new MySqlTimeSeriesStore()) - ->setDependencies($c->get('logService'), - $c->get('layoutFactory'), - $c->get('campaignFactory')) - ->setStore($c->get('store')); + $timeSeriesStore = new MySqlTimeSeriesStore(); } else { $timeSeriesStore = $c->get('configService')->timeSeriesStore; $timeSeriesStore = $timeSeriesStore(); + } - return $timeSeriesStore->setDependencies( + return $timeSeriesStore + ->setDependencies( $c->get('logService'), $c->get('layoutFactory'), $c->get('campaignFactory'), @@ -139,8 +137,8 @@ public static function create() $c->get('widgetFactory'), $c->get('displayFactory'), $c->get('displayGroupFactory') - ); - } + ) + ->setStore($c->get('store')); }, 'state' => function() { return new ApplicationState(); diff --git a/lib/Storage/MongoDbTimeSeriesStore.php b/lib/Storage/MongoDbTimeSeriesStore.php index 58f252f520..c9666473d3 100644 --- a/lib/Storage/MongoDbTimeSeriesStore.php +++ b/lib/Storage/MongoDbTimeSeriesStore.php @@ -88,40 +88,60 @@ class MongoDbTimeSeriesStore implements TimeSeriesStoreInterface */ public function __construct($config = null) { - $this->config = $config; } /** * @inheritdoc */ - public function setDependencies($log, $layoutFactory = null, $campaignFactory = null, $mediaFactory = null, $widgetFactory = null, $displayFactory = null, $displayGroupFactory = null) + public function setDependencies($log, $layoutFactory, $campaignFactory, $mediaFactory, $widgetFactory, $displayFactory, $displayGroupFactory) { $this->log = $log; + $this->layoutFactory = $layoutFactory; + $this->campaignFactory = $campaignFactory; $this->mediaFactory = $mediaFactory; $this->widgetFactory = $widgetFactory; - $this->layoutFactory = $layoutFactory; $this->displayFactory = $displayFactory; $this->displayGroupFactory = $displayGroupFactory; - $this->campaignFactory = $campaignFactory; + return $this; + } - try { - $uri = isset($this->config['uri']) ? $this->config['uri'] : 'mongodb://' . $this->config['host'] . ':' . $this->config['port']; - $this->client = new Client($uri, [ - 'username' => $this->config['username'], - 'password' => $this->config['password'] - ], (array_key_exists('driverOptions', $this->config) ? $this->config['driverOptions'] : [])); - } catch (\MongoDB\Exception\RuntimeException $e) { - $this->log->critical($e->getMessage()); + /** + * Set Client in the event you want to completely replace the configuration options and roll your own client. + * @param \MongoDB\Client $client + */ + public function setClient($client) + { + $this->client = $client; + } + + /** + * Get a MongoDB client to use. + * @return \MongoDB\Client + * @throws \Xibo\Support\Exception\GeneralException + */ + private function getClient() + { + if ($this->client === null) { + try { + $uri = isset($this->config['uri']) ? $this->config['uri'] : 'mongodb://' . $this->config['host'] . ':' . $this->config['port']; + $this->client = new Client($uri, [ + 'username' => $this->config['username'], + 'password' => $this->config['password'] + ], (array_key_exists('driverOptions', $this->config) ? $this->config['driverOptions'] : [])); + } catch (\MongoDB\Exception\RuntimeException $e) { + $this->log->error('Unable to connect to MongoDB: ' . $e->getMessage()); + $this->log->debug($e->getTraceAsString()); + throw new GeneralException('Connection to Time Series Database failed, please try again.'); + } } - return $this; + return $this->client; } /** @inheritdoc */ public function addStat($statData) { - // We need to transform string date to UTC date $statData['statDate'] = new UTCDateTime($statData['statDate']->format('U') * 1000); @@ -358,12 +378,14 @@ public function addStat($statData) } - /** @inheritdoc */ + /** @inheritdoc + * @throws \Xibo\Support\Exception\GeneralException + */ public function addStatFinalize() { // Insert statistics if (count($this->stats) > 0) { - $collection = $this->client->selectCollection($this->config['database'], $this->table); + $collection = $this->getClient()->selectCollection($this->config['database'], $this->table); try { $collection->insertMany($this->stats); @@ -375,7 +397,7 @@ public function addStatFinalize() } // Create a period collection if it doesnot exist - $collectionPeriod = $this->client->selectCollection($this->config['database'], $this->periodTable); + $collectionPeriod = $this->getClient()->selectCollection($this->config['database'], $this->periodTable); try { $cursor = $collectionPeriod->findOne(['name' => 'period']); @@ -397,7 +419,7 @@ public function addStatFinalize() /** @inheritdoc */ public function getEarliestDate() { - $collection = $this->client->selectCollection($this->config['database'], $this->table); + $collection = $this->getClient()->selectCollection($this->config['database'], $this->table); try { $earliestDate = $collection->aggregate([ [ @@ -521,7 +543,7 @@ public function getStats($filterBy = []) $campaignIds[] = $this->layoutFactory->getCampaignIdFromLayoutHistory($layoutId); } catch (NotFoundException $notFoundException) { // Ignore the missing one - $this->getLog()->debug('Filter for Layout without Layout History Record, layoutId is ' . $layoutId); + $this->log->debug('Filter for Layout without Layout History Record, layoutId is ' . $layoutId); } } $match['$match']['campaignId'] = [ '$in' => $campaignIds ]; @@ -532,7 +554,7 @@ public function getStats($filterBy = []) $match['$match']['mediaId'] = [ '$in' => $mediaIds ]; } - $collection = $this->client->selectCollection($this->config['database'], $this->table); + $collection = $this->getClient()->selectCollection($this->config['database'], $this->table); $group = [ '$group' => [ @@ -559,9 +581,6 @@ public function getStats($filterBy = []) $totalCount = $totalCursor->toArray(); $total = (count($totalCount) > 0) ? $totalCount[0]['count'] : 0; - } catch (\MongoDB\Exception\RuntimeException $e) { - $this->log->error('Error: Total Count. '. $e->getMessage()); - throw new GeneralException(__('Sorry we encountered an error getting Proof of Play data, please consult your administrator')); } catch (\Exception $e) { $this->log->error('Error: Total Count. '. $e->getMessage()); throw new GeneralException(__('Sorry we encountered an error getting Proof of Play data, please consult your administrator')); @@ -619,9 +638,6 @@ public function getStats($filterBy = []) // Total $result->totalCount = $total; - } catch (\MongoDB\Exception\RuntimeException $e) { - $this->log->error('Error: Get total. '. $e->getMessage()); - throw new GeneralException(__('Sorry we encountered an error getting Proof of Play data, please consult your administrator')); } catch (\Exception $e) { $this->log->error('Error: Get total. '. $e->getMessage()); throw new GeneralException(__('Sorry we encountered an error getting Proof of Play data, please consult your administrator')); @@ -655,7 +671,7 @@ public function getExportStatsCount($filterBy = []) $match['$match']['displayId'] = [ '$in' => $displayIds ]; } - $collection = $this->client->selectCollection($this->config['database'], $this->table); + $collection = $this->getClient()->selectCollection($this->config['database'], $this->table); // Get total try { @@ -673,9 +689,6 @@ public function getExportStatsCount($filterBy = []) $totalCount = $totalCursor->toArray(); $total = (count($totalCount) > 0) ? $totalCount[0]['count'] : 0; - } catch (\MongoDB\Exception\RuntimeException $e) { - $this->log->error($e->getMessage()); - throw new GeneralException(__('Sorry we encountered an error getting total number of Proof of Play data, please consult your administrator')); } catch (\Exception $e) { $this->log->error($e->getMessage()); throw new GeneralException(__('Sorry we encountered an error getting total number of Proof of Play data, please consult your administrator')); @@ -699,7 +712,7 @@ public function deleteStats($maxage, $fromDt = null, $options = []) $toDt = new UTCDateTime($maxage->format('U')*1000); - $collection = $this->client->selectCollection($this->config['database'], $this->table); + $collection = $this->getClient()->selectCollection($this->config['database'], $this->table); $rows = 1; $count = 0; @@ -764,7 +777,7 @@ public function executeQuery($options = []) $aggregateConfig['maxTimeMS']= $options['maxTimeMS']; } - $collection = $this->client->selectCollection($this->config['database'], $options['collection']); + $collection = $this->getClient()->selectCollection($this->config['database'], $options['collection']); try { $cursor = $collection->aggregate($options['query'], $aggregateConfig); @@ -775,10 +788,10 @@ public function executeQuery($options = []) } catch (\MongoDB\Driver\Exception\RuntimeException $e) { $this->log->error($e->getMessage()); + $this->log->debug($e->getTraceAsString()); throw new GeneralException($e->getMessage()); } return $results; - } } \ No newline at end of file diff --git a/lib/Storage/MySqlTimeSeriesStore.php b/lib/Storage/MySqlTimeSeriesStore.php index 93b827fbc9..720c51aea2 100644 --- a/lib/Storage/MySqlTimeSeriesStore.php +++ b/lib/Storage/MySqlTimeSeriesStore.php @@ -62,7 +62,7 @@ public function __construct($config = null) /** * @inheritdoc */ - public function setDependencies($log, $layoutFactory = null, $campaignFactory = null, $mediaFactory = null, $widgetFactory = null, $displayFactory = null) + public function setDependencies($log, $layoutFactory, $campaignFactory, $mediaFactory, $widgetFactory, $displayFactory, $displayGroupFactory) { $this->log = $log; $this->layoutFactory = $layoutFactory; @@ -424,9 +424,7 @@ public function getExportStatsCount($filterBy = []) $resTotal = $this->store->select($sql, $params); // Total - $totalCount = isset($resTotal[0]['total']) ? $resTotal[0]['total'] : 0; - - return $totalCount; + return isset($resTotal[0]['total']) ? $resTotal[0]['total'] : 0; } /** @inheritdoc */ diff --git a/lib/Storage/TimeSeriesStoreInterface.php b/lib/Storage/TimeSeriesStoreInterface.php index 79e171b92e..997ca1303b 100644 --- a/lib/Storage/TimeSeriesStoreInterface.php +++ b/lib/Storage/TimeSeriesStoreInterface.php @@ -46,13 +46,22 @@ public function __construct($config = null); /** * Set Time series Dependencies * @param LogServiceInterface $logger + * @param LayoutFactory $layoutFactory + * @param CampaignFactory $campaignFactory * @param MediaFactory $mediaFactory * @param WidgetFactory $widgetFactory - * @param LayoutFactory $layoutFactory * @param DisplayFactory $displayFactory - * @param CampaignFactory $campaignFactory + * @param \Xibo\Entity\DisplayGroup $displayGroupFactory */ - public function setDependencies($logger, $layoutFactory = null, $campaignFactory = null, $mediaFactory = null, $widgetFactory = null, $displayFactory = null); + public function setDependencies( + $logger, + $layoutFactory, + $campaignFactory, + $mediaFactory, + $widgetFactory, + $displayFactory, + $displayGroupFactory + ); /** * Process and add a single statdata to array