Clean up SoftwareReleaseSchema

This is mainly about error handling, simpler and more useful.

- Always warn when a valid SR can't be loaded, with a message that
  contains the original exception (for example, compared to before
  this commit, it will tell if a JSON file can't be found,
  or where JSON has syntax errors).

- Same as previous point if serialisation type is invalid or missing.
  If the caller needs it to transform parameters (parameters file),
  it will raise with the original exception. Otherwise, it falls back
  on json-in-xml. In some places, such fallback is a change of
  behaviour and I have no opinion about it except that at least
  it's now consistent throughout slapos.core.

- Remove warning about RootSoftwareInstance/default: meaningless
  because contradicted the comment, and useless because the transition
  to 'default' is already complete for SR schemas. There are still 3
  lines of backward compatibility code for the rest of slapos.core.

- Don't read the same file several times. Note however that this
  performance fix is only for the SoftwareReleaseSchema class:
  the caller should be fixed to not instanciate several times with
  the same parameters (from do_request & _requestComputerPartition).

See merge request !621

slapos/cli/info.py

@@ -40,7 +40,6 @@ from slapos.util import (
   SoftwareReleaseSchema,
   SoftwareReleaseSerialisation,
   StrPrettyPrinter,
-  UndefinedSerializationError,
   xml2dict,
 )
 
@@ -94,11 +93,7 @@ def do_info(logger, conf, local):
     else:
         # this is slapproxy connection dict
         connection_parameter_dict = xml2dict(instance._connection_dict)
-    try:
-        software_serialisation = software_schema.getSerialisation()
-    except UndefinedSerializationError:
-        software_serialisation = SoftwareReleaseSerialisation.JsonInXml
-    if software_serialisation == SoftwareReleaseSerialisation.JsonInXml:
+    if software_schema.getSerialisation() == SoftwareReleaseSerialisation.JsonInXml:
         if '_' in connection_parameter_dict:
             connection_parameter_dict = json.loads(connection_parameter_dict['_'])
 

slapos/cli/request.py

@@ -41,7 +41,7 @@ from slapos.client import (ClientConfig, _getSoftwareReleaseFromSoftwareString,
                            init)
 from slapos.slap import ResourceNotReady
 from slapos.util import (SoftwareReleaseSchema, SoftwareReleaseSerialisation,
-                         UndefinedSerializationError, StrPrettyPrinter)
+                         StrPrettyPrinter)
 
 try:
     from typing import IO, Dict
@@ -153,13 +153,13 @@ def do_request(logger, conf, local):
         conf.software_url = local[conf.software_url]
 
     software_schema = SoftwareReleaseSchema(conf.software_url, conf.type)
-    parameters = conf.parameters
     if conf.parameters_file:
-        if conf.force_serialisation:
-            parameters = getParametersFromFile(conf.parameters_file, SoftwareReleaseSerialisation(conf.force_serialisation))
+        serialisation = conf.force_serialisation
+        parameters = getParametersFromFile(conf.parameters_file,
+            SoftwareReleaseSerialisation(serialisation) if serialisation else
+            software_schema.getSerialisation(strict=True))
     else:
-            # getSerialisation will throw an exception if serialization cannot be found
-            parameters = getParametersFromFile(conf.parameters_file, software_schema.getSerialisation())
+        parameters = conf.parameters
     try:
         partition = local['slap'].registerOpenOrder().request(
             software_release=conf.software_url,
@@ -173,11 +173,10 @@ def do_request(logger, conf, local):
         logger.info('Instance requested.\nState is : %s.', partition.getState())
         logger.info('Connection parameters of instance are:')
         connection_parameter_dict = partition.getConnectionParameterDict()
-        try:
         if software_schema.getSerialisation() == SoftwareReleaseSerialisation.JsonInXml:
-                if '_' in connection_parameter_dict:
+            try:
                 connection_parameter_dict = json.loads(connection_parameter_dict['_'])
-        except UndefinedSerializationError:
+            except KeyError:
                 pass
         logger.info(StrPrettyPrinter().pformat(connection_parameter_dict))
         logger.info('You can rerun the command to get up-to-date information.')

slapos/slap/slap.py

@@ -49,7 +49,7 @@ import six
 from .exception import ResourceNotReady, ServerError, NotFoundError, \
           ConnectionError
 from .hateoas import SlapHateoasNavigator, ConnectionHelper
-from slapos.util import (SoftwareReleaseSchema, UndefinedSerializationError,
+from slapos.util import (SoftwareReleaseSchema,
                          bytes2str, calculate_dict_hash, dict2xml, dumps, loads,
                          unicode2str, xml2dict)
 
@@ -110,11 +110,6 @@ class SlapRequester(SlapDocument):
         "Request parameters do not validate against schema definition:\n{e}".format(e=e),
         UserWarning,
       )
-    except UndefinedSerializationError as e:
-      warnings.warn(
-        "No serialization type found:\n{e}".format(e=e),
-        UserWarning,
-      )
     except Exception as e:
       # note that we intentionally catch wide exceptions, so that if anything
       # is wrong with fetching the schema or the schema itself this does not

slapos/tests/test_cli.py

@@ -40,7 +40,7 @@ import pkg_resources
 from contextlib import contextmanager
 from mock import patch, create_autospec
 import mock
-from slapos.util import sqlite_connect, bytes2str, UndefinedSerializationError, dict2xml
+from slapos.util import sqlite_connect, bytes2str, dict2xml
 from slapos.slap.slap import DEFAULT_SOFTWARE_TYPE
 
 import slapos.cli.console
@@ -83,7 +83,9 @@ x2IMeSwJ82BpdEI5niXxB+iT0HxhmR+XaMI=
 def raiseNotFoundError(*args, **kwargs):
   raise slapos.slap.NotFoundError()
 
+
 class CliMixin(unittest.TestCase):
+
   def setUp(self):
     slap = slapos.slap.slap()
     self.logger = create_autospec(logging.Logger)
@@ -91,6 +93,24 @@ class CliMixin(unittest.TestCase):
     self.conf = create_autospec(ClientConfig)
     self.sign_cert_list = signature_certificate_list
 
+  def _do_request(self, connection_dict={}, json_in_xml=False):
+    cp = slapos.slap.ComputerPartition(
+        'computer_' + self.id(),
+        'partition_' + self.id())
+    cp._requested_state = 'started'
+    cp._connection_dict = {'_': json.dumps(connection_dict)} \
+        if json_in_xml else connection_dict
+
+    with patch.object(
+        slapos.slap.slap,
+        'registerOpenOrder',
+        return_value=mock.create_autospec(slapos.slap.OpenOrder)) as registerOpenOrder:
+      registerOpenOrder().request.return_value = cp
+      slapos.cli.request.do_request(self.logger, self.conf, self.local)
+
+    return registerOpenOrder().request
+
+
 class TestCliCacheBinarySr(CliMixin):
 
   test_url = "https://lab.nexedi.com/nexedi/slapos/raw/1.0.102/software/slaprunner/software.cfg"
@@ -901,9 +921,9 @@ class TestCliRequest(CliMixin):
     self.assertEqual(parse_option_dict(['a=a\nb']), {'a': 'a\nb'})
     self.assertEqual(parse_option_dict([]), {})
 
-  def test_request(self):
+  def _test_request(self, software_url, json_in_xml=False):
     self.conf.reference = 'instance reference'
-    self.conf.software_url = 'software URL'
+    self.conf.software_url = software_url
     self.conf.parameters = {'key': 'value'}
     self.conf.parameters_file = None
     self.conf.node = {'computer_guid': 'COMP-1234'}
@@ -912,14 +932,9 @@ class TestCliRequest(CliMixin):
     self.conf.slave = False
     self.conf.force_serialisation = None
 
-    with patch.object(
-        slapos.slap.slap,
-        'registerOpenOrder',
-        return_value=mock.create_autospec(slapos.slap.OpenOrder)) as registerOpenOrder:
-      slapos.cli.request.do_request(self.logger, self.conf, self.local)
-
-    registerOpenOrder().request.assert_called_once_with(
-        software_release='software URL',
+    connection_dict = {'foo': 'bar'}
+    self._do_request(connection_dict, json_in_xml).assert_called_once_with(
+        software_release=software_url,
         partition_reference='instance reference',
         partition_parameter_kw={'key': 'value'},
         software_type=None,
@@ -927,17 +942,23 @@ class TestCliRequest(CliMixin):
         state=None,
         shared=False,
     )
+    self.assertEqual(self.logger.info.mock_calls, [
+        mock.call('Requesting %s as instance of %s...',
+                  'instance reference', software_url),
+        mock.call('Instance requested.\nState is : %s.', 'started'),
+        mock.call('Connection parameters of instance are:'),
+        mock.call("{'foo': 'bar'}"),
+        mock.call('You can rerun the command to get up-to-date information.'),
+    ])
 
-    self.logger.info.assert_any_call(
-        'Requesting %s as instance of %s...',
-        'instance reference',
-        'software URL',
-    )
+  def test_request(self):
+    self._test_request('software URL', {'foo': 'bar'})
 
   def test_request_json_in_xml_published_parameters(self):
     tmpdir = tempfile.mkdtemp()
     self.addCleanup(shutil.rmtree, tmpdir)
-    with open(os.path.join(tmpdir, 'software.cfg.json'), 'w') as f:
+    sr = os.path.join(tmpdir, 'software.cfg')
+    with open(sr + '.json', 'w') as f:
       json.dump(
           {
               "name": "Test Software",
@@ -953,40 +974,7 @@ class TestCliRequest(CliMixin):
                   },
               }
           }, f)
-
-    self.conf.reference = 'instance reference'
-    self.conf.software_url = os.path.join(tmpdir, 'software.cfg')
-    self.conf.parameters = {'key': 'value'}
-    self.conf.parameters_file = None
-    self.conf.node = {'computer_guid': 'COMP-1234'}
-    self.conf.type = None
-    self.conf.state = None
-    self.conf.slave = False
-    self.conf.force_serialisation = None
-
-    cp = slapos.slap.ComputerPartition(
-        'computer_%s' % self.id(),
-        'partition_%s' % self.id())
-    cp._requested_state = 'started'
-    cp._connection_dict = {'_': json.dumps({'foo': 'bar'})}
-
-    with patch.object(
-        slapos.slap.slap,
-        'registerOpenOrder',
-        return_value=mock.create_autospec(slapos.slap.OpenOrder)) as registerOpenOrder:
-      registerOpenOrder().request.return_value = cp
-      slapos.cli.request.do_request(self.logger, self.conf, self.local)
-
-    registerOpenOrder().request.assert_called_once()
-
-    self.assertEqual(self.logger.info.mock_calls, [
-        mock.call('Requesting %s as instance of %s...', self.conf.reference,
-                  self.conf.software_url),
-        mock.call('Instance requested.\nState is : %s.', 'started'),
-        mock.call('Connection parameters of instance are:'),
-        mock.call("{'foo': 'bar'}"),
-        mock.call('You can rerun the command to get up-to-date information.'),
-    ])
+    self._test_request(sr, True)
 
 
 class TestCliRequestParameterFile(CliMixin):
@@ -1031,7 +1019,7 @@ class TestCliRequestParameterFileUndefinedSerialization(TestCliRequestParameterF
   def test_request_parameters_file(self):
     self._request_parameters_file_setup()
     self.assertRaises(
-        UndefinedSerializationError,
+        TypeError,
         slapos.cli.request.do_request,
         self.logger,
         self.conf,
@@ -1055,13 +1043,9 @@ class TestCliRequestParametersFileJson(TestCliRequestParameterFile):
     with mock.patch(
         'slapos.cli.request.SoftwareReleaseSchema.getSerialisation',
         return_value=self.serialization):
-      with patch.object(
-          slapos.slap.slap,
-          'registerOpenOrder',
-          return_value=mock.create_autospec(slapos.slap.OpenOrder)) as registerOpenOrder:
-        slapos.cli.request.do_request(self.logger, self.conf, self.local)
-
-      registerOpenOrder().request.assert_called_once_with(
+      self._do_request(
+          json_in_xml=self.serialization!='xml',
+      ).assert_called_once_with(
           software_release='software URL',
           partition_reference='instance reference',
           partition_parameter_kw=self.expected_partition_parameter_kw,
@@ -1138,13 +1122,7 @@ class TestCliRequestForceSerialisation(TestCliRequestParameterFile):
   def test_request_parameters_file(self):
     self._request_parameters_file_setup()
     self.conf.force_serialisation = 'json-in-xml'
-    with patch.object(
-        slapos.slap.slap,
-        'registerOpenOrder',
-        return_value=mock.create_autospec(slapos.slap.OpenOrder)) as registerOpenOrder:
-      slapos.cli.request.do_request(self.logger, self.conf, self.local)
-
-    registerOpenOrder().request.assert_called_once_with(
+    self._do_request(json_in_xml=True).assert_called_once_with(
         software_release='software URL',
         partition_reference='instance reference',
         partition_parameter_kw=self.expected_partition_parameter_kw,

slapos/tests/test_slap.py

@@ -1036,11 +1036,11 @@ class TestComputerPartition(SlapMixin):
             })
       raise ValueError(404)
 
-    for handler, warning_expected in (
-        (broken_reference, True),
-        (wrong_software_cfg_schema, False),
-        (wrong_instance_parameter_schema, True),
-        (invalid_instance_parameter_schema, True),
+    for handler in (
+      broken_reference,
+      wrong_software_cfg_schema,
+      wrong_instance_parameter_schema,
+      invalid_instance_parameter_schema,
     ):
       with httmock.HTTMock(handler):
         with mock.patch.object(warnings, 'warn') as warn:
@@ -1050,10 +1050,7 @@ class TestComputerPartition(SlapMixin):
           cp.request(
               'https://example.com/software.cfg', 'default', 'reference',
               partition_parameter_kw={'foo': 'bar'})
-        if warning_expected:
         warn.assert_called()
-        else:
-          warn.assert_not_called()
 
   def _test_new_computer_partition_state(self, state):
     """

slapos/tests/test_util.py

@@ -38,7 +38,6 @@ from six.moves import SimpleHTTPServer
 import jsonschema
 
 import slapos.util
-from slapos.slap.slap import DEFAULT_SOFTWARE_TYPE
 from slapos.testing.utils import ManagedHTTPServer
 from slapos.util import (SoftwareReleaseSchema, SoftwareReleaseSerialisation,
                          string_to_boolean, unicode2str)
@@ -270,11 +269,11 @@ class SoftwareReleaseSchemaTestMixin(object):
 
   def test_serialisation(self):
     schema = SoftwareReleaseSchema(self.software_url, None)
-    self.assertEqual(schema.getSerialisation(), self.serialisation)
+    self.assertEqual(schema.getSerialisation(strict=True), self.serialisation)
 
   def test_serialisation_alternate_software_type(self):
     schema = SoftwareReleaseSchema(self.software_url, 'alternate')
-    self.assertEqual(schema.getSerialisation(), self.serialisation_alt)
+    self.assertEqual(schema.getSerialisation(strict=True), self.serialisation_alt)
 
   def test_instance_request_parameter_schema_default_software_type(self):
     schema = SoftwareReleaseSchema(self.software_url, None)
@@ -354,7 +353,7 @@ class SoftwareReleaseSchemaTestFileSoftwareReleaseMixin(SoftwareReleaseSchemaTes
               "description": "Dummy software for Test",
               "serialisation": self.serialisation,
               "software-type": {
-                  DEFAULT_SOFTWARE_TYPE: {
+                  'default': {
                       "title": "Default",
                       "description": "Default type",
                       "request": "instance-default-input-schema.json",
@@ -373,9 +372,8 @@ class SoftwareReleaseSchemaTestFileSoftwareReleaseMixin(SoftwareReleaseSchemaTes
           }, f)
 
     for software_type in ('default', 'alternate'):
-      with open(
-          tmpfile('instance-{software_type}-input-schema.json'.format(
-              software_type=software_type)), 'w') as f:
+      with open(tmpfile('instance-%s-input-schema.json' % software_type),
+                'w') as f:
         json.dump(
             {
                 "$schema": "http://json-schema.org/draft-07/schema",
@@ -392,9 +390,8 @@ class SoftwareReleaseSchemaTestFileSoftwareReleaseMixin(SoftwareReleaseSchemaTes
                 },
                 "type": "object"
             }, f)
-      with open(
-          tmpfile('instance-{software_type}-output-schema.json'.format(
-              software_type=software_type)), 'w') as f:
+      with open(tmpfile('instance-%s-output-schema.json' % software_type),
+                'w') as f:
         json.dump(
             {
                 "$schema": "http://json-schema.org/draft-07/schema",

slapos/util.py

@@ -72,11 +72,6 @@ _ALLOWED_CLASS_SET = frozenset((
 ))
 
 
-class UndefinedSerializationError(ValueError):
-    """Raised when the serialization type is not found"""
-    pass
-
-
 class SafeXMLMarshaller(Marshaller):
   def m_instance(self, value, kw):
     cls = value.__class__
@@ -406,30 +401,32 @@ def rmtree(path):
 
 
 
-def _readAsJson(url):
+def _readAsJson(url, set_schema_id=False):
   # type: (str) -> Optional[Dict]
   """Reads and parse the json file located at `url`.
 
   `url` can also be the path of a local file.
   """
-  if url.startswith('file://'):
-    url = url[len('file://'):]
-  path = url if os.path.exists(url) else None
-
-  if path:
-    with open(path) as f:
   try:
-        return json.load(f)
-      except ValueError:
-        return None
     if url.startswith('http://') or url.startswith('https://'):
-    try:
       r = requests.get(url, timeout=60) # we need a timeout !
       r.raise_for_status()
-      return r.json()
-    except (requests.exceptions.RequestException, ValueError):
-      return None
-  return None
+      r = r.json()
+    else:
+      # XXX: https://discuss.python.org/t/file-uris-in-python/15600
+      if url.startswith('file://'):
+        path = url[7:]
+      else:
+        path = url
+        url = 'file:' + url
+      with open(path) as f:
+        r = json.load(f)
+    if set_schema_id and r:
+      r.setdefault('$id', url)
+    return r
+  except Exception as e:
+    warnings.warn("Unable to load JSON %s (%s: %s)"
+                  % (url, type(e).__name__, e))
 
 
 class SoftwareReleaseSerialisation(str, enum.Enum):
@@ -438,53 +435,61 @@ class SoftwareReleaseSerialisation(str, enum.Enum):
 
 
 class SoftwareReleaseSchema(object):
+
   def __init__(self, software_url, software_type):
     # type: (str, Optional[str]) ->  None
     self.software_url = software_url
-    self.software_type = software_type
+    # XXX: Transition from DEFAULT_SOFTWARE_TYPE ("RootSoftwareInstance")
+    #      to "default" is already complete for SR schemas.
+    from slapos.slap.slap import DEFAULT_SOFTWARE_TYPE
+    if software_type == DEFAULT_SOFTWARE_TYPE:
+      software_type = None
+    self.software_type = software_type or 'default'
+
+  def _warn(self, message, e):
+    warnings.warn(
+      "%s for software type %r of software release %s (%s: %s)"
+      % (message, self.software_type, self.software_url, type(e).__name__, e),
+      stacklevel=2)
 
   def getSoftwareSchema(self):
     # type: () -> Optional[Dict]
     """Returns the schema for this software.
     """
-    return _readAsJson(self.software_url + '.json')
+    try:
+      return self._software_schema
+    except AttributeError:
+      schema = self._software_schema = _readAsJson(self.software_url + '.json')
+    return schema
 
   def getSoftwareTypeSchema(self):
     # type: () -> Optional[Dict]
     """Returns schema for this software type.
     """
     software_schema = self.getSoftwareSchema()
-    if software_schema is None:
-      return None
-
-    software_type = self.software_type
-    from slapos.slap.slap import DEFAULT_SOFTWARE_TYPE
-    if software_type is None:
-      software_type = DEFAULT_SOFTWARE_TYPE
-    # XXX Some software are using "default" for default software type, because
-    # we are transitionning from DEFAULT_SOFTWARE_TYPE ("RootSoftwareInstance")
-    # to "default".
-    if software_type == DEFAULT_SOFTWARE_TYPE \
-        and software_type not in software_schema['software-type'] \
-        and 'default' in software_schema['software-type']:
-      warnings.warn(
-          "Software release {} does not have schema for DEFAULT_SOFTWARE_TYPE but has one for 'default'."
-          " Using 'default' instead.".format(self.software_url),
-          UserWarning,
-      )
-      software_type = 'default'
-    return software_schema['software-type'].get(software_type)
+    if software_schema is not None:
+      try:
+        return software_schema['software-type'][self.software_type]
+      except Exception as e:
+        self._warn("No schema defined", e)
 
-  def getSerialisation(self):
-    # type: () -> SoftwareReleaseSerialisation
+  def getSerialisation(self, strict=False):
+    # type: (bool) -> SoftwareReleaseSerialisation
     """Returns the serialisation method used for parameters.
+
+    If strict is False, catch exceptions and return JsonInXml.
     """
     software_schema = self.getSoftwareTypeSchema()
     if software_schema is None or 'serialisation' not in software_schema:
       software_schema = self.getSoftwareSchema()
-    if software_schema is None:
-      raise UndefinedSerializationError
+    try:
       return SoftwareReleaseSerialisation(software_schema['serialisation'])
+    except Exception as e:
+      if software_schema is not None: # else there was already a warning
+        self._warn("Invalid or undefined serialisation", e)
+      if strict:
+        raise
+    return SoftwareReleaseSerialisation.JsonInXml
 
   def getInstanceRequestParameterSchemaURL(self):
     # type: () -> Optional[str]
@@ -493,22 +498,18 @@ class SoftwareReleaseSchema(object):
     software_type_schema = self.getSoftwareTypeSchema()
     if software_type_schema is None:
       return None
-    software_url = self.software_url
-    if os.path.exists(software_url):
-      software_url = 'file://' + software_url
-    return urljoin(software_url, software_type_schema['request'])
+    return urljoin(self.software_url, software_type_schema['request'])
 
   def getInstanceRequestParameterSchema(self):
     # type: () -> Optional[Dict]
     """Returns the schema defining instance parameters.
     """
-    instance_parameter_schema_url = self.getInstanceRequestParameterSchemaURL()
-    if instance_parameter_schema_url is None:
-      return None
-    schema = _readAsJson(instance_parameter_schema_url)
-    if schema:
-      # so that jsonschema knows how to resolve references
-      schema.setdefault('$id', instance_parameter_schema_url)
+    try:
+      return self._request_schema
+    except AttributeError:
+      url = self.getInstanceRequestParameterSchemaURL()
+      schema = None if url is None else _readAsJson(url, True)
+      self._request_schema = schema
     return schema
 
   def getInstanceConnectionParameterSchemaURL(self):
@@ -524,13 +525,12 @@ class SoftwareReleaseSchema(object):
     # type: () -> Optional[Dict]
     """Returns the schema defining connection parameters published by the instance.
     """
-    instance_parameter_schema_url = self.getInstanceConnectionParameterSchemaURL()
-    if instance_parameter_schema_url is None:
-      return None
-    schema = _readAsJson(instance_parameter_schema_url)
-    if schema:
-      # so that jsonschema knows how to resolve references
-      schema.setdefault('$id', instance_parameter_schema_url)
+    try:
+      return self._response_schema
+    except AttributeError:
+      url = self.getInstanceConnectionParameterSchemaURL()
+      schema = None if url is None else _readAsJson(url, True)
+      self._response_schema = schema
     return schema
 
   def validateInstanceParameterDict(self, parameter_dict):
@@ -539,19 +539,15 @@ class SoftwareReleaseSchema(object):
 
     Raise jsonschema.ValidationError if parameters does not validate.
     """
-    schema_url = self.getInstanceRequestParameterSchemaURL()
-    if schema_url:
-      instance = parameter_dict
-      if self.getSerialisation() == SoftwareReleaseSerialisation.JsonInXml:
+    schema = self.getInstanceRequestParameterSchema()
+    if schema:
+      if self.getSerialisation(strict=True) == SoftwareReleaseSerialisation.JsonInXml:
         try:
-          instance = json.loads(parameter_dict['_'])
+          parameter_dict = json.loads(parameter_dict['_'])
         except KeyError:
-          instance = parameter_dict
-      instance.pop('$schema', None)
-      jsonschema.validate(
-          instance=instance,
-          schema=self.getInstanceRequestParameterSchema(),
-      )
+          pass
+      parameter_dict.pop('$schema', None)
+      jsonschema.validate(instance=parameter_dict, schema=schema)
 
 
 # BBB on python3 we can use pprint.pformat