Change the config handling (#85)

Better paths handling in config
* Use pathlib instead of os.path
* Better handling of the relative paths in manifest
  - can use all the "." + something (i.e: ./dir, ../, ./../, etc)
* Add raises to avoid errors :
  - double abs path resulting in single abs path
  - manifest anchor with more than one sub-anchor
  - check if all the paths returned are absolute paths
* manifest not mandatory
* strings resolved as path only if contains '$' or starts by '.'

bluepysnap/config.py

@@ -20,11 +20,12 @@
 # TODO: move to `libsonata` library
 
 import collections
-import os.path
+from pathlib2 import Path
 
 import six
 
 from bluepysnap import utils
+from bluepysnap.exceptions import BluepySnapError
 
 
 class Config(object):
@@ -43,9 +44,9 @@ def __init__(self, filepath):
         Returns:
              Config: A Config object.
         """
-        configdir = os.path.abspath(os.path.dirname(filepath))
-        content = utils.load_json(filepath)
-        self.manifest = Config._resolve_manifest(content.pop('manifest'), configdir)
+        configdir = str(Path(filepath).parent.resolve())
+        content = utils.load_json(str(filepath))
+        self.manifest = Config._resolve_manifest(content.pop('manifest', {}), configdir)
         self.content = content
 
     @staticmethod
@@ -56,33 +57,46 @@ def _resolve_manifest(manifest, configdir):
         result['${configdir}'] = configdir
 
         for k, v in six.iteritems(result):
-            if v == '.':
-                result[k] = configdir
-            elif v.startswith('./'):
-                result[k] = os.path.join(configdir, v[2:])
+            if v.startswith('.'):
+                result[k] = str(Path(configdir, v).resolve())
 
         while True:
             update = False
             for k, v in six.iteritems(result):
+                if v.count('$') > 1:
+                    raise BluepySnapError(
+                        '{} is not a valid anchor : contains more than one sub anchor.'.format(k))
                 if v.startswith('$'):
                     tokens = v.split('/', 1)
                     resolved = result[tokens[0]]
                     if '$' not in resolved:
-                        result[k] = os.path.join(resolved, *tokens[1:])
+                        result[k] = str(Path(resolved, *tokens[1:]))
                         update = True
             if not update:
                 break
 
+        for k, v in result.items():
+            if not v.startswith('/'):
+                raise BluepySnapError("{} cannot be resolved as an abs path.".format(k))
+
         return result
 
-    def _resolve_path(self, value):
+    def _resolve_string(self, value):
+        # not a startswith to detect the badly placed anchors
         if '$' in value:
             vs = [
                 self.manifest[v] if v.startswith('$') else v
                 for v in value.split('/')
             ]
-            return os.path.join(*vs)
+            abs_paths = [v for v in vs[1:] if v.startswith('/')]
+            if len(abs_paths) != 0:
+                raise BluepySnapError("Misplaced anchors in : {}."
+                                      "Please verify your '$' usage.".format(value))
+            return str(Path(*vs))
+        elif value.startswith('.'):
+            return str(Path(self.manifest['${configdir}'], value).resolve())
         else:
+            # we cannot know if a string is a path or not if it does not contain anchor or .
             return value
 
     def _resolve(self, value):
@@ -91,7 +105,7 @@ def _resolve(self, value):
                 k: self._resolve(v) for k, v in six.iteritems(value)
             }
         elif isinstance(value, six.string_types):
-            return self._resolve_path(value)
+            return self._resolve_string(value)
         elif isinstance(value, collections.Iterable):
             return [self._resolve(v) for v in value]
         else:

tests/test_config.py

@@ -1,5 +1,10 @@
+import pytest
+from pathlib2 import Path
+
 import bluepysnap.config as test_module
-from utils import TEST_DATA_DIR
+from bluepysnap.exceptions import BluepySnapError
+
+from utils import TEST_DATA_DIR, edit_config, copy_config
 
 
 def test_parse():
@@ -8,13 +13,113 @@ def test_parse():
     )
 
     # check double resolution and '.' works: $COMPONENT_DIR -> $BASE_DIR -> '.'
-    assert (
-        actual['components']['morphologies_dir'] ==
-        str(TEST_DATA_DIR / 'morphologies')
-    )
+    assert actual['components']['morphologies_dir'] == str(TEST_DATA_DIR / 'morphologies')
 
     # check resolution and './' works: $NETWORK_DIR -> './'
-    assert (
-        actual['networks']['nodes'][0]['nodes_file'] ==
-        str(TEST_DATA_DIR / 'nodes.h5')
+    assert actual['networks']['nodes'][0]['nodes_file'] == str(TEST_DATA_DIR / 'nodes.h5')
+
+    # check resolution of '../' works: $PARENT --> '../'
+    with copy_config() as config_path:
+        with edit_config(config_path) as config:
+            config["manifest"]["$PARENT"] = "../"
+            config["components"]["other"] = "$PARENT/other"
+
+        actual = test_module.Config.parse(config_path)
+        assert (
+                actual['components']['other'] ==
+                str(Path(config_path.parent / "../other").resolve())
+        )
+
+    # check resolution of '../' works in a path outside manifest
+    with copy_config() as config_path:
+        with edit_config(config_path) as config:
+            config["components"]["other"] = "../other"
+
+        actual = test_module.Config.parse(config_path)
+        assert (
+                actual['components']['other'] ==
+                str(Path(config_path.parent / "../other").resolve())
+        )
+
+    # check resolution without manifest of '../' works in a path outside
+    # i.e. : self.manifest contains the configdir even if manifest is not here
+    with copy_config() as config_path:
+        with edit_config(config_path) as config:
+            for k in list(config): config.pop(k)
+            config["something"] = "../other"
+        actual = test_module.Config.parse(config_path)
+        assert actual["something"] == str(Path(config_path.parent / "../other").resolve())
+
+    # check resolution with multiple slashes
+    with copy_config() as config_path:
+        with edit_config(config_path) as config:
+            config["something"] = "$COMPONENT_DIR/something////else"
+        actual = test_module.Config.parse(config_path)
+        assert actual["something"] == str(Path(config_path.parent) / 'something' / 'else')
+
+    # check resolution for non path objects
+    with copy_config() as config_path:
+        with edit_config(config_path) as config:
+            for k in list(config): config.pop(k)
+            config["string"] = "string"
+            config["int"] = 1
+            config["double"] = 0.2
+            # just to check because we use starting with '.' as a special case
+            config["tricky_double"] = .2
+            config["path"] = "./path"
+
+        actual = test_module.Config.parse(config_path)
+
+        # string
+        assert actual["string"] == "string"
+        # int
+        assert actual["int"] == 1
+        # double
+        assert actual["double"] == 0.2
+        assert actual["tricky_double"] == 0.2
+        # path
+        assert actual["path"] == str(Path(config_path.parent / "./path").resolve())
+
+
+def test_bad_manifest():
+    # not abs path in
+    with copy_config() as config_path:
+        with edit_config(config_path) as config:
+            config["manifest"]["$BASE_DIR"] = "not_absolute"
+        with pytest.raises(BluepySnapError):
+            test_module.Config.parse(config_path)
+
+    with copy_config() as config_path:
+        with edit_config(config_path) as config:
+            config["manifest"]["$COMPONENT_DIR"] = "$BASE_DIR/$NETWORK_DIR"
+        with pytest.raises(BluepySnapError):
+            test_module.Config.parse(config_path)
+
+    with copy_config() as config_path:
+        with edit_config(config_path) as config:
+            config["components"]["other"] = "$COMPONENT_DIR/$BASE_DIR"
+        with pytest.raises(BluepySnapError):
+            test_module.Config.parse(config_path)
+
+    with copy_config() as config_path:
+        with edit_config(config_path) as config:
+            config["components"]["other"] = "something/$COMPONENT_DIR/"
+        with pytest.raises(BluepySnapError):
+            test_module.Config.parse(config_path)
+
+    with copy_config() as config_path:
+        with edit_config(config_path) as config:
+            config["components"]["other"] = "/something/$COMPONENT_DIR/"
+        with pytest.raises(BluepySnapError):
+            test_module.Config.parse(config_path)
+
+
+def test_simulation_config():
+    actual = test_module.Config.parse(
+        str(TEST_DATA_DIR / 'simulation_config.json')
     )
+    assert actual["target_simulator"] == "my_simulator"
+    assert actual["network"] == str(Path(TEST_DATA_DIR / "circuit_config.json").resolve())
+    assert actual["mechanisms_dir"] == str(Path(TEST_DATA_DIR / "../shared_components_mechanisms").resolve())
+    assert actual["conditions"]["celsius"] == 34.0
+    assert actual["conditions"]["v_init"] == -80

tests/utils.py

@@ -42,7 +42,20 @@ def copy_circuit(config='circuit_config.json'):
     with setup_tempdir() as tmp_dir:
         copy_tree(str(TEST_DATA_DIR), tmp_dir)
         circuit_copy_path = Path(tmp_dir)
-        yield (circuit_copy_path, circuit_copy_path / config)
+        yield circuit_copy_path, circuit_copy_path / config
+
+
+@contextmanager
+def copy_config(config='circuit_config.json'):
+    """Copies config to a temp directory.
+
+    Returns:
+        yields a path to the copy of the config file
+    """
+    with setup_tempdir() as tmp_dir:
+        output = Path(tmp_dir, config)
+        shutil.copy(str(TEST_DATA_DIR / config), str(output))
+        yield output
 
 
 @contextmanager