From 76b9243ca45219545437a089ce50fe47ca6c49e9 Mon Sep 17 00:00:00 2001 From: Matthew Hilton Date: Fri, 8 Sep 2023 11:38:17 +1000 Subject: [PATCH 1/4] [#46] Add connection timeout configuration option --- classes/esrequest.php | 5 ++++- lang/en/search_elastic.php | 2 ++ settings.php | 3 +++ version.php | 2 +- 4 files changed, 10 insertions(+), 2 deletions(-) diff --git a/classes/esrequest.php b/classes/esrequest.php index 66078c3..28b6138 100644 --- a/classes/esrequest.php +++ b/classes/esrequest.php @@ -62,8 +62,11 @@ public function __construct($handler = false) { $this->config = get_config('search_elastic'); $this->signing = (isset($this->config->signing) ? (bool)$this->config->signing : false); + $config = [ + 'connect_timeout' => intval($this->config->connecttimeout) + ]; + // Allow the caller to instantiate the Guzzle client with a custom handler. - $config = []; if ($handler) { $config['handler'] = $handler; } diff --git a/lang/en/search_elastic.php b/lang/en/search_elastic.php index 27e3208..c91a435 100644 --- a/lang/en/search_elastic.php +++ b/lang/en/search_elastic.php @@ -44,6 +44,8 @@
For more information, follow this link: {$a}'; $string['complexhelpurl'] = 'https://lucene.apache.org/core/2_9_4/queryparsersyntax.html'; +$string['connecttimeout'] = 'Connect timeout (seconds)'; +$string['connecttimeout_help'] = 'Guzzle connection timeout. Use 0 to disable. See https://docs.guzzlephp.org/en/stable/request-options.html#connect-timeout'; $string['enrichdesc'] = 'Global Search can enrich the indexed data used in search by extracting text and other data from files. The data extracted from files in Moodle is controlled by the following groups of settings.'; $string['enrichsettings'] = 'Data enrichment settings'; diff --git a/settings.php b/settings.php index d4f6af1..46ab3c0 100644 --- a/settings.php +++ b/settings.php @@ -53,6 +53,9 @@ $settings->add(new admin_setting_configtext('search_elastic/apikey', get_string ('apikey', 'search_elastic'), get_string ('apikey_help', 'search_elastic'), '')); + $settings->add(new admin_setting_configtext('search_elastic/connecttimeout', get_string('connecttimeout', 'search_elastic'), + get_string('connecttimeout_help', 'search_elastic'), 5, PARAM_INT)); + $settings->add(new admin_setting_heading('signingsettings', get_string('signingsettings', 'search_elastic'), '')); $settings->add(new admin_setting_configcheckbox('search_elastic/signing', get_string('signing', 'search_elastic'), get_string ('signing_help', 'search_elastic'), 0)); diff --git a/version.php b/version.php index 9bd8ab0..22d0026 100644 --- a/version.php +++ b/version.php @@ -24,7 +24,7 @@ defined('MOODLE_INTERNAL') || die(); -$plugin->version = 2023012400; +$plugin->version = 2023090700; $plugin->release = '4.1 Build (2023012400)'; // Build same as version. $plugin->requires = 2016052304; $plugin->component = 'search_elastic'; From 68524e830205acac147f7f362df580e9bba4f1a4 Mon Sep 17 00:00:00 2001 From: Matthew Hilton Date: Fri, 8 Sep 2023 11:39:12 +1000 Subject: [PATCH 2/4] [#107] Engine status gives better error messages --- classes/engine.php | 48 ++++++++++++++++++++----------- lang/en/search_elastic.php | 2 ++ tests/engine_test.php | 58 +++++++++++++++++++------------------- 3 files changed, 63 insertions(+), 45 deletions(-) diff --git a/classes/engine.php b/classes/engine.php index e564fd8..27dd73b 100644 --- a/classes/engine.php +++ b/classes/engine.php @@ -108,7 +108,7 @@ private function log($msg) { * * @return url|bool Returns url if succes or false on error. */ - private function get_url() { + public function get_url() { $returnval = false; if (!empty($this->config->hostname) && !empty($this->config->port)) { @@ -255,26 +255,42 @@ private function create_index() { * Is the Elasticsearch server endpoint configured in Moodle * and available. * - * @param object $stack The Guzzle client stack to use. - * @return true|string Returns true if all good or an error string. + * @param object|false $stack Optional custom Guzzle HTTP stack. + * @return string|bool true if server is ready, else error string. */ - public function is_server_ready($stack=false) { - $url = $this->get_url(); - $returnval = true; - $client = new \search_elastic\esrequest($stack); - - if (!$url) { - $returnval = get_string('noconfig', 'search_elastic'); - } else { - $response = $client->get($url); - $responsecode = $response->getStatusCode(); + public function is_server_ready($stack = false) { + // Not configured yet. + if (empty($this->get_url())) { + return get_string('connection:na', 'search_elastic'); } - if (isset($responsecode) && $responsecode != 200) { - $returnval = get_string('noserver', 'search_elastic'); + // Test a connection to the configured server. + $status = $this->get_server_status_code($stack); + + if ($status !== 200) { + return get_string('connection:status', 'search_elastic', [ + 'url' => $this->get_url(), + 'status' => $status + ]); } - return $returnval; + return true; + } + + /** + * Tests connection to the server and returns the HTTP status code. + * + * @param object|false $stack Optional custom Guzzle HTTP stack. + * @return int HTTP response code. + */ + public function get_server_status_code($stack = false): int { + $url = $this->get_url(); + $client = new \search_elastic\esrequest($stack); + + $response = $client->get($url); + $responsecode = $response->getStatusCode(); + + return $responsecode; } /** diff --git a/lang/en/search_elastic.php b/lang/en/search_elastic.php index c91a435..a9091a7 100644 --- a/lang/en/search_elastic.php +++ b/lang/en/search_elastic.php @@ -44,6 +44,8 @@
For more information, follow this link: {$a}'; $string['complexhelpurl'] = 'https://lucene.apache.org/core/2_9_4/queryparsersyntax.html'; +$string['connection:na'] = 'Server not configured'; +$string['connection:status'] = 'The configured elastic server at {$a->url} returned the status code {$a->status}'; $string['connecttimeout'] = 'Connect timeout (seconds)'; $string['connecttimeout_help'] = 'Guzzle connection timeout. Use 0 to disable. See https://docs.guzzlephp.org/en/stable/request-options.html#connect-timeout'; $string['enrichdesc'] = 'Global Search can enrich the indexed data used in search by extracting text and other data from files. diff --git a/tests/engine_test.php b/tests/engine_test.php index 405f7f5..1da2220 100644 --- a/tests/engine_test.php +++ b/tests/engine_test.php @@ -126,50 +126,50 @@ public function file_indexing_provider() { } /** - * Test check if Elasticsearch server is ready. + * Provides values to test_is_server_ready + * + * @return array */ - public function test_is_server_ready() { - // Create a mock stack and queue a response. - $container = []; - $mock = new MockHandler([ - new Response(200, ['Content-Type' => 'application/json']) - ]); - - $stack = HandlerStack::create($mock); - - // Reflection magic as we are directly testing a private method. - $method = new \ReflectionMethod('\search_elastic\engine', 'is_server_ready'); - $method->setAccessible(true); // Allow accessing of private method. - $proxy = $method->invoke(new \search_elastic\engine, $stack); - - // Check the results. - $this->assertEquals(true, $proxy); + public function is_server_ready_provider(): array { + return [ + '200' => [ + 'code' => 200, + 'ok' => true + ], + '404' => [ + 'code' => 404, + 'ok' => false + ] + ]; } /** * Test check if Elasticsearch server is ready. + * + * @param int $code Simulated HTTP status code + * @param bool $expectedok If the server should be expected to be ready/ok. + * @dataProvider is_server_ready_provider */ - public function test_is_server_ready_false() { + public function test_is_server_ready(int $code, bool $expectedok) { // Create a mock stack and queue a response. - $container = []; $mock = new MockHandler([ - new Response(404, ['Content-Type' => 'application/json']) + new Response($code, ['Content-Type' => 'application/json']) ]); $stack = HandlerStack::create($mock); - // Reflection magic as we are directly testing a private method. - $method = new \ReflectionMethod('\search_elastic\engine', 'is_server_ready'); - $method->setAccessible(true); // Allow accessing of private method. - $proxy = $method->invoke(new \search_elastic\engine, $stack); - - $expected = 'Elasticsearch endpoint unreachable'; + $engine = new \search_elastic\engine(); + $status = $engine->is_server_ready($stack); - // Check the results. - $this->assertEquals($expected, $proxy); + if ($expectedok) { + $this->assertTrue($status); + } else { + // Status should contain a string with the 404 code in it. + $this->assertTrue(is_string($status)); + $this->assertTrue(strpos($status, '404') != false); + } } - /** * Test deleting docs by type id. */ From b5c2ac10e53702d6fc9ef705940ffb8f0a21dd0f Mon Sep 17 00:00:00 2001 From: Matthew Hilton Date: Fri, 8 Sep 2023 11:39:44 +1000 Subject: [PATCH 3/4] [#107] Switch healthcheck to use check API --- classes/admin_setting_check.php | 83 +++++++++++++++++++++++++ classes/check/server_ready_check.php | 82 ++++++++++++++++++++++++ lang/en/search_elastic.php | 2 + lib.php | 8 +++ settings.php | 20 +++--- tests/server_ready_check_test.php | 93 ++++++++++++++++++++++++++++ 6 files changed, 279 insertions(+), 9 deletions(-) create mode 100644 classes/admin_setting_check.php create mode 100644 classes/check/server_ready_check.php create mode 100644 tests/server_ready_check_test.php diff --git a/classes/admin_setting_check.php b/classes/admin_setting_check.php new file mode 100644 index 0000000..33de049 --- /dev/null +++ b/classes/admin_setting_check.php @@ -0,0 +1,83 @@ +. + +namespace search_elastic; + +use admin_setting; +use core\check\check; + +/** + * Admin setting for check api. + * + * @package search_elastic + * @copyright Matthew Hilton + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class admin_setting_check extends admin_setting { + /** @var check $check The check to display */ + private $check; + + /** + * Creates check setting. + * + * @param string $name name of setting + * @param string $heading title of setting + * @param check $check check to display + */ + public function __construct(string $name, string $heading, check $check) { + $this->nosave = true; + $this->check = $check; + parent::__construct($name, $heading, '', ''); + } + + /** + * Returns setting (unused) + * + * @return true + */ + public function get_setting() { + return true; + } + + /** + * Writes the setting (unused) + * + * @param mixed $data + */ + public function write_setting($data) { + return ''; + } + + /** + * Ouputs the admin setting HTML to be rendered. + * + * @param mixed $data + * @param string $query + * @return string html + */ + public function output_html($data, $query = '') { + global $OUTPUT; + + // Run the check and get the result. + $checkresult = $this->check->get_result(); + + $resulthtml = $OUTPUT->check_result($checkresult); + $resultinfo = $checkresult->get_summary(); + + $out = $resulthtml . ' ' . $resultinfo; + return format_admin_setting($this, $this->visiblename, '', $out); + } +} diff --git a/classes/check/server_ready_check.php b/classes/check/server_ready_check.php new file mode 100644 index 0000000..e97bdfe --- /dev/null +++ b/classes/check/server_ready_check.php @@ -0,0 +1,82 @@ +. + +namespace search_elastic\check; + +use action_link; +use core\check\check; +use core\check\result; +use moodle_url; +use search_elastic\engine; + +/** + * Elasticsearch plugin server ready check. + * + * @package search_elastic + * @copyright Matthew Hilton + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class server_ready_check extends check { + /** @var object|false Custom Guzzle stack to use for requests. Used only for unit testing. */ + private $stack; + + /** + * Create check. + * @param object|false $stack Custom HTTP stack for unit testing. + */ + public function __construct($stack = false) { + $this->stack = $stack; + } + + /** + * Performs check and returns result. + * + * @return result + */ + public function get_result(): result { + // Check the plugin is configured. + if (empty(get_config('search_elastic', 'hostname'))) { + return new result(result::NA, get_string('connection:na', 'search_elastic')); + } + + // Query the server to see the HTTP response. + $engine = new engine(); + $url = $engine->get_url(); + $status = $engine->get_server_status_code($this->stack); + $resultstatus = $status === 200 ? result::OK : result::ERROR; + + // Format the check details nicely. + $statusdetails = get_string('connection:status', 'search_elastic', [ + 'url' => $url, + 'status' => $status + ]); + + return new result($resultstatus, $statusdetails); + } + + /** + * Returns the link to action this check if it failed. + * + * @return action_link A link to the elasticsearch engine plugin settings + */ + public function get_action_link(): action_link { + $configstr = get_string('adminsettings', 'search_elastic'); + $configurl = new moodle_url('/admin/settings.php', ['section' => 'elasticsettings']); + + return new action_link($configurl, $configstr); + } +} + diff --git a/lang/en/search_elastic.php b/lang/en/search_elastic.php index a9091a7..f5f0ac3 100644 --- a/lang/en/search_elastic.php +++ b/lang/en/search_elastic.php @@ -36,6 +36,8 @@ $string['boostsettings'] = 'Boosting settings'; $string['boostvalue'] = ''; $string['boostvalue_help'] = 'Set the value you want this search area to be boosted by in the search results. Higher boost values give more priority.'; +$string['checkserver_ready_check'] = 'Elastic server connection'; +$string['connectiontest'] = 'Server connection test'; $string['complexhelptext'] = 'The field to be searched may be specified by prefixing the search query with \'title:\', \'content:\', \'name:\', or \'intro:\'. For example, searching for \'title:news\' would return results with the word \'news\' in the title.
Boolean operators (\'AND\', \'OR\') may be used to combine or exclude keywords. diff --git a/lib.php b/lib.php index aa21d95..3042675 100644 --- a/lib.php +++ b/lib.php @@ -55,3 +55,11 @@ function search_elastic_extend_navigation_user() { redirect(new moodle_url('/search/engine/elastic/index.php')); } } + +/** + * Returns the status checks for this plugin. + * @return array + */ +function search_elastic_status_checks(): array { + return [new \search_elastic\check\server_ready_check()]; +} diff --git a/settings.php b/settings.php index 46ab3c0..87a6dc7 100644 --- a/settings.php +++ b/settings.php @@ -22,22 +22,24 @@ * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ +use search_elastic\admin_setting_check; +use search_elastic\check\server_ready_check; + defined('MOODLE_INTERNAL') || die(); if ($hassiteconfig) { $ADMIN->add('searchplugins', new admin_category('search_elastic', get_string('pluginname', 'search_elastic'))); $settings = new admin_settingpage('elasticsettings', get_string('adminsettings', 'search_elastic')); - - // Check status of the server. - $engine = new \search_elastic\engine(); - $status = $engine->is_server_ready(); - if ($status === true) { - $statustext = $OUTPUT->notification(get_string('reachable', 'search_elastic'), \core\output\notification::NOTIFY_SUCCESS); - } else { - $statustext = $OUTPUT->notification($status); + $settings->add(new admin_setting_heading('basicsettings', get_string('basicsettings', 'search_elastic'), '')); + + // Only check status when when in full tree (i.e. not search) + // and only when viewing the elastic settings - this is because it can take a long time to run. + // We don't want it to run unnecessarily. + if ($ADMIN->fulltree && optional_param('section', null, PARAM_TEXT) == 'elasticsettings') { + $settings->add(new admin_setting_check('search_elastic/connectiontest', get_string('connectiontest', 'search_elastic'), + new server_ready_check())); } - $settings->add(new admin_setting_heading('basicsettings', get_string('basicsettings', 'search_elastic'), $statustext)); $settings->add(new admin_setting_configtext('search_elastic/hostname', get_string ('hostname', 'search_elastic'), get_string ('hostname_help', 'search_elastic'), 'http://127.0.0.1', PARAM_URL)); diff --git a/tests/server_ready_check_test.php b/tests/server_ready_check_test.php new file mode 100644 index 0000000..5c217b2 --- /dev/null +++ b/tests/server_ready_check_test.php @@ -0,0 +1,93 @@ +. + + +namespace search_elastic; + +use advanced_testcase; +use core\check\result; +use GuzzleHttp\Handler\MockHandler; +use GuzzleHttp\HandlerStack; +use GuzzleHttp\Psr7\Response; +use search_elastic\check\server_ready_check; + +/** + * Elasticsearch plugin server ready check tests + * + * @package search_elastic + * @copyright Matthew Hilton + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + * @covers \search_elastic\check\server_ready_check + */ +class server_ready_check_test extends advanced_testcase { + + /** @var string Valid hostname for testing */ + private const VALID_HOSTNAME = 'valid.com'; + + /** @var string Invalid hostname for testing */ + private const INVALID_HOSTNAME = 'invalid.com'; + + /** @var string Empty hostname for testing */ + private const EMPTY_HOSTNAME = ''; + + /** + * Provides server ready test configurations. + * @return array + */ + public function server_ready_provider(): array { + return [ + 'not set' => [ + 'hostname' => self::EMPTY_HOSTNAME, + 'status' => result::NA, + ], + 'invalid hostname' => [ + 'hostname' => self::INVALID_HOSTNAME, + 'status' => result::ERROR + ], + 'valid hostname' => [ + 'hostname' => self::VALID_HOSTNAME, + 'status' => result::OK + ] + ]; + } + + /** + * Tests check result. + * + * @param string $hostname + * @param string $expectedstatus + * @dataProvider server_ready_provider + */ + public function test_check(string $hostname, string $expectedstatus) { + $this->resetAfterTest(); + set_config('hostname', $hostname, 'search_elastic'); + + $testhostnamestatus = [ + self::VALID_HOSTNAME => 200, + self::INVALID_HOSTNAME => 404, + self::EMPTY_HOSTNAME => 400 + ]; + + $mock = new MockHandler([ + new Response($testhostnamestatus[$hostname], ['Content-Type' => 'application/json']) + ]); + $stack = HandlerStack::create($mock); + + $check = new server_ready_check($stack); + $result = $check->get_result(); + $this->assertEquals($expectedstatus, $result->get_status()); + } +} From c23d1598b37422a3435b71c0bc6b60cc7219f16b Mon Sep 17 00:00:00 2001 From: Matthew Hilton Date: Thu, 14 Sep 2023 08:12:22 +1000 Subject: [PATCH 4/4] [#107] Add notification formatting to check statuses --- classes/admin_setting_check.php | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/classes/admin_setting_check.php b/classes/admin_setting_check.php index 33de049..25ab873 100644 --- a/classes/admin_setting_check.php +++ b/classes/admin_setting_check.php @@ -18,6 +18,8 @@ use admin_setting; use core\check\check; +use core\check\result; +use core\output\notification; /** * Admin setting for check api. @@ -76,8 +78,18 @@ public function output_html($data, $query = '') { $resulthtml = $OUTPUT->check_result($checkresult); $resultinfo = $checkresult->get_summary(); - $out = $resulthtml . ' ' . $resultinfo; + + switch($checkresult->get_status()){ + case result::CRITICAL: + case result::ERROR: + $out = $OUTPUT->notification($out, notification::NOTIFY_ERROR, false); + break; + + case result::OK: + $out = $OUTPUT->notification($out, notification::NOTIFY_SUCCESS, false); + break; + } return format_admin_setting($this, $this->visiblename, '', $out); } }