Repository: schuderer/mllaunchpad

Issue:
Misleading error message "mllaunchpad.wsgi: Config file could not be loaded"
This error also occurs if loading a previous model fails during the the ModelApi initialization. The try-catch block in the wsgi module mistakenly assumes that FileNotFoundError can only occur when loading the config, but it can also occur on initializing ModelApi (if the model could not be loaded from the model store). See:

https://github.com/schuderer/mllaunchpad/blob/cc378e1f1935d5211addf385075323a68253e746/mllaunchpad/wsgi.py#L33

Should probably be handled separately.

Discussion:
schuderer: Thanks for the PR! 👌 I’m on the fence on whether for the ModelApi case, we **really** only want to log the error instead of actually raising it. If this problem occurs when running the API, we actually want the program to halt with an exception, which would mean raising an error instead of logging something and continuing. On the other hand, this will probably cause problems when building the docs (using nox). Could you change the second logger.error to a raise, and try what happens when you run nox -s docs?
schuderer: Nevermind, it would just fail because conf has not been assigned. 

I’d probably just put a ‘conf = None’ assignment in the top except branch and replace the second try-except with just an ‘if conf:’ (no else or except branch necessary). 

Thoughts?
bobplatte: Makes sense, the less try-except statements the better I would say. 
I will update the code accordingly and create a new draft.
schuderer: Nonono, just push a commit with the change to this one
schuderer: Thanks! I took the liberty to do two small things:
- Add a regression unit test.
- Move the CHANGELOG entry to the "Unreleased" section (your contribution will be part of an upcoming release, not the existing v0.0.7 release (too late for that, it's already out :o) ).

Patch:
diff --git a/AUTHORS.rst b/AUTHORS.rst
index b107270..a030315 100644
--- a/AUTHORS.rst
+++ b/AUTHORS.rst
@@ -13,6 +13,7 @@ Contributors
 * Elisa Partodikromo <https://github.com/planeetjupyter>
 * Gosia Rorat <https://github.com/gosiarorat>
 * Bart Driessen <https://github.com/Bart92>
+* Bob Platte <https://github.com/bobplatte>
 
 Apache License 2.0
 
diff --git a/CHANGELOG.rst b/CHANGELOG.rst
index 51d7a14..c8ccdda 100644
--- a/CHANGELOG.rst
+++ b/CHANGELOG.rst
@@ -25,6 +25,10 @@ and this project adheres to `Semantic Versioning <https://semver.org/spec/v2.0.0
 Unreleased
 ------------------------------------------------------------------------------
 
+* |Fixed| Fix misleading error message at WSGI entry point if model could
+  not be loaded,
+  `issue #61 <https://github.com/schuderer/mllaunchpad/issues/61>`_,
+  by `Bob Platte <https://github.com/bobplatte>`_.
 * |Enhancement| Config file is now being checked for omitted required keys,
   (no issue), by `Andreas Schuderer <https://github.com/schuderer>`_.
 * |Fixed| Use correct reference to werkzeug's FileStorage,
diff --git a/mllaunchpad/wsgi.py b/mllaunchpad/wsgi.py
index f0247ab..e1284a3 100644
--- a/mllaunchpad/wsgi.py
+++ b/mllaunchpad/wsgi.py
@@ -25,24 +25,27 @@ logger = logging.getLogger(__name__)
 # necessary to wrap the preparatory code in a try:except: statement.
 try:
     conf = config.get_validated_config()
-
-    # if you change the name of the application variable, you need to
-    # specify it explicitly for gunicorn: gunicorn ... launchpad.wsgi:appname
-    application = Flask(__name__, root_path=conf["api"].get("root_path"))
-
-    ModelApi(conf, application)
 except FileNotFoundError:
     logger.error(
         "Config file could not be loaded. Starting the Flask application "
         "will fail."
     )
+    conf = None
 
-if __name__ == "__main__":
-    logger.warning(
-        "Starting Flask debug server.\nIn production, please use a WSGI server, "
-        + "e.g. 'gunicorn -w 4 -b 127.0.0.1:5000 mllaunchpad.wsgi:application'"
-    )
-    application.run(debug=True)
+    # if you change the name of the application variable, you need to
+    # specify it explicitly for gunicorn: gunicorn ... launchpad.wsgi:appname
+if conf:
+    application = Flask(__name__, root_path=conf["api"].get("root_path"))
+    ModelApi(conf, application)
+
+    if __name__ == "__main__":
+        logger.warning(
+            "Starting Flask debug server.\nIn production, please use a WSGI server, "
+            + "e.g. 'gunicorn -w 4 -b 127.0.0.1:5000 mllaunchpad.wsgi:application'"
+        )
+        # Flask apps must not be run in debug mode in production, because this allows for arbitrary code execution.
+        # We know that and advise the user that this is only for debugging, so this is not a security issue (marked nosec):
+        application.run(debug=True)  # nosec
 
 # To start an instance of production server with 4 workers:
 #  1. Set environment variables if required


Test patch:
diff --git a/tests/test_api.py b/tests/test_api.py
index db17a5c..53c3cb9 100644
--- a/tests/test_api.py
+++ b/tests/test_api.py
@@ -1,7 +1,7 @@
 #!/usr/bin/env python
 # -*- coding: utf-8 -*-
 
-"""Tests for `mllaunchpad.config` module."""
+"""Tests for `mllaunchpad.api` module."""
 
 # Stdlib imports
 from tempfile import NamedTemporaryFile
diff --git a/tests/test_wsgi.py b/tests/test_wsgi.py
new file mode 100644
index 0000000..6d9f4f3
--- /dev/null
+++ b/tests/test_wsgi.py
@@ -0,0 +1,52 @@
+#!/usr/bin/env python
+# -*- coding: utf-8 -*-
+
+"""Tests for `mllaunchpad.wsgi` module."""
+
+# Stdlib imports
+from importlib import reload
+from unittest.mock import patch
+
+# Third-party imports
+import pytest
+
+
+@patch("mllaunchpad.config.get_validated_config")
+def test_log_error_on_config_filenotfound(mock_get_cfg, caplog):
+    """Test that a FileNotFoundError on loading the config does
+    not cause an exception, but only log a 'not found, will fail' error.
+    """
+    mock_get_cfg.side_effect = FileNotFoundError
+
+    # This import is just to make sure the 'wsgi' symbol is known.
+    # We will ignore what happens and afterwards reload (re-import).
+    try:
+        import mllaunchpad.wsgi as wsgi  # pylint: disable=unused-import
+    except FileNotFoundError:
+        pass
+    # The proper "import" test:
+    reload(wsgi)
+    assert "will fail".lower() in caplog.text.lower()
+
+
+@patch("mllaunchpad.api.ModelApi")
+@patch("mllaunchpad.config.get_validated_config")
+def test_regression_61_misleading(mock_get_cfg, mock_get_api, caplog):
+    """Test that a FileNotFoundError on loading the Model does
+    cause a proper exception and not merely log a 'config not found' error.
+    https://github.com/schuderer/mllaunchpad/issues/61
+    """
+    mock_get_cfg.return_value = {"api": {}}
+    mock_get_api.side_effect = FileNotFoundError
+
+    # This import is just to make sure the 'wsgi' symbol is known.
+    # We will ignore what happens and afterwards reload (re-import).
+    try:
+        import mllaunchpad.wsgi as wsgi  # pylint: disable=unused-import
+    except FileNotFoundError:
+        pass
+    # The proper "import" test:
+    with pytest.raises(FileNotFoundError):
+        reload(wsgi)
+        assert "will fail".lower() not in caplog.text.lower()
+    assert mock_get_cfg.called