IPv6

[amotl]

About

On another PR, a readiness question about IPv6 came up. While everything should work with IPv6 in theory, some value validators or converters that touch host names or IPv4 fields might need a bit of adjustment.

Thoughts

Optimally, omniload would demonstrate readiness by adding a few software tests in this regard?

[amotl]

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Missing edge-case coverage for _endpoint_namespace's IPv6 and no-hostname branches.

Both _endpoint_namespace tests only exercise plain hostnames. The IPv6-bracketing branch (if ":" in host: host = f"[{host}]") and the "no hostname" fallback (e.g. a malformed or path-only endpoint) in src/dlt_filesystem/source/impl/remote.py (lines 26-27, 30-31) aren't covered. Since this helper directly feeds storage_namespace, which is hashed into the persistent incremental cursor key, an untested regression here could silently change/collide cursor identity for IPv6-hosted S3-compatible endpoints.

As per path instructions for tests/**/test_*.py: "Prefer flagging missing edge-case coverage over style nits."

🧪 Suggested additional test cases

 def test_endpoint_namespace_normalizes_bare_host_and_url_forms():
     assert _endpoint_namespace("account.blob.example", "default") == (
         _endpoint_namespace("https://account.blob.example", "default")
     )
+
+
+def test_endpoint_namespace_brackets_ipv6_hosts():
+    assert _endpoint_namespace("https://[::1]:9000/bucket", "default") == "[::1]:9000/bucket"
+
+
+def test_endpoint_namespace_falls_back_when_hostname_is_missing():
+    assert _endpoint_namespace("/just/a/path", "default") == "default"

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

def test_endpoint_namespace_excludes_credentials_and_query_values():
    assert (
        _endpoint_namespace(
            "https://user:password@MINIO.example:9000/storage/?token=secret",
            "default",
        )
        == "minio.example:9000/storage"
    )


def test_endpoint_namespace_normalizes_bare_host_and_url_forms():
    assert _endpoint_namespace("account.blob.example", "default") == (
        _endpoint_namespace("https://account.blob.example", "default")
    )


def test_endpoint_namespace_brackets_ipv6_hosts():
    assert _endpoint_namespace("https://[::1]:9000/bucket", "default") == "[::1]:9000/bucket"


def test_endpoint_namespace_falls_back_when_hostname_is_missing():
    assert _endpoint_namespace("/just/a/path", "default") == "default"

🤖 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/test_source_incremental.py` around lines 191 - 204, Add
focused tests for _endpoint_namespace covering an IPv6 host and an endpoint with
no hostname, such as a path-only or malformed URL. Assert the IPv6 result
preserves bracketed host formatting and the no-hostname case follows the
helper’s existing fallback behavior, using the same default namespace context as
the current tests.

Source: Path instructions

Originally posted by @coderabbitai[bot] in #239 (comment)