Allow "version" to be within workflow names in workbooks
There were two bugs in the schema validation for Mistral workflow names within Workbooks. The goal in this part of the validation is to detect the version property and assign it the enum type and then assin all other single word properties to use the workflow/action schemas. 1. Detecting version properties. The code before this patch has the following line. "version": {"enum": ["2.0", 2.0]}, This looked safe, but "version" is actually a regular expression. Meaning that it will match any string containging "version". Adding ^ at the start and $ at the end means that it matches a string from the start to the end and is exactly "version". 2. Detecting workflows/actions. The current regular expression was "^(?!version)\w+$": _action_schema This worked in most cases. It basically means "Any word that doesn't start with version". So this worked, but was incorrect for workflows called "versionworkflow" - it didn't match this when it should have. Using ?: to create a non-capturing portion of the regular expression solved this issue. Any workflows that have a name that doesn't match this regular expression. Such as those with a space (like "my worflow") were silently ignored before this patch. The addition of additionalProperties: False in the schema shows users an error if they use an invalid workflow name. Closes-Bug: #1645354 Change-Id: Ib2b406a05cf15b41be075f886de77509a9da8535
This commit is contained in:
parent
44c32994c4
commit
ba67b3aff8
@ -0,0 +1,43 @@
|
|||||||
|
version: '2.0'
|
||||||
|
|
||||||
|
name: workbook_schema_test
|
||||||
|
description: >
|
||||||
|
This is a test workbook to verify workbook the schema validation.
|
||||||
|
Specifically we want to test the validation of workflow names.
|
||||||
|
See bug #1645354 for more details.
|
||||||
|
|
||||||
|
actions:
|
||||||
|
actionversion:
|
||||||
|
base: std.noop
|
||||||
|
|
||||||
|
versionaction:
|
||||||
|
base: std.noop
|
||||||
|
|
||||||
|
actionversionaction:
|
||||||
|
base: std.noop
|
||||||
|
|
||||||
|
workflows:
|
||||||
|
|
||||||
|
workflowversion:
|
||||||
|
description: Workflow name ending with version
|
||||||
|
tasks:
|
||||||
|
task1:
|
||||||
|
action: actionversion
|
||||||
|
|
||||||
|
versionworkflow:
|
||||||
|
description: Workflow name starting with version
|
||||||
|
tasks:
|
||||||
|
task1:
|
||||||
|
action: versionaction
|
||||||
|
|
||||||
|
workflowversionworkflow:
|
||||||
|
description: Workflow name with version in the middle
|
||||||
|
tasks:
|
||||||
|
task1:
|
||||||
|
action: actionversionaction
|
||||||
|
|
||||||
|
version_workflow:
|
||||||
|
description: Workflow name starting with version and an underscore
|
||||||
|
tasks:
|
||||||
|
task1:
|
||||||
|
workflow: workflowversion
|
@ -14,11 +14,13 @@
|
|||||||
# limitations under the License.
|
# limitations under the License.
|
||||||
|
|
||||||
import copy
|
import copy
|
||||||
|
import re
|
||||||
|
|
||||||
import yaml
|
import yaml
|
||||||
|
|
||||||
from mistral import exceptions as exc
|
from mistral import exceptions as exc
|
||||||
from mistral.tests.unit.workbook.v2 import base
|
from mistral.tests.unit.workbook.v2 import base
|
||||||
|
from mistral.workbook.v2 import workbook
|
||||||
|
|
||||||
|
|
||||||
class WorkbookSpecValidation(base.WorkbookSpecValidationTestCase):
|
class WorkbookSpecValidation(base.WorkbookSpecValidationTestCase):
|
||||||
@ -379,3 +381,52 @@ class WorkbookSpecValidation(base.WorkbookSpecValidationTestCase):
|
|||||||
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):
|
||||||
|
wb_spec = self._parse_dsl_spec(dsl_file='workbook_schema_test.yaml')
|
||||||
|
|
||||||
|
d = wb_spec.to_dict()
|
||||||
|
|
||||||
|
self.assertEqual('2.0', d['version'])
|
||||||
|
self.assertEqual('2.0', d['workflows']['version'])
|
||||||
|
|
||||||
|
workflow_names = ['workflowversion', 'versionworkflow',
|
||||||
|
'workflowversionworkflow', 'version_workflow']
|
||||||
|
|
||||||
|
action_names = ['actionversion', 'versionaction',
|
||||||
|
'actionversionaction']
|
||||||
|
|
||||||
|
for name in workflow_names:
|
||||||
|
self.assertEqual('2.0', d['workflows'][name]['version'])
|
||||||
|
self.assertEqual(name, d['workflows'][name]['name'])
|
||||||
|
|
||||||
|
for name in action_names:
|
||||||
|
self.assertEqual('2.0', d['actions'][name]['version'])
|
||||||
|
self.assertEqual(name, d['actions'][name]['name'])
|
||||||
|
|
||||||
|
def test_name_regex(self):
|
||||||
|
|
||||||
|
# We want to match a string containing version at any point.
|
||||||
|
valid_names = (
|
||||||
|
"workflowversion",
|
||||||
|
"versionworkflow",
|
||||||
|
"workflowversionworkflow",
|
||||||
|
"version_workflow",
|
||||||
|
)
|
||||||
|
|
||||||
|
for valid in valid_names:
|
||||||
|
result = re.match(workbook.NON_VERSION_WORD_REGEX, valid)
|
||||||
|
self.assertNotEqual(None, result,
|
||||||
|
"Expected match for: {}".format(valid))
|
||||||
|
|
||||||
|
# ... except, we don't want to match a string that isn't just one word
|
||||||
|
# or is exactly "version"
|
||||||
|
invalid_names = (
|
||||||
|
"version",
|
||||||
|
"my workflow",
|
||||||
|
)
|
||||||
|
|
||||||
|
for invalid in invalid_names:
|
||||||
|
result = re.match(workbook.NON_VERSION_WORD_REGEX, invalid)
|
||||||
|
self.assertEqual(None, result,
|
||||||
|
"Didn't expected match for: {}".format(invalid))
|
||||||
|
@ -17,6 +17,9 @@ from mistral.workbook.v2 import actions as act
|
|||||||
from mistral.workbook.v2 import base
|
from mistral.workbook.v2 import base
|
||||||
from mistral.workbook.v2 import workflows as wf
|
from mistral.workbook.v2 import workflows as wf
|
||||||
|
|
||||||
|
# We want to match any single word that isn't exactly "version"
|
||||||
|
NON_VERSION_WORD_REGEX = "^(?!version$)\w+$"
|
||||||
|
|
||||||
|
|
||||||
class WorkbookSpec(base.BaseSpec):
|
class WorkbookSpec(base.BaseSpec):
|
||||||
# See http://json-schema.org
|
# See http://json-schema.org
|
||||||
@ -33,17 +36,19 @@ class WorkbookSpec(base.BaseSpec):
|
|||||||
"type": "object",
|
"type": "object",
|
||||||
"minProperties": 1,
|
"minProperties": 1,
|
||||||
"patternProperties": {
|
"patternProperties": {
|
||||||
"version": {"enum": ["2.0", 2.0]},
|
"^version$": {"enum": ["2.0", 2.0]},
|
||||||
"^(?!version)\w+$": _action_schema
|
NON_VERSION_WORD_REGEX: _action_schema
|
||||||
}
|
},
|
||||||
|
"additionalProperties": False
|
||||||
},
|
},
|
||||||
"workflows": {
|
"workflows": {
|
||||||
"type": "object",
|
"type": "object",
|
||||||
"minProperties": 1,
|
"minProperties": 1,
|
||||||
"patternProperties": {
|
"patternProperties": {
|
||||||
"version": {"enum": ["2.0", 2.0]},
|
"^version$": {"enum": ["2.0", 2.0]},
|
||||||
"^(?!version)\w+$": _workflow_schema
|
NON_VERSION_WORD_REGEX: _workflow_schema
|
||||||
}
|
},
|
||||||
|
"additionalProperties": False
|
||||||
}
|
}
|
||||||
},
|
},
|
||||||
"additionalProperties": False
|
"additionalProperties": False
|
||||||
|
Loading…
Reference in New Issue
Block a user