fixup! Add support for a few built-in python object types as values

get is used by __getitem__, but also by other callers as it is public
method. So deserialize values if needed on each access by get, and
remove now needless deserialization in __getitem__

As Options.get is used internally, add Options._get without
deserialisation and use it in places, which expect clean result, and
adapt Options.get to be simple wrapper with deserialization without
using decoration with functools.

/reviewed-on nexedi/slapos.buildout!10

src/zc/buildout/buildout.py

@@ -1438,7 +1438,17 @@ class Options(DictMixin):
                        for s in v.split('$$')])
         self._cooked[option] = v
 
-    def get(self, option, default=None, seen=None, last=True):
+    def get(self, *args, **kw):
+      v = self._get(*args, **kw)
+      if hasattr(v, 'startswith') and v.startswith(SERIALISED_VALUE_MAGIC):
+        v = loads(v)
+      return v
+
+    def _get(self, option, default=None, seen=None, last=True):
+        # TODO: raise instead of handling a default parameter,
+        #       so that get() never tries to deserialize a default value
+        #       (and then: move deserialization to __getitem__
+        #                  and make get() use __getitem__)
         try:
             if last:
                 return self._data[option].replace('$${', '${')
@@ -1513,7 +1523,7 @@ class Options(DictMixin):
                     options = self.buildout[section]
                 finally:
                     del self.buildout._initializing[-1]
-            v = options.get(option, None, seen, last=last)
+            v = options._get(option, None, seen, last=last)
             if v is None:
                 if option == '_buildout_section_name_':
                     v = self.name
@@ -1529,8 +1539,6 @@ class Options(DictMixin):
         v = self.get(key)
         if v is None:
             raise MissingOption("Missing option: %s:%s" % (self.name, key))
-        elif v.startswith(SERIALISED_VALUE_MAGIC):
-            v = loads(v)
         return v
 
     def __setitem__(self, option, value):
@@ -1650,8 +1658,12 @@ def _save_option(option, value, f):
 
 def _save_options(section, options, f):
     print_('[%s]' % section, file=f)
-    for option in sorted(options.keys()):
-        _save_option(option, options.get(option), f)
+    try:
+        get_option = options._get
+    except AttributeError:
+        get_option = options.get
+    for option in sorted(options):
+        _save_option(option, get_option(option), f)
 
 def _default_globals():
     """Return a mapping of default and precomputed expressions.

[Vincent Pelletier]

There is a problem here: now that get deserialises, values which are set to serialised None cause below MissingOption exception on access.

This is especially visible with xml instance parameter scheme, as empty strings become None values, making for a very confusing debugging session.

IMHO the original code uses a bad pattern anyway: I believe in python dict-ish get should always be a try..except KeyError: around a __getitem__, not the other way around. It was enough originally as None was never a valid value (only strings), but breaks when serialisation support is added. And this commit made the breakage apparent (...though it slipped through review).

[Julien Muchembled]

the original code uses a bad pattern anyway: I believe in python dict-ish get should always be a try..except KeyError: around a __getitem__, not the other way around

Yes, that's what I said in a comment of !10 (merged).

I also got the MissingOption issue 2 days ago. In order to keep minimal patches, we can also use a MARKER value.

[Vincent Pelletier]

MARKER is fine for me.

FWIW, this bug prevents deploying ERP5 SR built-in SMTP relay without a next-hop, as it considers the key is missing in corresponding section, in turn making it impossible to deploy a "safe" clone (ie, an ERP5 which would not send mails to actual customers), as the SMTP relay has the ability to funnel all such mails to a replacement address.

[Julien Muchembled]

Is 33b34339 ok ?

[Alain Takoudjou]

@jm @vpelletier this problem was present on kvm cluster (see: slapos!231 (closed)). After applying the patch 33b34339 it solved the problem. @jm Can you merge it ?

[Julien Muchembled]

Thanks. 2.5.2+slapos010 released.