diff --git a/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/service/SkillPublishService.java b/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/service/SkillPublishService.java index c204306a..610c8203 100644 --- a/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/service/SkillPublishService.java +++ b/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/service/SkillPublishService.java @@ -62,6 +62,12 @@ public class SkillPublishService { private static final DateTimeFormatter AUTO_VERSION_FORMATTER = DateTimeFormatter.ofPattern("yyyyMMdd.HHmmss").withZone(ZoneId.systemDefault()); + private static final Set REPLACEABLE_VERSION_STATUSES = Set.of( + SkillVersionStatus.DRAFT, + SkillVersionStatus.SCAN_FAILED, + SkillVersionStatus.UPLOADED, + SkillVersionStatus.REJECTED + ); private static final Logger log = LoggerFactory.getLogger(SkillPublishService.class); public record PublishResult( @@ -566,7 +572,7 @@ public class SkillPublishService { } private void deleteReplaceableVersionArtifacts(Skill skill, SkillVersion version, String namespaceSlug) { - if (version.getStatus() == SkillVersionStatus.PUBLISHED) { + if (!REPLACEABLE_VERSION_STATUSES.contains(version.getStatus())) { throw new DomainBadRequestException("error.skill.version.exists", version.getVersion()); } @@ -577,8 +583,10 @@ public class SkillPublishService { skillRepository.flush(); } - reviewTaskRepository.findBySkillVersionIdAndStatus(version.getId(), ReviewTaskStatus.PENDING) - .ifPresent(reviewTaskRepository::delete); + // Every review task referencing this version has to go, not just a PENDING one: + // a rejected version still owns a REJECTED task whose foreign key blocks the + // skill_version delete below, which surfaces to the caller as an HTTP 500. + reviewTaskRepository.deleteBySkillVersionIdIn(List.of(version.getId())); List files = skillFileRepository.findByVersionId(version.getId()); List storageKeys = new ArrayList<>(); diff --git a/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/skill/service/SkillPublishServiceTest.java b/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/skill/service/SkillPublishServiceTest.java index 73f118fd..a75f971f 100644 --- a/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/skill/service/SkillPublishServiceTest.java +++ b/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/skill/service/SkillPublishServiceTest.java @@ -260,7 +260,7 @@ class SkillPublishServiceTest { } @Test - void testPublishFromEntries_ShouldReplaceDraftVersionWithSameVersion() throws Exception { + void testPublishFromEntries_ShouldReplaceRejectedVersionWithSameVersion() throws Exception { String namespaceSlug = "test-ns"; String publisherId = "user-100"; String skillMdContent = "---\nname: test-skill\ndescription: Test\nversion: 1.0.0\n---\nBody"; @@ -275,9 +275,9 @@ class SkillPublishServiceTest { Skill skill = new Skill(1L, "test-skill", publisherId, SkillVisibility.PUBLIC); setId(skill, 1L); - SkillVersion draftVersion = new SkillVersion(1L, "1.0.0", publisherId); - draftVersion.setStatus(SkillVersionStatus.DRAFT); - setId(draftVersion, 8L); + SkillVersion rejectedVersion = new SkillVersion(1L, "1.0.0", publisherId); + rejectedVersion.setStatus(SkillVersionStatus.REJECTED); + setId(rejectedVersion, 8L); SkillFile oldFile = new SkillFile(8L, "SKILL.md", (long) skillMdContent.length(), "text/markdown", "abc", "skills/1/8/SKILL.md"); when(namespaceRepository.findBySlug(namespaceSlug)).thenReturn(Optional.of(namespace)); @@ -288,7 +288,7 @@ class SkillPublishServiceTest { when(skillRepository.findByNamespaceIdAndSlug(any(), eq("test-skill"))).thenReturn(List.of(skill)); when(skillRepository.findByNamespaceIdAndSlugAndOwnerId(any(), eq("test-skill"), eq(publisherId))).thenReturn(Optional.of(skill)); when(skillVersionRepository.findBySkillIdAndStatus(1L, SkillVersionStatus.PENDING_REVIEW)).thenReturn(List.of()); - when(skillVersionRepository.findBySkillIdAndVersion(1L, "1.0.0")).thenReturn(Optional.of(draftVersion)); + when(skillVersionRepository.findBySkillIdAndVersion(1L, "1.0.0")).thenReturn(Optional.of(rejectedVersion)); when(skillFileRepository.findByVersionId(8L)).thenReturn(List.of(oldFile)); when(skillVersionRepository.save(any(SkillVersion.class))).thenAnswer(invocation -> { SkillVersion saved = invocation.getArgument(0); @@ -309,10 +309,60 @@ class SkillPublishServiceTest { assertEquals("1.0.0", result.version().getVersion()); assertEquals(SkillVersionStatus.PENDING_REVIEW, result.version().getStatus()); + verify(reviewTaskRepository).deleteBySkillVersionIdIn(List.of(8L)); verify(skillFileRepository).deleteByVersionId(8L); - verify(skillVersionRepository).delete(draftVersion); + verify(skillVersionRepository).delete(rejectedVersion); verify(skillVersionRepository).flush(); verify(objectStorageService).deleteObjects(List.of("skills/1/8/SKILL.md", "packages/1/8/bundle.zip")); + + ArgumentCaptor reviewTaskCaptor = ArgumentCaptor.forClass(ReviewTask.class); + verify(reviewTaskRepository).save(reviewTaskCaptor.capture()); + assertEquals(result.version().getId(), reviewTaskCaptor.getValue().getSkillVersionId()); + assertEquals(publisherId, reviewTaskCaptor.getValue().getSubmittedBy()); + } + + @Test + void testPublishFromEntries_ShouldRejectReplacementOfYankedVersion() throws Exception { + String namespaceSlug = "test-ns"; + String publisherId = "user-100"; + String skillMdContent = "---\nname: test-skill\ndescription: Test\nversion: 1.0.0\n---\nBody"; + + PackageEntry skillMd = new PackageEntry("SKILL.md", skillMdContent.getBytes(), skillMdContent.length(), "text/markdown"); + List entries = List.of(skillMd); + + Namespace namespace = new Namespace(namespaceSlug, "Test NS", "user-1"); + setId(namespace, 1L); + NamespaceMember member = mock(NamespaceMember.class); + SkillMetadata metadata = new SkillMetadata("test-skill", "Test", "1.0.0", "Body", Map.of()); + + Skill skill = new Skill(1L, "test-skill", publisherId, SkillVisibility.PUBLIC); + setId(skill, 1L); + SkillVersion yankedVersion = new SkillVersion(1L, "1.0.0", publisherId); + yankedVersion.setStatus(SkillVersionStatus.YANKED); + setId(yankedVersion, 8L); + + when(namespaceRepository.findBySlug(namespaceSlug)).thenReturn(Optional.of(namespace)); + when(namespaceMemberRepository.findByNamespaceIdAndUserId(any(), eq(publisherId))).thenReturn(Optional.of(member)); + when(skillPackageValidator.validate(entries)).thenReturn(ValidationResult.pass()); + when(skillMetadataParser.parse(skillMdContent)).thenReturn(metadata); + when(prePublishValidator.validate(any())).thenReturn(ValidationResult.pass()); + when(skillRepository.findByNamespaceIdAndSlug(any(), eq("test-skill"))).thenReturn(List.of(skill)); + when(skillRepository.findByNamespaceIdAndSlugAndOwnerId(any(), eq("test-skill"), eq(publisherId))).thenReturn(Optional.of(skill)); + when(skillVersionRepository.findBySkillIdAndVersion(1L, "1.0.0")).thenReturn(Optional.of(yankedVersion)); + + DomainBadRequestException exception = assertThrows(DomainBadRequestException.class, () -> + service.publishFromEntries( + namespaceSlug, + entries, + publisherId, + SkillVisibility.PUBLIC, + Set.of() + )); + + assertEquals("error.skill.version.exists", exception.messageCode()); + verify(reviewTaskRepository, never()).deleteBySkillVersionIdIn(anyList()); + verify(skillVersionRepository, never()).delete(any()); + verify(skillFileRepository, never()).deleteByVersionId(any()); } @Test diff --git a/web/e2e/helpers/test-data-builder.ts b/web/e2e/helpers/test-data-builder.ts index f255ea4a..71cd9ebd 100644 --- a/web/e2e/helpers/test-data-builder.ts +++ b/web/e2e/helpers/test-data-builder.ts @@ -3,6 +3,7 @@ import { tmpdir } from 'node:os' import { execFileSync } from 'node:child_process' import path from 'node:path' import type { APIRequestContext, Page, TestInfo } from '@playwright/test' +import type { components } from '../../src/api/generated/schema' import { csrfHeaders } from './csrf' type CleanupTask = () => Promise @@ -32,14 +33,9 @@ export interface SeededReviewData { skill: SeededSkill } -interface ReviewTaskSummary { - id: number - namespace: string - skillSlug: string - status: string - submittedBy: string - version: string -} +type ReviewTaskResponse = components['schemas']['ReviewTaskResponse'] +type SkillVersionResponse = components['schemas']['SkillVersionResponse'] +type SkillVersionStatus = NonNullable interface NamespaceCandidate { userId: string @@ -463,19 +459,17 @@ export class E2eTestDataBuilder { async waitForPendingReview(namespaceSlug: string, skillSlug: string, version: string): Promise { for (let attempt = 0; attempt < 20; attempt += 1) { try { - const page = await parseEnvelope<{ - items: ReviewTaskSummary[] - }>( + const page = await parseEnvelope( await this.request.get('/api/web/reviews?status=PENDING&page=0&size=100&sortDirection=DESC'), ) - const matched = page.items.find((item) => + const matched = page.items?.find((item) => item.namespace === namespaceSlug && item.skillSlug === skillSlug && item.version === version && item.status === 'PENDING', ) - if (matched) { + if (matched?.id != null) { return matched.id } } catch { @@ -488,6 +482,38 @@ export class E2eTestDataBuilder { throw new Error(`Timed out waiting for pending review ${namespaceSlug}/${skillSlug}@${version}`) } + async waitForVersionStatus( + namespaceSlug: string, + skillSlug: string, + version: string, + expectedStatus: SkillVersionStatus, + ): Promise { + for (let attempt = 0; attempt < 60; attempt += 1) { + try { + const page = await parseEnvelope( + await this.request.get( + `/api/web/skills/${encodeURIComponent(namespaceSlug)}/${encodeURIComponent(skillSlug)}/versions?page=0&size=100`, + ), + ) + + const matched = page.items?.find((item) => + item.version === version && item.status === expectedStatus, + ) + if (matched?.id != null) { + return matched.id + } + } catch { + // Security scanning and version projection can complete asynchronously. + } + + await new Promise((resolve) => setTimeout(resolve, 1_000)) + } + + throw new Error( + `Timed out waiting for ${namespaceSlug}/${skillSlug}@${version} to reach ${expectedStatus}`, + ) + } + async approveReview(reviewTaskId: number, comment = 'Approved by Playwright E2E'): Promise { let lastError: unknown for (let attempt = 0; attempt < 60; attempt += 1) { @@ -512,6 +538,15 @@ export class E2eTestDataBuilder { throw lastError instanceof Error ? lastError : new Error('approveReview timed out') } + async rejectReview(reviewTaskId: number, comment = 'Rejected by Playwright E2E'): Promise { + await parseEnvelope( + await this.request.post(`/api/web/reviews/${reviewTaskId}/reject`, { + data: { comment }, + headers: await csrfHeaders(this.page), + }), + ) + } + async searchNamespaceMemberCandidates(slug: string, search: string): Promise { const query = new URLSearchParams({ search }) return parseEnvelope( diff --git a/web/e2e/rejected-version-republish.spec.ts b/web/e2e/rejected-version-republish.spec.ts new file mode 100644 index 00000000..cbbcada4 --- /dev/null +++ b/web/e2e/rejected-version-republish.spec.ts @@ -0,0 +1,85 @@ +import { expect, test } from '@playwright/test' +import { setEnglishLocale } from './helpers/auth-fixtures' +import { loginWithCredentials, registerSession } from './helpers/session' +import { E2eTestDataBuilder } from './helpers/test-data-builder' + +function getOptionalEnv(name: string): string | undefined { + const value = process.env[name]?.trim() + return value ? value : undefined +} + +function adminCredentials() { + return { + username: getOptionalEnv('E2E_ADMIN_USERNAME') ?? getOptionalEnv('BOOTSTRAP_ADMIN_USERNAME') ?? 'admin', + password: getOptionalEnv('E2E_ADMIN_PASSWORD') ?? getOptionalEnv('BOOTSTRAP_ADMIN_PASSWORD') ?? 'ChangeMe!2026', + } +} + +test.describe('Rejected version replacement (Real API)', () => { + test.describe.configure({ timeout: 150_000 }) + + test.beforeEach(async ({ page }, testInfo) => { + await setEnglishLocale(page) + await registerSession(page, testInfo) + }) + + test('re-publishes the same version after rejection', async ({ page, browser }, testInfo) => { + const publisherBuilder = new E2eTestDataBuilder(page, testInfo) + await publisherBuilder.init() + + const adminContext = await browser.newContext() + const adminPage = await adminContext.newPage() + const adminBuilder = new E2eTestDataBuilder(adminPage, testInfo) + await loginWithCredentials(adminPage, adminCredentials(), testInfo) + await adminBuilder.init() + + try { + const namespace = await publisherBuilder.ensureWritableNamespace() + const skillName = `replace-rejected-${Date.now().toString(36)}` + const firstPublish = await publisherBuilder.publishSkill(namespace.slug, { + name: skillName, + version: '1.0.0', + }) + const rejectedReviewId = await adminBuilder.waitForPendingReview( + namespace.slug, + firstPublish.slug, + firstPublish.version, + ) + await publisherBuilder.waitForVersionStatus( + namespace.slug, + firstPublish.slug, + firstPublish.version, + 'PENDING_REVIEW', + ) + await adminBuilder.rejectReview(rejectedReviewId) + + const replacement = await publisherBuilder.publishSkill(namespace.slug, { + name: skillName, + description: 'Replacement after review rejection', + version: '1.0.0', + }) + const replacementReviewId = await adminBuilder.waitForPendingReview( + namespace.slug, + replacement.slug, + replacement.version, + ) + await publisherBuilder.waitForVersionStatus( + namespace.slug, + replacement.slug, + replacement.version, + 'PENDING_REVIEW', + ) + + expect(replacement.skillId).toBe(firstPublish.skillId) + expect(replacement.version).toBe(firstPublish.version) + expect(replacementReviewId).not.toBe(rejectedReviewId) + + const replacedReviewResponse = await adminPage.request.get(`/api/web/reviews/${rejectedReviewId}`) + expect(replacedReviewResponse.status()).toBe(404) + } finally { + await adminBuilder.cleanup() + await adminContext.close() + await publisherBuilder.cleanup() + } + }) +})