shahar1 commented on code in PR #70740:
URL: https://github.com/apache/airflow/pull/70740#discussion_r3695204837
##########
providers/google/src/airflow/providers/google/cloud/transfers/azure_fileshare_to_gcs.py:
##########
@@ -127,6 +128,14 @@ def __init__(
else:
self.return_gcs_uris = return_gcs_uris
+ def render_template_fields(self, context: Context, jinja_env:
jinja2.Environment | None = None) -> None:
Review Comment:
This override won't run for mapped tasks.
`MappedOperator.render_template_fields` calls
`unmapped_task._do_render_template_fields(...)` directly rather than the
operator's own `render_template_fields` — see the comment at
[`mappedoperator.py:844-847`](https://github.com/apache/airflow/blob/main/task-sdk/src/airflow/sdk/definitions/mappedoperator.py#L844).
I confirmed it with a scratch DAG: a subclass override fires for a plain task
and not for a `.partial().expand()` one.
So `AzureFileShareToGCSOperator.partial(directory_name="dir").expand(...)`
regresses: `__init__` no longer aliases, the override never fires, and
`execute` hits `RuntimeError: The directory_name must be set!.`. That path
works on `main` today — `unmap()` goes through `__init__`, which aliased the
raw template string into `directory_path`, and `_do_render_template_fields`
then rendered it correctly. (Which also means the un-rendered-alias bug this PR
fixes only ever affected non-mapped tasks.)
Decide in `__init__` and apply in `execute` — that keeps your pre-render
decision, which is the right call, and `execute` runs on both the mapped and
unmapped paths:
```python
# in __init__, replacing the render_template_fields override
self._use_directory_name = directory_path is None and directory_name is not
None
# at the top of execute()
if self._use_directory_name:
self.directory_path = self.directory_name
```
`_use_directory_name` isn't a template field, so `validate_operators_init`
stays satisfied and the exemption removal still holds. If you'd rather keep it
at render time, overriding `_do_render_template_fields` covers both paths too,
but it's a private API — I'd lean toward the flag.
Either way, please add a mapped-task test (`.partial(directory_name=...)` →
assert `directory_path` after execute), since that's the case that regressed.
##########
providers/google/tests/unit/google/cloud/transfers/test_azure_fileshare_to_gcs.py:
##########
@@ -56,6 +61,96 @@ def test_init(self):
assert operator.dest_gcs == GCS_PATH_PREFIX
assert operator.google_impersonation_chain == IMPERSONATION_CHAIN
+
@mock.patch("airflow.providers.google.cloud.transfers.azure_fileshare_to_gcs.AzureFileShareHook")
+
@mock.patch("airflow.providers.google.cloud.transfers.azure_fileshare_to_gcs.GCSHook")
+ def test_directory_name_alias_uses_rendered_value(self, gcs_mock_hook,
azure_fileshare_mock_hook):
+ """A templated directory_name is aliased to directory_path using its
rendered value, not the Jinja expression."""
+ dag = DAG("test_azure_fileshare_alias", schedule=None,
start_date=DEFAULT_DATE)
+ with pytest.warns(AirflowProviderDeprecationWarning, match="Use
'directory_path' instead"):
+ operator = AzureFileShareToGCSOperator(
+ task_id=TASK_ID,
+ share_name=AZURE_FILESHARE_SHARE,
+ directory_name="{{ params.legacy_dir }}",
+ params={"legacy_dir": "rendered/dir"},
+ azure_fileshare_conn_id=AZURE_FILESHARE_CONN_ID,
+ gcp_conn_id=GCS_CONN_ID,
+ dest_gcs=GCS_PATH_PREFIX,
+ return_gcs_uris=True,
+ dag=dag,
+ )
+ assert operator.directory_path is None
+
+ operator.render_template_fields({"params": {"legacy_dir":
"rendered/dir"}})
+ assert operator.directory_path == "rendered/dir"
+
+ azure_fileshare_mock_hook.return_value.list_files.return_value =
MOCK_FILES
+ operator.execute(None)
+ azure_fileshare_mock_hook.assert_any_call(
+ share_name=AZURE_FILESHARE_SHARE,
+ azure_fileshare_conn_id=AZURE_FILESHARE_CONN_ID,
+ directory_path="rendered/dir",
+ )
+
+
@mock.patch("airflow.providers.google.cloud.transfers.azure_fileshare_to_gcs.AzureFileShareHook")
+
@mock.patch("airflow.providers.google.cloud.transfers.azure_fileshare_to_gcs.GCSHook")
+ def test_native_directory_path_rendering_to_none_is_not_aliased(
+ self, gcs_mock_hook, azure_fileshare_mock_hook
+ ):
+ """
+ Behaviour-preservation guard for the __init__ ->
render_template_fields move.
+
+ Deciding the alias from the pre-render values must be kept: with
render_template_as_native_obj
+ an explicitly supplied directory_path can render to None, and
re-inferring the alias afterwards
+ would wrongly fall back to the deprecated directory_name. This passes
on the unrefactored code
+ (the provision decision was made in __init__) and fails if the
decision is moved past rendering.
+ """
+ dag = DAG(
+ "test_azure_fileshare_native",
+ schedule=None,
+ start_date=DEFAULT_DATE,
+ render_template_as_native_obj=True,
+ )
+ operator = AzureFileShareToGCSOperator(
+ task_id=TASK_ID,
+ share_name=AZURE_FILESHARE_SHARE,
+ directory_path="{{ params.p }}",
+ directory_name="legacy",
+ params={"p": None},
+ azure_fileshare_conn_id=AZURE_FILESHARE_CONN_ID,
+ gcp_conn_id=GCS_CONN_ID,
+ dest_gcs=GCS_PATH_PREFIX,
+ return_gcs_uris=True,
+ dag=dag,
+ )
+
+ operator.render_template_fields({"params": {"p": None}})
+ assert operator.directory_path is None
+
+ # The deprecated alias must not silently substitute directory_name; a
genuinely unset
+ # directory surfaces as the operator's own error instead of listing
the wrong directory.
+ azure_fileshare_mock_hook.return_value.list_files.return_value =
MOCK_FILES
+ with pytest.raises(RuntimeError, match="directory_name must be set"):
+ operator.execute(None)
+
+ @pytest.mark.parametrize(
+ "extra_kwargs",
+ [
+ pytest.param({"directory_path": AZURE_FILESHARE_DIRECTORY_PATH},
id="directory_path-supplied"),
+ pytest.param({}, id="neither-supplied"),
+ ],
+ )
+ def test_no_directory_name_deprecation_without_directory_name(self,
extra_kwargs):
Review Comment:
This test passes unchanged on `main` — the `__init__` condition is
semantically identical before and after
(`self.directory_path`/`self.directory_name` were assigned unmodified from the
parameters just above), so it exercises pre-existing behaviour rather than
anything this PR changes.
[AGENTS.md Testing
Standards](https://github.com/apache/airflow/blob/main/AGENTS.md#testing-standards):
*"Target exactly 100% coverage of what the PR changes — no more, no less...
every test must fail without the PR's change. Do not add tests for pre-existing
logic that was already present before the PR."*
Worth dropping, unless you're deliberately backfilling a gap — in which case
a one-line note saying so would help.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
