This is an automated email from the ASF dual-hosted git repository. github-merge-queue[bot] pushed a commit to branch gh-readonly-queue/main/pr-7622-1b75787631ba4cae44473d2131168cd1d2ef1de3 in repository https://gitbox.apache.org/repos/asf/texera.git
commit baefb5d5984c86a78b829167ae2344996f2ef5a2 Author: Tanishq Gandhi <[email protected]> AuthorDate: Thu Aug 13 05:24:05 2026 +0000 feat(storage): accept legacy unprefixed dataset paths for backward compatibility (#7622) ### What changes were proposed in this PR? #6502 made the `datasets` resource-type prefix **required** on dataset logical paths. This PR makes the readers accept **both** forms again, so the prefix becomes required only once the ML-model work has landed and every stored path has been migrated. ``` prefixed (target): /datasets/<owner>/<name>/<version>/<file> legacy (accepted): /<owner>/<name>/<version>/<file> ``` Motivation: `sql/updates/36.sql` rewrites the `fileName` and `datasetVersionPath` operator properties, but it cannot rewrite a path a user hardcoded inside a Python UDF (that lives in the operator's `code` property). Those paths worked before #6502 and started raising afterwards. Rewriting user source in a migration would be unsafe, so the readers tolerate the legacy form during the transition instead. Disambiguation: a leading segment that names a known `ResourceType` commits to the prefixed form, so `/datasets/<owner>/<name>/<version>` (too few segments) is rejected rather than silently re-read as a legacy path with owner `datasets`. Each tolerant branch carries a `TODO(datasets-prefix)` marker so the fallback can be removed in one pass. ### Any related issues, documentation, discussions? Follow-up to #6502 Part of the ML-model resource work tracked in #6495. ### How was this PR tested? Updated and added unit tests, all passing locally: Also verified end-to-end against a local instance, using workflows whose stored paths had the prefix stripped: - CSV File Scan on `/<owner>/reviews/v1/reviews.csv` ran green and emitted rows, as did two further scan workflows. - A Python UDF calling `DatasetFileDocument("/<owner>/iris/v2/Iris.csv")` parsed the legacy path successfully ### Was this PR authored or co-authored using generative AI tooling? Generated-by: Claude Code (Claude Opus 4.8) --------- Co-authored-by: ali <[email protected]> Co-authored-by: Claude Opus 4.8 <[email protected]> --- .../pytexera/storage/dataset_file_document.py | 34 +++++++++++++--------- .../pytexera/storage/test_dataset_file_document.py | 28 ++++++++++++------ .../texera/amber/core/storage/FileResolver.scala | 18 +++++------- .../texera/amber/storage/FileResolverSpec.scala | 24 ++++++++------- .../source/dataset/FileListerSourceOpExec.scala | 16 ++++++---- .../dataset/FileListerSourceOpExecSpec.scala | 34 +++++++++++++++++----- .../dataset-selection-modal.component.spec.ts | 10 +++++++ .../dataset-selection-modal.component.ts | 8 +++-- 8 files changed, 114 insertions(+), 58 deletions(-) diff --git a/amber/src/main/python/pytexera/storage/dataset_file_document.py b/amber/src/main/python/pytexera/storage/dataset_file_document.py index 82c1c1ae91..4446da5ee5 100644 --- a/amber/src/main/python/pytexera/storage/dataset_file_document.py +++ b/amber/src/main/python/pytexera/storage/dataset_file_document.py @@ -65,22 +65,28 @@ class DatasetFileDocument: """ parts = file_path.strip("/").split("/") - if len(parts) < 5: - raise ValueError( - "Invalid file path format. Expected: " - "/datasets/ownerEmail/datasetName/versionName/fileRelativePath" - ) + invalid_format = ValueError( + "Invalid file path format. Expected: " + "/datasets/ownerEmail/datasetName/versionName/fileRelativePath" + ) - # Validate the leading prefix against the known resource types. - try: + # TODO(datasets-prefix): require the prefix once all stored paths are migrated (36.sql) and ml model support work is completed. + if parts and parts[0] in {t.value for t in ResourceType}: + if len(parts) < 5: + raise invalid_format self.resource_type = ResourceType(parts[0]) - except ValueError: - raise ValueError(f"Unknown resource type prefix: {parts[0]!r}") - - self.owner_email = parts[1] - self.dataset_name = parts[2] - self.version_name = parts[3] - self.file_relative_path = "/".join(parts[4:]) + self.owner_email = parts[1] + self.dataset_name = parts[2] + self.version_name = parts[3] + self.file_relative_path = "/".join(parts[4:]) + elif len(parts) >= 4: + self.resource_type = ResourceType.DATASETS + self.owner_email = parts[0] + self.dataset_name = parts[1] + self.version_name = parts[2] + self.file_relative_path = "/".join(parts[3:]) + else: + raise invalid_format self.jwt_token = os.getenv("USER_JWT_TOKEN") self.presign_endpoint = os.getenv("FILE_SERVICE_GET_PRESIGNED_URL_ENDPOINT") diff --git a/amber/src/test/python/pytexera/storage/test_dataset_file_document.py b/amber/src/test/python/pytexera/storage/test_dataset_file_document.py index 284a75fbfe..760e6234be 100644 --- a/amber/src/test/python/pytexera/storage/test_dataset_file_document.py +++ b/amber/src/test/python/pytexera/storage/test_dataset_file_document.py @@ -60,20 +60,30 @@ class TestDatasetFileDocumentInit: assert doc.owner_email == "[email protected]" assert doc.file_relative_path == "file.csv" - def test_rejects_unprefixed_path(self, auth_env): - # Without the datasets prefix the path is not a dataset path. - with pytest.raises(ValueError, match="Invalid file path format"): - DatasetFileDocument("/[email protected]/ds/v1/file.csv") + def test_accepts_legacy_unprefixed_path(self, auth_env): + doc = DatasetFileDocument("/[email protected]/ds/v1/file.csv") + assert doc.owner_email == "[email protected]" + assert doc.dataset_name == "ds" + assert doc.version_name == "v1" + assert doc.file_relative_path == "file.csv" + + def test_legacy_unprefixed_path_keeps_nested_relative_path(self, auth_env): + doc = DatasetFileDocument("/[email protected]/ds/v1/a/b/file.csv") + assert doc.file_relative_path == "a/b/file.csv" - def test_rejects_unknown_resource_type_prefix(self, auth_env): - # A leading segment that is not a known resource type is rejected. - with pytest.raises(ValueError, match="Unknown resource type prefix"): - DatasetFileDocument("/notAResourceType/[email protected]/ds/v1/file.csv") + def test_unknown_leading_segment_is_read_as_a_legacy_owner(self, auth_env): + doc = DatasetFileDocument("/notAResourceType/[email protected]/ds/v1/file.csv") + assert doc.owner_email == "notAResourceType" + assert doc.file_relative_path == "v1/file.csv" - def test_rejects_path_with_too_few_segments(self, auth_env): + def test_rejects_prefixed_path_with_too_few_segments(self, auth_env): with pytest.raises(ValueError, match="Invalid file path format"): DatasetFileDocument("/datasets/[email protected]/ds/v1") + def test_rejects_legacy_path_with_too_few_segments(self, auth_env): + with pytest.raises(ValueError, match="Invalid file path format"): + DatasetFileDocument("/[email protected]/ds/v1") + def test_requires_jwt_token_in_environment(self, monkeypatch): monkeypatch.delenv("USER_JWT_TOKEN", raising=False) monkeypatch.setenv("FILE_SERVICE_GET_PRESIGNED_URL_ENDPOINT", CUSTOM_ENDPOINT) diff --git a/common/workflow-core/src/main/scala/org/apache/texera/amber/core/storage/FileResolver.scala b/common/workflow-core/src/main/scala/org/apache/texera/amber/core/storage/FileResolver.scala index 88df489991..ac7f8d71f6 100644 --- a/common/workflow-core/src/main/scala/org/apache/texera/amber/core/storage/FileResolver.scala +++ b/common/workflow-core/src/main/scala/org/apache/texera/amber/core/storage/FileResolver.scala @@ -78,6 +78,7 @@ object FileResolver { /** * Parses a dataset logical path into its components, or None if it is not a well-formed dataset path. * Expected format: /datasets/ownerEmail/datasetName/versionName/fileRelativePath + * The legacy unprefixed format is also accepted. * * @param fileName The file path to parse * @return Some((ownerEmail, datasetName, versionName, fileRelativePath)) if valid, None otherwise @@ -88,16 +89,13 @@ object FileResolver { val filePath = Paths.get(fileName) val pathSegments = (0 until filePath.getNameCount).map(filePath.getName(_).toString).toArray - if (pathSegments.length < 5 || !ResourceType.isValidPrefix(pathSegments(0))) { - return None - } - - val ownerEmail = pathSegments(1) - val datasetName = pathSegments(2) - val versionName = pathSegments(3) - val fileRelativePathSegments = pathSegments.drop(4) - - Some((ownerEmail, datasetName, versionName, fileRelativePathSegments)) + // TODO(datasets-prefix): require the prefix once all stored paths are migrated (36.sql). + if (pathSegments.headOption.exists(ResourceType.isValidPrefix)) { + if (pathSegments.length < 5) None + else Some((pathSegments(1), pathSegments(2), pathSegments(3), pathSegments.drop(4))) + } else if (pathSegments.length >= 4) { + Some((pathSegments(0), pathSegments(1), pathSegments(2), pathSegments.drop(3))) + } else None } /** diff --git a/common/workflow-core/src/test/scala/org/apache/texera/amber/storage/FileResolverSpec.scala b/common/workflow-core/src/test/scala/org/apache/texera/amber/storage/FileResolverSpec.scala index 921ce0052c..e6533c7f1b 100644 --- a/common/workflow-core/src/test/scala/org/apache/texera/amber/storage/FileResolverSpec.scala +++ b/common/workflow-core/src/test/scala/org/apache/texera/amber/storage/FileResolverSpec.scala @@ -82,10 +82,10 @@ class FileResolverSpec private val dataset1TxtFilePath = "/datasets/[email protected]/test_dataset/v1/1.txt" - // Unprefixed form (no resource-type segment); no longer resolvable as a dataset. + // Legacy unprefixed form. private val unprefixedDataset1TxtFilePath = "/[email protected]/test_dataset/v1/1.txt" - // The leading segment is not a known resource type, so this is not a resolvable path. + // Leading segment names no known dataset owner. private val unknownResourceTypeFilePath = "/notAResourceType/[email protected]/test_dataset/v1/1.txt" @@ -124,14 +124,15 @@ class FileResolverSpec ) } - "FileResolver" should "not resolve a path without a resource-type prefix" in { - // Without a leading resource-type segment the path is not resolvable - assertThrows[FileNotFoundException] { - FileResolver.resolve(unprefixedDataset1TxtFilePath) - } + "FileResolver" should "still resolve a legacy unprefixed dataset path" in { + val dataset1TxtUri = FileResolver.resolve(unprefixedDataset1TxtFilePath) + + assert( + dataset1TxtUri.toString == f"${FileResolver.DATASET_FILE_URI_SCHEME}:///${testDataset.getRepositoryName}/${testDatasetVersion1.getVersionHash}/1.txt" + ) } - "FileResolver" should "not resolve a path whose prefix is not a known resource type" in { + "FileResolver" should "not resolve a path whose leading segment names no dataset" in { assertThrows[FileNotFoundException] { FileResolver.resolve(unknownResourceTypeFilePath) } @@ -178,8 +179,11 @@ class FileResolverSpec ) } - it should "return None for an unprefixed path (the datasets prefix is required)" in { - assert(FileResolver.parseDatasetOwnerAndName(unprefixedDataset1TxtFilePath).isEmpty) + it should "still extract owner and name from a legacy unprefixed path" in { + assert( + FileResolver.parseDatasetOwnerAndName(unprefixedDataset1TxtFilePath) + == Some(("[email protected]", "test_dataset")) + ) } it should "return None when the prefixed path has too few segments" in { diff --git a/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/source/dataset/FileListerSourceOpExec.scala b/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/source/dataset/FileListerSourceOpExec.scala index 54bc607be1..fb27257865 100644 --- a/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/source/dataset/FileListerSourceOpExec.scala +++ b/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/source/dataset/FileListerSourceOpExec.scala @@ -41,12 +41,16 @@ object FileListerSourceOpExec { datasetVersionPath: String ): (String, String, String, String) = { val segments = datasetVersionPath.split("/").filter(_.nonEmpty) - require( - segments.length >= 4 && ResourceType.isValidPrefix(segments.head), - s"Invalid dataset version path '$datasetVersionPath'; " + - "expected /datasets/ownerEmail/datasetName/versionName" - ) - (segments(0), segments(1), segments(2), segments(3)) + val invalidPath = s"Invalid dataset version path '$datasetVersionPath'; " + + "expected /datasets/ownerEmail/datasetName/versionName" + + if (segments.headOption.exists(ResourceType.isValidPrefix)) { + require(segments.length >= 4, invalidPath) + (segments(0), segments(1), segments(2), segments(3)) + } else { + require(segments.length >= 3, invalidPath) + (ResourceType.Datasets.toString, segments(0), segments(1), segments(2)) + } } private[dataset] def canonicalVersionPath( diff --git a/common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/source/dataset/FileListerSourceOpExecSpec.scala b/common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/source/dataset/FileListerSourceOpExecSpec.scala index 11908a9d49..19fb7513d5 100644 --- a/common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/source/dataset/FileListerSourceOpExecSpec.scala +++ b/common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/source/dataset/FileListerSourceOpExecSpec.scala @@ -71,21 +71,41 @@ class FileListerSourceOpExecSpec extends AnyFlatSpec { ) } - it should "reject a path without a resource-type prefix" in { - assertThrows[IllegalArgumentException] { + it should "still accept a legacy unprefixed path" in { + val (prefix, owner, name, version) = FileListerSourceOpExec.parseDatasetVersionPath("/alice/ds/v1") - } + assert(prefix == "datasets") + assert(owner == "alice") + assert(name == "ds") + assert(version == "v1") + } + + it should "normalize a legacy unprefixed path to the canonical prefixed form" in { + val (prefix, owner, name, version) = + FileListerSourceOpExec.parseDatasetVersionPath("/alice/ds/v1") + assert( + FileListerSourceOpExec.canonicalVersionPath(prefix, owner, name, version) + == "/datasets/alice/ds/v1" + ) } - it should "reject a path whose prefix is not a known resource type" in { + it should "read an unknown leading segment as a legacy owner" in { + val (_, owner, name, version) = + FileListerSourceOpExec.parseDatasetVersionPath("/notAResourceType/alice/ds") + assert(owner == "notAResourceType") + assert(name == "alice") + assert(version == "ds") + } + + it should "reject a prefixed path with too few segments" in { assertThrows[IllegalArgumentException] { - FileListerSourceOpExec.parseDatasetVersionPath("/notAResourceType/alice/ds/v1") + FileListerSourceOpExec.parseDatasetVersionPath("/datasets/alice/ds") } } - it should "reject a path with too few segments" in { + it should "reject a legacy path with too few segments" in { assertThrows[IllegalArgumentException] { - FileListerSourceOpExec.parseDatasetVersionPath("/datasets/alice/ds") + FileListerSourceOpExec.parseDatasetVersionPath("/alice/ds") } } } diff --git a/frontend/src/app/workspace/component/dataset-selection-modal/dataset-selection-modal.component.spec.ts b/frontend/src/app/workspace/component/dataset-selection-modal/dataset-selection-modal.component.spec.ts index d1483b71ad..ddd595a10f 100644 --- a/frontend/src/app/workspace/component/dataset-selection-modal/dataset-selection-modal.component.spec.ts +++ b/frontend/src/app/workspace/component/dataset-selection-modal/dataset-selection-modal.component.spec.ts @@ -120,6 +120,16 @@ describe("DatasetSelectionModalComponent", () => { expect(datasetService.retrieveDatasetVersionFileTree).toHaveBeenCalledWith(10, 100); }); + it("ngOnInit also accepts a legacy unprefixed data.selectedPath", () => { + modalData.fileMode = true; + modalData.selectedPath = `/${OWNER}/myds/v1`; + + build(); + + expect(component.selectedDataset).toBe(dataset); + expect(component.selectedVersion).toBe(version); + }); + it("onDatasetChange loads the version list and auto-selects a version in file mode", () => { modalData.fileMode = true; build(); diff --git a/frontend/src/app/workspace/component/dataset-selection-modal/dataset-selection-modal.component.ts b/frontend/src/app/workspace/component/dataset-selection-modal/dataset-selection-modal.component.ts index 5ee5b9115e..97ecf579ca 100644 --- a/frontend/src/app/workspace/component/dataset-selection-modal/dataset-selection-modal.component.ts +++ b/frontend/src/app/workspace/component/dataset-selection-modal/dataset-selection-modal.component.ts @@ -84,8 +84,12 @@ export class DatasetSelectionModalComponent implements OnInit { this.datasets = datasets; const selectedPath = this.data.selectedPath; if (selectedPath) { - // Stored paths always carry the resource-type prefix; skip it so that owner/dataset/version line up. - const [, ownerEmail, datasetName, versionName] = selectedPath.split("/").filter(part => part.length > 0); + const segments = selectedPath.split("/").filter(part => part.length > 0); + // TODO(datasets-prefix): require the prefix once all ml model support PRs are done. + if ((Object.values(ResourceType) as string[]).includes(segments[0])) { + segments.shift(); + } + const [ownerEmail, datasetName, versionName] = segments; this.selectedDataset = this.datasets.find( dataset => dataset.ownerEmail === ownerEmail && dataset.dataset.name === datasetName );
