fix MAGPIE_PORT incorrectly retrieved (fix #417)

CHANGES.rst

@@ -7,7 +7,11 @@ Changes
 `Unreleased <https://github.com/Ouranosinc/Magpie/tree/master>`_ (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 <https://github.com/Ouranosinc/Magpie/issues/417>`_).
 
 `3.9.0 <https://github.com/Ouranosinc/Magpie/tree/3.9.0>`_ (2021-04-06)
 ------------------------------------------------------------------------------------

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())

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")