Remove no-op methods

Author: dandavison

temporalio/worker/_command_aware_visitor.py

@@ -9,7 +9,6 @@
 from temporalio.bridge._visitor import PayloadVisitor, VisitorFunctions
 from temporalio.bridge.proto.workflow_activation import workflow_activation_pb2
 from temporalio.bridge.proto.workflow_activation.workflow_activation_pb2 import (
-    FireTimer,
     ResolveActivity,
     ResolveChildWorkflowExecution,
     ResolveChildWorkflowExecutionStart,
@@ -20,18 +19,11 @@
 )
 from temporalio.bridge.proto.workflow_commands import workflow_commands_pb2
 from temporalio.bridge.proto.workflow_commands.workflow_commands_pb2 import (
-    CancelSignalWorkflow,
-    CancelTimer,
-    RequestCancelActivity,
-    RequestCancelExternalWorkflowExecution,
-    RequestCancelLocalActivity,
-    RequestCancelNexusOperation,
     ScheduleActivity,
     ScheduleLocalActivity,
     ScheduleNexusOperation,
     SignalExternalWorkflowExecution,
     StartChildWorkflowExecution,
-    StartTimer,
 )
 
 
@@ -51,11 +43,11 @@ class CommandInfo:
 class CommandAwarePayloadVisitor(PayloadVisitor):
     """Payload visitor that sets command context during traversal.
 
-    Override methods are explicitly defined for all workflow commands and
-    activation jobs that have a 'seq' field.
+    Override methods are explicitly defined for workflow commands and
+    activation jobs that have both a 'seq' field and payloads to visit.
     """
 
-    # Workflow commands
+    # Workflow commands with payloads
     async def _visit_coresdk_workflow_commands_ScheduleActivity(
         self, fs: VisitorFunctions, o: ScheduleActivity
     ) -> None:
@@ -88,72 +80,13 @@ async def _visit_coresdk_workflow_commands_SignalExternalWorkflowExecution(
                 fs, o
             )
 
-    async def _visit_coresdk_workflow_commands_RequestCancelExternalWorkflowExecution(
-        self, fs: VisitorFunctions, o: RequestCancelExternalWorkflowExecution
-    ) -> None:
-        with current_command(
-            CommandType.COMMAND_TYPE_REQUEST_CANCEL_EXTERNAL_WORKFLOW_EXECUTION, o.seq
-        ):
-            # Note: Base class doesn't have this visitor (no payloads to visit)
-            pass
-
     async def _visit_coresdk_workflow_commands_ScheduleNexusOperation(
         self, fs: VisitorFunctions, o: ScheduleNexusOperation
     ) -> None:
         with current_command(CommandType.COMMAND_TYPE_SCHEDULE_NEXUS_OPERATION, o.seq):
             await super()._visit_coresdk_workflow_commands_ScheduleNexusOperation(fs, o)
 
-    async def _visit_coresdk_workflow_commands_RequestCancelNexusOperation(
-        self, fs: VisitorFunctions, o: RequestCancelNexusOperation
-    ) -> None:
-        with current_command(
-            CommandType.COMMAND_TYPE_REQUEST_CANCEL_NEXUS_OPERATION, o.seq
-        ):
-            # Note: Base class doesn't have this visitor (no payloads to visit)
-            pass
-
-    async def _visit_coresdk_workflow_commands_StartTimer(
-        self, fs: VisitorFunctions, o: StartTimer
-    ) -> None:
-        with current_command(CommandType.COMMAND_TYPE_START_TIMER, o.seq):
-            # Note: Base class doesn't have this visitor (no payloads to visit)
-            pass
-
-    async def _visit_coresdk_workflow_commands_CancelTimer(
-        self, fs: VisitorFunctions, o: CancelTimer
-    ) -> None:
-        with current_command(CommandType.COMMAND_TYPE_CANCEL_TIMER, o.seq):
-            # Note: Base class doesn't have this visitor (no payloads to visit)
-            pass
-
-    async def _visit_coresdk_workflow_commands_RequestCancelActivity(
-        self, fs: VisitorFunctions, o: RequestCancelActivity
-    ) -> None:
-        with current_command(
-            CommandType.COMMAND_TYPE_REQUEST_CANCEL_ACTIVITY_TASK, o.seq
-        ):
-            # Note: Base class doesn't have this visitor (no payloads to visit)
-            pass
-
-    async def _visit_coresdk_workflow_commands_RequestCancelLocalActivity(
-        self, fs: VisitorFunctions, o: RequestCancelLocalActivity
-    ) -> None:
-        with current_command(
-            CommandType.COMMAND_TYPE_REQUEST_CANCEL_ACTIVITY_TASK, o.seq
-        ):
-            # Note: Base class doesn't have this visitor (no payloads to visit)
-            pass
-
-    async def _visit_coresdk_workflow_commands_CancelSignalWorkflow(
-        self, fs: VisitorFunctions, o: CancelSignalWorkflow
-    ) -> None:
-        # CancelSignalWorkflow has seq but no server command type
-        # (it's an internal SDK command). Set context to None.
-        with current_command(None, o.seq):  # type: ignore
-            # Note: Base class doesn't have this visitor (no payloads to visit)
-            pass
-
-    # Workflow activation jobs
+    # Workflow activation jobs with payloads
     async def _visit_coresdk_workflow_activation_ResolveActivity(
         self, fs: VisitorFunctions, o: ResolveActivity
     ) -> None:
@@ -216,13 +149,6 @@ async def _visit_coresdk_workflow_activation_ResolveNexusOperation(
                 fs, o
             )
 
-    async def _visit_coresdk_workflow_activation_FireTimer(
-        self, fs: VisitorFunctions, o: FireTimer
-    ) -> None:
-        with current_command(CommandType.COMMAND_TYPE_START_TIMER, o.seq):
-            # Note: Base class doesn't have this visitor (no payloads to visit)
-            pass
-
 
 def _get_workflow_command_protos_with_seq() -> Iterator[Type[Any]]:
     """Get concrete classes of all workflow command protos with a seq field."""

tests/worker/test_command_aware_visitor.py

@@ -1,46 +1,75 @@
-"""Test that CommandAwarePayloadVisitor handles all commands with seq fields."""
+"""Test that CommandAwarePayloadVisitor handles all commands with seq fields that have payloads."""
 
+from temporalio.bridge._visitor import PayloadVisitor
 from temporalio.worker._command_aware_visitor import (
     CommandAwarePayloadVisitor,
     _get_workflow_activation_job_protos_with_seq,
     _get_workflow_command_protos_with_seq,
 )
 
 
-def test_command_aware_visitor_has_methods_for_all_seq_protos():
-    """Verify CommandAwarePayloadVisitor has methods for all protos with seq fields."""
-    visitor = CommandAwarePayloadVisitor()
+def test_command_aware_visitor_has_methods_for_all_seq_protos_with_payloads():
+    """Verify CommandAwarePayloadVisitor has methods for all protos with seq fields that have payloads.
 
-    # Check all workflow commands with seq have corresponding methods
+    We only override methods when the base class has a visitor method (i.e., there are payloads to visit).
+    Commands without payloads don't need overrides since there's nothing to visit.
+    """
+    visitor = CommandAwarePayloadVisitor()
 
+    # Find all protos with seq
     command_protos = list(_get_workflow_command_protos_with_seq())
     job_protos = list(_get_workflow_activation_job_protos_with_seq())
     assert command_protos, "Should find workflow commands with seq"
     assert job_protos, "Should find workflow activation jobs with seq"
 
+    # Check workflow commands - only ones with payloads need overrides
     commands_missing = []
+    commands_with_payloads = []
     for proto_class in command_protos:
         method_name = f"_visit_coresdk_workflow_commands_{proto_class.__name__}"
-        if not hasattr(visitor, method_name):
-            commands_missing.append(proto_class.__name__)
+        # Only check if base class has this visitor (meaning there are payloads)
+        if hasattr(PayloadVisitor, method_name):
+            commands_with_payloads.append(proto_class.__name__)
+            # Check if CommandAwarePayloadVisitor has its own override (not just inherited)
+            if method_name not in CommandAwarePayloadVisitor.__dict__:
+                commands_missing.append(proto_class.__name__)
 
-    # Check all workflow activation jobs with seq have corresponding methods
+    # Check workflow activation jobs - only ones with payloads need overrides
     jobs_missing = []
+    jobs_with_payloads = []
     for proto_class in job_protos:
         method_name = f"_visit_coresdk_workflow_activation_{proto_class.__name__}"
-        if not hasattr(visitor, method_name):
-            jobs_missing.append(proto_class.__name__)
+        # Only check if base class has this visitor (meaning there are payloads)
+        if hasattr(PayloadVisitor, method_name):
+            jobs_with_payloads.append(proto_class.__name__)
+            # Check if CommandAwarePayloadVisitor has its own override (not just inherited)
+            if method_name not in CommandAwarePayloadVisitor.__dict__:
+                jobs_missing.append(proto_class.__name__)
 
     errors = []
     if commands_missing:
         errors.append(
-            f"Missing visitor methods for commands with seq: {commands_missing}\n"
+            f"Missing visitor methods for commands with seq and payloads: {commands_missing}\n"
             f"Add methods to CommandAwarePayloadVisitor for these commands."
         )
     if jobs_missing:
         errors.append(
-            f"Missing visitor methods for activation jobs with seq: {jobs_missing}\n"
+            f"Missing visitor methods for activation jobs with seq and payloads: {jobs_missing}\n"
             f"Add methods to CommandAwarePayloadVisitor for these jobs."
         )
 
     assert not errors, "\n".join(errors)
+
+    # Verify we found the expected commands/jobs with payloads
+    assert len(commands_with_payloads) > 0, "Should find commands with payloads"
+    assert len(jobs_with_payloads) > 0, "Should find activation jobs with payloads"
+
+    # Sanity check: we should have fewer overrides than total protos with seq
+    # (because some don't have payloads)
+    assert len(commands_with_payloads) < len(
+        command_protos
+    ), "Should have some commands without payloads"
+    # All activation jobs except FireTimer have payloads
+    assert (
+        len(jobs_with_payloads) == len(job_protos) - 1
+    ), "Should have exactly one activation job without payloads (FireTimer)"