Validate PsrpOperator parameters after rendering

providers/microsoft/psrp/docs/changelog.rst

@@ -27,6 +27,17 @@
 Changelog
 ---------
 
+Main
+....
+
+Breaking changes
+~~~~~~~~~~~~~~~~~
+
+* ``PsrpOperator`` no longer derives ``task_id`` from ``cmdlet``. ``cmdlet`` is a template field,
+  so validating and reading it in ``__init__`` operated on the un-rendered Jinja expression;
+  the parameter validation now runs in ``execute()``. Pass ``task_id`` explicitly when using
+  ``cmdlet``.
+
 3.2.6
 .....
 

providers/microsoft/psrp/src/airflow/providers/microsoft/psrp/operators/psrp.py

@@ -56,8 +56,7 @@ class PsrpOperator(BaseOperator):
     :param command: command to execute on remote host. (templated)
     :param powershell: powershell to execute on remote host. (templated)
     :param cmdlet:
-        cmdlet to execute on remote host (templated). Also used as the default
-        value for `task_id`.
+        cmdlet to execute on remote host (templated).
     :param arguments:
         When using the `cmdlet` or `powershell` option, use `arguments` to
         provide arguments (templated).
@@ -106,15 +105,6 @@ def __init__(
         psrp_session_init: Command | None = None,
         **kwargs,
     ) -> None:
-        args = {command, powershell, cmdlet}
-        if not exactly_one(*args):
-            raise ValueError("Must provide exactly one of 'command', 'powershell', or 'cmdlet'")
-        if arguments and not (powershell or cmdlet):
-            raise ValueError("Arguments only allowed with 'powershell' or 'cmdlet'")
-        if parameters and not (powershell or cmdlet):
-            raise ValueError("Parameters only allowed with 'powershell' or 'cmdlet'")
-        if cmdlet:
-            kwargs.setdefault("task_id", cmdlet)
         super().__init__(**kwargs)
         self.conn_id = psrp_conn_id
         self.command = command
@@ -128,6 +118,12 @@ def __init__(
         self.psrp_session_init = psrp_session_init
 
     def execute(self, context: Context) -> list[Any] | None:
+        if not exactly_one(*{self.command, self.powershell, self.cmdlet}):
+            raise ValueError("Must provide exactly one of 'command', 'powershell', or 'cmdlet'")
+        if self.arguments and not (self.powershell or self.cmdlet):
+            raise ValueError("Arguments only allowed with 'powershell' or 'cmdlet'")
+        if self.parameters and not (self.powershell or self.cmdlet):
+            raise ValueError("Parameters only allowed with 'powershell' or 'cmdlet'")
         with (
             PsrpHook(
                 self.conn_id,

providers/microsoft/psrp/tests/unit/microsoft/psrp/operators/test_psrp.py

@@ -41,12 +41,31 @@ class ExecuteParameter(NamedTuple):
 class TestPsrpOperator:
     def test_no_command_or_powershell(self):
         exception_msg = "Must provide exactly one of 'command', 'powershell', or 'cmdlet'"
+        op = PsrpOperator(task_id="test_task_id", psrp_conn_id=CONNECTION_ID)
         with pytest.raises(ValueError, match=exception_msg):
-            PsrpOperator(task_id="test_task_id", psrp_conn_id=CONNECTION_ID)
+            op.execute(None)
 
-    def test_cmdlet_task_id_default(self):
-        operator = PsrpOperator(cmdlet="Invoke-Foo", psrp_conn_id=CONNECTION_ID)
-        assert operator.task_id == "Invoke-Foo"
+    @pytest.mark.parametrize(
+        ("kwargs", "match"),
+        [
+            ({"command": "hostname", "arguments": ["x"]}, "Arguments only allowed"),
+            ({"command": "hostname", "parameters": {"k": "v"}}, "Parameters only allowed"),
+        ],
+    )
+    def test_arguments_and_parameters_require_powershell_or_cmdlet(self, kwargs, match):
+        op = PsrpOperator(task_id="test_task_id", psrp_conn_id=CONNECTION_ID, **kwargs)
+        with pytest.raises(ValueError, match=match):
+            op.execute(None)
+
+    def test_command_validated_after_rendering(self):
+        # command is a template field: __init__-time validation saw the raw Jinja (truthy),
+        # so an expression that renders to an empty string slipped through. The check must
+        # run on the rendered value in execute().
+        op = PsrpOperator(task_id="test", psrp_conn_id=CONNECTION_ID, command="{{ '' }}")
+        op.render_template_fields({})
+        assert op.command == ""
+        with pytest.raises(ValueError, match="exactly one"):
+            op.execute(None)
 
     @pytest.mark.parametrize("do_xcom_push", [True, False])
     @pytest.mark.parametrize(
@@ -116,14 +135,14 @@ def test_execute(self, hook_impl, parameter, had_errors, rc, do_xcom_push):
         assert ps.mock_calls == expected_ps_calls
 
     def test_securestring_sandboxed(self):
-        op = PsrpOperator(psrp_conn_id=CONNECTION_ID, cmdlet="test")
+        op = PsrpOperator(task_id="test_task_id", psrp_conn_id=CONNECTION_ID, cmdlet="test")
         template = op.get_template_env().from_string("{{ 'foo' | securestring }}")
         with pytest.raises(AirflowException):
             template.render()
 
     @patch.object(BaseOperator, "get_template_env")
     def test_securestring_native(self, get_template_env):
-        op = PsrpOperator(psrp_conn_id=CONNECTION_ID, cmdlet="test")
+        op = PsrpOperator(task_id="test_task_id", psrp_conn_id=CONNECTION_ID, cmdlet="test")
         get_template_env.return_value = NativeEnvironment()
         template = op.get_template_env().from_string("{{ 'foo' | securestring }}")
         rendered = template.render()

scripts/ci/prek/validate_operators_init_exemptions.txt

@@ -61,7 +61,6 @@ providers/google/src/airflow/providers/google/cloud/transfers/gcs_to_local.py::G
 providers/google/src/airflow/providers/google/marketing_platform/operators/campaign_manager.py::GoogleCampaignManagerDeleteReportOperator
 providers/microsoft/azure/src/airflow/providers/microsoft/azure/transfers/gcs_to_wasb.py::GCSToAzureBlobStorageOperator
 providers/microsoft/azure/src/airflow/providers/microsoft/azure/transfers/oracle_to_azure_data_lake.py::OracleToAzureDataLakeOperator
-providers/microsoft/psrp/src/airflow/providers/microsoft/psrp/operators/psrp.py::PsrpOperator
 providers/oracle/src/airflow/providers/oracle/transfers/oracle_to_oracle.py::OracleToOracleOperator
 providers/papermill/src/airflow/providers/papermill/operators/papermill.py::PapermillOperator
 providers/ssh/src/airflow/providers/ssh/operators/ssh.py::SSHOperator