[locationinfo] CourseBackends: Lots of bug fixes, missing error checks, improvements:

- Add more checks for returned data structures from backend, like keys in arrays
- Better error messages if something goes wrong, not just "server sent wrong xml"
- Make checkConnection() of davinci and hisinone not require a valid room id, which
  we don't have in general
- hisinone: Parse data structure just once for every room
- Request coalescing: Only try so if getRefreshTime() > getCacheTime()
- Move toArray() to base class instead of having two copies
- Sanitize variable naming conventions

1 files changed, 88 insertions, 72 deletions

diff --git a/modules-available/locationinfo/inc/coursebackend.inc.php b/modules-available/locationinfo/inc/coursebackend.inc.php
index 11d833e6..7dc50549 100644
--- a/modules-available/locationinfo/inc/coursebackend.inc.php
+++ b/modules-available/locationinfo/inc/coursebackend.inc.php
@@ -12,17 +12,24 @@ abstract class CourseBackend
 
 	/**
 	 * @var array list of known backends
-	 * @var boolean  true if there was an error
-	 * @var string with the error message
-	 * @var int as internal serverID
-	 * @var string url of the service
 	 */
 	private static $backendTypes = false;
-	public $error;
-	public $errormsg;
-	public $serverID;
-	public $location;
-	const nrOtherRooms = 5;
+	/**
+	 * @var boolean|string false = no error, error message otherwise
+	 */
+	protected $error;
+	/**
+	 * @var int as internal serverId
+	 */
+	protected $serverId;
+	/**
+	 * @var string url of the service
+	 */
+	protected $location;
+	/**
+	 * @const int max number of additional locations to fetch (for backends that benefit from request coalesc.)
+	 */
+	const MAX_ADDIDIONAL_LOCATIONS = 5;
 
 	/**
 	 * CourseBackend constructor.
@@ -31,7 +38,6 @@ abstract class CourseBackend
 	{
 		$this->location = "";
 		$this->error = false;
-		$this->errormsg = "";
 	}
 
 	/**
@@ -68,7 +74,7 @@ abstract class CourseBackend
 	 * Get fresh instance of ConfigModule subclass for given module type.
 	 *
 	 * @param string $moduleType name of module type
-	 * @return \ConfigModule module instance
+	 * @return \CourseBackend module instance
 	 */
 	public static function getInstance($moduleType)
 	{
@@ -106,10 +112,10 @@ abstract class CourseBackend
 	 *
 	 * @param array $data with the credentials
 	 * @param string $url address of the server
-	 * @param int $serverID ID of the server
+	 * @param int $serverId ID of the server
 	 * @returns bool if the credentials were in the correct format
 	 */
-	public abstract function setCredentials($data, $url, $serverID);
+	public abstract function setCredentials($data, $url, $serverId);
 
 	/**
 	 * @return int desired caching time of results, in seconds. 0 = no caching
@@ -125,7 +131,7 @@ abstract class CourseBackend
 	/**
 	 * Internal version of fetch, to be overridden by subclasses.
 	 *
-	 * @param $roomIds array with local ID as key and serverID as value
+	 * @param $roomIds array with local ID as key and serverId as value
 	 * @return array a recursive array that uses the roomID as key
 	 * and has the schedule array as value. A shedule array contains an array in this format:
 	 * ["start"=>'JJJJ-MM-DD HH:MM:SS',"end"=>'JJJJ-MM-DD HH:MM:SS',"title"=>string]
@@ -138,80 +144,75 @@ abstract class CourseBackend
 	 * @param array $roomId array of room ID to fetch
 	 * @return array|bool array containing the timetables as value and roomid as key as result, or false on error
 	 */
-	public final function fetchSchedule($roomIDs)
+	public final function fetchSchedule($requestedLocationIds)
 	{
-		if (empty($roomIDs)) {
-			$this->error = true;
-			$this->errormsg = 'No roomid was given to fetch Shedule';
+		if (!is_array($requestedLocationIds)) {
+			$this->error = 'No array of roomids was given to fetchSchedule';
 			return false;
 		}
-		$sqlr = implode(",", $roomIDs);
-		$sqlr = '(' . $sqlr . ')';
-		$q = "SELECT locationid, calendar, serverroomid, lastcalendarupdate FROM location_info WHERE locationid IN " . $sqlr;
-		$dbquery1 = Database::simpleQuery($q);
-		$result = [];
-		$sRoomIDs = [];
-		$newResult = [];
-		foreach ($dbquery1->fetchAll(PDO::FETCH_ASSOC) as $row) {
-			$sRoomID = $row['serverroomid'];
-			$lastUpdate = $row['lastcalendarupdate'];
-			$calendar = $row['calendar'];
+		if (empty($requestedLocationIds))
+			return array();
+		$NOW = time();
+		$dbquery1 = Database::simpleQuery("SELECT locationid, calendar, serverroomid, lastcalendarupdate
+				FROM location_info WHERE locationid IN (:locations)",
+				array('locations' => array_values($requestedLocationIds)));
+		$returnValue = [];
+		$remoteIds = [];
+		while ($row = $dbquery1->fetch(PDO::FETCH_ASSOC)) {
 			//Check if in cache if lastUpdate is null then it is interpreted as 1970
-			if ($lastUpdate > strtotime("-" . $this->getCacheTime() . "seconds")) {
-				$result[$row['locationid']] = json_decode($calendar);
+			if ($row['lastcalendarupdate'] + $this->getCacheTime() > $NOW) {
+				$returnValue[$row['locationid']] = json_decode($row['calendar']);
 			} else {
-				$sRoomIDs[$row['locationid']] = $sRoomID;
+				$remoteIds[$row['locationid']] = $row['serverroomid'];
 			}
 
 		}
-		//Check if we should refresh other rooms recently requested by front ends
-		if ($this->getCacheTime() > 0) {
-			$i = 0; //number of rooms getting refreshed
-			$dbquery4 = Database::simpleQuery("SELECT locationid ,serverroomid, lastcalendarupdate FROM location_info WHERE serverid= :id", array('id' => $this->serverID));
-			foreach ($dbquery4->fetchAll(PDO::FETCH_COLUMN) as $row) {
-				if (isset($row['lastcalendarupdate'])) {
-					$lastUpdate = $row['lastcalendarupdate'];
-					if ($lastUpdate < strtotime("-" . $this->getRefreshTime() . "seconds")
-							&& $lastUpdate > strtotime("-" . $this->getCacheTime() . "seconds"
-							&& $i < self::nrOtherRooms)) {
-						$sRoomIDs[$row['locationid']] = $row['serverroomid'];
-						$i = $i + 1;
-					}
-				}
-			}
+		// No need for additional round trips to backend
+		if (empty($remoteIds)) {
+			return $returnValue;
 		}
-		//This is true if there is no need to check the HisInOne Server
-		if (empty($sRoomIDs)) {
-			return $result;
+		// Check if we should refresh other rooms recently requested by front ends
+		if ($this->getRefreshTime() > $this->getCacheTime()) {
+			$dbquery4 = Database::simpleQuery("SELECT locationid, serverroomid FROM location_info
+					WHERE serverid = :serverid AND serverroomid NOT IN (:skiplist)
+					AND lastcalendarupdate BETWEEN :lowerage AND :upperage
+					LIMIT " . self::MAX_ADDIDIONAL_LOCATIONS, array(
+						'serverid' => $this->serverId,
+						'skiplist' => array_values($remoteIds),
+						'lowerage' => $NOW - $this->getRefreshTime(),
+						'upperage' => $NOW - $this->getCacheTime(),
+			));
+			while ($row = $dbquery4->fetch(PDO::FETCH_ASSOC)) {
+				$remoteIds[$row['locationid']] = $row['serverroomid'];
+			}
 		}
-		$results = $this->fetchSchedulesInternal($sRoomIDs);
-		if ($results === false) {
+		$backendResponse = $this->fetchSchedulesInternal($remoteIds);
+		if ($backendResponse === false) {
 			return false;
 		}
 
-		foreach ($sRoomIDs as $location => $serverRoom) {
-			$newResult[$location] = $results[$serverRoom];
-		}
-
 		if ($this->getCacheTime() > 0) {
-			foreach ($newResult as $key => $value) {
-				$value = json_encode($value);
-				$now = strtotime('Now');
+			// Caching requested by backend, write to DB
+			foreach ($backendResponse as $serverRoomId => $calendar) {
+				$value = json_encode($calendar);
 				Database::simpleQuery("UPDATE location_info SET calendar = :ttable, lastcalendarupdate = :now
-					WHERE locationid = :id ", array(
-					'id' => $key,
+					WHERE serverid = :serverid AND serverroomid = :serverroomid", array(
+					'serverid' => $this->serverId,
+					'serverroomid' => $serverRoomId,
 					'ttable' => $value,
-					'now' => $now
+					'now' => $NOW
 				));
 			}
 		}
-		//get all schedules that are wanted from roomIDs
-		foreach ($roomIDs as $id) {
-			if (isset($newResult[$id])) {
-				$result[$id] = $newResult[$id];
+		// Add rooms that were requested to the final return value
+		foreach ($remoteIds as $location => $serverRoomId) {
+			if (isset($backendResponse[$serverRoomId]) && in_array($location, $requestedLocationIds)) {
+				// Only add if we can map it back to our location id AND it was not an unsolicited coalesced refresh
+				$returnValue[$location] = $backendResponse[$serverRoomId];
 			}
 		}
-		return $result;
+
+		return $returnValue;
 	}
 
 	/**
@@ -219,10 +220,7 @@ abstract class CourseBackend
 	 */
 	public final function getError()
 	{
-		if ($this->error) {
-			return $this->errormsg;
-		}
-		return false;
+		return $this->error;
 	}
 
 	/**
@@ -232,7 +230,7 @@ abstract class CourseBackend
 	 * and leaf nodes. The result is always an array on success, or
 	 * false if not found.
 	 */
-	function getAttributes($array, $path)
+	protected function getAttributes($array, $path)
 	{
 		if (!is_array($path)) {
 			// Convert 'path/syntax/foo/wanteditem' to array for further processing and recursive calls
@@ -280,4 +278,22 @@ abstract class CourseBackend
 		// Unique non-leaf node - simple recursion
 		return $this->getAttributes($array[$element], $path);
 	}
+
+	/**
+	 * @param string $response xml document to convert
+	 * @return bool|array array representation of the xml if possible, false otherwise
+	 */
+	protected function toArray($response)
+	{
+		$cleanresponse = preg_replace('/(<\/?)(\w+):([^>]*>)/', '$1$2$3', $response);
+		try {
+			$xml = new SimpleXMLElement($cleanresponse);
+		} catch (Exception $e) {
+			$this->error = 'Could not parse reply as XML, got ' . get_class($e) . ': ' . $e->getMessage();
+			return false;
+		}
+		$array = json_decode(json_encode((array)$xml), true);
+		return $array;
+	}
+
 }