[statistics] Make query builder a bit more OOP

9 files changed, 175 insertions, 115 deletions

diff --git a/modules-available/statistics/api.inc.php b/modules-available/statistics/api.inc.php
index 431d4cd4..7aa6de34 100644
--- a/modules-available/statistics/api.inc.php
+++ b/modules-available/statistics/api.inc.php
@@ -1,26 +1,12 @@
 <?php
 
 if (Request::any('action') === 'test' && isLocalExecution()) {
-	$uuid = Request::any('uuid', '', 'string');
-	/*
-	error_log(HardwareInfo::getKclModifications());
-	exit;
-	$x = new HardwareQuery(HardwareInfo::PCI_DEVICE);
-	//$x->addCompare(false, 'Memory Slot Occupied', '>=', true, 'Memory Slot Count');
-	$x->addWhere(true, 'vendor', '=', '8086');
-	$x->addGlobalColumn('device');
-	$res = $x->query();
-	foreach ($res as $row) {
-		error_log(json_encode($row));
-	}
-	exit;
-	*/
-	$data = file_get_contents('/tmp/bla.json');
-	Database::exec(
-		"UPDATE machine SET data = :data WHERE machineuuid = :uuid",
-		['uuid' => $uuid, 'data' => $data]);
-	HardwareParser::parseMachine($uuid, json_decode($data, true));
-	echo 'Kweries: ' . Database::getQueryCount();
+	$q = new HardwareQuery(HardwareInfo::RAM_MODULE);
+	$speed = $q->addGlobalColumn('Speed');
+	$q->addLocalColumn('Configured Speed')->addCondition('<', $speed);
+	$speed->addCondition('>', 2666);
+	$q->addMachineWhere('clientip', '>', 5);
+	echo $q->buildQuery(), "\n";
 	exit;
 }
 
@@ -352,7 +338,7 @@ if ($type[0] === '~') {
 					$hwid = $hwids[$screen['name']];
 				} else {
 					$hwid = (int)Database::insertIgnore('statistic_hw', 'hwid',
-						array('hwtype' => DeviceType::SCREEN, 'hwname' => $screen['name']));
+						array('hwtype' => HardwareInfo::SCREEN, 'hwname' => $screen['name']));
 					$hwids[$screen['name']] = $hwid;
 				}
 				// Now add new entries
@@ -398,7 +384,7 @@ if ($type[0] === '~') {
 				Database::exec("UPDATE machine_x_hw x, statistic_hw h
 						SET x.disconnecttime = UNIX_TIMESTAMP()
 						WHERE x.machineuuid = :uuid AND x.hwid = h.hwid AND h.hwtype = :type AND x.disconnecttime = 0",
-					array('uuid' => $uuid, 'type' => DeviceType::SCREEN));
+					array('uuid' => $uuid, 'type' => HardwareInfo::SCREEN));
 			} else {
 				// Some screens connected, make sure old entries get removed
 				Database::exec("UPDATE machine_x_hw x, statistic_hw h
@@ -407,7 +393,7 @@ if ($type[0] === '~') {
 						AND x.machineuuid = :uuid", array(
 					'pairs' => $keepPair,
 					'uuid' => $uuid,
-					'type' => DeviceType::SCREEN,
+					'type' => HardwareInfo::SCREEN,
 				));
 			}
 		}
diff --git a/modules-available/statistics/hooks/config-tgz.inc.php b/modules-available/statistics/hooks/config-tgz.inc.php
index 0a43e115..b732fd7a 100644
--- a/modules-available/statistics/hooks/config-tgz.inc.php
+++ b/modules-available/statistics/hooks/config-tgz.inc.php
@@ -4,7 +4,7 @@ $res = Database::simpleQuery('SELECT h.hwname FROM statistic_hw h'
 	. " INNER JOIN statistic_hw_prop p ON (h.hwid = p.hwid AND p.prop = :projector)"
 	. " WHERE h.hwtype = :screen ORDER BY h.hwname ASC", array(
 	'projector' => 'projector',
-	'screen' => DeviceType::SCREEN,
+	'screen' => HardwareInfo::SCREEN,
 ));
 
 if ($res !== false) { // CHeck this in case we're running on old DB during update
diff --git a/modules-available/statistics/inc/devicetype.inc.php b/modules-available/statistics/inc/devicetype.inc.php
deleted file mode 100644
index a01ec310..00000000
--- a/modules-available/statistics/inc/devicetype.inc.php
+++ /dev/null
@@ -1,7 +0,0 @@
-<?php
-
-class DeviceType
-{
-	const SCREEN = 'SCREEN';
-
-}
diff --git a/modules-available/statistics/inc/hardwareinfo.inc.php b/modules-available/statistics/inc/hardwareinfo.inc.php
index df33a5f6..6ca070ee 100644
--- a/modules-available/statistics/inc/hardwareinfo.inc.php
+++ b/modules-available/statistics/inc/hardwareinfo.inc.php
@@ -12,6 +12,7 @@ class HardwareInfo
 	const PCI_DEVICE = 'PCI_DEVICE';
 	const HDD = 'HDD';
 	const CPU = 'CPU';
+	const SCREEN = 'SCREEN';
 
 	/**
 	 * Get a KCL modification string for the given machine, enabling GVT, PCI passthrough etc.
@@ -44,7 +45,7 @@ class HardwareInfo
 			return '';
 		$hw = new HardwareQuery(self::PCI_DEVICE, $best['machineuuid'], true);
 		// TODO: Get list of enabled pass through groups for this client's location
-		$hw->addJoin(true, '@PASSTHROUGH', 'passthrough_group_x_location', 'groupid',
+		$hw->addForeignJoin(true, '@PASSTHROUGH', 'passthrough_group_x_location', 'groupid',
 				'locationid', $locations);
 		$hw->addGlobalColumn('vendor');
 		$hw->addGlobalColumn('device');
@@ -77,7 +78,7 @@ class HardwareInfo
 			foreach (array_keys($slots) as $slot) {
 				//error_log('Querying slot ' . $slot);
 				$hw = new HardwareQuery(self::PCI_DEVICE, $best['machineuuid'], true);
-				$hw->addWhere(false, 'slot', 'LIKE', $slot . '.%');
+				$hw->addLocalColumn('slot')->addCondition('LIKE', $slot . '.%');
 				$hw->addGlobalColumn('vendor');
 				$hw->addGlobalColumn('device');
 				foreach ($hw->query() as $row) {
diff --git a/modules-available/statistics/inc/hardwarequery.inc.php b/modules-available/statistics/inc/hardwarequery.inc.php
index 24b27190..00a9bbc9 100644
--- a/modules-available/statistics/inc/hardwarequery.inc.php
+++ b/modules-available/statistics/inc/hardwarequery.inc.php
@@ -10,10 +10,10 @@ class HardwareQuery
 	private $columns = [];
 
 	/**
-	 * @param string $type type form HardwareInfo
-	 * @param ?string $uuid
+	 * @param string $type hardware type form HardwareInfo
+	 * @param ?string $uuid If set, only return data for specific client
 	 */
-	public function __construct(string $type, $uuid = null, $connectedOnly = true)
+	public function __construct(string $type, string $uuid = null, $connectedOnly = true)
 	{
 		if ($connectedOnly) {
 			$this->joins['mxhw_join'] = "INNER JOIN machine_x_hw mxhw ON (mxhw.hwid = shw.hwid AND mxhw.disconnecttime = 0)";
@@ -30,50 +30,33 @@ class HardwareQuery
 
 	private function id(): string
 	{
-		return 't' . (++$this->id);
-	}
-
-	private function fillTableVars(bool $global, &$srcTable, &$table, &$column)
-	{
-		if ($global) {
-			$srcTable = 'shw';
-			$table = 'statistic_hw_prop';
-			$column = 'hwid';
-		} else {
-			$srcTable = 'mxhw';
-			$table = 'machine_x_hw_prop';
-			$column = 'machinehwid';
-		}
+		return 'b' . (++$this->id);
 	}
 
 	/**
-	 * @param bool $global
-	 * @param string $prop
-	 * @param string $op
-	 * @param string|string[] $value
+	 * Add join of a virtual column (hw property) to an arbitrary table and column.
+	 * @param bool $global Is the virtual column global or local to machine?
+	 * @param string $prop Name of property/virtual column
+	 * @param string $jTable Table to join on
+	 * @param string $jColumn Column to join on
+	 * @param string $condColumn optionally, another column from the joined table to match against $condVal
+	 * @param string|array $condVal optionally, a literal, or array of literals, to match foreign column against
+	 * @return void
 	 */
-	public function addWhere(bool $global, string $prop, string $op, $value)
-	{
-		if (isset($this->columns[$prop]))
-			return;
-		$this->fillTableVars($global, $srcTable, $table, $column);
-		$tid = $this->id();
-		$pid = $this->id();
-		$vid = $this->id();
-		$valueCol = ($op === '<' || $op === '>' || $op === '<=' || $op === '>=') ? 'numeric' : 'value';
-		$this->joins[$prop] = "INNER JOIN $table $tid ON ($srcTable.$column = $tid.$column AND
-			$tid.prop = :$pid AND $tid.`$valueCol` $op (:$vid))";
-		$this->args[$pid] = $prop;
-		$this->args[$vid] = $value;
-		$this->columns[$prop] = "$tid.`value` AS `$prop`";
-	}
-
-	public function addJoin(bool $global, string $prop, string $jTable, string $jColumn, string $condColumn = '', $condVal = null)
+	public function addForeignJoin(bool $global, string $prop, string $jTable, string $jColumn, string $condColumn = '', $condVal = null)
 	{
 		if (isset($this->columns[$jTable]))
 			return;
 		if (!isset($this->columns[$prop])) {
-			$this->fillTableVars($global, $srcTable, $table, $column);
+			if ($global) {
+				$srcTable = 'shw';
+				$table = 'statistic_hw_prop';
+				$column = 'hwid';
+			} else {
+				$srcTable = 'mxhw';
+				$table = 'machine_x_hw_prop';
+				$column = 'machinehwid';
+			}
 			$tid = $this->id();
 			$pid = $this->id();
 			$this->joins[$prop] = "INNER JOIN $table $tid ON ($srcTable.$column = $tid.$column
@@ -99,55 +82,33 @@ class HardwareQuery
 	{
 		if (isset($this->columns[$column]))
 			return;
-		$valueCol = ($op === '<' || $op === '>' || $op === '<=' || $op === '>=') ? 'numeric' : 'value';
 		$vid = $this->id();
 		$this->joins['machine'] = 'INNER JOIN machine m USING (machineuuid)';
-		$this->where[] = "m.$column $op :$vid";
+		$this->where[] = "m.$column $op (:$vid)";
 		$this->args[$vid] = $value;
 		$this->columns[$column] = "m.$column";
 	}
 
-	public function addCompare(bool $global1, string $prop1, string $op, string $global2, string $prop2)
+	public function addGlobalColumn(string $prop): HardwareQueryColumn
 	{
-		$this->fillTableVars($global1, $srcTable1, $table1, $column1);
-		$this->fillTableVars($global2, $srcTable2, $table2, $column2);
-		$tid1 = $this->id();
-		$pid1 = $this->id();
-		$tid2 = $this->id();
-		$pid2 = $this->id();
-		$valueCol = ($op === '<' || $op === '>' || $op === '<=' || $op === '>=') ? 'numeric' : 'value';
-		$this->joins[] = "INNER JOIN $table1 $tid1 ON ($srcTable1.$column1 = $tid1.$column1 AND
-			$tid1.prop = :$pid1)";
-		$this->joins[] = "INNER JOIN $table2 $tid2 ON ($srcTable2.$column2 = $tid2.$column2 AND
-			$tid2.prop = :$pid2 AND $tid1.`$valueCol` $op $tid2.`$valueCol`)";
-		$this->args[$pid1] = $prop1;
-		$this->args[$pid2] = $prop2;
-		$this->columns[$prop1] = "$tid1.`value` AS `$prop1`";
-		$this->columns[$prop2] = "$tid2.`value` AS `$prop2`";
+		return $this->addColumn(true, $prop);
 	}
 
-	public function addGlobalColumn(string $prop)
+	public function addLocalColumn(string $prop): HardwareQueryColumn
 	{
-		$this->addColumn(true, $prop);
+		return $this->addColumn(false, $prop);
 	}
 
-	public function addLocalColumn(string $prop)
+	public function addColumn(bool $global, string $prop, string $alias = null): HardwareQueryColumn
 	{
-		$this->addColumn(false, $prop);
-	}
-
-	public function addColumn(bool $global, string $prop)
-	{
-		if (isset($this->columns[$prop]))
-			return;
-		$this->fillTableVars($global, $srcTable, $table, $column);
-		$tid = $this->id();
-		$pid = $this->id();
-		$this->joins[$prop] = "LEFT JOIN $table $tid ON ($srcTable.$column = $tid.$column AND $tid.prop = :$pid)";
-		$this->args[$pid] = $prop;
-		$this->columns[$prop] = "$tid.`value` AS `$prop`";
+		return $this->columns[] = new HardwareQueryColumn($global, $prop, $alias);
 	}
 
+	/**
+	 * Join the machine table and add the given column from it to the SELECT
+	 * @param string $column
+	 * @return void
+	 */
 	public function addMachineColumn(string $column)
 	{
 		if (isset($this->columns[$column]))
@@ -161,9 +122,27 @@ class HardwareQuery
 	 */
 	public function query(string $groupBy = '')
 	{
-		$columns = $this->columns;
+		return Database::simpleQuery($this->buildQuery($groupBy), $this->args);
+	}
+
+	/**
+	 * Build query string
+	 * @param string $groupBy Column to group by
+	 */
+	public function buildQuery(string $groupBy = ''): string
+	{
+		$groupConcat = !empty($groupBy) && $groupBy !== 'mxhw.machinehwid';
+		$columns = [];
+		foreach ($this->columns as $column) {
+			if ($column instanceof HardwareQueryColumn) {
+				$column->generate($this->joins, $columns, $this->args, $groupConcat);
+			} else {
+				$columns[] = $column;
+			}
+		}
 		$columns[] = 'mxhw.machineuuid';
 		$columns[] = 'shw.hwid';
+		// TODO: Untangle this implicit magic
 		if (empty($groupBy) || $groupBy === 'mxhw.machinehwid') {
 			$columns[] = 'mxhw.disconnecttime';
 		} else {
@@ -173,12 +152,11 @@ class HardwareQuery
 			$columns[] = 'Count(*) AS group_count';
 			$groupBy = " GROUP BY $groupBy";
 		}
-		$query = 'SELECT ' . implode(', ', $columns)
+		return 'SELECT ' . implode(', ', $columns)
 			. ' FROM statistic_hw shw '
 			. implode(' ', $this->joins)
 			. ' WHERE ' . implode(' AND ', $this->where)
 			. $groupBy;
-		return Database::simpleQuery($query, $this->args);
 	}
 
 }
\ No newline at end of file
diff --git a/modules-available/statistics/inc/hardwarequerycolumn.inc.php b/modules-available/statistics/inc/hardwarequerycolumn.inc.php
new file mode 100644
index 00000000..d073530b
--- /dev/null
+++ b/modules-available/statistics/inc/hardwarequerycolumn.inc.php
@@ -0,0 +1,88 @@
+<?php
+
+class HardwareQueryColumn
+{
+	/** @var int For unique table names in join */
+	private static $id = 0;
+
+	private $global;
+	private $tableAlias;
+	private $virtualColumnName;
+	private $alias;
+	private $conditions = [];
+	private $params = [];
+	private $classId;
+
+	private static function getId(): string
+	{
+		return 't' . ++self::$id;
+	}
+
+	public function __construct(bool $global, string $column, string $alias = null)
+	{
+		$this->classId = ++self::$id;
+		$this->global = $global;
+		$this->tableAlias = self::getId();
+		$this->virtualColumnName = $column;
+		$this->alias = $alias ?? $column;
+	}
+
+	/**
+	 * Add necessary conditions, joins, columns to final SQL arrays. To be called
+	 * from HardwareQuery::buildQuery().
+	 * @param bool $groupConcat whether to add distinct GROUP_CONCAT to column.
+	 */
+	public function generate(array &$joins, array &$columns, array &$params, bool $groupConcat)
+	{
+		if ($this->global) {
+			$srcTable = 'shw';
+			$table = 'statistic_hw_prop';
+			$column = 'hwid';
+		} else {
+			$srcTable = 'mxhw';
+			$table = 'machine_x_hw_prop';
+			$column = 'machinehwid';
+		}
+		$tid = $this->tableAlias;
+		$pid = self::getId();
+		$this->conditions[] = "$srcTable.$column = $tid.$column AND $tid.prop = :$pid";
+		$params[$pid] = $this->virtualColumnName; // value of property column is our virtual column
+		// If we have just one condition, it's the join condition itself. Since we pretend we're just adding
+		// a column to the query, do a left join, so the "column" is NULL if the join doesn't match.
+		// If however any conditions were added to this class via the addCondition() method, do a regular
+		// INNER JOIN, so the result will be empty if the condition doesn't match.
+		$type = count($this->conditions) === 1 ? 'LEFT' : 'INNER';
+		$joins[] = "$type JOIN $table $tid ON (" . implode(' AND ', $this->conditions) . ")";
+		if ($groupConcat) {
+			$columns[] = "Group_Concat(DISTINCT $tid.`value` SEPARATOR ', ') AS `{$this->alias}`";
+		} else {
+			$columns[] = "$tid.`value` AS `{$this->alias}`";
+		}
+		$params += $this->params;
+	}
+
+	/**
+	 * @param string $op Operator (<>=, IN, LIKE)
+	 * @param string|string[]|HardwareQueryColumn $other value to compare with.
+	 * 	Can be a literal, an array (if opererator is IN), or another Column
+	 * @return void
+	 */
+	public function addCondition(string $op, $other)
+	{
+		$valueCol = ($op === '<' || $op === '>' || $op === '<=' || $op === '>=') ? 'numeric' : 'value';
+		if ($other instanceof HardwareQueryColumn) {
+			$cond = "{$this->tableAlias}.`$valueCol` $op {$other->tableAlias}.`$valueCol`";
+			// Don't reference a column of a table that hasn't been joined yet
+			if ($this->classId > $other->classId) {
+				$this->conditions[] = $cond;
+			} else {
+				$other->conditions[] = $cond;
+			}
+		} else {
+			$pid = self::getId();
+			$this->conditions[] = "{$this->tableAlias}.`$valueCol` $op (:$pid)";
+			$this->params[$pid] = $other;
+		}
+	}
+
+}
\ No newline at end of file
diff --git a/modules-available/statistics/pages/hints.inc.php b/modules-available/statistics/pages/hints.inc.php
index 278c0e26..67c42a18 100644
--- a/modules-available/statistics/pages/hints.inc.php
+++ b/modules-available/statistics/pages/hints.inc.php
@@ -19,6 +19,10 @@ class SubPage
 		self::showUnusedSpace($locs);
 	}
 
+	/**
+	 * Machines that have less than 8GB of RAM. Highlight those
+	 * that still have free memory slots.
+	 */
 	private static function showMemoryUpgrade(array $locs)
 	{
 		$q = new HardwareQuery(HardwareInfo::MAINBOARD);
@@ -30,7 +34,8 @@ class SubPage
 		$q->addGlobalColumn('Memory Maximum Capacity');
 		$q->addMachineColumn('clientip');
 		$q->addMachineColumn('hostname');
-		$q->addWhere(false, 'Memory Installed Capacity', '<', 8 * 1024 * 1024 * 1024);
+		$col = $q->addLocalColumn('Memory Installed Capacity');
+		$col->addCondition('<', 8 * 1024 * 1024 * 1024);
 		$list = [];
 		foreach ($q->query() as $row) {
 			if (HardwareParser::convertSize($row['Memory Installed Capacity'], 'M', false)
@@ -47,6 +52,10 @@ class SubPage
 		Render::addTemplate('hints-ram-upgrade', ['list' => $list]);
 	}
 
+	/**
+	 * Show machines where RAM modules are running slower
+	 * than their design speed.
+	 */
 	private static function showMemorySlow(array $locs)
 	{
 		$q = new HardwareQuery(HardwareInfo::RAM_MODULE);
@@ -62,11 +71,16 @@ class SubPage
 		$q->addLocalColumn('Serial Number');
 		$q->addMachineColumn('clientip');
 		$q->addMachineColumn('hostname');
-		$q->addCompare(true, 'Speed', '>', false, 'Configured Memory Speed');
+		$col = $q->addGlobalColumn('Speed');
+		$col->addCondition('>', $q->addLocalColumn('Configured Memory Speed'));
 		$list = $q->query()->fetchAll();
 		Render::addTemplate('hints-ram-underclocked', ['list' => $list]);
 	}
 
+	/**
+	 * Show machines that have unpartitioned space available,
+	 * and no ID44 or ID45.
+	 */
 	private static function showUnusedSpace(array $locs)
 	{
 		$id44 = $id45 = [];
@@ -77,7 +91,7 @@ class SubPage
 		}
 		$q->addMachineColumn('clientip');
 		$q->addMachineColumn('hostname');
-		$q->addWhere(false, 'unused', '>', 2000000000); // 2 GB
+		$q->addLocalColumn('unused')->addCondition('>', 2000000000); // 2 GB
 		$q->addMachineWhere('id44mb', '<', 20000); // 20 GB
 		foreach ($q->query()->fetchAll() as $row) {
 			$row['unused_s'] = Util::readableFileSize($row['unused']);
@@ -91,7 +105,7 @@ class SubPage
 		}
 		$q->addMachineColumn('clientip');
 		$q->addMachineColumn('hostname');
-		$q->addWhere(false, 'unused', '>', 25000000000); // 25 GB
+		$q->addLocalColumn('unused')->addCondition('>', 25000000000); // 25 GB
 		$q->addMachineWhere('id44mb', '>', 20000); // 20 GB
 		$q->addMachineWhere('id45mb', '<', 20000); // 20 GB
 		foreach ($q->query()->fetchAll() as $row) {
diff --git a/modules-available/statistics/pages/machine.inc.php b/modules-available/statistics/pages/machine.inc.php
index 019b0d8d..6ea0ce28 100644
--- a/modules-available/statistics/pages/machine.inc.php
+++ b/modules-available/statistics/pages/machine.inc.php
@@ -229,7 +229,7 @@ class SubPage
 			. " LEFT JOIN machine_x_hw_prop p ON (m.machinehwid = p.machinehwid AND p.prop = 'resolution')"
 			. " LEFT JOIN statistic_hw_prop q ON (m.hwid = q.hwid AND q.prop = 'projector')"
 			. " WHERE m.machineuuid = :uuid",
-			array('screen' => DeviceType::SCREEN, 'uuid' => $uuid));
+			array('screen' => HardwareInfo::SCREEN, 'uuid' => $uuid));
 		$client['screens'] = array();
 		$ports = array();
 		foreach ($res as $row) {
diff --git a/modules-available/statistics/pages/projectors.inc.php b/modules-available/statistics/pages/projectors.inc.php
index 97a21ebd..91abfe9c 100644
--- a/modules-available/statistics/pages/projectors.inc.php
+++ b/modules-available/statistics/pages/projectors.inc.php
@@ -49,7 +49,7 @@ class SubPage
 			. " INNER JOIN statistic_hw_prop p ON (h.hwid = p.hwid AND p.prop = :projector)"
 			. " WHERE h.hwtype = :screen ORDER BY h.hwname ASC", array(
 			'projector' => 'projector',
-			'screen' => DeviceType::SCREEN,
+			'screen' => HardwareInfo::SCREEN,
 		));
 		$data = array(
 			'projectors' => $res->fetchAll()