This is an automated email from the ASF dual-hosted git repository. github-merge-queue[bot] pushed a commit to branch gh-readonly-queue/main/pr-6426-e08b7f336af68ce1bcaca012589beb41c7aed132 in repository https://gitbox.apache.org/repos/asf/texera.git
commit 6b451771aa4fff9c0dc63b8b6ebd7b7bbfcf4d28 Author: Kunwoo (Chris) <[email protected]> AuthorDate: Thu Jul 16 19:52:05 2026 -0400 fix(file-service): reject invalid or duplicate dataset renames (#6426) ### What changes were proposed in this PR? The dataset rename endpoint previously skipped both checks that dataset creation enforces: name validation and the per owner duplicate name check. A rename could therefore produce two datasets with the same `(owner, name)` pair, which breaks every lookup that resolves a dataset by `(owner email, dataset name)`. File uploads and workflow file reads fail with a `TooManyRowsException` for both datasets. This PR adds the missing guards to the backend rename endpoint, and a `UNIQUE (owner_uid, name)` constraint at the database level so that concurrent writes racing past the application-level check cannot reintroduce duplicates. The migration script `sql/updates/28.sql` deduplicates pre-existing duplicates before adding the constraint (the oldest dataset keeps its name, later ones get a `-<did>` suffix). The frontend rename input now applies the same name validation client-side and shows the detailed reason from the backend when a rename is rejected, instead of the generic "Update dataset name failed". Some additional changes: - `validateDatasetName` now also enforces the 128-character column limit - DEFAULT_DATASET_NAME changed from "Untitled dataset" to "Untitled-dataset", since the fallback name used for empty rename input must itself pass the new validation ### Any related issues, documentation, discussions? Closes #6424 ### How was this PR tested? https://github.com/user-attachments/assets/823bd44d-fef3-4aa0-b2c5-e6d27d359d9b ### Was this PR authored or co-authored using generative AI tooling? Generated-by: Claude Code, Claude Fable 5 --- .../texera/service/resource/DatasetResource.scala | 92 ++++++---- .../service/resource/DatasetResourceSpec.scala | 188 +++++++++++++++++++++ .../card-item/card-item.component.spec.ts | 42 ++++- .../list-item/card-item/card-item.component.ts | 21 ++- .../user/list-item/list-item.component.spec.ts | 58 +++++++ .../user/list-item/list-item.component.ts | 21 ++- .../user-dataset-list-item.component.spec.ts | 32 +++- .../user-dataset-list-item.component.ts | 14 +- .../service/user/dataset/dataset.service.spec.ts | 40 ++++- .../service/user/dataset/dataset.service.ts | 13 +- sql/changelog.xml | 5 + sql/texera_ddl.sql | 3 +- sql/updates/28.sql | 75 ++++++++ 13 files changed, 560 insertions(+), 44 deletions(-) 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 789f6373b9..c376e4ce04 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 @@ -279,21 +279,16 @@ class DatasetResource extends LazyLogging { val isDatasetPublic = request.isDatasetPublic val isDatasetDownloadable = request.isDatasetDownloadable - // validate dataset name - try { - validateDatasetName(datasetName) - } catch { - case e: IllegalArgumentException => - throw new BadRequestException(e.getMessage) - } + validateDatasetName(datasetName) // Check if a dataset with the same name already exists - val existingDatasets = context - .selectFrom(DATASET) - .where(DATASET.OWNER_UID.eq(uid)) - .and(DATASET.NAME.eq(datasetName)) - .fetch() - if (!existingDatasets.isEmpty) { + val duplicateExists = ctx.fetchExists( + ctx + .selectFrom(DATASET) + .where(DATASET.OWNER_UID.eq(uid)) + .and(DATASET.NAME.eq(datasetName)) + ) + if (duplicateExists) { throw new BadRequestException("Dataset with the same name already exists") } @@ -306,11 +301,13 @@ class DatasetResource extends LazyLogging { dataset.setOwnerUid(uid) // insert record and get created dataset with did - val createdDataset = ctx - .insertInto(DATASET) - .set(ctx.newRecord(DATASET, dataset)) - .returning() - .fetchOne() + val createdDataset = failOnDuplicateDatasetName { + ctx + .insertInto(DATASET) + .set(ctx.newRecord(DATASET, dataset)) + .returning() + .fetchOne() + } // Initialize the repository in LakeFS val repositoryName = s"dataset-${createdDataset.getDid}" @@ -514,8 +511,24 @@ class DatasetResource extends LazyLogging { throw new ForbiddenException(ERR_USER_HAS_NO_ACCESS_TO_DATASET_MESSAGE) } + validateDatasetName(modificator.name) + + // Check if the owner already has another dataset with the same name + val duplicateExists = ctx.fetchExists( + ctx + .selectFrom(DATASET) + .where(DATASET.OWNER_UID.eq(dataset.getOwnerUid)) + .and(DATASET.NAME.eq(modificator.name)) + .and(DATASET.DID.notEqual(dataset.getDid)) + ) + if (duplicateExists) { + throw new BadRequestException("Dataset with the same name already exists") + } + dataset.setName(modificator.name) - datasetDao.update(dataset) + failOnDuplicateDatasetName { + datasetDao.update(dataset) + } Response.ok().build() } } @@ -1423,29 +1436,48 @@ class DatasetResource extends LazyLogging { .fetchInto(classOf[String]) } + private val DATASET_NAME_MAX_LENGTH = 128 + private val DATASET_NAME_PATTERN = "^[A-Za-z0-9_-]+$".r + /** * Validates the dataset name. * * Rules: - * - Must be at least 1 character long. - * - Only lowercase letters, numbers, underscores, and hyphens are allowed. - * - Cannot start with a hyphen. + * - Must be 1 to 128 characters long. + * - Only letters, numbers, underscores, and hyphens are allowed. * * @param name The dataset name to validate. - * @throws java.lang.IllegalArgumentException if the name is invalid. + * @throws jakarta.ws.rs.BadRequestException if the name is invalid. */ private def validateDatasetName(name: String): Unit = { - val datasetNamePattern = "^[A-Za-z0-9_-]+$".r - if (!datasetNamePattern.matches(name)) { - throw new IllegalArgumentException( - s"Invalid dataset name: '$name'. " + - "Dataset names must be at least 1 character long and " + - "contain only lowercase letters, numbers, underscores, and hyphens, " + - "and cannot start with a hyphen." + if (name == null || !DATASET_NAME_PATTERN.matches(name)) { + throw new BadRequestException( + "Invalid dataset name: only letters, numbers, underscores, and hyphens are allowed." + ) + } + if (name.length > DATASET_NAME_MAX_LENGTH) { + throw new BadRequestException( + s"Invalid dataset name: name must be at most $DATASET_NAME_MAX_LENGTH characters long." ) } } + /** + * Runs a dataset 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 failOnDuplicateDatasetName[T](op: => T): T = { + try op + catch { + case e: DataAccessException => + if (e.sqlState() == "23505") { + throw new BadRequestException("Dataset with the same name already exists") + } + throw e + } + } + private def fetchDatasetVersions(ctx: DSLContext, did: Integer): List[DatasetVersion] = { ctx .selectFrom(DATASET_VERSION) diff --git a/file-service/src/test/scala/org/apache/texera/service/resource/DatasetResourceSpec.scala b/file-service/src/test/scala/org/apache/texera/service/resource/DatasetResourceSpec.scala index e4e6165241..99d1fd8311 100644 --- a/file-service/src/test/scala/org/apache/texera/service/resource/DatasetResourceSpec.scala +++ b/file-service/src/test/scala/org/apache/texera/service/resource/DatasetResourceSpec.scala @@ -801,6 +801,194 @@ class DatasetResourceSpec reloaded.getRepositoryName shouldEqual "rename-keeps-repo-stable" } + it should "refuse to rename dataset to an invalid name" in { + val dataset = new Dataset + dataset.setName("rename-invalid-src") + dataset.setRepositoryName("rename-invalid-src-repo") + dataset.setDescription("for rename invalid-name test") + dataset.setOwnerUid(ownerUser.getUid) + dataset.setIsPublic(true) + dataset.setIsDownloadable(true) + datasetDao.insert(dataset) + + Seq("", "a/b", "has space", "名前", "dot.dot", "a" * 129, null) foreach { invalidName => + withClue(s"renaming to '$invalidName': ") { + assertThrows[BadRequestException] { + datasetResource.updateDatasetName( + DatasetResource.DatasetNameModification(dataset.getDid, invalidName), + sessionUser + ) + } + } + } + + datasetDao.fetchOneByDid(dataset.getDid).getName shouldEqual "rename-invalid-src" + } + + it should "refuse to rename dataset to a name already used by another dataset of the same owner" in { + val existing = new Dataset + existing.setName("rename-dup-existing") + existing.setRepositoryName("rename-dup-existing-repo") + existing.setDescription("existing dataset for duplicate rename test") + existing.setOwnerUid(ownerUser.getUid) + existing.setIsPublic(true) + existing.setIsDownloadable(true) + datasetDao.insert(existing) + + val renamed = new Dataset + renamed.setName("rename-dup-source") + renamed.setRepositoryName("rename-dup-source-repo") + renamed.setDescription("dataset being renamed in duplicate rename test") + renamed.setOwnerUid(ownerUser.getUid) + renamed.setIsPublic(true) + renamed.setIsDownloadable(true) + datasetDao.insert(renamed) + + assertThrows[BadRequestException] { + datasetResource.updateDatasetName( + DatasetResource.DatasetNameModification(renamed.getDid, "rename-dup-existing"), + sessionUser + ) + } + + datasetDao.fetchOneByDid(renamed.getDid).getName shouldEqual "rename-dup-source" + } + + it should "allow renaming a dataset to its own current name" in { + val dataset = new Dataset + dataset.setName("rename-self-noop") + dataset.setRepositoryName("rename-self-noop-repo") + dataset.setDescription("for rename-to-self test") + dataset.setOwnerUid(ownerUser.getUid) + dataset.setIsPublic(true) + dataset.setIsDownloadable(true) + datasetDao.insert(dataset) + + val response = datasetResource.updateDatasetName( + DatasetResource.DatasetNameModification(dataset.getDid, "rename-self-noop"), + sessionUser + ) + + response.getStatus shouldEqual 200 + datasetDao.fetchOneByDid(dataset.getDid).getName shouldEqual "rename-self-noop" + } + + it should "allow renaming to a name used by a dataset of a different owner" in { + val otherOwners = new Dataset + otherOwners.setName("rename-cross-owner") + otherOwners.setRepositoryName("rename-cross-owner-repo") + otherOwners.setDescription("other owner's dataset for cross-owner rename test") + otherOwners.setOwnerUid(otherAdminUser.getUid) + otherOwners.setIsPublic(true) + otherOwners.setIsDownloadable(true) + datasetDao.insert(otherOwners) + + val dataset = new Dataset + dataset.setName("rename-cross-source") + dataset.setRepositoryName("rename-cross-source-repo") + dataset.setDescription("dataset being renamed in cross-owner rename test") + dataset.setOwnerUid(ownerUser.getUid) + dataset.setIsPublic(true) + dataset.setIsDownloadable(true) + datasetDao.insert(dataset) + + val response = datasetResource.updateDatasetName( + DatasetResource.DatasetNameModification(dataset.getDid, "rename-cross-owner"), + sessionUser + ) + + response.getStatus shouldEqual 200 + datasetDao.fetchOneByDid(dataset.getDid).getName shouldEqual "rename-cross-owner" + } + + "dataset table" should "enforce uniqueness of (owner_uid, name) at the database level" in { + val first = new Dataset + first.setName("db-unique-ds") + first.setRepositoryName("db-unique-ds-repo") + first.setDescription("first dataset for unique constraint test") + first.setOwnerUid(ownerUser.getUid) + first.setIsPublic(true) + first.setIsDownloadable(true) + datasetDao.insert(first) + + val duplicate = new Dataset + duplicate.setName("db-unique-ds") + duplicate.setRepositoryName("db-unique-ds-repo-2") + duplicate.setDescription("duplicate dataset for unique constraint test") + duplicate.setOwnerUid(ownerUser.getUid) + duplicate.setIsPublic(true) + duplicate.setIsDownloadable(true) + + assertThrows[org.jooq.exception.DataAccessException] { + datasetDao.insert(duplicate) + } + } + + "failOnDuplicateDatasetName" should "translate a unique-constraint violation into BadRequestException" in { + val existing = new Dataset + existing.setName("race-existing") + existing.setRepositoryName("race-existing-repo") + existing.setDescription("existing dataset for constraint-translation test") + existing.setOwnerUid(ownerUser.getUid) + existing.setIsPublic(true) + existing.setIsDownloadable(true) + datasetDao.insert(existing) + + val victim = new Dataset + victim.setName("race-victim") + victim.setRepositoryName("race-victim-repo") + victim.setDescription("dataset whose rename loses the race") + victim.setOwnerUid(ownerUser.getUid) + victim.setIsPublic(true) + victim.setIsDownloadable(true) + datasetDao.insert(victim) + + // Simulate the write that loses the race: the constraint fires because the + // duplicate already exists, and the helper must map it to a 400. + victim.setName("race-existing") + assertThrows[BadRequestException] { + datasetResource.failOnDuplicateDatasetName { + datasetDao.update(victim) + } + } + + datasetDao.fetchOneByDid(victim.getDid).getName shouldEqual "race-victim" + } + + it should "rethrow DataAccessExceptions that are not unique violations" in { + assertThrows[org.jooq.exception.DataAccessException] { + datasetResource.failOnDuplicateDatasetName { + throw new org.jooq.exception.DataAccessException( + "lock timeout", + new java.sql.SQLException("lock timeout", "55P03") + ) + } + } + } + + it should "rethrow DataAccessExceptions whose cause carries no SQL state" in { + assertThrows[org.jooq.exception.DataAccessException] { + datasetResource.failOnDuplicateDatasetName { + throw new org.jooq.exception.DataAccessException( + "no sql state", + new java.sql.SQLException("constructed without a SQL state") + ) + } + } + } + + it should "let exceptions other than DataAccessException propagate unchanged" in { + assertThrows[IllegalStateException] { + datasetResource.failOnDuplicateDatasetName { + throw new IllegalStateException("unrelated failure") + } + } + } + + it should "return the result of the operation when no exception is thrown" in { + datasetResource.failOnDuplicateDatasetName(42) shouldEqual 42 + } + // =========================================================================== // Multipart upload tests (merged in) // =========================================================================== diff --git a/frontend/src/app/dashboard/component/user/list-item/card-item/card-item.component.spec.ts b/frontend/src/app/dashboard/component/user/list-item/card-item/card-item.component.spec.ts index ff174e93bc..bc51a33198 100644 --- a/frontend/src/app/dashboard/component/user/list-item/card-item/card-item.component.spec.ts +++ b/frontend/src/app/dashboard/component/user/list-item/card-item/card-item.component.spec.ts @@ -81,7 +81,7 @@ describe("CardItemComponent", () => { setCoverFromFile: vi.fn(), clearCover: vi.fn().mockReturnValue(of(undefined)), }; - const datasetServiceSpy = { getDatasetCoverUrl: vi.fn() }; + const datasetServiceSpy = { getDatasetCoverUrl: vi.fn(), updateDatasetName: vi.fn() }; await TestBed.configureTestingModule({ imports: [CardItemComponent, HttpClientTestingModule, BrowserAnimationsModule, RouterTestingModule], @@ -127,6 +127,46 @@ describe("CardItemComponent", () => { expect(component.editingName).toBe(false); }); + it("should reject an invalid dataset name, revert to original, and exit editing", () => { + component.entry = makeDatasetEntry({ id: 5, name: "invalid name" }); + component.originalName = "original-name"; + component.editingName = true; + const notificationService = TestBed.inject(NotificationService); + const errorSpy = vi.spyOn(notificationService, "error"); + + component.confirmUpdateCustomName("invalid name"); + + expect(datasetService.updateDatasetName).not.toHaveBeenCalled(); + expect(errorSpy).toHaveBeenCalled(); + expect(component.entry.name).toBe("original-name"); + expect(component.editingName).toBe(false); + }); + + it("should call the dataset service for a valid dataset rename", () => { + component.entry = makeDatasetEntry({ id: 5, name: "new-valid-name" }); + component.originalName = "old-name"; + datasetService.updateDatasetName.mockReturnValue(of({} as any)); + + component.confirmUpdateCustomName("new-valid-name"); + + expect(datasetService.updateDatasetName).toHaveBeenCalledWith(5, "new-valid-name"); + }); + + it("should surface the error message and revert the name when a dataset rename fails", () => { + component.entry = makeDatasetEntry({ id: 5, name: "new-valid-name" }); + component.originalName = "old-name"; + component.editingName = true; + datasetService.updateDatasetName.mockReturnValue(throwError(() => new Error("boom"))); + const notificationService = TestBed.inject(NotificationService); + const errorSpy = vi.spyOn(notificationService, "error"); + + component.confirmUpdateCustomName("new-valid-name"); + + expect(errorSpy).toHaveBeenCalledWith("boom"); + expect(component.entry.name).toBe("old-name"); + expect(component.editingName).toBe(false); + }); + it("should update workflow description successfully", () => { component.entry = makeWorkflowEntry({ id: 1, description: "Old Description" }); workflowPersistService.updateWorkflowDescription.mockReturnValue(of({} as Response)); diff --git a/frontend/src/app/dashboard/component/user/list-item/card-item/card-item.component.ts b/frontend/src/app/dashboard/component/user/list-item/card-item/card-item.component.ts index 7048daec4c..c890bc4a25 100644 --- a/frontend/src/app/dashboard/component/user/list-item/card-item/card-item.component.ts +++ b/frontend/src/app/dashboard/component/user/list-item/card-item/card-item.component.ts @@ -52,8 +52,13 @@ import { ActionType, HubService } from "../../../../../hub/service/hub.service"; import { DownloadService } from "src/app/dashboard/service/user/download/download.service"; import { formatSize } from "src/app/common/util/size-formatter.util"; import { formatRelativeTime, formatCount } from "src/app/common/util/format.util"; -import { DatasetService, DEFAULT_DATASET_NAME } from "../../../../service/user/dataset/dataset.service"; +import { + DatasetService, + DEFAULT_DATASET_NAME, + validateDatasetName, +} from "../../../../service/user/dataset/dataset.service"; import { NotificationService } from "../../../../../common/service/notification/notification.service"; +import { extractErrorMessage } from "../../../../../common/util/error"; import { WorkflowCoverService } from "../../../../service/user/workflow-cover/workflow-cover.service"; import { HUB_DATASET_RESULT_DETAIL, @@ -370,8 +375,8 @@ export class CardItemComponent implements OnChanges { next: () => { this.entry[propertyName] = newValue; // Dynamic property assignment }, - error: () => { - this.notificationService.error("Update failed"); + error: (err: unknown) => { + this.notificationService.error(extractErrorMessage(err)); (this.entry as any)[propertyName] = originalValue ?? ""; // Fallback to original value this.setEditingState(propertyName, false); }, @@ -396,6 +401,16 @@ export class CardItemComponent implements OnChanges { } const newName = this.entry.type === "workflow" ? name || DEFAULT_WORKFLOW_NAME : name || DEFAULT_DATASET_NAME; + if (this.entry.type === "dataset") { + const nameError = validateDatasetName(newName); + if (nameError) { + this.notificationService.error(nameError); + this.entry.name = this.originalName; + this.editingName = false; + return; + } + } + if (this.entry.type === "workflow") { this.updateProperty( this.workflowPersistService.updateWorkflowName.bind(this.workflowPersistService), diff --git a/frontend/src/app/dashboard/component/user/list-item/list-item.component.spec.ts b/frontend/src/app/dashboard/component/user/list-item/list-item.component.spec.ts index 44bec7e07f..10b07b82ce 100644 --- a/frontend/src/app/dashboard/component/user/list-item/list-item.component.spec.ts +++ b/frontend/src/app/dashboard/component/user/list-item/list-item.component.spec.ts @@ -30,6 +30,8 @@ import { UserService } from "../../../../common/service/user/user.service"; import { commonTestProviders } from "../../../../common/testing/test-utils"; import type { Mocked } from "vitest"; import { DashboardEntry } from "src/app/dashboard/type/dashboard-entry"; +import { DatasetService } from "../../../service/user/dataset/dataset.service"; +import { NotificationService } from "../../../../common/service/notification/notification.service"; import { HUB_DATASET_RESULT_DETAIL, HUB_WORKFLOW_RESULT_DETAIL, @@ -42,14 +44,17 @@ describe("ListItemComponent", () => { let component: ListItemComponent; let fixture: ComponentFixture<ListItemComponent>; let workflowPersistService: Mocked<WorkflowPersistService>; + let datasetService: Mocked<DatasetService>; beforeEach(async () => { const workflowPersistServiceSpy = { updateWorkflowName: vi.fn(), updateWorkflowDescription: vi.fn() }; + const datasetServiceSpy = { updateDatasetName: vi.fn() }; await TestBed.configureTestingModule({ imports: [ListItemComponent, HttpClientTestingModule, BrowserAnimationsModule, RouterTestingModule], providers: [ { provide: WorkflowPersistService, useValue: workflowPersistServiceSpy }, + { provide: DatasetService, useValue: datasetServiceSpy }, { provide: UserService, useClass: StubUserService }, NzModalService, ...commonTestProviders, @@ -59,6 +64,7 @@ describe("ListItemComponent", () => { fixture = TestBed.createComponent(ListItemComponent); component = fixture.componentInstance; workflowPersistService = TestBed.inject(WorkflowPersistService) as unknown as Mocked<WorkflowPersistService>; + datasetService = TestBed.inject(DatasetService) as unknown as Mocked<DatasetService>; // initializeEntry() needs a fully-formed workflow entry to avoid throwing // when the template renders for the first time. Each test below overwrites // component.entry directly, which exercises confirm methods without going @@ -189,4 +195,56 @@ describe("ListItemComponent", () => { expect(component.entryLink).toEqual([HUB_DATASET_RESULT_DETAIL, "301"]); }); }); + + it("should reject an invalid dataset name, revert to original, and exit editing", () => { + component.entry = { + id: 5, + name: "has space", + type: "dataset", + } as unknown as DashboardEntry; + component.originalName = "original-name"; + component.editingName = true; + const notificationService = TestBed.inject(NotificationService); + const errorSpy = vi.spyOn(notificationService, "error"); + + component.confirmUpdateCustomName("has space"); + + expect(datasetService.updateDatasetName).not.toHaveBeenCalled(); + expect(errorSpy).toHaveBeenCalled(); + expect(component.entry.name).toBe("original-name"); + expect(component.editingName).toBe(false); + }); + + it("should call the dataset service for a valid dataset rename", () => { + component.entry = { + id: 5, + name: "new-valid-name", + type: "dataset", + } as unknown as DashboardEntry; + component.originalName = "old-name"; + datasetService.updateDatasetName.mockReturnValue(of({} as any)); + + component.confirmUpdateCustomName("new-valid-name"); + + expect(datasetService.updateDatasetName).toHaveBeenCalledWith(5, "new-valid-name"); + }); + + it("should surface the error message and revert the name when a dataset rename fails", () => { + component.entry = { + id: 5, + name: "new-valid-name", + type: "dataset", + } as unknown as DashboardEntry; + component.originalName = "old-name"; + component.editingName = true; + datasetService.updateDatasetName.mockReturnValue(throwError(() => new Error("boom"))); + const notificationService = TestBed.inject(NotificationService); + const errorSpy = vi.spyOn(notificationService, "error"); + + component.confirmUpdateCustomName("new-valid-name"); + + expect(errorSpy).toHaveBeenCalledWith("boom"); + expect(component.entry.name).toBe("old-name"); + expect(component.editingName).toBe(false); + }); }); diff --git a/frontend/src/app/dashboard/component/user/list-item/list-item.component.ts b/frontend/src/app/dashboard/component/user/list-item/list-item.component.ts index 5cca45ced4..5291b79b1d 100644 --- a/frontend/src/app/dashboard/component/user/list-item/list-item.component.ts +++ b/frontend/src/app/dashboard/component/user/list-item/list-item.component.ts @@ -43,8 +43,13 @@ import { ActionType, HubService } from "../../../../hub/service/hub.service"; import { DownloadService } from "src/app/dashboard/service/user/download/download.service"; import { formatSize } from "src/app/common/util/size-formatter.util"; import { formatCount, formatRelativeTime } from "src/app/common/util/format.util"; -import { DatasetService, DEFAULT_DATASET_NAME } from "../../../service/user/dataset/dataset.service"; +import { + DatasetService, + DEFAULT_DATASET_NAME, + validateDatasetName, +} from "../../../service/user/dataset/dataset.service"; import { NotificationService } from "../../../../common/service/notification/notification.service"; +import { extractErrorMessage } from "../../../../common/util/error"; import { HUB_DATASET_RESULT_DETAIL, HUB_WORKFLOW_RESULT_DETAIL, @@ -311,8 +316,8 @@ export class ListItemComponent implements OnChanges { this.renderMarkdownPreview(newValue); } }, - error: () => { - this.notificationService.error("Update failed"); + error: (err: unknown) => { + this.notificationService.error(extractErrorMessage(err)); (this.entry as any)[propertyName] = originalValue ?? ""; // Fallback to original value if (propertyName === "description") { this.renderMarkdownPreview(originalValue); @@ -336,6 +341,16 @@ export class ListItemComponent implements OnChanges { public confirmUpdateCustomName(name: string): void { const newName = this.entry.type === "workflow" ? name || DEFAULT_WORKFLOW_NAME : name || DEFAULT_DATASET_NAME; + if (this.entry.type === "dataset") { + const nameError = validateDatasetName(newName); + if (nameError) { + this.notificationService.error(nameError); + this.entry.name = this.originalName; + this.editingName = false; + return; + } + } + if (this.entry.type === "workflow") { this.updateProperty( this.workflowPersistService.updateWorkflowName.bind(this.workflowPersistService), diff --git a/frontend/src/app/dashboard/component/user/user-dataset/user-dataset-list-item/user-dataset-list-item.component.spec.ts b/frontend/src/app/dashboard/component/user/user-dataset/user-dataset-list-item/user-dataset-list-item.component.spec.ts index eba371fdb4..513d803b87 100644 --- a/frontend/src/app/dashboard/component/user/user-dataset/user-dataset-list-item/user-dataset-list-item.component.spec.ts +++ b/frontend/src/app/dashboard/component/user/user-dataset/user-dataset-list-item/user-dataset-list-item.component.spec.ts @@ -189,7 +189,37 @@ describe("UserDatasetListItemComponent", () => { component.confirmUpdateDatasetCustomName("renamed-dataset"); - expect(notificationService.error).toHaveBeenCalledExactlyOnceWith("Update dataset name failed"); + expect(notificationService.error).toHaveBeenCalledExactlyOnceWith("boom"); + expect(component.entry.dataset.name).toBe(originalName); + expect(component.editingName).toBe(false); + }); + + ["", "a/b", "has space", "名前"].forEach(invalidName => { + it(`rejects the invalid name '${invalidName}' without calling the service`, () => { + const originalName = component.entry.dataset.name; + component.editingName = true; + + component.confirmUpdateDatasetCustomName(invalidName); + + expect(datasetService.updateDatasetName).not.toHaveBeenCalled(); + expect(notificationService.error).toHaveBeenCalledExactlyOnceWith( + "Invalid dataset name: only letters, numbers, underscores, and hyphens are allowed (max 128 characters)" + ); + expect(component.entry.dataset.name).toBe(originalName); + expect(component.editingName).toBe(false); + }); + }); + + it("surfaces the backend error message when the rename is rejected", () => { + const originalName = component.entry.dataset.name; + component.editingName = true; + datasetService.updateDatasetName.mockReturnValue( + throwError(() => ({ error: { message: "Dataset with the same name already exists" } })) + ); + + component.confirmUpdateDatasetCustomName("renamed-dataset"); + + expect(notificationService.error).toHaveBeenCalledExactlyOnceWith("Dataset with the same name already exists"); expect(component.entry.dataset.name).toBe(originalName); expect(component.editingName).toBe(false); }); diff --git a/frontend/src/app/dashboard/component/user/user-dataset/user-dataset-list-item/user-dataset-list-item.component.ts b/frontend/src/app/dashboard/component/user/user-dataset/user-dataset-list-item/user-dataset-list-item.component.ts index 6846911823..e6e583c4f3 100644 --- a/frontend/src/app/dashboard/component/user/user-dataset/user-dataset-list-item/user-dataset-list-item.component.ts +++ b/frontend/src/app/dashboard/component/user/user-dataset/user-dataset-list-item/user-dataset-list-item.component.ts @@ -20,9 +20,10 @@ import { UntilDestroy, untilDestroyed } from "@ngneat/until-destroy"; import { Component, EventEmitter, Input, Output } from "@angular/core"; import { Dataset } from "../../../../../common/type/dataset"; -import { DatasetService } from "../../../../service/user/dataset/dataset.service"; +import { DatasetService, validateDatasetName } from "../../../../service/user/dataset/dataset.service"; import { ShareAccessComponent } from "../../share-access/share-access.component"; import { NotificationService } from "../../../../../common/service/notification/notification.service"; +import { extractErrorMessage } from "../../../../../common/util/error"; import { NzModalService } from "ng-zorro-antd/modal"; import { DashboardDataset } from "../../../../type/dashboard-dataset.interface"; import { USER_DATASET } from "../../../../../app-routing.constant"; @@ -119,6 +120,13 @@ export class UserDatasetListItemComponent { return; } + const nameError = validateDatasetName(name); + if (nameError) { + this.notificationService.error(nameError); + this.editingName = false; + return; + } + if (this.entry.dataset.did) this.datasetService .updateDatasetName(this.entry.dataset.did, name) @@ -128,8 +136,8 @@ export class UserDatasetListItemComponent { this.entry.dataset.name = name; this.editingName = false; }, - error: () => { - this.notificationService.error("Update dataset name failed"); + error: (err: unknown) => { + this.notificationService.error(extractErrorMessage(err)); this.editingName = false; }, }); diff --git a/frontend/src/app/dashboard/service/user/dataset/dataset.service.spec.ts b/frontend/src/app/dashboard/service/user/dataset/dataset.service.spec.ts index 229ac32c5e..c5ec10cd7c 100644 --- a/frontend/src/app/dashboard/service/user/dataset/dataset.service.spec.ts +++ b/frontend/src/app/dashboard/service/user/dataset/dataset.service.spec.ts @@ -21,7 +21,7 @@ import { TestBed } from "@angular/core/testing"; import { HttpClientTestingModule, HttpTestingController } from "@angular/common/http/testing"; import { firstValueFrom } from "rxjs"; -import { DATASET_BASE_URL, DatasetService } from "./dataset.service"; +import { DATASET_BASE_URL, DatasetService, validateDatasetName } from "./dataset.service"; import { AppSettings } from "../../../../common/app-setting"; import { commonTestProviders } from "../../../../common/testing/test-utils"; import { Dataset, DatasetVersion } from "../../../../common/type/dataset"; @@ -116,6 +116,44 @@ class FakeXMLHttpRequest { } } +describe("validateDatasetName", () => { + it("returns null for a valid name", () => { + expect(validateDatasetName("my-dataset_1")).toBeNull(); + }); + + it("returns null for a single valid character", () => { + expect(validateDatasetName("a")).toBeNull(); + }); + + it("returns null for a name exactly at the 128-character limit", () => { + expect(validateDatasetName("a".repeat(128))).toBeNull(); + }); + + it("returns an error for an empty string", () => { + expect(validateDatasetName("")).not.toBeNull(); + }); + + it("returns an error for names with spaces", () => { + expect(validateDatasetName("has space")).not.toBeNull(); + }); + + it("returns an error for names with dots", () => { + expect(validateDatasetName("dot.dot")).not.toBeNull(); + }); + + it("returns an error for names with slashes", () => { + expect(validateDatasetName("a/b")).not.toBeNull(); + }); + + it("returns an error for names with non-ASCII characters", () => { + expect(validateDatasetName("名前")).not.toBeNull(); + }); + + it("returns an error for names exceeding 128 characters", () => { + expect(validateDatasetName("a".repeat(129))).not.toBeNull(); + }); +}); + describe("DatasetService", () => { let service: DatasetService; let http: HttpTestingController; diff --git a/frontend/src/app/dashboard/service/user/dataset/dataset.service.ts b/frontend/src/app/dashboard/service/user/dataset/dataset.service.ts index 6ec6a25aba..38cb3f2fce 100644 --- a/frontend/src/app/dashboard/service/user/dataset/dataset.service.ts +++ b/frontend/src/app/dashboard/service/user/dataset/dataset.service.ts @@ -43,7 +43,18 @@ export const DATASET_DELETE_URL = DATASET_BASE_URL + "/delete"; export const DATASET_VERSION_BASE_URL = "version"; export const DATASET_VERSION_RETRIEVE_LIST_URL = DATASET_VERSION_BASE_URL + "/list"; export const DATASET_VERSION_LATEST_URL = DATASET_VERSION_BASE_URL + "/latest"; -export const DEFAULT_DATASET_NAME = "Untitled dataset"; +export const DEFAULT_DATASET_NAME = "Untitled-dataset"; + +export const DATASET_NAME_MAX_LENGTH = 128; +const DATASET_NAME_PATTERN = /^[A-Za-z0-9_-]+$/; + +export function validateDatasetName(name: string): string | null { + if (!DATASET_NAME_PATTERN.test(name) || name.length > DATASET_NAME_MAX_LENGTH) { + return "Invalid dataset name: only letters, numbers, underscores, and hyphens are allowed (max 128 characters)"; + } + return null; +} + export const DATASET_PUBLIC_VERSION_BASE_URL = "publicVersion"; export const DATASET_PUBLIC_VERSION_RETRIEVE_LIST_URL = DATASET_PUBLIC_VERSION_BASE_URL + "/list"; export const DATASET_GET_OWNERS_URL = DATASET_BASE_URL + "/user-dataset-owners"; diff --git a/sql/changelog.xml b/sql/changelog.xml index c9f4b0d191..469868f895 100644 --- a/sql/changelog.xml +++ b/sql/changelog.xml @@ -48,6 +48,11 @@ <sqlFile path="sql/updates/27.sql"/> </changeSet> + <!-- Enforce unique dataset names per owner --> + <changeSet id="28" author="kunwp1"> + <sqlFile path="sql/updates/28.sql"/> + </changeSet> + <!-- example changeSet <changeSet id="1" author="author"> <sqlFile path="sql/updates/1.sql"/> diff --git a/sql/texera_ddl.sql b/sql/texera_ddl.sql index 8202614932..8a1e8782cd 100644 --- a/sql/texera_ddl.sql +++ b/sql/texera_ddl.sql @@ -287,7 +287,8 @@ CREATE TABLE IF NOT EXISTS dataset description TEXT NOT NULL, creation_time TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP, cover_image varchar(255), - FOREIGN KEY (owner_uid) REFERENCES "user"(uid) ON DELETE CASCADE + FOREIGN KEY (owner_uid) REFERENCES "user"(uid) ON DELETE CASCADE, + UNIQUE (owner_uid, name) ); -- dataset_user_access diff --git a/sql/updates/28.sql b/sql/updates/28.sql new file mode 100644 index 0000000000..5fa0b9bc0e --- /dev/null +++ b/sql/updates/28.sql @@ -0,0 +1,75 @@ +/* + * 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. + */ + +\c texera_db + +SET search_path TO texera_db; + +BEGIN; + +-- Datasets are looked up by (owner, name) in the file service and the file +-- resolver, so this pair must be unique. Before adding the constraint, +-- deterministically rename any pre-existing duplicates (kept: the oldest did; +-- renamed: name suffixed with "-<did>", truncated to fit VARCHAR(128)). +-- Each rename is reported via RAISE NOTICE: this is a user-visible data +-- change, and workflows that reference a renamed dataset by path will resolve +-- to the surviving dataset afterward, so operators should review the notices +-- and notify the affected dataset owners. +DO $$ +DECLARE + rec RECORD; + renamed INT := 0; + iterations INT := 0; +BEGIN + LOOP + FOR rec IN + UPDATE dataset d + SET name = LEFT(d.name, 128 - LENGTH('-' || d.did::text)) || '-' || d.did::text + FROM ( + SELECT did, name AS old_name, + ROW_NUMBER() OVER (PARTITION BY owner_uid, name ORDER BY did) AS rn + FROM dataset + ) dups + WHERE d.did = dups.did AND dups.rn > 1 + RETURNING d.did, d.owner_uid, dups.old_name, d.name AS new_name + LOOP + renamed := renamed + 1; + RAISE NOTICE 'Renamed duplicate dataset did=% (owner_uid=%): "%" -> "%"', + rec.did, rec.owner_uid, rec.old_name, rec.new_name; + END LOOP; + + EXIT WHEN NOT EXISTS ( + SELECT 1 FROM dataset GROUP BY owner_uid, name HAVING COUNT(*) > 1 + ); + + iterations := iterations + 1; + IF iterations > 10 THEN + RAISE EXCEPTION 'Could not deduplicate dataset (owner_uid, name) pairs after 10 passes; resolve duplicates manually before re-running.'; + END IF; + END LOOP; + + IF renamed > 0 THEN + RAISE NOTICE 'Renamed % duplicate dataset name(s) in total; workflows referencing the old names now resolve to the surviving datasets.', renamed; + END IF; +END $$; + +ALTER TABLE dataset + ADD CONSTRAINT dataset_owner_uid_name_key UNIQUE (owner_uid, name); + +COMMIT;
