aicam commented on code in PR #6502:
URL: https://github.com/apache/texera/pull/6502#discussion_r3688287224
##########
common/workflow-core/src/test/scala/org/apache/texera/amber/storage/FileResolverSpec.scala:
##########
@@ -118,6 +124,27 @@ class FileResolverSpec
)
}
+ "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 resolve a path whose prefix is not the datasets
prefix" in {
Review Comment:
We don't want to force `datasets` prefix, its an enum that can extend
##########
amber/src/main/python/pytexera/storage/dataset_file_document.py:
##########
@@ -56,20 +58,23 @@ def __init__(self, file_path: str):
Parses the file path into dataset metadata.
:param file_path:
- Expected format -
"/ownerEmail/datasetName/versionName/fileRelativePath"
- Example:
"/[email protected]/twitterDataset/v1/california/irvine/tw1.csv"
+ Expected format -
+ "/datasets/ownerEmail/datasetName/versionName/fileRelativePath"
+ Example:
+ "/datasets/[email protected]/twitterDataset/v1/california/tw1.csv"
"""
parts = file_path.strip("/").split("/")
- if len(parts) < 4:
+
+ if len(parts) < 5 or parts[0] != ResourceType.DATASETS.value:
Review Comment:
Check `amber/src/main/python/core/storage/vfs_uri_factory.py`, we should
check if part[0] is a valid input based on all possible ResourceType keys,
follow same pattern as `VFSResourceType`
##########
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/[email protected]/twitterDataset/v1")
+ assert(owner == "[email protected]")
+ 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 {
Review Comment:
ditto
##########
common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/source/dataset/FileListerSourceOpExec.scala:
##########
@@ -28,13 +29,34 @@ 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
(/datasets/ownerEmail/datasetName/versionName) into its
+ * (ownerEmail, datasetName, versionName) components.
+ *
+ * @throws IllegalArgumentException if the path is not a well-formed
dataset version path
+ */
+ private[dataset] def parseDatasetVersionPath(
+ datasetVersionPath: String
+ ): (String, String, String) = {
+ val segments = datasetVersionPath.split("/").filter(_.nonEmpty)
+ require(
+ segments.length >= 4 && segments.head == ResourceType.Datasets.toString,
Review Comment:
ditto
##########
amber/src/main/python/pytexera/storage/dataset_file_document.py:
##########
@@ -90,6 +95,7 @@ def get_presigned_url(self) -> str:
"""
headers = {"Authorization": f"Bearer {self.jwt_token}"}
encoded_file_path = urllib.parse.quote(
+ f"/{ResourceType.DATASETS.value}"
Review Comment:
It shouldn't pass `dataset` by default, make sure it either pass as input or
from class itself (using `self`)
##########
common/workflow-core/src/main/scala/org/apache/texera/amber/core/storage/FileResolver.scala:
##########
@@ -88,23 +88,23 @@ object FileResolver {
val filePath = Paths.get(fileName)
val pathSegments = (0 until
filePath.getNameCount).map(filePath.getName(_).toString).toArray
- if (pathSegments.length < 4) {
+ if (pathSegments.length < 5 || pathSegments(0) !=
ResourceType.Datasets.toString) {
Review Comment:
ditto
##########
file-service/src/main/scala/org/apache/texera/service/resource/DatasetResource.scala:
##########
@@ -2280,7 +2298,8 @@ class DatasetResource extends LazyLogging {
)
val owner = getOwner(ctx, did)
- val fullPath = s"${owner.getEmail}/${dataset.getName}/$coverImage"
+ val fullPath =
+
s"${ResourceType.Datasets}/${owner.getEmail}/${dataset.getName}/$coverImage"
Review Comment:
It should be passed not hardcoded
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
