address review comments

docs/conf.py

@@ -364,12 +364,14 @@
     ("py:class", "Path"),
     ("py:class", "Persistent"),
     ("py:class", "WorkflowFileCompositePlan"),
+    ("py:class", "itertools.count"),
     ("py:class", "optional"),
     ("py:class", '"ValueResolver"'),
     ("py:exc", "errors.ParameterError"),
 ]
 
 nitpick_ignore_regex = [
+    ("py:class", r"bashlex.*"),
     ("py:class", r"calamus.*"),
     ("py:class", r"docker.*"),
     ("py:class", r"marshmallow.*"),

docs/reference/core.rst

@@ -186,6 +186,10 @@ Workflows
    :members:
    :show-inheritance:
 
+.. automodule:: renku.core.workflow.model.workflow_file
+   :members:
+   :show-inheritance:
+
 .. automodule:: renku.core.workflow.plan_factory
    :members:
    :show-inheritance:
@@ -197,6 +201,10 @@ Workflows
    :members:
    :show-inheritance:
 
+.. automodule:: renku.core.workflow.workflow_file
+   :members:
+   :show-inheritance:
+
 
 Sessions
 --------

docs/spelling_wordlist.txt

@@ -263,6 +263,7 @@ typesystem
 Ubuntu
 Unmount
 ui
+Unescape
 unhandled
 unicode
 unlink

renku/command/view_model/plan.py

@@ -258,8 +258,6 @@ def plan_view(workflow: AbstractPlan, latest: bool = False) -> Union[CompositePl
     Returns:
         View model for converted Plan.
     """
-
-    # TODO: Should we pass ``latest`` to the following calls
     if isinstance(workflow, CompositePlan):
-        return CompositePlanViewModel.from_composite_plan(workflow)
+        return CompositePlanViewModel.from_composite_plan(workflow, latest=latest)
     return PlanViewModel.from_plan(cast(Plan, workflow))

renku/core/workflow/activity.py

@@ -21,7 +21,7 @@
 import os
 from collections import defaultdict
 from pathlib import Path
-from typing import Dict, FrozenSet, Iterable, List, Optional, Set, Tuple
+from typing import Dict, FrozenSet, Iterable, List, NamedTuple, Optional, Set, Tuple
 
 import networkx
 from pydantic import validate_arguments
@@ -238,10 +238,23 @@ def sort_activities(activities: List[Activity], remove_overridden_parents=True)
     return list(networkx.topological_sort(graph))
 
 
+class ModifiedActivitiesEntities(NamedTuple):
+    """A class containing sets of modified/deleted activities and entities for both normal and hidden entities."""
+
+    modified: Set[Tuple[Activity, Entity]]
+    """Set of modified activity and entity tuples."""
+
+    deleted: Set[Tuple[Activity, Entity]]
+    """Set of deleted activity and entity tuples."""
+
+    hidden_modified: Set[Tuple[Activity, Entity]]
+    """Set of modified activity and entity tuples for hidden entities."""
+
+
 @inject.autoparams("activity_gateway")
 def get_all_modified_and_deleted_activities_and_entities(
     repository, activity_gateway: IActivityGateway, check_hidden_dependencies: bool = False
-) -> Tuple[Set[Tuple[Activity, Entity]], Set[Tuple[Activity, Entity]], Set[Tuple[Activity, Entity]]]:
+) -> ModifiedActivitiesEntities:
     """
     Return latest activities with at least one modified or deleted input along with the modified/deleted input entity.
 
@@ -252,8 +265,7 @@ def get_all_modified_and_deleted_activities_and_entities(
         activity_gateway(IActivityGateway): The injected Activity gateway.
 
     Returns:
-        Tuple[Set[Tuple[Activity, Entity]], Set[Tuple[Activity, Entity]], Set[Tuple[Activity, Entity]]]: Tuple of
-            modified and deleted activities and entities and also modified activities and entities due to hidden usages.
+        ModifiedActivitiesEntities: Modified and deleted activities and entities.
 
     """
     all_activities = activity_gateway.get_all_activities()
@@ -336,7 +348,7 @@ def has_an_existing_generation(activity) -> bool:
 
 def get_modified_activities(
     activities: FrozenSet[Activity], repository, check_hidden_dependencies: bool
-) -> Tuple[Set[Tuple[Activity, Entity]], Set[Tuple[Activity, Entity]], Set[Tuple[Activity, Entity]]]:
+) -> ModifiedActivitiesEntities:
     """Get lists of activities that have modified/deleted usage entities."""
 
     def get_modified_activities_helper(hashes, modified, deleted, hidden: bool):
@@ -372,7 +384,7 @@ def get_modified_activities_helper(hashes, modified, deleted, hidden: bool):
         hashes = repository.get_object_hashes(paths=hidden_paths)
         get_modified_activities_helper(hashes=hashes, modified=hidden_modified, deleted=set(), hidden=True)
 
-    return modified, deleted, hidden_modified
+    return ModifiedActivitiesEntities(modified=modified, deleted=deleted, hidden_modified=hidden_modified)
 
 
 def filter_overridden_activities(activities: List[Activity]) -> FrozenSet[Activity]:

renku/core/workflow/parser/renku.py

@@ -65,6 +65,41 @@ def __init__(
         self.required: bool = required
         self.list_subtype: Optional[Union[Type, Tuple[Type, ...]]] = list_subtype
 
+    def validate(self, value: Any, kind: str):
+        """Validate a given value to have correct type."""
+        if value is None:
+            if not self.required:
+                return
+            raise errors.ParseError(f"Attribute '{self.name}' in {kind} is required and cannot be None")
+
+        if not isinstance(value, self.types):
+            valid_type_names: Union[str, List[str]] = [t.__name__ for t in self.types]
+            if len(valid_type_names) == 1:
+                valid_type_names = valid_type_names[0]
+
+            raise errors.ParseError(
+                f"Invalid type for attribute '{self.name}' in {kind}: Required '{valid_type_names}' but got "
+                f"'{type(value).__name__}': {value}"
+            )
+        elif self.list_subtype:
+            subtype = self.list_subtype
+            if not isinstance(value, (list, tuple)):
+                raise errors.ParseError(
+                    f"Expected List[{subtype}] for attribute '{self.name}' in {kind} but got "
+                    f"'{type(value).__name__}': {value}"
+                )
+            for element in value:
+                if not isinstance(element, subtype):
+                    if isinstance(subtype, tuple):
+                        subtype_name = ", ".join(s.__name__ for s in subtype)
+                        subtype_name = f"({subtype_name})"
+                    else:
+                        subtype_name = subtype.__name__
+                    raise errors.ParseError(
+                        f"Expected '{subtype_name}' for elements of attribute '{self.name}' in {kind} but "
+                        f"got '{type(element).__name__}': {element}"
+                    )
+
 
 def convert_to_workflow_file(data: Dict[str, Any], path: Union[Path, str]) -> WorkflowFile:
     """Create an instance of ``WorkflowFile``."""
@@ -153,7 +188,6 @@ def convert_to_base_parameter(cls: Type[BaseParameterType], data: Union[Dict[str
     if cls == Parameter:
         attributes.append(Attribute("value", str, int, float, bool, required=True))
     else:
-        # assert cls == Output, f"Invalid parameter type: {cls}"
         attributes.extend(
             [
                 Attribute("path", str, int, float, required=True),
@@ -226,40 +260,7 @@ def validate_attributes(*, kind: str, name: Optional[str], data: Dict[str, Any],
             raise errors.ParseError(f"Invalid attributes for {kind}: {invalid_attributes_str}")
 
         for attribute in attributes:
-            if attribute.name not in data or (not attribute.required and data[attribute.name] is None):
-                continue
-
-            value = data[attribute.name]
-            if not isinstance(value, attribute.types):
-                valid_type_names: Union[str, List[str]] = [t.__name__ for t in attribute.types]
-                if len(valid_type_names) == 1:
-                    valid_type_names = valid_type_names[0]
-
-                raise errors.ParseError(
-                    f"Invalid type for attribute '{attribute.name}' in {kind}: Required '{valid_type_names}' but got "
-                    f"'{type(value).__name__}': {value}"
-                )
-            elif attribute.list_subtype:
-                subtype = attribute.list_subtype
-                if not isinstance(value, (list, tuple)):
-                    raise errors.ParseError(
-                        f"Expected List[{subtype}] for attribute '{attribute.name}' in {kind} but got "
-                        f"'{type(value).__name__}': {value}"
-                    )
-                for element in value:
-                    if not isinstance(element, subtype):
-                        if isinstance(subtype, tuple):
-                            subtype_name = ", ".join(s.__name__ for s in subtype)
-                            subtype_name = f"({subtype_name})"
-                        else:
-                            subtype_name = subtype.__name__
-                        raise errors.ParseError(
-                            f"Expected '{subtype_name}' for elements of attribute '{attribute.name}' in {kind} but "
-                            f"got '{type(element).__name__}': {element}"
-                        )
-
-    required_attributes = [a for a in attributes if a.required]
-    if required_attributes:
-        for attribute in required_attributes:
-            if attribute.name not in data:
+            if attribute.name in data:
+                attribute.validate(value=data[attribute.name], kind=kind)
+            elif attribute.required:
                 raise errors.ParseError(f"Required attribute '{attribute.name}' isn't set for {kind}: {data}")

tests/core/workflow/test_workflow_file.py

@@ -947,3 +947,68 @@ def test_workflow_file_with_cycle_raises_error(workflow_file_project, with_injec
         assert "Input: data/collection/models.csv" in exception.value.args[0]
         assert "workflow-file.head" in exception.value.args[0]
         assert "Output: data/collection/models.csv" in exception.value.args[0]
+
+
+def test_parse_error_when_required_attribute_is_null():
+    """Test error raises when a required attribute has a None value."""
+    workflow_file = textwrap.dedent(
+        """
+        name:
+        steps:
+          step:
+            command: cmd
+        """
+    )
+
+    with pytest.raises(
+        errors.ParseError, match=re.escape("Attribute 'name' in workflow file is required and cannot be None")
+    ):
+        convert_to_workflow_file(data=load_yaml(workflow_file), path="")
+
+    workflow_file = textwrap.dedent(
+        """
+        name: workflow-file
+        steps:
+          step:
+            command:
+        """
+    )
+
+    with pytest.raises(
+        errors.ParseError, match=re.escape("Attribute 'command' in step 'step' is required and cannot be None")
+    ):
+        convert_to_workflow_file(data=load_yaml(workflow_file), path="")
+
+    workflow_file = textwrap.dedent(
+        """
+        name: workflow-file
+        steps:
+          step:
+            command: cmd
+            inputs:
+              input:
+                path:
+        """
+    )
+
+    with pytest.raises(
+        errors.ParseError, match=re.escape("Attribute 'path' in input 'input' is required and cannot be None")
+    ):
+        convert_to_workflow_file(data=load_yaml(workflow_file), path="")
+
+    workflow_file = textwrap.dedent(
+        """
+        name: workflow-file
+        steps:
+          step:
+            command: cmd
+            parameters:
+              parameter:
+                value: null
+        """
+    )
+
+    with pytest.raises(
+        errors.ParseError, match=re.escape("Attribute 'value' in parameter 'parameter' is required and cannot be None")
+    ):
+        convert_to_workflow_file(data=load_yaml(workflow_file), path="")