From 8f0f19dd44c0e5344fb100b8d883f38fa858a99e Mon Sep 17 00:00:00 2001 From: fmigneault Date: Thu, 8 Apr 2021 22:17:21 -0400 Subject: [PATCH] fix MAGPIE_PORT incorrectly retrieved (fix https://github.com/Ouranosinc/Magpie/issues/417) --- CHANGES.rst | 6 ++++- magpie/utils.py | 30 ++++++++++++------------ tests/test_utils.py | 56 +++++++++++++++++++++++++++++++++++++++++---- 3 files changed, 73 insertions(+), 19 deletions(-) diff --git a/CHANGES.rst b/CHANGES.rst index 040235a05..765620466 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -7,7 +7,11 @@ Changes `Unreleased `_ (latest) ------------------------------------------------------------------------------------ -* Nothing yet. +Bug Fixes +~~~~~~~~~~~~~~~~~~~~~ +* Fix default ``MAGPIE_PORT`` value not applied and validate other parsing resolution order for any environment + variable or settings that can interact with ``MAGPIE_URL`` definition + (resolves `#417 `_). `3.9.0 `_ (2021-04-06) ------------------------------------------------------------------------------------ diff --git a/magpie/utils.py b/magpie/utils.py index 8c1589ecc..c103fb06e 100644 --- a/magpie/utils.py +++ b/magpie/utils.py @@ -410,24 +410,26 @@ def get_magpie_url(container=None): :param container: container that provides access to application settings. :return: resolved Magpie URL """ - if container is None: - LOGGER.warning("Registry not specified, trying to find Magpie URL from environment") - url = get_constant("MAGPIE_URL", raise_missing=False, raise_not_set=False, print_missing=False) - if url: - return url - hostname = (get_constant("HOSTNAME", raise_not_set=False, raise_missing=False) or - get_constant("MAGPIE_HOST", raise_not_set=False, raise_missing=False)) - if not hostname: - raise ConfigurationError("Missing or unset MAGPIE_HOST or HOSTNAME value.") - magpie_port = get_constant("MAGPIE_PORT", raise_not_set=False) - magpie_scheme = get_constant("MAGPIE_SCHEME", raise_not_set=False, raise_missing=False, default_value="http") - return "{}://{}{}".format(magpie_scheme, hostname, ":{}".format(magpie_port) if magpie_port else "") try: + settings = get_settings(container, app=True) + magpie_url = get_constant("MAGPIE_URL", settings, raise_missing=False, raise_not_set=False, print_missing=False) + if not magpie_url: + hostname = (get_constant("MAGPIE_HOST", settings, raise_not_set=False, raise_missing=False, + print_missing=True) or + get_constant("HOSTNAME", settings, raise_not_set=False, raise_missing=False, + print_missing=True, default_value="localhost")) + if not hostname: + raise ConfigurationError("Missing, unset or empty value for MAGPIE_HOST or HOSTNAME setting.") + magpie_port = get_constant("MAGPIE_PORT", settings, print_missing=True, + raise_not_set=False, raise_missing=False, default_value=2001) + magpie_scheme = get_constant("MAGPIE_SCHEME", settings, print_missing=True, + raise_not_set=False, raise_missing=False, default_value="http") + magpie_url = "{}://{}{}".format(magpie_scheme, hostname, ":{}".format(magpie_port) if magpie_port else "") + # add "http" scheme to url if omitted from config since further 'requests' calls fail without it # mostly for testing when only "localhost" is specified # otherwise config should explicitly define it with 'MAGPIE_URL' env or 'magpie.url' config - settings = get_settings(container) - url_parsed = urlparse(get_constant("MAGPIE_URL", settings, "magpie.url").strip("/")) + url_parsed = urlparse(magpie_url.strip("/")) if url_parsed.scheme in ["http", "https"]: return url_parsed.geturl() magpie_url = "http://{}".format(url_parsed.geturl()) diff --git a/tests/test_utils.py b/tests/test_utils.py index b60335146..5519e83ac 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -5,9 +5,9 @@ test_utils ---------------------------------- -Tests for the various utility operations employed by magpie. +Tests for the various utility operations employed by Magpie. """ - +import os import unittest from distutils.version import LooseVersion @@ -16,11 +16,11 @@ from pyramid.httpexceptions import HTTPBadRequest, HTTPForbidden, HTTPInternalServerError, HTTPOk from pyramid.settings import asbool -from magpie import __meta__ +from magpie import __meta__, constants from magpie.api import exception as ax from magpie.api import generic as ag from magpie.api import requests as ar -from magpie.utils import CONTENT_TYPE_JSON, ExtendedEnum, get_header +from magpie.utils import CONTENT_TYPE_JSON, ExtendedEnum, get_header, get_magpie_url from tests import runner, utils @@ -321,3 +321,51 @@ def test_guess_target_format_default(self): content_type, where = ag.guess_target_format(request) utils.check_val_equal(content_type, CONTENT_TYPE_JSON) utils.check_val_equal(where, True) + + def test_get_magpie_url_defined_or_defaults(self): + # Disable constants globals() for every case, since it can pre-loaded from .env when running all tests. + # Always need to provide a settings container (even empty direct when nothing define in settings), + # otherwise 'get_constant' can find the current thread settings generated by any test app + with mock.patch.object(constants, "MAGPIE_URL", None): + + with mock.patch.dict(os.environ, {"MAGPIE_URL": ""}): + url = utils.check_no_raise(lambda: get_magpie_url({})) + utils.check_val_equal(url, "http://localhost:2001") + + url = utils.check_no_raise(lambda: get_magpie_url({"magpie.url": "https://test-server.com"})) + utils.check_val_equal(url, "https://test-server.com") + + url = utils.check_no_raise(lambda: get_magpie_url({"magpie.host": "localhost"})) + utils.check_val_equal(url, "http://localhost:2001") + + url = utils.check_no_raise(lambda: get_magpie_url({"magpie.host": "test-server.com"})) + utils.check_val_equal(url, "http://test-server.com:2001") + + url = utils.check_no_raise(lambda: get_magpie_url({"magpie.host": "test.com", "magpie.port": "1234"})) + utils.check_val_equal(url, "http://test.com:1234") + + url = utils.check_no_raise(lambda: get_magpie_url({"magpie.port": "1234"})) + utils.check_val_equal(url, "http://localhost:1234") + + url = utils.check_no_raise(lambda: get_magpie_url({"magpie.port": "9000", "magpie.scheme": "https"})) + utils.check_val_equal(url, "https://localhost:9000") + + with mock.patch.dict(os.environ, {"MAGPIE_URL": "localhost:9871"}): + url = utils.check_no_raise(lambda: get_magpie_url({"magpie.url": "https://test-server.com"})) + utils.check_val_equal(url, "https://test-server.com") # settings priority over envs + + url = utils.check_no_raise(lambda: get_magpie_url({})) + utils.check_val_equal(url, "http://localhost:9871") # env URL found if not in settings + + url = utils.check_no_raise(lambda: get_magpie_url({"magpie.host": "server"})) # ignored, URL priority + utils.check_val_equal(url, "http://localhost:9871") # URL fixed with missing scheme even if defined + + with mock.patch.dict(os.environ, {"MAGPIE_URL": "", "MAGPIE_PORT": "1234"}): + url = utils.check_no_raise(lambda: get_magpie_url({"magpie.url": "https://test-server.com"})) + utils.check_val_equal(url, "https://test-server.com") # ignore port, URL has priority + + url = utils.check_no_raise(lambda: get_magpie_url({"magpie.host": "server"})) + utils.check_val_equal(url, "http://server:1234") + + url = utils.check_no_raise(lambda: get_magpie_url({"magpie.scheme": "https"})) + utils.check_val_equal(url, "https://localhost:1234")