Refactor reading config properties from a file

lib/galaxy/exceptions/__init__.py

@@ -203,9 +203,13 @@ class NotImplemented(MessageException):
     err_code = error_codes.NOT_IMPLEMENTED
 
 
-# non-web exceptions
+class InvalidFileFormatError(MessageException):
+    status_code = 500
+    err_code = error_codes.INVALID_FILE_FORMAT
 
 
+# non-web exceptions
+
 class ContainerCLIError(Exception):
     def __init__(self, msg=None, stdout=None, stderr=None, returncode=None,
                  command=None, subprocess_command=None, **kwargs):

lib/galaxy/exceptions/error_codes.json

@@ -149,6 +149,11 @@
     "code": 500004,
     "message": "Tool execution failed due to an internal server error."
    },
+   {
+    "name": "INVALID_FILE_FORMAT",
+    "code": 500005,
+    "message": "File format not supported for this operation."
+   },
    {
     "name": "NOT_IMPLEMENTED",
     "code": 501001,

lib/galaxy/util/properties.py

@@ -12,6 +12,7 @@ import yaml
 from six import iteritems, string_types
 from six.moves.configparser import ConfigParser
 
+from galaxy.exceptions import InvalidFileFormatError
 from galaxy.util.path import extensions, has_ext, joinext
 
 
@@ -63,38 +64,24 @@ def find_config_file(names, exts=None, dirs=None, include_samples=False):
 
 
 def load_app_properties(
-    kwds={},
+    kwds=None,
     ini_file=None,
     ini_section=None,
     config_file=None,
     config_section=None,
     config_prefix="GALAXY_CONFIG_"
 ):
-    properties = kwds.copy() if kwds else {}
-    properties['__file__'] = None
     if config_file is None:
         config_file = ini_file
         config_section = ini_section
 
+    # read from file or init w/no file
     if config_file:
-        properties['__file__'] = os.path.abspath(config_file)
-        if not has_ext(config_file, 'yaml', aliases=True, ignore='sample'):
-            if config_section is None:
-                config_section = "app:main"
-            parser = nice_config_parser(config_file)
-            if parser.has_section(config_section):
-                properties.update(dict(parser.items(config_section)))
-            else:
-                properties.update(parser.defaults())
-        else:
-            if config_section is None:
-                config_section = "galaxy"
-
-            with open(config_file, "r") as f:
-                raw_properties = yaml.safe_load(f)
-            properties = __default_properties(config_file)
-            properties.update(raw_properties.get(config_section) or {})
+        properties = read_properties_from_file(config_file, config_section)
+    else:
+        properties = {'__file__': None}
 
+    # update from env
     override_prefix = "%sOVERRIDE_" % config_prefix
     for key in os.environ:
         if key.startswith(override_prefix):
@@ -105,9 +92,40 @@ def load_app_properties(
             if config_key not in properties:
                 properties[config_key] = os.environ[key]
 
+    # update from kwds
+    if kwds:
+        properties.update(kwds)
+
     return properties
 
 
+def read_properties_from_file(config_file, config_section=None):
+    properties = {}
+    if has_ext(config_file, 'yaml', aliases=True, ignore='sample'):
+        if config_section is None:
+            config_section = "galaxy"
+        properties.update(__default_properties(config_file))
+        raw_properties = _read_from_yaml_file(config_file)
+        if raw_properties:
+            properties.update(raw_properties.get(config_section) or {})
+    elif has_ext(config_file, 'ini', aliases=True, ignore='sample'):
+        if config_section is None:
+            config_section = "app:main"
+        parser = nice_config_parser(config_file)  # default properties loaded w/parser
+        if parser.has_section(config_section):
+            properties.update(dict(parser.items(config_section)))
+        else:
+            properties.update(parser.defaults())
+    else:
+        raise InvalidFileFormatError()
+    return properties
+
+
+def _read_from_yaml_file(path):
+    with open(path, "r") as f:
+        return yaml.safe_load(f)
+
+
 def nice_config_parser(path):
     parser = NicerConfigParser(path, defaults=__default_properties(path))
     parser.optionxform = str  # Don't lower-case keys

test/unit/util/test_properties.py

@@ -0,0 +1,115 @@
+import pytest
+
+from galaxy.exceptions import InvalidFileFormatError
+from galaxy.util import properties
+from galaxy.util.properties import read_properties_from_file
+
+KEY1, KEY2, KEY3, KEY4, KEY5, KEY6 = 'k1', 'k2', 'k3', 'k4', 'k5', 'k6'
+VAL1, VAL2, VAL3, VAL4, VAL5, VAL6 = 1, 2, 3, 4, 5, 6
+OTHER_SECTION = 'other'
+
+
+@pytest.fixture
+def mock_properties(monkeypatch):
+    # Keys in defaults dict and other dicts should not intersect. The tests assume
+    # that len(dict1.update(dict2)) == len(dict1) + len(dict2). So, for example, if
+    # defaults = {foo: 1, bar:2} and other_section = {foo:3}, the final dict will have len=2.
+    def mock_default_properties(path):
+        return {KEY1: VAL1, KEY2: VAL2}
+
+    def mock_read_from_yaml(path):
+        return {
+            'galaxy': {KEY3: VAL3, KEY4: VAL4},
+            OTHER_SECTION: {KEY5: VAL5, KEY6: VAL6}
+        }
+
+    class MockConfigParser():
+        def __init__(self):
+            self._items = {
+                'app:main': [(KEY3, VAL3), (KEY4, VAL4)],
+                OTHER_SECTION: [(KEY5, VAL5), (KEY6, VAL6)]
+            }
+
+        def has_section(self, section):
+            return section in self._items.keys()
+
+        def items(self, section):
+            return self._items[section] + self.defaults()
+
+        def defaults(self):
+            return [(KEY1, VAL1), (KEY2, VAL2)]
+
+    def mock_nice_config_parser(path):
+        return MockConfigParser()
+
+    monkeypatch.setattr(properties, '_read_from_yaml_file', mock_read_from_yaml)
+    monkeypatch.setattr(properties, 'nice_config_parser', mock_nice_config_parser)
+    monkeypatch.setattr(properties, '__default_properties', mock_default_properties)
+
+
+def test_read_galaxy_properties_from_yaml(mock_properties):
+    file = 'foo.yaml'
+    result = read_properties_from_file(file)
+
+    assert len(result) == 4
+    assert result[KEY1] == VAL1
+    assert result[KEY2] == VAL2
+    assert result[KEY3] == VAL3
+    assert result[KEY4] == VAL4
+
+
+def test_read_other_properties_from_yaml(mock_properties):
+    file = 'foo.yaml'
+    result = read_properties_from_file(file, config_section=OTHER_SECTION)
+
+    assert len(result) == 4
+    assert result[KEY1] == VAL1
+    assert result[KEY2] == VAL2
+    assert result[KEY5] == VAL5
+    assert result[KEY6] == VAL6
+
+
+def test_get_default_properties_from_yaml(mock_properties):
+    file = 'foo.yaml'
+    result = read_properties_from_file(file, config_section="section's not here, man")
+
+    assert len(result) == 2
+    assert result[KEY1] == VAL1
+    assert result[KEY2] == VAL2
+
+
+def test_read_galaxy_properties_from_ini(mock_properties):
+    file = 'foo.ini'
+    result = read_properties_from_file(file)
+
+    assert len(result) == 4
+    assert result[KEY1] == VAL1
+    assert result[KEY2] == VAL2
+    assert result[KEY3] == VAL3
+    assert result[KEY4] == VAL4
+
+
+def test_read_other_properties_from_ini(mock_properties):
+    file = 'foo.ini'
+    result = read_properties_from_file(file, config_section=OTHER_SECTION)
+
+    assert len(result) == 4
+    assert result[KEY1] == VAL1
+    assert result[KEY2] == VAL2
+    assert result[KEY5] == VAL5
+    assert result[KEY6] == VAL6
+
+
+def test_get_default_properties_from_ini(mock_properties):
+    file = 'foo.ini'
+    result = read_properties_from_file(file, config_section="section's not here, man")
+
+    assert len(result) == 2
+    assert result[KEY1] == VAL1
+    assert result[KEY2] == VAL2
+
+
+def test_dont_read_properties_from_other_format(mock_properties):
+    file = 'invalid'
+    with pytest.raises(InvalidFileFormatError):
+        read_properties_from_file(file)