Filesystem integration tests - #249
Conversation
|
Warning Review limit reached
Next review available in: 35 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
WalkthroughRemote GCS and Azure filesystem construction now normalizes credentials, supports Azure connection strings, and preserves destination compatibility. New emulator-backed integration tests validate S3, Azure, and GCS ingestion into DuckDB. Dependency constraints and logging output were also updated. ChangesRemote filesystem support
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant IntegrationTest
participant CloudEmulator
participant run_ingest
participant DuckDB
IntegrationTest->>CloudEmulator: provision source asset
IntegrationTest->>run_ingest: ingest emulator endpoint
run_ingest->>CloudEmulator: read CSV asset
run_ingest->>DuckDB: load records
IntegrationTest->>DuckDB: verify 20 rows
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
2549e80 to
f7e8af6
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #249 +/- ##
==========================================
+ Coverage 55.58% 55.71% +0.12%
==========================================
Files 210 210
Lines 9871 9894 +23
==========================================
+ Hits 5487 5512 +25
+ Misses 4384 4382 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
f7e8af6 to
2697d90
Compare
91a03c2 to
58d993c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/dlt_filesystem/util/auth.py (1)
46-89: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
connection_stringmode silently ignores conflicting credentials and the docstring wasn't updated.The early return at Line 75-79 skips all subsequent parsing/validation once
connection_stringis supplied. If a caller also passesaccount_key,sas_token, or a partial service-principal triplet alongsideconnection_string, those values are silently dropped with no conflict error, unlike the existing account-key/SAS vs. service-principal conflict checks below. The docstring (lines 54-59) still only documents two auth modes and omitsconnection_stringas a third mode.🛡️ Proposed fix: validate that no other credential material is mixed with connection_string
connection_string = one("connection_string") api_version = one("api_version") if connection_string is not None: + conflicting = [ + k for k in ("account_name", "account_key", "sas_token", *AZURE_SERVICE_PRINCIPAL_FIELDS) + if one(k) is not None + ] + if conflicting: + raise ValueError( + "Conflicting Azure credentials: connection_string cannot be combined " + f"with {', '.join(conflicting)}." + ) return AzureBlobAuth( connection_string=connection_string, api_version=api_version, )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/dlt_filesystem/util/auth.py` around lines 46 - 89, Update the credential parsing function around the connection_string early return to document connection_string as a third auth mode and validate that no account_key, sas_token, or service-principal fields are supplied alongside it. Raise the existing conflict ValueError for any mixed credential material before returning AzureBlobAuth; preserve the current connection_string-only behavior and validation for the other auth modes.src/dlt_filesystem/target/remote.py (1)
156-192: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReject
connection_stringin the Azure destination before constructing credentials.
parse_azure_blob_auth()returns anAzureBlobAuthwith onlyconnection_stringwhen that query param is supplied, leavingis_service_principalfalse and everyaccount_name,account_key,sas_token, andaccount_hostguard skipped. The code then returns a blankAzureCredentials(), while dlt’s Azure credential specs do not exposeconnection_string. Raise a clear unsupported-credential error for destination URIs usingconnection_string.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/dlt_filesystem/target/remote.py` around lines 156 - 192, Update credentials to detect when parse_azure_blob_auth returns a connection_string-only authentication configuration and raise a clear unsupported-credential error before constructing AzureServicePrincipalCredentials or AzureCredentials. Preserve the existing service-principal and account-key mappings for supported authentication fields.
🧹 Nitpick comments (6)
tests/main/filesystem/test_remote_integration.py (3)
41-54: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
"2025-11-05"API-version literal is duplicated between the monkeypatch and the test call.The hardcoded version in
adlfs_patch(Line 47) must stay in sync with theapi_version=2025-11-05query param intest_azure_source(Line 133) purely by convention; a shared module-level constant would prevent drift if one is updated without the other.Also applies to: 128-133
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/main/filesystem/test_remote_integration.py` around lines 41 - 54, Define a shared module-level constant for the Azure API version and reuse it in both _create_aio_blob_service_client_from_connection_string_with_api_version and the api_version query parameter in test_azure_source, removing the duplicated literal while preserving the existing value.
98-106: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueSQL built via f-string flagged by both Ruff (S608) and OpenGrep.
table_nameis only ever a hardcoded literal from within this same file today, so there's no attacker-controlled input, but the pattern is easy to copy into less-controlled code later. Consider quoting the identifier defensively.🛡️ Proposed fix
def duckdb_table_cardinality(db_path: Path, table_name: str) -> int: """Return number of records in database table.""" import duckdb db = duckdb.connect(db_path) - result = db.execute(f"SELECT * FROM {table_name}").fetchall() + quoted = ".".join(f'"{part}"' for part in table_name.split(".")) + result = db.execute(f"SELECT * FROM {quoted}").fetchall() count = len(result) db.close() return count🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/main/filesystem/test_remote_integration.py` around lines 98 - 106, Update duckdb_table_cardinality to quote or safely escape the table_name identifier before constructing the SQL query, avoiding direct interpolation of the raw value while preserving the existing row-count behavior.Source: Linters/SAST tools
62-71: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReading the fixture CSV via
read_text()uses implicit/locale-dependent encoding.Unlike the
floci/gcsfakeserverfixtures which upload the file directly (upload_file/upload_from_filename), this fixture loads content viaPath(...).read_text()without an explicitencoding=, which is platform/locale dependent and can differ from the other two fixtures' behavior across CI environments.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/main/filesystem/test_remote_integration.py` around lines 62 - 71, Update the fixture upload in the test setup around BlobServiceClient and cc.upload_blob to read create_replace.csv with an explicit UTF-8 encoding, matching the deterministic file handling used by the other fixtures.tests/dlt_filesystem/gcs.py (1)
30-46: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDoctest example references an undefined variable.
The example defines
endpoint_url(Line 37) but then usesconnection_string(Line 41) inclient_options, which is never defined in this snippet. As per path instructions for test files ("Prefer flagging missing edge-case coverage over style nits"), this is a functional-correctness slip in the example rather than a style nit — running this as a doctest would raiseNameError.📝 Proposed fix
>>> with GCSFakeServerContainer() as container: ... endpoint_url = container.get_endpoint_url() ... client = google.cloud.storage.Client( ... credentials=AnonymousCredentials(), ... project="test", - ... client_options={"api_endpoint": connection_string}, + ... client_options={"api_endpoint": endpoint_url}, ... )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/dlt_filesystem/gcs.py` around lines 30 - 46, Update the doctest example to pass the defined endpoint_url variable to google.cloud.storage.Client via client_options instead of the undefined connection_string variable. Keep the rest of the GCSFakeServerContainer setup and upload flow unchanged.Source: Path instructions
src/dlt_filesystem/source/impl/remote.py (2)
67-83: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMerged query params can silently override control kwargs with un-typed strings.
Line 71 unconditionally merges every remaining URI query param into
kwargs, after the caller's own**kwargswere already set. If a URI ever includes a query key that collides with an internal control kwarg (e.g.filesystem_incremental,column_types), the raw string value overwrites the caller-supplied typed value — and since any non-empty string (including"false") is truthy,filesystem_incremental=falsein a URI would still enable incrementality. The existingif "token" not in kwargsguard shows the pattern was already considered fortoken; the same precedence should apply broadly.♻️ Proposed fix: don't let query params clobber already-set control kwargs
- # Merge params into fs kwargs. - kwargs.update({key: value[0] for key, value in params.items()}) + # Merge params into fs kwargs, without overriding kwargs already + # supplied by the caller (e.g. filesystem_incremental, column_types). + for key, value in params.items(): + kwargs.setdefault(key, value[0])🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/dlt_filesystem/source/impl/remote.py` around lines 67 - 83, Update the parameter merge in the remote filesystem initialization flow to preserve caller-supplied kwargs: when converting remaining query parameters, only add keys that are not already present in kwargs. Keep the existing token handling and credential precedence intact, including typed control values such as filesystem_incremental and column_types.
172-198: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale docstring:
_azure_kwargsno longer builds a filesystem.The docstring still says "Build an
adlfs.AzureBlobFileSystemfrom resolved Azure auth params", but per the AI summary and the code the function now only returns a kwargs dict; construction happens separately viaself.fs_class(**kwargs). Worth updating to avoid confusing future readers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/dlt_filesystem/source/impl/remote.py` around lines 172 - 198, The docstring for _azure_kwargs inaccurately describes filesystem construction; update it to state that the function builds and returns keyword arguments from resolved Azure authentication parameters. Keep the existing parameter-forwarding details and deferred-import explanation accurate, without implying that AzureBlobFileSystem is instantiated here.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/dlt_filesystem/target/remote.py`:
- Around line 156-192: Update credentials to detect when parse_azure_blob_auth
returns a connection_string-only authentication configuration and raise a clear
unsupported-credential error before constructing
AzureServicePrincipalCredentials or AzureCredentials. Preserve the existing
service-principal and account-key mappings for supported authentication fields.
In `@src/dlt_filesystem/util/auth.py`:
- Around line 46-89: Update the credential parsing function around the
connection_string early return to document connection_string as a third auth
mode and validate that no account_key, sas_token, or service-principal fields
are supplied alongside it. Raise the existing conflict ValueError for any mixed
credential material before returning AzureBlobAuth; preserve the current
connection_string-only behavior and validation for the other auth modes.
---
Nitpick comments:
In `@src/dlt_filesystem/source/impl/remote.py`:
- Around line 67-83: Update the parameter merge in the remote filesystem
initialization flow to preserve caller-supplied kwargs: when converting
remaining query parameters, only add keys that are not already present in
kwargs. Keep the existing token handling and credential precedence intact,
including typed control values such as filesystem_incremental and column_types.
- Around line 172-198: The docstring for _azure_kwargs inaccurately describes
filesystem construction; update it to state that the function builds and returns
keyword arguments from resolved Azure authentication parameters. Keep the
existing parameter-forwarding details and deferred-import explanation accurate,
without implying that AzureBlobFileSystem is instantiated here.
In `@tests/dlt_filesystem/gcs.py`:
- Around line 30-46: Update the doctest example to pass the defined endpoint_url
variable to google.cloud.storage.Client via client_options instead of the
undefined connection_string variable. Keep the rest of the
GCSFakeServerContainer setup and upload flow unchanged.
In `@tests/main/filesystem/test_remote_integration.py`:
- Around line 41-54: Define a shared module-level constant for the Azure API
version and reuse it in both
_create_aio_blob_service_client_from_connection_string_with_api_version and the
api_version query parameter in test_azure_source, removing the duplicated
literal while preserving the existing value.
- Around line 98-106: Update duckdb_table_cardinality to quote or safely escape
the table_name identifier before constructing the SQL query, avoiding direct
interpolation of the raw value while preserving the existing row-count behavior.
- Around line 62-71: Update the fixture upload in the test setup around
BlobServiceClient and cc.upload_blob to read create_replace.csv with an explicit
UTF-8 encoding, matching the deterministic file handling used by the other
fixtures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 236e0d25-f933-4073-b547-7d47a81cec84
📒 Files selected for processing (9)
pyproject.tomlsrc/dlt_filesystem/source/impl/remote.pysrc/dlt_filesystem/target/remote.pysrc/dlt_filesystem/util/auth.pysrc/omniload/source/docebo/adapter.pysrc/omniload/source/github/adapter.pytests/dlt_filesystem/gcs.pytests/dlt_filesystem/test_source_incremental.pytests/main/filesystem/test_remote_integration.py
💤 Files with no reviewable changes (1)
- src/omniload/source/docebo/adapter.py
58d993c to
c1d6109
Compare
1 participant
About
Add a few integration test cases that use remote filesystems.
References