feat: add validation that associative op's args are equal length (#96)

Problem:

The Open Job Description specification says "Every comma-separated expression
within an associative operator must have the exact same number of values defined
in their range." The current model validators to not ensure that this is the case.

Reference: https://github.com/OpenJobDescription/openjd-specifications/wiki/2023-09-Template-Schemas#343-combinationexpr

Solution:

I've implemented validate_step_parameter_space_dimensions() to validate this constraint
and added a validator to the StepParameterSpace to use it. My first inclination was
to implement this validation as part of constructing the StepParameterSpaceIterator
since it already has a recursive expression parse, but that is a chicken & egg problem;
we need a StepParameterSpace to construct a StepParameterSpaceIterator, but we don't
have one yet when doing validation for the construction of a StepParameterSpace.

Signed-off-by: Daniel Neilson <[email protected]>

Author: ddneilson

src/openjd/model/_internal/__init__.py

@@ -8,11 +8,13 @@
 from ._combination_expr import Parser as CombinationExpressionParser
 from ._combination_expr import ProductNode as CombinationExpressionProductNode
 from ._create_job import instantiate_model
+from ._param_space_dim_validation import validate_step_parameter_space_dimensions
 from ._variable_reference_validation import prevalidate_model_template_variable_references
 
 __all__ = (
     "instantiate_model",
     "prevalidate_model_template_variable_references",
+    "validate_step_parameter_space_dimensions",
     "validate_unique_elements",
     "CombinationExpressionAssociationNode",
     "CombinationExpressionIdentifierNode",

src/openjd/model/_internal/_param_space_dim_validation.py

@@ -0,0 +1,53 @@
+# Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
+
+from functools import reduce
+from operator import mul
+from ._combination_expr import AssociationNode, IdentifierNode, Node, Parser, ProductNode
+from .._errors import ExpressionError
+
+
+def validate_step_parameter_space_dimensions(
+    parameter_range_lengths: dict[str, int], combination: str
+) -> None:
+    """This validates that a CombinationExpr satisfies constraints placed
+    on the ranges of the elements of the expression.
+    Specifically that the arguments to an Associative operator all have
+    the exact same number of elements.
+
+    Args:
+      - parameter_range_lengths: dict[str,int] -- A map from identifier name
+          to the number of elements in that parameter's range.
+      - combination: str -- The combination expression.
+
+    Raises:
+       ExpressionError if the combination expression violates constraints.
+    """
+    parse_tree = Parser().parse(combination)
+    _validate_expr_tree(parse_tree, parameter_range_lengths)
+
+
+def _validate_expr_tree(root: Node, parameter_range_lengths: dict[str, int]) -> int:
+    # Returns the length of the subtree while recursively validating it.
+    if isinstance(root, IdentifierNode):
+        name = root.parameter
+        return parameter_range_lengths[name]
+    elif isinstance(root, AssociationNode):
+        # Association requires that all arguments are the exact same length.
+        # Ensure that is the case
+        arg_lengths = tuple(
+            _validate_expr_tree(child, parameter_range_lengths) for child in root.children
+        )
+        if len(set(arg_lengths)) > 1:
+            raise ExpressionError(
+                (
+                    "Associative expressions must have arguments with identical ranges. "
+                    "Expression %s has argument lengths %s." % (str(root), arg_lengths)
+                )
+            )
+        return arg_lengths[0]
+    else:
+        # For type hinting
+        assert isinstance(root, ProductNode)
+        return reduce(
+            mul, (_validate_expr_tree(child, parameter_range_lengths) for child in root.children), 1
+        )

src/openjd/model/v2023_09/_model.py

@@ -32,6 +32,7 @@
 )
 from .._internal import (
     CombinationExpressionParser,
+    validate_step_parameter_space_dimensions,
     validate_unique_elements,
 )
 from .._internal._variable_reference_validation import (
@@ -729,6 +730,25 @@ class StepParameterSpace(OpenJDModel_v2023_09):
     taskParameterDefinitions: dict[Identifier, TaskRangeParameter]
     combination: Optional[CombinationExpr] = None
 
+    @validator("combination")
+    def _validate_parameter_space(cls, v: str, values: dict[str, Any]) -> str:
+        if v is None:
+            return v
+        param_defs = cast(dict[Identifier, TaskRangeParameter], values["taskParameterDefinitions"])
+        parameter_range_lengths = {
+            id: (
+                len(param.range)
+                if isinstance(param.range, list)
+                else len(IntRangeExpr.from_str(param.range))
+            )
+            for id, param in param_defs.items()
+        }
+        try:
+            validate_step_parameter_space_dimensions(parameter_range_lengths, v)
+        except ExpressionError as e:
+            raise ValueError(str(e)) from None
+        return v
+
 
 class StepParameterSpaceDefinition(OpenJDModel_v2023_09):
     """Definition of a Step's parameter space. The parameter space is the multidimensional

test/openjd/model/_internal/test_param_space_dim_validation.py

@@ -0,0 +1,92 @@
+# Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
+
+import pytest
+import re
+
+from openjd.model._internal import validate_step_parameter_space_dimensions
+from openjd.model import ExpressionError
+
+
+@pytest.mark.parametrize(
+    "param_ranges, combination",
+    [
+        pytest.param({"A": 5}, "A", id="Identifier"),
+        pytest.param({"A": 5, "B": 10}, "A * B", id="Product"),
+        pytest.param({"A": 5, "B": 5}, "(A,B)", id="Association"),
+        pytest.param({"A": 5, "B": 5, "C": 10}, "(A,B)*C", id="Association-Product"),
+        pytest.param({"A": 5, "B": 5, "C": 10}, "C*(A,B)", id="Product-Association"),
+        pytest.param({"A": 5, "B": 5, "C": 1}, "(A*C,B)", id="Nested product 2"),
+        pytest.param({"A": 5, "B": 5, "C": 1}, "(A,B*C)", id="Nested product 2"),
+        pytest.param({"A": 5, "B": 5, "C": 5}, "((A,C),B)", id="Nested association 1"),
+        pytest.param({"A": 5, "B": 5, "C": 5}, "(A,(B,C))", id="Nested association 2"),
+    ],
+)
+def test_allowable(param_ranges: dict[str, int], combination: str) -> None:
+    # Test that perfectly valid parameter spaces do not raise exceptions
+
+    # THEN
+    validate_step_parameter_space_dimensions(param_ranges, combination)
+
+
+@pytest.mark.parametrize(
+    "param_ranges, combination, expected_exception",
+    [
+        pytest.param(
+            {"A": 5, "B": 10},
+            "(A,B)",
+            "Associative expressions must have arguments with identical ranges. Expression (A, B) has argument lengths (5, 10).",
+            id="2-arg",
+        ),
+        pytest.param(
+            {"A": 5, "B": 10, "C": 10},
+            "(A,B,C)",
+            "Associative expressions must have arguments with identical ranges. Expression (A, B, C) has argument lengths (5, 10, 10).",
+            id="3-arg; A",
+        ),
+        pytest.param(
+            {"A": 10, "B": 5, "C": 10},
+            "(A,B,C)",
+            "Associative expressions must have arguments with identical ranges. Expression (A, B, C) has argument lengths (10, 5, 10).",
+            id="3-arg; B",
+        ),
+        pytest.param(
+            {"A": 10, "B": 10, "C": 5},
+            "(A,B,C)",
+            "Associative expressions must have arguments with identical ranges. Expression (A, B, C) has argument lengths (10, 10, 5).",
+            id="3-arg; C",
+        ),
+        pytest.param(
+            {"A": 5, "B": 5, "C": 5},
+            "(A,B*C)",
+            "Associative expressions must have arguments with identical ranges. Expression (A, B * C) has argument lengths (5, 25).",
+            id="Nested product",
+        ),
+        pytest.param(
+            {"A": 5, "B": 10, "C": 10},
+            "(A,(B,C))",
+            "Associative expressions must have arguments with identical ranges. Expression (A, (B, C)) has argument lengths (5, 10).",
+            id="Nested Association",
+        ),
+        pytest.param(
+            {"A": 5, "B": 10, "C": 10},
+            "C * (A,B)",
+            "Associative expressions must have arguments with identical ranges. Expression (A, B) has argument lengths (5, 10).",
+            id="Recurse to association 1",
+        ),
+        pytest.param(
+            {"A": 5, "B": 10, "C": 10},
+            "(A,B) * C",
+            "Associative expressions must have arguments with identical ranges. Expression (A, B) has argument lengths (5, 10).",
+            id="Recurse to association 2",
+        ),
+    ],
+)
+def test_mismatched_association(
+    param_ranges: dict[str, int], combination: str, expected_exception: str
+) -> None:
+    # Test that expressions that contain associations with mismatched argument lengths
+    # all raise the expected exception.
+
+    # THEN
+    with pytest.raises(ExpressionError, match=re.escape(expected_exception)):
+        validate_step_parameter_space_dimensions(param_ranges, combination)

test/openjd/model/test_create_job.py

@@ -4,6 +4,7 @@
 import tempfile
 import pytest
 from pathlib import Path
+from typing import Any
 
 from openjd.model import (
     DecodeValidationError,
@@ -737,3 +738,49 @@ def test_fails_to_instantiate(self) -> None:
             "1 validation errors for JobTemplate\nname:\n\tensure this value has at most 128 characters"
             in str(excinfo.value)
         )
+
+    def test_uneven_parameter_space_association(self) -> None:
+        # Test that when the arguments to an Association operator in a
+        # parameter space combination expression have differing lengths then
+        # we raise an appropriate exception.
+        #
+        # Note: This validation is run in the create job flow because we need
+        # to have a fully instantiated the step parameter space's task parameter
+        # definitions to know how large each parameter range is.
+
+        # GIVEN
+        job_template = _parse_model(
+            model=JobTemplate_2023_09,
+            obj={
+                "specificationVersion": "jobtemplate-2023-09",
+                "name": "Job",
+                "steps": [
+                    {
+                        "name": "Step",
+                        "parameterSpace": {
+                            "taskParameterDefinitions": [
+                                {"name": "A", "type": "INT", "range": "1-10"},
+                                {"name": "B", "type": "INT", "range": [1, 2]},
+                            ],
+                            "combination": "(A,B)",
+                        },
+                        "script": {"actions": {"onRun": {"command": "do something"}}},
+                    }
+                ],
+            },
+        )
+        parameter_values = dict[str, Any]()
+
+        # WHEN
+        with pytest.raises(DecodeValidationError) as excinfo:
+            # This'll have an error when instantiating the Job due to the Job's name being too long.
+            create_job(
+                job_template=job_template,
+                job_parameter_values=parameter_values,
+            )
+
+        # THEN
+        assert (
+            "1 validation errors for JobTemplate\nsteps[0] -> steps[0] -> parameterSpace -> combination:\n\tAssociative expressions must have arguments with identical ranges. Expression (A, B) has argument lengths (10, 2)."
+            in str(excinfo.value)
+        )