Bugfix on settings_loader default values solves #308

globconf.d/lodel2.ini

@@ -2,6 +2,13 @@
 debug = False
 sitename = noname
 datasource_connectors = dummy_datasource
+session_handler = filesystem_session
+
+[lodel2.datasources.default]
+identifier = dummy_datasource.default
+
+[lodel2.datasource.dummy_datasource.default]
+dummy =
 
 [lodel2.logging.stderr]
 level = ERROR

lodel/plugins/dummy_datasource/__init__.py

@@ -12,7 +12,7 @@ __plugin_deps__ = []
 CONFSPEC = {
     'lodel2.datasource.dummy_datasource.*' : {
         'dummy': (  None,
-                    Validator('dummy'))}
+                    Validator('dummy', none_is_valid=True))}
 }
 
 

lodel/settings/settings_loader.py

@@ -8,7 +8,8 @@ from lodel.context import LodelContext
 
 LodelContext.expose_modules(globals(), {
     'lodel.logger': 'logger',
-    'lodel.settings.utils': ['SettingsError', 'SettingsErrors']})
+    'lodel.settings.utils': ['SettingsError', 'SettingsErrors'],
+    'lodel.validator.validator': ['ValidationError']})
 
 ##@brief Merges and loads configuration files
 class SettingsLoader(object):
@@ -36,9 +37,9 @@ class SettingsLoader(object):
         l_dir = glob.glob(self.__conf_path+'/*.ini')
         logger.debug("SettingsLoader found those settings files : %s" % (
             ', '.join(l_dir)))
-
         for f_ini in l_dir:
-            config = configparser.ConfigParser(default_section=self.DEFAULT_SECTION, interpolation=None)
+            config = configparser.ConfigParser(
+	    	default_section = self.DEFAULT_SECTION ,interpolation=None)
             config.read(f_ini)
             for section in [s for s in config if s != self.DEFAULT_SECTION]:
                 if section not in conf:
@@ -60,39 +61,24 @@ class SettingsLoader(object):
     # @param keyname str
     # @param validator callable : takes one argument value and raises validation fail
     # @param default_value *
-    # @param mandatory bool
     # @return the option
-    def getoption(self, section, keyname, validator, default_value=None, mandatory=False):
-        conf = self.__conf
+    def getoption(self,section,keyname,validator,default_value=None):
+        conf=self.__conf
         if section not in conf:
             conf[section] = dict()
 
         sec = conf[section]
-        result = None
         if keyname in sec:
             result = sec[keyname]['value']
-            if result is not None:
-                result = result.strip()
-                if len(result) == 0:
-                    result = None
             try:
                 del self.__conf_sv[section + ':' + keyname]
             except KeyError: #allready fetched
                 pass
-        if result is None:
-            if default_value is None and mandatory:
-                msg = "Default value mandatory for option %s" % keyname
-                expt = SettingsError(msg=msg, key_id=section+'.'+keyname, \
-                    filename=sec[keyname]['file'])
-                self.__errors_list.append(expt)
-                return
-            else:
-                sec[keyname] = dict()
-                sec[keyname]['value'] = default_value
-                sec[keyname]['file'] = SettingsLoader.DEFAULT_FILENAME
-                result = default_value
-                logger.debug("Using default value for configuration key %s:%s" \
-                    % (section, keyname))
+        else:
+            #default values
+            sec[keyname] = dict()
+            result = sec[keyname]['value'] = default_value
+            sec[keyname]['file'] = SettingsLoader.DEFAULT_FILENAME
 
         try:
             return validator(result)
@@ -103,7 +89,7 @@ class SettingsLoader(object):
                                     key_id=section+'.'+keyname)
                 self.__errors_list.append(expt)
             else:
-                #expt = ValidationError("For %s.%s : %s" % (section, keyname, e))
+                expt = ValidationError("For %s.%s : %s" % (section, keyname, e))
                 expt2 = SettingsError(msg=str(expt), \
                                         key_id=section+'.'+keyname, \
                                         filename=sec[keyname]['file'])

lodel/validator/validator.py

@@ -57,6 +57,11 @@ class Validator(MlNamedObject):
     # @return properly casted value
     # @throw ValidationError
     def __call__(self, value):
+        if value is None:
+            if self.__none_is_valid:
+                return None
+            else:
+                raise ValidationError('None is not a valid value')
         if self.__none_is_valid and value is None:
             return None
         try:
@@ -94,6 +99,9 @@ class Validator(MlNamedObject):
     def create_list_validator(cls, validator_name, elt_validator, description=None, separator=','):
         def list_validator(value):
             res = list()
+            if value is None:
+                return res
+            errors = list()
             for elt in value.split(separator):
                 elt = elt_validator(elt)
                 if len(elt) > 0:
@@ -334,7 +342,7 @@ def emfield_val(value):
     spl = value.split('.')
     if len(spl) != 2:
         msg = "Expected a value in the form CLASSNAME.FIELDNAME but got : %s"
-        raise SettingsValidationError(msg % value)
+        raise ValidationError(msg % value)
     value = tuple(spl)
     # Late validation hook
 
@@ -344,11 +352,11 @@ def emfield_val(value):
         classnames = {cls.__name__.lower(): cls for cls in dyncode.dynclasses}
         if value[0].lower() not in classnames:
             msg = "Following dynamic class do not exists in current EM : %s"
-            raise SettingsValidationError(msg % value[0])
+            raise ValidationError(msg % value[0])
         ccls = classnames[value[0].lower()]
         if value[1].lower() not in ccls.fieldnames(True):
             msg = "Following field not found in class %s : %s"
-            raise SettingsValidationError(msg % value)
+            raise ValidationError(msg % value)
     return value
 
 # @brief Validator for plugin name & optionnaly type

tests/settings/test_settings_loader.py

@@ -6,6 +6,7 @@ import os.path
 from lodel.settings.utils import *
 from lodel.plugin.exceptions import *
 from lodel.settings.settings_loader import SettingsLoader
+from lodel.validator.validator import *
 
 
 
@@ -87,12 +88,21 @@ class SettingsLoaderTestCase(unittest.TestCase):
         """ Testing behavior of default value in getoption """
         loader = SettingsLoader('tests/settings/settings_examples/simple.conf.d')
         # for non existing keys in file
-        value = loader.getoption('lodel2.foo.bar', 'foofoofoo', dummy_validator, 'hello 42', False)
+        value = loader.getoption('lodel2.foo.bar', 'foofoofoo', dummy_validator, 'hello 42')
         self.assertEqual(value, 'hello 42')
         # for non existing section in file
-        value = loader.getoption('lodel2.foofoo', 'foofoofoo', dummy_validator, 'hello 42', False)
+        value = loader.getoption('lodel2.foofoo', 'foofoofoo', dummy_validator, 'hello 42')
         self.assertEqual(value, 'hello 42')
 
+    def test_geoption_invalid_default_value(self):
+        """ Testing the behavior when the default value is invalid """
+        loader = SettingsLoader('tests/settings/settings_examples/simple.conf.d')
+        mandatory_validator = Validator('string', none_is_valid=False)
+        with self.assertRaises(SettingsErrors):
+            value = loader.getoption(
+                'lodel2.foo.bar', 'foofoofooDEFAULT', mandatory_validator)
+            loader.raise_errors()
+
     def test_getoption_complex(self):
         """ Testing behavior of getoption with less simple files & confs """
 
@@ -235,10 +245,10 @@ class SettingsLoaderTestCase(unittest.TestCase):
         loader = SettingsLoader('tests/settings/settings_examples/conf_setdef.d')
             
         # for non existing keys in file
-        value = loader.getoption('lodel2.foo.bar', 'foofoofoo', dummy_validator, 'hello 42', False)
+        value = loader.getoption('lodel2.foo.bar', 'foofoofoo', dummy_validator, 'hello 42')
         self.assertEqual(value, 'hello 42')
         # for non existing section in file
-        value = loader.getoption('lodel2.foofoo', 'foofoofoo', dummy_validator, 'hello 42', False)
+        value = loader.getoption('lodel2.foofoo', 'foofoofoo', dummy_validator, 'hello 42')
         self.assertEqual(value, 'hello 42')
         
         loader.setoption('lodel2.foo.bar', 'foofoofoo', 'test ok', dummy_validator)
@@ -253,8 +263,3 @@ class SettingsLoaderTestCase(unittest.TestCase):
         
         os.remove('tests/settings/settings_examples/conf_setdef.d/generated.ini')
         
-    def test_invalid_conf(self):
-        from lodel.settings.settings import Settings
-        Settings.stop()
-        with self.assertRaises((SettingsErrors, PluginError)):
-            Settings('tests/settings/settings_examples/bad_conf.d')