Copilot commented on code in PR #6473: URL: https://github.com/apache/texera/pull/6473#discussion_r3600974795
########## common/workflow-core/src/test/scala/org/apache/texera/amber/core/storage/model/DatasetFileDocumentSpec.scala: ########## @@ -0,0 +1,143 @@ +/* + * 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.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers + +import java.net.{URI, URLEncoder} +import java.nio.charset.StandardCharsets +import java.nio.file.Paths + +class DatasetFileDocumentSpec extends AnyFlatSpec with Matchers { + + // Realistic 40-char git commit hash, mirroring the URIs produced by FileResolver + // (format: dataset:///{repositoryName}/{versionHash}/{fileRelativePath}). + private val versionHash = "97fd4c2a755b69b7c66d322eab40b7e5c2ad5d10" + + "DatasetFileDocument" should "parse a valid 3-segment dataset URI into its components" in { + val uri = new URI(s"dataset:///test_dataset/$versionHash/1.txt") + val doc = new DatasetFileDocument(uri) + + doc.getRepositoryName() shouldBe "test_dataset" + doc.getVersionHash() shouldBe versionHash + doc.getFileRelativePath() shouldBe "1.txt" + } + + it should "join multi-segment relative paths correctly" in { + val uri = new URI(s"dataset:///my_repo/$versionHash/some/nested/dir/data.csv") + val doc = new DatasetFileDocument(uri) + + doc.getRepositoryName() shouldBe "my_repo" + doc.getVersionHash() shouldBe versionHash + doc.getFileRelativePath() shouldBe Paths.get("some", "nested", "dir", "data.csv").toString + } + + it should "URL-decode the version hash" in { + // FileResolver URL-encodes segments and then builds the URI with the multi-arg + // constructor, so uri.getPath still contains URLEncoder-encoded segments. + val uri = new URI("dataset", "", "/repo/hash%20with%2Bspecials/file.txt", null) + val doc = new DatasetFileDocument(uri) + + doc.getVersionHash() shouldBe "hash with+specials" + doc.getFileRelativePath() shouldBe "file.txt" + } Review Comment: This test assumes the URI path still contains percent-escapes (e.g., `%20`, `%2B`) when `DatasetFileDocument` reads `uri.getPath`, but `DatasetFileDocument` splits on `uri.getPath` and then runs `URLDecoder.decode`. In practice, `URLDecoder` is what turns `+` into spaces, so using a `+`-encoded hash here better reflects what the parser actually does (and avoids relying on `%2B` round-tripping, which the current implementation cannot preserve). ########## common/workflow-core/src/test/scala/org/apache/texera/amber/core/storage/model/DatasetFileDocumentSpec.scala: ########## @@ -0,0 +1,143 @@ +/* + * 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.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers + +import java.net.{URI, URLEncoder} +import java.nio.charset.StandardCharsets +import java.nio.file.Paths + +class DatasetFileDocumentSpec extends AnyFlatSpec with Matchers { + + // Realistic 40-char git commit hash, mirroring the URIs produced by FileResolver + // (format: dataset:///{repositoryName}/{versionHash}/{fileRelativePath}). + private val versionHash = "97fd4c2a755b69b7c66d322eab40b7e5c2ad5d10" + + "DatasetFileDocument" should "parse a valid 3-segment dataset URI into its components" in { + val uri = new URI(s"dataset:///test_dataset/$versionHash/1.txt") + val doc = new DatasetFileDocument(uri) + + doc.getRepositoryName() shouldBe "test_dataset" + doc.getVersionHash() shouldBe versionHash + doc.getFileRelativePath() shouldBe "1.txt" + } + + it should "join multi-segment relative paths correctly" in { + val uri = new URI(s"dataset:///my_repo/$versionHash/some/nested/dir/data.csv") + val doc = new DatasetFileDocument(uri) + + doc.getRepositoryName() shouldBe "my_repo" + doc.getVersionHash() shouldBe versionHash + doc.getFileRelativePath() shouldBe Paths.get("some", "nested", "dir", "data.csv").toString + } + + it should "URL-decode the version hash" in { + // FileResolver URL-encodes segments and then builds the URI with the multi-arg + // constructor, so uri.getPath still contains URLEncoder-encoded segments. + val uri = new URI("dataset", "", "/repo/hash%20with%2Bspecials/file.txt", null) + val doc = new DatasetFileDocument(uri) + + doc.getVersionHash() shouldBe "hash with+specials" + doc.getFileRelativePath() shouldBe "file.txt" + } + + it should "URL-decode each relative path segment" in { + val uri = new URI("dataset", "", "/repo/hash/dir+one/file%23two%20a.csv", null) + val doc = new DatasetFileDocument(uri) + + doc.getRepositoryName() shouldBe "repo" + doc.getVersionHash() shouldBe "hash" + doc.getFileRelativePath() shouldBe Paths.get("dir one", "file#two a.csv").toString + } + + it should "return the parsed components and the original URI through its getters" in { + val uri = new URI("dataset", "", s"/repo/$versionHash/a%20b/c.csv", null) + val doc = new DatasetFileDocument(uri) + + doc.getRepositoryName() shouldBe "repo" + doc.getVersionHash() shouldBe versionHash + doc.getFileRelativePath() shouldBe Paths.get("a b", "c.csv").toString + // getURI must hand back the exact URI the document was constructed with. + doc.getURI shouldBe uri + doc.getURI.toString shouldBe uri.toString + } + + it should "not URL-decode the repository name" in { + // parseUri only URLDecoder-decodes the version hash and the relative-path + // segments; the repository name is returned raw. This mirrors FileResolver, + // which URLEncoder-encodes only the fileRelativePath segments. The multi-arg + // URI constructor is required here: a single-arg URI already percent-decodes + // getPath, so "%20" in a raw URI string would reach parseUri as a space. + val uri = new URI("dataset", "", "/repo%20name/hash%20value/file.txt", null) + val doc = new DatasetFileDocument(uri) + + doc.getRepositoryName() shouldBe "repo%20name" + // Same encoded token in the version-hash position IS decoded (asymmetry pin). + doc.getVersionHash() shouldBe "hash value" + doc.getFileRelativePath() shouldBe "file.txt" + } Review Comment: This test uses `%20` in the repository-name segment and expects it to remain encoded, but `DatasetFileDocument` derives segments from `uri.getPath`, so percent-escapes are already resolved by the URI layer before the code ever sees them. If the intent is to pin the asymmetry that the repo name is *not* `URLDecoder`-decoded (i.e., `+` is preserved) while the version hash *is* decoded (`+` -> space), the test should use `+` tokens instead of `%20`. -- 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]
