From 44418c209428335e611bfb9384578fb18b88978d Mon Sep 17 00:00:00 2001 From: Simon Rettberg Date: Wed, 19 Apr 2017 15:51:47 +0200 Subject: [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 --- .../locationinfo/inc/coursebackend.inc.php | 160 +++++++++++---------- 1 file changed, 88 insertions(+), 72 deletions(-) (limited to 'modules-available/locationinfo/inc/coursebackend.inc.php') 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; + } + } -- cgit v1.2.3-55-g7522