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 f2a9f1a2..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()); } diff --git a/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/skill/service/SkillPublishServiceReplaceTest.java b/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/skill/service/SkillPublishServiceReplaceTest.java deleted file mode 100644 index d7c5af9e..00000000 --- a/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/skill/service/SkillPublishServiceReplaceTest.java +++ /dev/null @@ -1,185 +0,0 @@ -package com.iflytek.skillhub.domain.skill.service; - -import com.fasterxml.jackson.databind.ObjectMapper; -import com.iflytek.skillhub.domain.namespace.NamespaceMemberRepository; -import com.iflytek.skillhub.domain.namespace.NamespaceRepository; -import com.iflytek.skillhub.domain.review.ReviewTaskRepository; -import com.iflytek.skillhub.domain.review.ReviewTaskStatus; -import com.iflytek.skillhub.domain.security.SecurityScanService; -import com.iflytek.skillhub.domain.shared.exception.DomainBadRequestException; -import com.iflytek.skillhub.domain.skill.Skill; -import com.iflytek.skillhub.domain.skill.SkillFileRepository; -import com.iflytek.skillhub.domain.skill.SkillRepository; -import com.iflytek.skillhub.domain.skill.SkillVersion; -import com.iflytek.skillhub.domain.skill.SkillVersionRepository; -import com.iflytek.skillhub.domain.skill.SkillVersionStatus; -import com.iflytek.skillhub.domain.skill.SkillVisibility; -import com.iflytek.skillhub.domain.skill.metadata.SkillMetadataParser; -import com.iflytek.skillhub.domain.skill.validation.PrePublishValidator; -import com.iflytek.skillhub.domain.skill.validation.SkillPackageValidator; -import com.iflytek.skillhub.storage.ObjectStorageService; -import org.junit.jupiter.api.Test; -import org.junit.jupiter.api.extension.ExtendWith; -import org.mockito.Mock; -import org.mockito.junit.jupiter.MockitoExtension; -import org.mockito.junit.jupiter.MockitoSettings; -import org.mockito.quality.Strictness; -import org.springframework.context.ApplicationEventPublisher; - -import java.lang.reflect.Field; -import java.lang.reflect.InvocationTargetException; -import java.lang.reflect.Method; -import java.time.Clock; -import java.util.List; - -import static org.assertj.core.api.Assertions.assertThat; -import static org.assertj.core.api.Assertions.assertThatThrownBy; -import static org.mockito.ArgumentMatchers.any; -import static org.mockito.Mockito.never; -import static org.mockito.Mockito.verify; -import static org.mockito.Mockito.when; - -/** - * Regression coverage for replacing a non-published version with the same version number. - */ -@ExtendWith(MockitoExtension.class) -@MockitoSettings(strictness = Strictness.LENIENT) -class SkillPublishServiceReplaceTest { - - @Mock - private NamespaceRepository namespaceRepository; - @Mock - private NamespaceMemberRepository namespaceMemberRepository; - @Mock - private SkillRepository skillRepository; - @Mock - private SkillVersionRepository skillVersionRepository; - @Mock - private SkillFileRepository skillFileRepository; - @Mock - private ObjectStorageService objectStorageService; - @Mock - private SkillPackageValidator skillPackageValidator; - @Mock - private SkillMetadataParser skillMetadataParser; - @Mock - private PrePublishValidator prePublishValidator; - @Mock - private ReviewTaskRepository reviewTaskRepository; - @Mock - private SecurityScanService securityScanService; - @Mock - private SkillStorageDeletionCompensationService compensationService; - @Mock - private ApplicationEventPublisher eventPublisher; - - private final ObjectMapper objectMapper = new ObjectMapper(); - - private SkillPublishService newService() { - return new SkillPublishService( - namespaceRepository, - namespaceMemberRepository, - skillRepository, - skillVersionRepository, - skillFileRepository, - objectStorageService, - skillPackageValidator, - skillMetadataParser, - prePublishValidator, - objectMapper, - reviewTaskRepository, - securityScanService, - compensationService, - eventPublisher, - Clock.systemUTC()); - } - - /** The id is database-generated, so tests set it directly. */ - private static void setId(Object target, Long id) { - try { - Field field = target.getClass().getDeclaredField("id"); - field.setAccessible(true); - field.set(target, id); - } catch (ReflectiveOperationException e) { - throw new IllegalStateException(e); - } - } - - /** Invokes the package-private replacement routine under test. */ - private void invokeDelete(SkillPublishService target, Skill skill, SkillVersion version) { - try { - Method method = SkillPublishService.class.getDeclaredMethod( - "deleteReplaceableVersionArtifacts", Skill.class, SkillVersion.class, String.class); - method.setAccessible(true); - method.invoke(target, skill, version, "ns"); - } catch (InvocationTargetException e) { - if (e.getCause() instanceof RuntimeException runtimeException) { - throw runtimeException; - } - throw new IllegalStateException(e.getCause()); - } catch (ReflectiveOperationException e) { - throw new IllegalStateException(e); - } - } - - private Skill skill(Long id, Long latestVersionId) { - Skill skill = new Skill(1L, "demo", "owner", SkillVisibility.PUBLIC); - setId(skill, id); - skill.setLatestVersionId(latestVersionId); - return skill; - } - - private SkillVersion version(Long id, SkillVersionStatus status) { - SkillVersion version = new SkillVersion(1L, "1.0.0", "owner"); - setId(version, id); - version.setStatus(status); - return version; - } - - /** - * A rejected version still owns a REJECTED review task. Deleting only the PENDING task - * left that row referencing the skill_version, so the delete hit a foreign key - * constraint and the re-upload surfaced as an HTTP 500. - */ - @Test - void replacingRejectedVersionRemovesReviewTasksOfEveryStatus() { - Skill skill = skill(1L, 10L); - SkillVersion rejected = version(10L, SkillVersionStatus.REJECTED); - when(skillFileRepository.findByVersionId(10L)).thenReturn(List.of()); - - SkillPublishService target = newService(); - invokeDelete(target, skill, rejected); - - verify(reviewTaskRepository).deleteBySkillVersionIdIn(List.of(10L)); - verify(reviewTaskRepository, never()) - .findBySkillVersionIdAndStatus(any(), any(ReviewTaskStatus.class)); - verify(skillVersionRepository).delete(rejected); - } - - @Test - void replacingLatestVersionClearsTheLatestPointerFirst() { - Skill skill = skill(1L, 10L); - SkillVersion pending = version(10L, SkillVersionStatus.PENDING_REVIEW); - when(skillFileRepository.findByVersionId(10L)).thenReturn(List.of()); - - SkillPublishService target = newService(); - invokeDelete(target, skill, pending); - - assertThat(skill.getLatestVersionId()).isNull(); - verify(skillRepository).save(skill); - verify(reviewTaskRepository).deleteBySkillVersionIdIn(List.of(10L)); - } - - @Test - void replacingPublishedVersionIsStillRejected() { - Skill skill = skill(1L, 10L); - SkillVersion published = version(10L, SkillVersionStatus.PUBLISHED); - - SkillPublishService target = newService(); - assertThatThrownBy(() -> invokeDelete(target, skill, published)) - .isInstanceOf(DomainBadRequestException.class); - - verify(reviewTaskRepository, never()).deleteBySkillVersionIdIn(any()); - verify(skillVersionRepository, never()).delete(any()); - } -} 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..46a1277a 100644 --- a/web/e2e/helpers/test-data-builder.ts +++ b/web/e2e/helpers/test-data-builder.ts @@ -41,6 +41,12 @@ interface ReviewTaskSummary { version: string } +interface SkillVersionSummary { + id: number + version: string + status: string +} + interface NamespaceCandidate { userId: string displayName: string @@ -488,6 +494,40 @@ export class E2eTestDataBuilder { throw new Error(`Timed out waiting for pending review ${namespaceSlug}/${skillSlug}@${version}`) } + async waitForVersionStatus( + namespaceSlug: string, + skillSlug: string, + version: string, + expectedStatus: string, + ): Promise { + for (let attempt = 0; attempt < 60; attempt += 1) { + try { + const page = await parseEnvelope<{ + items: SkillVersionSummary[] + }>( + 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) { + 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 +552,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..80a2704e --- /dev/null +++ b/web/e2e/rejected-version-republish.spec.ts @@ -0,0 +1,103 @@ +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' + +interface ReviewTaskSummary { + id: number +} + +interface ReviewTaskPage { + items: ReviewTaskSummary[] +} + +interface ApiEnvelope { + code: number + data: T +} + +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 rejectedReviewsResponse = await adminPage.request.get( + '/api/web/reviews?status=REJECTED&page=0&size=100&sortDirection=DESC', + ) + expect(rejectedReviewsResponse.ok()).toBe(true) + const rejectedReviews = await rejectedReviewsResponse.json() as ApiEnvelope + expect(rejectedReviews.code).toBe(0) + expect(rejectedReviews.data.items.some((item) => item.id === rejectedReviewId)).toBe(false) + } finally { + await adminBuilder.cleanup() + await adminContext.close() + await publisherBuilder.cleanup() + } + }) +})