fix(cli): correct bad plan ID format (#3594)

Author: olevski

renku/command/checks/__init__.py

@@ -28,7 +28,7 @@
 from .project import check_project_id_group
 from .storage import check_lfs_info
 from .validate_shacl import check_datasets_structure, check_project_structure
-from .workflow import check_activity_catalog, check_plan_modification_date
+from .workflow import check_activity_catalog, check_plan_id, check_plan_modification_date
 
 # Checks will be executed in the order as they are listed in __all__. They are mostly used in ``doctor`` command to
 # inspect broken things. The order of operations matters when fixing issues, so, don't sort this list.
@@ -47,5 +47,6 @@
     "check_project_id_group",
     "check_project_structure",
     "check_plan_modification_date",
+    "check_plan_id",
     "check_activity_dates",
 )

renku/command/checks/workflow.py

@@ -127,3 +127,44 @@ def fix_plan_dates(plans: List[AbstractPlan], plan_gateway):
             if plan.date_removed and plan.date_removed < plan.date_created:
                 plan.date_removed = plan.date_created + timedelta(seconds=1)
             plan.freeze()
+
+
+@inject.autoparams("plan_gateway")
+def check_plan_id(fix, plan_gateway: IPlanGateway, **_) -> Tuple[bool, bool, Optional[str]]:
+    """Check if all plans have correct IDs.
+
+    Args:
+        fix(bool): Whether to fix found issues.
+        plan_gateway(IPlanGateway): Injected PlanGateway.
+        _: keyword arguments.
+
+    Returns:
+        Tuple[bool, Optional[str]]: Tuple of whether there are plans with invalid IDs, if an automated fix is
+            available and a string of their IDs
+    """
+    plans: List[AbstractPlan] = plan_gateway.get_all_plans()
+
+    to_be_processed = []
+    for plan in plans:
+        if isinstance(plan.id, str) and plan.id.startswith("/plans//plans"):
+            to_be_processed.append(plan)
+
+    if not to_be_processed:
+        return True, False, None
+    if not fix:
+        ids = [plan.id for plan in to_be_processed]
+        message = (
+            WARNING
+            + "The following workflows have incorrect IDs (use 'renku doctor --fix' to fix them):\n\t"
+            + "\n\t".join(ids)
+        )
+        return False, True, message
+
+    for plan in to_be_processed:
+        plan.unfreeze()
+        plan.id = plan.id.replace("//plans/", "/")
+        plan.freeze()
+    project_context.database.commit()
+    communication.info("Workflow IDs were fixed")
+
+    return True, False, None

renku/ui/service/serializers/workflows.py

@@ -17,7 +17,7 @@
 """Renku service workflow serializers."""
 from enum import Enum
 
-from marshmallow import Schema, fields
+from marshmallow import Schema, fields, pre_dump
 from marshmallow_oneofschema import OneOfSchema
 
 from renku.domain_model.dataset import DatasetCreatorsJson
@@ -43,6 +43,23 @@ class AbstractPlanResponse(Schema):
     touches_existing_files = fields.Boolean()
     duration = fields.Integer(dump_default=None)
 
+    @pre_dump(pass_many=True)
+    def fix_ids(self, objs, many, **kwargs):
+        """Renku up to 2.4.1 had a bug that created wrong ids for workflow file entities, this fixes those on export."""
+
+        def _replace_id(obj):
+            obj.unfreeze()
+            obj.id = obj.id.replace("//plans/", "/")
+            obj.freeze()
+
+        if many:
+            for obj in objs:
+                _replace_id(obj)
+            return objs
+
+        _replace_id(objs)
+        return objs
+
 
 class WorflowPlanEntryResponse(AbstractPlanResponse):
     """Serialize a plan to a response object."""

tests/core/test_plan.py

@@ -19,7 +19,7 @@
 
 import pytest
 
-from renku.command.checks import check_plan_modification_date
+from renku.command.checks import check_plan_id, check_plan_modification_date
 from renku.core import errors
 from renku.core.workflow.plan import (
     get_activities,
@@ -189,3 +189,13 @@ def test_modification_date_fix(project_with_injection):
     assert dummy_date == plan.date_modified
     assert unrelated.date_created == unrelated.date_modified
     assert date_created == plan.date_created
+
+
+def test_plan_id_fix(project_with_injection):
+    """Check that plans with incorrect IDs are fixed."""
+    _, _, plan, _, _, unrelated = create_dummy_plans()
+
+    plan.id = "/plans/" + plan.id
+    assert plan.id.startswith("/plans//plans")
+    check_plan_id(fix=True)
+    assert not plan.id.startswith("/plans//plans")