slap/slap.py: drop parameter schema validation

The performance impact is way to important, as it requires to
fetch all schema files.
For history, one use case was: 10 seconds to validate, 50ms to request.

Software releases should instead use the slapconfiguration:jsonschema
recipe to reject wrong input.
The recipe must use the schema informations stored on the installation directory.
slapos!1638

Move automated schema validation to the cli/request.py client.
Inform when the client start the parameter validation, to spot slowness.

slapos/cli/request.py

@@ -31,6 +31,7 @@ import argparse
 import json
 import os.path
 import pprint
+import warnings
 
 import lxml.etree
 import six
@@ -41,7 +42,7 @@ from slapos.client import (ClientConfig, _getSoftwareReleaseFromSoftwareString,
                            init)
 from slapos.slap import ResourceNotReady
 from slapos.util import (SoftwareReleaseSchema, SoftwareReleaseSerialisation,
-                         StrPrettyPrinter)
+                         SoftwareReleaseSchemaValidationError, StrPrettyPrinter)
 
 try:
     from typing import IO, Dict
@@ -142,8 +143,30 @@ class RequestCommand(ClientConfigCommand):
         do_request(self.app.log, conf, local)
 
 
+def _validateRequestParameters(software_schema, parameter_dict):
+  """Validate requests parameters.
+
+  Default behavior is to fetch schema and warn using `warnings.warn` in case
+  of problems
+  """
+  try:
+    software_schema.validateInstanceParameterDict(parameter_dict)
+  except SoftwareReleaseSchemaValidationError as e:
+    warnings.warn(
+      "Request parameters do not validate against schema definition:\n{e}".format(e=e.format_error(indent=2)),
+      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
+    # prevent users from requesting instances.
+    warnings.warn(
+      "Error validating request parameters against schema definition:\n{e.__class__.__name__} {e}".format(e=e),
+      UserWarning,
+    )
+
 def do_request(logger, conf, local):
-    logger.info('Requesting %s as instance of %s...',
+    logger.info('Validating parameters for %s as instance of %s...',
                 conf.reference, conf.software_url)
 
     conf.software_url = _getSoftwareReleaseFromSoftwareString(
@@ -160,6 +183,20 @@ def do_request(logger, conf, local):
             software_schema.getSerialisation(strict=True))
     else:
         parameters = conf.parameters
+
+    # special case for the magic git.erp5.org used as an alias
+    # for frontend software release that does not have a software.cfg.json
+    if conf.software_url not in (
+      # no corresponding schema
+      'http://git.erp5.org/gitweb/slapos.git/blob_plain/HEAD:/software/apache-frontend/software.cfg',
+    ):
+      _validateRequestParameters(
+        software_schema,
+        parameters,
+      )
+
+    logger.info('Requesting %s as instance of %s...',
+                conf.reference, conf.software_url)
     try:
         partition = local['slap'].registerOpenOrder().request(
             software_release=conf.software_url,

slapos/slap/slap.py

@@ -40,7 +40,6 @@ import os
 import logging
 import re
 from functools import wraps
-import warnings
 
 import json
 import six
@@ -53,8 +52,7 @@ except ImportError: # XXX to be removed once we depend on typing
 from .exception import ResourceNotReady, ServerError, NotFoundError, \
           ConnectionError
 from .hateoas import SlapHateoasNavigator, ConnectionHelper
-from slapos.util import (SoftwareReleaseSchema, SoftwareReleaseSchemaValidationError,
-                         bytes2str, calculate_dict_hash, dict2xml, dumps, loads,
+from slapos.util import (bytes2str, calculate_dict_hash, dict2xml, dumps, loads,
                          unicode2str, xml2dict)
 
 from xml.sax import saxutils
@@ -92,44 +90,7 @@ class SlapRequester(SlapDocument):
   """
   Abstract class that allow to factor method for subclasses that use "request()"
   """
-  def _validateRequestParameters(self, software_release, software_type, parameter_dict):
-    """Validate requests parameters.
-
-    Default behavior is to fetch schema and warn using `warnings.warn` in case
-    of problems, with a special case for the magic git.erp5.org used as an alias
-    for frontend software release that does not have a software.cfg.json
-    """
-    if software_release in (
-      # no corresponding schema
-      'http://git.erp5.org/gitweb/slapos.git/blob_plain/HEAD:/software/apache-frontend/software.cfg',
-    ):
-      return
-
-    try:
-      SoftwareReleaseSchema(
-        software_release,
-        software_type,
-      ).validateInstanceParameterDict(parameter_dict)
-    except SoftwareReleaseSchemaValidationError as e:
-      warnings.warn(
-        "Request parameters do not validate against schema definition:\n{e}".format(e=e.format_error(indent=2)),
-        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
-      # prevent users from requesting instances.
-      warnings.warn(
-        "Error validating request parameters against schema definition:\n{e.__class__.__name__} {e}".format(e=e),
-        UserWarning,
-      )
-
   def _requestComputerPartition(self, request_dict):
-    self._validateRequestParameters(
-      request_dict['software_release'],
-      request_dict['software_type'],
-      loads(request_dict['partition_parameter_xml']),
-    )
     try:
       xml = self._connection_helper.POST('requestComputerPartition', data=request_dict)
     except ResourceNotReady:

slapos/tests/test_cli.py

@@ -25,6 +25,7 @@
 #
 ##############################################################################
 
+import httmock
 import json
 import logging
 import pprint
@@ -36,6 +37,7 @@ import textwrap
 import sys
 import os
 import pkg_resources
+import warnings
 
 from contextlib import contextmanager
 from mock import patch, create_autospec
@@ -976,6 +978,251 @@ class TestCliRequest(CliMixin):
           }, f)
     self._test_request(sr, True)
 
+  def test_request_validate_request_parameter(self):
+    self.conf.reference = 'instance reference'
+    self.conf.software_url = 'https://example.com/software.cfg'
+    self.conf.parameters = {'foo': 'bar'}
+    self.conf.parameters_file = None
+    self.conf.node = {'computer_guid': 'COMP-1234'}
+    self.conf.type = 'default'
+    self.conf.state = None
+    self.conf.slave = False
+    self.conf.force_serialisation = None
+    connection_dict = {'foo': 'bar'}
+
+    def _readAsJson(url, set_schema_id=False):
+      if url == 'https://example.com/software.cfg.json':
+        assert not set_schema_id
+        return {
+          "name": "Test Software",
+          "description": "Dummy software for Test",
+          "serialisation": "json-in-xml",
+          "software-type": {
+            "default": {
+              "title": "Default",
+              "description": "Default type",
+              "request": "instance-default-input-schema.json",
+              "response": "instance-default-output-schema.json",
+              "index": 0
+            },
+          }
+        }
+      if url == 'https://example.com/instance-default-input-schema.json':
+        assert set_schema_id
+        return {
+          "$id": url,
+          "$schema": "http://json-schema.org/draft-07/schema",
+          "description": "Simple instance parameters schema for tests",
+          "required": ["foo"],
+          "properties": {
+            "foo": {
+              "$ref": "./schemas-definitions.json#/foo",
+            }
+          },
+          "additionalProperties": False,
+          "type": "object",
+        }
+      if url == 'https://example.com/schemas-definitions.json':
+        assert not set_schema_id
+        return {
+          "foo": {
+            "type": "string",
+            "const": "bar",
+          },
+        }
+      assert False, "Unexpected url %s" % url
+
+    with mock.patch(
+        'slapos.util.SoftwareReleaseSchema._readAsJson',
+        side_effect=_readAsJson) as _readAsJson_mock, \
+        mock.patch.object(warnings, 'warn') as warn:
+      cp = slapos.slap.ComputerPartition('computer_id', 'partition_id')
+      cp._connection_helper = mock.Mock()
+      cp._connection_helper.POST.side_effect = slapos.slap.ResourceNotReady
+
+      self._do_request(connection_dict, False).assert_called_once_with(
+          software_release='https://example.com/software.cfg',
+          partition_reference='instance reference',
+          partition_parameter_kw={'foo': 'bar'},
+          software_type='default',
+          filter_kw={'computer_guid': 'COMP-1234'},
+          state=None,
+          shared=False,
+      )
+    self.assertEqual(
+      _readAsJson_mock.call_args_list,
+      [
+        mock.call('https://example.com/software.cfg.json'),
+        mock.call('https://example.com/instance-default-input-schema.json', True),
+        mock.call('https://example.com/schemas-definitions.json'),
+      ])
+    warn.assert_not_called()
+
+    self.conf.parameters = {'foo': 'baz'}
+    with mock.patch(
+        'slapos.util.SoftwareReleaseSchema._readAsJson',
+        side_effect=_readAsJson), \
+        mock.patch.object(warnings, 'warn') as warn:
+      cp = slapos.slap.ComputerPartition('computer_id', 'partition_id')
+      cp._connection_helper = mock.Mock()
+      cp._connection_helper.POST.side_effect = slapos.slap.ResourceNotReady
+      self._do_request(connection_dict, False).assert_called_once_with(
+          software_release='https://example.com/software.cfg',
+          partition_reference='instance reference',
+          partition_parameter_kw={'foo': 'baz'},
+          software_type='default',
+          filter_kw={'computer_guid': 'COMP-1234'},
+          state=None,
+          shared=False,
+      )
+    warn.assert_called_with(
+      "Request parameters do not validate against schema definition:\n"
+      "  $.foo: 'bar' was expected",
+      UserWarning
+    )
+
+    self.conf.parameters = {'fooo': 'xxx'}
+    with mock.patch(
+        'slapos.util.SoftwareReleaseSchema._readAsJson',
+        side_effect=_readAsJson), \
+        mock.patch.object(warnings, 'warn') as warn:
+      cp = slapos.slap.ComputerPartition('computer_id', 'partition_id')
+      cp._connection_helper = mock.Mock()
+      cp._connection_helper.POST.side_effect = slapos.slap.ResourceNotReady
+      self._do_request(connection_dict, False).assert_called_once_with(
+          software_release='https://example.com/software.cfg',
+          partition_reference='instance reference',
+          partition_parameter_kw={'fooo': 'xxx'},
+          software_type='default',
+          filter_kw={'computer_guid': 'COMP-1234'},
+          state=None,
+          shared=False,
+      )
+    warn.assert_called_with(
+      "Request parameters do not validate against schema definition:\n"
+      "  $: 'foo' is a required property\n"
+      "  $: Additional properties are not allowed ('fooo' was unexpected)",
+      UserWarning
+    )
+
+
+  def test_request_validate_request_parameter_broken_software_release_schema(self):
+    """Corner case tests for incorrect software release schema, these should
+    not prevent the request (mostly for backward compatibility)
+    """
+    self.conf.reference = 'reference'
+    self.conf.software_url = 'https://example.com/software.cfg'
+    self.conf.parameters = {'foo': 'bar'}
+    self.conf.parameters_file = None
+    self.conf.node = {'computer_guid': 'COMP-1234'}
+    self.conf.type = 'default'
+    self.conf.state = None
+    self.conf.slave = False
+    self.conf.force_serialisation = None
+    connection_dict = {'foo': 'bar'}
+
+    def wrong_software_cfg_schema(url, req):
+      if url.path.endswith('/software.cfg.json'):
+        return "wrong"
+      raise ValueError(404)
+
+    def wrong_instance_parameter_schema(url, req):
+      if url.path.endswith('/software.cfg.json'):
+        return json.dumps(
+          {
+              "name": "Test Software",
+              "description": "Dummy software for Test",
+              "serialisation": "json-in-xml",
+              "software-type": {
+                  'default': {
+                      "title": "Default",
+                      "description": "Default type",
+                      "request": "instance-default-input-schema.json",
+                      "response": "instance-default-output-schema.json",
+                      "index": 0
+                  },
+              }
+          })
+      if url.path.endswith('/instance-default-input-schema.json'):
+        return "wrong"
+      raise ValueError(404)
+
+    def invalid_instance_parameter_schema(url, req):
+      if url.path.endswith('/software.cfg.json'):
+        return json.dumps(
+          {
+              "name": "Test Software",
+              "description": "Dummy software for Test",
+              "serialisation": "json-in-xml",
+              "software-type": {
+                  'default': {
+                      "title": "Default",
+                      "description": "Default type",
+                      "request": "instance-default-input-schema.json",
+                      "response": "instance-default-output-schema.json",
+                      "index": 0
+                  },
+              }
+          })
+      if url.path.endswith('/instance-default-input-schema.json'):
+        return json.dumps(
+            {
+                "$schema": "http://json-schema.org/draft-07/schema",
+                "description": "Invalid json schema",
+                "required": {"wrong": True},
+                "properties": {
+                    ["wrong schema"]
+                },
+                "type": "object"
+            })
+      raise ValueError(404)
+
+    def broken_reference(url, req):
+      if url.path.endswith('/software.cfg.json'):
+        return json.dumps(
+          {
+              "name": "Test Software",
+              "description": "Dummy software for Test",
+              "serialisation": "json-in-xml",
+              "software-type": {
+                  'default': {
+                      "title": "Default",
+                      "description": "Default type",
+                      "request": "instance-default-input-schema.json",
+                      "response": "instance-default-output-schema.json",
+                      "index": 0
+                  },
+              }
+          })
+      if url.path.endswith('/instance-default-input-schema.json'):
+        return json.dumps(
+            {
+                "$schema": "http://json-schema.org/draft-07/schema",
+                "description": "Simple instance parameters schema for tests",
+                "required": ["foo"],
+                "properties": {
+                    "foo": {
+                        "$ref": "broken"
+                    }
+                },
+                "type": "object"
+            })
+      raise ValueError(404)
+
+    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:
+          cp = slapos.slap.ComputerPartition('computer_id', 'partition_id')
+          cp._connection_helper = mock.Mock()
+          cp._connection_helper.POST.side_effect = slapos.slap.ResourceNotReady
+          self._do_request(connection_dict, False)
+        warn.assert_called()
+
 
 class TestCliRequestParameterFile(CliMixin):
   """Request with --parameter-file, with a .json file.

slapos/tests/test_slap.py

@@ -873,211 +873,6 @@ class TestComputerPartition(SlapMixin):
         content_list = f.read().splitlines()
         self.assertEqual(sorted(content_list), ['myref', 'mysecondref'])
 
-  def test_request_validate_request_parameter(self):
-
-    def _readAsJson(url, set_schema_id=False):
-      if url == 'https://example.com/software.cfg.json':
-        assert not set_schema_id
-        return {
-          "name": "Test Software",
-          "description": "Dummy software for Test",
-          "serialisation": "json-in-xml",
-          "software-type": {
-            "default": {
-              "title": "Default",
-              "description": "Default type",
-              "request": "instance-default-input-schema.json",
-              "response": "instance-default-output-schema.json",
-              "index": 0
-            },
-          }
-        }
-      if url == 'https://example.com/instance-default-input-schema.json':
-        assert set_schema_id
-        return {
-          "$id": url,
-          "$schema": "http://json-schema.org/draft-07/schema",
-          "description": "Simple instance parameters schema for tests",
-          "required": ["foo"],
-          "properties": {
-            "foo": {
-              "$ref": "./schemas-definitions.json#/foo",
-            }
-          },
-          "additionalProperties": False,
-          "type": "object",
-        }
-      if url == 'https://example.com/schemas-definitions.json':
-        assert not set_schema_id
-        return {
-          "foo": {
-            "type": "string",
-            "const": "bar",
-          },
-        }
-      assert False, "Unexpected url %s" % url
-
-    with mock.patch(
-        'slapos.util.SoftwareReleaseSchema._readAsJson',
-        side_effect=_readAsJson) as _readAsJson_mock, \
-        mock.patch.object(warnings, 'warn') as warn:
-      cp = slapos.slap.ComputerPartition('computer_id', 'partition_id')
-      cp._connection_helper = mock.Mock()
-      cp._connection_helper.POST.side_effect = slapos.slap.ResourceNotReady
-      cp.request(
-          'https://example.com/software.cfg', 'default', 'reference',
-          partition_parameter_kw={'foo': 'bar'})
-    self.assertEqual(
-      _readAsJson_mock.call_args_list,
-      [
-        mock.call('https://example.com/software.cfg.json'),
-        mock.call('https://example.com/instance-default-input-schema.json', True),
-        mock.call('https://example.com/schemas-definitions.json'),
-      ])
-    warn.assert_not_called()
-
-    with mock.patch(
-        'slapos.util.SoftwareReleaseSchema._readAsJson',
-        side_effect=_readAsJson), \
-        mock.patch.object(warnings, 'warn') as warn:
-      cp = slapos.slap.ComputerPartition('computer_id', 'partition_id')
-      cp._connection_helper = mock.Mock()
-      cp._connection_helper.POST.side_effect = slapos.slap.ResourceNotReady
-      cp.request(
-          'https://example.com/software.cfg', 'default', 'reference',
-          partition_parameter_kw={'foo': 'baz'})
-    warn.assert_called_with(
-      "Request parameters do not validate against schema definition:\n"
-      "  $.foo: 'bar' was expected",
-      UserWarning
-    )
-
-    with mock.patch(
-        'slapos.util.SoftwareReleaseSchema._readAsJson',
-        side_effect=_readAsJson), \
-        mock.patch.object(warnings, 'warn') as warn:
-      cp = slapos.slap.ComputerPartition('computer_id', 'partition_id')
-      cp._connection_helper = mock.Mock()
-      cp._connection_helper.POST.side_effect = slapos.slap.ResourceNotReady
-      cp.request(
-          'https://example.com/software.cfg', 'default', 'reference',
-          partition_parameter_kw={'fooo': 'xxx'})
-    warn.assert_called_with(
-      "Request parameters do not validate against schema definition:\n"
-      "  $: 'foo' is a required property\n"
-      "  $: Additional properties are not allowed ('fooo' was unexpected)",
-      UserWarning
-    )
-
-
-  def test_request_validate_request_parameter_broken_software_release_schema(self):
-    """Corner case tests for incorrect software release schema, these should
-    not prevent the request (mostly for backward compatibility)
-    """
-    def wrong_software_cfg_schema(url, req):
-      if url.path.endswith('/software.cfg.json'):
-        return "wrong"
-      raise ValueError(404)
-
-    def wrong_instance_parameter_schema(url, req):
-      if url.path.endswith('/software.cfg.json'):
-        return json.dumps(
-          {
-              "name": "Test Software",
-              "description": "Dummy software for Test",
-              "serialisation": "json-in-xml",
-              "software-type": {
-                  'default': {
-                      "title": "Default",
-                      "description": "Default type",
-                      "request": "instance-default-input-schema.json",
-                      "response": "instance-default-output-schema.json",
-                      "index": 0
-                  },
-              }
-          })
-      if url.path.endswith('/instance-default-input-schema.json'):
-        return "wrong"
-      raise ValueError(404)
-
-    def invalid_instance_parameter_schema(url, req):
-      if url.path.endswith('/software.cfg.json'):
-        return json.dumps(
-          {
-              "name": "Test Software",
-              "description": "Dummy software for Test",
-              "serialisation": "json-in-xml",
-              "software-type": {
-                  'default': {
-                      "title": "Default",
-                      "description": "Default type",
-                      "request": "instance-default-input-schema.json",
-                      "response": "instance-default-output-schema.json",
-                      "index": 0
-                  },
-              }
-          })
-      if url.path.endswith('/instance-default-input-schema.json'):
-        return json.dumps(
-            {
-                "$schema": "http://json-schema.org/draft-07/schema",
-                "description": "Invalid json schema",
-                "required": {"wrong": True},
-                "properties": {
-                    ["wrong schema"]
-                },
-                "type": "object"
-            })
-      raise ValueError(404)
-
-    def broken_reference(url, req):
-      if url.path.endswith('/software.cfg.json'):
-        return json.dumps(
-          {
-              "name": "Test Software",
-              "description": "Dummy software for Test",
-              "serialisation": "json-in-xml",
-              "software-type": {
-                  'default': {
-                      "title": "Default",
-                      "description": "Default type",
-                      "request": "instance-default-input-schema.json",
-                      "response": "instance-default-output-schema.json",
-                      "index": 0
-                  },
-              }
-          })
-      if url.path.endswith('/instance-default-input-schema.json'):
-        return json.dumps(
-            {
-                "$schema": "http://json-schema.org/draft-07/schema",
-                "description": "Simple instance parameters schema for tests",
-                "required": ["foo"],
-                "properties": {
-                    "foo": {
-                        "$ref": "broken"
-                    }
-                },
-                "type": "object"
-            })
-      raise ValueError(404)
-
-    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:
-          cp = slapos.slap.ComputerPartition('computer_id', 'partition_id')
-          cp._connection_helper = mock.Mock()
-          cp._connection_helper.POST.side_effect = slapos.slap.ResourceNotReady
-          cp.request(
-              'https://example.com/software.cfg', 'default', 'reference',
-              partition_parameter_kw={'foo': 'bar'})
-        warn.assert_called()
-
   def _test_new_computer_partition_state(self, state):
     """
     Helper method to automate assertions of failing states on new Computer