From 5382e82f98a8a972caa2467911e24e10c102e0f1 Mon Sep 17 00:00:00 2001 From: rudolfix Date: Sun, 23 Aug 2026 18:56:16 +0200 Subject: [PATCH 1/5] allows empty size in FileItem, fallback for http filesystem with separate file dewtails fetch --- dlt/common/storages/fsspec_filesystem.py | 75 +++++++++------ dlt/common/storages/transactional_file.py | 9 +- dlt/sources/filesystem/__init__.py | 8 +- dlt/sources/filesystem/helpers.py | 1 + .../verified-sources/filesystem/index.md | 18 +++- tests/common/schema/test_schema_utils.py | 34 +++---- tests/common/storages/test_http_filesystem.py | 96 +++++++++++++++++++ tests/common/storages/utils.py | 3 +- .../load/filesystem/test_filesystem_common.py | 4 +- .../filesystem/test_filesystem_source.py | 3 +- tests/normalize/test_json_item_inference.py | 1 - tests/utils.py | 42 ++++++-- 12 files changed, 225 insertions(+), 69 deletions(-) create mode 100644 tests/common/storages/test_http_filesystem.py diff --git a/dlt/common/storages/fsspec_filesystem.py b/dlt/common/storages/fsspec_filesystem.py index 7d0bd0fbe6..bfc84f7794 100644 --- a/dlt/common/storages/fsspec_filesystem.py +++ b/dlt/common/storages/fsspec_filesystem.py @@ -4,6 +4,8 @@ import mimetypes import pathlib import posixpath +from datetime import datetime, timezone # noqa: I251 +from email.utils import parsedate_to_datetime from io import BytesIO from typing import ( Literal, @@ -25,7 +27,6 @@ from dlt import version from dlt.common.typing import TypedDict, NotRequired -from dlt.common.pendulum import pendulum from dlt.common.configuration.specs import ( GcpCredentials, AwsCredentials, @@ -39,7 +40,7 @@ FilesystemConfiguration, make_fsspec_url, ) -from dlt.common.time import ensure_pendulum_datetime_utc +from dlt.common.time import ensure_datetime_utc from dlt.common.typing import DictStrAny from dlt.common.utils import without_none @@ -52,35 +53,34 @@ class FileItem(TypedDict): relative_path: str mime_type: str encoding: NotRequired[str] - modification_date: pendulum.DateTime - size_in_bytes: int + modification_date: datetime + size_in_bytes: Optional[int] file_content: NotRequired[bytes] -# Map of protocol to mtime resolver +def _http_mtime(f: DictStrAny) -> Optional[datetime]: + """Reads `Last-Modified` which only `info()` responses carry, http listings have no mtime.""" + last_modified = f.get("Last-Modified") + if last_modified is None: + return None + # an http date is not iso, only the email parser reads it + return ensure_datetime_utc(parsedate_to_datetime(last_modified)) + + +# Map of protocol to mtime resolver, returns None when the listing has no modification date # we only need to support a small finite set of protocols -MTIME_DISPATCH = { - "s3": lambda f: ensure_pendulum_datetime_utc(f["LastModified"]), - "adl": lambda f: ensure_pendulum_datetime_utc(f["LastModified"]), - "az": lambda f: ensure_pendulum_datetime_utc(f["last_modified"]), - "gcs": lambda f: ensure_pendulum_datetime_utc(f["updated"]), - "hf": lambda f: ensure_pendulum_datetime_utc(f["last_commit"]["date"]), - "https": lambda f: cast( - pendulum.DateTime, - pendulum.parse( - f.get("Last-Modified", pendulum.now().isoformat()), exact=True, strict=False - ), - ), - "http": lambda f: cast( - pendulum.DateTime, - pendulum.parse( - f.get("Last-Modified", pendulum.now().isoformat()), exact=True, strict=False - ), - ), - "file": lambda f: ensure_pendulum_datetime_utc(f["mtime"]), - "memory": lambda f: ensure_pendulum_datetime_utc(f["created"]), - "gdrive": lambda f: ensure_pendulum_datetime_utc(f["modifiedTime"]), - "sftp": lambda f: ensure_pendulum_datetime_utc(f["mtime"]), +MTIME_DISPATCH: Dict[str, Callable[[DictStrAny], Optional[datetime]]] = { + "s3": lambda f: ensure_datetime_utc(f["LastModified"]), + "adl": lambda f: ensure_datetime_utc(f["LastModified"]), + "az": lambda f: ensure_datetime_utc(f["last_modified"]), + "gcs": lambda f: ensure_datetime_utc(f["updated"]), + "hf": lambda f: ensure_datetime_utc(f["last_commit"]["date"]), + "https": _http_mtime, + "http": _http_mtime, + "file": lambda f: ensure_datetime_utc(f["mtime"]), + "memory": lambda f: ensure_datetime_utc(f["created"]), + "gdrive": lambda f: ensure_datetime_utc(f["modifiedTime"]), + "sftp": lambda f: ensure_datetime_utc(f["mtime"]), } # Support aliases MTIME_DISPATCH["gs"] = MTIME_DISPATCH["gcs"] @@ -350,7 +350,10 @@ def guess_mime_type(file_name: str) -> Sequence[str]: def glob_files( - fs_client: AbstractFileSystem, bucket_url: str, file_glob: str = "**" + fs_client: AbstractFileSystem, + bucket_url: str, + file_glob: str = "**", + fetch_file_info: bool = False, ) -> Iterator[FileItem]: """Get the files from the filesystem client. @@ -358,6 +361,8 @@ def glob_files( fs_client (AbstractFileSystem): The filesystem client. bucket_url (str): The url to the bucket. file_glob (str): A glob for the filename filter. + fetch_file_info (bool): Calls `info` on each listed file that the listing reports no size + or modification date for, at the cost of one request per file. Defaults to False. Returns: Iterable[FileItem]: The list of files. @@ -387,10 +392,18 @@ def glob_files( " version 2023.9.0 or later" ) + scheme = bucket_url_parsed.scheme + for file, md in glob_result.items(): if md["type"] != "file": continue - scheme = bucket_url_parsed.scheme + size, modification_date = md.get("size"), MTIME_DISPATCH[scheme](md) + # http listings are scraped from an html index and carry neither size nor mtime + if fetch_file_info and (size is None or modification_date is None): + md = fs_client.info(file) + size, modification_date = md.get("size"), MTIME_DISPATCH[scheme](md) + if modification_date is None: + modification_date = datetime.now(timezone.utc) # relative paths are always POSIX if is_local_fs: @@ -410,8 +423,8 @@ def glob_files( relative_path=rel_path, file_url=file_url, mime_type=mime_type, - modification_date=MTIME_DISPATCH[scheme](md), - size_in_bytes=int(md["size"]), + modification_date=modification_date, + size_in_bytes=None if size is None else int(size), ) if encoding is not None: file_item["encoding"] = encoding diff --git a/dlt/common/storages/transactional_file.py b/dlt/common/storages/transactional_file.py index a25f9bb3f7..26cecb638e 100644 --- a/dlt/common/storages/transactional_file.py +++ b/dlt/common/storages/transactional_file.py @@ -9,13 +9,13 @@ import string import time import typing as t +from datetime import datetime, timedelta, timezone # noqa: I251 from pathlib import Path import posixpath from contextlib import contextmanager from threading import Timer import fsspec -from dlt.common.pendulum import pendulum, timedelta from dlt.common.storages.fsspec_filesystem import MTIME_DISPATCH @@ -96,7 +96,7 @@ def _stop_heartbeat(self) -> None: def _sync_locks(self) -> t.List[str]: """Gets a list of lock names after removing stale locks. The list is time-sortable with earliest created lock coming first.""" output = [] - now = pendulum.now() + now = datetime.now(timezone.utc) for lock in self._fs.ls(posixpath.dirname(self.lock_path), refresh=True, detail=True): name = lock["name"] @@ -104,7 +104,10 @@ def _sync_locks(self) -> t.List[str]: continue # Purge stale locks mtime = self.extract_mtime(lock) - if now - mtime > timedelta(seconds=TransactionalFile.LOCK_TTL_SECONDS): + # a filesystem that reports no mtime cannot expire locks, keep them + if mtime is not None and now - mtime > timedelta( + seconds=TransactionalFile.LOCK_TTL_SECONDS + ): try: # Janitors can race, so we ignore errors self._fs.rm(name) except OSError: diff --git a/dlt/sources/filesystem/__init__.py b/dlt/sources/filesystem/__init__.py index 57fe6bbc01..282ebba032 100644 --- a/dlt/sources/filesystem/__init__.py +++ b/dlt/sources/filesystem/__init__.py @@ -100,6 +100,7 @@ def filesystem( # noqa DOC file_glob: str = "*", files_per_page: int = DEFAULT_CHUNK_SIZE, extract_content: bool = False, + fetch_file_info: bool = False, kwargs: Optional[Dict[str, Any]] = None, client_kwargs: Optional[Dict[str, Any]] = None, incremental: Optional[dlt.sources.incremental[Any]] = None, @@ -114,6 +115,9 @@ def filesystem( # noqa DOC files_per_page (int, optional): The number of files to process at once, defaults to 100. extract_content (bool, optional): If true, the content of the file will be extracted if false it will return a fsspec file, defaults to False. + fetch_file_info (bool, optional): If true, `size_in_bytes` and `modification_date` are read + per file for filesystems whose listing omits them (http), at the cost of one request per + file. Without it such files report `size_in_bytes` as None, defaults to False. kwargs (Optional[Dict[str, Any]]): Additional arguments passed to fsspec constructor ie. dict(use_ssl=True) for s3fs client_kwargs (Optional[Dict[str, Any]]): Additional arguments passed to underlying fsspec native client ie. dict(verify="public.crt) for botocore incremental (Optional[dlt.sources.incremental[Any]]): Defines incremental cursor on listed files, with `modification_date` @@ -133,7 +137,7 @@ def filesystem( # noqa DOC files_chunk: List[FileItem] = [] - iter_ = glob_files(fs_client, bucket_url, file_glob) + iter_ = glob_files(fs_client, bucket_url, file_glob, fetch_file_info) # if incremental is set with row order, use it to order the results # NOTE: fsspec glob for buckets reads all files before running iterator @@ -144,7 +148,7 @@ def filesystem( # noqa DOC ) iter_ = iter( sorted( - list(glob_files(fs_client, bucket_url, file_glob)), + list(glob_files(fs_client, bucket_url, file_glob, fetch_file_info)), key=lambda f_: f_[incremental.cursor_path], # type: ignore[literal-required] reverse=reverse, ) diff --git a/dlt/sources/filesystem/helpers.py b/dlt/sources/filesystem/helpers.py index 3ee482f19a..b63bfc10d5 100644 --- a/dlt/sources/filesystem/helpers.py +++ b/dlt/sources/filesystem/helpers.py @@ -29,6 +29,7 @@ class FilesystemConfigurationResource(FilesystemConfigurationWithLocalFiles): file_glob: Optional[str] = "*" files_per_page: int = DEFAULT_CHUNK_SIZE extract_content: bool = False + fetch_file_info: bool = False @resolve_type("credentials") def resolve_credentials_type(self) -> Type[CredentialsConfiguration]: diff --git a/docs/website/docs/dlt-ecosystem/verified-sources/filesystem/index.md b/docs/website/docs/dlt-ecosystem/verified-sources/filesystem/index.md index 476cbe94fd..8b71511eb9 100644 --- a/docs/website/docs/dlt-ecosystem/verified-sources/filesystem/index.md +++ b/docs/website/docs/dlt-ecosystem/verified-sources/filesystem/index.md @@ -366,6 +366,15 @@ Full list of `filesystem` resource parameters: * `files_per_page` - number of files processed at once. The default value is `100`. * `extract_content` - if true, the content of the file will be read and returned in the resource. The default value is `False`. +* `fetch_file_info` - if true, `size_in_bytes` and `modification_date` are read for each listed file that the + listing itself does not report them for. The default value is `False`. + + :::note + Only `http` and `https` need this. There is no listing protocol over HTTP, so fsspec builds the file list by + scraping the HTML index page, which carries neither a size nor a modification date. Without `fetch_file_info`, + such files have `size_in_bytes` set to `None` and `modification_date` set to the time of listing. With it, dlt + makes one extra request per file to read them. + ::: ### 2. Choose the right reader @@ -607,6 +616,11 @@ filtered_files = filesystem(bucket_url="s3://bucket_name", file_glob="**/*.json" If for some reason you only want to load small files, you can also do that: +:::note +Over `http`/`https`, pass `fetch_file_info=True` to the resource. Without it `size_in_bytes` is `None` and the +comparison below fails. +::: + ```py import dlt from dlt.sources.filesystem import filesystem, read_csv @@ -653,8 +667,8 @@ The filesystem ensures consistent file representation across bucket types and of - `file_name` - name of the file from the bucket URL. - `relative_path` - set when doing `glob`, is a relative path to a `bucket_url` argument. - `mime_type` - file's MIME type. It is sourced from the bucket provider or inferred from its extension. -- `modification_date` - file's last modification time (format: `pendulum.DateTime`). -- `size_in_bytes` - file size. +- `modification_date` - file's last modification time (format: `datetime.datetime`, always UTC). +- `size_in_bytes` - file size. `None` when the filesystem does not report one, see `fetch_file_info`. - `file_content` - content, provided upon request. :::info diff --git a/tests/common/schema/test_schema_utils.py b/tests/common/schema/test_schema_utils.py index d25f25ef62..da6cb954fd 100644 --- a/tests/common/schema/test_schema_utils.py +++ b/tests/common/schema/test_schema_utils.py @@ -174,13 +174,15 @@ def test_version_table_has_column_descriptions() -> None: "inserted_at", "schema_name", "version_hash", - "schema" + "schema", ] for col_name in expected_columns: assert col_name in table["columns"], f"Column {col_name} missing from version_table" col = table["columns"][col_name] assert "description" in col, f"Column {col_name} missing description" - assert isinstance(col["description"], str), f"Column {col_name} description should be a string" + assert isinstance( + col["description"], str + ), f"Column {col_name} description should be a string" assert len(col["description"]) > 0, f"Column {col_name} description should not be empty" @@ -193,18 +195,14 @@ def test_loads_table_has_column_descriptions() -> None: assert table["description"] == "Created by DLT. Tracks completed loads" # Verify all columns have descriptions - expected_columns = [ - "load_id", - "schema_name", - "status", - "inserted_at", - "schema_version_hash" - ] + expected_columns = ["load_id", "schema_name", "status", "inserted_at", "schema_version_hash"] for col_name in expected_columns: assert col_name in table["columns"], f"Column {col_name} missing from loads_table" col = table["columns"][col_name] assert "description" in col, f"Column {col_name} missing description" - assert isinstance(col["description"], str), f"Column {col_name} description should be a string" + assert isinstance( + col["description"], str + ), f"Column {col_name} description should be a string" assert len(col["description"]) > 0, f"Column {col_name} description should not be empty" @@ -231,7 +229,9 @@ def test_pipeline_state_table_has_column_descriptions() -> None: assert col_name in table["columns"], f"Column {col_name} missing from pipeline_state_table" col = table["columns"][col_name] assert "description" in col, f"Column {col_name} missing description" - assert isinstance(col["description"], str), f"Column {col_name} description should be a string" + assert isinstance( + col["description"], str + ), f"Column {col_name} description should be a string" assert len(col["description"]) > 0, f"Column {col_name} description should not be empty" @@ -257,9 +257,9 @@ def test_internal_tables_column_descriptions_are_not_empty() -> None: for table_name, table in tables: for col_name, col in table["columns"].items(): - assert "description" in col, ( - f"Table {table_name}, column {col_name} missing description" - ) - assert col["description"] is not None and col["description"].strip() != "", ( - f"Table {table_name}, column {col_name} has empty description" - ) \ No newline at end of file + assert ( + "description" in col + ), f"Table {table_name}, column {col_name} missing description" + assert ( + col["description"] is not None and col["description"].strip() != "" + ), f"Table {table_name}, column {col_name} has empty description" diff --git a/tests/common/storages/test_http_filesystem.py b/tests/common/storages/test_http_filesystem.py new file mode 100644 index 0000000000..394c4571ad --- /dev/null +++ b/tests/common/storages/test_http_filesystem.py @@ -0,0 +1,96 @@ +from datetime import datetime, timezone # noqa: I251 +from typing import Dict, List + +import fsspec +import pytest + +from dlt.common.storages.fsspec_filesystem import FileItem, glob_files + +from tests.utils import autoindex_http_server + +HTTP_BUCKET_URL = "http://localhost:8190" + +# expected sizes of the csv sample files, keyed by relative path +CSV_SAMPLE_SIZES = { + "csv/freshman_kgs.csv": 1455, + "csv/freshman_lbs.csv": 1528, + "csv/mlb_players.csv": 45498, + "csv/mlb_teams_2012.csv": 541, +} + + +@pytest.fixture +def http_fs() -> fsspec.AbstractFileSystem: + # a cached listing would mask what each glob actually requests + return fsspec.filesystem("http", use_listings_cache=False) + + +def _by_relative_path(items: List[FileItem]) -> Dict[str, FileItem]: + return {item["relative_path"]: item for item in items} + + +@pytest.mark.serial +def test_glob_http_without_file_info(autoindex_http_server, http_fs) -> None: + """fsspec scrapes http listings from an html index which reports no size, so files list + without one instead of failing.""" + items = _by_relative_path(list(glob_files(http_fs, HTTP_BUCKET_URL, "csv/*.csv"))) + + assert set(items) == set(CSV_SAMPLE_SIZES) + for item in items.values(): + assert item["size_in_bytes"] is None + # no Last-Modified in a listing, mtime falls back to now + assert item["modification_date"].tzinfo is not None + assert (datetime.now(timezone.utc) - item["modification_date"]).total_seconds() < 60 + + +@pytest.mark.serial +def test_glob_http_with_file_info(autoindex_http_server, http_fs) -> None: + """`fetch_file_info` reads size and mtime per file, which is the only place http reports them.""" + items = _by_relative_path( + list(glob_files(http_fs, HTTP_BUCKET_URL, "csv/*.csv", fetch_file_info=True)) + ) + + assert set(items) == set(CSV_SAMPLE_SIZES) + for rel_path, item in items.items(): + assert item["size_in_bytes"] == CSV_SAMPLE_SIZES[rel_path] + # a real Last-Modified is the file's mtime on disk, never a fresh `now` + assert (datetime.now(timezone.utc) - item["modification_date"]).total_seconds() > 60 + + +@pytest.mark.serial +@pytest.mark.parametrize("fetch_file_info", (True, False), ids=("with_info", "without_info")) +def test_glob_http_recursive(autoindex_http_server, http_fs, fetch_file_info: bool) -> None: + items = _by_relative_path( + list(glob_files(http_fs, HTTP_BUCKET_URL, "**/*.csv", fetch_file_info=fetch_file_info)) + ) + + assert set(items) == set(CSV_SAMPLE_SIZES) | { + "met_csv/A801/A881_20230920.csv", + "met_csv/A803/A803_20230919.csv", + "met_csv/A803/A803_20230920.csv", + } + + +@pytest.mark.serial +@pytest.mark.parametrize("fetch_file_info", (True, False), ids=("with_info", "without_info")) +def test_glob_http_single_file(autoindex_http_server, http_fs, fetch_file_info: bool) -> None: + """A glob without wildcards resolves through `info` so it reports size either way.""" + items = list( + glob_files(http_fs, HTTP_BUCKET_URL, "csv/mlb_players.csv", fetch_file_info=fetch_file_info) + ) + + assert len(items) == 1 + assert items[0]["file_name"] == "mlb_players.csv" + assert items[0]["size_in_bytes"] == CSV_SAMPLE_SIZES["csv/mlb_players.csv"] + + +@pytest.mark.serial +def test_glob_http_modification_date_is_stdlib_datetime(autoindex_http_server, http_fs) -> None: + import pendulum + + items = list(glob_files(http_fs, HTTP_BUCKET_URL, "csv/*.csv", fetch_file_info=True)) + + for item in items: + assert isinstance(item["modification_date"], datetime) + assert not isinstance(item["modification_date"], pendulum.DateTime) + assert item["modification_date"].tzinfo is timezone.utc diff --git a/tests/common/storages/utils.py b/tests/common/storages/utils.py index ffe13d5938..ecb44d0553 100644 --- a/tests/common/storages/utils.py +++ b/tests/common/storages/utils.py @@ -1,5 +1,6 @@ import os import glob +from datetime import datetime # noqa: I251 from pathlib import Path from urllib.parse import urlparse import pytest @@ -88,7 +89,7 @@ def assert_sample_files( assert item["file_url"].endswith(item["relative_path"]) assert isinstance(item["mime_type"], str) assert isinstance(item["size_in_bytes"], int) - assert isinstance(item["modification_date"], pendulum.DateTime) + assert isinstance(item["modification_date"], datetime) # create file dict file_dict = FileItemDict(item, fs_client) diff --git a/tests/load/filesystem/test_filesystem_common.py b/tests/load/filesystem/test_filesystem_common.py index 0d40c7ed61..ef79422965 100644 --- a/tests/load/filesystem/test_filesystem_common.py +++ b/tests/load/filesystem/test_filesystem_common.py @@ -134,7 +134,9 @@ def check_file_exists(filedir_: str, file_url_: str): def check_file_changed(file_url_: str): details = filesystem.info(file_url_) assert details["size"] == 11 - assert (MTIME_DISPATCH[config.protocol](details) - now).seconds < 160 + mtime = MTIME_DISPATCH[config.protocol](details) + assert mtime is not None + assert (mtime - now).seconds < 160 bucket_url = os.environ["DESTINATION__FILESYSTEM__BUCKET_URL"] config = get_config() diff --git a/tests/load/sources/filesystem/test_filesystem_source.py b/tests/load/sources/filesystem/test_filesystem_source.py index 8fb0536267..d1d219a3a1 100644 --- a/tests/load/sources/filesystem/test_filesystem_source.py +++ b/tests/load/sources/filesystem/test_filesystem_source.py @@ -1,4 +1,5 @@ import os +from datetime import datetime # noqa: I251 from typing import Any, Dict, List, cast from fsspec import AbstractFileSystem @@ -68,7 +69,7 @@ def assert_sample_content(items: List[FileItemDict]): assert item["size_in_bytes"] == 14 assert item["file_url"].endswith("/samples/sample.txt") assert item["mime_type"] == "text/plain" - assert isinstance(item["modification_date"], pendulum.DateTime) + assert isinstance(item["modification_date"], datetime) yield items diff --git a/tests/normalize/test_json_item_inference.py b/tests/normalize/test_json_item_inference.py index e445c5c065..3756208ace 100644 --- a/tests/normalize/test_json_item_inference.py +++ b/tests/normalize/test_json_item_inference.py @@ -1132,4 +1132,3 @@ def test_variant_column_description_with_bool_variant( assert variant_col["variant"] is True assert variant_col["description"] == "Variant of column value__v_bool generated by dlt" - diff --git a/tests/utils.py b/tests/utils.py index eee3cdc24f..d8fd19c23a 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -201,6 +201,17 @@ def list_directory(self, path: Union[str, PathLike]) -> None: return None +class AutoindexHandler(http.server.SimpleHTTPRequestHandler): + """Serves files and renders an html index for directories, like nginx/apache autoindex.""" + + @classmethod + def factory(cls, *args, directory: Path) -> "AutoindexHandler": + return cls(*args, directory=directory) + + def __init__(self, *args, directory: Optional[Path] = None): + super().__init__(*args, directory=str(directory) if directory else None) + + class MockHttpResponse(Response): def __init__(self, status_code: int) -> None: self.status_code = status_code @@ -266,17 +277,10 @@ def auto_module_test_run_context(auto_module_test_storage) -> Iterator[None]: yield from create_test_run_context() -@pytest.fixture -def public_http_server(): - """ - A simple HTTP server serving files from the current directory. - Used to simulate public CDN. It allows only file access, directory listing is forbidden. - """ +def _serve_sample_files(handler: Any, port: int) -> Iterator[http.server.ThreadingHTTPServer]: httpd = http.server.ThreadingHTTPServer( - ("localhost", 8189), - partial( - PublicCDNHandler.factory, directory=Path.cwd().joinpath("tests/common/storages/samples") - ), + ("localhost", port), + partial(handler.factory, directory=Path.cwd().joinpath("tests/common/storages/samples")), ) server_thread = threading.Thread(target=httpd.serve_forever, daemon=True) server_thread.start() @@ -289,6 +293,24 @@ def public_http_server(): httpd.server_close() +@pytest.fixture +def public_http_server() -> Iterator[http.server.ThreadingHTTPServer]: + """ + A simple HTTP server serving files from the current directory. + Used to simulate public CDN. It allows only file access, directory listing is forbidden. + """ + yield from _serve_sample_files(PublicCDNHandler, 8189) + + +@pytest.fixture +def autoindex_http_server() -> Iterator[http.server.ThreadingHTTPServer]: + """ + Serves the same files as `public_http_server` but renders an html directory index. + fsspec has no listing protocol over http, it scrapes that index to list files. + """ + yield from _serve_sample_files(AutoindexHandler, 8190) + + def create_test_run_context() -> Iterator[None]: # this plugs active context ctx = PluggableRunContext() From adaac6a7a2ef5c63644c468afb6edd160cfa83eb Mon Sep 17 00:00:00 2001 From: rudolfix Date: Sun, 23 Aug 2026 20:09:06 +0200 Subject: [PATCH 2/5] moves http filesystem tests --- .../filesystem}/test_http_filesystem.py | 44 +++++++++---------- 1 file changed, 21 insertions(+), 23 deletions(-) rename tests/{common/storages => load/sources/filesystem}/test_http_filesystem.py (71%) diff --git a/tests/common/storages/test_http_filesystem.py b/tests/load/sources/filesystem/test_http_filesystem.py similarity index 71% rename from tests/common/storages/test_http_filesystem.py rename to tests/load/sources/filesystem/test_http_filesystem.py index 394c4571ad..fbc833f1f7 100644 --- a/tests/common/storages/test_http_filesystem.py +++ b/tests/load/sources/filesystem/test_http_filesystem.py @@ -3,14 +3,16 @@ import fsspec import pytest +from fsspec import AbstractFileSystem +from dlt.common import pendulum from dlt.common.storages.fsspec_filesystem import FileItem, glob_files from tests.utils import autoindex_http_server -HTTP_BUCKET_URL = "http://localhost:8190" +AUTOINDEX_BUCKET_URL = "http://localhost:8190" -# expected sizes of the csv sample files, keyed by relative path +# sizes of the csv sample files served by `autoindex_http_server`, keyed by relative path CSV_SAMPLE_SIZES = { "csv/freshman_kgs.csv": 1455, "csv/freshman_lbs.csv": 1528, @@ -20,7 +22,7 @@ @pytest.fixture -def http_fs() -> fsspec.AbstractFileSystem: +def http_fs() -> AbstractFileSystem: # a cached listing would mask what each glob actually requests return fsspec.filesystem("http", use_listings_cache=False) @@ -30,10 +32,10 @@ def _by_relative_path(items: List[FileItem]) -> Dict[str, FileItem]: @pytest.mark.serial -def test_glob_http_without_file_info(autoindex_http_server, http_fs) -> None: +def test_glob_http_without_file_info(autoindex_http_server, http_fs: AbstractFileSystem) -> None: """fsspec scrapes http listings from an html index which reports no size, so files list without one instead of failing.""" - items = _by_relative_path(list(glob_files(http_fs, HTTP_BUCKET_URL, "csv/*.csv"))) + items = _by_relative_path(list(glob_files(http_fs, AUTOINDEX_BUCKET_URL, "csv/*.csv"))) assert set(items) == set(CSV_SAMPLE_SIZES) for item in items.values(): @@ -44,10 +46,10 @@ def test_glob_http_without_file_info(autoindex_http_server, http_fs) -> None: @pytest.mark.serial -def test_glob_http_with_file_info(autoindex_http_server, http_fs) -> None: +def test_glob_http_with_file_info(autoindex_http_server, http_fs: AbstractFileSystem) -> None: """`fetch_file_info` reads size and mtime per file, which is the only place http reports them.""" items = _by_relative_path( - list(glob_files(http_fs, HTTP_BUCKET_URL, "csv/*.csv", fetch_file_info=True)) + list(glob_files(http_fs, AUTOINDEX_BUCKET_URL, "csv/*.csv", fetch_file_info=True)) ) assert set(items) == set(CSV_SAMPLE_SIZES) @@ -55,13 +57,17 @@ def test_glob_http_with_file_info(autoindex_http_server, http_fs) -> None: assert item["size_in_bytes"] == CSV_SAMPLE_SIZES[rel_path] # a real Last-Modified is the file's mtime on disk, never a fresh `now` assert (datetime.now(timezone.utc) - item["modification_date"]).total_seconds() > 60 + assert isinstance(item["modification_date"], datetime) + assert not isinstance(item["modification_date"], pendulum.DateTime) @pytest.mark.serial @pytest.mark.parametrize("fetch_file_info", (True, False), ids=("with_info", "without_info")) -def test_glob_http_recursive(autoindex_http_server, http_fs, fetch_file_info: bool) -> None: +def test_glob_http_recursive( + autoindex_http_server, http_fs: AbstractFileSystem, fetch_file_info: bool +) -> None: items = _by_relative_path( - list(glob_files(http_fs, HTTP_BUCKET_URL, "**/*.csv", fetch_file_info=fetch_file_info)) + list(glob_files(http_fs, AUTOINDEX_BUCKET_URL, "**/*.csv", fetch_file_info=fetch_file_info)) ) assert set(items) == set(CSV_SAMPLE_SIZES) | { @@ -73,24 +79,16 @@ def test_glob_http_recursive(autoindex_http_server, http_fs, fetch_file_info: bo @pytest.mark.serial @pytest.mark.parametrize("fetch_file_info", (True, False), ids=("with_info", "without_info")) -def test_glob_http_single_file(autoindex_http_server, http_fs, fetch_file_info: bool) -> None: +def test_glob_http_single_file( + autoindex_http_server, http_fs: AbstractFileSystem, fetch_file_info: bool +) -> None: """A glob without wildcards resolves through `info` so it reports size either way.""" items = list( - glob_files(http_fs, HTTP_BUCKET_URL, "csv/mlb_players.csv", fetch_file_info=fetch_file_info) + glob_files( + http_fs, AUTOINDEX_BUCKET_URL, "csv/mlb_players.csv", fetch_file_info=fetch_file_info + ) ) assert len(items) == 1 assert items[0]["file_name"] == "mlb_players.csv" assert items[0]["size_in_bytes"] == CSV_SAMPLE_SIZES["csv/mlb_players.csv"] - - -@pytest.mark.serial -def test_glob_http_modification_date_is_stdlib_datetime(autoindex_http_server, http_fs) -> None: - import pendulum - - items = list(glob_files(http_fs, HTTP_BUCKET_URL, "csv/*.csv", fetch_file_info=True)) - - for item in items: - assert isinstance(item["modification_date"], datetime) - assert not isinstance(item["modification_date"], pendulum.DateTime) - assert item["modification_date"].tzinfo is timezone.utc From ab71570d1299d993c83e4ac71e2cc8f4f4c96198 Mon Sep 17 00:00:00 2001 From: rudolfix Date: Sun, 23 Aug 2026 21:51:08 +0200 Subject: [PATCH 3/5] fixes docs --- .../verified-sources/filesystem/index.md | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/docs/website/docs/dlt-ecosystem/verified-sources/filesystem/index.md b/docs/website/docs/dlt-ecosystem/verified-sources/filesystem/index.md index 8b71511eb9..dd186b5aae 100644 --- a/docs/website/docs/dlt-ecosystem/verified-sources/filesystem/index.md +++ b/docs/website/docs/dlt-ecosystem/verified-sources/filesystem/index.md @@ -366,14 +366,14 @@ Full list of `filesystem` resource parameters: * `files_per_page` - number of files processed at once. The default value is `100`. * `extract_content` - if true, the content of the file will be read and returned in the resource. The default value is `False`. -* `fetch_file_info` - if true, `size_in_bytes` and `modification_date` are read for each listed file that the - listing itself does not report them for. The default value is `False`. +* `fetch_file_info` - if true, dlt reads `size_in_bytes` and `modification_date` that the listing does not + report. The default value is `False`. :::note - Only `http` and `https` need this. There is no listing protocol over HTTP, so fsspec builds the file list by - scraping the HTML index page, which carries neither a size nor a modification date. Without `fetch_file_info`, - such files have `size_in_bytes` set to `None` and `modification_date` set to the time of listing. With it, dlt - makes one extra request per file to read them. + Only the `http` and `https` filesystems need this parameter. HTTP has no listing protocol. fsspec builds + the file list from the HTML index page, which reports neither size nor modification date. Without + `fetch_file_info`, these files have no `size_in_bytes`. Their `modification_date` is the time of the + listing. With `fetch_file_info`, dlt makes one extra request per file to read both values. ::: ### 2. Choose the right reader @@ -617,8 +617,8 @@ filtered_files = filesystem(bucket_url="s3://bucket_name", file_glob="**/*.json" If for some reason you only want to load small files, you can also do that: :::note -Over `http`/`https`, pass `fetch_file_info=True` to the resource. Without it `size_in_bytes` is `None` and the -comparison below fails. +Over `http` and `https`, the resource needs `fetch_file_info=True`. Without `fetch_file_info`, +`size_in_bytes` is missing and the filter below raises `KeyError`. ::: ```py @@ -668,7 +668,7 @@ The filesystem ensures consistent file representation across bucket types and of - `relative_path` - set when doing `glob`, is a relative path to a `bucket_url` argument. - `mime_type` - file's MIME type. It is sourced from the bucket provider or inferred from its extension. - `modification_date` - file's last modification time (format: `datetime.datetime`, always UTC). -- `size_in_bytes` - file size. `None` when the filesystem does not report one, see `fetch_file_info`. +- `size_in_bytes` - file size. Not present when the filesystem does not report it (see `fetch_file_info`). - `file_content` - content, provided upon request. :::info From b57f1469c61aaadc40b40787036ebf220e692428 Mon Sep 17 00:00:00 2001 From: rudolfix Date: Sun, 23 Aug 2026 21:51:24 +0200 Subject: [PATCH 4/5] makes size not required, other fixes --- dlt/common/storages/fsspec_filesystem.py | 7 ++++--- dlt/common/storages/transactional_file.py | 15 ++++++++++----- dlt/sources/filesystem/__init__.py | 5 ++--- tests/common/storages/test_transactional_file.py | 10 ++++++++++ .../sources/filesystem/test_http_filesystem.py | 2 +- 5 files changed, 27 insertions(+), 12 deletions(-) diff --git a/dlt/common/storages/fsspec_filesystem.py b/dlt/common/storages/fsspec_filesystem.py index bfc84f7794..b15388edf2 100644 --- a/dlt/common/storages/fsspec_filesystem.py +++ b/dlt/common/storages/fsspec_filesystem.py @@ -54,7 +54,7 @@ class FileItem(TypedDict): mime_type: str encoding: NotRequired[str] modification_date: datetime - size_in_bytes: Optional[int] + size_in_bytes: NotRequired[int] file_content: NotRequired[bytes] @@ -398,7 +398,7 @@ def glob_files( if md["type"] != "file": continue size, modification_date = md.get("size"), MTIME_DISPATCH[scheme](md) - # http listings are scraped from an html index and carry neither size nor mtime + # if mtime or size are not available, get info for particular file if fetch_file_info and (size is None or modification_date is None): md = fs_client.info(file) size, modification_date = md.get("size"), MTIME_DISPATCH[scheme](md) @@ -424,8 +424,9 @@ def glob_files( file_url=file_url, mime_type=mime_type, modification_date=modification_date, - size_in_bytes=None if size is None else int(size), ) + if size is not None: + file_item["size_in_bytes"] = int(size) if encoding is not None: file_item["encoding"] = encoding yield file_item diff --git a/dlt/common/storages/transactional_file.py b/dlt/common/storages/transactional_file.py index 26cecb638e..5390db6008 100644 --- a/dlt/common/storages/transactional_file.py +++ b/dlt/common/storages/transactional_file.py @@ -102,12 +102,17 @@ def _sync_locks(self) -> t.List[str]: name = lock["name"] if not name.startswith(self.lock_prefix): continue - # Purge stale locks + # purge stale locks mtime = self.extract_mtime(lock) - # a filesystem that reports no mtime cannot expire locks, keep them - if mtime is not None and now - mtime > timedelta( - seconds=TransactionalFile.LOCK_TTL_SECONDS - ): + if mtime is None: + # without an mtime a lock left behind by a crashed holder never expires and + # everybody else waits on it forever + raise RuntimeError( + f"Filesystem `{self._fs.protocol}` reports no modification time for lock file" + f" `{name}` so stale locks cannot be detected. `TransactionalFile` cannot lock" + " on this filesystem." + ) + if now - mtime > timedelta(seconds=TransactionalFile.LOCK_TTL_SECONDS): try: # Janitors can race, so we ignore errors self._fs.rm(name) except OSError: diff --git a/dlt/sources/filesystem/__init__.py b/dlt/sources/filesystem/__init__.py index 282ebba032..1844e61f8a 100644 --- a/dlt/sources/filesystem/__init__.py +++ b/dlt/sources/filesystem/__init__.py @@ -115,9 +115,8 @@ def filesystem( # noqa DOC files_per_page (int, optional): The number of files to process at once, defaults to 100. extract_content (bool, optional): If true, the content of the file will be extracted if false it will return a fsspec file, defaults to False. - fetch_file_info (bool, optional): If true, `size_in_bytes` and `modification_date` are read - per file for filesystems whose listing omits them (http), at the cost of one request per - file. Without it such files report `size_in_bytes` as None, defaults to False. + fetch_file_info (bool, optional): If true, `dlt` gets missing `size_in_bytes` and `modification_date` from file details. This + requires a server call per file. Currently needed by `http` filesystem only. Defaults to False. kwargs (Optional[Dict[str, Any]]): Additional arguments passed to fsspec constructor ie. dict(use_ssl=True) for s3fs client_kwargs (Optional[Dict[str, Any]]): Additional arguments passed to underlying fsspec native client ie. dict(verify="public.crt) for botocore incremental (Optional[dlt.sources.incremental[Any]]): Defines incremental cursor on listed files, with `modification_date` diff --git a/tests/common/storages/test_transactional_file.py b/tests/common/storages/test_transactional_file.py index 7afdf10c38..6c0b1deaa0 100644 --- a/tests/common/storages/test_transactional_file.py +++ b/tests/common/storages/test_transactional_file.py @@ -185,3 +185,13 @@ def test_file_transaction_directory(fs: fsspec.AbstractFileSystem): writer.write(b"test 1") writer.release_lock() + + +def test_lock_requires_mtime(fs: fsspec.AbstractFileSystem, file_name: str): + """A filesystem without mtime cannot expire stale locks, so locking must fail loudly + instead of waiting on a lock no one will ever release.""" + writer = TransactionalFile(file_name, fs) + writer.extract_mtime = lambda _: None + + with pytest.raises(RuntimeError, match="no modification time"): + writer.acquire_lock() diff --git a/tests/load/sources/filesystem/test_http_filesystem.py b/tests/load/sources/filesystem/test_http_filesystem.py index fbc833f1f7..dbe7cc4ea8 100644 --- a/tests/load/sources/filesystem/test_http_filesystem.py +++ b/tests/load/sources/filesystem/test_http_filesystem.py @@ -39,7 +39,7 @@ def test_glob_http_without_file_info(autoindex_http_server, http_fs: AbstractFil assert set(items) == set(CSV_SAMPLE_SIZES) for item in items.values(): - assert item["size_in_bytes"] is None + assert "size_in_bytes" not in item # no Last-Modified in a listing, mtime falls back to now assert item["modification_date"].tzinfo is not None assert (datetime.now(timezone.utc) - item["modification_date"]).total_seconds() < 60 From e62dfbddf0857195f01d042700621547ce29ba5f Mon Sep 17 00:00:00 2001 From: rudolfix Date: Mon, 24 Aug 2026 23:03:15 +0200 Subject: [PATCH 5/5] review fixes --- dlt/sources/filesystem/__init__.py | 6 ++-- .../filesystem/test_http_filesystem.py | 10 +++++-- tests/utils.py | 28 ++++--------------- 3 files changed, 17 insertions(+), 27 deletions(-) diff --git a/dlt/sources/filesystem/__init__.py b/dlt/sources/filesystem/__init__.py index 1844e61f8a..89e1eece1f 100644 --- a/dlt/sources/filesystem/__init__.py +++ b/dlt/sources/filesystem/__init__.py @@ -100,10 +100,10 @@ def filesystem( # noqa DOC file_glob: str = "*", files_per_page: int = DEFAULT_CHUNK_SIZE, extract_content: bool = False, - fetch_file_info: bool = False, kwargs: Optional[Dict[str, Any]] = None, client_kwargs: Optional[Dict[str, Any]] = None, incremental: Optional[dlt.sources.incremental[Any]] = None, + fetch_file_info: bool = False, ) -> Iterator[List[FileItem]]: """This resource lists files in `bucket_url` using `file_glob` pattern. The files are yielded as FileItem which also provide methods to open and read file data. It should be combined with transformers that further process (ie. load files) @@ -115,12 +115,12 @@ def filesystem( # noqa DOC files_per_page (int, optional): The number of files to process at once, defaults to 100. extract_content (bool, optional): If true, the content of the file will be extracted if false it will return a fsspec file, defaults to False. - fetch_file_info (bool, optional): If true, `dlt` gets missing `size_in_bytes` and `modification_date` from file details. This - requires a server call per file. Currently needed by `http` filesystem only. Defaults to False. kwargs (Optional[Dict[str, Any]]): Additional arguments passed to fsspec constructor ie. dict(use_ssl=True) for s3fs client_kwargs (Optional[Dict[str, Any]]): Additional arguments passed to underlying fsspec native client ie. dict(verify="public.crt) for botocore incremental (Optional[dlt.sources.incremental[Any]]): Defines incremental cursor on listed files, with `modification_date` being the most common choice that returns only files created from the previous run. + fetch_file_info (bool, optional): If true, `dlt` gets missing `size_in_bytes` and `modification_date` from file details. This + requires a server call per file. Currently needed by `http` filesystem only. Defaults to False. Yields: List[FileItem]: The list of files. diff --git a/tests/load/sources/filesystem/test_http_filesystem.py b/tests/load/sources/filesystem/test_http_filesystem.py index dbe7cc4ea8..e2ba3ee703 100644 --- a/tests/load/sources/filesystem/test_http_filesystem.py +++ b/tests/load/sources/filesystem/test_http_filesystem.py @@ -1,3 +1,4 @@ +import os from datetime import datetime, timezone # noqa: I251 from typing import Dict, List @@ -8,6 +9,7 @@ from dlt.common import pendulum from dlt.common.storages.fsspec_filesystem import FileItem, glob_files +from tests.common.storages.utils import TEST_SAMPLE_FILES from tests.utils import autoindex_http_server AUTOINDEX_BUCKET_URL = "http://localhost:8190" @@ -55,8 +57,12 @@ def test_glob_http_with_file_info(autoindex_http_server, http_fs: AbstractFileSy assert set(items) == set(CSV_SAMPLE_SIZES) for rel_path, item in items.items(): assert item["size_in_bytes"] == CSV_SAMPLE_SIZES[rel_path] - # a real Last-Modified is the file's mtime on disk, never a fresh `now` - assert (datetime.now(timezone.utc) - item["modification_date"]).total_seconds() > 60 + # the server sends `Last-Modified` off the sample file mtime, truncated to whole seconds, + # so we get that back and not a fresh `now` + on_disk = datetime.fromtimestamp( + os.path.getmtime(os.path.join(TEST_SAMPLE_FILES, rel_path)), timezone.utc + ) + assert 0 <= (on_disk - item["modification_date"]).total_seconds() < 1 assert isinstance(item["modification_date"], datetime) assert not isinstance(item["modification_date"], pendulum.DateTime) diff --git a/tests/utils.py b/tests/utils.py index d8fd19c23a..754d245db4 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -10,7 +10,7 @@ from functools import partial from os import environ from pathlib import Path -from typing import Any, Dict, Iterable, Iterator, Literal, Optional, Union, get_args, List +from typing import Any, Dict, Iterable, Iterator, Literal, Optional, Type, Union, get_args, List from unittest.mock import patch import pytest @@ -189,29 +189,11 @@ def TEST_DICT_CONFIG_PROVIDER(): class PublicCDNHandler(http.server.SimpleHTTPRequestHandler): - @classmethod - def factory(cls, *args, directory: Path) -> "PublicCDNHandler": - return cls(*args, directory=directory) - - def __init__(self, *args, directory: Optional[Path] = None): - super().__init__(*args, directory=str(directory) if directory else None) - def list_directory(self, path: Union[str, PathLike]) -> None: self.send_error(HTTPStatus.FORBIDDEN, "Directory listing is forbidden") return None -class AutoindexHandler(http.server.SimpleHTTPRequestHandler): - """Serves files and renders an html index for directories, like nginx/apache autoindex.""" - - @classmethod - def factory(cls, *args, directory: Path) -> "AutoindexHandler": - return cls(*args, directory=directory) - - def __init__(self, *args, directory: Optional[Path] = None): - super().__init__(*args, directory=str(directory) if directory else None) - - class MockHttpResponse(Response): def __init__(self, status_code: int) -> None: self.status_code = status_code @@ -277,10 +259,12 @@ def auto_module_test_run_context(auto_module_test_storage) -> Iterator[None]: yield from create_test_run_context() -def _serve_sample_files(handler: Any, port: int) -> Iterator[http.server.ThreadingHTTPServer]: +def _serve_sample_files( + handler: Type[http.server.SimpleHTTPRequestHandler], port: int +) -> Iterator[http.server.ThreadingHTTPServer]: httpd = http.server.ThreadingHTTPServer( ("localhost", port), - partial(handler.factory, directory=Path.cwd().joinpath("tests/common/storages/samples")), + partial(handler, directory=Path.cwd().joinpath("tests/common/storages/samples")), ) server_thread = threading.Thread(target=httpd.serve_forever, daemon=True) server_thread.start() @@ -308,7 +292,7 @@ def autoindex_http_server() -> Iterator[http.server.ThreadingHTTPServer]: Serves the same files as `public_http_server` but renders an html directory index. fsspec has no listing protocol over http, it scrapes that index to list files. """ - yield from _serve_sample_files(AutoindexHandler, 8190) + yield from _serve_sample_files(http.server.SimpleHTTPRequestHandler, 8190) def create_test_run_context() -> Iterator[None]: