feat(sdk): enforce s3:// URIs for string S3 params + ingestion (EL) docs (#9912)

* feat(pipelines): ingestion (EL) templates + docs Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(pipelines): review nits — draft collision guard, template-mode selection reset, invariant test Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(pipelines): lead the insert menu with ingestion templates Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * refactor(pipelines): ingestion story as docs-only — drop editor template UI The insert-menu template section mixed two selection grammars in one popover and confused more than it helped. The three E2E-verified example pipelines now live verbatim in docs/pipeline-ingestion.md; the Python bare-string S3 key fix in pipelineTemplates.ts stays. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(sdk): bare string S3 keys in py/ts clients + asset parsers A plain string passed where an S3Object is expected is now a bare key in the default storage — previously the py client silently degraded it to s3="" (auto-generated key) and both asset parsers canonicalized it without the leading slash, splitting lineage. parseS3Object moves to s3Types.ts so it is unit-testable without the generated services. The pipeline template fix from the earlier commit is superseded (bare strings are the supported spelling again); docs examples flipped to bare keys. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * refactor(sdk): enforce s3:// URIs for string S3Object params Bare strings now raise/throw with a hint pointing at the s3:///<key> spelling instead of being treated as keys (previous commit) or silently degrading to an empty key (original behavior). One string spelling everywhere: SDK calls, // on annotations, and DuckDB SQL all use s3:///<key>. TS regains the s3://-template-literal type; the asset parsers record no asset for a bare string (the call can only error); templates emit the URI form. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs(pipelines): move ingestion (EL) guide to windmilldocs, keep design constraints User-facing how-to (engine choice, cursor recipes, schema drift, worked examples) moves to windmilldocs core_concepts/63_pipelines (windmilldocs#1462); the repo keeps only the design constraints future feature work must not break, as a section of ducklake-materialization.md. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * chore: regenerate system prompts after parse_s3_object docstring change Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(sdk): reject empty-key s3 URIs; align asset parsers with the runtime rule Addresses CI review: s3:/// and s3://bucket/ now raise (an empty key would fall back to the auto-generated-key path the strict contract exists to prevent); the asset parsers' string branch applies the same valid-URI-with-non-empty-key rule so no R/W edge is recorded for a call that can only error (the generic URI-literal scan still records ambiguous access-None assets, by design); comments rephrased as current constraints per AGENTS.md. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
2026-08-18 16:02:10 +00:00 · 2026-07-04 16:07:39 +00:00
parent 33521505db
commit 5ad2de91a2
19 changed files with 417 additions and 93 deletions
@@ -124,5 +124,49 @@ SET s3_secret_access_key='80yMndIMcyXwEujxVNINQbf0tBlIzRaLPyM2m1n4';
            wmill.load_s3_file(s3_obj)


+class TestParseS3Object(unittest.TestCase):
+    """Pure-unit tests for parse_s3_object — no network/env needed."""
+
+    def test_bare_string_raises_with_uri_hint(self):
+        # A bare key is rejected rather than silently uploading under an
+        # auto-generated name; the error points at the s3:/// spelling.
+        with self.assertRaisesRegex(ValueError, "s3:///dir/file.json"):
+            wmill.parse_s3_object("dir/file.json")
+
+    def test_triple_slash_uri_is_default_storage(self):
+        self.assertEqual(
+            wmill.parse_s3_object("s3:///dir/file.json"),
+            S3Object(s3="dir/file.json", storage=None),
+        )
+
+    def test_full_uri_splits_storage_and_key(self):
+        self.assertEqual(
+            wmill.parse_s3_object("s3://bucket/dir/f"),
+            S3Object(s3="dir/f", storage="bucket"),
+        )
+
+    def test_malformed_uri_raises(self):
+        # `s3://x` has no key part — fail loudly instead of silently
+        # misplacing the object.
+        with self.assertRaises(ValueError):
+            wmill.parse_s3_object("s3://broken")
+
+    def test_empty_key_uri_raises(self):
+        # An empty key is never a valid target: it would fall back to an
+        # auto-generated key, which is requested by omitting the object.
+        with self.assertRaises(ValueError):
+            wmill.parse_s3_object("s3:///")
+        with self.assertRaises(ValueError):
+            wmill.parse_s3_object("s3://bucket/")
+
+    def test_empty_string_raises(self):
+        # Auto-generated keys are requested by omitting the object (None),
+        # not by an empty string.
+        with self.assertRaises(ValueError):
+            wmill.parse_s3_object("")
+
+    def test_s3object_passes_through(self):
+        self.assertEqual(wmill.parse_s3_object(S3Object(s3="x")), S3Object(s3="x"))
+
 if __name__ == "__main__":
    unittest.main()
@@ -2212,12 +2212,27 @@ def parse_resource_syntax(s: str) -> Optional[str]:
    return None

 def parse_s3_object(s3_object: S3Object | str) -> S3Object:
-    """Parse S3 object from string or S3Object format."""
+    """Parse S3 object from a `s3://<storage>/<key>` URI string (`s3:///<key>`
+    for the default storage) or S3Object format. Any other string raises
+    rather than falling back to an auto-generated key: an auto key is
+    requested by omitting the object, and a fallback would silently misplace
+    the upload on any typo.
+    """
    if isinstance(s3_object, str):
-        match = re.match(r'^s3://([^/]*)/(.*)$', s3_object)
+        match = re.match(r'^s3://([^/]*)/(.+)$', s3_object)
        if match:
-            return S3Object(s3=match.group(2) or "", storage=match.group(1) or None)
-        return S3Object(s3="")
+            return S3Object(s3=match.group(2), storage=match.group(1) or None)
+        if s3_object.startswith("s3://"):
+            raise ValueError(
+                f"Invalid s3 object URI {s3_object!r}: expected "
+                "s3://<storage>/<key> with a non-empty key "
+                "(s3:///<key> for the default storage)"
+            )
+        raise ValueError(
+            f"Invalid s3 object {s3_object!r}: expected an s3://<storage>/<key> "
+            f"URI (e.g. 's3:///{s3_object}' for key {s3_object!r} in the default "
+            "storage) or S3Object(s3=<key>)"
+        )
    else:
        return s3_object