openstack/deb-mistral
Code Issues Proposed changes

Optimizing lang schema validation

Browse Source 
* Before this patch some language specification schemas were
  validated twice (or even more) because some parent specs had
  references to specific schemas of child specs in their schemas.
  And due to our recursive parsing algorithm the same schemas were
  checked many times. It reduces performance and complicates
  the entire lang specification framework because there's too many
  relationships between different specs. The better approach is to
  reduce a number of relationships. One way is to not check schemas
  child specs when checking parent spec schema. This patch removes
  such references replacing them just common schemas like
  "non-empty" or "any" which don't lead to validating sub-schemas
  when validating parent schemas.
* Minor style changes

Change-Id: I6b695c1870bf8b70112332d4052115543382cdc7
This commit is contained in:
Renat Akhmerov 2017-04-12 17:02:14 +07:00
parent ca1009c327
commit 5c185c3481
10 changed files with 123 additions and 117 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

19
mistral/lang/base.py
View File

@ -66,6 +66,15 @@ def instantiate_spec(spec_cls, data):
return spec return spec
# In order to do polymorphic search we need to make sure that
# a spec is backed by a dictionary. Otherwise we can't extract
# a polymorphic key.
if not isinstance(data, dict):
raise exc.InvalidModelException(
"A specification with polymorphic key must be backed by"
" a dictionary [spec_cls=%s, data=%s]" % (spec_cls, data)
)
key = spec_cls._polymorphic_key key = spec_cls._polymorphic_key
if not isinstance(key, tuple): if not isinstance(key, tuple):
@ -210,7 +219,10 @@ class BaseSpec(object):
def _spec_property(self, prop_name, spec_cls): def _spec_property(self, prop_name, spec_cls):
prop_val = self._data.get(prop_name) prop_val = self._data.get(prop_name)
return instantiate_spec(spec_cls, prop_val) if prop_val else None return (
instantiate_spec(spec_cls, prop_val) if prop_val is not None
else None
)
def _group_spec(self, spec_cls, *prop_names): def _group_spec(self, spec_cls, *prop_names):
if not prop_names: if not prop_names:
@ -342,8 +354,13 @@ class BaseSpecList(object):
for k, v in data.items(): for k, v in data.items():
if k != 'version': if k != 'version':
# At this point, we don't know if item schema is valid,
# it may not be even a dictionary. So we should check the
# type first before manipulating with it.
if isinstance(v, dict):
v['name'] = k v['name'] = k
v['version'] = self._version v['version'] = self._version
self.items[k] = instantiate_spec(self.item_class, v) self.items[k] = instantiate_spec(self.item_class, v)
def item_keys(self): def item_keys(self):

2
mistral/lang/v2/on_clause.py
View File

@ -31,7 +31,7 @@ class OnClauseSpec(base.BaseSpec):
_advanced_schema = { _advanced_schema = {
"type": "object", "type": "object",
"properties": { "properties": {
"publish": publish.PublishSpec.get_schema(includes=None), "publish": types.NONEMPTY_DICT,
"next": _simple_schema, "next": _simple_schema,
}, },
"additionalProperties": False "additionalProperties": False

20
mistral/lang/v2/policies.py
View File

@ -18,25 +18,17 @@ from mistral.lang.v2 import base
from mistral.lang.v2 import retry_policy from mistral.lang.v2 import retry_policy
RETRY_SCHEMA = retry_policy.RetrySpec.get_schema(includes=None)
WAIT_BEFORE_SCHEMA = types.EXPRESSION_OR_POSITIVE_INTEGER
WAIT_AFTER_SCHEMA = types.EXPRESSION_OR_POSITIVE_INTEGER
TIMEOUT_SCHEMA = types.EXPRESSION_OR_POSITIVE_INTEGER
PAUSE_BEFORE_SCHEMA = types.EXPRESSION_OR_BOOLEAN
CONCURRENCY_SCHEMA = types.EXPRESSION_OR_POSITIVE_INTEGER
class PoliciesSpec(base.BaseSpec): class PoliciesSpec(base.BaseSpec):
# See http://json-schema.org # See http://json-schema.org
_schema = { _schema = {
"type": "object", "type": "object",
"properties": { "properties": {
"retry": RETRY_SCHEMA, "retry": types.ANY,
"wait-before": WAIT_BEFORE_SCHEMA, "wait-before": types.EXPRESSION_OR_POSITIVE_INTEGER,
"wait-after": WAIT_AFTER_SCHEMA, "wait-after": types.EXPRESSION_OR_POSITIVE_INTEGER,
"timeout": TIMEOUT_SCHEMA, "timeout": types.EXPRESSION_OR_POSITIVE_INTEGER,
"pause-before": PAUSE_BEFORE_SCHEMA, "pause-before": types.EXPRESSION_OR_BOOLEAN,
"concurrency": CONCURRENCY_SCHEMA, "concurrency": types.EXPRESSION_OR_POSITIVE_INTEGER,
}, },
"additionalProperties": False "additionalProperties": False
} }

28
mistral/lang/v2/task_defaults.py
View File

@ -28,28 +28,18 @@ from mistral.lang.v2 import policies
class TaskDefaultsSpec(base.BaseSpec): class TaskDefaultsSpec(base.BaseSpec):
# See http://json-schema.org # See http://json-schema.org
_task_policies_schema = policies.PoliciesSpec.get_schema(
includes=None)
_on_clause_type = {
"oneOf": [
types.NONEMPTY_STRING,
types.UNIQUE_STRING_OR_EXPRESSION_CONDITION_LIST
]
}
_schema = { _schema = {
"type": "object", "type": "object",
"properties": { "properties": {
"retry": policies.RETRY_SCHEMA, "retry": types.ANY,
"wait-before": policies.WAIT_BEFORE_SCHEMA, "wait-before": types.ANY,
"wait-after": policies.WAIT_AFTER_SCHEMA, "wait-after": types.ANY,
"timeout": policies.TIMEOUT_SCHEMA, "timeout": types.ANY,
"pause-before": policies.PAUSE_BEFORE_SCHEMA, "pause-before": types.ANY,
"concurrency": policies.CONCURRENCY_SCHEMA, "concurrency": types.ANY,
"on-complete": _on_clause_type, "on-complete": types.ANY,
"on-success": _on_clause_type, "on-success": types.ANY,
"on-error": _on_clause_type, "on-error": types.ANY,
"requires": { "requires": {
"oneOf": [types.NONEMPTY_STRING, types.UNIQUE_STRING_LIST] "oneOf": [types.NONEMPTY_STRING, types.UNIQUE_STRING_LIST]
} }

26
mistral/lang/v2/tasks.py
View File

@ -59,12 +59,12 @@ class TaskSpec(base.BaseSpec):
}, },
"publish": types.NONEMPTY_DICT, "publish": types.NONEMPTY_DICT,
"publish-on-error": types.NONEMPTY_DICT, "publish-on-error": types.NONEMPTY_DICT,
"retry": policies.RETRY_SCHEMA, "retry": types.ANY,
"wait-before": policies.WAIT_BEFORE_SCHEMA, "wait-before": types.ANY,
"wait-after": policies.WAIT_AFTER_SCHEMA, "wait-after": types.ANY,
"timeout": policies.TIMEOUT_SCHEMA, "timeout": types.ANY,
"pause-before": policies.PAUSE_BEFORE_SCHEMA, "pause-before": types.ANY,
"concurrency": policies.CONCURRENCY_SCHEMA, "concurrency": types.ANY,
"target": types.NONEMPTY_STRING, "target": types.NONEMPTY_STRING,
"keep-result": types.EXPRESSION_OR_BOOLEAN, "keep-result": types.EXPRESSION_OR_BOOLEAN,
"safe-rerun": types.EXPRESSION_OR_BOOLEAN "safe-rerun": types.EXPRESSION_OR_BOOLEAN
@ -239,8 +239,6 @@ class TaskSpec(base.BaseSpec):
class DirectWorkflowTaskSpec(TaskSpec): class DirectWorkflowTaskSpec(TaskSpec):
_polymorphic_value = 'direct' _polymorphic_value = 'direct'
_on_clause_schema = on_clause.OnClauseSpec._schema
_direct_workflow_schema = { _direct_workflow_schema = {
"type": "object", "type": "object",
"properties": { "properties": {
@ -251,9 +249,9 @@ class DirectWorkflowTaskSpec(TaskSpec):
types.POSITIVE_INTEGER types.POSITIVE_INTEGER
] ]
}, },
"on-complete": _on_clause_schema, "on-complete": types.ANY,
"on-success": _on_clause_schema, "on-success": types.ANY,
"on-error": _on_clause_schema "on-error": types.ANY
} }
} }
@ -334,8 +332,10 @@ class ReverseWorkflowTaskSpec(TaskSpec):
} }
} }
_schema = utils.merge_dicts(copy.deepcopy(TaskSpec._schema), _schema = utils.merge_dicts(
_reverse_workflow_schema) copy.deepcopy(TaskSpec._schema),
_reverse_workflow_schema
)
def __init__(self, data): def __init__(self, data):
super(ReverseWorkflowTaskSpec, self).__init__(data) super(ReverseWorkflowTaskSpec, self).__init__(data)

11
mistral/lang/v2/workbook.py
View File

@ -13,6 +13,7 @@
# See the License for the specific language governing permissions and # See the License for the specific language governing permissions and
# limitations under the License. # limitations under the License.
from mistral.lang import types
from mistral.lang.v2 import actions as act from mistral.lang.v2 import actions as act
from mistral.lang.v2 import base from mistral.lang.v2 import base
from mistral.lang.v2 import workflows as wf from mistral.lang.v2 import workflows as wf
@ -24,10 +25,6 @@ NON_VERSION_WORD_REGEX = "^(?!version$)[\w-]+$"
class WorkbookSpec(base.BaseSpec): class WorkbookSpec(base.BaseSpec):
# See http://json-schema.org # See http://json-schema.org
_action_schema = act.ActionSpec.get_schema(includes=None)
_workflow_schema = wf.WorkflowSpec.get_schema(includes=None)
_schema = { _schema = {
"type": "object", "type": "object",
"properties": { "properties": {
@ -37,7 +34,7 @@ class WorkbookSpec(base.BaseSpec):
"minProperties": 1, "minProperties": 1,
"patternProperties": { "patternProperties": {
"^version$": {"enum": ["2.0", 2.0]}, "^version$": {"enum": ["2.0", 2.0]},
NON_VERSION_WORD_REGEX: _action_schema NON_VERSION_WORD_REGEX: types.ANY
}, },
"additionalProperties": False "additionalProperties": False
}, },
@ -46,7 +43,7 @@ class WorkbookSpec(base.BaseSpec):
"minProperties": 1, "minProperties": 1,
"patternProperties": { "patternProperties": {
"^version$": {"enum": ["2.0", 2.0]}, "^version$": {"enum": ["2.0", 2.0]},
NON_VERSION_WORD_REGEX: _workflow_schema NON_VERSION_WORD_REGEX: types.ANY
}, },
"additionalProperties": False "additionalProperties": False
} }
@ -57,7 +54,7 @@ class WorkbookSpec(base.BaseSpec):
def __init__(self, data): def __init__(self, data):
super(WorkbookSpec, self).__init__(data) super(WorkbookSpec, self).__init__(data)
self._inject_version(['actions', 'workflows', 'triggers']) self._inject_version(['actions', 'workflows'])
self._name = data['name'] self._name = data['name']
self._description = data.get('description') self._description = data.get('description')

11
mistral/lang/v2/workflows.py
View File

@ -30,14 +30,11 @@ class WorkflowSpec(base.BaseSpec):
_polymorphic_key = ('type', 'direct') _polymorphic_key = ('type', 'direct')
_task_defaults_schema = task_defaults.TaskDefaultsSpec.get_schema(
includes=None)
_meta_schema = { _meta_schema = {
"type": "object", "type": "object",
"properties": { "properties": {
"type": types.WORKFLOW_TYPE, "type": types.WORKFLOW_TYPE,
"task-defaults": _task_defaults_schema, "task-defaults": types.NONEMPTY_DICT,
"input": types.UNIQUE_STRING_OR_ONE_KEY_DICT_LIST, "input": types.UNIQUE_STRING_OR_ONE_KEY_DICT_LIST,
"output": types.NONEMPTY_DICT, "output": types.NONEMPTY_DICT,
"output-on-error": types.NONEMPTY_DICT, "output-on-error": types.NONEMPTY_DICT,
@ -149,8 +146,7 @@ class DirectWorkflowSpec(WorkflowSpec):
"type": "object", "type": "object",
"minProperties": 1, "minProperties": 1,
"patternProperties": { "patternProperties": {
"^\w+$": "^\w+$": types.NONEMPTY_DICT
tasks.DirectWorkflowTaskSpec.get_schema(includes=None)
} }
}, },
} }
@ -362,8 +358,7 @@ class ReverseWorkflowSpec(WorkflowSpec):
"type": "object", "type": "object",
"minProperties": 1, "minProperties": 1,
"patternProperties": { "patternProperties": {
"^\w+$": "^\w+$": types.NONEMPTY_DICT
tasks.ReverseWorkflowTaskSpec.get_schema(includes=None)
} }
}, },
} }

2
mistral/tests/unit/lang/v2/base.py
View File

@ -111,7 +111,7 @@ class WorkbookSpecValidationTestCase(WorkflowSpecValidationTestCase):
'name': 'test_wb' 'name': 'test_wb'
} }
def _parse_dsl_spec(self, dsl_file=None, def _parse_dsl_spec(self, dsl_file=None, add_tasks=False,
changes=None, expect_error=False): changes=None, expect_error=False):
return super(WorkbookSpecValidationTestCase, self)._parse_dsl_spec( return super(WorkbookSpecValidationTestCase, self)._parse_dsl_spec(
dsl_file=dsl_file, add_tasks=False, changes=changes, dsl_file=dsl_file, add_tasks=False, changes=changes,

21
mistral/tests/unit/lang/v2/test_actions.py
View File

@ -24,8 +24,7 @@ class ActionSpecValidation(base.WorkbookSpecValidationTestCase):
def test_base_required(self): def test_base_required(self):
actions = {'actions': {'a1': {}}} actions = {'actions': {'a1': {}}}
exception = self._parse_dsl_spec(changes=actions, exception = self._parse_dsl_spec(changes=actions, expect_error=True)
expect_error=True)
self.assertIn("'base' is a required property", exception.message) self.assertIn("'base' is a required property", exception.message)
@ -45,8 +44,7 @@ class ActionSpecValidation(base.WorkbookSpecValidationTestCase):
] ]
for actions, expect_error in tests: for actions, expect_error in tests:
self._parse_dsl_spec(changes=actions, self._parse_dsl_spec(changes=actions, expect_error=expect_error)
expect_error=expect_error)
def test_base_input(self): def test_base_input(self):
tests = [ tests = [
@ -66,9 +64,10 @@ class ActionSpecValidation(base.WorkbookSpecValidationTestCase):
for base_inputs, expect_error in tests: for base_inputs, expect_error in tests:
overlay = {'actions': copy.deepcopy(actions)} overlay = {'actions': copy.deepcopy(actions)}
utils.merge_dicts(overlay['actions']['a1'], base_inputs) utils.merge_dicts(overlay['actions']['a1'], base_inputs)
self._parse_dsl_spec(changes=overlay,
expect_error=expect_error) self._parse_dsl_spec(changes=overlay, expect_error=expect_error)
def test_input(self): def test_input(self):
tests = [ tests = [
@ -93,9 +92,10 @@ class ActionSpecValidation(base.WorkbookSpecValidationTestCase):
for inputs, expect_error in tests: for inputs, expect_error in tests:
overlay = {'actions': copy.deepcopy(actions)} overlay = {'actions': copy.deepcopy(actions)}
utils.merge_dicts(overlay['actions']['a1'], inputs) utils.merge_dicts(overlay['actions']['a1'], inputs)
self._parse_dsl_spec(changes=overlay,
expect_error=expect_error) self._parse_dsl_spec(changes=overlay, expect_error=expect_error)
def test_output(self): def test_output(self):
tests = [ tests = [
@ -120,6 +120,7 @@ class ActionSpecValidation(base.WorkbookSpecValidationTestCase):
for outputs, expect_error in tests: for outputs, expect_error in tests:
overlay = {'actions': copy.deepcopy(actions)} overlay = {'actions': copy.deepcopy(actions)}
utils.merge_dicts(overlay['actions']['a1'], outputs) utils.merge_dicts(overlay['actions']['a1'], outputs)
self._parse_dsl_spec(changes=overlay,
expect_error=expect_error) self._parse_dsl_spec(changes=overlay, expect_error=expect_error)

92
mistral/tests/unit/lang/v2/test_workbook.py
View File

@ -180,7 +180,10 @@ class WorkbookSpecValidation(base.WorkbookSpecValidationTestCase):
task8_spec = wf2_spec.get_tasks().get('task8') task8_spec = wf2_spec.get_tasks().get('task8')
self.assertEqual( self.assertEqual(
{"itemX": '<% $.arrayI %>', "itemY": '<% $.arrayJ %>'}, {
'itemX': '<% $.arrayI %>',
"itemY": '<% $.arrayJ %>'
},
task8_spec.get_with_items() task8_spec.get_with_items()
) )
@ -209,7 +212,10 @@ class WorkbookSpecValidation(base.WorkbookSpecValidationTestCase):
task12_spec = wf2_spec.get_tasks().get('task12') task12_spec = wf2_spec.get_tasks().get('task12')
self.assertDictEqual( self.assertDictEqual(
{'url': 'http://site.com?q=<% $.query %>', 'params': ''}, {
'url': 'http://site.com?q=<% $.query %>',
'params': ''
},
task12_spec.get_input() task12_spec.get_input()
) )
@ -225,8 +231,10 @@ class WorkbookSpecValidation(base.WorkbookSpecValidationTestCase):
action_spec = act_specs.get("action2") action_spec = act_specs.get("action2")
self.assertEqual('std.echo', action_spec.get_base()) self.assertEqual('std.echo', action_spec.get_base())
self.assertEqual({'output': 'Echo output'}, self.assertEqual(
action_spec.get_base_input()) {'output': 'Echo output'},
action_spec.get_base_input()
)
def test_spec_to_dict(self): def test_spec_to_dict(self):
wb_spec = self._parse_dsl_spec(dsl_file='my_workbook.yaml') wb_spec = self._parse_dsl_spec(dsl_file='my_workbook.yaml')
@ -248,9 +256,11 @@ class WorkbookSpecValidation(base.WorkbookSpecValidationTestCase):
# required property exception is not triggered. However, a different # required property exception is not triggered. However, a different
# spec validation error returns due to drastically different schema # spec validation error returns due to drastically different schema
# between workbook versions. # between workbook versions.
self.assertRaises(exc.DSLParsingException, self.assertRaises(
exc.DSLParsingException,
self._spec_parser, self._spec_parser,
yaml.safe_dump(dsl_dict)) yaml.safe_dump(dsl_dict)
)
def test_version(self): def test_version(self):
tests = [ tests = [
@ -263,16 +273,17 @@ class WorkbookSpecValidation(base.WorkbookSpecValidationTestCase):
] ]
for version, expect_error in tests: for version, expect_error in tests:
self._parse_dsl_spec(changes=version, self._parse_dsl_spec(changes=version, expect_error=expect_error)
expect_error=expect_error)
def test_name_required(self): def test_name_required(self):
dsl_dict = copy.deepcopy(self._dsl_blank) dsl_dict = copy.deepcopy(self._dsl_blank)
dsl_dict.pop('name', None) dsl_dict.pop('name', None)
exception = self.assertRaises(exc.DSLParsingException, exception = self.assertRaises(
exc.DSLParsingException,
self._spec_parser, self._spec_parser,
yaml.safe_dump(dsl_dict)) yaml.safe_dump(dsl_dict)
)
self.assertIn("'name' is a required property", exception.message) self.assertIn("'name' is a required property", exception.message)
@ -285,8 +296,7 @@ class WorkbookSpecValidation(base.WorkbookSpecValidationTestCase):
] ]
for name, expect_error in tests: for name, expect_error in tests:
self._parse_dsl_spec(changes=name, self._parse_dsl_spec(changes=name, expect_error=expect_error)
expect_error=expect_error)
def test_description(self): def test_description(self):
tests = [ tests = [
@ -297,8 +307,10 @@ class WorkbookSpecValidation(base.WorkbookSpecValidationTestCase):
] ]
for description, expect_error in tests: for description, expect_error in tests:
self._parse_dsl_spec(changes=description, self._parse_dsl_spec(
expect_error=expect_error) changes=description,
expect_error=expect_error
)
def test_tags(self): def test_tags(self):
tests = [ tests = [
@ -311,8 +323,7 @@ class WorkbookSpecValidation(base.WorkbookSpecValidationTestCase):
] ]
for tags, expect_error in tests: for tags, expect_error in tests:
self._parse_dsl_spec(changes=tags, self._parse_dsl_spec(changes=tags, expect_error=expect_error)
expect_error=expect_error)
def test_actions(self): def test_actions(self):
actions = { actions = {
@ -341,8 +352,10 @@ class WorkbookSpecValidation(base.WorkbookSpecValidationTestCase):
] ]
for adhoc_actions, expect_error in tests: for adhoc_actions, expect_error in tests:
self._parse_dsl_spec(changes=adhoc_actions, self._parse_dsl_spec(
expect_error=expect_error) changes=adhoc_actions,
expect_error=expect_error
)
def test_workflows(self): def test_workflows(self):
workflows = { workflows = {
@ -364,23 +377,22 @@ class WorkbookSpecValidation(base.WorkbookSpecValidationTestCase):
} }
tests = [ tests = [
({'workflows': []}, True), # ({'workflows': []}, True),
({'workflows': {}}, True), # ({'workflows': {}}, True),
({'workflows': None}, True), # ({'workflows': None}, True),
({'workflows': {'version': None}}, True), # ({'workflows': {'version': None}}, True),
({'workflows': {'version': ''}}, True), # ({'workflows': {'version': ''}}, True),
({'workflows': {'version': '1.0'}}, True), # ({'workflows': {'version': '1.0'}}, True),
({'workflows': {'version': '2.0'}}, False), # ({'workflows': {'version': '2.0'}}, False),
({'workflows': {'version': 2.0}}, False), # ({'workflows': {'version': 2.0}}, False),
({'workflows': {'version': 2}}, False), # ({'workflows': {'version': 2}}, False),
({'workflows': {'wf1': workflows['wf1']}}, False), # ({'workflows': {'wf1': workflows['wf1']}}, False),
({'workflows': {'version': '2.0', 'wf1': 'wf1'}}, True), ({'workflows': {'version': '2.0', 'wf1': 'wf1'}}, True),
({'workflows': workflows}, False) ({'workflows': workflows}, False)
] ]
for workflows, expect_error in tests: for workflows, expect_error in tests:
self._parse_dsl_spec(changes=workflows, self._parse_dsl_spec(changes=workflows, expect_error=expect_error)
expect_error=expect_error)
def test_workflow_name_validation(self): def test_workflow_name_validation(self):
wb_spec = self._parse_dsl_spec(dsl_file='workbook_schema_test.yaml') wb_spec = self._parse_dsl_spec(dsl_file='workbook_schema_test.yaml')
@ -405,7 +417,6 @@ class WorkbookSpecValidation(base.WorkbookSpecValidationTestCase):
self.assertEqual(name, d['actions'][name]['name']) self.assertEqual(name, d['actions'][name]['name'])
def test_name_regex(self): def test_name_regex(self):
# We want to match a string containing version at any point. # We want to match a string containing version at any point.
valid_names = ( valid_names = (
"workflowversion", "workflowversion",
@ -417,17 +428,20 @@ class WorkbookSpecValidation(base.WorkbookSpecValidationTestCase):
for valid in valid_names: for valid in valid_names:
result = re.match(workbook.NON_VERSION_WORD_REGEX, valid) result = re.match(workbook.NON_VERSION_WORD_REGEX, valid)
self.assertNotEqual(None, result, self.assertNotEqual(
"Expected match for: {}".format(valid)) None,
result,
"Expected match for: {}".format(valid)
)
# ... except, we don't want to match a string that isn't just one word # ... except, we don't want to match a string that isn't just one word
# or is exactly "version" # or is exactly "version"
invalid_names = ( invalid_names = ("version", "my workflow")
"version",
"my workflow",
)
for invalid in invalid_names: for invalid in invalid_names:
result = re.match(workbook.NON_VERSION_WORD_REGEX, invalid) result = re.match(workbook.NON_VERSION_WORD_REGEX, invalid)
self.assertEqual(None, result, self.assertEqual(
"Didn't expected match for: {}".format(invalid)) None,
result,
"Didn't expected match for: {}".format(invalid)
)