fix: no longer shortcircuit validation if there are parameter referen…

…ce errors (#71)

Problem:
The improvement to validation errors implemented in #47
introduced an error. If there are errors flagged for parameter references
(e.g. definition of an unknown variable) then we are aborting the rest of the
model validation. This can make diagnosing errors in a template harder than it
should be. The root of the bug is that pydantic itself short-circuits model
parsing if any pre-root-validator raises an error.

For example, validation of the following template will not show that the field
'parmeterDefinitions' is unknown (it should be 'parameterDefinitions' -- notice the
missing 'a' in 'para'):

```
specificationVersion: jobtemplate-2023-09
name: DemoJob
parmeterDefinitions:
- name: Foo
  type: INT
steps:
- name: DemoStep
  script:
    actions:
      onRun:
        command: "{{Param.Foo}}"
```

Solution:
I had to move the variable reference validation out of pydantic's validation flows.
It doesn't work as a regular root validator (see the other PR for why), and we can't
have it short-circuiting validation as a pre-root-validator. So, instead I've updated
the _parse_model() function to run the variable reference validation if the model
has a _root_template_prevalidator method defined.

This does come with a trade-off. We no longer run the template variable validation at
all if the user is directly constructing a data model using the model classes. Adding
a __init__() to JobTemplate and EnvironmentTemplate to force the validator to run
ended up in doubling-up all variable reference validation errors. So, this'll have to
be a tradeoff that we accept until a better solution comes around.

Signed-off-by: Daniel Neilson <53624638+ddneilson@users.noreply.github.com>

src/openjd/model/_parse.py

@@ -4,11 +4,12 @@
 from dataclasses import is_dataclass
 from decimal import Decimal
 from enum import Enum
-from typing import Any, ClassVar, Type, TypeVar, Union, cast
+from typing import Any, ClassVar, Optional, Type, TypeVar, Union, cast
 
 import yaml
 from pydantic import BaseModel
 from pydantic import ValidationError as PydanticValidationError
+from pydantic.error_wrappers import ErrorWrapper
 
 from ._errors import DecodeValidationError
 from ._types import EnvironmentTemplate, JobTemplate, OpenJDModel, TemplateSpecificationVersion
@@ -47,16 +48,30 @@ def _parse_model(*, model: Type[T], obj: Any) -> T:
     if is_dataclass(model):
         return cast(T, cast(PydanticDataclass, model).__pydantic_model__.parse_obj(obj))
     else:
-        return cast(T, cast(BaseModel, model).parse_obj(obj))
+        prevalidator_error: Optional[PydanticValidationError] = None
+        if hasattr(model, "_root_template_prevalidator"):
+            try:
+                getattr(model, "_root_template_prevalidator")(obj)
+            except PydanticValidationError as exc:
+                prevalidator_error = exc
+        try:
+            result = cast(T, cast(BaseModel, model).parse_obj(obj))
+        except PydanticValidationError as exc:
+            errors: list[ErrorWrapper] = cast(list[ErrorWrapper], exc.raw_errors)
+            if prevalidator_error is not None:
+                errors.extend(cast(list[ErrorWrapper], prevalidator_error.raw_errors))
+            raise PydanticValidationError(errors, model)
+        if prevalidator_error is not None:
+            raise prevalidator_error
+        return result
 
 
 def parse_model(*, model: Type[T], obj: Any) -> T:
     try:
         return _parse_model(model=model, obj=obj)
     except PydanticValidationError as exc:
-        raise DecodeValidationError(
-            pydantic_validationerrors_to_str(model, cast(list[ErrorDict], exc.errors()))
-        )
+        errors: list[ErrorDict] = cast(list[ErrorDict], exc.errors())
+        raise DecodeValidationError(pydantic_validationerrors_to_str(model, errors))
 
 
 def document_string_to_object(*, document: str, document_type: DocumentType) -> dict[str, Any]:

src/openjd/model/v2023_09/_model.py

@@ -2217,8 +2217,11 @@ def _unique_environment_names(
             return validate_unique_elements(v, item_value=lambda v: v.name, property="name")
         return v
 
-    @root_validator(pre=True)
-    def _prevalidate_template_variable_references(cls, values: dict[str, Any]) -> dict[str, Any]:
+    @classmethod
+    def _root_template_prevalidator(cls, values: dict[str, Any]) -> dict[str, Any]:
+        # The name of this validator is very important. It is specifically looked for
+        # in the _parse_model function to run this validation as a pre-root-validator
+        # without the usual short-circuit of pre-root-validators that pydantic does.
         errors = prevalidate_model_template_variable_references(
             cast(Type[OpenJDModel], cls), values
         )
@@ -2340,8 +2343,11 @@ def _unique_parameter_names(
             return validate_unique_elements(v, item_value=lambda v: v.name, property="name")
         return v
 
-    @root_validator(pre=True)
-    def _prevalidate_template_variable_references(cls, values: dict[str, Any]) -> dict[str, Any]:
+    @classmethod
+    def _root_template_prevalidator(cls, values: dict[str, Any]) -> dict[str, Any]:
+        # The name of this validator is very important. It is specifically looked for
+        # in the _parse_model function to run this validation as a pre-root-validator
+        # without the usual short-circuit of pre-root-validators that pydantic does.
         errors = prevalidate_model_template_variable_references(
             cast(Type[OpenJDModel], cls), values
         )

test/openjd/model/v2023_09/test_environment_template.py

@@ -222,7 +222,7 @@ def test_parse_success(self, data: dict[str, Any]) -> None:
                         },
                     },
                 },
-                2,  # Validation of Job Foo & Task Foo
+                3,  # "Blah" is not a valid integer + Validation of Job Foo & Task Foo
                 id="all parameter symbols are defined when validation errors",
             ),
         ),

test/openjd/model/v2023_09/test_template_variables.py

@@ -19,7 +19,10 @@
 STEP_TEMPLATE = {"name": "StepName", "script": STEP_SCRIPT}
 STEP_SCRIPT_FOO = {
     "actions": {
-        "onRun": {"command": "foo {{Param.Foo}} {{RawParam.Foo}} {{Session.WorkingDirectory}}"}
+        "onRun": {
+            "command": "foo {{Param.Foo}} {{RawParam.Foo}} {{Session.WorkingDirectory}}",
+            "args": ["foo {{Param.Foo}} {{RawParam.Foo}} {{Session.WorkingDirectory}}"],
+        }
     }
 }
 STEP_TEMPLATE_FOO = {"name": "StepName", "script": STEP_SCRIPT_FOO}
@@ -520,9 +523,19 @@ def test_parse_success(self, data: dict[str, Any]) -> None:
                     "name": "Foo",
                     "steps": [STEP_TEMPLATE_FOO],
                 },
-                2,
+                4,
                 id="step missing parameter",
             ),
+            pytest.param(
+                {
+                    "specificationVersion": "jobtemplate-2023-09",
+                    "name": "Foo",
+                    "parmDef": [FOO_PARAMETER_INT],
+                    "steps": [STEP_TEMPLATE_FOO],
+                },
+                5,  # extra field + 2 param refs in each of command+args
+                id="key error and path parameter 'Foo' missing",
+            ),
             pytest.param(
                 {
                     "specificationVersion": "jobtemplate-2023-09",
@@ -540,7 +553,7 @@ def test_parse_success(self, data: dict[str, Any]) -> None:
                     "steps": [STEP_TEMPLATE_FOO],
                     "jobEnvironments": [ENVIRONMENT_FOO],
                 },
-                4,
+                6,
                 id="step and environment missing parameter",
             ),
             pytest.param(