dabla commented on code in PR #67016:
URL: https://github.com/apache/airflow/pull/67016#discussion_r3957848833
##########
providers/microsoft/azure/src/airflow/providers/microsoft/azure/hooks/wasb.py:
##########
@@ -463,6 +475,98 @@ def download(
# TODO: rework the interface as it might also return Awaitable
return blob_client.download_blob(offset=offset, length=length,
**kwargs) # type: ignore[return-value]
+ def _sync_to_local_dir_delete_stale_local_files(
+ self, current_wasb_objects: list[Path], local_dir: Path
+ ) -> None:
+ current_wasb_keys = {key.resolve() for key in current_wasb_objects}
+
+ for item in local_dir.rglob("*"):
+ if item.is_file() and item.resolve() not in current_wasb_keys:
+ self.log.debug("Deleting stale local file: %s", item)
+ item.unlink()
+ for root, dirs, _ in os.walk(local_dir, topdown=False):
+ for d in dirs:
+ dir_path = os.path.join(root, d)
+ if not os.listdir(dir_path):
+ self.log.debug("Deleting stale empty directory: %s",
dir_path)
+ os.rmdir(dir_path)
+
+ def _sync_to_local_dir_if_changed(
+ self, container_name: str, blob: BlobProperties, local_target_path:
Path
+ ) -> None:
+ should_download = False
+ download_logs: list[str] = []
+ download_log_params: list[Any] = []
+
+ if not local_target_path.exists():
+ should_download = True
+ download_logs.append("Local file %s does not exist.")
+ download_log_params.append(local_target_path)
+ else:
+ local_stats = local_target_path.stat()
+ if blob.size != local_stats.st_size:
+ should_download = True
+ download_logs.append("Blob size (%s) and local file size (%s)
differ.")
+ download_log_params.extend([blob.size, local_stats.st_size])
+
+ blob_last_modified = blob.last_modified
+ if blob_last_modified and local_stats.st_mtime <
blob_last_modified.timestamp():
+ should_download = True
+ download_logs.append("Blob last modified (%s) and local file
last modified (%s) differ.")
+ download_log_params.extend([blob_last_modified.timestamp(),
local_stats.st_mtime])
+
+ if should_download:
+ self.get_file(
+ file_path=str(local_target_path),
+ container_name=container_name,
+ blob_name=blob.name,
+ )
+ download_logs.append("Downloaded %s to %s")
+ download_log_params.extend([blob.name,
local_target_path.as_posix()])
+ self.log.debug(" ".join(download_logs), *download_log_params)
+ else:
+ self.log.debug(
+ "Local file %s is up-to-date with blob %s. Skipping download.",
+ local_target_path.as_posix(),
+ blob.name,
+ )
+
+ def sync_to_local_dir(
+ self,
+ container_name: str,
+ local_dir: Path,
+ prefix: str | None = None,
+ delete_stale: bool = True,
+ ) -> None:
+ """Download files from an Azure Blob Storage container to a local
directory."""
+ self.log.debug("Downloading data from wasb://%s/%s to %s",
container_name, prefix, local_dir)
+
+ local_wasb_objects: list[Path] = []
+ container = self._get_container_client(container_name)
+ self.check_for_variable_type("container", container, ContainerClient)
+ container = cast("ContainerClient", container)
+
+ for blob in container.list_blobs(name_starts_with=prefix):
+ if blob.name.endswith("/"):
+ continue
+ blob_path = Path(blob.name)
+ if prefix:
+ local_target_path =
local_dir.joinpath(blob_path.relative_to(prefix))
Review Comment:
**[warning]** `list_blobs(name_starts_with=prefix)` is a raw string-prefix
match, not folder-aware, so it can pick up blobs outside the intended prefix
and crash the sync.
```python
for blob in container.list_blobs(name_starts_with=prefix):
...
if prefix:
local_target_path = local_dir.joinpath(blob_path.relative_to(prefix))
```
Azure's `name_starts_with` matches on the literal string, not a path segment
boundary. If `prefix="project1/dags"` and the container also has blobs under a
sibling path such as `project1/dags-archive/old_dag.py`, that blob's name also
starts with the string `"project1/dags"` and will be included in the listing.
`Path("project1/dags-archive/old_dag.py").relative_to("project1/dags")` then
raises `ValueError` (not a subpath), which will propagate out of
`sync_to_local_dir` and fail the whole bundle refresh — even though the
offending blob was never meant to be part of this bundle.
The bundle's own `check_for_prefix` validation (in
`WasbDagBundle._initialize`) passes `delimiter="/"` for exactly this reason,
but `sync_to_local_dir`'s own listing call does not. Consider normalizing the
prefix to end with `/` before listing (e.g. `prefix = prefix.rstrip("/") + "/"
if prefix else prefix`), or catching the mismatch defensively, so a same-prefix
sibling blob can't crash the whole sync.
--
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]
