From 0d80726f3080c562b7c3462598568ced1d71dd73 Mon Sep 17 00:00:00 2001 From: ali Date: Tue, 31 Mar 2026 08:40:44 -0700 Subject: [PATCH 01/16] feat: `datasets` prefix --- .../amber/core/storage/FileResolver.scala | 19 ++++++++++++++----- .../amber/storage/FileResolverSpec.scala | 4 ++-- .../service/resource/DatasetResource.scala | 10 +++++++--- .../type/dataset/DatasetFileNode.scala | 8 ++++++-- frontend/src/app/common/type/dataset-file.ts | 14 ++++++++++---- .../app/common/type/datasetVersionFileTree.ts | 11 ++++++----- 6 files changed, 45 insertions(+), 21 deletions(-) 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 c8a407df993..dda1df984ce 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 @@ -75,9 +75,13 @@ object FileResolver { filePath.toUri } + private val RESOURCE_TYPE_PREFIXES = Set("datasets") + /** * Parses a dataset file path and extracts its components. - * Expected format: /ownerEmail/datasetName/versionName/fileRelativePath + * Expected format: /datasets/ownerEmail/datasetName/versionName/fileRelativePath + * + * The first segment is a resource type prefix (e.g. "datasets") and is stripped before parsing. * * @param fileName The file path to parse * @return Some((ownerEmail, datasetName, versionName, fileRelativePath)) if valid, None otherwise @@ -86,7 +90,12 @@ object FileResolver { fileName: String ): Option[(String, String, String, Array[String])] = { val filePath = Paths.get(fileName) - val pathSegments = (0 until filePath.getNameCount).map(filePath.getName(_).toString).toArray + var pathSegments = (0 until filePath.getNameCount).map(filePath.getName(_).toString).toArray + + // Strip known resource type prefix if present + if (pathSegments.nonEmpty && RESOURCE_TYPE_PREFIXES.contains(pathSegments(0))) { + pathSegments = pathSegments.drop(1) + } if (pathSegments.length < 4) { return None @@ -103,8 +112,8 @@ object FileResolver { /** * Attempts to resolve a given fileName to a URI. * - * The fileName format should be: /ownerEmail/datasetName/versionName/fileRelativePath - * e.g. /bob@texera.com/twitterDataset/v1/california/irvine/tw1.csv + * The fileName format should be: /datasets/ownerEmail/datasetName/versionName/fileRelativePath + * e.g. /datasets/bob@texera.com/twitterDataset/v1/california/irvine/tw1.csv * The output dataset URI format is: {DATASET_FILE_URI_SCHEME}:///{repositoryName}/{versionHash}/fileRelativePath * e.g. {DATASET_FILE_URI_SCHEME}:///dataset-15/adeq233td/some/dir/file.txt * @@ -195,7 +204,7 @@ object FileResolver { /** * Parses a dataset file path to extract owner email and dataset name. - * Expected format: /ownerEmail/datasetName/versionName/fileRelativePath + * Expected format: /datasets/ownerEmail/datasetName/versionName/fileRelativePath * * @param path The file path from operator properties * @return Some((ownerEmail, datasetName)) if path is valid, None otherwise 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 593ac8d3471..2602dab4c79 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 @@ -79,9 +79,9 @@ class FileResolverSpec private val localCsvFilePath = "common/workflow-core/src/test/resources/country_sales_small.csv" - private val datasetACsvFilePath = "/test_user@test.com/test_dataset/v2/directory/a.csv" + private val datasetACsvFilePath = "/datasets/test_user@test.com/test_dataset/v2/directory/a.csv" - private val dataset1TxtFilePath = "/test_user@test.com/test_dataset/v1/1.txt" + private val dataset1TxtFilePath = "/datasets/test_user@test.com/test_dataset/v1/1.txt" override protected def beforeAll(): Unit = { initializeDBAndReplaceDSLContext() diff --git a/file-service/src/main/scala/org/apache/texera/service/resource/DatasetResource.scala b/file-service/src/main/scala/org/apache/texera/service/resource/DatasetResource.scala index efed479653a..7d5a22ad734 100644 --- a/file-service/src/main/scala/org/apache/texera/service/resource/DatasetResource.scala +++ b/file-service/src/main/scala/org/apache/texera/service/resource/DatasetResource.scala @@ -1188,7 +1188,7 @@ class DatasetResource extends LazyLogging { throw new NotFoundException(ERR_DATASET_VERSION_NOT_FOUND_MESSAGE) ) - val ownerNode = DatasetFileNode + val datasetsNode = DatasetFileNode .fromLakeFSRepositoryCommittedObjects( Map( (user.getEmail, dataset.getName, latestVersion.getName) -> LakeFSStorageClient @@ -1197,6 +1197,8 @@ class DatasetResource extends LazyLogging { ) .head + val ownerNode = datasetsNode.children.get.head + DashboardDatasetVersion( latestVersion, ownerNode.children.get @@ -1404,7 +1406,7 @@ class DatasetResource extends LazyLogging { val datasetName = dataset.dataset.getName val repositoryName = dataset.dataset.getRepositoryName - val ownerFileNode = DatasetFileNode + val datasetsNode = DatasetFileNode .fromLakeFSRepositoryCommittedObjects( Map( (dataset.ownerEmail, datasetName, datasetVersion.getName) -> LakeFSStorageClient @@ -1413,6 +1415,8 @@ class DatasetResource extends LazyLogging { ) .head + val ownerFileNode = datasetsNode.children.get.head + DatasetVersionRootFileNodesResponse( ownerFileNode.children.get .find(_.getName == datasetName) @@ -1423,7 +1427,7 @@ class DatasetResource extends LazyLogging { .head .children .get, - DatasetFileNode.calculateTotalSize(List(ownerFileNode)) + DatasetFileNode.calculateTotalSize(List(datasetsNode)) ) } diff --git a/file-service/src/main/scala/org/apache/texera/service/type/dataset/DatasetFileNode.scala b/file-service/src/main/scala/org/apache/texera/service/type/dataset/DatasetFileNode.scala index 7c91d30b94a..b26ffaa01b8 100644 --- a/file-service/src/main/scala/org/apache/texera/service/type/dataset/DatasetFileNode.scala +++ b/file-service/src/main/scala/org/apache/texera/service/type/dataset/DatasetFileNode.scala @@ -81,6 +81,10 @@ object DatasetFileNode { ): List[DatasetFileNode] = { val rootNode = new DatasetFileNode("/", "directory", null, "") + // Add "datasets" prefix node + val datasetsNode = new DatasetFileNode("datasets", "directory", rootNode, "") + rootNode.children = Some(List(datasetsNode)) + // Owner level nodes map val ownerNodes = mutable.Map[String, DatasetFileNode]() @@ -88,8 +92,8 @@ object DatasetFileNode { case ((ownerEmail, datasetName, versionName), objects) => val ownerNode = ownerNodes.getOrElseUpdate( ownerEmail, { - val newNode = new DatasetFileNode(ownerEmail, "directory", rootNode, ownerEmail) - rootNode.children = Some(rootNode.getChildren :+ newNode) + val newNode = new DatasetFileNode(ownerEmail, "directory", datasetsNode, ownerEmail) + datasetsNode.children = Some(datasetsNode.getChildren :+ newNode) newNode } ) diff --git a/frontend/src/app/common/type/dataset-file.ts b/frontend/src/app/common/type/dataset-file.ts index 5fe561d3720..64d8224555b 100644 --- a/frontend/src/app/common/type/dataset-file.ts +++ b/frontend/src/app/common/type/dataset-file.ts @@ -17,8 +17,8 @@ * under the License. */ -// user given filePath is /ownerEmail/datasetName/versionName/fileRelativePath -// e.g. /bob@texera.com/twitterDataset/v1/california/irvine/tw1.csv +// user given filePath is /datasets/ownerEmail/datasetName/versionName/fileRelativePath +// e.g. /datasets/bob@texera.com/twitterDataset/v1/california/irvine/tw1.csv export interface DatasetFile { ownerEmail: string; datasetName: string; @@ -28,11 +28,17 @@ export interface DatasetFile { /** * Parses a file path string to a DatasetFile interface. + * The first segment "datasets" is stripped before parsing. * @param filePath - The file path string to parse. * @returns The parsed DatasetFile object. */ export function parseFilePathToDatasetFile(filePath: string): DatasetFile { - const parts = filePath.split("/").filter(part => part.length > 0); + let parts = filePath.split("/").filter(part => part.length > 0); + + // Strip the "datasets" prefix if present + if (parts.length > 0 && parts[0] === "datasets") { + parts = parts.slice(1); + } if (parts.length < 4) { throw new Error("Invalid file path format"); @@ -56,5 +62,5 @@ export function parseFilePathToDatasetFile(filePath: string): DatasetFile { */ export function parseDatasetFileToFilePath(datasetFile: DatasetFile): string { const { ownerEmail, datasetName, versionName, fileRelativePath } = datasetFile; - return `/${ownerEmail}/${datasetName}/${versionName}/${fileRelativePath}`; + return `/datasets/${ownerEmail}/${datasetName}/${versionName}/${fileRelativePath}`; } diff --git a/frontend/src/app/common/type/datasetVersionFileTree.ts b/frontend/src/app/common/type/datasetVersionFileTree.ts index 8d1686998ca..98f898d4432 100644 --- a/frontend/src/app/common/type/datasetVersionFileTree.ts +++ b/frontend/src/app/common/type/datasetVersionFileTree.ts @@ -31,19 +31,20 @@ export function getFullPathFromDatasetFileNode(node: DatasetFileNode): string { } /** - * Returns the relative path of a DatasetFileNode by stripping the first three segments. + * Returns the relative path of a DatasetFileNode by stripping the first four segments + * (datasets/ownerEmail/datasetName/versionName). * @param node The DatasetFileNode whose relative path is needed. - * @returns The relative path (without the first three segments and without a leading slash). + * @returns The relative path (without the first four segments and without a leading slash). */ export function getRelativePathFromDatasetFileNode(node: DatasetFileNode): string { const fullPath = getFullPathFromDatasetFileNode(node); // Get the full path const pathSegments = fullPath.split("/").filter(segment => segment.length > 0); // Split and remove empty segments - if (pathSegments.length <= 3) { - return ""; // If there are 3 or fewer segments, return an empty string (no relative path exists) + if (pathSegments.length <= 4) { + return ""; // If there are 4 or fewer segments, return an empty string (no relative path exists) } - return pathSegments.slice(3).join("/"); // Join remaining segments as the relative path + return pathSegments.slice(4).join("/"); // Join remaining segments as the relative path } export function getPathsUnderOrEqualDatasetFileNode(node: DatasetFileNode): string[] { From 1dfc91717e467a2c9e0394dd1c0a73ac5993a17e Mon Sep 17 00:00:00 2001 From: Tanishq Gandhi Date: Fri, 26 Jun 2026 12:04:17 -0700 Subject: [PATCH 02/16] fix(file-service): correct dataset owner email, harden file tree, add tests Review fixes on #5911: use dataset owner email in retrieveLatestDatasetVersion, replace brittle Option.get/.head with headOption, strip datasets prefix on selection-modal reopen, clarify FileResolver docs; add FileResolver/DatasetFileNode and frontend path-helper tests. Co-Authored-By: Claude Opus 4.8 --- .../amber/core/storage/FileResolver.scala | 10 ++- .../amber/storage/FileResolverSpec.scala | 30 ++++++++ .../service/resource/DatasetResource.scala | 18 ++++- .../type/dataset/DatasetFileNode.scala | 16 +++-- .../service/type/DatasetFileNodeSpec.scala | 69 +++++++++++++++++++ .../type/datasetVersionFileTree.spec.ts | 58 ++++++++++++++++ .../dataset-selection-modal.component.ts | 8 ++- 7 files changed, 198 insertions(+), 11 deletions(-) create mode 100644 file-service/src/test/scala/org/apache/texera/service/type/DatasetFileNodeSpec.scala create mode 100644 frontend/src/app/common/type/datasetVersionFileTree.spec.ts 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 dda1df984ce..ca6613684a4 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 @@ -79,9 +79,15 @@ object FileResolver { /** * Parses a dataset file path and extracts its components. - * Expected format: /datasets/ownerEmail/datasetName/versionName/fileRelativePath * - * The first segment is a resource type prefix (e.g. "datasets") and is stripped before parsing. + * Two formats are accepted: + * - Prefixed (current): /datasets/ownerEmail/datasetName/versionName/fileRelativePath + * - Legacy (unprefixed): /ownerEmail/datasetName/versionName/fileRelativePath + * + * A leading resource-type prefix listed in [[RESOURCE_TYPE_PREFIXES]] (e.g. "datasets") is + * stripped if present; legacy paths without the prefix are still parsed for backward + * compatibility. An unrecognized leading segment is NOT stripped and is treated as the + * ownerEmail segment. * * @param fileName The file path to parse * @return Some((ownerEmail, datasetName, versionName, fileRelativePath)) if valid, None otherwise 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 2602dab4c79..306f0235f6a 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 @@ -83,6 +83,12 @@ class FileResolverSpec private val dataset1TxtFilePath = "/datasets/test_user@test.com/test_dataset/v1/1.txt" + // Legacy (pre-prefix) format, kept resolvable for backward compatibility. + private val legacyDataset1TxtFilePath = "/test_user@test.com/test_dataset/v1/1.txt" + + // "models" is not (yet) a registered resource-type prefix, so it must NOT be stripped. + private val unknownPrefixFilePath = "/models/test_user@test.com/test_dataset/v1/1.txt" + override protected def beforeAll(): Unit = { initializeDBAndReplaceDSLContext() @@ -118,6 +124,30 @@ class FileResolverSpec ) } + "FileResolver" should "resolve a legacy unprefixed dataset path identically to the prefixed path" in { + val prefixedUri = FileResolver.resolve(dataset1TxtFilePath) + val legacyUri = FileResolver.resolve(legacyDataset1TxtFilePath) + + assert(legacyUri == prefixedUri) + assert( + legacyUri.toString == f"${FileResolver.DATASET_FILE_URI_SCHEME}:///${testDataset.getRepositoryName}/${testDatasetVersion1.getVersionHash}/1.txt" + ) + } + + "FileResolver" should "not strip an unregistered resource-type prefix" in { + // "models" is treated as the ownerEmail segment, so the dataset lookup fails and + // resolution falls back to a (missing) local file. + assertThrows[FileNotFoundException] { + FileResolver.resolve(unknownPrefixFilePath) + } + } + + "FileResolver" should "throw not found exception when a prefixed path has too few segments" in { + assertThrows[FileNotFoundException] { + FileResolver.resolve("/datasets/test_user@test.com/test_dataset") + } + } + "FileResolver" should "throw not found exception" in { assertThrows[FileNotFoundException] { FileResolver.resolve("some/random/path") diff --git a/file-service/src/main/scala/org/apache/texera/service/resource/DatasetResource.scala b/file-service/src/main/scala/org/apache/texera/service/resource/DatasetResource.scala index 7d5a22ad734..0cec8fb72d8 100644 --- a/file-service/src/main/scala/org/apache/texera/service/resource/DatasetResource.scala +++ b/file-service/src/main/scala/org/apache/texera/service/resource/DatasetResource.scala @@ -1191,13 +1191,21 @@ class DatasetResource extends LazyLogging { val datasetsNode = DatasetFileNode .fromLakeFSRepositoryCommittedObjects( Map( - (user.getEmail, dataset.getName, latestVersion.getName) -> LakeFSStorageClient + ( + getOwner(ctx, did).getEmail, + dataset.getName, + latestVersion.getName + ) -> LakeFSStorageClient .retrieveObjectsOfVersion(dataset.getRepositoryName, latestVersion.getVersionHash) ) ) .head - val ownerNode = datasetsNode.children.get.head + val ownerNode = datasetsNode.getChildren.headOption.getOrElse( + throw new IllegalStateException( + s"Dataset file tree for ${dataset.getName} is missing its owner node" + ) + ) DashboardDatasetVersion( latestVersion, @@ -1415,7 +1423,11 @@ class DatasetResource extends LazyLogging { ) .head - val ownerFileNode = datasetsNode.children.get.head + val ownerFileNode = datasetsNode.getChildren.headOption.getOrElse( + throw new IllegalStateException( + s"Dataset file tree for $datasetName is missing its owner node" + ) + ) DatasetVersionRootFileNodesResponse( ownerFileNode.children.get diff --git a/file-service/src/main/scala/org/apache/texera/service/type/dataset/DatasetFileNode.scala b/file-service/src/main/scala/org/apache/texera/service/type/dataset/DatasetFileNode.scala index b26ffaa01b8..56de8dbc0ec 100644 --- a/file-service/src/main/scala/org/apache/texera/service/type/dataset/DatasetFileNode.scala +++ b/file-service/src/main/scala/org/apache/texera/service/type/dataset/DatasetFileNode.scala @@ -26,9 +26,10 @@ import java.util import scala.collection.mutable // DatasetFileNode represents a unique file in dataset, its full path is in the format of: -// /ownerEmail/datasetName/versionName/fileRelativePath -// e.g. /bob@texera.com/twitterDataset/v1/california/irvine/tw1.csv -// ownerName is bob@texera.com; datasetName is twitterDataset, versionName is v1, fileRelativePath is california/irvine/tw1.csv +// /datasets/ownerEmail/datasetName/versionName/fileRelativePath +// e.g. /datasets/bob@texera.com/twitterDataset/v1/california/irvine/tw1.csv +// the leading "datasets" segment is the resource-type prefix; ownerName is bob@texera.com; +// datasetName is twitterDataset, versionName is v1, fileRelativePath is california/irvine/tw1.csv class DatasetFileNode( val name: String, // direct name of this node val nodeType: String, // "file" or "directory" @@ -157,14 +158,19 @@ object DatasetFileNode { map: Map[(String, String, String), List[PhysicalFileNode]] ): List[DatasetFileNode] = { val rootNode = new DatasetFileNode("/", "directory", null, "") + + // Add "datasets" prefix node + val datasetsNode = new DatasetFileNode("datasets", "directory", rootNode, "") + rootNode.children = Some(List(datasetsNode)) + val ownerNodes = mutable.Map[String, DatasetFileNode]() map.foreach { case ((ownerEmail, datasetName, versionName), physicalNodes) => val ownerNode = ownerNodes.getOrElseUpdate( ownerEmail, { - val newNode = new DatasetFileNode(ownerEmail, "directory", rootNode, ownerEmail) - rootNode.children = Some(rootNode.getChildren :+ newNode) + val newNode = new DatasetFileNode(ownerEmail, "directory", datasetsNode, ownerEmail) + datasetsNode.children = Some(datasetsNode.getChildren :+ newNode) newNode } ) diff --git a/file-service/src/test/scala/org/apache/texera/service/type/DatasetFileNodeSpec.scala b/file-service/src/test/scala/org/apache/texera/service/type/DatasetFileNodeSpec.scala new file mode 100644 index 00000000000..1860b128a71 --- /dev/null +++ b/file-service/src/test/scala/org/apache/texera/service/type/DatasetFileNodeSpec.scala @@ -0,0 +1,69 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.texera.service.`type` + +import io.lakefs.clients.sdk.model.ObjectStats +import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers + +class DatasetFileNodeSpec extends AnyFlatSpec with Matchers { + + private def obj(path: String, size: Long): ObjectStats = + new ObjectStats().path(path).sizeBytes(Long.box(size)) + + private def child(node: DatasetFileNode, name: String): DatasetFileNode = + node.getChildren + .find(_.getName == name) + .getOrElse(fail(s"expected child '$name' under '${node.getName}'")) + + "fromLakeFSRepositoryCommittedObjects" should "root the tree at a 'datasets' prefix node" in { + val tree = DatasetFileNode.fromLakeFSRepositoryCommittedObjects( + Map(("bob@texera.com", "twitterDataset", "v1") -> List(obj("b.txt", 50L))) + ) + + tree should have size 1 + val datasetsNode = tree.head + datasetsNode.getName shouldBe "datasets" + datasetsNode.getFilePath shouldBe "/datasets" + } + + it should "nest owner/dataset/version under the datasets node with prefixed file paths" in { + val tree = DatasetFileNode.fromLakeFSRepositoryCommittedObjects( + Map( + ("bob@texera.com", "twitterDataset", "v1") -> + List(obj("dir/a.csv", 100L), obj("b.txt", 50L)) + ) + ) + + val owner = child(tree.head, "bob@texera.com") + owner.getFilePath shouldBe "/datasets/bob@texera.com" + + val version = child(child(owner, "twitterDataset"), "v1") + version.getFilePath shouldBe "/datasets/bob@texera.com/twitterDataset/v1" + + val topFile = child(version, "b.txt") + topFile.getNodeType shouldBe "file" + topFile.getSize shouldBe Some(50L) + topFile.getFilePath shouldBe "/datasets/bob@texera.com/twitterDataset/v1/b.txt" + + val nested = child(child(version, "dir"), "a.csv") + nested.getFilePath shouldBe "/datasets/bob@texera.com/twitterDataset/v1/dir/a.csv" + } +} diff --git a/frontend/src/app/common/type/datasetVersionFileTree.spec.ts b/frontend/src/app/common/type/datasetVersionFileTree.spec.ts new file mode 100644 index 00000000000..5fdbb9f8fc7 --- /dev/null +++ b/frontend/src/app/common/type/datasetVersionFileTree.spec.ts @@ -0,0 +1,58 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { + DatasetFileNode, + getFullPathFromDatasetFileNode, + getRelativePathFromDatasetFileNode, +} from "./datasetVersionFileTree"; + +describe("datasetVersionFileTree path helpers", () => { + const fileNode = (parentDir: string, name: string): DatasetFileNode => ({ + name, + type: "file", + parentDir, + }); + + describe("getFullPathFromDatasetFileNode", () => { + it("joins parentDir and name", () => { + const node = fileNode("/datasets/bob@texera.com/twitterDataset/v1/california/irvine", "tw1.csv"); + expect(getFullPathFromDatasetFileNode(node)).toBe( + "/datasets/bob@texera.com/twitterDataset/v1/california/irvine/tw1.csv" + ); + }); + }); + + describe("getRelativePathFromDatasetFileNode", () => { + it("strips the datasets/owner/dataset/version prefix (4 segments)", () => { + const node = fileNode("/datasets/bob@texera.com/twitterDataset/v1/california/irvine", "tw1.csv"); + expect(getRelativePathFromDatasetFileNode(node)).toBe("california/irvine/tw1.csv"); + }); + + it("returns the bare file name for a file at the version root", () => { + const node = fileNode("/datasets/bob@texera.com/twitterDataset/v1", "readme.txt"); + expect(getRelativePathFromDatasetFileNode(node)).toBe("readme.txt"); + }); + + it("returns empty string when there is no path below the version", () => { + const node = fileNode("/datasets/bob@texera.com/twitterDataset", "v1"); + expect(getRelativePathFromDatasetFileNode(node)).toBe(""); + }); + }); +}); 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 7f70792f937..32a7dc195e9 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 @@ -78,7 +78,13 @@ export class DatasetSelectionModalComponent implements OnInit { this.datasets = datasets; const selectedPath = this.data.selectedPath; if (selectedPath) { - const [ownerEmail, datasetName, versionName] = selectedPath.split("/").filter(part => part.length > 0); + const segments = selectedPath.split("/").filter(part => part.length > 0); + // Drop the resource-type prefix ("datasets") if present, so owner/dataset/version + // line up whether the stored path is prefixed (file mode) or not (version mode). + if (segments[0] === "datasets") { + segments.shift(); + } + const [ownerEmail, datasetName, versionName] = segments; this.selectedDataset = this.datasets.find( dataset => dataset.ownerEmail === ownerEmail && dataset.dataset.name === datasetName ); From 14318872a2fdfc9334cb0d77a3e88fe996ed6590 Mon Sep 17 00:00:00 2001 From: Tanishq Gandhi Date: Wed, 1 Jul 2026 16:16:04 -0700 Subject: [PATCH 03/16] test+fix: extend datasets prefix to python UDF path + add frontend parse tests --- .../pytexera/storage/dataset_file_document.py | 24 +++++- .../storage/test_dataset_file_document.py | 28 +++++++ .../src/app/common/type/dataset-file.spec.ts | 78 +++++++++++++++++++ .../dataset-selection-modal.component.ts | 4 +- 4 files changed, 128 insertions(+), 6 deletions(-) create mode 100644 frontend/src/app/common/type/dataset-file.spec.ts 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 5a063f60470..d046597467d 100644 --- a/amber/src/main/python/pytexera/storage/dataset_file_document.py +++ b/amber/src/main/python/pytexera/storage/dataset_file_document.py @@ -24,6 +24,11 @@ class DatasetFileDocument: + # Leading resource-type segment on dataset logical paths. It is stripped when + # parsing and re-emitted when calling the file-service, mirroring FileResolver + # on the backend. The legacy unprefixed format is still accepted. + RESOURCE_TYPE_PREFIX = "datasets" + # (connect, read) timeout and retry settings for the file-service GETs below. # Read timeout bounds inactivity between bytes, not total download time. _CONNECT_TIMEOUT_SECONDS = 5 @@ -56,14 +61,24 @@ def __init__(self, file_path: str): Parses the file path into dataset metadata. :param file_path: - Expected format - "/ownerEmail/datasetName/versionName/fileRelativePath" - Example: "/bob@texera.com/twitterDataset/v1/california/irvine/tw1.csv" + Expected format - + "/datasets/ownerEmail/datasetName/versionName/fileRelativePath" + Example: + "/datasets/bob@texera.com/twitterDataset/v1/california/tw1.csv" + A leading "datasets" resource-type segment is stripped if present; the + legacy unprefixed format is still accepted for backward compatibility. """ parts = file_path.strip("/").split("/") + + # Strip the resource-type prefix ("datasets") if present, keeping legacy + # unprefixed paths working. Mirrors FileResolver on the backend. + if parts and parts[0] == self.RESOURCE_TYPE_PREFIX: + parts = parts[1:] + if len(parts) < 4: raise ValueError( - "Invalid file path format. " - "Expected: /ownerEmail/datasetName/versionName/fileRelativePath" + "Invalid file path format. Expected: " + "/datasets/ownerEmail/datasetName/versionName/fileRelativePath" ) self.owner_email = parts[0] @@ -90,6 +105,7 @@ def get_presigned_url(self) -> str: """ headers = {"Authorization": f"Bearer {self.jwt_token}"} encoded_file_path = urllib.parse.quote( + f"/{self.RESOURCE_TYPE_PREFIX}" f"/{self.owner_email}" f"/{self.dataset_name}" f"/{self.version_name}" 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 36882fe27ee..75caf511ba4 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,6 +60,22 @@ def test_strips_leading_and_trailing_slashes_before_parsing(self, auth_env): assert doc.owner_email == "bob@x.com" assert doc.file_relative_path == "file.csv" + def test_strips_datasets_resource_type_prefix(self, auth_env): + doc = DatasetFileDocument("/datasets/bob@x.com/ds/v1/file.csv") + assert doc.owner_email == "bob@x.com" + assert doc.dataset_name == "ds" + assert doc.version_name == "v1" + assert doc.file_relative_path == "file.csv" + + def test_only_datasets_prefix_is_stripped(self, auth_env): + # Any other leading segment (e.g. "models") is treated as the owner email, + # matching the backend which only registers "datasets" as a prefix. + doc = DatasetFileDocument("/models/bob@x.com/ds/v1/file.csv") + assert doc.owner_email == "models" + assert doc.dataset_name == "bob@x.com" + assert doc.version_name == "ds" + assert doc.file_relative_path == "v1/file.csv" + def test_rejects_path_with_fewer_than_four_segments(self, auth_env): with pytest.raises(ValueError, match="Invalid file path format"): DatasetFileDocument("/bob@x.com/ds/v1") @@ -128,6 +144,18 @@ def test_url_encodes_filepath_query_parameter(self, monkeypatch): assert "bob%40x.com" in file_path assert file_path.startswith("/") + def test_sends_datasets_prefixed_filepath(self, monkeypatch): + # The reconstructed filePath is re-emitted with the "datasets" prefix, + # even when the input path was the legacy unprefixed form. + doc = self._make_doc(monkeypatch, path="/bob@x.com/ds/v1/file.csv") + with patch( + "pytexera.storage.dataset_file_document.requests.Session.get" + ) as mock_get: + mock_get.return_value = make_response(200, body={"presignedUrl": "u"}) + doc.get_presigned_url() + _, kwargs = mock_get.call_args + assert kwargs["params"]["filePath"].startswith("/datasets/") + def test_calls_configured_endpoint(self, monkeypatch): doc = self._make_doc(monkeypatch) with patch( diff --git a/frontend/src/app/common/type/dataset-file.spec.ts b/frontend/src/app/common/type/dataset-file.spec.ts new file mode 100644 index 00000000000..6f07ff9f5a6 --- /dev/null +++ b/frontend/src/app/common/type/dataset-file.spec.ts @@ -0,0 +1,78 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { parseFilePathToDatasetFile, parseDatasetFileToFilePath } from "./dataset-file"; + +describe("dataset-file path helpers", () => { + describe("parseFilePathToDatasetFile", () => { + it("parses a datasets-prefixed path", () => { + expect( + parseFilePathToDatasetFile("/datasets/bob@texera.com/twitterDataset/v1/california/irvine/tw1.csv") + ).toEqual({ + ownerEmail: "bob@texera.com", + datasetName: "twitterDataset", + versionName: "v1", + fileRelativePath: "california/irvine/tw1.csv", + }); + }); + + it("still parses a legacy unprefixed path (backward compatibility)", () => { + expect(parseFilePathToDatasetFile("/bob@texera.com/twitterDataset/v1/california/irvine/tw1.csv")).toEqual({ + ownerEmail: "bob@texera.com", + datasetName: "twitterDataset", + versionName: "v1", + fileRelativePath: "california/irvine/tw1.csv", + }); + }); + + it("does not strip a non-datasets leading segment (only 'datasets' is a prefix)", () => { + // "models" is treated as the ownerEmail segment, matching FileResolver on the backend. + expect(parseFilePathToDatasetFile("/models/bob@texera.com/twitterDataset/v1/tw1.csv")).toEqual({ + ownerEmail: "models", + datasetName: "bob@texera.com", + versionName: "twitterDataset", + fileRelativePath: "v1/tw1.csv", + }); + }); + + it("throws when the path has fewer than four segments", () => { + expect(() => parseFilePathToDatasetFile("/datasets/bob@texera.com/twitterDataset")).toThrowError( + "Invalid file path format" + ); + }); + }); + + describe("parseDatasetFileToFilePath", () => { + it("emits the datasets prefix", () => { + expect( + parseDatasetFileToFilePath({ + ownerEmail: "bob@texera.com", + datasetName: "twitterDataset", + versionName: "v1", + fileRelativePath: "california/irvine/tw1.csv", + }) + ).toBe("/datasets/bob@texera.com/twitterDataset/v1/california/irvine/tw1.csv"); + }); + }); + + it("round-trips a prefixed path through parse then build", () => { + const path = "/datasets/bob@texera.com/twitterDataset/v1/california/irvine/tw1.csv"; + expect(parseDatasetFileToFilePath(parseFilePathToDatasetFile(path))).toBe(path); + }); +}); 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 32a7dc195e9..18b2319fdd2 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 @@ -80,7 +80,7 @@ export class DatasetSelectionModalComponent implements OnInit { if (selectedPath) { const segments = selectedPath.split("/").filter(part => part.length > 0); // Drop the resource-type prefix ("datasets") if present, so owner/dataset/version - // line up whether the stored path is prefixed (file mode) or not (version mode). + // line up. Tolerates legacy paths saved before the prefix existed. if (segments[0] === "datasets") { segments.shift(); } @@ -119,7 +119,7 @@ export class DatasetSelectionModalComponent implements OnInit { this.fileTree = data.fileNodes; }); if (!this.data.fileMode) { - this.selectedPath = `/${this.selectedDataset.ownerEmail}/${this.selectedDataset.dataset.name}/${this.selectedVersion.name}`; + this.selectedPath = `/datasets/${this.selectedDataset.ownerEmail}/${this.selectedDataset.dataset.name}/${this.selectedVersion.name}`; } } } From 1d9cbd11070f1b32c4bdcc614970618e62c3cc1d Mon Sep 17 00:00:00 2001 From: Tanishq Gandhi Date: Thu, 2 Jul 2026 15:18:58 -0700 Subject: [PATCH 04/16] refactor: remove unused frontend path parser; reword prefix docs --- .../pytexera/storage/dataset_file_document.py | 10 +-- .../amber/core/storage/FileResolver.scala | 11 ++- .../amber/storage/FileResolverSpec.scala | 12 +-- .../src/app/common/type/dataset-file.spec.ts | 78 ------------------- frontend/src/app/common/type/dataset-file.ts | 66 ---------------- .../dataset-selection-modal.component.ts | 4 +- 6 files changed, 18 insertions(+), 163 deletions(-) delete mode 100644 frontend/src/app/common/type/dataset-file.spec.ts delete mode 100644 frontend/src/app/common/type/dataset-file.ts 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 d046597467d..a0048177124 100644 --- a/amber/src/main/python/pytexera/storage/dataset_file_document.py +++ b/amber/src/main/python/pytexera/storage/dataset_file_document.py @@ -26,7 +26,7 @@ class DatasetFileDocument: # Leading resource-type segment on dataset logical paths. It is stripped when # parsing and re-emitted when calling the file-service, mirroring FileResolver - # on the backend. The legacy unprefixed format is still accepted. + # on the backend. An unprefixed path is parsed as-is. RESOURCE_TYPE_PREFIX = "datasets" # (connect, read) timeout and retry settings for the file-service GETs below. @@ -65,13 +65,13 @@ def __init__(self, file_path: str): "/datasets/ownerEmail/datasetName/versionName/fileRelativePath" Example: "/datasets/bob@texera.com/twitterDataset/v1/california/tw1.csv" - A leading "datasets" resource-type segment is stripped if present; the - legacy unprefixed format is still accepted for backward compatibility. + A leading "datasets" resource-type segment is stripped if present; an + unprefixed path is parsed as-is. """ parts = file_path.strip("/").split("/") - # Strip the resource-type prefix ("datasets") if present, keeping legacy - # unprefixed paths working. Mirrors FileResolver on the backend. + # Strip the resource-type prefix ("datasets") if present; an unprefixed + # path is parsed as-is. Mirrors FileResolver on the backend. if parts and parts[0] == self.RESOURCE_TYPE_PREFIX: parts = parts[1:] 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 ca6613684a4..c408a54dbc0 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 @@ -80,14 +80,13 @@ object FileResolver { /** * Parses a dataset file path and extracts its components. * - * Two formats are accepted: - * - Prefixed (current): /datasets/ownerEmail/datasetName/versionName/fileRelativePath - * - Legacy (unprefixed): /ownerEmail/datasetName/versionName/fileRelativePath + * Two path shapes are accepted: + * - Prefixed: /datasets/ownerEmail/datasetName/versionName/fileRelativePath + * - Unprefixed: /ownerEmail/datasetName/versionName/fileRelativePath * * A leading resource-type prefix listed in [[RESOURCE_TYPE_PREFIXES]] (e.g. "datasets") is - * stripped if present; legacy paths without the prefix are still parsed for backward - * compatibility. An unrecognized leading segment is NOT stripped and is treated as the - * ownerEmail segment. + * stripped if present. A path whose leading segment is not a recognized prefix is parsed + * as-is, with that segment treated as the ownerEmail. * * @param fileName The file path to parse * @return Some((ownerEmail, datasetName, versionName, fileRelativePath)) if valid, None otherwise 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 306f0235f6a..426c34fbaaa 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 @@ -83,8 +83,8 @@ class FileResolverSpec private val dataset1TxtFilePath = "/datasets/test_user@test.com/test_dataset/v1/1.txt" - // Legacy (pre-prefix) format, kept resolvable for backward compatibility. - private val legacyDataset1TxtFilePath = "/test_user@test.com/test_dataset/v1/1.txt" + // Unprefixed form (no resource-type segment); parsed the same way. + private val unprefixedDataset1TxtFilePath = "/test_user@test.com/test_dataset/v1/1.txt" // "models" is not (yet) a registered resource-type prefix, so it must NOT be stripped. private val unknownPrefixFilePath = "/models/test_user@test.com/test_dataset/v1/1.txt" @@ -124,13 +124,13 @@ class FileResolverSpec ) } - "FileResolver" should "resolve a legacy unprefixed dataset path identically to the prefixed path" in { + "FileResolver" should "resolve an unprefixed dataset path identically to the prefixed path" in { val prefixedUri = FileResolver.resolve(dataset1TxtFilePath) - val legacyUri = FileResolver.resolve(legacyDataset1TxtFilePath) + val unprefixedUri = FileResolver.resolve(unprefixedDataset1TxtFilePath) - assert(legacyUri == prefixedUri) + assert(unprefixedUri == prefixedUri) assert( - legacyUri.toString == f"${FileResolver.DATASET_FILE_URI_SCHEME}:///${testDataset.getRepositoryName}/${testDatasetVersion1.getVersionHash}/1.txt" + unprefixedUri.toString == f"${FileResolver.DATASET_FILE_URI_SCHEME}:///${testDataset.getRepositoryName}/${testDatasetVersion1.getVersionHash}/1.txt" ) } diff --git a/frontend/src/app/common/type/dataset-file.spec.ts b/frontend/src/app/common/type/dataset-file.spec.ts deleted file mode 100644 index 6f07ff9f5a6..00000000000 --- a/frontend/src/app/common/type/dataset-file.spec.ts +++ /dev/null @@ -1,78 +0,0 @@ -/** - * Licensed to the Apache Software Foundation (ASF) under one - * or more contributor license agreements. See the NOTICE file - * distributed with this work for additional information - * regarding copyright ownership. The ASF licenses this file - * to you under the Apache License, Version 2.0 (the - * "License"); you may not use this file except in compliance - * with the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, - * software distributed under the License is distributed on an - * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - * KIND, either express or implied. See the License for the - * specific language governing permissions and limitations - * under the License. - */ - -import { parseFilePathToDatasetFile, parseDatasetFileToFilePath } from "./dataset-file"; - -describe("dataset-file path helpers", () => { - describe("parseFilePathToDatasetFile", () => { - it("parses a datasets-prefixed path", () => { - expect( - parseFilePathToDatasetFile("/datasets/bob@texera.com/twitterDataset/v1/california/irvine/tw1.csv") - ).toEqual({ - ownerEmail: "bob@texera.com", - datasetName: "twitterDataset", - versionName: "v1", - fileRelativePath: "california/irvine/tw1.csv", - }); - }); - - it("still parses a legacy unprefixed path (backward compatibility)", () => { - expect(parseFilePathToDatasetFile("/bob@texera.com/twitterDataset/v1/california/irvine/tw1.csv")).toEqual({ - ownerEmail: "bob@texera.com", - datasetName: "twitterDataset", - versionName: "v1", - fileRelativePath: "california/irvine/tw1.csv", - }); - }); - - it("does not strip a non-datasets leading segment (only 'datasets' is a prefix)", () => { - // "models" is treated as the ownerEmail segment, matching FileResolver on the backend. - expect(parseFilePathToDatasetFile("/models/bob@texera.com/twitterDataset/v1/tw1.csv")).toEqual({ - ownerEmail: "models", - datasetName: "bob@texera.com", - versionName: "twitterDataset", - fileRelativePath: "v1/tw1.csv", - }); - }); - - it("throws when the path has fewer than four segments", () => { - expect(() => parseFilePathToDatasetFile("/datasets/bob@texera.com/twitterDataset")).toThrowError( - "Invalid file path format" - ); - }); - }); - - describe("parseDatasetFileToFilePath", () => { - it("emits the datasets prefix", () => { - expect( - parseDatasetFileToFilePath({ - ownerEmail: "bob@texera.com", - datasetName: "twitterDataset", - versionName: "v1", - fileRelativePath: "california/irvine/tw1.csv", - }) - ).toBe("/datasets/bob@texera.com/twitterDataset/v1/california/irvine/tw1.csv"); - }); - }); - - it("round-trips a prefixed path through parse then build", () => { - const path = "/datasets/bob@texera.com/twitterDataset/v1/california/irvine/tw1.csv"; - expect(parseDatasetFileToFilePath(parseFilePathToDatasetFile(path))).toBe(path); - }); -}); diff --git a/frontend/src/app/common/type/dataset-file.ts b/frontend/src/app/common/type/dataset-file.ts deleted file mode 100644 index 64d8224555b..00000000000 --- a/frontend/src/app/common/type/dataset-file.ts +++ /dev/null @@ -1,66 +0,0 @@ -/** - * Licensed to the Apache Software Foundation (ASF) under one - * or more contributor license agreements. See the NOTICE file - * distributed with this work for additional information - * regarding copyright ownership. The ASF licenses this file - * to you under the Apache License, Version 2.0 (the - * "License"); you may not use this file except in compliance - * with the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, - * software distributed under the License is distributed on an - * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - * KIND, either express or implied. See the License for the - * specific language governing permissions and limitations - * under the License. - */ - -// user given filePath is /datasets/ownerEmail/datasetName/versionName/fileRelativePath -// e.g. /datasets/bob@texera.com/twitterDataset/v1/california/irvine/tw1.csv -export interface DatasetFile { - ownerEmail: string; - datasetName: string; - versionName: string; - fileRelativePath: string; -} - -/** - * Parses a file path string to a DatasetFile interface. - * The first segment "datasets" is stripped before parsing. - * @param filePath - The file path string to parse. - * @returns The parsed DatasetFile object. - */ -export function parseFilePathToDatasetFile(filePath: string): DatasetFile { - let parts = filePath.split("/").filter(part => part.length > 0); - - // Strip the "datasets" prefix if present - if (parts.length > 0 && parts[0] === "datasets") { - parts = parts.slice(1); - } - - if (parts.length < 4) { - throw new Error("Invalid file path format"); - } - - const [ownerEmail, datasetName, versionName, ...fileRelativePathParts] = parts; - const fileRelativePath = fileRelativePathParts.join("/"); - - return { - ownerEmail, - datasetName, - versionName, - fileRelativePath, - }; -} - -/** - * Converts a DatasetFile object to a file path string. - * @param datasetFile - The DatasetFile object to convert. - * @returns The file path string. - */ -export function parseDatasetFileToFilePath(datasetFile: DatasetFile): string { - const { ownerEmail, datasetName, versionName, fileRelativePath } = datasetFile; - return `/datasets/${ownerEmail}/${datasetName}/${versionName}/${fileRelativePath}`; -} 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 18b2319fdd2..8980a6e82b2 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 @@ -79,8 +79,8 @@ export class DatasetSelectionModalComponent implements OnInit { const selectedPath = this.data.selectedPath; if (selectedPath) { const segments = selectedPath.split("/").filter(part => part.length > 0); - // Drop the resource-type prefix ("datasets") if present, so owner/dataset/version - // line up. Tolerates legacy paths saved before the prefix existed. + // Drop the resource-type prefix ("datasets") if present so owner/dataset/version + // line up (the stored path may or may not carry the prefix). if (segments[0] === "datasets") { segments.shift(); } From a31c8977b56770a02672a1ad8d27617592a044a6 Mon Sep 17 00:00:00 2001 From: Tanishq Gandhi Date: Fri, 17 Jul 2026 14:27:01 -0700 Subject: [PATCH 05/16] test(frontend): use datasets-prefixed path in cover-image spec The datasets logical-path prefix strips four leading segments (datasets/owner/dataset/version); update the cover-image test's input path to include the prefix so the extracted relative path is the file name, not an empty string. Co-Authored-By: Claude Opus 4.8 --- .../user-dataset-version-filetree.component.spec.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frontend/src/app/dashboard/component/user/user-dataset/user-dataset-explorer/user-dataset-version-filetree/user-dataset-version-filetree.component.spec.ts b/frontend/src/app/dashboard/component/user/user-dataset/user-dataset-explorer/user-dataset-version-filetree/user-dataset-version-filetree.component.spec.ts index 0abb9626218..673c1d39bda 100644 --- a/frontend/src/app/dashboard/component/user/user-dataset/user-dataset-explorer/user-dataset-version-filetree/user-dataset-version-filetree.component.spec.ts +++ b/frontend/src/app/dashboard/component/user/user-dataset/user-dataset-explorer/user-dataset-version-filetree/user-dataset-version-filetree.component.spec.ts @@ -167,9 +167,9 @@ describe("UserDatasetVersionFiletreeComponent", () => { const emitted: string[] = []; component.setCoverImage.subscribe((path: string) => emitted.push(path)); - // parentDir has exactly the three stripped segments (owner/dataset/version), + // parentDir has exactly the four stripped segments (datasets/owner/dataset/version), // so the relative path is just the file name. - component.onSetCover({ name: "photo.png", type: "file", parentDir: "/owner/dataset/v1" }); + component.onSetCover({ name: "photo.png", type: "file", parentDir: "/datasets/owner/dataset/v1" }); expect(emitted).toEqual(["photo.png"]); }); From 6c76f6b7aeb93609b33857830c12b31869463464 Mon Sep 17 00:00:00 2001 From: Tanishq Gandhi Date: Tue, 21 Jul 2026 14:30:30 -0700 Subject: [PATCH 06/16] feat(storage): require the datasets prefix on dataset logical paths Now that models are coming as a separate resource/table, the leading resource-type segment is what selects the backing table, so an unprefixed path can no longer be routed unambiguously. Make the "datasets" prefix required instead of a tolerated fallback, and migrate existing data. - FileResolver: a dataset path must start with the "datasets" segment; unprefixed paths are no longer treated as dataset paths. - pytexera DatasetFileDocument: same rule, mirroring the backend. - FileListerSourceOpExec: parse the now-prefixed datasetVersionPath (skip the "datasets" segment); extracted into a testable helper. - Migration (sql/updates/29.sql): prepend "datasets/" to legacy paths stored in workflow.content and workflow_version.content, covering both the fileName (scan sources) and datasetVersionPath (file lister) operator properties. Only values whose first two segments match an existing (user.email, dataset.name) are rewritten, so local paths and URLs are left untouched; email format is irrelevant (owner may be a username without "@"). Uses create_missing=false and is idempotent. - Example workflows: use datasets-prefixed paths. Tests: FileResolverSpec and WorkflowExecutionsResourceSpec updated; test_dataset_file_document.py updated; new FileListerSourceOpExecSpec. Co-Authored-By: Claude Opus 4.8 --- .../pytexera/storage/dataset_file_document.py | 28 ++- .../storage/test_dataset_file_document.py | 55 +++--- .../WorkflowExecutionsResourceSpec.scala | 4 +- ...e] Data Exploration on Movies Dataset.json | 2 +- ...ple] Machine Learning on Iris Dataset.json | 2 +- .../amber/core/storage/FileResolver.scala | 34 ++-- .../amber/storage/FileResolverSpec.scala | 27 ++- .../dataset/FileListerSourceOpExec.scala | 29 ++- .../dataset/FileListerSourceOpExecSpec.scala | 67 +++++++ sql/changelog.xml | 5 + sql/updates/29.sql | 180 ++++++++++++++++++ 11 files changed, 348 insertions(+), 85 deletions(-) create mode 100644 common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/source/dataset/FileListerSourceOpExecSpec.scala create mode 100644 sql/updates/29.sql 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 a0048177124..57185566571 100644 --- a/amber/src/main/python/pytexera/storage/dataset_file_document.py +++ b/amber/src/main/python/pytexera/storage/dataset_file_document.py @@ -24,9 +24,9 @@ class DatasetFileDocument: - # Leading resource-type segment on dataset logical paths. It is stripped when - # parsing and re-emitted when calling the file-service, mirroring FileResolver - # on the backend. An unprefixed path is parsed as-is. + # Leading resource-type segment on dataset logical paths. It identifies the path as + # a dataset path and is REQUIRED, mirroring FileResolver on the backend. A path + # without it is rejected. RESOURCE_TYPE_PREFIX = "datasets" # (connect, read) timeout and retry settings for the file-service GETs below. @@ -65,26 +65,24 @@ def __init__(self, file_path: str): "/datasets/ownerEmail/datasetName/versionName/fileRelativePath" Example: "/datasets/bob@texera.com/twitterDataset/v1/california/tw1.csv" - A leading "datasets" resource-type segment is stripped if present; an - unprefixed path is parsed as-is. + The leading "datasets" resource-type segment is required; a path + without it is rejected. Mirrors FileResolver on the backend. """ parts = file_path.strip("/").split("/") - # Strip the resource-type prefix ("datasets") if present; an unprefixed - # path is parsed as-is. Mirrors FileResolver on the backend. - if parts and parts[0] == self.RESOURCE_TYPE_PREFIX: - parts = parts[1:] - - if len(parts) < 4: + # The datasets prefix is required and identifies the path as a dataset + # path; without it (datasets/owner/name/version/ => >= 5 segments) + # the path is invalid. Mirrors FileResolver on the backend. + if len(parts) < 5 or parts[0] != self.RESOURCE_TYPE_PREFIX: raise ValueError( "Invalid file path format. Expected: " "/datasets/ownerEmail/datasetName/versionName/fileRelativePath" ) - self.owner_email = parts[0] - self.dataset_name = parts[1] - self.version_name = parts[2] - self.file_relative_path = "/".join(parts[3:]) + self.owner_email = parts[1] + self.dataset_name = parts[2] + self.version_name = parts[3] + self.file_relative_path = "/".join(parts[4:]) 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 75caf511ba4..925d51e078c 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 @@ -44,67 +44,61 @@ def make_response(status_code: int, body=None, content: bytes = b""): class TestDatasetFileDocumentInit: - def test_parses_minimal_four_part_path(self, auth_env): - doc = DatasetFileDocument("/bob@x.com/ds/v1/file.csv") + def test_parses_prefixed_path(self, auth_env): + doc = DatasetFileDocument("/datasets/bob@x.com/ds/v1/file.csv") assert doc.owner_email == "bob@x.com" assert doc.dataset_name == "ds" assert doc.version_name == "v1" assert doc.file_relative_path == "file.csv" def test_joins_nested_relative_path_back_with_slashes(self, auth_env): - doc = DatasetFileDocument("/bob@x.com/ds/v1/a/b/c/file.csv") + doc = DatasetFileDocument("/datasets/bob@x.com/ds/v1/a/b/c/file.csv") assert doc.file_relative_path == "a/b/c/file.csv" def test_strips_leading_and_trailing_slashes_before_parsing(self, auth_env): - doc = DatasetFileDocument("///bob@x.com/ds/v1/file.csv///") + doc = DatasetFileDocument("///datasets/bob@x.com/ds/v1/file.csv///") assert doc.owner_email == "bob@x.com" assert doc.file_relative_path == "file.csv" - def test_strips_datasets_resource_type_prefix(self, auth_env): - doc = DatasetFileDocument("/datasets/bob@x.com/ds/v1/file.csv") - assert doc.owner_email == "bob@x.com" - assert doc.dataset_name == "ds" - assert doc.version_name == "v1" - 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("/bob@x.com/ds/v1/file.csv") - def test_only_datasets_prefix_is_stripped(self, auth_env): - # Any other leading segment (e.g. "models") is treated as the owner email, - # matching the backend which only registers "datasets" as a prefix. - doc = DatasetFileDocument("/models/bob@x.com/ds/v1/file.csv") - assert doc.owner_email == "models" - assert doc.dataset_name == "bob@x.com" - assert doc.version_name == "ds" - assert doc.file_relative_path == "v1/file.csv" + def test_rejects_non_datasets_prefix(self, auth_env): + # Any other leading segment (e.g. "models") is not the datasets prefix. + with pytest.raises(ValueError, match="Invalid file path format"): + DatasetFileDocument("/models/bob@x.com/ds/v1/file.csv") - def test_rejects_path_with_fewer_than_four_segments(self, auth_env): + def test_rejects_path_with_too_few_segments(self, auth_env): with pytest.raises(ValueError, match="Invalid file path format"): - DatasetFileDocument("/bob@x.com/ds/v1") + DatasetFileDocument("/datasets/bob@x.com/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) with pytest.raises(ValueError, match="JWT token is required"): - DatasetFileDocument("/bob@x.com/ds/v1/file.csv") + DatasetFileDocument("/datasets/bob@x.com/ds/v1/file.csv") def test_treats_empty_jwt_as_missing(self, monkeypatch): # An empty string is falsy and should be rejected just like an unset var. monkeypatch.setenv("USER_JWT_TOKEN", "") with pytest.raises(ValueError, match="JWT token is required"): - DatasetFileDocument("/bob@x.com/ds/v1/file.csv") + DatasetFileDocument("/datasets/bob@x.com/ds/v1/file.csv") def test_falls_back_to_default_endpoint_when_env_missing(self, monkeypatch): monkeypatch.setenv("USER_JWT_TOKEN", "tok") monkeypatch.delenv("FILE_SERVICE_GET_PRESIGNED_URL_ENDPOINT", raising=False) - doc = DatasetFileDocument("/bob@x.com/ds/v1/file.csv") + doc = DatasetFileDocument("/datasets/bob@x.com/ds/v1/file.csv") assert doc.presign_endpoint == DEFAULT_ENDPOINT def test_uses_explicit_endpoint_from_environment(self, auth_env): - doc = DatasetFileDocument("/bob@x.com/ds/v1/file.csv") + doc = DatasetFileDocument("/datasets/bob@x.com/ds/v1/file.csv") assert doc.presign_endpoint == CUSTOM_ENDPOINT class TestGetPresignedUrl: - def _make_doc(self, monkeypatch, path="/bob@x.com/ds/v1/file.csv"): + def _make_doc(self, monkeypatch, path="/datasets/bob@x.com/ds/v1/file.csv"): monkeypatch.setenv("USER_JWT_TOKEN", "test-jwt-token") monkeypatch.setenv("FILE_SERVICE_GET_PRESIGNED_URL_ENDPOINT", CUSTOM_ENDPOINT) return DatasetFileDocument(path) @@ -132,7 +126,7 @@ def test_sends_bearer_authorization_header_with_jwt(self, monkeypatch): def test_url_encodes_filepath_query_parameter(self, monkeypatch): # urllib.parse.quote keeps "/" as safe by default, but encodes "@" # and " " — pin both pieces so the contract is explicit. - doc = self._make_doc(monkeypatch, path="/bob@x.com/ds/v1/data file.csv") + doc = self._make_doc(monkeypatch, path="/datasets/bob@x.com/ds/v1/data file.csv") with patch( "pytexera.storage.dataset_file_document.requests.Session.get" ) as mock_get: @@ -145,9 +139,8 @@ def test_url_encodes_filepath_query_parameter(self, monkeypatch): assert file_path.startswith("/") def test_sends_datasets_prefixed_filepath(self, monkeypatch): - # The reconstructed filePath is re-emitted with the "datasets" prefix, - # even when the input path was the legacy unprefixed form. - doc = self._make_doc(monkeypatch, path="/bob@x.com/ds/v1/file.csv") + # The reconstructed filePath sent to the file-service carries the "datasets" prefix. + doc = self._make_doc(monkeypatch, path="/datasets/bob@x.com/ds/v1/file.csv") with patch( "pytexera.storage.dataset_file_document.requests.Session.get" ) as mock_get: @@ -220,7 +213,7 @@ class TestReadFile: def _make_doc(self, monkeypatch): monkeypatch.setenv("USER_JWT_TOKEN", "test-jwt-token") monkeypatch.setenv("FILE_SERVICE_GET_PRESIGNED_URL_ENDPOINT", CUSTOM_ENDPOINT) - return DatasetFileDocument("/bob@x.com/ds/v1/file.csv") + return DatasetFileDocument("/datasets/bob@x.com/ds/v1/file.csv") def test_returns_bytesio_with_downloaded_content(self, monkeypatch): doc = self._make_doc(monkeypatch) @@ -274,7 +267,7 @@ class TestTimeoutsAndRetries: def _make_doc(self, monkeypatch): monkeypatch.setenv("USER_JWT_TOKEN", "test-jwt-token") monkeypatch.setenv("FILE_SERVICE_GET_PRESIGNED_URL_ENDPOINT", CUSTOM_ENDPOINT) - return DatasetFileDocument("/bob@x.com/ds/v1/file.csv") + return DatasetFileDocument("/datasets/bob@x.com/ds/v1/file.csv") def test_presigned_url_request_passes_request_timeout(self, monkeypatch): doc = self._make_doc(monkeypatch) diff --git a/amber/src/test/scala/org/apache/texera/web/resource/dashboard/user/workflow/WorkflowExecutionsResourceSpec.scala b/amber/src/test/scala/org/apache/texera/web/resource/dashboard/user/workflow/WorkflowExecutionsResourceSpec.scala index c01489aa865..e9c92d4ac5e 100644 --- a/amber/src/test/scala/org/apache/texera/web/resource/dashboard/user/workflow/WorkflowExecutionsResourceSpec.scala +++ b/amber/src/test/scala/org/apache/texera/web/resource/dashboard/user/workflow/WorkflowExecutionsResourceSpec.scala @@ -666,7 +666,7 @@ class WorkflowExecutionsResourceSpec val content = """{ | "operators": [ - | {"operatorID": "scanA", "operatorProperties": {"fileName": "/owner@example.com/LockedDS/v1/data.csv"}}, + | {"operatorID": "scanA", "operatorProperties": {"fileName": "/datasets/owner@example.com/LockedDS/v1/data.csv"}}, | {"operatorID": "downstreamB", "operatorProperties": {}} | ], | "links": [ @@ -725,7 +725,7 @@ class WorkflowExecutionsResourceSpec val content = """{ | "operators": [ - | {"operatorID": "scan", "operatorProperties": {"fileName": "/test@example.com/MyDS/v1/data.csv"}} + | {"operatorID": "scan", "operatorProperties": {"fileName": "/datasets/test@example.com/MyDS/v1/data.csv"}} | ], | "links": [] |}""".stripMargin diff --git a/bin/single-node/examples/workflows/[Example] Data Exploration on Movies Dataset.json b/bin/single-node/examples/workflows/[Example] Data Exploration on Movies Dataset.json index 3e9f99c613f..cdc9a0e8708 100644 --- a/bin/single-node/examples/workflows/[Example] Data Exploration on Movies Dataset.json +++ b/bin/single-node/examples/workflows/[Example] Data Exploration on Movies Dataset.json @@ -8,7 +8,7 @@ "fileEncoding": "UTF_8", "customDelimiter": ",", "hasHeader": true, - "fileName": "/texera/popular-movies-of-imdb/v1/TMDb_updated.csv", + "fileName": "/datasets/texera/popular-movies-of-imdb/v1/TMDb_updated.csv", "offset": 0, "limit": 1000 }, diff --git a/bin/single-node/examples/workflows/[Example] Machine Learning on Iris Dataset.json b/bin/single-node/examples/workflows/[Example] Machine Learning on Iris Dataset.json index d36d35dae56..7e9a5d11fd4 100644 --- a/bin/single-node/examples/workflows/[Example] Machine Learning on Iris Dataset.json +++ b/bin/single-node/examples/workflows/[Example] Machine Learning on Iris Dataset.json @@ -8,7 +8,7 @@ "fileEncoding": "UTF_8", "customDelimiter": ",", "hasHeader": true, - "fileName": "/texera/iris-species/v1/Iris.csv" + "fileName": "/datasets/texera/iris-species/v1/Iris.csv" }, "inputPorts": [], "outputPorts": [ 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 c408a54dbc0..ae2bde1ba97 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 @@ -75,18 +75,19 @@ object FileResolver { filePath.toUri } - private val RESOURCE_TYPE_PREFIXES = Set("datasets") + // The resource-type segment that identifies a dataset logical path. The prefix selects the + // resource kind (and thus the backing table) during resolution, so it is REQUIRED: a path + // without it is not a dataset path. Other resource types (e.g. models) get their own prefix. + private val DATASET_PATH_PREFIX = "datasets" /** * Parses a dataset file path and extracts its components. * - * Two path shapes are accepted: - * - Prefixed: /datasets/ownerEmail/datasetName/versionName/fileRelativePath - * - Unprefixed: /ownerEmail/datasetName/versionName/fileRelativePath + * Expected format: /datasets/ownerEmail/datasetName/versionName/fileRelativePath * - * A leading resource-type prefix listed in [[RESOURCE_TYPE_PREFIXES]] (e.g. "datasets") is - * stripped if present. A path whose leading segment is not a recognized prefix is parsed - * as-is, with that segment treated as the ownerEmail. + * The leading [[DATASET_PATH_PREFIX]] segment is required — it identifies the path as a + * dataset path (as opposed to another resource type). A path without the prefix is not a + * dataset path and yields None (it may still resolve as a local file or fail). * * @param fileName The file path to parse * @return Some((ownerEmail, datasetName, versionName, fileRelativePath)) if valid, None otherwise @@ -95,21 +96,18 @@ object FileResolver { fileName: String ): Option[(String, String, String, Array[String])] = { val filePath = Paths.get(fileName) - var pathSegments = (0 until filePath.getNameCount).map(filePath.getName(_).toString).toArray - - // Strip known resource type prefix if present - if (pathSegments.nonEmpty && RESOURCE_TYPE_PREFIXES.contains(pathSegments(0))) { - pathSegments = pathSegments.drop(1) - } + val pathSegments = (0 until filePath.getNameCount).map(filePath.getName(_).toString).toArray - if (pathSegments.length < 4) { + // A dataset path must start with the datasets prefix and carry at least + // datasets/ownerEmail/datasetName/versionName/ (5 segments). + if (pathSegments.length < 5 || pathSegments(0) != DATASET_PATH_PREFIX) { return None } - val ownerEmail = pathSegments(0) - val datasetName = pathSegments(1) - val versionName = pathSegments(2) - val fileRelativePathSegments = pathSegments.drop(3) + val ownerEmail = pathSegments(1) + val datasetName = pathSegments(2) + val versionName = pathSegments(3) + val fileRelativePathSegments = pathSegments.drop(4) Some((ownerEmail, datasetName, versionName, fileRelativePathSegments)) } 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 426c34fbaaa..922f26ac3c9 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 @@ -83,11 +83,11 @@ class FileResolverSpec private val dataset1TxtFilePath = "/datasets/test_user@test.com/test_dataset/v1/1.txt" - // Unprefixed form (no resource-type segment); parsed the same way. + // Unprefixed form (no resource-type segment); no longer resolvable as a dataset. private val unprefixedDataset1TxtFilePath = "/test_user@test.com/test_dataset/v1/1.txt" - // "models" is not (yet) a registered resource-type prefix, so it must NOT be stripped. - private val unknownPrefixFilePath = "/models/test_user@test.com/test_dataset/v1/1.txt" + // "models" is not the dataset prefix, so this is not a dataset path. + private val nonDatasetPrefixFilePath = "/models/test_user@test.com/test_dataset/v1/1.txt" override protected def beforeAll(): Unit = { initializeDBAndReplaceDSLContext() @@ -124,21 +124,18 @@ class FileResolverSpec ) } - "FileResolver" should "resolve an unprefixed dataset path identically to the prefixed path" in { - val prefixedUri = FileResolver.resolve(dataset1TxtFilePath) - val unprefixedUri = FileResolver.resolve(unprefixedDataset1TxtFilePath) - - assert(unprefixedUri == prefixedUri) - assert( - unprefixedUri.toString == f"${FileResolver.DATASET_FILE_URI_SCHEME}:///${testDataset.getRepositoryName}/${testDatasetVersion1.getVersionHash}/1.txt" - ) + "FileResolver" should "not resolve an unprefixed dataset path (the datasets prefix is required)" in { + // Without the datasets prefix the path is not a dataset path; it falls through to the + // local-file resolver and fails. + assertThrows[FileNotFoundException] { + FileResolver.resolve(unprefixedDataset1TxtFilePath) + } } - "FileResolver" should "not strip an unregistered resource-type prefix" in { - // "models" is treated as the ownerEmail segment, so the dataset lookup fails and - // resolution falls back to a (missing) local file. + "FileResolver" should "not resolve a path whose prefix is not the datasets prefix" in { + // "models" is not the dataset prefix, so this is not a dataset path. assertThrows[FileNotFoundException] { - FileResolver.resolve(unknownPrefixFilePath) + FileResolver.resolve(nonDatasetPrefixFilePath) } } 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 c58da8ca847..82872cb6c7b 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 @@ -28,13 +28,38 @@ import org.apache.texera.dao.jooq.generated.tables.Dataset.DATASET import org.apache.texera.dao.jooq.generated.tables.DatasetVersion.DATASET_VERSION import org.apache.texera.dao.jooq.generated.tables.User.USER +object FileListerSourceOpExec { + + /** + * Parses a dataset version path into its (ownerEmail, datasetName, versionName) components. + * + * Expected format: /datasets/ownerEmail/datasetName/versionName + * + * The leading "datasets" resource-type prefix is required (mirroring FileResolver); empty + * segments produced by leading/trailing slashes are ignored. + * + * @throws IllegalArgumentException if the path is not a prefixed dataset version path + */ + private[dataset] def parseDatasetVersionPath( + datasetVersionPath: String + ): (String, String, String) = { + val segments = datasetVersionPath.split("/").filter(_.nonEmpty) + require( + segments.length >= 4 && segments.head == "datasets", + s"Invalid dataset version path '$datasetVersionPath'; " + + "expected /datasets/ownerEmail/datasetName/versionName" + ) + (segments(1), segments(2), segments(3)) + } +} + class FileListerSourceOpExec private[dataset] (descString: String) extends SourceOperatorExecutor { private val desc: FileListerSourceOpDesc = objectMapper.readValue(descString, classOf[FileListerSourceOpDesc]) override def produceTuple(): Iterator[TupleLike] = { - val Seq(_, ownerEmail, datasetName, versionName, _*) = - desc.datasetVersionPath.split("/").toSeq + val (ownerEmail, datasetName, versionName) = + FileListerSourceOpExec.parseDatasetVersionPath(desc.datasetVersionPath) val (repositoryName, versionHash) = SqlServer 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 new file mode 100644 index 00000000000..b5b5611c991 --- /dev/null +++ b/common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/source/dataset/FileListerSourceOpExecSpec.scala @@ -0,0 +1,67 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.texera.amber.operator.source.dataset + +import org.scalatest.flatspec.AnyFlatSpec + +class FileListerSourceOpExecSpec extends AnyFlatSpec { + + "parseDatasetVersionPath" should "extract components from a datasets-prefixed path" in { + val (owner, name, version) = + FileListerSourceOpExec.parseDatasetVersionPath("/datasets/bob@texera.com/twitterDataset/v1") + assert(owner == "bob@texera.com") + assert(name == "twitterDataset") + assert(version == "v1") + } + + it should "work when the owner segment is a username without an '@'" in { + val (owner, name, version) = + FileListerSourceOpExec.parseDatasetVersionPath("/datasets/texera/test-ds/v1") + assert(owner == "texera") + assert(name == "test-ds") + assert(version == "v1") + } + + it should "ignore trailing slashes and extra segments" in { + val (owner, name, version) = + FileListerSourceOpExec.parseDatasetVersionPath("/datasets/alice/ds/v2/extra/") + assert(owner == "alice") + assert(name == "ds") + assert(version == "v2") + } + + it should "reject an unprefixed path (the datasets prefix is required)" in { + assertThrows[IllegalArgumentException] { + FileListerSourceOpExec.parseDatasetVersionPath("/alice/ds/v1") + } + } + + it should "reject a path whose prefix is not the datasets prefix" in { + assertThrows[IllegalArgumentException] { + FileListerSourceOpExec.parseDatasetVersionPath("/models/alice/ds/v1") + } + } + + it should "reject a path with too few segments" in { + assertThrows[IllegalArgumentException] { + FileListerSourceOpExec.parseDatasetVersionPath("/datasets/alice/ds") + } + } +} diff --git a/sql/changelog.xml b/sql/changelog.xml index 469868f8958..1c8219c5458 100644 --- a/sql/changelog.xml +++ b/sql/changelog.xml @@ -53,6 +53,11 @@ + + + + + + + + + + diff --git a/sql/updates/29.sql b/sql/updates/29.sql index fc3ff67a3de..50e74d557ff 100644 --- a/sql/updates/29.sql +++ b/sql/updates/29.sql @@ -26,17 +26,16 @@ BEGIN; -- The file resolver now requires an explicit resource-type prefix on dataset -- logical paths (/datasets/ownerEmail/datasetName/versionName/...) so other -- resource types (e.g. models) can be told apart by the prefix. Existing --- workflows store legacy, unprefixed dataset paths inside workflow.content and +-- workflows store unprefixed dataset paths inside workflow.content and -- workflow_version.content, in two operator properties: -- * fileName (scan-source operators): /owner/name/version/file -- * datasetVersionPath (file-lister operator): /owner/name/version -- This migration prepends the "datasets" segment to both. -- -- A value is treated as a dataset path only when its first two segments match an --- existing (user.email, dataset.name) pair -- that pair is unique, and this test --- is independent of the email format (Texera sets email = username, so an owner --- segment need not contain "@"). Local file paths and URLs match no dataset and --- are left untouched. Already-prefixed values are skipped (idempotent). jsonb_set +-- existing (user.email, dataset.name) pair -- that pair is unique. +-- Local file paths and URLs match no dataset and are left untouched. +-- Already-prefixed values are skipped (idempotent). jsonb_set -- uses create_missing = false so absent properties are never added. DO $$ From 144ee2f2465db67b058a6b03ee305f3ad1e67103 Mon Sep 17 00:00:00 2001 From: Tanishq Gandhi Date: Fri, 24 Jul 2026 11:54:02 -0700 Subject: [PATCH 13/16] feat(storage): add model file storage and path resolution --- .../common/config/EnvironmentalVariable.scala | 2 + .../amber/core/storage/DocumentFactory.scala | 7 +- .../amber/core/storage/FileResolver.scala | 171 ++++++++++++----- .../amber/core/storage/ResourceType.scala | 1 + .../storage/model/DatasetFileDocument.scala | 146 +-------------- .../storage/model/LakeFSFileDocument.scala | 175 ++++++++++++++++++ .../storage/model/ModelFileDocument.scala | 43 +++++ ...et.scala => OnVersionedFileResource.scala} | 5 +- .../core/storage/DocumentFactorySpec.scala | 39 +++- .../amber/storage/FileResolverSpec.scala | 98 +++++++++- .../service/resource/DatasetResource.scala | 11 +- sql/texera_ddl.sql | 80 -------- sql/updates/30.sql | 77 +------- 13 files changed, 501 insertions(+), 354 deletions(-) create mode 100644 common/workflow-core/src/main/scala/org/apache/texera/amber/core/storage/model/LakeFSFileDocument.scala create mode 100644 common/workflow-core/src/main/scala/org/apache/texera/amber/core/storage/model/ModelFileDocument.scala rename common/workflow-core/src/main/scala/org/apache/texera/amber/core/storage/model/{OnDataset.scala => OnVersionedFileResource.scala} (90%) diff --git a/common/config/src/main/scala/org/apache/texera/common/config/EnvironmentalVariable.scala b/common/config/src/main/scala/org/apache/texera/common/config/EnvironmentalVariable.scala index a335ddeff6c..a79fc6cef20 100644 --- a/common/config/src/main/scala/org/apache/texera/common/config/EnvironmentalVariable.scala +++ b/common/config/src/main/scala/org/apache/texera/common/config/EnvironmentalVariable.scala @@ -36,6 +36,8 @@ object EnvironmentalVariable { * FileService related endpoint */ val ENV_FILE_SERVICE_GET_PRESIGNED_URL_ENDPOINT = "FILE_SERVICE_GET_PRESIGNED_URL_ENDPOINT" + val ENV_FILE_SERVICE_GET_MODEL_PRESIGNED_URL_ENDPOINT = + "FILE_SERVICE_GET_MODEL_PRESIGNED_URL_ENDPOINT" val ENV_FILE_SERVICE_UPLOAD_ONE_FILE_TO_DATASET_ENDPOINT = "FILE_SERVICE_UPLOAD_ONE_FILE_TO_DATASET_ENDPOINT" diff --git a/common/workflow-core/src/main/scala/org/apache/texera/amber/core/storage/DocumentFactory.scala b/common/workflow-core/src/main/scala/org/apache/texera/amber/core/storage/DocumentFactory.scala index a26340e79cc..a020dfdbb48 100644 --- a/common/workflow-core/src/main/scala/org/apache/texera/amber/core/storage/DocumentFactory.scala +++ b/common/workflow-core/src/main/scala/org/apache/texera/amber/core/storage/DocumentFactory.scala @@ -20,7 +20,10 @@ package org.apache.texera.amber.core.storage import org.apache.texera.common.config.StorageConfig -import org.apache.texera.amber.core.storage.FileResolver.DATASET_FILE_URI_SCHEME +import org.apache.texera.amber.core.storage.FileResolver.{ + DATASET_FILE_URI_SCHEME, + MODEL_FILE_URI_SCHEME +} import org.apache.texera.amber.core.storage.VFSResourceType._ import org.apache.texera.amber.core.storage.VFSURIFactory.{VFS_FILE_URI_SCHEME, decodeURI} import org.apache.texera.amber.core.storage.model._ @@ -58,6 +61,7 @@ object DocumentFactory { def openReadonlyDocument(fileUri: URI): ReadonlyVirtualDocument[_] = { fileUri.getScheme match { case DATASET_FILE_URI_SCHEME => new DatasetFileDocument(fileUri) + case MODEL_FILE_URI_SCHEME => new ModelFileDocument(fileUri) case "file" => new ReadonlyLocalFileDocument(fileUri) case unsupportedScheme => throw new UnsupportedOperationException( @@ -164,6 +168,7 @@ object DocumentFactory { def openDocument(uri: URI): (VirtualDocument[_], Option[Schema]) = { uri.getScheme match { case DATASET_FILE_URI_SCHEME => (new DatasetFileDocument(uri), None) + case MODEL_FILE_URI_SCHEME => (new ModelFileDocument(uri), None) case VFS_FILE_URI_SCHEME => val (_, _, _, resourceType) = decodeURI(uri) val storageKey = sanitizeURIPath(uri) 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 9631cf402ee..8db38a047aa 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 @@ -24,8 +24,15 @@ import org.apache.texera.dao.SqlServer import org.apache.texera.dao.SqlServer.withTransaction import org.apache.texera.dao.jooq.generated.tables.Dataset.DATASET import org.apache.texera.dao.jooq.generated.tables.DatasetVersion.DATASET_VERSION +import org.apache.texera.dao.jooq.generated.tables.Model.MODEL +import org.apache.texera.dao.jooq.generated.tables.ModelVersion.MODEL_VERSION import org.apache.texera.dao.jooq.generated.tables.User.USER -import org.apache.texera.dao.jooq.generated.tables.pojos.{Dataset, DatasetVersion} +import org.apache.texera.dao.jooq.generated.tables.pojos.{ + Dataset, + DatasetVersion, + Model, + ModelVersion +} import java.net.{URI, URLEncoder} import java.nio.charset.StandardCharsets @@ -39,6 +46,7 @@ import scala.util.{Success, Try} object FileResolver { val DATASET_FILE_URI_SCHEME = "dataset" + val MODEL_FILE_URI_SCHEME = "model" /** * Resolves a given fileName to either a file on the local file system or a dataset file. @@ -51,7 +59,7 @@ object FileResolver { if (isFileResolved(fileName)) { return new URI(fileName) } - val resolvers: Seq[String => URI] = Seq(localResolveFunc, datasetResolveFunc) + val resolvers: Seq[String => URI] = Seq(localResolveFunc, datasetResolveFunc, modelResolveFunc) // Try each resolver function in sequence resolvers @@ -76,32 +84,85 @@ 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 + * Parses a versioned-resource file path of the form + * //ownerEmail/resourceName/versionName/fileRelativePath and extracts its components. * + * @param resourceType the resource-type segment the path must start with * @param fileName The file path to parse - * @return Some((ownerEmail, datasetName, versionName, fileRelativePath)) if valid, None otherwise + * @return Some((ownerEmail, resourceName, versionName, fileRelativePath)) if valid, None otherwise */ - private def parseDatasetFilePath( + private def parsePrefixedPath( + resourceType: ResourceType.Value, fileName: String ): Option[(String, String, String, Array[String])] = { val filePath = Paths.get(fileName) val pathSegments = (0 until filePath.getNameCount).map(filePath.getName(_).toString).toArray - if (pathSegments.length < 5 || pathSegments(0) != ResourceType.Datasets.toString) { + if (pathSegments.length < 5 || pathSegments(0) != resourceType.toString) { return None } val ownerEmail = pathSegments(1) - val datasetName = pathSegments(2) + val resourceName = pathSegments(2) val versionName = pathSegments(3) val fileRelativePathSegments = pathSegments.drop(4) - Some((ownerEmail, datasetName, versionName, fileRelativePathSegments)) + Some((ownerEmail, resourceName, versionName, fileRelativePathSegments)) + } + + private def parseDatasetFilePath( + fileName: String + ): Option[(String, String, String, Array[String])] = + parsePrefixedPath(ResourceType.Datasets, fileName) + + /** + * Resolves a versioned-resource logical path to its physical `scheme:///` URI. + * + * @throws java.io.FileNotFoundException if the path is not a valid `resourceType` path, the + * resource/version does not exist, or the URI is malformed + */ + private def resolveVersionedFile( + scheme: String, + resourceType: ResourceType.Value, + fileName: String, + notFoundMessage: String + )(lookup: (String, String, String) => (String, String)): URI = { + val (ownerEmail, resourceName, versionName, fileRelativePathSegments) = + parsePrefixedPath(resourceType, fileName).getOrElse( + throw new FileNotFoundException(notFoundMessage) + ) + + val (repositoryName, versionHash) = lookup(ownerEmail, resourceName, versionName) + + val fileRelativePath = + Paths.get(fileRelativePathSegments.head, fileRelativePathSegments.tail: _*) + + // Convert each segment of fileRelativePath to an encoded String + val encodedFileRelativePath = fileRelativePath + .iterator() + .asScala + .map { segment => + URLEncoder.encode(segment.toString, StandardCharsets.UTF_8) + } + .toArray + + // Prepend repositoryName and versionHash to the encoded path segments + val allPathSegments = Array(repositoryName, versionHash) ++ encodedFileRelativePath + + // Build the format /{repositoryName}/{versionHash}/{fileRelativePath}, both Linux and Windows use forward slash as the splitter + val uriSplitter = "/" + val encodedPath = uriSplitter + allPathSegments.mkString(uriSplitter) + + try { + new URI(scheme, "", encodedPath, null) + } catch { + case _: Exception => + throw new FileNotFoundException(notFoundMessage) + } } /** - * Attempts to resolve a given fileName to a URI. + * Attempts to resolve a dataset file path to a URI. * * The fileName format should be: /datasets/ownerEmail/datasetName/versionName/fileRelativePath * e.g. /datasets/bob@texera.com/twitterDataset/v1/california/irvine/tw1.csv @@ -109,20 +170,16 @@ object FileResolver { * e.g. {DATASET_FILE_URI_SCHEME}:///dataset-15/adeq233td/some/dir/file.txt * * @param fileName the name of the file to attempt resolving as a DatasetFileDocument - * @return Either[String, DatasetFileDocument] - Right(document) if creation succeeds * @throws java.io.FileNotFoundException if the dataset file does not exist or cannot be created */ - private def datasetResolveFunc(fileName: String): URI = { - val (ownerEmail, datasetName, versionName, fileRelativePathSegments) = - parseDatasetFilePath(fileName).getOrElse( - throw new FileNotFoundException(s"Dataset file $fileName not found.") - ) - - val fileRelativePath = - Paths.get(fileRelativePathSegments.head, fileRelativePathSegments.tail: _*) - - // fetch the dataset and version from DB to get dataset ID and version hash - val (dataset, datasetVersion) = + private def datasetResolveFunc(fileName: String): URI = + resolveVersionedFile( + DATASET_FILE_URI_SCHEME, + ResourceType.Datasets, + fileName, + s"Dataset file $fileName not found." + ) { (ownerEmail, datasetName, versionName) => + // fetch the dataset and version from DB to get the repository name and version hash withTransaction( SqlServer .getInstance() @@ -148,35 +205,57 @@ object FileResolver { if (dataset == null || datasetVersion == null) { throw new FileNotFoundException(s"Dataset file $fileName not found.") } - (dataset, datasetVersion) + (dataset.getRepositoryName, datasetVersion.getVersionHash) } + } - // Convert each segment of fileRelativePath to an encoded String - val encodedFileRelativePath = fileRelativePath - .iterator() - .asScala - .map { segment => - URLEncoder.encode(segment.toString, StandardCharsets.UTF_8) - } - .toArray - - // Prepend dataset name and versionHash to the encoded path segments - val allPathSegments = Array( - dataset.getRepositoryName, - datasetVersion.getVersionHash - ) ++ encodedFileRelativePath + /** + * Attempts to resolve a model file path to a URI. + * + * The fileName format should be: /models/ownerEmail/modelName/versionName/fileRelativePath + * e.g. /models/bob@texera.com/resnet/v1/weights/model.pt + * The output model URI format is: {MODEL_FILE_URI_SCHEME}:///{repositoryName}/{versionHash}/fileRelativePath + * e.g. {MODEL_FILE_URI_SCHEME}:///model-15/adeq233td/weights/model.pt + * + * @param fileName the name of the file to attempt resolving as a ModelFileDocument + * @throws java.io.FileNotFoundException if the model file does not exist or cannot be created + */ + private def modelResolveFunc(fileName: String): URI = + resolveVersionedFile( + MODEL_FILE_URI_SCHEME, + ResourceType.Models, + fileName, + s"Model file $fileName not found." + ) { (ownerEmail, modelName, versionName) => + // fetch the model and version from DB to get the repository name and version hash + withTransaction( + SqlServer + .getInstance() + .createDSLContext() + ) { ctx => + // fetch the model from DB + val model = ctx + .select(MODEL.fields: _*) + .from(MODEL) + .leftJoin(USER) + .on(USER.UID.eq(MODEL.OWNER_UID)) + .where(USER.EMAIL.eq(ownerEmail)) + .and(MODEL.NAME.eq(modelName)) + .fetchOneInto(classOf[Model]) - // Build the format /{repositoryName}/{versionHash}/{fileRelativePath}, both Linux and Windows use forward slash as the splitter - val uriSplitter = "/" - val encodedPath = uriSplitter + allPathSegments.mkString(uriSplitter) + // fetch the model version from DB + val modelVersion = ctx + .selectFrom(MODEL_VERSION) + .where(MODEL_VERSION.MID.eq(model.getMid)) + .and(MODEL_VERSION.NAME.eq(versionName)) + .fetchOneInto(classOf[ModelVersion]) - try { - new URI(DATASET_FILE_URI_SCHEME, "", encodedPath, null) - } catch { - case e: Exception => - throw new FileNotFoundException(s"Dataset file $fileName not found.") + if (model == null || modelVersion == null) { + throw new FileNotFoundException(s"Model file $fileName not found.") + } + (model.getRepositoryName, modelVersion.getVersionHash) + } } - } /** * Checks if a given file path has a valid scheme. diff --git a/common/workflow-core/src/main/scala/org/apache/texera/amber/core/storage/ResourceType.scala b/common/workflow-core/src/main/scala/org/apache/texera/amber/core/storage/ResourceType.scala index 9c83ebd7494..03c1ce4c01f 100644 --- a/common/workflow-core/src/main/scala/org/apache/texera/amber/core/storage/ResourceType.scala +++ b/common/workflow-core/src/main/scala/org/apache/texera/amber/core/storage/ResourceType.scala @@ -27,4 +27,5 @@ package org.apache.texera.amber.core.storage */ object ResourceType extends Enumeration { val Datasets: Value = Value("datasets") + val Models: Value = Value("models") } diff --git a/common/workflow-core/src/main/scala/org/apache/texera/amber/core/storage/model/DatasetFileDocument.scala b/common/workflow-core/src/main/scala/org/apache/texera/amber/core/storage/model/DatasetFileDocument.scala index 6d8f917c7f4..044db2013b9 100644 --- a/common/workflow-core/src/main/scala/org/apache/texera/amber/core/storage/model/DatasetFileDocument.scala +++ b/common/workflow-core/src/main/scala/org/apache/texera/amber/core/storage/model/DatasetFileDocument.scala @@ -19,28 +19,14 @@ package org.apache.texera.amber.core.storage.model -import com.typesafe.scalalogging.LazyLogging import org.apache.texera.common.config.EnvironmentalVariable -import org.apache.texera.amber.core.storage.model.DatasetFileDocument.{ - fileServiceGetPresignURLEndpoint, - userJwtToken -} -import org.apache.texera.amber.core.storage.util.LakeFSStorageClient +import org.apache.texera.amber.core.storage.model.DatasetFileDocument.fileServiceGetPresignURLEndpoint import org.apache.texera.amber.core.storage.util.dataset.GitVersionControlLocalFileStorage -import java.io.{File, FileOutputStream, InputStream} -import java.net._ -import java.nio.charset.StandardCharsets -import java.nio.file.{Files, Path, Paths} -import scala.jdk.CollectionConverters.IteratorHasAsScala +import java.net.URI +import java.nio.file.Path object DatasetFileDocument { - // Since requests need to be sent to the FileService in order to read the file, we store USER_JWT_TOKEN in the environment vars - // This variable should be NON-EMPTY in the dynamic-computing-unit architecture, i.e. each user-created computing unit should store user's jwt token. - // In the local development or other architectures, this token can be empty. - lazy val userJwtToken: String = - sys.env.getOrElse(EnvironmentalVariable.ENV_USER_JWT_TOKEN, "").trim - // The endpoint of getting presigned url from the file service, also stored in the environment vars. lazy val fileServiceGetPresignURLEndpoint: String = sys.env @@ -52,122 +38,12 @@ object DatasetFileDocument { } private[storage] class DatasetFileDocument(uri: URI) - extends VirtualDocument[Nothing] - with OnDataset - with LazyLogging { - // Utility function to parse and decode URI segments into individual components - private def parseUri(uri: URI): (String, String, Path) = { - val segments = Paths.get(uri.getPath).iterator().asScala.map(_.toString).toArray - if (segments.length < 3) - throw new IllegalArgumentException("URI format is incorrect") - - // parse uri to dataset components - val repositoryName = segments(0) - val datasetVersionHash = URLDecoder.decode(segments(1), StandardCharsets.UTF_8) - val decodedRelativeSegments = - segments.drop(2).map(part => URLDecoder.decode(part, StandardCharsets.UTF_8)) - val fileRelativePath = Paths.get(decodedRelativeSegments.head, decodedRelativeSegments.tail: _*) - - (repositoryName, datasetVersionHash, fileRelativePath) - } - - // Extract components from URI using the utility function - private val (repositoryName, datasetVersionHash, fileRelativePath) = parseUri(uri) - - private var tempFile: Option[File] = None - - override def getURI: URI = uri - - override def asInputStream(): InputStream = { - - def fallbackToLakeFS(exception: Throwable): InputStream = { - logger.warn(s"${exception.getMessage}. Falling back to LakeFS direct file fetch.", exception) - val file = LakeFSStorageClient.getFileFromRepo( - getRepositoryName(), - getVersionHash(), - getFileRelativePath() - ) - Files.newInputStream(file.toPath) - } - - if (userJwtToken.isEmpty) { - try { - val presignUrl = LakeFSStorageClient.getFilePresignedUrl( - getRepositoryName(), - getVersionHash(), - getFileRelativePath() - ) - new URL(presignUrl).openStream() - } catch { - case e: Exception => - fallbackToLakeFS(e) - } - } else { - val presignRequestUrl = - s"$fileServiceGetPresignURLEndpoint?repositoryName=${getRepositoryName()}&commitHash=${getVersionHash()}&filePath=${URLEncoder - .encode(getFileRelativePath(), StandardCharsets.UTF_8.name())}" - - val connection = new URL(presignRequestUrl).openConnection().asInstanceOf[HttpURLConnection] - connection.setRequestMethod("GET") - connection.setRequestProperty("Authorization", s"Bearer $userJwtToken") - - try { - if (connection.getResponseCode != HttpURLConnection.HTTP_OK) { - throw new RuntimeException( - s"Failed to retrieve presigned URL: HTTP ${connection.getResponseCode}" - ) - } - - // Read response body as a string - val responseBody = - new String(connection.getInputStream.readAllBytes(), StandardCharsets.UTF_8) - - // Extract presigned URL from JSON response - val presignedUrl = responseBody - .split("\"presignedUrl\"\\s*:\\s*\"")(1) - .split("\"")(0) - - new URL(presignedUrl).openStream() - } catch { - case e: Exception => - fallbackToLakeFS(e) - } finally { - connection.disconnect() - } - } - } - - override def asFile(): File = { - tempFile match { - case Some(file) => file - case None => - val tempFilePath = Files.createTempFile("versionedFile", ".tmp") - val tempFileStream = new FileOutputStream(tempFilePath.toFile) - val inputStream = asInputStream() - - val buffer = new Array[Byte](1024) - - // Create an iterator to repeatedly call inputStream.read, and direct buffered data to file - Iterator - .continually(inputStream.read(buffer)) - .takeWhile(_ != -1) - .foreach(tempFileStream.write(buffer, 0, _)) - - inputStream.close() - tempFileStream.close() - - val file = tempFilePath.toFile - tempFile = Some(file) - file - } - } + extends LakeFSFileDocument(uri, fileServiceGetPresignURLEndpoint) { override def clear(): Unit = { - // first remove the temporary file - tempFile match { - case Some(file) => Files.delete(file.toPath) - case None => // Do nothing - } + // first remove the temporary file (handled by the shared base) + super.clear() + lazy val datasetsRootPath = Path .of(sys.env.getOrElse("TEXERA_HOME", ".")) @@ -179,16 +55,10 @@ private[storage] class DatasetFileDocument(uri: URI) datasetsRootPath.resolve(did.toString) } - // then remove the dataset file + // then remove the dataset file from the local git-backed storage GitVersionControlLocalFileStorage.removeFileFromRepo( getDatasetPath(0), getDatasetPath(0).resolve(fileRelativePath) ) } - - override def getRepositoryName(): String = repositoryName - - override def getVersionHash(): String = datasetVersionHash - - override def getFileRelativePath(): String = fileRelativePath.toString } diff --git a/common/workflow-core/src/main/scala/org/apache/texera/amber/core/storage/model/LakeFSFileDocument.scala b/common/workflow-core/src/main/scala/org/apache/texera/amber/core/storage/model/LakeFSFileDocument.scala new file mode 100644 index 00000000000..044437889e9 --- /dev/null +++ b/common/workflow-core/src/main/scala/org/apache/texera/amber/core/storage/model/LakeFSFileDocument.scala @@ -0,0 +1,175 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.texera.amber.core.storage.model + +import com.typesafe.scalalogging.LazyLogging +import org.apache.texera.common.config.EnvironmentalVariable +import org.apache.texera.amber.core.storage.model.LakeFSFileDocument.userJwtToken +import org.apache.texera.amber.core.storage.util.LakeFSStorageClient + +import java.io.{File, FileOutputStream, InputStream} +import java.net._ +import java.nio.charset.StandardCharsets +import java.nio.file.{Files, Path, Paths} +import scala.jdk.CollectionConverters.IteratorHasAsScala + +object LakeFSFileDocument { + // Since requests need to be sent to the FileService in order to read the file, we store USER_JWT_TOKEN in the environment vars + // This variable should be NON-EMPTY in the dynamic-computing-unit architecture, i.e. each user-created computing unit should store user's jwt token. + // In the local development or other architectures, this token can be empty. + lazy val userJwtToken: String = + sys.env.getOrElse(EnvironmentalVariable.ENV_USER_JWT_TOKEN, "").trim +} + +/** + * A read-only document over a single file stored in a LakeFS repository, addressed by the URI + * {scheme}:///{repositoryName}/{versionHash}/{fileRelativePath}. This is the shared behavior + * for every versioned-file resource (datasets, models, …): the file bytes are fetched via a + * presigned URL, falling back to a direct LakeFS fetch. + * + * @param uri the resolved {scheme}:///{repositoryName}/{versionHash}/{file} URI + * @param presignEndpoint the file-service presign-download endpoint for this resource kind + */ +private[storage] abstract class LakeFSFileDocument(uri: URI, presignEndpoint: String) + extends VirtualDocument[Nothing] + with OnVersionedFileResource + with LazyLogging { + // Utility function to parse and decode URI segments into individual components + private def parseUri(uri: URI): (String, String, Path) = { + val segments = Paths.get(uri.getPath).iterator().asScala.map(_.toString).toArray + if (segments.length < 3) + throw new IllegalArgumentException("URI format is incorrect") + + // parse uri to (repositoryName, versionHash, fileRelativePath) + val repositoryName = segments(0) + val versionHash = URLDecoder.decode(segments(1), StandardCharsets.UTF_8) + val decodedRelativeSegments = + segments.drop(2).map(part => URLDecoder.decode(part, StandardCharsets.UTF_8)) + val fileRelativePath = Paths.get(decodedRelativeSegments.head, decodedRelativeSegments.tail: _*) + + (repositoryName, versionHash, fileRelativePath) + } + + // Extract components from URI using the utility function + protected val (repositoryName, versionHash, fileRelativePath) = parseUri(uri) + + protected var tempFile: Option[File] = None + + override def getURI: URI = uri + + override def asInputStream(): InputStream = { + + def fallbackToLakeFS(exception: Throwable): InputStream = { + logger.warn(s"${exception.getMessage}. Falling back to LakeFS direct file fetch.", exception) + val file = LakeFSStorageClient.getFileFromRepo( + getRepositoryName(), + getVersionHash(), + getFileRelativePath() + ) + Files.newInputStream(file.toPath) + } + + if (userJwtToken.isEmpty) { + try { + val presignUrl = LakeFSStorageClient.getFilePresignedUrl( + getRepositoryName(), + getVersionHash(), + getFileRelativePath() + ) + new URL(presignUrl).openStream() + } catch { + case e: Exception => + fallbackToLakeFS(e) + } + } else { + val presignRequestUrl = + s"$presignEndpoint?repositoryName=${getRepositoryName()}&commitHash=${getVersionHash()}&filePath=${URLEncoder + .encode(getFileRelativePath(), StandardCharsets.UTF_8.name())}" + + val connection = new URL(presignRequestUrl).openConnection().asInstanceOf[HttpURLConnection] + connection.setRequestMethod("GET") + connection.setRequestProperty("Authorization", s"Bearer $userJwtToken") + + try { + if (connection.getResponseCode != HttpURLConnection.HTTP_OK) { + throw new RuntimeException( + s"Failed to retrieve presigned URL: HTTP ${connection.getResponseCode}" + ) + } + + // Read response body as a string + val responseBody = + new String(connection.getInputStream.readAllBytes(), StandardCharsets.UTF_8) + + // Extract presigned URL from JSON response + val presignedUrl = responseBody + .split("\"presignedUrl\"\\s*:\\s*\"")(1) + .split("\"")(0) + + new URL(presignedUrl).openStream() + } catch { + case e: Exception => + fallbackToLakeFS(e) + } finally { + connection.disconnect() + } + } + } + + override def asFile(): File = { + tempFile match { + case Some(file) => file + case None => + val tempFilePath = Files.createTempFile("versionedFile", ".tmp") + val tempFileStream = new FileOutputStream(tempFilePath.toFile) + val inputStream = asInputStream() + + val buffer = new Array[Byte](1024) + + // Create an iterator to repeatedly call inputStream.read, and direct buffered data to file + Iterator + .continually(inputStream.read(buffer)) + .takeWhile(_ != -1) + .foreach(tempFileStream.write(buffer, 0, _)) + + inputStream.close() + tempFileStream.close() + + val file = tempFilePath.toFile + tempFile = Some(file) + file + } + } + + override def clear(): Unit = { + // remove the temporary file, if one was materialized + tempFile match { + case Some(file) => Files.delete(file.toPath) + case None => // Do nothing + } + tempFile = None + } + + override def getRepositoryName(): String = repositoryName + + override def getVersionHash(): String = versionHash + + override def getFileRelativePath(): String = fileRelativePath.toString +} diff --git a/common/workflow-core/src/main/scala/org/apache/texera/amber/core/storage/model/ModelFileDocument.scala b/common/workflow-core/src/main/scala/org/apache/texera/amber/core/storage/model/ModelFileDocument.scala new file mode 100644 index 00000000000..e67ec92992b --- /dev/null +++ b/common/workflow-core/src/main/scala/org/apache/texera/amber/core/storage/model/ModelFileDocument.scala @@ -0,0 +1,43 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.texera.amber.core.storage.model + +import org.apache.texera.common.config.EnvironmentalVariable +import org.apache.texera.amber.core.storage.model.ModelFileDocument.fileServiceGetModelPresignURLEndpoint + +import java.net.URI + +object ModelFileDocument { + // The endpoint of getting a presigned url for a model file from the file service. + lazy val fileServiceGetModelPresignURLEndpoint: String = + sys.env + .getOrElse( + EnvironmentalVariable.ENV_FILE_SERVICE_GET_MODEL_PRESIGNED_URL_ENDPOINT, + "http://localhost:9092/api/model/presign-download" + ) + .trim +} + +/** + * A read-only document over a single file in a model's LakeFS repository (`model-{mid}`), + * addressed by a `model:///{repositoryName}/{versionHash}/{fileRelativePath}` URI. + */ +private[storage] class ModelFileDocument(uri: URI) + extends LakeFSFileDocument(uri, fileServiceGetModelPresignURLEndpoint) diff --git a/common/workflow-core/src/main/scala/org/apache/texera/amber/core/storage/model/OnDataset.scala b/common/workflow-core/src/main/scala/org/apache/texera/amber/core/storage/model/OnVersionedFileResource.scala similarity index 90% rename from common/workflow-core/src/main/scala/org/apache/texera/amber/core/storage/model/OnDataset.scala rename to common/workflow-core/src/main/scala/org/apache/texera/amber/core/storage/model/OnVersionedFileResource.scala index 6c19ee1002b..cbb76ee4f59 100644 --- a/common/workflow-core/src/main/scala/org/apache/texera/amber/core/storage/model/OnDataset.scala +++ b/common/workflow-core/src/main/scala/org/apache/texera/amber/core/storage/model/OnVersionedFileResource.scala @@ -19,7 +19,10 @@ package org.apache.texera.amber.core.storage.model -trait OnDataset { +/** + * A document backed by a versioned file in a LakeFS repository. + */ +trait OnVersionedFileResource { def getRepositoryName(): String def getVersionHash(): String diff --git a/common/workflow-core/src/test/scala/org/apache/texera/amber/core/storage/DocumentFactorySpec.scala b/common/workflow-core/src/test/scala/org/apache/texera/amber/core/storage/DocumentFactorySpec.scala index d67b90f3637..6e0e032a270 100644 --- a/common/workflow-core/src/test/scala/org/apache/texera/amber/core/storage/DocumentFactorySpec.scala +++ b/common/workflow-core/src/test/scala/org/apache/texera/amber/core/storage/DocumentFactorySpec.scala @@ -19,12 +19,22 @@ package org.apache.texera.amber.core.storage -import org.apache.texera.amber.core.storage.model.VirtualDocument +import org.apache.texera.amber.core.storage.FileResolver.{ + DATASET_FILE_URI_SCHEME, + MODEL_FILE_URI_SCHEME +} +import org.apache.texera.amber.core.storage.model.{ + DatasetFileDocument, + ModelFileDocument, + OnVersionedFileResource, + VirtualDocument +} import org.apache.texera.amber.core.tuple.{Schema, Tuple} import org.scalatest.flatspec.AnyFlatSpec import org.scalatest.matchers.should.Matchers import java.net.URI +import java.nio.file.Paths /** * Unit tests for `DocumentFactory.createOrReuseDocument`, the create-or-reuse @@ -93,4 +103,31 @@ class DocumentFactorySpec extends AnyFlatSpec with Matchers { ) probed shouldBe false } + + // Scheme routing: openReadonlyDocument dispatches a resolved {scheme}:///repo/hash/file URI to + // the matching versioned-file document. Constructing these documents only parses the URI (no DB + // or LakeFS access), so we can assert routing + parsing without any backend. + "openReadonlyDocument" should "route the dataset scheme to a DatasetFileDocument" in { + val doc = DocumentFactory.openReadonlyDocument( + new URI(s"$DATASET_FILE_URI_SCHEME:///dataset-1/abc123/dir/a.csv") + ) + doc shouldBe a[DatasetFileDocument] + } + + it should "route the model scheme to a ModelFileDocument and parse its URI components" in { + val doc = DocumentFactory.openReadonlyDocument( + new URI(s"$MODEL_FILE_URI_SCHEME:///model-1/abc123/weights/model.pt") + ) + doc shouldBe a[ModelFileDocument] + + val resource = doc.asInstanceOf[OnVersionedFileResource] + resource.getRepositoryName() shouldBe "model-1" + resource.getVersionHash() shouldBe "abc123" + resource.getFileRelativePath() shouldBe Paths.get("weights", "model.pt").toString + } + + it should "reject an unsupported scheme" in { + an[UnsupportedOperationException] should be thrownBy + DocumentFactory.openReadonlyDocument(new URI("bogus:///repo/hash/file")) + } } 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 7f4a0478612..a5f441265a8 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 @@ -23,8 +23,20 @@ import org.apache.texera.amber.core.storage.FileResolver import org.apache.commons.vfs2.FileNotFoundException import org.apache.texera.dao.MockTexeraDB import org.apache.texera.dao.jooq.generated.enums.UserRoleEnum -import org.apache.texera.dao.jooq.generated.tables.daos.{DatasetDao, DatasetVersionDao, UserDao} -import org.apache.texera.dao.jooq.generated.tables.pojos.{Dataset, DatasetVersion, User} +import org.apache.texera.dao.jooq.generated.tables.daos.{ + DatasetDao, + DatasetVersionDao, + ModelDao, + ModelVersionDao, + UserDao +} +import org.apache.texera.dao.jooq.generated.tables.pojos.{ + Dataset, + DatasetVersion, + Model, + ModelVersion, + User +} import org.scalatest.flatspec.AnyFlatSpec import org.scalatest.{BeforeAndAfterAll, BeforeAndAfterEach} @@ -77,17 +89,59 @@ class FileResolverSpec datasetVersion } + private val testModel: Model = { + val model = new Model + model.setMid(Integer.valueOf(1)) + model.setName("test_model") + model.setRepositoryName("model-1") + model.setDescription("model for test") + model.setIsPublic(true) + model.setIsDownloadable(true) + model.setOwnerUid(Integer.valueOf(1)) + model + } + + private val testModelVersion1: ModelVersion = { + val modelVersion = new ModelVersion + modelVersion.setMid(Integer.valueOf(1)) + modelVersion.setName("v1") + modelVersion.setMvid(Integer.valueOf(1)) + modelVersion.setCreatorUid(Integer.valueOf(1)) + modelVersion.setVersionHash("a1b2c3d4e5f60718293a4b5c6d7e8f9001122334") + modelVersion + } + + private val testModelVersion2: ModelVersion = { + val modelVersion = new ModelVersion + modelVersion.setMid(Integer.valueOf(1)) + modelVersion.setName("v2") + modelVersion.setMvid(Integer.valueOf(2)) + modelVersion.setCreatorUid(Integer.valueOf(1)) + modelVersion.setVersionHash("998877665544332211ffeeddccbbaa0099887766") + modelVersion + } + private val localCsvFilePath = "common/workflow-core/src/test/resources/country_sales_small.csv" private val datasetACsvFilePath = "/datasets/test_user@test.com/test_dataset/v2/directory/a.csv" private val dataset1TxtFilePath = "/datasets/test_user@test.com/test_dataset/v1/1.txt" + private val modelWeightsFilePath = "/models/test_user@test.com/test_model/v2/weights/model.pt" + + private val modelReadmeFilePath = "/models/test_user@test.com/test_model/v1/README.md" + // Unprefixed form (no resource-type segment); no longer resolvable as a dataset. private val unprefixedDataset1TxtFilePath = "/test_user@test.com/test_dataset/v1/1.txt" - // "models" is not the dataset prefix, so this is not a dataset path. - private val nonDatasetPrefixFilePath = "/models/test_user@test.com/test_dataset/v1/1.txt" + // An unknown resource-type prefix belongs to neither datasets nor models. + private val unknownPrefixFilePath = "/notaprefix/test_user@test.com/test_dataset/v1/1.txt" + + // A model name presented under the datasets prefix must not resolve as a dataset. + private val modelNameUnderDatasetPrefix = "/datasets/test_user@test.com/test_model/v1/README.md" + + // A dataset name presented under the models prefix must not resolve as a model. + private val datasetNameUnderModelPrefix = "/models/test_user@test.com/test_dataset/v1/1.txt" override protected def beforeAll(): Unit = { initializeDBAndReplaceDSLContext() @@ -104,6 +158,15 @@ class FileResolverSpec val datasetVersionDao = new DatasetVersionDao(getDSLContext.configuration()) datasetVersionDao.insert(testDatasetVersion1) datasetVersionDao.insert(testDatasetVersion2) + + // add test model + val modelDao = new ModelDao(getDSLContext.configuration()) + modelDao.insert(testModel) + + // add test model versions + val modelVersionDao = new ModelVersionDao(getDSLContext.configuration()) + modelVersionDao.insert(testModelVersion1) + modelVersionDao.insert(testModelVersion2) } "FileResolver" should "resolve local file correctly" in { @@ -124,6 +187,18 @@ class FileResolverSpec ) } + "FileResolver" should "resolve model file correctly" in { + val modelWeightsUri = FileResolver.resolve(modelWeightsFilePath) + val modelReadmeUri = FileResolver.resolve(modelReadmeFilePath) + + assert( + modelWeightsUri.toString == f"${FileResolver.MODEL_FILE_URI_SCHEME}:///${testModel.getRepositoryName}/${testModelVersion2.getVersionHash}/weights/model.pt" + ) + assert( + modelReadmeUri.toString == f"${FileResolver.MODEL_FILE_URI_SCHEME}:///${testModel.getRepositoryName}/${testModelVersion1.getVersionHash}/README.md" + ) + } + "FileResolver" should "not resolve an unprefixed dataset path (the datasets prefix is required)" in { // Without the datasets prefix the path is not a dataset path; it falls through to the // local-file resolver and fails. @@ -132,10 +207,19 @@ class FileResolverSpec } } - "FileResolver" should "not resolve a path whose prefix is not the datasets prefix" in { - // "models" is not the dataset prefix, so this is not a dataset path. + "FileResolver" should "not resolve a path whose prefix is neither datasets nor models" in { + assertThrows[FileNotFoundException] { + FileResolver.resolve(unknownPrefixFilePath) + } + } + + "FileResolver" should "keep datasets and models isolated by prefix" in { + // a real dataset name under the models prefix is not a model, and vice-versa + assertThrows[FileNotFoundException] { + FileResolver.resolve(datasetNameUnderModelPrefix) + } assertThrows[FileNotFoundException] { - FileResolver.resolve(nonDatasetPrefixFilePath) + FileResolver.resolve(modelNameUnderDatasetPrefix) } } diff --git a/file-service/src/main/scala/org/apache/texera/service/resource/DatasetResource.scala b/file-service/src/main/scala/org/apache/texera/service/resource/DatasetResource.scala index bf26814424b..c27117c17ee 100644 --- a/file-service/src/main/scala/org/apache/texera/service/resource/DatasetResource.scala +++ b/file-service/src/main/scala/org/apache/texera/service/resource/DatasetResource.scala @@ -25,7 +25,7 @@ import jakarta.annotation.security.{PermitAll, RolesAllowed} import jakarta.ws.rs._ import jakarta.ws.rs.core._ import org.apache.texera.common.config.StorageConfig -import org.apache.texera.amber.core.storage.model.OnDataset +import org.apache.texera.amber.core.storage.model.OnVersionedFileResource import org.apache.texera.amber.core.storage.util.LakeFSStorageClient import org.apache.texera.amber.core.storage.{DocumentFactory, FileResolver, ResourceType} import org.apache.texera.auth.SessionUser @@ -1604,7 +1604,8 @@ class DatasetResource extends LazyLogging { // Case 3: Neither repositoryName nor commitHash are provided, resolve normally val response = withTransaction(context) { ctx => val fileUri = FileResolver.resolve(decodedPathStr) - val document = DocumentFactory.openReadonlyDocument(fileUri).asInstanceOf[OnDataset] + val document = + DocumentFactory.openReadonlyDocument(fileUri).asInstanceOf[OnVersionedFileResource] val datasetDao = new DatasetDao(ctx.configuration()) val datasets = datasetDao.fetchByRepositoryName(document.getRepositoryName()).asScala.toList @@ -2246,7 +2247,7 @@ class DatasetResource extends LazyLogging { s"${ResourceType.Datasets}/${owner.getEmail}/${dataset.getName}/$normalized" ) ) - .asInstanceOf[OnDataset] + .asInstanceOf[OnVersionedFileResource] val fileSize = withLakeFSErrorHandling(s"reading the size of cover image '$normalized'") { LakeFSStorageClient.getFileSize( @@ -2303,7 +2304,7 @@ class DatasetResource extends LazyLogging { val document = DocumentFactory .openReadonlyDocument(FileResolver.resolve(fullPath)) - .asInstanceOf[OnDataset] + .asInstanceOf[OnVersionedFileResource] val presignedUrl = withLakeFSErrorHandling( s"generating a presigned URL for cover image '$coverImage'" @@ -2353,7 +2354,7 @@ class DatasetResource extends LazyLogging { val document = DocumentFactory .openReadonlyDocument(FileResolver.resolve(fullPath)) - .asInstanceOf[OnDataset] + .asInstanceOf[OnVersionedFileResource] val presignedUrl = withLakeFSErrorHandling( s"generating a presigned URL for cover image '$coverImage'" diff --git a/sql/texera_ddl.sql b/sql/texera_ddl.sql index 0dc10e90f9c..8a3c333d2ca 100644 --- a/sql/texera_ddl.sql +++ b/sql/texera_ddl.sql @@ -61,12 +61,9 @@ DROP TABLE IF EXISTS workflow_of_project CASCADE; DROP TABLE IF EXISTS workflow_executions CASCADE; DROP TABLE IF EXISTS dataset_upload_session CASCADE; DROP TABLE IF EXISTS dataset_upload_session_part CASCADE; -DROP TABLE IF EXISTS model_upload_session CASCADE; -DROP TABLE IF EXISTS model_upload_session_part CASCADE; DROP TABLE IF EXISTS dataset CASCADE; DROP TABLE IF EXISTS dataset_user_access CASCADE; DROP TABLE IF EXISTS dataset_version CASCADE; -DROP TABLE IF EXISTS model_user_access CASCADE; DROP TABLE IF EXISTS model_version CASCADE; DROP TABLE IF EXISTS model CASCADE; DROP TABLE IF EXISTS public_project CASCADE; @@ -77,8 +74,6 @@ DROP TABLE IF EXISTS workflow_view_count CASCADE; DROP TABLE IF EXISTS user_action CASCADE; DROP TABLE IF EXISTS dataset_user_likes CASCADE; DROP TABLE IF EXISTS dataset_view_count CASCADE; -DROP TABLE IF EXISTS model_user_likes CASCADE; -DROP TABLE IF EXISTS model_view_count CASCADE; DROP TABLE IF EXISTS site_settings CASCADE; DROP TABLE IF EXISTS computing_unit_user_access CASCADE; DROP TABLE IF EXISTS notebook CASCADE; @@ -383,17 +378,6 @@ CREATE TABLE IF NOT EXISTS model UNIQUE (owner_uid, name) ); --- model_user_access -CREATE TABLE IF NOT EXISTS model_user_access -( - mid INT NOT NULL, - uid INT NOT NULL, - privilege privilege_enum NOT NULL DEFAULT 'NONE', - PRIMARY KEY (mid, uid), - FOREIGN KEY (mid) REFERENCES model(mid) ON DELETE CASCADE, - FOREIGN KEY (uid) REFERENCES "user"(uid) ON DELETE CASCADE - ); - -- model_version CREATE TABLE IF NOT EXISTS model_version ( @@ -406,70 +390,6 @@ CREATE TABLE IF NOT EXISTS model_version FOREIGN KEY (mid) REFERENCES model(mid) ON DELETE CASCADE ); -CREATE TABLE IF NOT EXISTS model_upload_session -( - mid INT NOT NULL, - uid INT NOT NULL, - file_path TEXT NOT NULL, - upload_id VARCHAR(256) NOT NULL UNIQUE, - physical_address TEXT, - num_parts_requested INT NOT NULL, - file_size_bytes BIGINT NOT NULL, - part_size_bytes BIGINT NOT NULL, - created_at TIMESTAMPTZ NOT NULL DEFAULT now(), - - PRIMARY KEY (uid, mid, file_path), - - FOREIGN KEY (mid) REFERENCES model(mid) ON DELETE CASCADE, - FOREIGN KEY (uid) REFERENCES "user"(uid) ON DELETE CASCADE, - - CONSTRAINT chk_model_upload_session_num_parts_requested_positive - CHECK (num_parts_requested >= 1), - - CONSTRAINT chk_model_upload_session_file_size_bytes_positive - CHECK (file_size_bytes > 0), - - CONSTRAINT chk_model_upload_session_part_size_bytes_positive - CHECK (part_size_bytes > 0), - - CONSTRAINT chk_model_upload_session_part_size_bytes_s3_upper_bound - CHECK (part_size_bytes <= 5368709120) -); - -CREATE TABLE IF NOT EXISTS model_upload_session_part -( - upload_id VARCHAR(256) NOT NULL, - part_number INT NOT NULL, - etag TEXT NOT NULL DEFAULT '', - - PRIMARY KEY (upload_id, part_number), - - CONSTRAINT chk_model_part_number_positive CHECK (part_number > 0), - - FOREIGN KEY (upload_id) - REFERENCES model_upload_session(upload_id) - ON DELETE CASCADE -); - --- model_user_likes table -CREATE TABLE IF NOT EXISTS model_user_likes -( - uid INTEGER NOT NULL, - mid INTEGER NOT NULL, - PRIMARY KEY (uid, mid), - FOREIGN KEY (uid) REFERENCES "user"(uid) ON DELETE CASCADE, - FOREIGN KEY (mid) REFERENCES model(mid) ON DELETE CASCADE - ); - --- model_view_count table -CREATE TABLE IF NOT EXISTS model_view_count -( - mid INTEGER NOT NULL, - view_count INTEGER NOT NULL DEFAULT 0, - PRIMARY KEY (mid), - FOREIGN KEY (mid) REFERENCES model(mid) ON DELETE CASCADE - ); - -- operator_executions (modified to match MySQL: no separate primary key; added console_messages_uri) CREATE TABLE IF NOT EXISTS operator_executions ( diff --git a/sql/updates/30.sql b/sql/updates/30.sql index 6dae197f400..5492387a347 100644 --- a/sql/updates/30.sql +++ b/sql/updates/30.sql @@ -23,9 +23,8 @@ SET search_path TO texera_db; BEGIN; --- Introduce ML models as a first-class resource, These --- tables mirror the dataset* tables (own primary key `mid`, own LakeFS repo --- namespace) and add model-specific attributes (framework, format). +-- Introduce ML models as a first-class resource (own primary key `mid`, own LakeFS repo namespace) +-- and add model-specific attributes (framework, format). CREATE TABLE IF NOT EXISTS model ( @@ -44,16 +43,6 @@ CREATE TABLE IF NOT EXISTS model UNIQUE (owner_uid, name) ); -CREATE TABLE IF NOT EXISTS model_user_access -( - mid INT NOT NULL, - uid INT NOT NULL, - privilege privilege_enum NOT NULL DEFAULT 'NONE', - PRIMARY KEY (mid, uid), - FOREIGN KEY (mid) REFERENCES model(mid) ON DELETE CASCADE, - FOREIGN KEY (uid) REFERENCES "user"(uid) ON DELETE CASCADE -); - CREATE TABLE IF NOT EXISTS model_version ( mvid SERIAL PRIMARY KEY, @@ -65,66 +54,4 @@ CREATE TABLE IF NOT EXISTS model_version FOREIGN KEY (mid) REFERENCES model(mid) ON DELETE CASCADE ); -CREATE TABLE IF NOT EXISTS model_upload_session -( - mid INT NOT NULL, - uid INT NOT NULL, - file_path TEXT NOT NULL, - upload_id VARCHAR(256) NOT NULL UNIQUE, - physical_address TEXT, - num_parts_requested INT NOT NULL, - file_size_bytes BIGINT NOT NULL, - part_size_bytes BIGINT NOT NULL, - created_at TIMESTAMPTZ NOT NULL DEFAULT now(), - - PRIMARY KEY (uid, mid, file_path), - - FOREIGN KEY (mid) REFERENCES model(mid) ON DELETE CASCADE, - FOREIGN KEY (uid) REFERENCES "user"(uid) ON DELETE CASCADE, - - CONSTRAINT chk_model_upload_session_num_parts_requested_positive - CHECK (num_parts_requested >= 1), - - CONSTRAINT chk_model_upload_session_file_size_bytes_positive - CHECK (file_size_bytes > 0), - - CONSTRAINT chk_model_upload_session_part_size_bytes_positive - CHECK (part_size_bytes > 0), - - CONSTRAINT chk_model_upload_session_part_size_bytes_s3_upper_bound - CHECK (part_size_bytes <= 5368709120) -); - -CREATE TABLE IF NOT EXISTS model_upload_session_part -( - upload_id VARCHAR(256) NOT NULL, - part_number INT NOT NULL, - etag TEXT NOT NULL DEFAULT '', - - PRIMARY KEY (upload_id, part_number), - - CONSTRAINT chk_model_part_number_positive CHECK (part_number > 0), - - FOREIGN KEY (upload_id) - REFERENCES model_upload_session(upload_id) - ON DELETE CASCADE -); - -CREATE TABLE IF NOT EXISTS model_user_likes -( - uid INTEGER NOT NULL, - mid INTEGER NOT NULL, - PRIMARY KEY (uid, mid), - FOREIGN KEY (uid) REFERENCES "user"(uid) ON DELETE CASCADE, - FOREIGN KEY (mid) REFERENCES model(mid) ON DELETE CASCADE -); - -CREATE TABLE IF NOT EXISTS model_view_count -( - mid INTEGER NOT NULL, - view_count INTEGER NOT NULL DEFAULT 0, - PRIMARY KEY (mid), - FOREIGN KEY (mid) REFERENCES model(mid) ON DELETE CASCADE -); - COMMIT; From aebfdb98f20d390cd26f97fb9950173ef21c6172 Mon Sep 17 00:00:00 2001 From: Tanishq Gandhi Date: Fri, 24 Jul 2026 12:39:41 -0700 Subject: [PATCH 14/16] feat(storage): add model file storage and path resolution --- .../texera/amber/core/storage/FileResolver.scala | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) 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 8db38a047aa..9265ec9f723 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 @@ -195,6 +195,11 @@ object FileResolver { .and(DATASET.NAME.eq(datasetName)) .fetchOneInto(classOf[Dataset]) + // fail early if the dataset does not exist (before dereferencing it below) + if (dataset == null) { + throw new FileNotFoundException(s"Dataset file $fileName not found.") + } + // fetch the dataset version from DB val datasetVersion = ctx .selectFrom(DATASET_VERSION) @@ -202,7 +207,7 @@ object FileResolver { .and(DATASET_VERSION.NAME.eq(versionName)) .fetchOneInto(classOf[DatasetVersion]) - if (dataset == null || datasetVersion == null) { + if (datasetVersion == null) { throw new FileNotFoundException(s"Dataset file $fileName not found.") } (dataset.getRepositoryName, datasetVersion.getVersionHash) @@ -243,6 +248,11 @@ object FileResolver { .and(MODEL.NAME.eq(modelName)) .fetchOneInto(classOf[Model]) + // fail early if the model does not exist (before dereferencing it below) + if (model == null) { + throw new FileNotFoundException(s"Model file $fileName not found.") + } + // fetch the model version from DB val modelVersion = ctx .selectFrom(MODEL_VERSION) @@ -250,7 +260,7 @@ object FileResolver { .and(MODEL_VERSION.NAME.eq(versionName)) .fetchOneInto(classOf[ModelVersion]) - if (model == null || modelVersion == null) { + if (modelVersion == null) { throw new FileNotFoundException(s"Model file $fileName not found.") } (model.getRepositoryName, modelVersion.getVersionHash) From e3347c182935913bf0de658e64748f1073ab4eef Mon Sep 17 00:00:00 2001 From: Tanishq Gandhi Date: Fri, 24 Jul 2026 14:16:50 -0700 Subject: [PATCH 15/16] feat(file-service): add model management API (metadata + access control) --- .../apache/texera/service/FileService.scala | 6 +- .../resource/ModelAccessResource.scala | 226 ++++++++ .../service/resource/ModelResource.scala | 504 +++++++++++++++++ .../resource/ModelAccessResourceSpec.scala | 432 +++++++++++++++ .../service/resource/ModelResourceSpec.scala | 516 ++++++++++++++++++ sql/changelog.xml | 4 + sql/texera_ddl.sql | 12 + sql/updates/31.sql | 39 ++ 8 files changed, 1738 insertions(+), 1 deletion(-) create mode 100644 file-service/src/main/scala/org/apache/texera/service/resource/ModelAccessResource.scala create mode 100644 file-service/src/main/scala/org/apache/texera/service/resource/ModelResource.scala create mode 100644 file-service/src/test/scala/org/apache/texera/service/resource/ModelAccessResourceSpec.scala create mode 100644 file-service/src/test/scala/org/apache/texera/service/resource/ModelResourceSpec.scala create mode 100644 sql/updates/31.sql diff --git a/file-service/src/main/scala/org/apache/texera/service/FileService.scala b/file-service/src/main/scala/org/apache/texera/service/FileService.scala index d8e3ff122d5..4546a9c0ec4 100644 --- a/file-service/src/main/scala/org/apache/texera/service/FileService.scala +++ b/file-service/src/main/scala/org/apache/texera/service/FileService.scala @@ -34,7 +34,9 @@ import org.apache.texera.service.`type`.serde.DatasetFileNodeSerializer import org.apache.texera.service.resource.{ DatasetAccessResource, DatasetResource, - HealthCheckResource + HealthCheckResource, + ModelAccessResource, + ModelResource } import org.apache.texera.service.util.S3StorageClient import org.apache.texera.service.util.LargeBinaryManager @@ -89,6 +91,8 @@ class FileService extends Application[FileServiceConfiguration] with LazyLogging environment.jersey.register(classOf[DatasetResource]) environment.jersey.register(classOf[DatasetAccessResource]) + environment.jersey.register(classOf[ModelResource]) + environment.jersey.register(classOf[ModelAccessResource]) RoleAnnotationEnforcer.enforce(environment.jersey.getResourceConfig, "FileService") diff --git a/file-service/src/main/scala/org/apache/texera/service/resource/ModelAccessResource.scala b/file-service/src/main/scala/org/apache/texera/service/resource/ModelAccessResource.scala new file mode 100644 index 00000000000..49effe90f08 --- /dev/null +++ b/file-service/src/main/scala/org/apache/texera/service/resource/ModelAccessResource.scala @@ -0,0 +1,226 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.texera.service.resource + +import io.dropwizard.auth.Auth +import jakarta.annotation.security.RolesAllowed +import jakarta.ws.rs.core.{MediaType, Response} +import jakarta.ws.rs._ +import org.apache.texera.auth.SessionUser +import org.apache.texera.dao.SqlServer +import org.apache.texera.dao.SqlServer.withTransaction +import org.apache.texera.dao.jooq.generated.Tables.USER +import org.apache.texera.dao.jooq.generated.enums.PrivilegeEnum +import org.apache.texera.dao.jooq.generated.tables.ModelUserAccess.MODEL_USER_ACCESS +import org.apache.texera.dao.jooq.generated.tables.daos.{ModelDao, ModelUserAccessDao, UserDao} +import org.apache.texera.dao.jooq.generated.tables.pojos.{ModelUserAccess, User} +import org.apache.texera.service.resource.ModelAccessResource.{ + AccessEntry, + context, + getOwner, + userHasWriteAccess +} +import org.jooq.{DSLContext, EnumType} + +object ModelAccessResource { + private def context: DSLContext = + SqlServer + .getInstance() + .createDSLContext() + + def isModelPublic(ctx: DSLContext, mid: Integer): Boolean = { + val modelDao = new ModelDao(ctx.configuration()) + Option(modelDao.fetchOneByMid(mid)) + .flatMap(model => Option(model.getIsPublic)) + .contains(true) + } + + def userHasReadAccess(ctx: DSLContext, mid: Integer, uid: Integer): Boolean = { + isModelPublic(ctx, mid) || + userHasWriteAccess(ctx, mid, uid) || + getModelUserAccessPrivilege(ctx, mid, uid) == PrivilegeEnum.READ + } + + def userOwnModel(ctx: DSLContext, mid: Integer, uid: Integer): Boolean = { + val modelDao = new ModelDao(ctx.configuration()) + + Option(modelDao.fetchOneByMid(mid)) + .exists(_.getOwnerUid == uid) + } + + def userHasWriteAccess(ctx: DSLContext, mid: Integer, uid: Integer): Boolean = { + userOwnModel(ctx, mid, uid) || + getModelUserAccessPrivilege(ctx, mid, uid) == PrivilegeEnum.WRITE + } + + def getModelUserAccessPrivilege( + ctx: DSLContext, + mid: Integer, + uid: Integer + ): PrivilegeEnum = { + Option( + ctx + .select(MODEL_USER_ACCESS.PRIVILEGE) + .from(MODEL_USER_ACCESS) + .where( + MODEL_USER_ACCESS.MID + .eq(mid) + .and(MODEL_USER_ACCESS.UID.eq(uid)) + ) + .fetchOneInto(classOf[PrivilegeEnum]) + ).getOrElse(PrivilegeEnum.NONE) + } + + def getOwner(ctx: DSLContext, mid: Integer): User = { + val modelDao = new ModelDao(ctx.configuration()) + val userDao = new UserDao(ctx.configuration()) + + Option(modelDao.fetchOneByMid(mid)) + .flatMap(model => Option(model.getOwnerUid)) + .map(ownerUid => userDao.fetchOneByUid(ownerUid)) + .orNull + } + + case class AccessEntry(email: String, name: String, privilege: EnumType) {} + +} + +@Produces(Array(MediaType.APPLICATION_JSON)) +@RolesAllowed(Array("REGULAR", "ADMIN")) +@Path("/access/model") +class ModelAccessResource { + + /** + * This method returns the owner of a model + * + * @param mid , model id + * @return ownerEmail, the owner's email + */ + @GET + @Path("/owner/{mid}") + def getOwnerEmailOfModel(@PathParam("mid") mid: Integer): String = { + var email = "" + withTransaction(context) { ctx => + val owner = getOwner(ctx, mid) + if (owner != null) { + email = owner.getEmail + } + } + email + } + + /** + * Returns information about all current shared access of the given model + * + * @param mid model id + * @return a List of email/name/permission + */ + @GET + @Path("/list/{mid}") + def getAccessList( + @PathParam("mid") mid: Integer + ): java.util.List[AccessEntry] = { + withTransaction(context) { ctx => + val modelDao = new ModelDao(ctx.configuration()) + ctx + .select( + USER.EMAIL, + USER.NAME, + MODEL_USER_ACCESS.PRIVILEGE + ) + .from(MODEL_USER_ACCESS) + .join(USER) + .on(USER.UID.eq(MODEL_USER_ACCESS.UID)) + .where( + MODEL_USER_ACCESS.MID + .eq(mid) + .and(MODEL_USER_ACCESS.UID.notEqual(modelDao.fetchOneByMid(mid).getOwnerUid)) + ) + .fetchInto(classOf[AccessEntry]) + } + } + + /** + * This method shares a model to a user with a specific access type + * + * @param mid the given model + * @param email the email which the access is given to + * @param privilege the type of Access given to the target user + * @return rejection if user not permitted to share the model or Success Message + */ + @PUT + @Path("/grant/{mid}/{email}/{privilege}") + def grantAccess( + @PathParam("mid") mid: Integer, + @PathParam("email") email: String, + @PathParam("privilege") privilege: String, + @Auth user: SessionUser + ): Response = { + withTransaction(context) { ctx => + if (!userHasWriteAccess(ctx, mid, user.getUid)) { + throw new ForbiddenException(s"You do not have permission to modify model $mid") + } + val modelUserAccessDao = new ModelUserAccessDao(ctx.configuration()) + val userDao = new UserDao(ctx.configuration()) + modelUserAccessDao.merge( + new ModelUserAccess( + mid, + userDao.fetchOneByEmail(email).getUid, + PrivilegeEnum.valueOf(privilege) + ) + ) + Response.ok().build() + } + } + + /** + * This method revoke the user's access of the given model + * + * @param mid the given model + * @param email the email of the use whose access is about to be removed + * @return message indicating a success message + */ + @DELETE + @Path("/revoke/{mid}/{email}") + def revokeAccess( + @PathParam("mid") mid: Integer, + @PathParam("email") email: String, + @Auth user: SessionUser + ): Response = { + withTransaction(context) { ctx => + if (!userHasWriteAccess(ctx, mid, user.getUid)) { + throw new ForbiddenException(s"You do not have permission to modify model $mid") + } + + val userDao = new UserDao(ctx.configuration()) + + ctx + .delete(MODEL_USER_ACCESS) + .where( + MODEL_USER_ACCESS.UID + .eq(userDao.fetchOneByEmail(email).getUid) + .and(MODEL_USER_ACCESS.MID.eq(mid)) + ) + .execute() + + Response.ok().build() + } + } +} diff --git a/file-service/src/main/scala/org/apache/texera/service/resource/ModelResource.scala b/file-service/src/main/scala/org/apache/texera/service/resource/ModelResource.scala new file mode 100644 index 00000000000..c6b4095910e --- /dev/null +++ b/file-service/src/main/scala/org/apache/texera/service/resource/ModelResource.scala @@ -0,0 +1,504 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.texera.service.resource + +import com.typesafe.scalalogging.LazyLogging +import io.dropwizard.auth.Auth +import jakarta.annotation.security.{PermitAll, RolesAllowed} +import jakarta.ws.rs._ +import jakarta.ws.rs.core._ +import org.apache.texera.amber.core.storage.util.LakeFSStorageClient +import org.apache.texera.auth.SessionUser +import org.apache.texera.common.config.StorageConfig +import org.apache.texera.dao.SqlServer +import org.apache.texera.dao.SqlServer.withTransaction +import org.apache.texera.dao.jooq.generated.enums.PrivilegeEnum +import org.apache.texera.dao.jooq.generated.tables.Model.MODEL +import org.apache.texera.dao.jooq.generated.tables.ModelUserAccess.MODEL_USER_ACCESS +import org.apache.texera.dao.jooq.generated.tables.User.USER +import org.apache.texera.dao.jooq.generated.tables.daos.{ModelDao, ModelUserAccessDao} +import org.apache.texera.dao.jooq.generated.tables.pojos.{Model, ModelUserAccess} +import org.apache.texera.service.resource.ModelAccessResource._ +import org.apache.texera.service.resource.ModelResource.{context, _} +import org.apache.texera.service.util.S3StorageClient +import org.apache.texera.service.util.LakeFSExceptionHandler.withLakeFSErrorHandling +import org.jooq.exception.DataAccessException +import org.jooq.{DSLContext, EnumType} + +import scala.collection.mutable.ListBuffer +import scala.jdk.CollectionConverters._ + +object ModelResource { + + // MVP supports a single framework; stored on the model so later frameworks can be added. + private val DEFAULT_FRAMEWORK = "pytorch" + + private def context = + SqlServer + .getInstance() + .createDSLContext() + + /** + * Helper function to get the model from DB using mid + */ + private def getModelByID(ctx: DSLContext, mid: Integer): Model = { + val modelDao = new ModelDao(ctx.configuration()) + val model = modelDao.fetchOneByMid(mid) + if (model == null) { + throw new NotFoundException(f"Model $mid not found") + } + model + } + + case class DashboardModel( + model: Model, + ownerEmail: String, + accessPrivilege: EnumType, + isOwner: Boolean, + size: Long + ) + + case class CreateModelRequest( + modelName: String, + modelDescription: String, + isModelPublic: Boolean, + isModelDownloadable: Boolean, + framework: String, + format: String + ) + + case class ModelDescriptionModification(mid: Integer, description: String) + + case class ModelNameModification(mid: Integer, name: String) +} + +@Produces(Array(MediaType.APPLICATION_JSON)) +@Path("/model") +class ModelResource extends LazyLogging { + private val ERR_USER_HAS_NO_ACCESS_TO_MODEL_MESSAGE = "User has no access to this model" + + private val MODEL_NAME_MAX_LENGTH = 128 + private val MODEL_NAME_PATTERN = "^[A-Za-z0-9_-]+$".r + + /** + * Validates the model name. + * + * Rules: + * - Must be 1 to 128 characters long. + * - Only letters, numbers, underscores, and hyphens are allowed. + * + * @param name The model name to validate. + * @throws jakarta.ws.rs.BadRequestException if the name is invalid. + */ + private def validateModelName(name: String): Unit = { + if (name == null || !MODEL_NAME_PATTERN.matches(name)) { + throw new BadRequestException( + "Invalid model name: only letters, numbers, underscores, and hyphens are allowed." + ) + } + if (name.length > MODEL_NAME_MAX_LENGTH) { + throw new BadRequestException( + s"Invalid model name: name must be at most $MODEL_NAME_MAX_LENGTH characters long." + ) + } + } + + /** + * Runs a model write and translates a (owner_uid, name) unique-constraint + * violation into the same BadRequestException the pre-checks throw, so + * requests losing a concurrent race get a 400 instead of a 500. + */ + private[resource] def failOnDuplicateModelName[T](op: => T): T = { + try op + catch { + case e: DataAccessException => + if (e.sqlState() == "23505") { + throw new BadRequestException("Model with the same name already exists") + } + throw e + } + } + + /** + * Helper function to get the model from DB with additional information including + * user access privilege and owner email + */ + private def getDashboardModel( + ctx: DSLContext, + mid: Integer, + requesterUid: Option[Integer] + ): DashboardModel = { + val targetModel = getModelByID(ctx, mid) + + if (requesterUid.isEmpty && !targetModel.getIsPublic) { + throw new ForbiddenException(ERR_USER_HAS_NO_ACCESS_TO_MODEL_MESSAGE) + } else if (requesterUid.exists(uid => !userHasReadAccess(ctx, mid, uid))) { + throw new ForbiddenException(ERR_USER_HAS_NO_ACCESS_TO_MODEL_MESSAGE) + } + + val userAccessPrivilege = requesterUid + .map(uid => getModelUserAccessPrivilege(ctx, mid, uid)) + .getOrElse(PrivilegeEnum.READ) + + val isOwner = requesterUid.contains(targetModel.getOwnerUid) + + DashboardModel( + targetModel, + getOwner(ctx, mid).getEmail, + userAccessPrivilege, + isOwner, + withLakeFSErrorHandling(s"retrieving the size of model '${targetModel.getName}'") { + LakeFSStorageClient.retrieveRepositorySize(targetModel.getRepositoryName) + } + ) + } + + @POST + @RolesAllowed(Array("REGULAR", "ADMIN")) + @Path("/create") + @Consumes(Array(MediaType.APPLICATION_JSON)) + def createModel( + request: CreateModelRequest, + @Auth user: SessionUser + ): DashboardModel = { + + withTransaction(context) { ctx => + val uid = user.getUid + val modelUserAccessDao: ModelUserAccessDao = new ModelUserAccessDao(ctx.configuration()) + + val modelName = request.modelName + val modelDescription = request.modelDescription + val isModelPublic = request.isModelPublic + val isModelDownloadable = request.isModelDownloadable + + validateModelName(modelName) + + // Check if a model with the same name already exists + val duplicateExists = ctx.fetchExists( + ctx + .selectFrom(MODEL) + .where(MODEL.OWNER_UID.eq(uid)) + .and(MODEL.NAME.eq(modelName)) + ) + if (duplicateExists) { + throw new BadRequestException("Model with the same name already exists") + } + + // insert the model into the database + val model = new Model() + model.setName(modelName) + model.setDescription(modelDescription) + model.setIsPublic(isModelPublic) + model.setIsDownloadable(isModelDownloadable) + model.setOwnerUid(uid) + model.setFramework(Option(request.framework).filter(_.nonEmpty).getOrElse(DEFAULT_FRAMEWORK)) + model.setFormat(request.format) + + // insert record and get created model with mid + val createdModel = failOnDuplicateModelName { + ctx + .insertInto(MODEL) + .set(ctx.newRecord(MODEL, model)) + .returning() + .fetchOne() + } + + // Initialize the repository in LakeFS + val repositoryName = s"model-${createdModel.getMid}" + try { + withLakeFSErrorHandling(s"creating the repository of model '${model.getName}'") { + LakeFSStorageClient.initRepo(repositoryName) + } + } catch { + case e: Exception => + // roll back the model record so a failed LakeFS init leaves no orphan row + ctx + .deleteFrom(MODEL) + .where(MODEL.MID.eq(createdModel.getMid)) + .execute() + e match { + case web: WebApplicationException => throw web + case other => + throw new WebApplicationException( + s"Failed to create the model: ${other.getMessage}" + ) + } + } + + // update repository name of the created model + createdModel.setRepositoryName(repositoryName) + createdModel.update() + + // Insert the requester as the WRITE access user for this model + val modelUserAccess = new ModelUserAccess() + modelUserAccess.setMid(createdModel.getMid) + modelUserAccess.setUid(uid) + modelUserAccess.setPrivilege(PrivilegeEnum.WRITE) + modelUserAccessDao.insert(modelUserAccess) + + DashboardModel( + createdModel.into(classOf[Model]), + user.getEmail, + PrivilegeEnum.WRITE, + isOwner = true, + 0 + ) + } + } + + @DELETE + @RolesAllowed(Array("REGULAR", "ADMIN")) + @Path("/{mid}") + def deleteModel(@PathParam("mid") mid: Integer, @Auth user: SessionUser): Response = { + val uid = user.getUid + withTransaction(context) { ctx => + val modelDao = new ModelDao(ctx.configuration()) + val model = getModelByID(ctx, mid) + if (!userOwnModel(ctx, model.getMid, uid)) { + // throw the exception that user has no access to certain model + throw new ForbiddenException(ERR_USER_HAS_NO_ACCESS_TO_MODEL_MESSAGE) + } + withLakeFSErrorHandling(s"deleting the repository of model '${model.getName}'") { + LakeFSStorageClient.deleteRepo(model.getRepositoryName) + } + // delete the directory on S3 + if ( + S3StorageClient.directoryExists(StorageConfig.lakefsBucketName, model.getRepositoryName) + ) { + S3StorageClient.deleteDirectory(StorageConfig.lakefsBucketName, model.getRepositoryName) + } + + // delete the model from the DB + modelDao.deleteById(model.getMid) + + Response.ok().build() + } + } + + @POST + @Consumes(Array(MediaType.APPLICATION_JSON)) + @Produces(Array(MediaType.APPLICATION_JSON)) + @RolesAllowed(Array("REGULAR", "ADMIN")) + @Path("/update/description") + def updateModelDescription( + modificator: ModelDescriptionModification, + @Auth sessionUser: SessionUser + ): Response = { + withTransaction(context) { ctx => + val uid = sessionUser.getUid + val modelDao = new ModelDao(ctx.configuration()) + val model = getModelByID(ctx, modificator.mid) + if (!userHasWriteAccess(ctx, modificator.mid, uid)) { + throw new ForbiddenException(ERR_USER_HAS_NO_ACCESS_TO_MODEL_MESSAGE) + } + + model.setDescription(modificator.description) + modelDao.update(model) + Response.ok().build() + } + } + + @POST + @Consumes(Array(MediaType.APPLICATION_JSON)) + @Produces(Array(MediaType.APPLICATION_JSON)) + @RolesAllowed(Array("REGULAR", "ADMIN")) + @Path("/update/name") + def updateModelName( + modificator: ModelNameModification, + @Auth sessionUser: SessionUser + ): Response = { + withTransaction(context) { ctx => + val uid = sessionUser.getUid + val modelDao = new ModelDao(ctx.configuration()) + val model = getModelByID(ctx, modificator.mid) + if (!userHasWriteAccess(ctx, modificator.mid, uid)) { + throw new ForbiddenException(ERR_USER_HAS_NO_ACCESS_TO_MODEL_MESSAGE) + } + + validateModelName(modificator.name) + + // Check if the owner already has another model with the same name + val duplicateExists = ctx.fetchExists( + ctx + .selectFrom(MODEL) + .where(MODEL.OWNER_UID.eq(model.getOwnerUid)) + .and(MODEL.NAME.eq(modificator.name)) + .and(MODEL.MID.notEqual(model.getMid)) + ) + if (duplicateExists) { + throw new BadRequestException("Model with the same name already exists") + } + + model.setName(modificator.name) + failOnDuplicateModelName { + modelDao.update(model) + } + Response.ok().build() + } + } + + @POST + @RolesAllowed(Array("REGULAR", "ADMIN")) + @Path("/{mid}/update/publicity") + def toggleModelPublicity( + @PathParam("mid") mid: Integer, + @Auth sessionUser: SessionUser + ): Response = { + withTransaction(context) { ctx => + val modelDao = new ModelDao(ctx.configuration()) + val uid = sessionUser.getUid + + if (!userHasWriteAccess(ctx, mid, uid)) { + throw new ForbiddenException(ERR_USER_HAS_NO_ACCESS_TO_MODEL_MESSAGE) + } + + val existedModel = getModelByID(ctx, mid) + val newPublicStatus = !existedModel.getIsPublic + existedModel.setIsPublic(newPublicStatus) + + modelDao.update(existedModel) + Response.ok().build() + } + } + + @POST + @RolesAllowed(Array("REGULAR", "ADMIN")) + @Path("/{mid}/update/downloadable") + def toggleModelDownloadable( + @PathParam("mid") mid: Integer, + @Auth sessionUser: SessionUser + ): Response = { + withTransaction(context) { ctx => + val modelDao = new ModelDao(ctx.configuration()) + val uid = sessionUser.getUid + + if (!userOwnModel(ctx, mid, uid)) { + throw new ForbiddenException("Only model owners can modify download permissions") + } + + val existedModel = getModelByID(ctx, mid) + val newDownloadableStatus = !existedModel.getIsDownloadable + + existedModel.setIsDownloadable(newDownloadableStatus) + + modelDao.update(existedModel) + Response.ok().build() + } + } + + @GET + @RolesAllowed(Array("REGULAR", "ADMIN")) + @Path("/list") + def listModels( + @Auth user: SessionUser + ): List[DashboardModel] = { + val uid = user.getUid + withTransaction(context)(ctx => { + var accessibleModels: ListBuffer[DashboardModel] = ListBuffer() + // first fetch all models user have explicit access to + accessibleModels = ListBuffer.from( + ctx + .select() + .from( + MODEL + .leftJoin(MODEL_USER_ACCESS) + .on(MODEL_USER_ACCESS.MID.eq(MODEL.MID)) + .leftJoin(USER) + .on(USER.UID.eq(MODEL.OWNER_UID)) + ) + .where(MODEL_USER_ACCESS.UID.eq(uid)) + .fetch() + .map(record => { + val model = record.into(MODEL).into(classOf[Model]) + val modelAccess = record.into(MODEL_USER_ACCESS).into(classOf[ModelUserAccess]) + val ownerEmail = record.into(USER).getEmail + DashboardModel( + isOwner = model.getOwnerUid == uid, + model = model, + accessPrivilege = modelAccess.getPrivilege, + ownerEmail = ownerEmail, + size = 0 + ) + }) + .asScala + ) + + // then we fetch the public models and merge it as a part of the result if not exist + val publicModels = ctx + .select() + .from( + MODEL + .leftJoin(USER) + .on(USER.UID.eq(MODEL.OWNER_UID)) + ) + .where(MODEL.IS_PUBLIC.eq(true)) + .fetch() + .asScala + .flatMap { record => + val model = record.into(MODEL).into(classOf[Model]) + val ownerEmail = record.into(USER).getEmail + try { + Some( + DashboardModel( + isOwner = false, + model = model, + accessPrivilege = PrivilegeEnum.READ, + ownerEmail = ownerEmail, + size = LakeFSStorageClient.retrieveRepositorySize(model.getRepositoryName) + ) + ) + } catch { + case e: io.lakefs.clients.sdk.ApiException => + logger.error( + s"LakeFS ApiException for model repository '${model.getRepositoryName}': ${e.getMessage}", + e + ) + None + } + } + publicModels.foreach { publicModel => + if (!accessibleModels.exists(_.model.getMid == publicModel.model.getMid)) { + accessibleModels = accessibleModels :+ publicModel + } + } + accessibleModels.toList + }) + } + + @GET + @RolesAllowed(Array("REGULAR", "ADMIN")) + @Path("/{mid}") + def getModel( + @PathParam("mid") mid: Integer, + @Auth user: SessionUser + ): DashboardModel = { + val uid = user.getUid + withTransaction(context)(ctx => getDashboardModel(ctx, mid, Some(uid))) + } + + @GET + @PermitAll + @Path("/public/{mid}") + def getPublicModel( + @PathParam("mid") mid: Integer + ): DashboardModel = { + withTransaction(context)(ctx => getDashboardModel(ctx, mid, None)) + } +} diff --git a/file-service/src/test/scala/org/apache/texera/service/resource/ModelAccessResourceSpec.scala b/file-service/src/test/scala/org/apache/texera/service/resource/ModelAccessResourceSpec.scala new file mode 100644 index 00000000000..5a50399a176 --- /dev/null +++ b/file-service/src/test/scala/org/apache/texera/service/resource/ModelAccessResourceSpec.scala @@ -0,0 +1,432 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.texera.service.resource + +import jakarta.ws.rs.ForbiddenException +import org.apache.texera.auth.SessionUser +import org.apache.texera.dao.MockTexeraDB +import org.apache.texera.dao.jooq.generated.enums.{PrivilegeEnum, UserRoleEnum} +import org.apache.texera.dao.jooq.generated.tables.ModelUserAccess.MODEL_USER_ACCESS +import org.apache.texera.dao.jooq.generated.tables.daos.{ModelDao, ModelUserAccessDao, UserDao} +import org.apache.texera.dao.jooq.generated.tables.pojos.{Model, ModelUserAccess, User} +import org.apache.texera.service.resource.ModelAccessResource.{ + getModelUserAccessPrivilege, + getOwner, + isModelPublic, + userHasReadAccess, + userHasWriteAccess, + userOwnModel +} +import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers +import org.scalatest.{BeforeAndAfterAll, BeforeAndAfterEach} + +import scala.jdk.CollectionConverters._ + +class ModelAccessResourceSpec + extends AnyFlatSpec + with Matchers + with MockTexeraDB + with BeforeAndAfterAll + with BeforeAndAfterEach { + + private val ownerUser: User = { + val user = new User + user.setName("model_owner") + user.setPassword("123") + user.setEmail("model_owner@test.com") + user.setRole(UserRoleEnum.REGULAR) + user + } + + private val readGranteeUser: User = { + val user = new User + user.setName("read_grantee") + user.setPassword("123") + user.setEmail("read_grantee@test.com") + user.setRole(UserRoleEnum.REGULAR) + user + } + + private val writeGranteeUser: User = { + val user = new User + user.setName("write_grantee") + user.setPassword("123") + user.setEmail("write_grantee@test.com") + user.setRole(UserRoleEnum.REGULAR) + user + } + + private val strangerUser: User = { + val user = new User + user.setName("stranger") + user.setPassword("123") + user.setEmail("stranger@test.com") + user.setRole(UserRoleEnum.REGULAR) + user + } + + private val privateModel: Model = { + val model = new Model + model.setName("private-model") + model.setRepositoryName("private-model") + model.setIsPublic(false) + model.setIsDownloadable(true) + model.setDescription("private model for access tests") + model.setFramework("pytorch") + model + } + + private val publicModel: Model = { + val model = new Model + model.setName("public-model") + model.setRepositoryName("public-model") + model.setIsPublic(true) + model.setIsDownloadable(true) + model.setDescription("public model for access tests") + model.setFramework("pytorch") + model + } + + private val nonExistentMid: Integer = 999999 + + lazy val accessResource = new ModelAccessResource() + + lazy val ownerSession = new SessionUser(ownerUser) + lazy val writeGranteeSession = new SessionUser(writeGranteeUser) + lazy val readGranteeSession = new SessionUser(readGranteeUser) + lazy val strangerSession = new SessionUser(strangerUser) + + private def grantDirectly(mid: Integer, uid: Integer, privilege: PrivilegeEnum): Unit = { + new ModelUserAccessDao(getDSLContext.configuration()) + .insert(new ModelUserAccess(mid, uid, privilege)) + } + + private def accessList(mid: Integer): List[ModelAccessResource.AccessEntry] = + accessResource.getAccessList(mid).asScala.toList + + override protected def beforeAll(): Unit = { + super.beforeAll() + initializeDBAndReplaceDSLContext() + + val userDao = new UserDao(getDSLContext.configuration()) + userDao.insert(ownerUser) + userDao.insert(readGranteeUser) + userDao.insert(writeGranteeUser) + userDao.insert(strangerUser) + + privateModel.setOwnerUid(ownerUser.getUid) + publicModel.setOwnerUid(ownerUser.getUid) + val modelDao = new ModelDao(getDSLContext.configuration()) + modelDao.insert(privateModel) + modelDao.insert(publicModel) + } + + override protected def beforeEach(): Unit = { + super.beforeEach() + // every test starts with no explicit grants + getDSLContext.deleteFrom(MODEL_USER_ACCESS).execute() + } + + override protected def afterAll(): Unit = { + try shutdownDB() + finally super.afterAll() + } + + // =========================================================================== + // Privilege helpers + // =========================================================================== + + "isModelPublic" should "be true for a public model and false for a private one" in { + isModelPublic(getDSLContext, publicModel.getMid) shouldBe true + isModelPublic(getDSLContext, privateModel.getMid) shouldBe false + } + + "userOwnModel" should "be true only for the owner" in { + userOwnModel(getDSLContext, privateModel.getMid, ownerUser.getUid) shouldBe true + userOwnModel(getDSLContext, privateModel.getMid, strangerUser.getUid) shouldBe false + } + + "getModelUserAccessPrivilege" should "return NONE for a user without an explicit grant" in { + getModelUserAccessPrivilege( + getDSLContext, + privateModel.getMid, + strangerUser.getUid + ) shouldEqual PrivilegeEnum.NONE + } + + it should "return the granted privilege for a grantee" in { + grantDirectly(privateModel.getMid, readGranteeUser.getUid, PrivilegeEnum.READ) + grantDirectly(privateModel.getMid, writeGranteeUser.getUid, PrivilegeEnum.WRITE) + + getModelUserAccessPrivilege( + getDSLContext, + privateModel.getMid, + readGranteeUser.getUid + ) shouldEqual PrivilegeEnum.READ + getModelUserAccessPrivilege( + getDSLContext, + privateModel.getMid, + writeGranteeUser.getUid + ) shouldEqual PrivilegeEnum.WRITE + } + + "the owner" should "have both read and write access to the model" in { + userHasReadAccess(getDSLContext, privateModel.getMid, ownerUser.getUid) shouldBe true + userHasWriteAccess(getDSLContext, privateModel.getMid, ownerUser.getUid) shouldBe true + } + + "a READ grantee" should "have read but not write access" in { + grantDirectly(privateModel.getMid, readGranteeUser.getUid, PrivilegeEnum.READ) + + userHasReadAccess(getDSLContext, privateModel.getMid, readGranteeUser.getUid) shouldBe true + userHasWriteAccess(getDSLContext, privateModel.getMid, readGranteeUser.getUid) shouldBe false + } + + "a WRITE grantee" should "have both read and write access" in { + grantDirectly(privateModel.getMid, writeGranteeUser.getUid, PrivilegeEnum.WRITE) + + userHasReadAccess(getDSLContext, privateModel.getMid, writeGranteeUser.getUid) shouldBe true + userHasWriteAccess(getDSLContext, privateModel.getMid, writeGranteeUser.getUid) shouldBe true + } + + "a user with no grant" should "have no access to a private model" in { + userHasReadAccess(getDSLContext, privateModel.getMid, strangerUser.getUid) shouldBe false + userHasWriteAccess(getDSLContext, privateModel.getMid, strangerUser.getUid) shouldBe false + } + + it should "have read but not write access to a public model" in { + userHasReadAccess(getDSLContext, publicModel.getMid, strangerUser.getUid) shouldBe true + userHasWriteAccess(getDSLContext, publicModel.getMid, strangerUser.getUid) shouldBe false + } + + it should "have no explicit privilege row on a public model" in { + // public read access comes from is_public, not from a model_user_access row + getModelUserAccessPrivilege( + getDSLContext, + publicModel.getMid, + strangerUser.getUid + ) shouldEqual PrivilegeEnum.NONE + } + + "an explicit WRITE grant on a public model" should "give a non-owner write access" in { + grantDirectly(publicModel.getMid, writeGranteeUser.getUid, PrivilegeEnum.WRITE) + + userHasWriteAccess(getDSLContext, publicModel.getMid, writeGranteeUser.getUid) shouldBe true + } + + "the privilege helpers" should "treat a nonexistent model as private, unowned, and ungranted" in { + isModelPublic(getDSLContext, nonExistentMid) shouldBe false + userOwnModel(getDSLContext, nonExistentMid, ownerUser.getUid) shouldBe false + getModelUserAccessPrivilege( + getDSLContext, + nonExistentMid, + ownerUser.getUid + ) shouldEqual PrivilegeEnum.NONE + userHasReadAccess(getDSLContext, nonExistentMid, ownerUser.getUid) shouldBe false + userHasWriteAccess(getDSLContext, nonExistentMid, ownerUser.getUid) shouldBe false + } + + "getOwner" should "return the owning user" in { + getOwner(getDSLContext, privateModel.getMid).getEmail shouldEqual ownerUser.getEmail + } + + it should "return null for a nonexistent model" in { + getOwner(getDSLContext, nonExistentMid) shouldBe null + } + + // =========================================================================== + // grantAccess / getAccessList + // =========================================================================== + + "grantAccess" should "add a grantee that appears in the access list with the granted privilege" in { + val response = accessResource.grantAccess( + privateModel.getMid, + readGranteeUser.getEmail, + "READ", + ownerSession + ) + response.getStatus shouldEqual 200 + + val entries = accessList(privateModel.getMid) + entries should have size 1 + entries.head.email shouldEqual readGranteeUser.getEmail + entries.head.name shouldEqual readGranteeUser.getName + entries.head.privilege shouldEqual PrivilegeEnum.READ + } + + it should "update the privilege in place when re-granting with a different privilege" in { + accessResource.grantAccess( + privateModel.getMid, + readGranteeUser.getEmail, + "READ", + ownerSession + ) + accessResource.grantAccess( + privateModel.getMid, + readGranteeUser.getEmail, + "WRITE", + ownerSession + ) + + val entries = accessList(privateModel.getMid) + entries should have size 1 + entries.head.email shouldEqual readGranteeUser.getEmail + entries.head.privilege shouldEqual PrivilegeEnum.WRITE + } + + it should "allow a WRITE grantee to share the model" in { + grantDirectly(privateModel.getMid, writeGranteeUser.getUid, PrivilegeEnum.WRITE) + + val response = accessResource.grantAccess( + privateModel.getMid, + strangerUser.getEmail, + "READ", + writeGranteeSession + ) + response.getStatus shouldEqual 200 + + userHasReadAccess(getDSLContext, privateModel.getMid, strangerUser.getUid) shouldBe true + } + + it should "be forbidden for a user without write access" in { + val ex = intercept[ForbiddenException] { + accessResource.grantAccess( + privateModel.getMid, + readGranteeUser.getEmail, + "READ", + strangerSession + ) + } + ex.getResponse.getStatus shouldEqual 403 + ex.getMessage should include( + s"You do not have permission to modify model ${privateModel.getMid}" + ) + } + + it should "be forbidden for a READ grantee" in { + grantDirectly(privateModel.getMid, readGranteeUser.getUid, PrivilegeEnum.READ) + + assertThrows[ForbiddenException] { + accessResource.grantAccess( + privateModel.getMid, + strangerUser.getEmail, + "READ", + readGranteeSession + ) + } + } + + "getAccessList" should "return an empty list when no access has been granted" in { + accessList(privateModel.getMid) shouldBe empty + } + + it should "not include the owner's own access row" in { + // even if the owner somehow has an explicit access row, the list only shows other users + grantDirectly(privateModel.getMid, ownerUser.getUid, PrivilegeEnum.WRITE) + grantDirectly(privateModel.getMid, readGranteeUser.getUid, PrivilegeEnum.READ) + + val entries = accessList(privateModel.getMid) + entries should have size 1 + entries.head.email shouldEqual readGranteeUser.getEmail + } + + it should "list multiple grantees with their respective privileges" in { + grantDirectly(privateModel.getMid, readGranteeUser.getUid, PrivilegeEnum.READ) + grantDirectly(privateModel.getMid, writeGranteeUser.getUid, PrivilegeEnum.WRITE) + + val entries = accessList(privateModel.getMid) + entries should have size 2 + val privilegeByEmail = entries.map(entry => entry.email -> entry.privilege).toMap + privilegeByEmail(readGranteeUser.getEmail) shouldEqual PrivilegeEnum.READ + privilegeByEmail(writeGranteeUser.getEmail) shouldEqual PrivilegeEnum.WRITE + } + + // =========================================================================== + // revokeAccess + // =========================================================================== + + "revokeAccess" should "remove the grantee from the access list and drop their access" in { + grantDirectly(privateModel.getMid, readGranteeUser.getUid, PrivilegeEnum.READ) + + val response = accessResource.revokeAccess( + privateModel.getMid, + readGranteeUser.getEmail, + ownerSession + ) + response.getStatus shouldEqual 200 + + accessList(privateModel.getMid) shouldBe empty + getModelUserAccessPrivilege( + getDSLContext, + privateModel.getMid, + readGranteeUser.getUid + ) shouldEqual PrivilegeEnum.NONE + userHasReadAccess(getDSLContext, privateModel.getMid, readGranteeUser.getUid) shouldBe false + } + + it should "allow a WRITE grantee to revoke another user's access" in { + grantDirectly(privateModel.getMid, writeGranteeUser.getUid, PrivilegeEnum.WRITE) + grantDirectly(privateModel.getMid, readGranteeUser.getUid, PrivilegeEnum.READ) + + val response = accessResource.revokeAccess( + privateModel.getMid, + readGranteeUser.getEmail, + writeGranteeSession + ) + response.getStatus shouldEqual 200 + + userHasReadAccess(getDSLContext, privateModel.getMid, readGranteeUser.getUid) shouldBe false + } + + it should "succeed as a no-op when the target user has no explicit grant" in { + val response = accessResource.revokeAccess( + privateModel.getMid, + strangerUser.getEmail, + ownerSession + ) + response.getStatus shouldEqual 200 + accessList(privateModel.getMid) shouldBe empty + } + + it should "be forbidden for a user without write access" in { + grantDirectly(privateModel.getMid, readGranteeUser.getUid, PrivilegeEnum.READ) + + assertThrows[ForbiddenException] { + accessResource.revokeAccess( + privateModel.getMid, + readGranteeUser.getEmail, + strangerSession + ) + } + } + + // =========================================================================== + // getOwnerEmailOfModel + // =========================================================================== + + "getOwnerEmailOfModel" should "return the owner's email" in { + accessResource.getOwnerEmailOfModel(privateModel.getMid) shouldEqual ownerUser.getEmail + } + + it should "return an empty string for a nonexistent model" in { + accessResource.getOwnerEmailOfModel(nonExistentMid) shouldEqual "" + } +} diff --git a/file-service/src/test/scala/org/apache/texera/service/resource/ModelResourceSpec.scala b/file-service/src/test/scala/org/apache/texera/service/resource/ModelResourceSpec.scala new file mode 100644 index 00000000000..f3c737d2e17 --- /dev/null +++ b/file-service/src/test/scala/org/apache/texera/service/resource/ModelResourceSpec.scala @@ -0,0 +1,516 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.texera.service.resource + +import jakarta.ws.rs._ +import org.apache.texera.auth.SessionUser +import org.apache.texera.dao.MockTexeraDB +import org.apache.texera.dao.jooq.generated.enums.{PrivilegeEnum, UserRoleEnum} +import org.apache.texera.dao.jooq.generated.tables.daos.{ModelDao, UserDao} +import org.apache.texera.dao.jooq.generated.tables.pojos.{Model, User} +import org.apache.texera.service.MockLakeFS +import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers +import org.scalatest.{BeforeAndAfterAll, BeforeAndAfterEach} + +class ModelResourceSpec + extends AnyFlatSpec + with Matchers + with MockTexeraDB + with MockLakeFS + with BeforeAndAfterAll + with BeforeAndAfterEach { + + private val ownerUser: User = { + val user = new User + user.setName("model_user") + user.setPassword("123") + user.setEmail("model_user@test.com") + user.setRole(UserRoleEnum.ADMIN) + user + } + + private val otherUser: User = { + val user = new User + user.setName("model_user2") + user.setPassword("123") + user.setEmail("model_user2@test.com") + user.setRole(UserRoleEnum.ADMIN) + user + } + + private val baseModel: Model = { + val model = new Model + model.setName("test-model") + model.setRepositoryName("test-model") + model.setIsPublic(true) + model.setIsDownloadable(true) + model.setDescription("model for test") + model.setFramework("pytorch") + model + } + + private lazy val modelDao = new ModelDao(getDSLContext.configuration()) + + lazy val modelResource = new ModelResource() + + lazy val sessionUser = new SessionUser(ownerUser) + lazy val sessionUser2 = new SessionUser(otherUser) + + private def assertStatus(ex: WebApplicationException, status: Int): Unit = + ex.getResponse.getStatus shouldEqual status + + override protected def beforeAll(): Unit = { + super.beforeAll() + + initializeDBAndReplaceDSLContext() + + val userDao = new UserDao(getDSLContext.configuration()) + userDao.insert(ownerUser) + userDao.insert(otherUser) + + baseModel.setOwnerUid(ownerUser.getUid) + modelDao.insert(baseModel) + } + + override protected def afterAll(): Unit = { + try shutdownDB() + finally super.afterAll() + } + + // =========================================================================== + // createModel + // =========================================================================== + "createModel" should "create a model successfully if the user has no model with the same name" in { + val request = ModelResource.CreateModelRequest( + modelName = "new-model", + modelDescription = "description for new model", + isModelPublic = false, + isModelDownloadable = true, + framework = "pytorch", + format = "torchscript" + ) + + val created = modelResource.createModel(request, sessionUser) + created.model.getName shouldEqual "new-model" + created.model.getDescription shouldEqual "description for new model" + created.model.getIsPublic shouldBe false + created.model.getIsDownloadable shouldBe true + created.model.getFramework shouldEqual "pytorch" + created.model.getFormat shouldEqual "torchscript" + // the LakeFS repository is named after the created model's id + created.model.getRepositoryName shouldEqual s"model-${created.model.getMid}" + } + + it should "default the framework to pytorch when none is provided" in { + val request = ModelResource.CreateModelRequest( + modelName = "framework-default-model", + modelDescription = "no framework provided", + isModelPublic = false, + isModelDownloadable = true, + framework = "", + format = null + ) + + val created = modelResource.createModel(request, sessionUser) + created.model.getFramework shouldEqual "pytorch" + } + + it should "refuse to create a model if the user already has one with the same name" in { + val request = ModelResource.CreateModelRequest( + modelName = "test-model", + modelDescription = "duplicate name", + isModelPublic = false, + isModelDownloadable = true, + framework = "pytorch", + format = null + ) + + assertThrows[BadRequestException] { + modelResource.createModel(request, sessionUser) + } + } + + it should "create a model successfully if another user has one with the same name" in { + val request = ModelResource.CreateModelRequest( + modelName = "test-model", + modelDescription = "same name, different owner", + isModelPublic = false, + isModelDownloadable = true, + framework = "pytorch", + format = null + ) + + val created = modelResource.createModel(request, sessionUser2) + created.model.getName shouldEqual "test-model" + } + + it should "reject an invalid model name" in { + val request = ModelResource.CreateModelRequest( + modelName = "bad name!", + modelDescription = "invalid name", + isModelPublic = false, + isModelDownloadable = true, + framework = "pytorch", + format = null + ) + + assertThrows[BadRequestException] { + modelResource.createModel(request, sessionUser) + } + } + + it should "return a DashboardModel with owner email, WRITE privilege, isOwner=true and size 0" in { + val request = ModelResource.CreateModelRequest( + modelName = "dashboard-model", + modelDescription = "dashboard properties", + isModelPublic = true, + isModelDownloadable = false, + framework = "pytorch", + format = null + ) + + val dashboard = modelResource.createModel(request, sessionUser) + dashboard.ownerEmail shouldEqual ownerUser.getEmail + dashboard.accessPrivilege shouldEqual PrivilegeEnum.WRITE + dashboard.isOwner shouldBe true + dashboard.size shouldEqual 0 + } + + // =========================================================================== + // getModel / listModels + // =========================================================================== + "getModel" should "return the dashboard model including its LakeFS repository size" in { + val request = ModelResource.CreateModelRequest( + modelName = "get-model", + modelDescription = "for get", + isModelPublic = false, + isModelDownloadable = true, + framework = "pytorch", + format = null + ) + val created = modelResource.createModel(request, sessionUser) + + val dashboard = modelResource.getModel(created.model.getMid, sessionUser) + dashboard.model.getMid shouldEqual created.model.getMid + dashboard.size should be >= 0L + } + + it should "forbid a stranger from getting a private model" in { + val request = ModelResource.CreateModelRequest( + modelName = "private-get-model", + modelDescription = "private", + isModelPublic = false, + isModelDownloadable = true, + framework = "pytorch", + format = null + ) + val created = modelResource.createModel(request, sessionUser) + + assertThrows[ForbiddenException] { + modelResource.getModel(created.model.getMid, sessionUser2) + } + } + + "listModels" should "include models the user owns" in { + val request = ModelResource.CreateModelRequest( + modelName = "listed-model", + modelDescription = "for list", + isModelPublic = false, + isModelDownloadable = true, + framework = "pytorch", + format = null + ) + val created = modelResource.createModel(request, sessionUser) + + val listed = modelResource.listModels(sessionUser) + listed.map(_.model.getMid) should contain(created.model.getMid) + } + + // =========================================================================== + // deleteModel + // =========================================================================== + "deleteModel" should "delete a model successfully if the user owns it" in { + val request = ModelResource.CreateModelRequest( + modelName = "delete-model", + modelDescription = "for delete", + isModelPublic = false, + isModelDownloadable = true, + framework = "pytorch", + format = null + ) + val created = modelResource.createModel(request, sessionUser) + + val response = modelResource.deleteModel(created.model.getMid, sessionUser) + response.getStatus shouldEqual 200 + modelDao.fetchOneByMid(created.model.getMid) shouldBe null + } + + it should "refuse to delete a model not owned by the user" in { + val request = ModelResource.CreateModelRequest( + modelName = "forbidden-delete-model", + modelDescription = "for forbidden delete", + isModelPublic = true, + isModelDownloadable = true, + framework = "pytorch", + format = null + ) + val created = modelResource.createModel(request, sessionUser) + + assertThrows[ForbiddenException] { + modelResource.deleteModel(created.model.getMid, sessionUser2) + } + modelDao.fetchOneByMid(created.model.getMid) should not be null + } + + it should "surface a LakeFS 404 as NotFoundException when deleting a model whose repo is missing" in { + val model = new Model + model.setName("delete-model-no-repo") + model.setRepositoryName("delete-model-no-repo") + model.setDescription("for lakefs 404 mapping test") + model.setOwnerUid(ownerUser.getUid) + model.setIsPublic(true) + model.setIsDownloadable(true) + model.setFramework("pytorch") + modelDao.insert(model) + // intentionally no repo created in LakeFS + + val ex = intercept[NotFoundException] { + modelResource.deleteModel(model.getMid, sessionUser) + } + assertStatus(ex, 404) + } + + // =========================================================================== + // update name / description / publicity / downloadable + // =========================================================================== + "updateModelName" should "rename a model the user can write to" in { + val created = modelResource.createModel( + ModelResource.CreateModelRequest( + modelName = "rename-me", + modelDescription = "d", + isModelPublic = false, + isModelDownloadable = true, + framework = "pytorch", + format = null + ), + sessionUser + ) + + val response = modelResource.updateModelName( + ModelResource.ModelNameModification(created.model.getMid, "renamed"), + sessionUser + ) + response.getStatus shouldEqual 200 + modelDao.fetchOneByMid(created.model.getMid).getName shouldEqual "renamed" + } + + "updateModelDescription" should "update the description of a model the user can write to" in { + val created = modelResource.createModel( + ModelResource.CreateModelRequest( + modelName = "describe-me", + modelDescription = "old", + isModelPublic = false, + isModelDownloadable = true, + framework = "pytorch", + format = null + ), + sessionUser + ) + + val response = modelResource.updateModelDescription( + ModelResource.ModelDescriptionModification(created.model.getMid, "new description"), + sessionUser + ) + response.getStatus shouldEqual 200 + modelDao.fetchOneByMid(created.model.getMid).getDescription shouldEqual "new description" + } + + "toggleModelPublicity" should "flip the public flag for a writer" in { + val created = modelResource.createModel( + ModelResource.CreateModelRequest( + modelName = "publicity-model", + modelDescription = "d", + isModelPublic = false, + isModelDownloadable = true, + framework = "pytorch", + format = null + ), + sessionUser + ) + + modelResource.toggleModelPublicity(created.model.getMid, sessionUser).getStatus shouldEqual 200 + modelDao.fetchOneByMid(created.model.getMid).getIsPublic shouldBe true + } + + "toggleModelDownloadable" should "flip the downloadable flag for the owner" in { + val created = modelResource.createModel( + ModelResource.CreateModelRequest( + modelName = "downloadable-model", + modelDescription = "d", + isModelPublic = false, + isModelDownloadable = true, + framework = "pytorch", + format = null + ), + sessionUser + ) + + modelResource + .toggleModelDownloadable(created.model.getMid, sessionUser) + .getStatus shouldEqual 200 + modelDao.fetchOneByMid(created.model.getMid).getIsDownloadable shouldBe false + } + + it should "forbid a non-owner from toggling downloadable" in { + val created = modelResource.createModel( + ModelResource.CreateModelRequest( + modelName = "downloadable-forbidden-model", + modelDescription = "d", + isModelPublic = true, + isModelDownloadable = true, + framework = "pytorch", + format = null + ), + sessionUser + ) + + assertThrows[ForbiddenException] { + modelResource.toggleModelDownloadable(created.model.getMid, sessionUser2) + } + } + + it should "refuse to rename a model to a name the owner already uses" in { + modelResource.createModel( + ModelResource.CreateModelRequest( + modelName = "dup-target", + modelDescription = "d", + isModelPublic = false, + isModelDownloadable = true, + framework = "pytorch", + format = null + ), + sessionUser + ) + val second = modelResource.createModel( + ModelResource.CreateModelRequest( + modelName = "dup-source", + modelDescription = "d", + isModelPublic = false, + isModelDownloadable = true, + framework = "pytorch", + format = null + ), + sessionUser + ) + + assertThrows[BadRequestException] { + modelResource.updateModelName( + ModelResource.ModelNameModification(second.model.getMid, "dup-target"), + sessionUser + ) + } + } + + it should "forbid a user without write access from renaming or re-describing a model" in { + val created = modelResource.createModel( + ModelResource.CreateModelRequest( + modelName = "no-write-updates", + modelDescription = "d", + isModelPublic = true, + isModelDownloadable = true, + framework = "pytorch", + format = null + ), + sessionUser + ) + + assertThrows[ForbiddenException] { + modelResource.updateModelName( + ModelResource.ModelNameModification(created.model.getMid, "hijacked"), + sessionUser2 + ) + } + assertThrows[ForbiddenException] { + modelResource.updateModelDescription( + ModelResource.ModelDescriptionModification(created.model.getMid, "hijacked"), + sessionUser2 + ) + } + } + + // =========================================================================== + // getPublicModel / listModels public merge + // =========================================================================== + "getPublicModel" should "return a public model without authentication" in { + val created = modelResource.createModel( + ModelResource.CreateModelRequest( + modelName = "public-get-model", + modelDescription = "d", + isModelPublic = true, + isModelDownloadable = true, + framework = "pytorch", + format = null + ), + sessionUser + ) + + val dashboard = modelResource.getPublicModel(created.model.getMid) + dashboard.model.getMid shouldEqual created.model.getMid + } + + it should "forbid access to a private model" in { + val created = modelResource.createModel( + ModelResource.CreateModelRequest( + modelName = "public-get-private-model", + modelDescription = "d", + isModelPublic = false, + isModelDownloadable = true, + framework = "pytorch", + format = null + ), + sessionUser + ) + + assertThrows[ForbiddenException] { + modelResource.getPublicModel(created.model.getMid) + } + } + + "listModels" should "include public models owned by another user" in { + val othersPublic = modelResource.createModel( + ModelResource.CreateModelRequest( + modelName = "others-public-model", + modelDescription = "d", + isModelPublic = true, + isModelDownloadable = true, + framework = "pytorch", + format = null + ), + sessionUser2 + ) + + val listed = modelResource.listModels(sessionUser) + val entry = listed.find(_.model.getMid == othersPublic.model.getMid) + entry should not be empty + entry.get.isOwner shouldBe false + entry.get.accessPrivilege shouldEqual PrivilegeEnum.READ + } +} diff --git a/sql/changelog.xml b/sql/changelog.xml index ac90daed46a..16e81dea42d 100644 --- a/sql/changelog.xml +++ b/sql/changelog.xml @@ -63,6 +63,10 @@ + + + +