From 138018dfe95093ca81a1d009e62c0ff00820692a Mon Sep 17 00:00:00 2001 From: Emmanuel Garette Date: Sat, 25 Jan 2014 11:20:11 +0100 Subject: [PATCH] if we delete all reference to a Config and we have reference to old SubConfig, Values, Multi or Settings, make a ConfigError instead of AttributError on NoneType object --- test/test_config.py | 24 +++++++++++- tiramisu/config.py | 10 ++++- tiramisu/setting.py | 25 ++++++++---- tiramisu/value.py | 94 +++++++++++++++++++++++++++++---------------- 4 files changed, 110 insertions(+), 43 deletions(-) diff --git a/test/test_config.py b/test/test_config.py index 5cdbe7d..6db3fcf 100644 --- a/test/test_config.py +++ b/test/test_config.py @@ -9,7 +9,7 @@ from py.test import raises from tiramisu.config import Config from tiramisu.option import IntOption, FloatOption, StrOption, ChoiceOption, \ BoolOption, UnicodeOption, OptionDescription -from tiramisu.error import ConflictError +from tiramisu.error import ConflictError, ConfigError def make_description(): @@ -273,4 +273,24 @@ def test_no_validation(): setting.append('validator') raises(ValueError, 'c.test1') del(c.test1) - assert c.test1 == None + assert c.test1 is None + + +def test_delete_config_with_subconfig(): + test = IntOption('test', '') + multi = IntOption('multi', '', multi=True) + od = OptionDescription('od', '', [test, multi]) + odroot = OptionDescription('odroot', '', [od]) + c = Config(odroot) + sub = c.od + val = c.cfgimpl_get_values() + setting = c.cfgimpl_get_settings() + val[test] + val[multi] + setting[test] + sub.make_dict() + del(c) + raises(ConfigError, 'val[test]') + raises(ConfigError, 'val[multi]') + raises(ConfigError, 'setting[test]') + raises(ConfigError, 'sub.make_dict()') diff --git a/tiramisu/config.py b/tiramisu/config.py index 1381d57..4bc0e15 100644 --- a/tiramisu/config.py +++ b/tiramisu/config.py @@ -156,7 +156,15 @@ class SubConfig(object): __repr__ = __str__ def _cfgimpl_get_context(self): - return self._impl_context() + """context could be None, we need to test it + context is None only if all reference to `Config` object is deleted + (for example we delete a `Config` and we manipulate a reference to + old `SubConfig`, `Values`, `Multi` or `Settings`) + """ + context = self._impl_context() + if context is None: + raise ConfigError(_('the context does not exist anymore')) + return context def cfgimpl_get_description(self): if self._impl_descr is None: diff --git a/tiramisu/setting.py b/tiramisu/setting.py index 02da5ae..101b551 100644 --- a/tiramisu/setting.py +++ b/tiramisu/setting.py @@ -24,7 +24,7 @@ from time import time from copy import copy import weakref from tiramisu.error import (RequirementError, PropertiesOptionError, - ConstError) + ConstError, ConfigError) from tiramisu.i18n import _ @@ -328,6 +328,17 @@ class Settings(object): self.context = weakref.ref(context) self._p_ = storage + def _getcontext(self): + """context could be None, we need to test it + context is None only if all reference to `Config` object is deleted + (for example we delete a `Config` and we manipulate a reference to + old `SubConfig`, `Values`, `Multi` or `Settings`) + """ + context = self.context() + if context is None: + raise ConfigError(_('the context does not exist anymore')) + return context + #____________________________________________________________ # properties methods def __contains__(self, propname): @@ -357,7 +368,7 @@ class Settings(object): if opt is not None and _path is None: _path = self._get_path_by_opt(opt) self._p_.reset_properties(_path) - self.context().cfgimpl_reset_cache() + self._getcontext().cfgimpl_reset_cache() def _getproperties(self, opt=None, path=None, is_apply_req=True): if opt is None: @@ -410,7 +421,7 @@ class Settings(object): self._p_.reset_properties(path) else: self._p_.setproperties(path, properties) - self.context().cfgimpl_reset_cache() + self._getcontext().cfgimpl_reset_cache() #____________________________________________________________ def validate_properties(self, opt_or_descr, is_descr, is_write, path, @@ -458,7 +469,7 @@ class Settings(object): properties -= frozenset(('mandatory', 'frozen')) else: if 'mandatory' in properties and \ - not self.context().cfgimpl_get_values()._isempty( + not self._getcontext().cfgimpl_get_values()._isempty( opt_or_descr, value): properties.remove('mandatory') if is_write and 'everything_frozen' in self_properties: @@ -581,6 +592,7 @@ class Settings(object): # filters the callbacks calc_properties = set() + context = self._getcontext() for requires in opt._requires: for require in requires: option, expected, action, inverse, \ @@ -592,8 +604,7 @@ class Settings(object): " '{0}' with requirement on: " "'{1}'").format(path, reqpath)) try: - value = self.context()._getattr(reqpath, - force_permissive=True) + value = context._getattr(reqpath, force_permissive=True) except PropertiesOptionError as err: if not transitive: continue @@ -622,7 +633,7 @@ class Settings(object): :param opt: `Option`'s object :returns: path """ - return self.context().cfgimpl_get_description().impl_get_path_by_opt(opt) + return self._getcontext().cfgimpl_get_description().impl_get_path_by_opt(opt) def get_modified_properties(self): return self._p_.get_modified_properties() diff --git a/tiramisu/value.py b/tiramisu/value.py index 8c50373..a02196a 100644 --- a/tiramisu/value.py +++ b/tiramisu/value.py @@ -46,13 +46,24 @@ class Values(object): # the storage type is dictionary or sqlite3 self._p_ = storage + def _getcontext(self): + """context could be None, we need to test it + context is None only if all reference to `Config` object is deleted + (for example we delete a `Config` and we manipulate a reference to + old `SubConfig`, `Values`, `Multi` or `Settings`) + """ + context = self.context() + if context is None: + raise ConfigError(_('the context does not exist anymore')) + return context + def _getdefault(self, opt): """ actually retrieves the default value :param opt: the `option.Option()` object """ - meta = self.context().cfgimpl_get_meta() + meta = self._getcontext().cfgimpl_get_meta() if meta is not None: value = meta.cfgimpl_get_values()[opt] else: @@ -105,11 +116,11 @@ class Values(object): if path is None: path = self._get_opt_path(opt) if self._p_.hasvalue(path): - setting = self.context().cfgimpl_get_settings() + context = self._getcontext() + setting = context.cfgimpl_get_settings() opt.impl_validate(opt.impl_getdefault(), - self.context(), - 'validator' in setting) - self.context().cfgimpl_reset_cache() + context, 'validator' in setting) + context.cfgimpl_reset_cache() if (opt.impl_is_multi() and opt.impl_get_multitype() == multitypes.master): for slave in opt.impl_get_master_slaves(): @@ -137,7 +148,7 @@ class Values(object): callback, callback_params = opt._callback if callback_params is None: callback_params = {} - return carry_out_calculation(opt, config=self.context(), + return carry_out_calculation(opt, config=self._getcontext(), callback=callback, callback_params=callback_params, index=index, max_len=max_len) @@ -151,7 +162,7 @@ class Values(object): if path is None: path = self._get_opt_path(opt) ntime = None - setting = self.context().cfgimpl_get_settings() + setting = self._getcontext().cfgimpl_get_settings() if 'cache' in setting and self._p_.hascache(path): if 'expire' in setting: ntime = int(time()) @@ -176,7 +187,8 @@ class Values(object): def _getitem(self, opt, path, validate, force_permissive, force_properties, validate_properties): # options with callbacks - setting = self.context().cfgimpl_get_settings() + context = self._getcontext() + setting = context.cfgimpl_get_settings() is_frozen = 'frozen' in setting[opt] # For calculating properties, we need value (ie for mandatory value). # If value is calculating with a PropertiesOptionError's option @@ -196,7 +208,7 @@ class Values(object): if (opt.impl_is_multi() and opt.impl_get_multitype() == multitypes.slave): masterp = self._get_opt_path(opt.impl_get_master_slaves()) - mastervalue = getattr(self.context(), masterp) + mastervalue = getattr(context, masterp) lenmaster = len(mastervalue) if lenmaster == 0: value = [] @@ -230,7 +242,7 @@ class Values(object): else: value = self._getvalue(opt, path, validate) if config_error is None and validate: - opt.impl_validate(value, self.context(), 'validator' in setting) + opt.impl_validate(value, context, 'validator' in setting) if config_error is None and self._is_default_owner(path) and \ 'force_store_value' in setting[opt]: self.setitem(opt, value, path, is_write=False) @@ -251,8 +263,9 @@ class Values(object): # is_write is, for example, used with "force_store_value" # user didn't change value, so not write # valid opt - opt.impl_validate(value, self.context(), - 'validator' in self.context().cfgimpl_get_settings()) + context = self._getcontext() + opt.impl_validate(value, context, + 'validator' in context.cfgimpl_get_settings()) if opt.impl_is_multi() and not isinstance(value, Multi): value = Multi(value, self.context, opt, path, setitem=True) self._setvalue(opt, path, value, force_permissive=force_permissive, @@ -261,14 +274,15 @@ class Values(object): def _setvalue(self, opt, path, value, force_permissive=False, force_properties=None, is_write=True, validate_properties=True): - self.context().cfgimpl_reset_cache() + context = self._getcontext() + context.cfgimpl_reset_cache() if validate_properties: - setting = self.context().cfgimpl_get_settings() + setting = context.cfgimpl_get_settings() setting.validate_properties(opt, False, is_write, value=value, path=path, force_permissive=force_permissive, force_properties=force_properties) - owner = self.context().cfgimpl_get_settings().getowner() + owner = context.cfgimpl_get_settings().getowner() self._p_.setvalue(path, value, owner) def getowner(self, opt): @@ -285,7 +299,7 @@ class Values(object): def _getowner(self, path): owner = self._p_.getowner(path, owners.default) - meta = self.context().cfgimpl_get_meta() + meta = self._getcontext().cfgimpl_get_meta() if owner is owners.default and meta is not None: owner = meta.cfgimpl_get_values()._getowner(path) return owner @@ -337,7 +351,7 @@ class Values(object): :param opt: the `option.Option` object :returns: a string with points like "gc.dummy.my_option" """ - return self.context().cfgimpl_get_description().impl_get_path_by_opt(opt) + return self._getcontext().cfgimpl_get_description().impl_get_path_by_opt(opt) # information def set_information(self, key, value): @@ -402,12 +416,24 @@ class Multi(list): self._valid_master(value) super(Multi, self).__init__(value) + def _getcontext(self): + """context could be None, we need to test it + context is None only if all reference to `Config` object is deleted + (for example we delete a `Config` and we manipulate a reference to + old `SubConfig`, `Values`, `Multi` or `Settings`) + """ + context = self.context() + if context is None: + raise ConfigError(_('the context does not exist anymore')) + return context + def _valid_slave(self, value, setitem): #if slave, had values until master's one - values = self.context().cfgimpl_get_values() - masterp = self.context().cfgimpl_get_description().impl_get_path_by_opt( + context = self._getcontext() + values = context.cfgimpl_get_values() + masterp = context.cfgimpl_get_description().impl_get_path_by_opt( self.opt.impl_get_master_slaves()) - mastervalue = getattr(self.context(), masterp) + mastervalue = getattr(context, masterp) masterlen = len(mastervalue) valuelen = len(value) is_default_owner = not values._is_default_owner(self.path) or setitem @@ -431,7 +457,7 @@ class Multi(list): def _valid_master(self, value): masterlen = len(value) - values = self.context().cfgimpl_get_values() + values = self._getcontext().cfgimpl_get_values() for slave in self.opt._master_slaves: path = values._get_opt_path(slave) if not values._is_default_owner(path): @@ -458,18 +484,19 @@ class Multi(list): self._validate(value, index) #assume not checking mandatory property super(Multi, self).__setitem__(index, value) - self.context().cfgimpl_get_values()._setvalue(self.opt, self.path, self) + self._getcontext().cfgimpl_get_values()._setvalue(self.opt, self.path, self) def append(self, value=undefined, force=False): """the list value can be updated (appened) only if the option is a master """ + context = self._getcontext() if not force: if self.opt.impl_get_multitype() == multitypes.slave: raise SlaveError(_("cannot append a value on a multi option {0}" " which is a slave").format(self.opt._name)) elif self.opt.impl_get_multitype() == multitypes.master: - values = self.context().cfgimpl_get_values() + values = context.cfgimpl_get_values() if value is undefined and self.opt.impl_has_callback(): value = values._getcallback_value(self.opt) #Force None il return a list @@ -480,9 +507,9 @@ class Multi(list): value = self.opt.impl_getdefault_multi() self._validate(value, index) super(Multi, self).append(value) - self.context().cfgimpl_get_values()._setvalue(self.opt, self.path, - self, - validate_properties=not force) + context.cfgimpl_get_values()._setvalue(self.opt, self.path, + self, + validate_properties=not force) if not force and self.opt.impl_get_multitype() == multitypes.master: for slave in self.opt.impl_get_master_slaves(): path = values._get_opt_path(slave) @@ -513,7 +540,7 @@ class Multi(list): super(Multi, self).sort(key=key, reverse=reverse) else: super(Multi, self).sort(cmp=cmp, key=key, reverse=reverse) - self.context().cfgimpl_get_values()._setvalue(self.opt, self.path, self) + self._getcontext().cfgimpl_get_values()._setvalue(self.opt, self.path, self) def reverse(self): if self.opt.impl_get_multitype() in [multitypes.slave, @@ -521,7 +548,7 @@ class Multi(list): raise SlaveError(_("cannot reverse multi option {0} if master or " "slave").format(self.opt._name)) super(Multi, self).reverse() - self.context().cfgimpl_get_values()._setvalue(self.opt, self.path, self) + self._getcontext().cfgimpl_get_values()._setvalue(self.opt, self.path, self) def insert(self, index, obj): if self.opt.impl_get_multitype() in [multitypes.slave, @@ -529,7 +556,7 @@ class Multi(list): raise SlaveError(_("cannot insert multi option {0} if master or " "slave").format(self.opt._name)) super(Multi, self).insert(index, obj) - self.context().cfgimpl_get_values()._setvalue(self.opt, self.path, self) + self._getcontext().cfgimpl_get_values()._setvalue(self.opt, self.path, self) def extend(self, iterable): if self.opt.impl_get_multitype() in [multitypes.slave, @@ -537,12 +564,12 @@ class Multi(list): raise SlaveError(_("cannot extend multi option {0} if master or " "slave").format(self.opt._name)) super(Multi, self).extend(iterable) - self.context().cfgimpl_get_values()._setvalue(self.opt, self.path, self) + self._getcontext().cfgimpl_get_values()._setvalue(self.opt, self.path, self) def _validate(self, value, force_index): if value is not None: try: - self.opt.impl_validate(value, context=self.context(), + self.opt.impl_validate(value, context=self._getcontext(), force_index=force_index) except ValueError as err: raise ValueError(_("invalid value {0} " @@ -560,13 +587,14 @@ class Multi(list): :type force: boolean :returns: item at index """ + context = self._getcontext() if not force: if self.opt.impl_get_multitype() == multitypes.slave: raise SlaveError(_("cannot pop a value on a multi option {0}" " which is a slave").format(self.opt._name)) elif self.opt.impl_get_multitype() == multitypes.master: for slave in self.opt.impl_get_master_slaves(): - values = self.context().cfgimpl_get_values() + values = context.cfgimpl_get_values() if not values.is_default_owner(slave): #get multi without valid properties values.getitem(slave, @@ -574,5 +602,5 @@ class Multi(list): ).pop(index, force=True) #set value without valid properties ret = super(Multi, self).pop(index) - self.context().cfgimpl_get_values()._setvalue(self.opt, self.path, self, validate_properties=not force) + context.cfgimpl_get_values()._setvalue(self.opt, self.path, self, validate_properties=not force) return ret